feat: enforce automation ownership and execution authority - #1678
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe PR adds canonical automation ownership and permission-based authorization. Routes admit automation records before mutation. Schedulers authorize owners, manual requesters, and Slack collaborators, then propagate execution identity and record denied executions. ChangesAutomation authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to This PR adds automation ownership and execution authorization across routes, scheduling, persistence, and shared contracts. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Requester
participant AutomationRoutes
participant Scheduler
participant AuthorizationGuard
participant Session
Requester->>AutomationRoutes: trigger automation
AutomationRoutes->>Scheduler: trigger(automationId, requesterUserId, enrichment)
Scheduler->>AuthorizationGuard: isAutomationExecutionAuthorized(request)
AuthorizationGuard-->>Scheduler: authorized or denied
alt authorized
Scheduler->>Session: create session with requester identity
Scheduler->>Session: enqueue prompt
else denied
Scheduler-->>AutomationRoutes: AutomationExecutionUnauthorizedError
AutomationRoutes-->>Requester: 403 Execution authorization required
end
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
| : null); | ||
| if ( | ||
| !executionPrincipal || | ||
| !(await isAutomationExecutionAuthorized( |
There was a problem hiding this comment.
This authorizes against the automation tables before the immutable firing targets are selected. tick() passes repository/environment rows prefetched before this query, so a concurrent target removal can make the guard see a targetless automation and authorize a principal without repositories.use/environments.use, after which the prefetched target is still launched. The same mismatch affects steering, where current automation targets are checked instead of the existing run snapshot. Please derive the target requirements from the firing/run snapshot and authorize that snapshot.
There was a problem hiding this comment.
Fixed. Execution authorization now derives repository/environment requirements from the immutable firing snapshot, and that same snapshot is launched. Slack steering no longer evaluates current automation targets because it continues an existing session under the actor’s collaboration authority.
| // ─── Event handler ─────────────────────────────────────────────────────── | ||
|
|
||
| /** Match an inbound event to automations and start or steer their invocations. */ | ||
| /** Match an inbound event to authorized automations and start or steer their invocations. */ |
There was a problem hiding this comment.
event() iterates multiple matching automations, but the new AutomationExecutionUnauthorizedError from startInvocation() is not caught per candidate. A single revoked owner aborts the loop before later authorized automations run; normalized event ingress returns 502, while direct automation/Sentry webhooks surface a server error and may be retried. Please catch authorization rejection inside the candidate loop, log/count it as skipped, and continue processing the remaining candidates.
There was a problem hiding this comment.
Fixed. Authorization denial is now a typed outcome handled per candidate. The denied candidate is logged and counted as skipped, while later matching automations continue. Added multi-candidate regression coverage.
| if (e instanceof TargetSelectionError) return error(e.message, 400); | ||
| throw e; | ||
| } | ||
| if (ctx.principal?.kind === "user") { |
There was a problem hiding this comment.
The environment lookup occurs at line 597 before this permission check. A role with automations.create but without environments.use can distinguish an existing environment ID (403) from a missing one (400), and we perform avoidable DB work for a request that must be denied. Please check environments.use before resolving the supplied IDs.
There was a problem hiding this comment.
Fixed. environments.use is checked before resolving environment IDs, so unauthorized callers cannot distinguish existing from missing environments. Added a regression test verifying no lookup occurs.
| executionPrincipal.platformUserId | ||
| )) | ||
| ) { | ||
| throw new AutomationExecutionUnauthorizedError(); |
There was a problem hiding this comment.
For a scheduled firing this exception is caught by tick() as a generic failure, but next_run_at is never advanced and the automation is not paused. The row therefore remains at the front of the overdue query on every tick; once 25 revoked-owner rows accumulate, they fill MAX_PER_TICK and permanently starve every later schedule. Please handle this purpose-specific failure by pausing the automation or atomically recording/advancing the rejected slot.
There was a problem hiding this comment.
Fixed. A denied scheduled firing now atomically records a childless skipped invocation and conditionally pauses the matching overdue slot. Added repeated-tick integration coverage confirming it cannot remain overdue and starve later schedules.
| const selection = getRepositorySelection(body); | ||
| const environmentSelection = getEnvironmentSelection(body); | ||
| const requiredTargetPermissions: PermissionId[] = [ | ||
| ...(selection.kind === "replace" ? (["repositories.use"] as const) : []), |
There was a problem hiding this comment.
This requires target-use permission even when the replacement is empty. A member who loses repositories.use or environments.use cannot clear the target that now makes the automation unexecutable, although clearing uses no target and creation only requires these permissions for non-empty selections. Could we gate each permission on a non-empty replacement so owners can recover by removing inaccessible targets?
There was a problem hiding this comment.
Fixed. Target-use permission is now required only for non-empty replacements. Authorized owners can clear repositories or environments after losing the corresponding use grant.
There was a problem hiding this comment.
Summary
PR #1678, feat: enforce automation ownership and execution authority, by @ColeMurray adds ownership-scoped CRUD/trigger admission, canonical automation ownership, and execution-time permission revalidation. The overall direction is strong, but three scheduler behaviors can bypass target authorization or deny service to unrelated automations, so this is not ready to merge.
- Files changed: 17
- Additions/deletions: +1,047 / -159
- Validation: the only published PR check currently visible is CodeRabbit, which passes; focused local tests could not run because the workspace dependencies resolve against a different checkout.
Critical Issues
- [Security]
packages/control-plane/src/scheduler/scheduler.ts:368- Execution authorization checks current target tables rather than the immutable firing/run target snapshot. A target edit race can authorize a targetless automation and then launch a prefetched repository/environment; Slack steering can likewise validate current selections rather than the existing session target. Authorize explicit target requirements derived from the snapshot that will execute. - [Correctness/Availability]
packages/control-plane/src/scheduler/scheduler.ts:375- Unauthorized scheduled rows keep their original overduenext_run_at. Since the overdue query selects the oldest 25, revoked automations can permanently consume the tick limit and starve valid schedules. Pause them or atomically record/advance the rejected slot. - [Correctness/Availability]
packages/control-plane/src/scheduler/scheduler.ts:922- An unauthorized candidate throws out of the event loop, preventing later authorized candidates from running and causing webhook ingress errors/retries. Handle authorization rejection per automation and continue fan-out.
Suggestions
- [Authorization UX]
packages/control-plane/src/routes/automations.ts:918- Require target-use permissions only for non-empty replacements so an owner who lost access can clear inaccessible targets and restore executability. - [Information Disclosure]
packages/control-plane/src/routes/automations.ts:602- Checkenvironments.usebefore resolving environment IDs; otherwise unauthorized creators can distinguish valid IDs from invalid ones through 403 versus 400 responses.
Nitpicks
None.
Positive Feedback
- Manual triggers correctly execute and attribute sessions under the requester principal, including SCM enrichment.
- The D1-backed execution predicate fails closed for missing, suspended, and unassigned users and covers repository/environment permissions.
- The added tests provide useful own-versus-any route coverage, manual principal attribution, legacy owner repair, and Slack owner revocation cases.
Questions
None.
Verdict
Request Changes: address the target-snapshot authorization gap and scheduler starvation/fan-out failure modes before merging.
There was a problem hiding this comment.
Requesting changes. The authorization behavior is currently split across route admission, an isolated SQL read, and scheduler branches, and the denial path is modeled as an exception even though each source needs deterministic domain handling. That produces two operational failures: one revoked event automation aborts unrelated candidates, while revoked schedules remain permanently overdue and can starve the bounded tick window.
There is a code-judo move here: resolve canonical ownership and execution principals in one AutomationExecutionPolicy, make authorization part of invocation admission, and return a typed denied outcome. Event, schedule, manual, and Slack steering paths can then translate that outcome explicitly without embedding identity migration and source-specific policy in Scheduler. This matters because scheduler.ts was already 1,499 lines and grows to 1,579, while automations.ts grows from 1,436 to 1,521; adding another policy layer inside those orchestration files is making already oversized modules harder to reason about.
The focused suites pass after building shared: 192 unit tests and 42 integration tests. The missing cases are multi-candidate denial, repeated overdue ticks/starvation, route-level legacy-owner admission, and Slack actor authorization.
| const { automation, source } = params; | ||
| const { source } = params; | ||
| let automation = params.automation; | ||
| if (!automation.user_id && automation.created_by && automation.created_by !== "anonymous") { |
There was a problem hiding this comment.
[deep review] This is ownership migration in the invocation hot path, so it cannot establish the invariant for paths that authorize before invoking. requireAutomation("manage" | "trigger") compares nullable user_id before the scheduler runs, and Slack steering performs its owner check before reaching this repair. A resolvable legacy owner can therefore be denied management/manual trigger forever (especially for a paused automation), despite the direct Scheduler test passing. Complete the backfill once, or put canonical owner resolution plus compare-and-set in the automation ownership/store layer and use it before every admission path. Ownership should be a canonical invariant, not a side effect of starting a run.
There was a problem hiding this comment.
Fixed. Canonical owner repair now lives in AutomationStore.resolveCanonicalOwner() with compare-and-set behavior. Router ownership admission and scheduler execution both use it. Added route-level legacy-owner coverage.
| : null); | ||
| if ( | ||
| !executionPrincipal || | ||
| !(await isAutomationExecutionAuthorized( |
There was a problem hiding this comment.
[deep review] This authorization read is detached from both the guarded invocation insert and the privileged session launch. Repository/provider resolution and a Slack thread fetch can happen after it, so suspension or role revocation in that window still admits and launches sessions under stale authority. The existing store already centralizes atomic invocation admission; pass the authorization predicate into that operation and distinguish overlap from authorization denial, then revalidate immediately before external session initialization if launch-time revocation must also fail closed. A standalone boolean preflight is not an authoritative execution boundary.
There was a problem hiding this comment.
I see the concern about the detached authorization read. I addressed the target-snapshot mismatch, but intentionally did not move authorization into the guarded insert or revalidate before session initialization. Our accepted policy is request-start authorization semantics: permissions captured when a firing begins remain valid for that request even if changed while it is executing. startInvocation() now snapshots targets first and authorizes that exact snapshot; later resolution and launch use it. Future requests observe revocation, while overlap and idempotency remain atomically guarded.
| executionPrincipal.platformUserId | ||
| )) | ||
| ) { | ||
| throw new AutomationExecutionUnauthorizedError(); |
There was a problem hiding this comment.
[deep review] Authorization denial is an expected domain outcome, not an exception, and throwing here breaks both unattended callers. event() does not isolate this per candidate, so one revoked automation aborts the loop, prevents later matching automations from running, and turns normalized webhook delivery into a 502. tick() catches it but leaves next_run_at unchanged; the same automation remains oldest forever, and 25 revoked rows can starve every valid schedule behind the query limit. Return a typed unauthorized outcome. Event handling should log/count it and continue; scheduling should make a durable transition (for example atomically pause with an authorization reason, or record a terminal skip while advancing the slot) so deterministic denial cannot remain perpetually overdue. Add multi-candidate and repeated-tick tests.
There was a problem hiding this comment.
Fixed through the typed unauthorized outcome: events isolate and continue, while schedules durably record and pause the denied slot. Multi-candidate and repeated-tick regressions were added.
| if (steerable?.session_id) { | ||
| let ownerAuthorized: boolean; | ||
| try { | ||
| ownerAuthorized = await isAutomationExecutionAuthorized(this.db, automation.id, [ |
There was a problem hiding this comment.
[deep review] This checks sessions.collaborate on the automation owner, but steerSession() resolves the Slack actor and enqueues the turn under that actor's canonical identity. A suspended actor or actor without collaboration authority can therefore steer as long as the owner is authorized; conversely this generic execution guard also requires the owner to retain sessions.create and target-use grants even though no new session or target is created. This is the wrong policy abstraction for steering. Resolve the Slack actor once and authorize that principal for collaboration, with any separate owner/session-continuation invariant expressed explicitly.
There was a problem hiding this comment.
Fixed. Slack identity is resolved once per event, the canonical actor is checked only for sessions.collaborate, and that same actor ID is passed into steering. Automation launch grants are no longer imposed on an existing-session continuation.
| const selection = getRepositorySelection(body); | ||
| const environmentSelection = getEnvironmentSelection(body); | ||
| const requiredTargetPermissions: PermissionId[] = [ | ||
| ...(selection.kind === "replace" ? (["repositories.use"] as const) : []), |
There was a problem hiding this comment.
[deep review] Treating every replacement as target use means repositories: [] and environmentIds: [] require .use. Once a target-use grant is revoked, an otherwise authorized owner cannot remove the now-unusable target and repair the automation; the new test currently codifies that dead end. Creation already applies the cleaner resulting-state rule. Require .use only when the replacement contains targets, so removal remains a management operation rather than target execution authority.
There was a problem hiding this comment.
Fixed. Empty repository/environment replacements are management-only operations and no longer require the corresponding .use permission.
## 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 -->
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
Terraform Validation Results
Pushed by: @ColeMurray, Action: |
## Summary - add workspace member, role, and status administration endpoints - expose Next.js BFF routes and current-user authorization hooks - add permission-aware settings navigation and controls - add a workspace access administration settings surface ## 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 - RBAC route integration tests: 19 passed - web typecheck passed - focused affected web tests: 102 passed ## Related pull requests [#1677](#1677) -> [#1676](#1676) -> [#1674](#1674) -> [#1678](#1678) -> [#1675](#1675) -> [#1673](#1673)
## 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 -->
Summary
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
Related pull requests
#1677 -> #1676 -> #1674 -> #1678 -> #1675 -> #1673
Summary by CodeRabbit
New Features
Bug Fixes