refactor(auth): narrow workspace-scoped role deps to WorkspaceRole#2851
Open
daryllimyt wants to merge 1 commit into
Open
refactor(auth): narrow workspace-scoped role deps to WorkspaceRole#2851daryllimyt wants to merge 1 commit into
daryllimyt wants to merge 1 commit into
Conversation
|
✅ No security or compliance issues detected. Reviewed everything up to 5bf701b. Security Overview
Detected Code Changes| Change Type | Relevant files ... (code changes summary truncated to fit VCS comment limits.) |
RoleACL(require_workspace="yes") already rejects requests without a workspace before route handlers run, but Role.workspace_id was typed WorkspaceID | None, forcing every workspace-scoped route to repeat an unreachable 'if role.workspace_id is None: raise' guard for the type checker. Introduce WorkspaceRole (Role with non-optional workspace_id), produce it in _validate_role when a workspace is required, and annotate the workspace-scoped dependency aliases with it. Delete the now-redundant route-level guards. Service-layer checks at genuine trust boundaries (BaseWorkspaceService, Temporal/executor payloads) are unchanged.
07f2c62 to
5bf701b
Compare
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
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.
What
Introduces
WorkspaceRole, aRolesubtype with a non-optionalworkspace_id, and makes allRoleACL(require_workspace="yes")dependency aliases produce it. This deletes ~35 repeated route-level guards of the form:Why
_validate_rolealready raises 403 before any route body runs whenrequire_workspace="yes"and the workspace is missing, so these guards were unreachable at runtime — they existed only to narrowRole.workspace_id: WorkspaceID | Nonefor the type checker. Encoding the guarantee in the dependency's return type does the narrowing once instead of per-route.Do we lose the HTTP exceptions?
No. FastAPI resolves the
RoleACL(require_workspace="yes")dependency before the route body runs, and_validate_roleraises there if the workspace is missing:The executor-token path has the same 403 check. A request without workspace context still gets an HTTP 403 — it just comes from the dependency, exactly as it always did.
The deleted guards sat after that dependency in the same handlers, so they could only run when
workspace_idwas already guaranteed non-None. Their responses (e.g. the secrets route's 400 "Workspace context is required", the agent routes' 403 "Workspace access required") were never reachable, so there is no client-facing behavior change: the real response was always the dependency's403 Forbidden.How
tracecat/auth/types.py: newWorkspaceRole(Role)overridingworkspace_id: WorkspaceID(non-optional, frozen).tracecat/auth/credentials.py:_validate_rolere-validates intoWorkspaceRolewhenrequire_workspace == "yes"(after scope computation, soctx_rolealso carries the narrowed instance).tracecat/auth/dependencies.py: all workspace-scoped aliases (WorkspaceUserRole,WorkspaceActorRole,WorkspaceActorRouteRole,WorkspaceUserRouteRole,WorkspaceServiceAccountRole,WorkspaceUserPathRole,ExecutorWorkspaceRole) plus the inlineWorkspaceUserInPath/WorkspaceUserOnlyInPathaliases now annotateWorkspaceRole.integrations,workflow/{management,actions,executions,store},agent/channels,agent/session,secrets, and the EEtracecat_ee/agentrouters (approvals + agent). Inworkflow/executions/router.py, the two private search helpers now takeWorkspaceRoledirectly.oauth_callback(arequire_workspace="no"route) now binds the workspace from the validated OAuth state row to a local instead of re-narrowing the role.What is intentionally unchanged
BaseWorkspaceService.__init__'s runtime guard: services are also constructed from Temporal activities, the executor, andsystem_role(), where noRoleACLguarantee exists.workspace_id is Nonechecks in engine/runtime code (dsl/,executor/,agent/), which receive roles deserialized from payloads/headers — there, noRoleACLguarantee exists, so the runtime checks still matter.Since
WorkspaceRolesubclassesRole, every service signature takingrole: Roleis unaffected.Testing
uv run basedpyright --warnings: 0 errors.uv run ruff check/ruff format: clean.test_role_acl,test_auth_scope_resolution,test_role_headers,test_workspace_scoped_routes,test_api_actor_roles, plus the touched routers' tests): 88 passed.test_api_secrets,test_agent_session_router,test_approvals_delete): 46 passed.tests/unitsweep: identical failure set (222) on this branch and on cleanmainin my environment — zero regressions introduced.Summary by cubic
Introduce
WorkspaceRole(aRolewith a non-optionalworkspace_id) and update all workspace-scoped auth dependencies to return it. This removes ~30 unreachable guards and tightens typing with no runtime behavior changes.WorkspaceRole(Role)intracecat/auth/types.py._validate_rolenow returnsWorkspaceRolewhenrequire_workspace="yes"after computing scopes.WorkspaceRole(e.g.,WorkspaceUserRole,WorkspaceActorRole,WorkspaceActorRouteRole,WorkspaceUserRouteRole,WorkspaceServiceAccountRole,WorkspaceUserPathRole,ExecutorWorkspaceRole,WorkspaceUserInPath,WorkspaceUserOnlyInPath).workspace_id is Nonechecks acrossintegrations,workflow/*, andagent/channels;oauth_callbacknow uses a localworkspace_id.WorkspaceRolesubclassesRole, so existing service signatures remain compatible.Written for commit 07f2c62. Summary will update on new commits.