Feature: page access groups with RBAC-based visibility control - #727
Feature: page access groups with RBAC-based visibility control#727Plattenspatz wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds access groups, access-group assignments for pages and roles, access checks for page loads, and UI/API updates to view and edit those assignments. It also adds an ChangesAccess Groups and Page Visibility
Sequence Diagram(s)sequenceDiagram
participant PageLoad as "src/routes/(kener)/[page_path]/+page.server.ts"
participant CheckPageAccess
participant DbImpl
PageLoad->>DbImpl: GetPageByPath(params.page_path)
DbImpl-->>PageLoad: page record
PageLoad->>CheckPageAccess: CheckPageAccess(page.id, loggedInUser)
CheckPageAccess->>DbImpl: load page and role access groups
DbImpl-->>CheckPageAccess: group IDs
CheckPageAccess-->>PageLoad: allow / login_required / denied
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Filename | Overview |
|---|---|
| migrations/20260508120000_add_access_groups.ts | Adds three new tables (access_groups, page_access_groups, role_access_groups), seeds system groups, back-fills existing pages with the public group, and assigns the admin role to the admin group. Uses hasTable guards for idempotency. Down migration drops all three tables cleanly. |
| src/lib/server/controllers/pagesController.ts | Adds access-group check logic (CheckPageAccess, GetAccessiblePages) and admin CRUD functions. The CreateAccessGroup guard only blocks the 'public' reserved ID, leaving 'admin' unprotected. CreatePage now always auto-assigns public before returning, but the REST API POST handler duplicates this logic. |
| src/routes/(manage)/manage/app/roles/+page.svelte | Adds Access Groups sheet, group management dialogs, and role-group assignment. Contains two bugs: groupToDelete is nulled before cleanup code reads it (deleted group stays in selection Set), and the 'no groups selected' label incorrectly states the role sees all pages when it actually sees only public ones. |
| src/routes/(kener)/+page.server.ts | Home page route now runs CheckPageAccess before loading dashboard data; correctly redirects to sign-in or returns 404 based on access result. |
| src/routes/(kener)/[page_path]/+page.server.ts | Dynamic page route now resolves the page first, then gates on CheckPageAccess; returns 404 for denied access to avoid leaking page existence. |
| src/routes/(api)/api/v4/pages/+server.ts | GET includes access_groups in page response; POST validates and applies provided access_groups, but duplicates the auto-public logic already handled inside CreatePage when no groups are specified. |
| src/routes/(api)/api/v4/access-groups/+server.ts | New endpoint for listing and creating access groups; protected by the global apiAuthHandle hook. Input validation is present for both handlers. |
| src/lib/server/db/repositories/pages.ts | Nine new repository methods for access group CRUD. Batch read (getAccessGroupsForPages) uses a single whereIn query. Writes use transactions. Implementation is clean and compatible with Knex's cross-DB abstractions. |
| src/routes/(manage)/manage/app/pages/[page_id]/+page.svelte | Access Groups card added with checkbox-style group selection and a separate save action. Fetches on mount; save correctly guards with !currentPage. The admin group is excluded from the page-assignment UI (intentional). |
| src/lib/server/api-server/pages/get.ts | Page switcher now calls GetAccessiblePages with the session user, filtering out pages the caller cannot see. Cookie-based session resolution is correctly guarded for anonymous requests. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Request hits page route] --> B{Page has 'public' group?}
B -- Yes --> C[✅ Allow — no login needed]
B -- No --> D{User logged in?}
D -- No --> E[🔀 Redirect to sign-in]
D -- Yes --> F{User role has 'admin' group?}
F -- Yes --> G[✅ Allow — admin sees everything]
F -- No --> H{Page has any access groups?}
H -- No --> I[❌ Denied — 404]
H -- Yes --> J{Role groups ∩ Page groups ≠ ∅?}
J -- Yes --> K[✅ Allow]
J -- No --> L[❌ Denied — 404]
style C fill:#22c55e,color:#fff
style G fill:#22c55e,color:#fff
style K fill:#22c55e,color:#fff
style E fill:#f59e0b,color:#fff
style I fill:#ef4444,color:#fff
style L fill:#ef4444,color:#fff
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Request hits page route] --> B{Page has 'public' group?}
B -- Yes --> C[✅ Allow — no login needed]
B -- No --> D{User logged in?}
D -- No --> E[🔀 Redirect to sign-in]
D -- Yes --> F{User role has 'admin' group?}
F -- Yes --> G[✅ Allow — admin sees everything]
F -- No --> H{Page has any access groups?}
H -- No --> I[❌ Denied — 404]
H -- Yes --> J{Role groups ∩ Page groups ≠ ∅?}
J -- Yes --> K[✅ Allow]
J -- No --> L[❌ Denied — 404]
style C fill:#22c55e,color:#fff
style G fill:#22c55e,color:#fff
style K fill:#22c55e,color:#fff
style E fill:#f59e0b,color:#fff
style I fill:#ef4444,color:#fff
style L fill:#ef4444,color:#fff
Comments Outside Diff (3)
-
src/routes/(manage)/manage/app/roles/+page.svelte, line 1303-1307 (link)Misleading "no groups" label — wrong access semantics
The tooltip says a role with no groups selected can "see all pages (including non-public ones)," but the actual logic in
CheckPageAccess/GetAccessiblePagesis the opposite: a role with no groups inrole_access_groupsfails both rule 3 (admin group check) and rules 5–6 (overlap check), so it can only see pages in thepublicgroup. An admin reading this message would believe leaving groups empty is equivalent to "allow all," configure it that way, then discover their users are silently locked out of all non-public pages. The label should say something like "No groups selected — this role can only see public pages." -
src/routes/(manage)/manage/app/roles/+page.svelte, line 1207-1227 (link)groupToDeleteis nulled before the cleanup code that reads itgroupToDeleteis set tonullon line 1213, but the conditional on line 1217 that removes the deleted group fromroleAccessGroupIdsthen readsgroupToDelete?.id || "". BecausegroupToDeleteis alreadynull, the optional chain returnsundefinedand the fallback""is used.has("")will always be false, so the group is never removed from the selected Set. The in-memory selection state becomes stale — the deleted group remains "checked" even after deletion, and will be re-submitted tosetRoleAccessGroupson the next save. -
src/routes/(api)/api/v4/pages/+server.ts, line 733-739 (link)Auto-public is applied twice when
access_groupsis omitted from aPOST /api/v4/pagesrequestCreatePage(inpagesController.ts) already callsdb.setPageAccessGroups(newPage.id, ["public"])whenautoPublicPagesis truthy, then thiselsebranch executes the identical logic a second time. The second call is a no-op (same result) so there is no data corruption, but it runs two DB transactions unnecessarily and duplicates the policy in two places. If the auto-public rule is ever changed inCreatePage, this copy will silently diverge. Consider removing theelsebranch here and trustingCreatePageto handle the default.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Merge upstream/main, resolve import conf..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| /** | ||
| * Create a new access group | ||
| */ | ||
| export async function CreateAccessGroup(data: { id: string; group_name: string; description?: string }) { |
There was a problem hiding this comment.
"public" is explicitly reserved — "admin" is not guarded
The function blocks creation of a group named "public" but not "admin". Both are system groups with special semantics. If the seeded "admin" row were absent (manual DB cleanup, failed migration), a caller could create a non-system "admin" group (is_system = 0), which is then deletable through the normal delete path, silently stripping admin-level page access from all roles. The guard should also reject data.id === "admin" with a matching error.
| <Sheet.Description> | ||
| {#if accessGroupsRole?.readonly === 1} | ||
| Select which page access groups this role can view. Leave empty to allow access to all pages. | ||
| {:else} | ||
| Select which page access groups this role can view. Leave empty to allow access to all pages. | ||
| {/if} | ||
| </Sheet.Description> |
There was a problem hiding this comment.
{#if} block are identical — the readonly check has no effect and the description is misleading (empty groups ≠ "access all pages"). Either remove the conditional or write distinct descriptions for read-only vs editable roles.
| <Sheet.Description> | |
| {#if accessGroupsRole?.readonly === 1} | |
| Select which page access groups this role can view. Leave empty to allow access to all pages. | |
| {:else} | |
| Select which page access groups this role can view. Leave empty to allow access to all pages. | |
| {/if} | |
| </Sheet.Description> | |
| <Sheet.Description> | |
| Select which page access groups this role can view. Roles without any group can only see public pages. | |
| </Sheet.Description> |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/routes/(api)/api/v4/pages/[page_path]/+server.ts (2)
32-45: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGuard access-group reads and writes with
VerifyAPIKey().The route now exposes page visibility groups and allows replacing them, but the handler does not perform the required API-key check. As per coding guidelines,
src/routes/(api)/**/*.ts: Use API authentication viaVerifyAPIKey()imported from$lib/server/controllers/apiController.Also applies to: 257-273
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(api)/api/v4/pages/[page_path]/+server.ts around lines 32 - 45, The page API handler is exposing and modifying access-group data without the required API authentication. Update the route in the `+server.ts` handler to call `VerifyAPIKey()` from `$lib/server/controllers/apiController` before reading or writing page access groups, and ensure both the access-group fetch and any replacement logic are gated behind that check.Source: Coding guidelines
236-273: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate
access_groupsbefore applying any PATCH mutations.Invalid group IDs are checked only after page fields and monitor mappings may already be updated, so a request can return
400while persisting partial changes. Validatebody.access_groupstype and IDs beforedb.updatePage()/ monitor replacement, then apply all writes atomically if possible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(api)/api/v4/pages/[page_path]/+server.ts around lines 236 - 273, Validate body.access_groups and its IDs before performing any PATCH writes in the page update handler, so invalid groups do not leave partial changes behind. In the +server route, move the access-group existence check ahead of db.updatePage(), db.deletePageMonitorsByPageId(), and addMonitorToPage(), and only call SetPageAccessGroups after all validation passes. If possible, keep the page update, monitor replacement, and access-group assignment in one atomic flow so the request either fully succeeds or fully fails.src/routes/(api)/api/v4/pages/+server.ts (2)
31-44: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGuard the new access-group API surface with
VerifyAPIKey().This route now returns page access-group assignments and accepts visibility assignments on creation, but the handler does not perform the required API-key check. As per coding guidelines,
src/routes/(api)/**/*.ts: Use API authentication viaVerifyAPIKey()imported from$lib/server/controllers/apiController.Also applies to: 179-203
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(api)/api/v4/pages/+server.ts around lines 31 - 44, The page API handler is exposing access-group data without the required API authentication. Update the `+server.ts` route to call `VerifyAPIKey()` from `$lib/server/controllers/apiController` before returning page details or accepting visibility/access-group assignments, and keep the check in the main handler path that builds the response with `GetPageAccessGroups`.Source: Coding guidelines
165-195: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate access groups before creating the page.
An invalid group ID returns
400afterdb.createPage()and monitor inserts have already run, leaving a page created from a failed request. Move access-group validation before any writes, or wrap page/monitor/group writes in one transaction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/`(api)/api/v4/pages/+server.ts around lines 165 - 195, The page creation flow in the page route currently validates access groups only after db.createPage() and monitor inserts have already run, so a bad group ID can leave partial writes behind. Move the access-group validation logic in the page handler before any database writes, or make the page creation, addMonitorToPage, and SetPageAccessGroups steps run in a single transaction so they all succeed or fail together.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/allPerms.ts`:
- Around line 227-228: The deleteAccessGroup permission mapping currently only
uses roles.write, which allows cascading changes to page_access_groups without
page-level authorization. Update the permission configuration in allPerms so
deleteAccessGroup also requires pages.write, or adjust the dispatcher to enforce
both roles.write and pages.write when handling this action. Use the
deleteAccessGroup mapping in allPerms as the place to locate and fix the check.
In `@src/lib/server/controllers/pagesController.ts`:
- Around line 402-418: Validate the incoming group_ids in SetPageAccessGroups
and SetRoleAccessGroups before calling db.setPageAccessGroups or
db.setRoleAccessGroups. De-duplicate the array, fetch the allowed IDs via
getAllAccessGroups(), and reject any unknown IDs so the controller never writes
invalid or duplicate assignments that can trigger FK or composite-key errors.
- Around line 37-42: The page creation flow in pagesController’s createPage path
is not atomic because db.createPage succeeds before db.setPageAccessGroups, so a
failure can leave a page created without its default visibility group. Fix this
by making both writes part of one transaction in the same controller flow, or by
adding compensation to delete the newly created page and rethrow if
setPageAccessGroups fails. Use the createPage, setPageAccessGroups, and
GetSiteDataByKey logic to locate the affected block.
- Around line 360-365: After normalizing the group ID in pagesController,
revalidate the resulting data.id before proceeding because values like "!!!" can
normalize to an empty string; also treat "admin" as a reserved ID alongside
"public" and reject both in the same validation path. Update the existing ID
normalization/validation logic around the data.id assignment so the create-group
flow fails fast for empty normalized IDs and reserved names.
In `@src/lib/server/db/repositories/pages.ts`:
- Around line 201-203: The getAllAccessGroups repository contract is missing the
is_system field, which forces unsafe casting in the controller and weakens the
DeleteAccessGroup guard. Update the return type in
PagesRepository.getAllAccessGroups to include is_system, and ensure the query
still returns that column from access_groups so downstream callers can rely on
the typed value without casting.
In `@src/routes/`(api)/api/v4/access-groups/+server.ts:
- Around line 13-56: The POST handler currently only validates the request body,
but the documented reserved-group rule is incomplete because CreateAccessGroup
only blocks public; add a centralized check in POST and/or inside
CreateAccessGroup to reject admin the same way as public before any persistence
occurs. Use the existing CreateAccessGroup contract and the access-groups POST
request flow to ensure both reserved IDs are denied consistently with a
BAD_REQUEST response.
- Around line 16-76: The POST and GET handlers in access-groups/+server.ts are
missing API-key protection, so both listing and creating groups can be reached
without authentication. Import VerifyAPIKey from
$lib/server/controllers/apiController and call it at the start of both
RequestHandler functions before any db access or CreateAccessGroup invocation,
returning the auth failure response if verification fails.
In `@src/routes/`(api)/api/v4/pages/+server.ts:
- Around line 179-201: The access_groups handling in the page creation flow
should reject malformed input instead of treating it like a missing field.
Update the logic around the access_groups validation in +server.ts so that when
body.access_groups is defined but not an array, it returns a 400 BAD_REQUEST
response rather than falling through to the auto-public branch; keep the
existing SetPageAccessGroups and autoPublicPages behavior only for valid arrays
or when access_groups is truly absent.
In `@src/routes/`(kener)/[page_path]/+page.server.ts:
- Around line 16-22: The access handling in the page loader should not redirect
anonymous users to signin for non-public paths, because that reveals whether the
page exists. Update the logic in the page access check flow around
CheckPageAccess so that "login_required" is treated like "denied" and returns
the same 404 "Page Not Found" response unless the request is coming from a
trusted login flow. Keep the redirect behavior only for explicit authenticated
login navigation, and make sure the response for inaccessible and nonexistent
paths is indistinguishable.
In `@src/routes/`(manage)/manage/api/+server.ts:
- Around line 433-438: Validate the manage action payload before calling the
RBAC replacement helpers, since `SetPageAccessGroups` and `SetRoleAccessGroups`
currently receive `data.page_id`, `data.roleId`, and `data.group_ids` directly.
Add checks in the `action` dispatch block in `+server.ts` to ensure the IDs are
present and well-formed, and verify every group in `data.group_ids` exists
before invoking those helpers. Keep the validation close to the
`setPageAccessGroups` and `setRoleAccessGroups` branches so bad requests are
rejected before any RBAC assignment changes are attempted.
In `@src/routes/`(manage)/manage/app/pages/[page_id]/+page.svelte:
- Around line 1065-1074: The row in the group access list is using nested
interactive controls because Checkbox.Root is placed inside Button, which can
break accessibility and keyboard behavior. Update the page access UI in the
+page.svelte section that renders each group row so the container is not a
Button wrapping the checkbox; instead make the row a non-interactive
label/container or let Checkbox.Root own the interaction via its own
onCheckedChange while preserving the existing toggleAccessGroup(group.id)
behavior and selected styling.
In `@src/routes/`(manage)/manage/app/roles/+page.svelte:
- Around line 976-1001: The empty-selection copy in the roles page is
inconsistent: the text in the access-groups section and the
`roleAccessGroupIds.size === 0` message describe opposite permissions. Update
the wording in the `+page.svelte` roles UI so both messages match the actual
backend behavior and the intended access model, using the `roleAccessGroupIds`
empty-state logic and the access-group description block as the references.
- Around line 980-987: The role access group row is nesting an interactive
Checkbox.Root inside a Button, which should be removed. Update the component in
+page.svelte so the group selection is driven by a label/container row or by
wiring Checkbox.Root directly with onCheckedChange, and keep the click target
and checkbox state aligned through toggleAccessGroup(group.id) and
roleAccessGroupIds.has(group.id) without wrapping one control inside the other.
- Around line 959-987: The access-group editor in +page.svelte still allows
changing and saving role page-access settings even when the role is readonly or
the user lacks assignment rights. Update the controls around the role access
sheet to use the same authorization guard already used elsewhere in this
component (for example, the readonly/assignment check that drives the
permissions sheet) so the toggle buttons and save action are disabled or hidden
for protected roles; make sure the logic is applied consistently in the
access-group row rendering and the save handler paths tied to the role access
modal.
- Around line 453-466: The cleanup in handleDeleteGroup is using groupToDelete
after it has already been cleared, so the deleted group ID is lost and
roleAccessGroupIds may not be updated correctly. Save the deleted group’s id in
a local variable before setting groupToDelete to null, then use that saved id
when checking and removing it from roleAccessGroupIds inside handleDeleteGroup.
---
Outside diff comments:
In `@src/routes/`(api)/api/v4/pages/[page_path]/+server.ts:
- Around line 32-45: The page API handler is exposing and modifying access-group
data without the required API authentication. Update the route in the
`+server.ts` handler to call `VerifyAPIKey()` from
`$lib/server/controllers/apiController` before reading or writing page access
groups, and ensure both the access-group fetch and any replacement logic are
gated behind that check.
- Around line 236-273: Validate body.access_groups and its IDs before performing
any PATCH writes in the page update handler, so invalid groups do not leave
partial changes behind. In the +server route, move the access-group existence
check ahead of db.updatePage(), db.deletePageMonitorsByPageId(), and
addMonitorToPage(), and only call SetPageAccessGroups after all validation
passes. If possible, keep the page update, monitor replacement, and access-group
assignment in one atomic flow so the request either fully succeeds or fully
fails.
In `@src/routes/`(api)/api/v4/pages/+server.ts:
- Around line 31-44: The page API handler is exposing access-group data without
the required API authentication. Update the `+server.ts` route to call
`VerifyAPIKey()` from `$lib/server/controllers/apiController` before returning
page details or accepting visibility/access-group assignments, and keep the
check in the main handler path that builds the response with
`GetPageAccessGroups`.
- Around line 165-195: The page creation flow in the page route currently
validates access groups only after db.createPage() and monitor inserts have
already run, so a bad group ID can leave partial writes behind. Move the
access-group validation logic in the page handler before any database writes, or
make the page creation, addMonitorToPage, and SetPageAccessGroups steps run in a
single transaction so they all succeed or fail together.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6d7fc8b8-ff65-4b86-8101-56b241a3e90c
📒 Files selected for processing (18)
migrations/20260508120000_add_access_groups.tssrc/lib/allPerms.tssrc/lib/server/api-server/pages/get.tssrc/lib/server/controllers/pagesController.tssrc/lib/server/controllers/siteDataKeys.tssrc/lib/server/db/dbimpl.tssrc/lib/server/db/repositories/pages.tssrc/lib/server/db/seedSiteData.tssrc/lib/types/api.tssrc/routes/(api)/api/v4/access-groups/+server.tssrc/routes/(api)/api/v4/pages/+server.tssrc/routes/(api)/api/v4/pages/[page_path]/+server.tssrc/routes/(kener)/+page.server.tssrc/routes/(kener)/[page_path]/+page.server.tssrc/routes/(manage)/manage/api/+server.tssrc/routes/(manage)/manage/app/pages/[page_id]/+page.sveltesrc/routes/(manage)/manage/app/roles/+page.sveltesrc/routes/(manage)/manage/app/site-configurations/+page.svelte
| createAccessGroup: "roles.write", | ||
| deleteAccessGroup: "roles.write", |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Require page-write permission when deleting access groups.
deleteAccessGroup can cascade-remove page_access_groups, so roles.write alone lets a role manager change page visibility. Add a pages.write check for this action, or require both roles.write and pages.write in the dispatcher.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/allPerms.ts` around lines 227 - 228, The deleteAccessGroup permission
mapping currently only uses roles.write, which allows cascading changes to
page_access_groups without page-level authorization. Update the permission
configuration in allPerms so deleteAccessGroup also requires pages.write, or
adjust the dispatcher to enforce both roles.write and pages.write when handling
this action. Use the deleteAccessGroup mapping in allPerms as the place to
locate and fix the check.
| const newPage = await db.createPage(data); | ||
|
|
||
| // Auto-assign "public" group to new pages (unless disabled in settings) | ||
| const autoPublic = await GetSiteDataByKey("autoPublicPages"); | ||
| if (autoPublic !== false && autoPublic !== "false") { | ||
| await db.setPageAccessGroups(newPage.id, ["public"]); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep page creation and default access assignment atomic.
If setPageAccessGroups fails after createPage, the request errors but the page remains created without its intended visibility group; retrying then hits the duplicate path check. Wrap both writes in one transaction or compensate by deleting the new page before rethrowing.
Localized compensation option
const newPage = await db.createPage(data);
- // Auto-assign "public" group to new pages (unless disabled in settings)
- const autoPublic = await GetSiteDataByKey("autoPublicPages");
- if (autoPublic !== false && autoPublic !== "false") {
- await db.setPageAccessGroups(newPage.id, ["public"]);
+ try {
+ // Auto-assign "public" group to new pages (unless disabled in settings)
+ const autoPublic = await GetSiteDataByKey("autoPublicPages");
+ if (autoPublic !== false && autoPublic !== "false") {
+ await db.setPageAccessGroups(newPage.id, ["public"]);
+ }
+ } catch (error) {
+ await db.deletePage(newPage.id);
+ throw error;
}
return newPage;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const newPage = await db.createPage(data); | |
| // Auto-assign "public" group to new pages (unless disabled in settings) | |
| const autoPublic = await GetSiteDataByKey("autoPublicPages"); | |
| if (autoPublic !== false && autoPublic !== "false") { | |
| await db.setPageAccessGroups(newPage.id, ["public"]); | |
| const newPage = await db.createPage(data); | |
| try { | |
| // Auto-assign "public" group to new pages (unless disabled in settings) | |
| const autoPublic = await GetSiteDataByKey("autoPublicPages"); | |
| if (autoPublic !== false && autoPublic !== "false") { | |
| await db.setPageAccessGroups(newPage.id, ["public"]); | |
| } | |
| } catch (error) { | |
| await db.deletePage(newPage.id); | |
| throw error; | |
| } | |
| return newPage; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/controllers/pagesController.ts` around lines 37 - 42, The page
creation flow in pagesController’s createPage path is not atomic because
db.createPage succeeds before db.setPageAccessGroups, so a failure can leave a
page created without its default visibility group. Fix this by making both
writes part of one transaction in the same controller flow, or by adding
compensation to delete the newly created page and rethrow if setPageAccessGroups
fails. Use the createPage, setPageAccessGroups, and GetSiteDataByKey logic to
locate the affected block.
| // Normalize ID: lowercase, hyphens, no special chars | ||
| data.id = data.id.toLowerCase().trim().replace(/\s+/g, "-").replace(/[^a-z0-9_-]/g, ""); | ||
|
|
||
| if (data.id === "public") { | ||
| throw new Error("Cannot create a group with the reserved ID 'public'"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Revalidate normalized IDs and reserve admin.
An ID like "!!!" normalizes to "" after the required-field check, and admin is also reserved by the access logic as a global override group. Reject both after normalization.
Suggested fix
// Normalize ID: lowercase, hyphens, no special chars
data.id = data.id.toLowerCase().trim().replace(/\s+/g, "-").replace(/[^a-z0-9_-]/g, "");
- if (data.id === "public") {
- throw new Error("Cannot create a group with the reserved ID 'public'");
+ if (!data.id) {
+ throw new Error("id must contain at least one URL-safe character");
+ }
+
+ if (data.id === PUBLIC_GROUP || data.id === ADMIN_GROUP) {
+ throw new Error(`Cannot create a group with the reserved ID '${data.id}'`);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/controllers/pagesController.ts` around lines 360 - 365, After
normalizing the group ID in pagesController, revalidate the resulting data.id
before proceeding because values like "!!!" can normalize to an empty string;
also treat "admin" as a reserved ID alongside "public" and reject both in the
same validation path. Update the existing ID normalization/validation logic
around the data.id assignment so the create-group flow fails fast for empty
normalized IDs and reserved names.
| export async function SetPageAccessGroups(page_id: number, group_ids: string[]) { | ||
| await db.setPageAccessGroups(page_id, group_ids); | ||
| return { success: true }; | ||
| } | ||
|
|
||
| /** | ||
| * Get access groups assigned to a role | ||
| */ | ||
| export async function GetRoleAccessGroups(role_id: string): Promise<string[]> { | ||
| return await db.getAccessGroupsForRole(role_id); | ||
| } | ||
|
|
||
| /** | ||
| * Set access groups for a role (replaces all existing assignments) | ||
| */ | ||
| export async function SetRoleAccessGroups(role_id: string, group_ids: string[]) { | ||
| await db.setRoleAccessGroups(role_id, group_ids); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate assignment group IDs before writing.
The manage API passes group_ids directly here; invalid IDs become dialect-specific FK errors, and duplicates can violate the composite primary key. De-duplicate and verify IDs against getAllAccessGroups() before calling the repository.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/controllers/pagesController.ts` around lines 402 - 418,
Validate the incoming group_ids in SetPageAccessGroups and SetRoleAccessGroups
before calling db.setPageAccessGroups or db.setRoleAccessGroups. De-duplicate
the array, fetch the allowed IDs via getAllAccessGroups(), and reject any
unknown IDs so the controller never writes invalid or duplicate assignments that
can trigger FK or composite-key errors.
| async getAllAccessGroups(): Promise<Array<{ id: string; group_name: string; description: string | null }>> { | ||
| return await this.knex("access_groups").orderBy("id", "asc"); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Include is_system in the repository contract.
DeleteAccessGroup depends on is_system, but this method’s return type omits it, forcing an unsafe cast in the controller and making the system-group deletion guard fragile if this query is later narrowed.
Suggested adjustment
- async getAllAccessGroups(): Promise<Array<{ id: string; group_name: string; description: string | null }>> {
+ async getAllAccessGroups(): Promise<
+ Array<{ id: string; group_name: string; description: string | null; is_system: number }>
+ > {
return await this.knex("access_groups").orderBy("id", "asc");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async getAllAccessGroups(): Promise<Array<{ id: string; group_name: string; description: string | null }>> { | |
| return await this.knex("access_groups").orderBy("id", "asc"); | |
| } | |
| async getAllAccessGroups(): Promise< | |
| Array<{ id: string; group_name: string; description: string | null; is_system: number }> | |
| > { | |
| return await this.knex("access_groups").orderBy("id", "asc"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/server/db/repositories/pages.ts` around lines 201 - 203, The
getAllAccessGroups repository contract is missing the is_system field, which
forces unsafe casting in the controller and weakens the DeleteAccessGroup guard.
Update the return type in PagesRepository.getAllAccessGroups to include
is_system, and ensure the query still returns that column from access_groups so
downstream callers can rely on the typed value without casting.
| <Button | ||
| variant={pageAccessGroupIds.has(group.id) ? "outline" : "ghost"} | ||
| class="h-auto w-full justify-start gap-3 p-3 text-left {pageAccessGroupIds.has(group.id) | ||
| ? 'border-primary bg-primary/5' | ||
| : ''}" | ||
| onclick={() => toggleAccessGroup(group.id)} | ||
| > | ||
| <Checkbox.Root | ||
| checked={pageAccessGroupIds.has(group.id)} | ||
| /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid nesting the checkbox control inside a button.
Checkbox.Root is itself an interactive control, so placing it inside Button creates nested interactive elements and can break keyboard/screen-reader behavior. Make the row a label/container or handle the checkbox’s own onCheckedChange directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/app/pages/[page_id]/+page.svelte around lines
1065 - 1074, The row in the group access list is using nested interactive
controls because Checkbox.Root is placed inside Button, which can break
accessibility and keyboard behavior. Update the page access UI in the
+page.svelte section that renders each group row so the container is not a
Button wrapping the checkbox; instead make the row a non-interactive
label/container or let Checkbox.Root own the interaction via its own
onCheckedChange while preserving the existing toggleAccessGroup(group.id)
behavior and selected styling.
| async function handleDeleteGroup() { | ||
| if (!groupToDelete) return; | ||
| deletingGroup = true; | ||
| try { | ||
| await apiCall("deleteAccessGroup", { id: groupToDelete.id }); | ||
| toast.success("Access group deleted"); | ||
| showDeleteGroupDialog = false; | ||
| groupToDelete = null; | ||
| allAccessGroups = await apiCall("getAccessGroups"); | ||
| // Remove from current selection if it was selected | ||
| if (roleAccessGroupIds.has(groupToDelete?.id || "")) { | ||
| const next = new Set(roleAccessGroupIds); | ||
| next.delete(groupToDelete?.id || ""); | ||
| roleAccessGroupIds = next; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the deleted group before clearing groupToDelete.
Line 460 sets groupToDelete = null, so the cleanup at Lines 463-466 always checks/deletes "" and leaves the deleted group selected in roleAccessGroupIds. Save the ID first, then remove it from the current selection.
Proposed fix
async function handleDeleteGroup() {
if (!groupToDelete) return;
+ const deletedGroupId = groupToDelete.id;
deletingGroup = true;
try {
- await apiCall("deleteAccessGroup", { id: groupToDelete.id });
+ await apiCall("deleteAccessGroup", { id: deletedGroupId });
toast.success("Access group deleted");
showDeleteGroupDialog = false;
- groupToDelete = null;
allAccessGroups = await apiCall("getAccessGroups");
// Remove from current selection if it was selected
- if (roleAccessGroupIds.has(groupToDelete?.id || "")) {
+ if (roleAccessGroupIds.has(deletedGroupId)) {
const next = new Set(roleAccessGroupIds);
- next.delete(groupToDelete?.id || "");
+ next.delete(deletedGroupId);
roleAccessGroupIds = next;
}
+ groupToDelete = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function handleDeleteGroup() { | |
| if (!groupToDelete) return; | |
| deletingGroup = true; | |
| try { | |
| await apiCall("deleteAccessGroup", { id: groupToDelete.id }); | |
| toast.success("Access group deleted"); | |
| showDeleteGroupDialog = false; | |
| groupToDelete = null; | |
| allAccessGroups = await apiCall("getAccessGroups"); | |
| // Remove from current selection if it was selected | |
| if (roleAccessGroupIds.has(groupToDelete?.id || "")) { | |
| const next = new Set(roleAccessGroupIds); | |
| next.delete(groupToDelete?.id || ""); | |
| roleAccessGroupIds = next; | |
| async function handleDeleteGroup() { | |
| if (!groupToDelete) return; | |
| const deletedGroupId = groupToDelete.id; | |
| deletingGroup = true; | |
| try { | |
| await apiCall("deleteAccessGroup", { id: deletedGroupId }); | |
| toast.success("Access group deleted"); | |
| showDeleteGroupDialog = false; | |
| allAccessGroups = await apiCall("getAccessGroups"); | |
| // Remove from current selection if it was selected | |
| if (roleAccessGroupIds.has(deletedGroupId)) { | |
| const next = new Set(roleAccessGroupIds); | |
| next.delete(deletedGroupId); | |
| roleAccessGroupIds = next; | |
| } | |
| groupToDelete = null; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/app/roles/+page.svelte around lines 453 - 466,
The cleanup in handleDeleteGroup is using groupToDelete after it has already
been cleared, so the deleted group ID is lost and roleAccessGroupIds may not be
updated correctly. Save the deleted group’s id in a local variable before
setting groupToDelete to null, then use that saved id when checking and removing
it from roleAccessGroupIds inside handleDeleteGroup.
| {#if accessGroupsRole?.readonly === 1} | ||
| Select which page access groups this role can view. Leave empty to allow access to all pages. | ||
| {:else} | ||
| Select which page access groups this role can view. Leave empty to allow access to all pages. | ||
| {/if} | ||
| </Sheet.Description> | ||
| </Sheet.Header> | ||
| <div class="p-4"> | ||
| {#if loadingAccessGroups} | ||
| <div class="flex items-center justify-center p-8"> | ||
| <Spinner class="h-6 w-6" /> | ||
| </div> | ||
| {:else} | ||
| <div class="space-y-3"> | ||
| {#if allAccessGroups.length === 0} | ||
| <p class="text-muted-foreground text-sm">No access groups configured yet.</p> | ||
| {:else} | ||
| <p class="text-muted-foreground mb-3 text-xs"> | ||
| Roles without access groups can only see public pages. Select groups below to grant access to non-public pages. The admin role has a built-in "admin" group that grants access to all pages. | ||
| </p> | ||
| {#each allAccessGroups.filter((g) => g.id !== "public") as group (group.id)} | ||
| <Button | ||
| variant={roleAccessGroupIds.has(group.id) ? "outline" : "ghost"} | ||
| class="h-auto w-full justify-start gap-3 p-3 text-left {roleAccessGroupIds.has(group.id) | ||
| ? 'border-primary bg-primary/5' | ||
| : ''}" | ||
| onclick={() => toggleAccessGroup(group.id)} | ||
| > | ||
| <Checkbox.Root checked={roleAccessGroupIds.has(group.id)} /> |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Respect readonly roles and write permissions for page-access edits.
The permissions sheet disables readonly roles and users without assignment rights, but this sheet still allows toggling and saving access groups. Gate the row controls and save action with the same kind of authorization check to avoid mutating protected role access.
Also applies to: 1047-1054
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/app/roles/+page.svelte around lines 959 - 987,
The access-group editor in +page.svelte still allows changing and saving role
page-access settings even when the role is readonly or the user lacks assignment
rights. Update the controls around the role access sheet to use the same
authorization guard already used elsewhere in this component (for example, the
readonly/assignment check that drives the permissions sheet) so the toggle
buttons and save action are disabled or hidden for protected roles; make sure
the logic is applied consistently in the access-group row rendering and the save
handler paths tied to the role access modal.
| <p class="text-muted-foreground mb-3 text-xs"> | ||
| Roles without access groups can only see public pages. Select groups below to grant access to non-public pages. The admin role has a built-in "admin" group that grants access to all pages. | ||
| </p> | ||
| {#each allAccessGroups.filter((g) => g.id !== "public") as group (group.id)} | ||
| <Button | ||
| variant={roleAccessGroupIds.has(group.id) ? "outline" : "ghost"} | ||
| class="h-auto w-full justify-start gap-3 p-3 text-left {roleAccessGroupIds.has(group.id) | ||
| ? 'border-primary bg-primary/5' | ||
| : ''}" | ||
| onclick={() => toggleAccessGroup(group.id)} | ||
| > | ||
| <Checkbox.Root checked={roleAccessGroupIds.has(group.id)} /> | ||
| <div class="flex flex-col"> | ||
| <span class="text-sm font-medium">{group.group_name}</span> | ||
| {#if group.description} | ||
| <span class="text-muted-foreground text-xs">{group.description}</span> | ||
| {/if} | ||
| </div> | ||
| </Button> | ||
| {/each} | ||
| {/if} | ||
|
|
||
| {#if roleAccessGroupIds.size === 0} | ||
| <p class="text-muted-foreground text-xs"> | ||
| No groups selected — this role can see all pages (including non-public ones). | ||
| </p> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the empty-selection semantics consistent.
Lines 976-978 say roles without access groups can only see public pages, while Lines 999-1001 say they can see all pages. Align this copy with the backend behavior and PR objective before admins configure tenant access from contradictory guidance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/app/roles/+page.svelte around lines 976 - 1001,
The empty-selection copy in the roles page is inconsistent: the text in the
access-groups section and the `roleAccessGroupIds.size === 0` message describe
opposite permissions. Update the wording in the `+page.svelte` roles UI so both
messages match the actual backend behavior and the intended access model, using
the `roleAccessGroupIds` empty-state logic and the access-group description
block as the references.
| <Button | ||
| variant={roleAccessGroupIds.has(group.id) ? "outline" : "ghost"} | ||
| class="h-auto w-full justify-start gap-3 p-3 text-left {roleAccessGroupIds.has(group.id) | ||
| ? 'border-primary bg-primary/5' | ||
| : ''}" | ||
| onclick={() => toggleAccessGroup(group.id)} | ||
| > | ||
| <Checkbox.Root checked={roleAccessGroupIds.has(group.id)} /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid nesting the checkbox control inside a button.
As in the page editor, Checkbox.Root inside Button creates nested interactive controls. Use a label/container row or wire Checkbox.Root directly via onCheckedChange.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/`(manage)/manage/app/roles/+page.svelte around lines 980 - 987,
The role access group row is nesting an interactive Checkbox.Root inside a
Button, which should be removed. Update the component in +page.svelte so the
group selection is driven by a label/container row or by wiring Checkbox.Root
directly with onCheckedChange, and keep the click target and checkbox state
aligned through toggleAccessGroup(group.id) and roleAccessGroupIds.has(group.id)
without wrapping one control inside the other.
1 participant
Summary
This PR adds access groups to Kener — a flexible, RBAC-based mechanism to control who can see which status pages. It enables multi-tenant setups where different customers or teams only see their own pages.
Instead of a simple public/internal toggle, access groups provide a unified model: a built-in
publicgroup controls whether a page is publicly visible, while custom groups (e.g.customer-a,customer-b) restrict access to users whose role includes that group. This keeps things simple for basic setups (everything is public by default) while supporting complex scenarios when needed.Motivation
This was built for a multi-tenant use case: a single Kener instance serving status pages for multiple customers, where each customer should only see their own pages. Currently, all pages are visible to everyone who knows the URL.
Resolves #725.
Related to #714 and #697 — this PR takes a different approach by building on the RBAC system (v4.0.23) rather than adding a binary internal/public flag. The
publicaccess group replaces the need for a separatepage_is_internalfield, and the same mechanism handles both public visibility and per-user filtering.How It Works
Access groups on pages
Each page can have one or more access groups assigned. Two system groups exist by default:
public— pages with this group are visible to everyone without login (current default behavior)admin— roles with this group can see all pages regardless of their groupsCustom groups (e.g.
customer-a) are created by admins and assigned to pages and roles as needed.Access check logic
Example
customer-a-status,shared-infra, andpublic-statuscustomer-b-status,shared-infra, andpublic-statuspublic-statusWhat's Included
Database
access_groups,page_access_groups(page ↔ group),role_access_groups(role ↔ group)publicandadminare seeded automatically and cannot be deletedpublicgroup on migration (backward compatible — nothing changes for existing installations)adminrole gets theadmingroup on migrationBackend
(kener)/+page.server.tsand(kener)/[page_path]/+page.server.tsapi-server/pages/get.ts)autoPublicPages— when enabled (default), newly created pages automatically get thepublicgroupAdmin UI
REST API (
/api/v4/)GET /api/v4/pagesandGET /api/v4/pages/{path}— response includesaccess_groupsarrayPOST /api/v4/pages— accepts optionalaccess_groupsarray (falls back to auto-public setting when omitted)PATCH /api/v4/pages/{path}— acceptsaccess_groupsarray to replace current assignments (omitting it leaves groups unchanged)GET /api/v4/access-groups— list all access groupsPOST /api/v4/access-groups— create a new access groupBackward Compatibility
This PR is fully backward compatible:
publicgroup during migration, so they remain visible to everyoneadminrole automatically gets theadmingroup, giving it access to all pagesaccess_groupsis an additive field in API responsesFiles Changed
Total: ~1,130 lines added across 18 files.
Testing
Tested on two independent instances (fresh install + existing installation with data) covering:
Summary by CodeRabbit
New Features
Bug Fixes