Feat/workspaces and roles - #80
Conversation
|
venkateshsakamuri-lab
added
product: dashboards
|
What looks good Blockers / must-fix before merge |
|
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.
Addresses PR #80 review. BLOCKER — POST /saved-dashboards/{id}/favorite had no authorization at all. A non-member could toggle the favorite flag on a workspace-restricted dashboard and, worse than reported, read the entire row back from the 200 response — dashboardConfig and chatMessages included — bypassing the very 404 that hides it. Reproduced live before the fix (200, is_favorite f->t, full config in the body); now 404 with the row untouched and nothing leaked. An audit of every handler in the controller found this was the only one missing its gate: the list endpoints use filterReadable, and the rest already assert both connection access and workspace membership. Also fixes the review's follow-ups: - assertWorkspaceAssignable called getWorkspace(), which asserts only visibility, so a VIEWER could create dashboards into a workspace while moveDashboard() required MANAGER for the same effect. Added assertCanAssignInto() so both paths agree, plus a connection-match check. - MANAGE_DASHBOARD_WORKSPACES was declared in the Permission enum and offered in the role editor but enforced nowhere, so uticking it had no effect — worse than not offering the toggle. createWorkspace now asserts it. - getFolders ran a DISTINCT over every dashboard on the connection, leaking the folder names of workspace-restricted ones. It now derives folders from the readable set (verified: non-member sees ['hi'], admin sees ['SecretFolder','hi']). Note on how this was missed: the earlier QA exercised the favorite toggle only as admin, which passes regardless of the guard. Testing a permission check requires the role that should fail.
136a8c1 to
81f332c
Compare
|
Thanks — the IDOR was real and I confirmed it before fixing. Reproduced live: non-member One correction to the report: it's worse than an unauthorized write. The 200 response returns the whole row — Blocker — fixed ( I audited every handler in the controller for the same gap — this was the only one. The list endpoints use Follow-ups — all fixed
Rebased onto main — picked up #78 (MCP credential-leak) and #73 (IDE docs), no conflicts. 50/50 backend tests pass; two regression tests added for the IDOR and the VIEWER-create path. Frontend builds clean. On how this slipped through my own QA: I exercised the favorite toggle as admin, which passes regardless of the guard. Testing a permission check requires the role that should fail — my mistake, and the reason the audit above covers every handler rather than just the reported one. |
|
One more item I raised in my own QA, now closed out as won't fix (deliberate): Workspace-name existence oracle. A user with connection access but no membership sees Not fixing it, for a concrete reason: Decision: workspace names are treated as non-sensitive among users who already share a connection. Flagging it explicitly rather than leaving it undocumented — if workspace names ever carry sensitive meaning (client names, project codenames), this should be revisited by scoping uniqueness per creator. Unchanged from the previous comment: the IDOR blocker, all three follow-ups, and the rebase onto main are done and verified. |
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> ## Summary Weekly product cut **v1.3.0** — **DeepSQL Desktop (IDE) first ship** as the headline, plus Agent/Brain/Editor hardening landed on `main` since `v1.2.0`. ## Release contents - Bump `backend/pom.xml` → `1.3.0` - Bump `desktop/package.json` → `1.0.0` (installers via separate `desktop-v1.0.0` tag) - `CHANGELOG.md` + `docs/releases/RELEASE_NOTES-v1.3.0.md` - `docs/oss-ux/RELEASE.md` version table + Desktop tagging notes - Root `README.md` + `desktop/README.md` release pointers ## Included since v1.2.0 | PR | Topic | |----|--------| | #73 | **DeepSQL Desktop** — Electron thin client (TLS + SSH tunnel) | | #78 | Agent MCP cross-user credential leak closed | | #77 / #74 | Brain review approvals + schema-doc dedupe | | #75 | Enforceable Agent writes / non-blocking save bubbles | | #76 | Editor CSV export bound + concurrency/cancel audit | | #71 | View as Agent enforces target user policy | | #70 | Schema allowlist walks whole statement | | #72 | Brain endpoint connection authz | **Not included:** #80 (workspaces & custom roles) — still open; favorite-endpoint auth fix required first. ## After merge ```bash git checkout main && git pull git tag -a v1.3.0 -m "DeepSQL v1.3.0" git push origin v1.3.0 # optional Desktop installers: git tag -a desktop-v1.0.0 -m "DeepSQL Desktop v1.0.0" git push origin desktop-v1.0.0 ``` Tag push triggers `.github/workflows/release.yml` for product artifacts; `desktop-v*` triggers `.github/workflows/desktop-release.yml`. <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-8ce91e70-c67b-48c6-84b3-05bb9d06231a?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-8ce91e70-c67b-48c6-84b3-05bb9d06231a&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
![https://cursor.com/assets/images/open-in-web-dark.png"><source](https://cursor.com/assets/images/open-in-web-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-web-light.png"><img](https://cursor.com/assets/images/open-in-web-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a](https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"><source](https://cursor.com/assets/images/open-in-cursor-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-cursor-light.png"><img](https://cursor.com/assets/images/open-in-cursor-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div](https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div){kind=link}
2 participants
No description provided.