-
Notifications
You must be signed in to change notification settings - Fork 329
feat(rbac): Seed registry scopes during sync #1985
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: main
Are you sure you want to change the base?
Conversation
66302e5 to
25cd48d
Compare
b6b9ce4 to
1246c82
Compare
25cd48d to
4fca5e5
Compare
1246c82 to
b43f87d
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: b43f87dcc5
ℹ️ 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".
alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py
Outdated
Show resolved
Hide resolved
alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py
Outdated
Show resolved
Hide resolved
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.
8 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/organization/schemas.py">
<violation number="1" location="tracecat/organization/schemas.py:46">
P1: Missing validation: the docstring states that either `role_id` or `role_slug` must be provided, but both fields default to `None` with no enforcement. Add a Pydantic `model_validator` to ensure at least one is provided.
(Based on your team's feedback about using Pydantic v2 model_validator for cross-field constraints.) [FEEDBACK_USED]</violation>
</file>
<file name="tracecat/registry/sync/jobs.py">
<violation number="1" location="tracecat/registry/sync/jobs.py:199">
P2: Avoid broad `except Exception` in this business-logic helper. Catch only expected DB/validation exceptions (e.g., SQLAlchemy `IntegrityError`) and let unexpected errors bubble to the caller or a higher-level handler so they are not silently swallowed.
(Based on your team's feedback about error handling posture.) [FEEDBACK_USED]</violation>
</file>
<file name="tracecat/authz/seeding.py">
<violation number="1" location="tracecat/authz/seeding.py:363">
P3: Use tuples() for the multi-column select result to follow the project’s SQLAlchemy result handling guidance.
(Based on your team's feedback about using result.tuples() for multi-column selects.) [FEEDBACK_USED]</violation>
<violation number="2" location="tracecat/authz/seeding.py:448">
P2: Move the Organization import to the module top level to comply with the project rule that imports must not be inside function bodies.</violation>
</file>
<file name="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py">
<violation number="1" location="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py:25">
P1: Adding a NOT NULL column without a default value will fail if the table contains existing rows. Consider adding a `server_default` that references an existing role ID, or split this into multiple steps: add as nullable, populate data, then alter to NOT NULL.</violation>
<violation number="2" location="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py:40">
P1: Same issue as `invitation` table: adding a NOT NULL column without a default will fail if `organization_invitation` contains existing rows.</violation>
</file>
<file name="tracecat/authz/service.py">
<violation number="1" location="tracecat/authz/service.py:96">
P2: Use `result.tuples().all()` instead of `result.all()` for multi-column query results.
(Based on your team's feedback about using tuples().all() for multi-column queries.) [FEEDBACK_USED]</violation>
</file>
<file name="tracecat/workspaces/router.py">
<violation number="1" location="tracecat/workspaces/router.py:257">
P2: Membership lookup ignores org-wide role assignments (workspace_id=NULL), so a user with an org-wide role is returned as VIEWER. Include org-wide assignments when resolving the role.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py
Outdated
Show resolved
Hide resolved
alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py
Outdated
Show resolved
Hide resolved
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/tracecat/organization/service.py
Lines 146 to 149 in e417cab
| membership = OrganizationMembership( | |
| user_id=user_id, | |
| organization_id=invitation.organization_id, | |
| role=invitation.role, |
The OrganizationMembership model no longer has a role column, but accept_invitation_for_user still instantiates it with role=invitation.role. On the first org invitation acceptance (the /invitations/accept flow used by the UI), SQLAlchemy will raise TypeError: 'role' is an invalid keyword argument, so users can’t accept invites at all. This should mirror the updated org-accept path by omitting role and creating a UserRoleAssignment from invitation.role_id instead.
ℹ️ 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.
6 issues found across 26 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="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py">
<violation number="1" location="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py:118">
P1: Downgrade will fail if tables contain data: `organization_invitation` and `invitation` add NOT NULL `role` columns without `server_default`. PostgreSQL cannot add a NOT NULL column to a non-empty table without a default value. Add `server_default` like the membership tables have.</violation>
</file>
<file name="tracecat/authz/service.py">
<violation number="1" location="tracecat/authz/service.py:99">
P1: Missing organization filter on org-wide role assignments could leak roles across organizations. When fetching org-wide assignments (`workspace_id IS NULL`), the query should also filter by `organization_id` to ensure data isolation between organizations.</violation>
</file>
<file name="tracecat/workspaces/router.py">
<violation number="1" location="tracecat/workspaces/router.py:385">
P2: Join should target RoleModel, not UserRoleAssignment, to avoid a self-join and ensure correct role lookup.</violation>
<violation number="2" location="tracecat/workspaces/router.py:399">
P2: Skip `role_slug is None` in get_workspace_membership so custom roles don’t block the org-wide fallback, matching list_memberships behavior.</violation>
</file>
<file name="tracecat/authz/seeding.py">
<violation number="1" location="tracecat/authz/seeding.py:279">
P2: seed_registry_scope is not concurrency-safe: it does a check-then-insert without ON CONFLICT handling, so concurrent seeding for the same action can raise a unique-constraint error. Use an upsert (ON CONFLICT DO NOTHING) or catch IntegrityError and re-select the existing scope to keep seeding idempotent.</violation>
<violation number="2" location="tracecat/authz/seeding.py:395">
P2: Role seeding is not concurrency-safe: the existence check + insert can race across processes and raise IntegrityError, breaking startup seeding. Use an upsert (ON CONFLICT DO NOTHING) or catch IntegrityError and re-query to keep seeding idempotent under concurrent startup.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py
Outdated
Show resolved
Hide resolved
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.
2 issues found across 5 files (changes from recent commits).
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="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py">
<violation number="1" location="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py:135">
P2: Downgrade backfills every organization role as MEMBER, which discards real role assignments. Consider adding the column nullable, updating it from role_id via role.slug, then setting NOT NULL (and only add a default if the original schema had one).</violation>
<violation number="2" location="alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py:168">
P2: Downgrade backfills every workspace role as EDITOR, losing actual role assignments. Consider adding the column nullable, updating it from role_id via role.slug, then setting NOT NULL (and only add a default if the original schema had one).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py
Outdated
Show resolved
Hide resolved
alembic/versions/4bb7e59026f3_drop_role_from_membership_and_.py
Outdated
Show resolved
Hide resolved
d1c99e7 to
f0f545f
Compare
e0947f9 to
96b59ed
Compare
96b59ed to
af52e2e
Compare
f0f545f to
1b379d9
Compare
af52e2e to
005f3b4
Compare
1b379d9 to
7fc9fa9
Compare
87388ed to
fe44660
Compare
09e1335 to
51957a0
Compare
daryllimyt
left a comment
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.
please rebase onto main
51957a0 to
a2069c4
Compare
39c23c9 to
5dcdedf
Compare
5dcdedf to
a34c7ca
Compare
7d82686 to
ed1dadb
Compare
This comment has been minimized.
This comment has been minimized.
7c8c52e to
a76a788
Compare
a76a788 to
c4ba9cc
Compare

Summary by cubic
Seeds RBAC defaults at startup and during registry sync: system scopes and preset org roles, plus registry action scopes. Removes role fields from memberships/invitations and updates invitations to use role_id/role_slug.
Written for commit 1246c82. Summary will update on new commits.