Feat/workspaces and roles - #80
Open
notSumit25 wants to merge 4 commits into
Open
Conversation
Replace the two-role hierarchy (DEVELOPER < ADMIN, compared by ordinal) with
permission sets. The shipped roles deliberately overlap without nesting —
Data Engineer has Dashboards but not Digest, Developer has Digest but no
connection settings — so "is role A at least role B" has no answer, and
Role.isAtLeast is gone.
ADMIN everything; a fixed point that ignores overrides, so no
configuration change can lock out the last administrator
DBA all menus + connection settings, but NOT user creation
DATA_ENGINEER Agent, Dashboards, Editor
DEVELOPER Agent, Digest, Dashboards, Performance, Editor
custom an admin-defined permission set (custom_roles)
One VIEW_* permission per sidebar section drives the nav, so the UI gates on
capabilities rather than a rank.
A "role code" is either a built-in Role name or a CustomRole.code — they share
the users.role namespace, so creation refuses a colliding code. Role.fromString
returns null for anything unrecognised instead of collapsing to DEVELOPER:
mapping a custom role onto a built-in one would hand its holders the wrong
permissions, and an unknown code must grant nothing rather than silent access.
Every token-minting path resolves by role code via PermissionService
(JwtUtil gained a String overload); a Role-typed path cannot represent a custom
role. AuthController gates its success branch on roleCode, not the nullable
role — the latter sent every custom-role login down the challenge branch and
NPE'd in Map.of, a 500 on every such sign-in.
RolePermissionConstraintInitializer drops the stale CHECK constraints Hibernate
generated under the old enums; ddl-auto=update never drops a constraint, so on
any existing database an override for DBA or a new section permission was
rejected outright.
Verified live against a running install, not inferred.
A DashboardWorkspace groups dashboards within one connection and carries its own member list, keyed by username to match connection_access_grant so "View as" resolves membership as the target user. The access rule is an AND, and it only ever narrows: connection access is checked first and unchanged, and workspace membership is an additional gate on top. Adding someone to a workspace can therefore never grant them a connection they were not already given. saved_dashboards.workspace_id is nullable — NULL means "not grouped", governed purely by the connection ACL exactly as before. Admins bypass the membership half, as they already bypass connection grants. Non-membership reports 404, not 403: a user outside the workspace must not learn the dashboard exists. Deleting a workspace detaches its dashboards rather than cascading — deleting a grouping must never destroy the things grouped — and removing the last MANAGER is refused so a workspace cannot be orphaned. Also closes a pre-existing authorization hole this feature sat on top of: /saved-dashboards create, list, get, update and delete took a caller-supplied connectionId or id and checked nothing, so any authenticated user could read every dashboard on every connection. Verified live before the fix by reading dashboards on a connection the user held no grant on. All of them now assert connection access and the workspace gate; DashboardAlertController does the same through its single requireDashboard choke point.
Assigning a connection now implies full content access. The old two-tier split (CHAT_EDITOR vs FULL_CONTENT) was a distinction users had to reason about for little benefit, and it silently hid the Dashboards section from anyone on the lower tier. CHAT_EDITOR is kept @deprecated purely so existing rows parse: fromString folds it — and a blank value — into FULL_CONTENT, and resolveAccess returns FULL_CONTENT for every grant. Legacy rows upgrade themselves on read, so no migration is required (verified: an untouched CHAT_EDITOR row now resolves with canManageContent=true). No grant still means NONE. The "Full Access" / "Chat + Editor" badges and the two-option selector are gone; assigning is one action, and only Owner/Admin badges remain since they mean something different. Note this widens access for anyone previously on the lower tier: they gain write access to that connection's Brain notes, schema docs, knowledge and dashboards. ConnectionAccessLevelCollapseTest covers the real resolution path. AccessControlServiceTest cannot: it stubs resolveAccess to return a fixed value, so its CHAT_EDITOR case passed identically before and after this change — a mock cannot catch a change to the thing it replaces. That test is annotated to say so rather than deleted, since it still guards the enum's own semantics.
…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.
notSumit25
requested review from
a team,
geekypunk and
venkateshsakamuri-lab
as code owners
August 23, 2026 10:26
Collaborator
Author
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.