You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Commit dd092ad
Browse filesBrowse the repository at this point in the historyBrowse files
fix(access): require MANAGE_CONNECTIONS to create a connection; hide admin surfaces
POST /connections had no authorization at all — it went straight to
test-and-save, so any authenticated user could create, then edit and delete,
their own connection. Verified live: as DATA_ENGINEER the request returned 200
and the row persisted with owner_username = analyst. An earlier check reported
403 only because the payload was malformed, so validation rejected it before
authorization was ever reached; the endpoint was open.
Creation is not scoped to an existing connection id, so none of the
assertCanManage*Connection* helpers apply. assertCanManageConnections() is
permission-based rather than admin-only, so DBA — which holds MANAGE_CONNECTIONS
by design — keeps working, as does any custom role granted it. It honours
security.auth.enabled like every other guard here, so the dev-mode bypass stays
coherent.
UI: Settings and Connections are administrative surfaces and are hidden from
Developer and Data Engineer. Enforced inside SettingsModal and
ManageConnectionsModal, not only at the call sites — both are opened from several
places, and gating each entry point separately means the next new one silently
reopens the hole. Hiding Settings also removes MCP tokens from those roles, which
is the intended trade.
Nav gating moves from a minimum-role table to per-section permissions, so custom
roles and admin overrides take effect without a code change.
Copy file name to clipboardExpand all lines: CLAUDE.md
+93Lines changed: 93 additions & 0 deletions
Display the source diff
Display the rich diff
Original file line number
Diff line number
Diff line change
@@ -330,6 +330,99 @@ returns a number).
330
330
4.**Tooltips**: Always use `HelpTooltip` component, never plain `title` attributes.
331
331
5.**Design**: Minimal black/white/grey palette, Inter font, subtle transitions. See UX guidelines in full CLAUDE.md.
332
332
333
+
### Roles, Permissions & Custom Roles
334
+
335
+
Roles are **not a hierarchy**. The old model ranked `DEVELOPER < ADMIN` and compared
336
+
`ordinal()`; the shipped roles deliberately overlap without nesting, so an ordering
337
+
comparison has no meaning and `Role.isAtLeast` is gone.
338
+
339
+
| Role | Sections | Notes |
340
+
|---|---|---|
341
+
|`ADMIN`| everything | Fixed point: holds **every** permission; overrides against it are refused, so the last admin cannot be locked out of user management. |
342
+
|`DBA`| all menus + connection settings |**No** user creation / invite codes / role management. |
343
+
|`DATA_ENGINEER`| Agent, Dashboards, Editor | No Digest, no Performance. |
`VIEW_EDITOR`). The frontend gates nav on those codes (`SECTION_PERMISSION` in
351
+
`src/lib/features.js`), not on a minimum role.
352
+
-**A "role code" is either a built-in `Role` name or a `CustomRole.code`** — they share
353
+
the `users.role` namespace, so `CustomRoleService` refuses a code colliding with a
354
+
built-in one. `Role.fromString` returns **null** for anything unrecognised instead of
355
+
collapsing to DEVELOPER: mapping a custom role onto a built-in one would hand its
356
+
holders the wrong permissions. Use `PermissionService.getEffectivePermissions(roleCode)`
357
+
— `User.getRoleEnum()` is null for a custom role and `Role.getPermissions()` skips
358
+
overrides.
359
+
-**Every token-minting path must resolve by role code.**`AuthSessionService`,
360
+
`PasswordlessAuthService`, `AuthInternalController`, `CustomUserDetailsService` and the
361
+
`/auth/me` payload all use `user.getRoleCode()` + `PermissionService`; `JwtUtil` gained
362
+
a `String roleCode` overload for exactly this. A `Role`-typed path cannot represent a
363
+
custom role, so a custom-role user would silently get the wrong claim.
364
+
-**An unknown role code grants nothing** rather than falling back — a deleted custom role
365
+
must not become silent Developer access. Deleting a custom role is refused while any
366
+
user still holds it.
367
+
-`RolePermissionOverride.role` is now a role-code **string** (same column), so overrides
368
+
work for custom roles too. Built-in role permission sets are code, not data: the API
369
+
refuses to edit them directly and points at overrides instead, so an admin's change
370
+
survives an upgrade.
371
+
372
+
### Connection access levels & the create-connection guard
373
+
374
+
-**There is one access level.**`ConnectionAccessLevel.CHAT_EDITOR` is `@Deprecated` and
375
+
retained only so pre-existing rows parse; `fromString` folds it (and a blank value) into
376
+
`FULL_CONTENT`, and `ConnectionAccessService.resolveAccess` returns `FULL_CONTENT` for
377
+
**every** grant. Assigning a connection therefore implies content access — no migration
378
+
was needed, legacy rows upgrade themselves on read. The "Full Access" / "Chat + Editor"
379
+
badges are gone; only Owner/Admin are surfaced.
380
+
-**`AccessControlServiceTest` cannot prove anything about this.** It stubs
381
+
`resolveAccess` to return a fixed `EffectiveConnectionAccess`, so its CHAT_EDITOR case
382
+
passes vacuously no matter what the resolver does. `ConnectionAccessLevelCollapseTest`
383
+
exercises the real path — add coverage there, not to the stubbed test.
384
+
-**`POST /connections` had no authorization at all.** It went straight to test-and-save,
385
+
so any authenticated user could create — then edit and delete — their own connection
386
+
(verified live: the row persisted with `owner_username = analyst` for a DATA_ENGINEER).
387
+
Hiding the sidebar button is not a control. It now calls
388
+
`accessControlService.assertCanManageConnections()`, which is **permission-based, not
389
+
admin-only**, so DBA and any custom role holding `MANAGE_CONNECTIONS` still work.
390
+
Creation is not scoped to a connection id, so none of the `assertCanManage*Connection*`
391
+
helpers apply — a new unscoped endpoint needs this guard explicitly.
392
+
-**Settings and Connections are admin surfaces in the UI.**`SettingsModal` and
393
+
`ManageConnectionsModal` each refuse to render without the relevant permission, enforced
394
+
*inside* the component rather than only at the call site: both are opened from several
395
+
places, and gating each entry point separately means the next one silently reopens the
396
+
hole. Hiding Settings also removes MCP tokens from those roles — that is intended.
397
+
398
+
### Dashboard workspaces
399
+
400
+
`DashboardWorkspace` groups dashboards within one connection and carries its own member
401
+
list (`DashboardWorkspaceMember`, keyed by **username** to match `connection_access_grant`
402
+
so "View as" resolves membership as the target user).
403
+
404
+
-**The rule is an AND, and it only ever narrows.** Connection access is checked first and
405
+
unchanged (`assertCanReadConnectionContent`); workspace membership is an *additional*
406
+
gate. Adding someone to a workspace can never grant them a connection they were not
407
+
already given. `saved_dashboards.workspace_id` is nullable — NULL means "not grouped",
408
+
governed purely by the connection ACL exactly as before.
409
+
- Admins bypass the membership half, matching how they already bypass connection grants.
410
+
-**Non-membership reports 404, not 403** — a user outside the workspace must not learn
411
+
the dashboard exists.
412
+
-**Deleting a workspace detaches its dashboards, never deletes them** (the FK is
413
+
deliberately non-cascading). Removing the last MANAGER is refused, otherwise the
414
+
workspace could never be changed again by anyone but an admin.
415
+
-`DashboardWorkspaceService.filterReadable` resolves a whole list in one membership
416
+
query; use it for any new dashboard-list endpoint rather than checking per row.
417
+
-**`/saved-dashboards` had no connection authorization at all** before this change —
418
+
create, list, get, update and delete took a caller-supplied `connectionId`/id and
419
+
checked nothing, so any authenticated user could read every dashboard on every
420
+
connection (verified live against a running install, not inferred). All of them now
421
+
assert connection access *and* the workspace gate; `DashboardAlertController` does the
422
+
same through its single `requireDashboard` choke point. This is the same
423
+
"authentication is not authorization" trap `BrainController` documents — there is still
424
+
no filter doing it for you.
425
+
333
426
### Admin profile switch
334
427
Admins can **View as** a sub-user from the top-right of the home layout (`ProfileSwitch`) to verify connection ACLs, chat/editor policies, and role-gated nav.
0 commit comments