feat: add workspace access administration - #1675
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (66)
📝 WalkthroughWalkthroughThe change adds control-plane RBAC mutations, effective-authorization caching, permission-aware settings navigation, protected settings controls, workspace member administration, and read-only skill details. It also adds web API proxies and integration and component test coverage. ChangesRBAC and permission-aware settings
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WebClient
participant AuthorizationHook
participant WebApiRoute
participant ControlPlane
participant SettingsPanel
WebClient->>AuthorizationHook: Load current-user authorization
AuthorizationHook->>WebApiRoute: GET /api/me/authorization
WebApiRoute->>ControlPlane: GET /me/authorization
ControlPlane-->>AuthorizationHook: Permissions and suspension state
AuthorizationHook-->>SettingsPanel: hasPermission
SettingsPanel->>SettingsPanel: Filter or disable protected controls
SettingsPanel-->>WebClient: Render authorized settings UI
sequenceDiagram
participant WorkspaceSettings
participant UseWorkspaceAdministration
participant MembersApiRoute
participant ControlPlane
WorkspaceSettings->>UseWorkspaceAdministration: Update member role or status
UseWorkspaceAdministration->>MembersApiRoute: PUT member mutation
MembersApiRoute->>ControlPlane: PUT role or status
ControlPlane-->>MembersApiRoute: 204 or authorization error
MembersApiRoute-->>UseWorkspaceAdministration: Mutation result
UseWorkspaceAdministration-->>WorkspaceSettings: Update caches and mutation state
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![open-inspect[bot]](https://avatars.githubusercontent.com/in/2661131?s=60&v=4){kind=small}
There was a problem hiding this comment.
This implementation establishes the RBAC surface, but the client and route structure currently make committed mutations appear to fail and split authorization policy across the registry, shell, command menu, and individual panels. That split has already produced multiple guaranteed-403 controls and stale URL/navigation behavior. The cleaner direction is to make mutation success atomic, compute a single typed access model at the app/settings boundary, and have navigation and panels consume that model rather than reconstructing permissions independently.
Focused validation passed: shared build, web typecheck, 26 focused web tests, and diff check. No production file crosses the 1k-line threshold in this PR.
| actorUserId: ctx.principal.userId, | ||
| requestId: ctx.request_id, | ||
| }); | ||
| return json(await service.getEffectiveAuthorization(targetUserId)); |
There was a problem hiding this comment.
[deep review] This makes the response non-atomic with the mutation: replaceMemberRole can commit the role and audit event, then this separate read can fail and return 503. A retry is not idempotent and can write another audit event, so the caller cannot tell whether the update happened. The status route repeats the same problem. Please make the service return the response state as part of the successful operation, or return a success status without a fallible post-commit read.
There was a problem hiding this comment.
Fixed. Successful role/status mutations now return 204 immediately after the atomic service operation; the routes no longer perform a fallible post-commit authorization read, so callers cannot receive a retryable-looking failure after commit.
| body: JSON.stringify(body), | ||
| }); | ||
| if (!response.ok) throw new Error(`Member update failed (${response.status})`); | ||
| await Promise.all([ |
There was a problem hiding this comment.
[deep review] A successful write is defined as failed if any protected revalidation rejects. This breaks self-demotion and self-suspension in particular: the PUT commits, the actor loses access/session authorization, /members, /roles, or /me/authorization returns 403/401, and the UI reports Member update failed even though state changed. Self-suspension also leaves the auth-session cache claiming the user is authenticated. Consume the mutation result/update the relevant caches directly and treat follow-up refreshes as cache maintenance, not part of mutation success.
There was a problem hiding this comment.
Fixed. The hook now applies the successful mutation directly to the members, roles, and authorization caches without protected revalidation. Self-suspension also clears the auth-session cache, and cache maintenance cannot turn a committed write into a mutation error.
| <select | ||
| aria-label={`Role for ${member.displayName ?? member.userId}`} | ||
| value={member.role.id} | ||
| onChange={(event) => |
There was a problem hiding this comment.
[deep review] These controls remain enabled while an update is in flight, so two rapid role selections (and likewise repeated Suspend/Restore clicks) issue concurrent writes. Arrival/completion order can differ from interaction order, leaving the earlier choice as final server state and allowing stale revalidation to win. Track the pending member/action and disable or serialize that member's controls so the state transition has one owner.
There was a problem hiding this comment.
Fixed. Workspace Settings tracks pending member IDs and disables both role and status controls for that member until the request settles, preventing overlapping writes and stale completion order.
| description: "Review and restore archived sessions", | ||
| keywords: "archive restore retention", | ||
| icon: DataControlsIcon, | ||
| visibility: anyOf("sessions.read"), |
There was a problem hiding this comment.
[deep review] This exposes Data Controls to sessions.read, but the panel always renders an enabled Unarchive action whose route requires sessions.lifecycle. Every Viewer therefore gets a control guaranteed to fail with 403. This is also the concrete consequence of the registry modeling only category visibility while each panel reconstructs mutation policy independently. Please model named read/write capabilities once in the settings access descriptor/context and have both navigation and panels consume it, rather than continuing to scatter hasPermission branches.
There was a problem hiding this comment.
Fixed. Data Controls now declares a named unarchiveSessions capability in the settings registry. The panel consumes that descriptor: sessions.read retains the archived list, while Unarchive is rendered only with sessions.lifecycle. Added read-only coverage.
| }, | ||
| ] | ||
| : []), | ||
| ...APP_DESTINATIONS.map(({ label, description, href, icon: Icon }) => ({ |
There was a problem hiding this comment.
[deep review] The menu now claims to be permission-aware, but this unconditionally advertises Automations and Analytics to custom roles lacking automations.read or analytics.read; those destinations then fail at their protected APIs. Put the required permission on the canonical destination metadata and filter every navigation surface from that same definition. Gating only New session and settings creates another partial policy layer.
There was a problem hiding this comment.
Fixed. Automations and Analytics permissions now live on canonical app-destination metadata. Both the global command menu and session sidebar filter from that definition; New session is also hidden without sessions.create.
| supportsRepoImages(), | ||
| hasPermission | ||
| ); | ||
| const unauthorizedSubroute = |
There was a problem hiding this comment.
[deep review] Unauthorized canonicalization is special-cased to integration subroutes, so /settings?tab=secrets without secret access keeps that URL while the page silently renders Appearance. The test calls this a redirect but never checks navigation. Remove this mode-specific branch: whenever the requested category differs from the resolved category, replace the URL with the resolved category so URL, history, nav state, and content have one invariant.
There was a problem hiding this comment.
Fixed. SettingsShell now replaces the URL whenever a non-null requested category differs from the resolved authorized category, including query-string tabs. Added a stale-tab canonicalization test.
| description: "Review and restore archived sessions", | ||
| keywords: "archive restore retention", | ||
| icon: DataControlsIcon, | ||
| visibility: anyOf("sessions.read"), |
There was a problem hiding this comment.
This makes Data Controls visible with only sessions.read, but the panel still always renders an enabled Unarchive button. The unarchive route requires sessions.lifecycle, so a Viewer/custom read-only role gets a mutation control that necessarily returns 403. Please gate that control on sessions.lifecycle while retaining the read-only archived-session list, and add a read-only test.
There was a problem hiding this comment.
Fixed via the shared Data Controls capability descriptor: read access still shows archived sessions, while the Unarchive action requires sessions.lifecycle. Added read-only coverage.
| body: JSON.stringify(body), | ||
| }); | ||
| if (!response.ok) throw new Error(`Member update failed (${response.status})`); | ||
| await Promise.all([ |
There was a problem hiding this comment.
A successful self-demotion or self-suspension can reject here after the mutation has already committed. These three revalidations run concurrently against the new authorization state: member/role reads can now return 403, and self-suspension can invalidate the session entirely. Promise.all then makes WorkspaceSettings report "Member update failed" for a successful update, which can encourage retries and leave the UI in a misleading state. Please refresh authorization first and only revalidate resources still permitted, or otherwise keep cache-refresh failures from changing the mutation result.
There was a problem hiding this comment.
Fixed. Successful writes now update caches locally without protected revalidation; self-suspension clears the auth-session cache, and cache maintenance cannot convert a committed mutation into an error.
| keywords: "prebuild containers", | ||
| icon: BoxIcon, | ||
| requiresRepoImages: true, | ||
| visibility: anyOf("image_builds.read"), |
There was a problem hiding this comment.
The category can now be opened with only image_builds.read, but ImagesSettings unconditionally calls useRepos(), whose endpoint requires repositories.read, and builds the whole screen from that result. A valid custom role with only image-build read access therefore receives a 403 that the panel ignores and is told that no repositories exist. Please either require both permissions for this category or make the panel work explicitly without repository-read access.
There was a problem hiding this comment.
Fixed. Images visibility now requires image_builds.read and repositories.read together, matching the panel's repository dependency. Added registry coverage for the custom-role case.
| match: RegExpMatchArray, | ||
| ctx: UserRouteContext | ||
| ): Promise<Response> { | ||
| const targetUserId = decodeURIComponent(match.groups!.id); |
There was a problem hiding this comment.
decodeURIComponent runs before the handled try block, while the route pattern accepts malformed escapes such as %. Such a request throws URIError and reaches the generic router error path as a 500 instead of the intended 400 invalid-user response. Please decode inside the handled validation path (for both role and status handlers) or use a safe decoder, and cover malformed percent encoding in the route tests.
There was a problem hiding this comment.
Fixed. Role and status member routes now use safe path decoding and return 400 for malformed percent encoding. Added integration coverage for both endpoints.
| <div className="divide-y divide-border-muted rounded border border-border-muted"> | ||
| {skills.map((item) => ( | ||
| <div key={item.id} className="flex items-start gap-3 p-4"> | ||
| <button |
There was a problem hiding this comment.
This disables the only path that loads a skill’s full body, files, assignments, and revision metadata for users with skills.read but not skills.manage. The backend permits those users to read individual skills, so the new read-only catalog exposes only summary rows rather than the readable resource. Please keep rows openable and render the detail surface in a genuinely read-only mode, with mutation/reimport/save actions disabled.
There was a problem hiding this comment.
Fixed. Skill rows remain openable with skills.read. Read-only users get a non-mutating detail surface with body, metadata, assignments, files, import provenance, and revision metadata; create, import, toggle, delete, and editor actions remain management-only.
There was a problem hiding this comment.
Summary
PR #1675, feat: add workspace access administration, by @ColeMurray changes 61 files (+2,693 / -506) to add workspace-member administration APIs and permission-aware settings UI. The backend authorization checks and ownership invariants are well covered, but several client permission contracts and one mutation error path need correction before merge.
Critical Issues
- [Correctness]
packages/web/src/hooks/use-workspace-administration.ts:51- A successful self-demotion or self-suspension can be reported as failed because post-commit member/role/authorization revalidations reject under the newly reduced authorization state. - [Authorization UX]
packages/web/src/components/settings/settings-registry.ts:229- Data Controls is visible withsessions.read, while its enabled Unarchive action requiressessions.lifecycleand always fails for read-only roles. - [Functionality]
packages/web/src/components/settings/skills-settings/skills-catalog.tsx:148-skills.readusers cannot open the only detail surface for skill content, files, assignments, and revision metadata. - [Authorization UX]
packages/web/src/components/settings/settings-registry.ts:196- Images is visible with onlyimage_builds.read, but the panel requires a repository list protected byrepositories.read; custom read-only roles receive an ignored 403 and a misleading empty state.
Suggestions
- [Error Handling]
packages/control-plane/src/routes/rbac.ts:106- Handle malformed percent-encoded member IDs as 400 responses rather than allowingdecodeURIComponentto throw outside the route error boundary. The status handler has the same issue. - [Permission Awareness]
packages/web/src/components/global-command-menu.tsx:119- Consider filtering Automations and Analytics destinations byautomations.readandanalytics.read, respectively, just as this PR now filters settings and session-creation entries. - [Navigation]
packages/web/src/components/settings/settings-shell.tsx:29- Unauthorized query-string categories resolve to a fallback panel without replacing the staletabURL; redirecting whenever a valid requested category differs from the resolved category would keep browser history and copied links accurate.
Nitpicks
None.
Positive Feedback
- Server-side permission enforcement remains authoritative, including stale-actor checks and last-active-Owner protections.
- The RBAC integration suite covers denial paths, transaction rollback, audit behavior, and ownership invariants thoroughly.
- The settings registry centralizes category visibility and lazy panel loading cleanly.
Questions
None.
Validation
- Shared package build passed.
- Control-plane RBAC integration tests passed: 19/19.
- Control-plane and web typechecks passed.
- Focused settings proxy tests passed: 11/11.
- GitHub currently reports only the CodeRabbit check, which passed/skipped because reviews are disabled for this base branch.
Verdict
Request Changes: Please address the mutation-result bug and permission/UI mismatches above before merging.
## Summary - define shared RBAC roles, permissions, and authorization contracts - add D1-backed authorization persistence and service APIs - add the RBAC migration, workspace-owner bootstrap CLI, and migration/compatibility coverage - incorporate review hardening for canonical role identity, precise missing-resource outcomes, atomic user merging, suspension and attribution preservation, catalog fencing, and exact bootstrap provenance ## Stack This is **1 of 6** and targets `main`. Merge order: 1. `rbac-foundation` (this PR) 2. `rbac-http-enforcement` 3. `rbac-session-authorization` 4. `rbac-automation-authorization` 5. `rbac-workspace-settings` 6. `rbac-permission-aware-ui` ## Validation - control-plane unit tests: 3,352 passed - control-plane integration tests: 1,018 passed - workspace-owner bootstrap tests: 13 passed - user-merge CLI adapter tests: 2 passed - shared RBAC tests: 11 passed - repository ESLint, formatting, and typecheck passed ## Related pull requests [#1677](#1677) -> [#1676](#1676) -> [#1674](#1674) -> [#1678](#1678) -> [#1675](#1675) -> [#1673](#1673) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added role-based access control with Owner, Administrator, Member, and Viewer roles. * Added permission-aware member management, including role changes and account suspension controls. * Added audit tracking for authorization changes and user merges. * Added a guarded workflow for assigning the first workspace Owner. * **Bug Fixes** * Improved user-merge handling and atomicity across related records. * Prevented unauthorized ownership transfers through custom roles. * **Chores** * Added migration support for existing and new users under the RBAC model. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary - make authentication and authorization policy explicit for every HTTP route - enforce active-user and permission requirements at the router boundary - map service principals to bounded permissions and propagate bot actors consistently - expose read-only RBAC role, member, and current-user authorization endpoints ## Stack This is **2 of 6** and targets `rbac-foundation`. Merge order: `rbac-foundation` -> `rbac-http-enforcement` (this PR) -> `rbac-session-authorization` -> `rbac-automation-authorization` -> `rbac-workspace-settings` -> `rbac-permission-aware-ui`. ## Validation - control-plane unit tests: 3,377 passed - control-plane integration tests: 1,027 passed - Linear bot tests: 233 passed - Slack bot tests: 432 passed - repository typecheck passed ## Related pull requests [#1677](#1677) -> [#1676](#1676) -> [#1674](#1674) -> [#1678](#1678) -> [#1675](#1675) -> [#1673](#1673) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added role- and permission-based access controls across control-plane routes. * Added endpoints for viewing access, roles, and workspace members. * Added service-specific permission limits and automation authorization. * Added session-target authorization and actor attribution for Linear and Slack actions. * **Bug Fixes** * Suspended workspace access is now blocked. * Actorless or unidentified service requests now fail safely. * Conflicting actor identities return a clear retryable error. * Health checks remain available when authorization data is unavailable. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary - enforce canonical session access inside the Session durable object - add short-lived authorization leases to connected WebSockets - revoke stale sockets when a user's authorization expires or changes - tighten session repository visibility and lifecycle authorization coverage ## Stack This is **3 of 6** and targets `rbac-http-enforcement`. Merge order: `rbac-foundation` -> `rbac-http-enforcement` -> `rbac-session-authorization` (this PR) -> `rbac-automation-authorization` -> `rbac-workspace-settings` -> `rbac-permission-aware-ui`. ## Validation - shared tests: 791 passed - control-plane unit tests: 3,372 passed - control-plane integration tests: 1,029 passed - control-plane typecheck passed ## Related pull requests [#1677](#1677) -> [#1676](#1676) -> [#1674](#1674) -> [#1678](#1678) -> [#1675](#1675) -> [#1673](#1673) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - WebSocket connections now verify required session permissions and automatically expire when authorization changes. - The web app refreshes credentials and reconnects when authorization is revoked. - Temporary server errors trigger automatic reconnection using the existing credential. - WebSocket authorization state now persists across session runtime recovery. - **Changes** - Participant creation through the session API is no longer available. - Session lifecycle actions no longer require participant identity in request bodies. - WebSocket token requests now require a canonical user identity. - Updated session endpoint documentation to reflect current behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary - add automation admission and ownership authorization guards - enforce create, manage, trigger, scheduler, invocation, and webhook authority - persist canonical automation ownership and expose it in shared contracts - cover own-vs-any permission behavior across execution paths ## Stack This is **4 of 6** and targets `rbac-session-authorization`. Merge order: `rbac-foundation` -> `rbac-http-enforcement` -> `rbac-session-authorization` -> `rbac-automation-authorization` (this PR) -> `rbac-workspace-settings` -> `rbac-permission-aware-ui`. ## Validation - shared tests: 792 passed - control-plane unit tests: 3,380 passed - focused automation integration tests: 105 passed - control-plane typecheck passed ## Related pull requests [#1677](#1677) -> [#1676](#1676) -> [#1674](#1674) -> [#1678](#1678) -> [#1675](#1675) -> [#1673](#1673) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added ownership-aware automation authorization for viewing, managing, triggering, and executing automations. - Added permission checks when automations target repositories or environments. - Manual runs now execute under the requester’s identity. - Collaboration actions can be authorized independently from automation launch permissions. - **Bug Fixes** - Unauthorized executions are blocked and reported appropriately. - Scheduled automations are paused after authorization failures. - Legacy automation ownership is repaired automatically when possible. - Improved handling of missing, suspended, or deleted identities. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
# Conflicts: # packages/control-plane/src/auth/identity-enforcement.test.ts # packages/control-plane/src/auth/identity-enforcement.ts # packages/control-plane/src/authorization/service-permissions.test.ts # packages/control-plane/src/authorization/service-permissions.ts # packages/control-plane/src/automation/authorization-guard.test.ts # packages/control-plane/src/automation/authorization-guard.ts # packages/control-plane/src/router.create-session.test.ts # packages/control-plane/src/router.policy.test.ts # packages/control-plane/src/router.scm-credentials.test.ts # packages/control-plane/src/router.session-prompt.test.ts # packages/control-plane/src/router.spawn-child.test.ts # packages/control-plane/src/router.ts # packages/control-plane/src/routes/automations.test.ts # packages/control-plane/src/routes/automations.ts # packages/control-plane/src/routes/rbac.ts # packages/control-plane/src/routes/session-create.ts # packages/control-plane/src/routes/session-ws-token.ts # packages/control-plane/src/scheduler/scheduler.test.ts # packages/control-plane/src/scheduler/scheduler.ts # packages/control-plane/src/session/authorization-lease.ts # packages/control-plane/src/session/components.ts # packages/control-plane/src/session/connection-authenticator.ts # packages/control-plane/src/session/websocket-manager.test.ts # packages/control-plane/src/session/websocket-manager.ts # packages/control-plane/src/webhooks/github.ts # packages/control-plane/test/integration/automation-authorization.test.ts # packages/control-plane/test/integration/automation-invocations.test.ts # packages/control-plane/test/integration/scheduler-slack-events.test.ts # packages/control-plane/test/integration/service-auth.test.ts # packages/control-plane/test/integration/websocket-client.test.ts # packages/linear-bot/src/webhook-handler.test.ts # packages/linear-bot/src/webhook-handler.ts # packages/shared/src/rbac.ts
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
## Summary - gate session actions and controls using the current user's permissions - gate automation creation, management, and triggering with own-vs-any authority - surface authorization-aware empty, denied, and read-only states - document RBAC behavior, deployment, and design decisions ## Stack This is **6 of 6** and targets `rbac-workspace-settings`. Merge order: `rbac-foundation` -> `rbac-http-enforcement` -> `rbac-session-authorization` -> `rbac-automation-authorization` -> `rbac-workspace-settings` -> `rbac-permission-aware-ui` (this PR). ## Validation - repository typecheck passed - production web build passed - web suite: 1,382 passed; four resource-sensitive timeouts pass in isolation (77/77) - PR #1677 review follow-up: control-plane unit 3,352 passed; integration 1,018 passed; bootstrap 13 passed; user-merge CLI adapter 2 passed; repository ESLint and formatting passed ## Related pull requests [#1677](#1677) -> [#1676](#1676) -> [#1674](#1674) -> [#1678](#1678) -> [#1675](#1675) -> [#1673](#1673) ## Original parity and review delta The stack was originally created byte-identical to the original [#1662](#1662) head (`fa3464ad`, tree `ed4b0e47b1af3545b34c3d4842e9266a39c395c7`). It now intentionally differs only by the 16 review-fix files from PR #1677 (`4866d41a3` and `675a55449`), which have been propagated through every downstream branch. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added comprehensive authentication, authorization, workspace roles, and deployment guidance. - Added permission-based controls for session creation, collaboration, lifecycle actions, sandbox access, and automation management. - Added safer session behavior that hides sandbox links and data when access is unavailable. - Added automatic handling for revoked session permissions and unauthorized actions. - **Bug Fixes** - Restricted automation controls and session actions to authorized users. - Improved authorization behavior for session connections and commands. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 participant
Summary
Stack
This is 5 of 6 and targets
rbac-automation-authorization.Merge order:
rbac-foundation->rbac-http-enforcement->rbac-session-authorization->rbac-automation-authorization->rbac-workspace-settings(this PR) ->rbac-permission-aware-ui.Validation
Related pull requests
#1677 -> #1676 -> #1674 -> #1678 -> #1675 -> #1673
Summary by CodeRabbit
New Features
Bug Fixes