-
Notifications
You must be signed in to change notification settings - Fork 329
feat(rbac): Add RBAC service and API routers #1986
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-scope-seeding
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. |
b6b9ce4 to
1246c82
Compare
98f1d4b to
25fe8c2
Compare
1246c82 to
b43f87d
Compare
25fe8c2 to
ac3b77b
Compare
d1c99e7 to
f0f545f
Compare
|
@cubic review |
@jordan-umusu I have started the AI code review. It will take a few minutes to complete. |
|
@codex review |
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
tracecat/alembic/versions/55c5d2499eee_add_rbac_tables.py
Lines 311 to 315 in ac3b77b
| sa.UniqueConstraint( | |
| "user_id", | |
| "organization_id", | |
| name=op.f("uq_user_role_assignment_user_id_organization_id"), | |
| ), |
The new user_role_assignment table enforces a unique constraint on (user_id, organization_id), which prevents a user from having more than one assignment within the same org. That makes it impossible to add a workspace-specific role for a user who already has an org-level role (the common case), e.g. accepting a workspace invitation will attempt to insert a workspace assignment and will hit a uniqueness violation. If the intent is “one role per workspace plus an optional org-wide assignment,” this constraint should be removed or replaced with a (user_id, workspace_id) unique plus the existing partial index for workspace_id IS NULL.
ℹ️ 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.
4 issues found across 22 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/db/models.py">
<violation number="1" location="tracecat/db/models.py:475">
P2: The new bidirectional relationship is incomplete: User.role_assignments specifies back_populates="user", but UserRoleAssignment.user does not define the reciprocal back_populates. This can lead to SQLAlchemy warnings and unsynchronized relationship state when assigning roles. Add back_populates="role_assignments" on UserRoleAssignment.user (or remove back_populates here if you want a unidirectional relationship).</violation>
</file>
<file name="packages/tracecat-ee/tracecat_ee/rbac/router.py">
<violation number="1" location="packages/tracecat-ee/tracecat_ee/rbac/router.py:166">
P1: This will raise `TypeError: got multiple values for keyword argument 'scopes'` at runtime. When SQLAlchemy loads `r.scopes`, it's stored in `r.__dict__`, so the unpacked dict already contains `scopes`, conflicting with the explicit `scopes=` argument. Use explicit attribute mapping like `get_role` does.</violation>
</file>
<file name="tests/unit/test_rbac_service.py">
<violation number="1" location="tests/unit/test_rbac_service.py:55">
P2: OrganizationMembership no longer accepts a `role` argument. This fixture will raise a TypeError on instantiation, so the test setup fails before the RBAC service is exercised. Remove the role parameter or assign roles via the RBAC assignment tables.</violation>
</file>
<file name="packages/tracecat-ee/tracecat_ee/rbac/service.py">
<violation number="1" location="packages/tracecat-ee/tracecat_ee/rbac/service.py:380">
P1: Security: The comment states this should verify user belongs to the organization, but the query only checks if the user exists globally. This could allow adding users from other organizations to groups. Consider joining with an org membership table to verify the user belongs to `self.organization_id`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
f0f545f to
1b379d9
Compare
ac3b77b to
06ce0a2
Compare
1b379d9 to
7fc9fa9
Compare
06ce0a2 to
ef01108
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef01108209
ℹ️ 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 17 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="packages/tracecat-ee/tracecat_ee/rbac/service.py">
<violation number="1" location="packages/tracecat-ee/tracecat_ee/rbac/service.py:628">
P1: Security: Missing organization membership verification. This only checks if the user exists globally, but doesn't verify they belong to the current organization. This could allow role assignment to users outside the organization. The `add_group_member` method correctly checks `OrganizationMembership` - this should follow the same pattern.</violation>
</file>
<file name="tracecat/db/models.py">
<violation number="1" location="tracecat/db/models.py:481">
P2: Duplicate `role_assignments` relationship overrides the earlier attribute definition. Remove the duplicate block to avoid confusion and accidental overrides if parameters diverge later.</violation>
</file>
<file name="tracecat/organization/router.py">
<violation number="1" location="tracecat/organization/router.py:146">
P3: `scalar_one_or_none()` drops the RoleModel columns from a multi-column select, leaving the joins/columns unused and misleading. Either select only `User` or switch back to `result.tuples().one_or_none()` to match the multi-column query.
(Based on your team's feedback about using result.tuples() for multi-column selects.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
7fc9fa9 to
d685936
Compare
ef01108 to
d73db0a
Compare
cdbf316 to
69e5c60
Compare
d73db0a to
a2665c6
Compare
998666b to
39c23c9
Compare
39c23c9 to
5dcdedf
Compare
a2665c6 to
217092b
Compare
217092b to
cb434f8
Compare
5dcdedf to
b51872c
Compare
b51872c to
a76a788
Compare
cb434f8 to
4cced9a
Compare
4cced9a to
659d664
Compare
Update import and usage to match the renamed constant from the auth-layer branch. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
205f2d1 to
06cfa32
Compare
a76a788 to
c4ba9cc
Compare
| class GroupAssignmentRead(BaseModel): | ||
| """Read schema for a group assignment.""" | ||
|
|
||
| model_config = ConfigDict(from_attributes=True) | ||
|
|
||
| id: UUID | ||
| organization_id: UUID | ||
| group_id: UUID | ||
| workspace_id: UUID | None = None | ||
| role_id: UUID | ||
| assigned_at: datetime | ||
| assigned_by: UUID | None = None |
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.
naming consistency with db model i.e. GroupRoleAssignment*
| conditions = [Scope.organization_id == self.organization_id] | ||
|
|
||
| if include_system: | ||
| # Include system scopes (organization_id IS NULL) | ||
| conditions = [ | ||
| (Scope.organization_id == self.organization_id) | ||
| | (Scope.organization_id.is_(None)) | ||
| ] | ||
|
|
||
| stmt = select(Scope).where(*conditions) |
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.
does this have to be wrapped in a list?
| return scope | ||
|
|
||
| @audit_log(resource_type="rbac_scope", action="delete", resource_id_attr="scope_id") | ||
| async def delete_scope(self, scope_id: UserID) -> None: |
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.
wrong type?
| @audit_log(resource_type="rbac_role", action="update", resource_id_attr="role_id") | ||
| async def update_role( | ||
| self, | ||
| role_id: UserID, |
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.
wrong type?
| await self.get_group(group_id) | ||
| await self.get_role(role_id) |
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.
nit: prefer to check existence explicitly instead of fetch and discard
| service = RBACService(session, role=role) | ||
| scopes = await service.list_scopes(include_system=include_system, source=source) | ||
| return ScopeList( | ||
| items=[ScopeRead.model_validate(s) for s in scopes], |
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.
if you have ScopeRead inherit from Schema it comes with a Schema.list_adapter() classmethod that lets us do this but with a single TypeAdapter call
| # User Scopes Router (OSS - needed for ScopeProvider) | ||
| # ============================================================================= | ||
|
|
||
| user_scopes_router = APIRouter(prefix="/users/me", tags=["users"]) |
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.
does this overwrite /users/me in the fastapi users router?
| UserRoleAssignment, | ||
| ) | ||
| from tracecat.db.models import ( | ||
| Role as RoleModel, |
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.
maybe DBRole is more consistent with how we did DBChatMessage

Summary by cubic
Adds a full RBAC system with service layer, REST APIs, and DB models for scopes, roles, groups, and role assignments. Seeds default scopes/roles and computes effective user scopes for access checks and the UI.
New Features
Migration
Written for commit ac3b77b. Summary will update on new commits.