-
Notifications
You must be signed in to change notification settings - Fork 329
feat(rbac): Add @require_scope decorators to all API endpoints #1987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feat/rbac-api
Are you sure you want to change the base?
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
98f1d4b to
25fe8c2
Compare
471896f to
da15e3c
Compare
25fe8c2 to
ac3b77b
Compare
da15e3c to
2a32249
Compare
|
@cubic review |
|
@codex review |
@jordan-umusu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a322498cf
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 issues found across 25 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tracecat/workspaces/router.py">
<violation number="1" location="tracecat/workspaces/router.py:61">
P0: Missing import for `require_scope` decorator. The decorator is used throughout this file but is never imported, which will cause a `NameError` at module load time and prevent the router from loading.</violation>
</file>
<file name="tracecat/vcs/router.py">
<violation number="1" location="tracecat/vcs/router.py:86">
P1: The `@require_scope` decorator is added to a webhook endpoint that has no authentication dependency. Unlike other endpoints in this file that inject `role: OrgAdminUser`, the `github_webhook` function only receives a `payload` parameter. Webhooks are external callbacks from GitHub, not authenticated user requests, so `ctx_scopes` won't be populated and scope checks will fail. Consider removing the scope check from this webhook endpoint or implementing webhook-specific authentication (e.g., verifying the webhook secret).</violation>
</file>
<file name="tracecat/organization/router.py">
<violation number="1" location="tracecat/organization/router.py:401">
P2: This endpoint returns invitation tokens but lacks an @require_scope check. It now relies only on OrgAdminUser, so admins without the org invitation scope can still fetch tokens, bypassing the new RBAC scope enforcement. Add a scope requirement such as org:member:invite (or the appropriate scope) to align with the rest of the RBAC-protected endpoints.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
ac3b77b to
06ce0a2
Compare
2a32249 to
f0b1736
Compare
06ce0a2 to
ef01108
Compare
f0b1736 to
201de20
Compare
d73db0a to
a2665c6
Compare
201de20 to
4f9db51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 26 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 388a93572e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
|
|
||
| @router.get("/app", response_model=AppSettingsRead) | ||
| @require_scope("org:settings:read") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve org-member read access to app settings
get_app_settings still takes OrgUserRole (so it is intended for all org users), but adding @require_scope("org:settings:read") excludes the default organization-member role, which only carries org:read and org:member:read. This introduces a 403 for regular members on /settings/app, even though the endpoint was previously readable to them.
Useful? React with 👍 / 👎.
388a935 to
8ae79ee
Compare
a2665c6 to
217092b
Compare
217092b to
cb434f8
Compare
8ae79ee to
7436e7e
Compare
0d31ddf to
afbba18
Compare
afbba18 to
4b2a22b
Compare
cb434f8 to
06cfa32
Compare
|
@cubic re-review |
@jordan-umusu I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 65 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="tracecat/workflow/management/router.py">
<violation number="1" location="tracecat/workflow/management/router.py:335">
P1: Three case trigger endpoints (`create_case_trigger`, `get_case_trigger`, `update_case_trigger`) are missing `@require_scope` decorators, while every other endpoint in this file has one. This leaves an RBAC gap — users without the required workflow scopes can still create, read, and update case triggers. They should have `@require_scope("workflow:create")` (or `"workflow:update"` since it's an upsert), `@require_scope("workflow:read")`, and `@require_scope("workflow:update")` respectively.</violation>
</file>
<file name="tracecat/agent/preset/router.py">
<violation number="1" location="tracecat/agent/preset/router.py:49">
P2: Scope for the creation endpoint should likely be `"agent:create"` instead of `"agent:update"`. Other routers in the codebase consistently use `:create` scopes for POST creation endpoints (e.g., `schedule:create`, `workflow:create`). Using `agent:update` here means users with only create permissions won't be able to create presets, and users who should only update won't be prevented from creating new ones.</violation>
</file>
<file name="tracecat/agent/router.py">
<violation number="1" location="tracecat/agent/router.py:116">
P2: Scope `agent:execute` on a credential creation endpoint is inconsistent with the CRUD naming convention used elsewhere in the codebase (e.g., `table:create` for POST endpoints in `tables/router.py`). The other agent endpoints correctly use `agent:read`, `agent:update`, `agent:delete`. This should likely be `agent:create` to match the pattern, unless `execute` is intentionally a distinct permission — but that would be confusing since this endpoint creates credentials, not executes an agent.</violation>
</file>
<file name="tests/unit/test_executor_token_service_id.py">
<violation number="1" location="tests/unit/test_executor_token_service_id.py:54">
P3: These tests rely on monkeypatching internal behavior, but `tests/unit` is documented as integration-style with no mocks. Prefer configuring real settings/fixtures and exercising the real dependency instead of patching internals in this directory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -23,6 +23,7 @@ | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Three case trigger endpoints (create_case_trigger, get_case_trigger, update_case_trigger) are missing @require_scope decorators, while every other endpoint in this file has one. This leaves an RBAC gap — users without the required workflow scopes can still create, read, and update case triggers. They should have @require_scope("workflow:create") (or "workflow:update" since it's an upsert), @require_scope("workflow:read"), and @require_scope("workflow:update") respectively.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/workflow/management/router.py, line 335:
<comment>Three case trigger endpoints (`create_case_trigger`, `get_case_trigger`, `update_case_trigger`) are missing `@require_scope` decorators, while every other endpoint in this file has one. This leaves an RBAC gap — users without the required workflow scopes can still create, read, and update case triggers. They should have `@require_scope("workflow:create")` (or `"workflow:update"` since it's an upsert), `@require_scope("workflow:read")`, and `@require_scope("workflow:update")` respectively.</comment>
<file context>
@@ -327,6 +332,7 @@ async def get_workflow(
status_code=status.HTTP_204_NO_CONTENT,
tags=["workflows"],
)
+@require_scope("workflow:update")
async def update_workflow(
role: WorkspaceUserRole,
</file context>
| response_model=AgentPresetRead, | ||
| status_code=status.HTTP_201_CREATED, | ||
| ) | ||
| @require_scope("agent:update") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Scope for the creation endpoint should likely be "agent:create" instead of "agent:update". Other routers in the codebase consistently use :create scopes for POST creation endpoints (e.g., schedule:create, workflow:create). Using agent:update here means users with only create permissions won't be able to create presets, and users who should only update won't be prevented from creating new ones.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/agent/preset/router.py, line 49:
<comment>Scope for the creation endpoint should likely be `"agent:create"` instead of `"agent:update"`. Other routers in the codebase consistently use `:create` scopes for POST creation endpoints (e.g., `schedule:create`, `workflow:create`). Using `agent:update` here means users with only create permissions won't be able to create presets, and users who should only update won't be prevented from creating new ones.</comment>
<file context>
@@ -46,6 +46,7 @@ async def list_agent_presets(
response_model=AgentPresetRead,
status_code=status.HTTP_201_CREATED,
)
+@require_scope("agent:update")
async def create_agent_preset(
*,
</file context>
| @require_scope("agent:update") | |
| @require_scope("agent:create") |
|
|
||
|
|
||
| @router.post("/credentials", status_code=status.HTTP_201_CREATED) | ||
| @require_scope("agent:execute") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Scope agent:execute on a credential creation endpoint is inconsistent with the CRUD naming convention used elsewhere in the codebase (e.g., table:create for POST endpoints in tables/router.py). The other agent endpoints correctly use agent:read, agent:update, agent:delete. This should likely be agent:create to match the pattern, unless execute is intentionally a distinct permission — but that would be confusing since this endpoint creates credentials, not executes an agent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tracecat/agent/router.py, line 116:
<comment>Scope `agent:execute` on a credential creation endpoint is inconsistent with the CRUD naming convention used elsewhere in the codebase (e.g., `table:create` for POST endpoints in `tables/router.py`). The other agent endpoints correctly use `agent:read`, `agent:update`, `agent:delete`. This should likely be `agent:create` to match the pattern, unless `execute` is intentionally a distinct permission — but that would be confusing since this endpoint creates credentials, not executes an agent.</comment>
<file context>
@@ -109,6 +113,7 @@ async def get_provider_credential_config(
@router.post("/credentials", status_code=status.HTTP_201_CREATED)
+@require_scope("agent:execute")
async def create_provider_credentials(
*,
</file context>
| def test_mint_and_verify_executor_token_roundtrip_service_id( | ||
| monkeypatch: pytest.MonkeyPatch, | ||
| ) -> None: | ||
| monkeypatch.setattr(config, "TRACECAT__SERVICE_KEY", "test-service-key") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3: These tests rely on monkeypatching internal behavior, but tests/unit is documented as integration-style with no mocks. Prefer configuring real settings/fixtures and exercising the real dependency instead of patching internals in this directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/unit/test_executor_token_service_id.py, line 54:
<comment>These tests rely on monkeypatching internal behavior, but `tests/unit` is documented as integration-style with no mocks. Prefer configuring real settings/fixtures and exercising the real dependency instead of patching internals in this directory.</comment>
<file context>
@@ -0,0 +1,145 @@
+def test_mint_and_verify_executor_token_roundtrip_service_id(
+ monkeypatch: pytest.MonkeyPatch,
+) -> None:
+ monkeypatch.setattr(config, "TRACECAT__SERVICE_KEY", "test-service-key")
+
+ token = mint_executor_token(
</file context>

Summary by cubic
Enforced RBAC across the platform by adding @require_scope to API endpoints and introducing action-level checks to prevent unauthorized action execution. Added new scopes for tags, variables, and org settings, and updated seeding and tests.
New Features
Migration
Written for commit da15e3c. Summary will update on new commits.