Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
thabofletcher
added a commit
that referenced
this pull request
Feb 3, 2026
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Greptile OverviewGreptile SummaryThis PR introduces the initial database schema and ORM models for a dynamic RBAC system, including Alembic migrations to create RBAC tables and seed permissions/roles, plus unit tests covering basic model behavior (roles, permissions, assignments, and constraints). The RBAC models are wired into the existing SQLAlchemy model registry via Key issues to address before merging:
After those are fixed, consider aligning server_default usage (SQL expressions vs strings) and making time-dependent tests deterministic. Confidence Score: 2/5
Important Files Changed
|
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py
Show resolved
Hide resolved
18 tasks
thabofletcher
added a commit
that referenced
this pull request
Feb 6, 2026
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
4e10302 to
ae00de0
Compare
Add industry-standard RBAC (Role-Based Access Control) system with: Database tables: - rbac_role: Dynamic role definitions with hierarchy support - rbac_permission: Permission definitions (seeded from SCOPE_REGISTRY) - rbac_role_permission: Role-permission mapping (many-to-many) - rbac_user_role: User role assignments with resource scoping + temporal validity - rbac_role_constraint: Separation of duties constraints SQLAlchemy models: - RBACRole: Supports role hierarchy via parent_role_id - RBACPermission: Permission definitions - RBACRolePermission: Junction table - RBACUserRole: Supports resource-scoped and time-bounded assignments - RBACRoleConstraint: Static/dynamic SoD and cardinality constraints Migrations: - Add RBAC tables with indexes and constraints - Seed default roles and permissions from existing role definitions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ations Changed revision IDs to unique values: - add_rbac_tables: a1b2c3d4e5f6 -> d9ee4ea46797 - seed_rbac_defaults: b2c3d4e5f6a7 -> f5f526cbc35a The original IDs conflicted with existing migrations: - xx_2025_11_10_1200_a1b2c3d4e5f6_add_test_datastore_to_connectiontype.py - xx_2025_11_12_1430_b2c3d4e5f6a7_add_default_identity_definitions.py Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit adds comprehensive tests for the RBAC models: - TestRBACPermission: Tests for creating and persisting permission records - TestRBACRole: Tests for custom roles, role hierarchy, and permission inheritance - TestRBACUserRole: Tests for role assignments, resource scoping, and temporal validity - TestRBACRoleConstraint: Tests for SoD and cardinality constraints Tests cover: - Basic CRUD operations - Parent-child role relationships - Permission inheritance through role hierarchy - Scoped role assignments (resource_type + resource_id) - Temporal role validity (valid_from, valid_until) - Separation of duties constraints - Cardinality constraints Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add passive_deletes=True to the user relationship so SQLAlchemy defers to the database's ON DELETE CASCADE behavior instead of trying to set user_id=NULL (which causes IntegrityError). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add scopes required for RBAC management UI: - rbac_role:create/read/update/delete - rbac_permission:read - rbac_user_role:create/read/update/delete - rbac_constraint:create/read/update/delete - rbac:evaluate Also add migration for existing installations to add these scopes and assign them to the Owner role. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This migration adds fidesplus-specific permissions (discovery_monitor, custom_field, taxonomy, etc.) to the rbac_permission table and assigns them to the appropriate system roles. This replaces the runtime seeding that was previously in fidesplus startup, following the pattern of keeping all DB changes in fides. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Updated down_revision from 6d5f70dd0ba5 to 627c230d9917 to align with the current head on main branch. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use RelationshipProperty[Type] for SQLAlchemy relationship type annotations to match the pattern used elsewhere in the fides codebase. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Addresses Greptile review feedback: 1. Fix migration docstring to match actual down_revision (627c230d9917) 2. Change rbac_role_permission from id-based PK to composite PK: - Migration now creates table with (role_id, permission_id) as composite PK - Removed id column and updated_at column (not needed for junction table) - Updated all seed migrations to not insert id column 3. Update RBACRolePermission model to override Base columns: - Set id = None to remove inherited id column - Set updated_at = None to remove inherited updated_at column - Composite PK on (role_id, permission_id) matches migration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…et down_revision to main head Co-authored-by: Cursor <cursoragent@cursor.com>
ae00de0 to
58d46de
Compare
- Update first RBAC migration down_revision to f85bd4c08401 (current main head) - Add backfill:exec scope to base seed migration - Add chat_provider:read, chat_provider:update scopes to fidesplus seed - Add privacy_assessment:read/create/update/delete scopes to fidesplus seed - Assign new scopes to owner and contributor roles as appropriate Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update column comments to match model docstrings exactly - Replace unique constraint + non-unique index with unique index for code and key columns (matching model unique=True, index=True) - Remove extraneous indexes not defined in models: - ix_rbac_role_permission_role_id/permission_id - ix_rbac_user_role_resource/validity - ix_rbac_role_constraint_type - Add column comments to junction table columns Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
tests/api/models/test_rbac.py (1)
375-388: Consider using__file__relative path more robustly.The path construction traverses multiple parent directories with hardcoded depth. If the test file moves or the directory structure changes, this will break silently.
♻️ Suggested improvement
- migrations_dir = ( - Path(__file__).parent.parent.parent.parent - / "src" - / "fides" - / "api" - / "alembic" - / "migrations" - / "versions" - ) + # Find repo root by looking for a known marker file + current = Path(__file__).resolve() + while current != current.parent: + if (current / "pyproject.toml").exists(): + break + current = current.parent + migrations_dir = current / "src" / "fides" / "api" / "alembic" / "migrations" / "versions"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/models/test_rbac.py` around lines 375 - 388, The path building using Path(__file__).parent.parent.parent.parent / "src" / "fides" / "api" / "alembic" / "migrations" / "versions" is brittle; change the logic in this test (symbols: migrations_dir, seed_migration) to resolve paths more robustly by using Path(__file__).resolve() and walking parents or by locating the installed package/module (e.g., import fides or use importlib.resources) to find the "alembic/migrations/versions" directory dynamically; update migrations_dir calculation to search upward for the repository root or the "src" or "fides" directory instead of hardcoded parent chaining so the glob("*seed_rbac_defaults*.py") still finds the file even if the test file or layout moves.src/fides/api/models/rbac/rbac_role.py (1)
97-102:lazy="dynamic"andList[...]type annotation do not match.
user_assignmentsis annotated asList["RBACUserRole"], but SQLAlchemy's dynamic loader returns anAppenderQuery(aQuerysubclass) instead of a list. Either switch to a non-dynamic loader or update the type annotation toAppenderQuery["RBACUserRole"]to match the actual runtime type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/models/rbac/rbac_role.py` around lines 97 - 102, The type annotation for user_assignments does not match the runtime type returned by relationship(..., lazy="dynamic"); update the code so they agree: either remove lazy="dynamic" on the relationship call to return a standard list-like collection (keeping the annotation as RelationshipProperty[List["RBACUserRole"]]) or change the annotation to RelationshipProperty[AppenderQuery["RBACUserRole"]] (import AppenderQuery) to reflect the dynamic loader; adjust the import and annotation next to user_assignments and ensure RBACUserRole and RelationshipProperty references remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py`:
- Around line 304-350: The RBAC constraint table (rbac_role_constraint)
currently allows invalid rows; add CHECK constraints in the migration alongside
the existing columns to enforce invariants: (1) constrain "constraint_type" to
allowed values ('static_sod','dynamic_sod','cardinality'), (2) ensure
"role_id_1" != "role_id_2" (no identical roles), (3) require "role_id_2" IS NOT
NULL for SoD types and IS NULL for 'cardinality' rows, and (4) require
"max_users" IS NOT NULL AND "max_users" > 0 for 'cardinality' rows (and
optionally NULL for others). Implement these using sa.CheckConstraint entries
referenced by the column names "constraint_type", "role_id_1", "role_id_2", and
"max_users" in the migration that creates rbac_role_constraint.
- Around line 217-228: Replace the single UNIQUE constraint on rbac_user_role
with three PostgreSQL partial unique indexes and add a CHECK to prevent orphaned
resource IDs: create uq_rbac_user_role_global (unique on user_id, role_id WHERE
resource_type IS NULL AND resource_id IS NULL), uq_rbac_user_role_type_scoped
(unique on user_id, role_id, resource_type WHERE resource_type IS NOT NULL AND
resource_id IS NULL), and uq_rbac_user_role_fully_scoped (unique on user_id,
role_id, resource_type, resource_id WHERE resource_id IS NOT NULL), and add the
CHECK constraint resource_id IS NULL OR resource_type IS NOT NULL; update the
Alembic migration in the migration file that defines the rbac_user_role table
(the file in the diff) to create these indexes and the check instead of the
previous UNIQUE, and update the RBACUserRole model
(src/fides/api/models/rbac/rbac_user_role.py) to reflect the new DB constraints
(remove the overlapping UNIQUE definition and ensure the model allows NULLs on
resource_type/resource_id consistent with the partial indexes).
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.py`:
- Around line 170-186: The contributor role currently ends up including RBAC
management scopes because RBAC_MANAGEMENT_SCOPES aren't excluded when building
contributor_scopes from SCOPE_DOCS; update the logic that constructs
contributor_scopes (the code that iterates SCOPE_DOCS and applies
NOT_CONTRIBUTOR_SCOPES) to also filter out any scope present in
RBAC_MANAGEMENT_SCOPES (or add RBAC_MANAGEMENT_SCOPES to NOT_CONTRIBUTOR_SCOPES)
so that contributor does not receive scopes listed in RBAC_MANAGEMENT_SCOPES;
locate the symbols contributor_scopes, SCOPE_DOCS, NOT_CONTRIBUTOR_SCOPES, and
RBAC_MANAGEMENT_SCOPES to apply this change.
- Around line 425-432: The downgrade currently issues broad deletes ("DELETE
FROM rbac_role WHERE is_system_role = true" and "DELETE FROM rbac_permission")
which remove any runtime/custom entries; narrow the deletes to only the rows
this migration seeded by filtering on the specific identifiers used in the
upgrade (e.g., delete from rbac_permission WHERE key IN (...) or WHERE name IN
(...) for the SCOPE_DOCS permission and any other seeds added here, and delete
from rbac_role WHERE name IN (...) or WHERE id IN (...) for the specific system
roles created), using the same unique keys/names inserted by this migration
(reference: rbac_permission, rbac_role, is_system_role, the SCOPE_DOCS seed) so
rollback only removes those seeded records.
In `@src/fides/api/models/rbac/rbac_role.py`:
- Around line 107-136: get_all_permissions (and similarly get_ancestor_roles)
can recurse infinitely if there's a cyclic parent_role chain; update
get_all_permissions to accept or create a visited set of role IDs, check and
skip/stop when encountering an already-seen role id to prevent recursion, and
ensure get_ancestor_roles uses the same visited-role guard; additionally, add
validation in the RBAC role save/update logic to detect and reject cycles by
walking parent_role_id pointers and failing the save if a cycle is found (refer
to methods get_all_permissions and get_ancestor_roles for where to add the
traversal/visited check and the save/update hook for cycle rejection).
In `@src/fides/api/models/rbac/rbac_user_role.py`:
- Around line 62-67: The Column definition for valid_from currently uses a plain
string server_default="now()", which SQLAlchemy treats as a literal; change it
to use a SQL expression like server_default=text("now()") (import text from
sqlalchemy or sqlalchemy.sql) so the model generates DEFAULT now() correctly;
update the valid_from Column in the rbac_user_role model to use
server_default=text("now()") and add the necessary import for text.
---
Nitpick comments:
In `@src/fides/api/models/rbac/rbac_role.py`:
- Around line 97-102: The type annotation for user_assignments does not match
the runtime type returned by relationship(..., lazy="dynamic"); update the code
so they agree: either remove lazy="dynamic" on the relationship call to return a
standard list-like collection (keeping the annotation as
RelationshipProperty[List["RBACUserRole"]]) or change the annotation to
RelationshipProperty[AppenderQuery["RBACUserRole"]] (import AppenderQuery) to
reflect the dynamic loader; adjust the import and annotation next to
user_assignments and ensure RBACUserRole and RelationshipProperty references
remain unchanged.
In `@tests/api/models/test_rbac.py`:
- Around line 375-388: The path building using
Path(__file__).parent.parent.parent.parent / "src" / "fides" / "api" / "alembic"
/ "migrations" / "versions" is brittle; change the logic in this test (symbols:
migrations_dir, seed_migration) to resolve paths more robustly by using
Path(__file__).resolve() and walking parents or by locating the installed
package/module (e.g., import fides or use importlib.resources) to find the
"alembic/migrations/versions" directory dynamically; update migrations_dir
calculation to search upward for the repository root or the "src" or "fides"
directory instead of hardcoded parent chaining so the
glob("*seed_rbac_defaults*.py") still finds the file even if the test file or
layout moves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88004cb9-d6e1-494d-bb4b-d07b0230615f
📒 Files selected for processing (14)
.fides/db_dataset.ymlchangelog/7285.yamlsrc/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.pysrc/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.pysrc/fides/api/alembic/migrations/versions/xx_2026_02_01_0900_9f6555f12ad1_add_rbac_management_scopes.pysrc/fides/api/alembic/migrations/versions/xx_2026_02_01_1000_a8b9c0d1e2f3_seed_fidesplus_scopes.pysrc/fides/api/models/rbac/__init__.pysrc/fides/api/models/rbac/rbac_permission.pysrc/fides/api/models/rbac/rbac_role.pysrc/fides/api/models/rbac/rbac_role_constraint.pysrc/fides/api/models/rbac/rbac_role_permission.pysrc/fides/api/models/rbac/rbac_user_role.pysrc/fides/api/models/sql_models.pytests/api/models/test_rbac.py
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py
Show resolved
Hide resolved
Comment on lines
+304
to
+350
| sa.Column( | ||
| "constraint_type", | ||
| sa.String(length=50), | ||
| nullable=False, | ||
| comment="Type of constraint: static_sod, dynamic_sod, or cardinality", | ||
| ), | ||
| sa.Column( | ||
| "role_id_1", | ||
| sa.String(length=255), | ||
| nullable=False, | ||
| comment="First role in the constraint (required for all types)", | ||
| ), | ||
| sa.Column( | ||
| "role_id_2", | ||
| sa.String(length=255), | ||
| nullable=True, | ||
| comment="Second role in the constraint (required for SoD, NULL for cardinality)", | ||
| ), | ||
| sa.Column( | ||
| "max_users", | ||
| sa.Integer(), | ||
| nullable=True, | ||
| comment="Maximum number of users for cardinality constraint", | ||
| ), | ||
| sa.Column( | ||
| "description", | ||
| sa.Text(), | ||
| nullable=True, | ||
| comment="Description of why this constraint exists", | ||
| ), | ||
| sa.Column( | ||
| "is_active", | ||
| sa.Boolean(), | ||
| server_default="true", | ||
| nullable=False, | ||
| comment="Whether this constraint is currently enforced", | ||
| ), | ||
| sa.ForeignKeyConstraint( | ||
| ["role_id_1"], | ||
| ["rbac_role.id"], | ||
| ondelete="CASCADE", | ||
| ), | ||
| sa.ForeignKeyConstraint( | ||
| ["role_id_2"], | ||
| ["rbac_role.id"], | ||
| ondelete="CASCADE", | ||
| ), |
There was a problem hiding this comment.
Reject impossible RBAC constraint rows at the DB layer.
This table currently accepts invalid combinations: cardinality without max_users, SoD rows without role_id_2, negative limits, or even the same role on both sides. The helper methods in src/fides/api/models/rbac/rbac_role_constraint.py assume those invariants hold, so letting bad rows persist will turn into silent authorization drift later.
🧱 Example hardening
sa.Column(
"is_active",
sa.Boolean(),
server_default="true",
nullable=False,
comment="Whether this constraint is currently enforced",
),
+ sa.CheckConstraint(
+ "(constraint_type IN ('static_sod', 'dynamic_sod') AND role_id_2 IS NOT NULL AND max_users IS NULL) OR "
+ "(constraint_type = 'cardinality' AND role_id_2 IS NULL AND max_users IS NOT NULL AND max_users > 0)",
+ name="ck_rbac_role_constraint_shape",
+ ),
+ sa.CheckConstraint(
+ "role_id_2 IS NULL OR role_id_1 <> role_id_2",
+ name="ck_rbac_role_constraint_distinct_roles",
+ ),
sa.ForeignKeyConstraint(
["role_id_1"],
["rbac_role.id"],
ondelete="CASCADE",
),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py`
around lines 304 - 350, The RBAC constraint table (rbac_role_constraint)
currently allows invalid rows; add CHECK constraints in the migration alongside
the existing columns to enforce invariants: (1) constrain "constraint_type" to
allowed values ('static_sod','dynamic_sod','cardinality'), (2) ensure
"role_id_1" != "role_id_2" (no identical roles), (3) require "role_id_2" IS NOT
NULL for SoD types and IS NULL for 'cardinality' rows, and (4) require
"max_users" IS NOT NULL AND "max_users" > 0 for 'cardinality' rows (and
optionally NULL for others). Implement these using sa.CheckConstraint entries
referenced by the column names "constraint_type", "role_id_1", "role_id_2", and
"max_users" in the migration that creates rbac_role_constraint.
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.py
Show resolved
Hide resolved
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.py
Outdated
Show resolved
Hide resolved
Renames RBACRoleConstraint to RBACConstraint and implements NIST RBAC set-based constraint model with roles relationship and threshold. Includes migration fix to chain after main branch head.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
src/fides/api/models/rbac/rbac_user_role.py (1)
68-72:⚠️ Potential issue | 🟡 MinorUse a SQL expression for
valid_from's server default.Line 70 uses a plain string, while the migration uses
sa.text("now()"). In SQLAlchemy 1.4 that string is treated as a literal default, so metadata-created tables getDEFAULT 'now()'instead ofDEFAULT now(). Keep the ORM definition aligned with the migration.Suggested fix
-from sqlalchemy import Column, DateTime, ForeignKey, String, UniqueConstraint +from sqlalchemy import Column, DateTime, ForeignKey, String, UniqueConstraint, text @@ valid_from = Column( DateTime(timezone=True), - server_default="now()", + server_default=text("now()"), nullable=True, comment="When this assignment becomes active. NULL means immediately.", )In SQLAlchemy 1.4, what DDL does `server_default="now()"` generate on a DateTime column, and what should be used instead to emit `DEFAULT now()`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/models/rbac/rbac_user_role.py` around lines 68 - 72, The Column valid_from is using a plain string for server_default which SQLAlchemy treats as a literal and produces DEFAULT 'now()' instead of DEFAULT now(); update the ORM Column definition for valid_from (in rbac_user_role.py) to use a SQL expression object (e.g., sqlalchemy.text or sa.text("now()")) as the server_default so the DDL emits DEFAULT now(); ensure the import for text is present where valid_from is defined.src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py (2)
217-228:⚠️ Potential issue | 🟠 MajorReplace the scoped
UNIQUEwith partial unique indexes.Line 264 still allows duplicate global rows
(user_id, role_id, NULL, NULL)and duplicate type-scoped rows(user_id, role_id, resource_type, NULL)on PostgreSQL, becauseNULLvalues are distinct under a normalUNIQUEconstraint. That breaks the single-assignment-per-scope assumption used bysrc/fides/api/models/rbac/rbac_user_role.pyLines 151-181. Add a scope-shapeCHECK, create one partial unique index per scope, and mirror the same shape in the ORM model instead of the currentUniqueConstraint.Suggested migration shape
sa.ForeignKeyConstraint( ["assigned_by"], ["fidesuser.id"], ondelete="SET NULL", ), sa.PrimaryKeyConstraint("id"), - sa.UniqueConstraint( - "user_id", - "role_id", - "resource_type", - "resource_id", - name="uq_rbac_user_role_assignment", + sa.CheckConstraint( + "resource_id IS NULL OR resource_type IS NOT NULL", + name="ck_rbac_user_role_resource_scope", ), ) + op.create_index( + "uq_rbac_user_role_global", + "rbac_user_role", + ["user_id", "role_id"], + unique=True, + postgresql_where=sa.text("resource_type IS NULL AND resource_id IS NULL"), + ) + op.create_index( + "uq_rbac_user_role_type_scoped", + "rbac_user_role", + ["user_id", "role_id", "resource_type"], + unique=True, + postgresql_where=sa.text("resource_type IS NOT NULL AND resource_id IS NULL"), + ) + op.create_index( + "uq_rbac_user_role_fully_scoped", + "rbac_user_role", + ["user_id", "role_id", "resource_type", "resource_id"], + unique=True, + postgresql_where=sa.text("resource_id IS NOT NULL"), + )In PostgreSQL, how do UNIQUE constraints treat NULL values, and what partial-unique-index pattern should be used to enforce uniqueness separately for global rows (resource_type/resource_id NULL), type-scoped rows (resource_id NULL), and fully scoped rows?Also applies to: 263-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py` around lines 217 - 228, The current UNIQUE constraint on the user-role association does not prevent duplicates because PostgreSQL treats NULLs as distinct; replace the single UniqueConstraint with a CHECK that enforces one of three scope shapes (global: resource_type IS NULL AND resource_id IS NULL; type-scoped: resource_type IS NOT NULL AND resource_id IS NULL; fully-scoped: resource_type IS NOT NULL AND resource_id IS NOT NULL), then create three partial UNIQUE indexes covering (user_id, role_id) for each scope condition (one WHERE resource_type IS NULL AND resource_id IS NULL, one WHERE resource_id IS NULL AND resource_type IS NOT NULL, and one WHERE resource_type IS NOT NULL AND resource_id IS NOT NULL) so uniqueness is enforced per-scope; mirror this shape in the ORM model (the class in rbac_user_role.py with the existing UniqueConstraint) by removing the UniqueConstraint and documenting/validating the same scope-check logic in the model (lines ~151-181) so DB and ORM enforce the same rules.
307-336:⚠️ Potential issue | 🟠 MajorReject invalid RBAC constraint rows at the DB layer.
src/fides/api/models/rbac/rbac_constraint.pyLines 126-198 assumesconstraint_typeis one of the three supported values andthreshold > 0. Right now a typo inconstraint_typesilently disables enforcement, andthreshold <= 0makes every SoD/cardinality check immediately fail. Add schema checks here so bad policy rows cannot persist.Suggested hardening
sa.Column( "is_active", sa.Boolean(), server_default="true", nullable=False, comment="Whether this constraint is currently enforced", ), + sa.CheckConstraint( + "constraint_type IN ('static_sod', 'dynamic_sod', 'cardinality')", + name="ck_rbac_constraint_type", + ), + sa.CheckConstraint( + "threshold > 0", + name="ck_rbac_constraint_threshold_positive", + ), sa.PrimaryKeyConstraint("id"), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py` around lines 307 - 336, Add DB-level validation to the RBAC constraint table so invalid rows cannot be persisted: in the Alembic migration that defines the RBAC constraint table (the migration creating columns "constraint_type" and "threshold"), add SQLAlchemy CheckConstraints that enforce constraint_type IN ('static_sod','dynamic_sod','cardinality') and threshold > 0 (e.g., sa.CheckConstraint("constraint_type IN ('static_sod','dynamic_sod','cardinality')", name="ck_rbac_constraint_type") and sa.CheckConstraint("threshold > 0", name="ck_rbac_threshold_positive")), ensuring names match existing table (the migration file and the model src/fides/api/models/rbac/rbac_constraint.py) so application-level assumptions cannot be violated by bad DB rows.src/fides/api/models/rbac/rbac_role.py (1)
113-142:⚠️ Potential issue | 🟠 MajorGuard hierarchy traversal against cyclic parents.
Lines 136-137 recurse into
parent_rolewithout a visited-set, and Lines 188-190 loop untilNone. A cyclicparent_role_idchain will therefore recurse forever inget_all_permissions()and loop forever inget_ancestor_roles(). Because roles are dynamic data, one bad row can take permission evaluation down.Suggested traversal guard
def get_all_permissions(self, db: Session) -> List["RBACPermission"]: """ Get all permissions for this role, including inherited from parent roles. @@ """ seen_permission_ids: Set[str] = set() all_permissions: List["RBACPermission"] = [] - - # Add direct permissions - for permission in self.permissions: - if permission.id not in seen_permission_ids and permission.is_active: - seen_permission_ids.add(permission.id) - all_permissions.append(permission) - - # Add inherited permissions from parent hierarchy - if self.parent_role: - for permission in self.parent_role.get_all_permissions(db): + current: Optional["RBACRole"] = self + visited_role_keys: Set[str] = set() + while current: + role_key = current.id or f"object:{id(current)}" + if role_key in visited_role_keys: + break + visited_role_keys.add(role_key) + for permission in current.permissions: if permission.id not in seen_permission_ids and permission.is_active: seen_permission_ids.add(permission.id) all_permissions.append(permission) + current = current.parent_role return all_permissions @@ def get_ancestor_roles(self) -> List["RBACRole"]: @@ """ ancestors: List["RBACRole"] = [] current = self.parent_role - while current: + visited_role_keys: Set[str] = {self.id or f"object:{id(self)}"} + while current: + role_key = current.id or f"object:{id(current)}" + if role_key in visited_role_keys: + break + visited_role_keys.add(role_key) ancestors.append(current) current = current.parent_role return ancestorsAlso applies to: 179-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/models/rbac/rbac_role.py` around lines 113 - 142, get_all_permissions (and similarly get_ancestor_roles) can recurse/loop forever on cyclic parent_role/parent_role_id chains; add a visited guard: track visited role IDs (e.g., seen_role_ids) and check before traversing into self.parent_role or adding parents in the loop, returning/stop traversal when a role ID is already seen. Update get_all_permissions to accept or create a local visited set (or use an iterative traversal over parent_role following parent_role_id) and ensure you check parent_role.id in seen_role_ids before recursing and add it when first visited; apply the same visited-set logic to get_ancestor_roles to break cycles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py`:
- Around line 217-228: The current UNIQUE constraint on the user-role
association does not prevent duplicates because PostgreSQL treats NULLs as
distinct; replace the single UniqueConstraint with a CHECK that enforces one of
three scope shapes (global: resource_type IS NULL AND resource_id IS NULL;
type-scoped: resource_type IS NOT NULL AND resource_id IS NULL; fully-scoped:
resource_type IS NOT NULL AND resource_id IS NOT NULL), then create three
partial UNIQUE indexes covering (user_id, role_id) for each scope condition (one
WHERE resource_type IS NULL AND resource_id IS NULL, one WHERE resource_id IS
NULL AND resource_type IS NOT NULL, and one WHERE resource_type IS NOT NULL AND
resource_id IS NOT NULL) so uniqueness is enforced per-scope; mirror this shape
in the ORM model (the class in rbac_user_role.py with the existing
UniqueConstraint) by removing the UniqueConstraint and documenting/validating
the same scope-check logic in the model (lines ~151-181) so DB and ORM enforce
the same rules.
- Around line 307-336: Add DB-level validation to the RBAC constraint table so
invalid rows cannot be persisted: in the Alembic migration that defines the RBAC
constraint table (the migration creating columns "constraint_type" and
"threshold"), add SQLAlchemy CheckConstraints that enforce constraint_type IN
('static_sod','dynamic_sod','cardinality') and threshold > 0 (e.g.,
sa.CheckConstraint("constraint_type IN
('static_sod','dynamic_sod','cardinality')", name="ck_rbac_constraint_type") and
sa.CheckConstraint("threshold > 0", name="ck_rbac_threshold_positive")),
ensuring names match existing table (the migration file and the model
src/fides/api/models/rbac/rbac_constraint.py) so application-level assumptions
cannot be violated by bad DB rows.
In `@src/fides/api/models/rbac/rbac_role.py`:
- Around line 113-142: get_all_permissions (and similarly get_ancestor_roles)
can recurse/loop forever on cyclic parent_role/parent_role_id chains; add a
visited guard: track visited role IDs (e.g., seen_role_ids) and check before
traversing into self.parent_role or adding parents in the loop, returning/stop
traversal when a role ID is already seen. Update get_all_permissions to accept
or create a local visited set (or use an iterative traversal over parent_role
following parent_role_id) and ensure you check parent_role.id in seen_role_ids
before recursing and add it when first visited; apply the same visited-set logic
to get_ancestor_roles to break cycles.
In `@src/fides/api/models/rbac/rbac_user_role.py`:
- Around line 68-72: The Column valid_from is using a plain string for
server_default which SQLAlchemy treats as a literal and produces DEFAULT 'now()'
instead of DEFAULT now(); update the ORM Column definition for valid_from (in
rbac_user_role.py) to use a SQL expression object (e.g., sqlalchemy.text or
sa.text("now()")) as the server_default so the DDL emits DEFAULT now(); ensure
the import for text is present where valid_from is defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8856add-7ed3-4622-b7d9-3a888c35ba33
📒 Files selected for processing (11)
.fides/db_dataset.ymlsrc/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.pysrc/fides/api/models/rbac/__init__.pysrc/fides/api/models/rbac/rbac_constraint.pysrc/fides/api/models/rbac/rbac_constraint_role.pysrc/fides/api/models/rbac/rbac_permission.pysrc/fides/api/models/rbac/rbac_role.pysrc/fides/api/models/rbac/rbac_role_permission.pysrc/fides/api/models/rbac/rbac_user_role.pysrc/fides/api/models/sql_models.pytests/api/models/test_rbac.py
✅ Files skipped from review due to trivial changes (1)
- tests/api/models/test_rbac.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/fides/api/models/rbac/init.py
- src/fides/api/models/sql_models.py
- src/fides/api/models/rbac/rbac_permission.py
Contributor
Author
|
Addressing review comments: No longer applies:
Addressing remaining issues:
|
Security fix: - Exclude RBAC_MANAGEMENT_SCOPES from contributor role to prevent privilege escalation (contributors could previously manage roles) Data integrity fixes: - Replace single UNIQUE constraint on rbac_user_role with three partial unique indexes to correctly handle NULL scope values in PostgreSQL - Add CHECK constraint to prevent orphaned resource_id without resource_type Migration safety: - Scope downgrade to only delete seeded permissions, preserving any custom permissions created after the migration Robustness: - Add cycle detection to get_all_permissions() and get_ancestor_roles() to prevent infinite recursion/loops from cyclic parent_role_id chains Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py (1)
307-368: Consider adding CHECK constraints for constraint validation.The NIST set-based model is well-designed, but the table currently accepts any
constraint_typestring and anythresholdvalue (including 0 or negative). Adding validation would prevent invalid constraint data:🛡️ Suggested hardening
sa.Column( "is_active", sa.Boolean(), server_default="true", nullable=False, comment="Whether this constraint is currently enforced", ), sa.PrimaryKeyConstraint("id"), + sa.CheckConstraint( + "constraint_type IN ('static_sod', 'dynamic_sod', 'cardinality')", + name="ck_rbac_constraint_type", + ), + sa.CheckConstraint( + "threshold > 0", + name="ck_rbac_constraint_threshold_positive", + ), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py` around lines 307 - 368, The rbac_constraint table currently allows arbitrary constraint_type strings and invalid threshold values; add CHECK constraints when creating the table (in the op.create_table call for "rbac_constraint") to restrict constraint_type to the allowed set (e.g., 'static_sod', 'dynamic_sod', 'cardinality') and enforce threshold > 0 (and optionally threshold >= 1 for cardinality-specific logic), and ensure the is_active default uses a proper SQL boolean expression; modify the op.create_table invocation that defines columns "constraint_type", "threshold", and "is_active" to include the appropriate sa.CheckConstraint(s) referencing those column names so invalid rows are rejected at the DB level.src/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.py (1)
333-346: Consider adding explicit timestamps for consistency with other migrations.The INSERT statement omits
created_atandupdated_atcolumns, relying on server defaults. While this works, the relatedseed_fidesplus_scopes.pymigration (lines 324-337) explicitly includescreated_at, updated_atwithnow(), now(). For consistency and explicit behavior, consider adding these columns.♻️ Optional consistency improvement
connection.execute( text( """ - INSERT INTO rbac_permission (id, code, description, resource_type, is_active) - VALUES (:id, :code, :description, :resource_type, true) + INSERT INTO rbac_permission (id, code, description, resource_type, is_active, created_at, updated_at) + VALUES (:id, :code, :description, :resource_type, true, now(), now()) """ ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.py` around lines 333 - 346, The INSERT into rbac_permission done in the connection.execute block omits created_at/updated_at; update that INSERT (the SQL string used in connection.execute) to explicitly include created_at and updated_at columns and set their values to now(), now() (matching the pattern used in seed_fidesplus_scopes.py), ensuring the same parameter dict (permission_id, scope_code, description, resource_type) remains unchanged; locate the SQL text in the migration file where rbac_permission is inserted and add these columns to make timestamp behavior explicit and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py`:
- Around line 1-16: The module docstring header has a mismatch: the "Revises:
f85bd4c08401" entry does not match the actual down_revision variable value used
by Alembic (down_revision = "bf12f05ef8eb"); update the docstring so the
"Revises:" line matches the down_revision value "bf12f05ef8eb" (keep the
existing Revision ID and other metadata unchanged) to avoid confusion when
inspecting the migration file.
In `@src/fides/api/models/rbac/rbac_role.py`:
- Around line 102-108: Remove the single UniqueConstraint defined in
RBACUserRole.__table_args__ (the constraint on user_id, role_id, resource_type,
resource_id) because it conflicts with the migration's three PostgreSQL partial
unique indexes; edit the RBACUserRole class to delete that UniqueConstraint
entry from __table_args__ and rely solely on the migration-created partial
unique indexes for global, type-scoped, and fully-scoped uniqueness enforcement.
---
Nitpick comments:
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py`:
- Around line 307-368: The rbac_constraint table currently allows arbitrary
constraint_type strings and invalid threshold values; add CHECK constraints when
creating the table (in the op.create_table call for "rbac_constraint") to
restrict constraint_type to the allowed set (e.g., 'static_sod', 'dynamic_sod',
'cardinality') and enforce threshold > 0 (and optionally threshold >= 1 for
cardinality-specific logic), and ensure the is_active default uses a proper SQL
boolean expression; modify the op.create_table invocation that defines columns
"constraint_type", "threshold", and "is_active" to include the appropriate
sa.CheckConstraint(s) referencing those column names so invalid rows are
rejected at the DB level.
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.py`:
- Around line 333-346: The INSERT into rbac_permission done in the
connection.execute block omits created_at/updated_at; update that INSERT (the
SQL string used in connection.execute) to explicitly include created_at and
updated_at columns and set their values to now(), now() (matching the pattern
used in seed_fidesplus_scopes.py), ensuring the same parameter dict
(permission_id, scope_code, description, resource_type) remains unchanged;
locate the SQL text in the migration file where rbac_permission is inserted and
add these columns to make timestamp behavior explicit and consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49fcc31c-a6a6-4c5a-8618-2014eefa2e51
📒 Files selected for processing (3)
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.pysrc/fides/api/alembic/migrations/versions/xx_2026_01_31_1100_f5f526cbc35a_seed_rbac_defaults.pysrc/fides/api/models/rbac/rbac_role.py
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py
Show resolved
Hide resolved
- Update migration docstring Revises: to match actual down_revision - Remove UniqueConstraint from RBACUserRole model since uniqueness is now enforced by partial indexes in the migration Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai please summarize any changes that are applicable and have not been addressed |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the current state of unresolved review comments on this PR. 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1942 🏁 Script executed: Length of output: 25 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 47545 🏁 Script executed: Length of output: 0
|
| Item | Status |
|---|---|
| Contributor scope escalation (RBAC_MANAGEMENT_SCOPES in contributor) | ✅ Fixed |
| UNIQUE constraint NULL handling (3 partial indexes) | ✅ Fixed |
| Downgrade scope (preserve custom permissions) | ✅ Fixed |
Cycle detection in get_all_permissions / get_ancestor_roles |
✅ Fixed |
| rbac_role_constraint CHECK constraints | ✅ No longer applicable (redesigned in #7367) |
| Migration revision mismatch | ✅ Fixed |
| rbac_role_permission PK mismatch | ✅ Fixed |
| Drift detection test | ✅ Already implemented |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/fides/api/models/rbac/rbac_user_role.py (1)
68-72:⚠️ Potential issue | 🟠 MajorUse a SQL expression for
valid_from's server default.This still diverges from the migration, which uses
sa.text("now()"). With SQLAlchemy, the plain string here is treated as a literal, so metadata-created tables getDEFAULT 'now()'instead of executingnow().Suggested change
-from sqlalchemy import Column, DateTime, ForeignKey, String +from sqlalchemy import Column, DateTime, ForeignKey, String, text @@ valid_from = Column( DateTime(timezone=True), - server_default="now()", + server_default=text("now()"), nullable=True, comment="When this assignment becomes active. NULL means immediately.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/models/rbac/rbac_user_role.py` around lines 68 - 72, The Column definition for valid_from uses a plain string server_default ("now()") which becomes a literal default instead of a SQL expression; update the valid_from Column (in rbac_user_role.py) to use a SQL expression like sa.text("now()") or sqlalchemy.text("now()") for server_default and add the corresponding import (sa or text) so the metadata-created tables get DEFAULT now() rather than DEFAULT 'now()'.
🧹 Nitpick comments (1)
src/fides/api/models/rbac/rbac_user_role.py (1)
58-67: Mirror the scope CHECK in the ORM metadata.The migration now rejects
resource_idwithoutresource_type, but the model metadata still allows that impossible shape. If this table is ever created from metadata instead of Alembic,is_global()/matches_resource()can end up evaluating states the schema is supposed to forbid.Suggested change
-from sqlalchemy import Column, DateTime, ForeignKey, String +from sqlalchemy import CheckConstraint, Column, DateTime, ForeignKey, String @@ class RBACUserRole(Base): + __table_args__ = ( + CheckConstraint( + "resource_id IS NULL OR resource_type IS NOT NULL", + name="ck_rbac_user_role_resource_scope", + ), + ) + """ User to role assignment with optional resource scoping and temporal validity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/models/rbac/rbac_user_role.py` around lines 58 - 67, The model allows resource_id/resource_type combinations the DB migration forbids; add a table-level CheckConstraint in the RBACUserRole model (e.g., in __table_args__) that mirrors the migration rule so resource_id cannot be set without resource_type and vice-versa (use a constraint like "(resource_type IS NULL AND resource_id IS NULL) OR (resource_type IS NOT NULL AND resource_id IS NOT NULL)"). This keeps the ORM metadata consistent with the migration and prevents impossible states observed by is_global() and matches_resource().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py`:
- Around line 68-85: Add a schema-level CHECK that prevents a role from being
its own parent: modify the table definition that creates the rbac_role columns
(the parent_role_id column and its FK) to include a CheckConstraint such as
CHECK (parent_role_id IS NULL OR parent_role_id <> id) with a clear name (e.g.,
rbac_role_no_self_parent) so the database rejects rows where parent_role_id
equals the row's id; update the migration that defines parent_role_id and the FK
(the same migration that adds parent_role_id and the ForeignKeyConstraint) to
add this CheckConstraint on creation.
---
Duplicate comments:
In `@src/fides/api/models/rbac/rbac_user_role.py`:
- Around line 68-72: The Column definition for valid_from uses a plain string
server_default ("now()") which becomes a literal default instead of a SQL
expression; update the valid_from Column (in rbac_user_role.py) to use a SQL
expression like sa.text("now()") or sqlalchemy.text("now()") for server_default
and add the corresponding import (sa or text) so the metadata-created tables get
DEFAULT now() rather than DEFAULT 'now()'.
---
Nitpick comments:
In `@src/fides/api/models/rbac/rbac_user_role.py`:
- Around line 58-67: The model allows resource_id/resource_type combinations the
DB migration forbids; add a table-level CheckConstraint in the RBACUserRole
model (e.g., in __table_args__) that mirrors the migration rule so resource_id
cannot be set without resource_type and vice-versa (use a constraint like
"(resource_type IS NULL AND resource_id IS NULL) OR (resource_type IS NOT NULL
AND resource_id IS NOT NULL)"). This keeps the ORM metadata consistent with the
migration and prevents impossible states observed by is_global() and
matches_resource().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 49758f11-d324-496b-b83a-a883a852dda3
📒 Files selected for processing (2)
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.pysrc/fides/api/models/rbac/rbac_user_role.py
src/fides/api/alembic/migrations/versions/xx_2026_01_31_1000_d9ee4ea46797_add_rbac_tables.py
Show resolved
Hide resolved
- Use text("now()") instead of string "now()" for valid_from server_default
to ensure proper SQL expression generation
- Change user_assignments lazy="dynamic" to lazy="selectin" to match
List type annotation (dynamic returns AppenderQuery, not List)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/fides/api/models/rbac/rbac_role.py (1)
86-92:⚠️ Potential issue | 🟠 MajorReject cyclic parent chains on write, not just on read.
The visited-set guards stop infinite recursion, but they still allow
A -> B -> Ato be persisted. After that,get_all_permissions()andget_ancestor_roles()quietly return truncated results instead of a valid hierarchy. Add save/update validation onparent_role_id(or abefore_flushhook) so invalid role graphs fail fast.Also applies to: 113-157, 194-213
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/models/rbac/rbac_role.py` around lines 86 - 92, RBACRole currently allows cyclic parent chains to be persisted (e.g., A->B->A) which breaks get_all_permissions and get_ancestor_roles; add write-time validation that walks the parent chain and rejects any assignment that would create a cycle. Implement this by adding either a SQLAlchemy before_flush/session event handler or a `@validates`("parent_role_id") validator on RBACRole that, for each role being added/updated (inspect session.new/session.dirty or the validated instance), follows parent_role/parent_role_id up the chain and raises an exception (ValueError/IntegrityError) if the candidate parent is already a descendant of the instance; reference the RBACRole class, the parent_role and parent_role_id fields, and ensure the same logic is applied where parent_role is set or updated so invalid graphs fail fast on save.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/api/models/rbac/rbac_role.py`:
- Around line 130-137: The cycle-detection uses self.id which is None for
transient instances causing false positives; update both get_all_permissions()
and get_ancestor_roles() to use a stable identity key that falls back to object
identity when the PK is unpersisted (e.g. use key = self.id if self.id is not
None else id(self) and check/add that key to _visited_role_ids instead of
self.id) so in-memory hierarchies don't get incorrectly treated as cycles while
preserving persisted-ID behavior; ensure you update checks and additions around
the _visited_role_ids usage accordingly.
---
Duplicate comments:
In `@src/fides/api/models/rbac/rbac_role.py`:
- Around line 86-92: RBACRole currently allows cyclic parent chains to be
persisted (e.g., A->B->A) which breaks get_all_permissions and
get_ancestor_roles; add write-time validation that walks the parent chain and
rejects any assignment that would create a cycle. Implement this by adding
either a SQLAlchemy before_flush/session event handler or a
`@validates`("parent_role_id") validator on RBACRole that, for each role being
added/updated (inspect session.new/session.dirty or the validated instance),
follows parent_role/parent_role_id up the chain and raises an exception
(ValueError/IntegrityError) if the candidate parent is already a descendant of
the instance; reference the RBACRole class, the parent_role and parent_role_id
fields, and ensure the same logic is applied where parent_role is set or updated
so invalid graphs fail fast on save.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21e53ce8-ba55-46d3-b7e7-297e1fda55d0
📒 Files selected for processing (2)
src/fides/api/models/rbac/rbac_role.pysrc/fides/api/models/rbac/rbac_user_role.py
- Add CHECK constraint to prevent self-parent roles in migration - Fix cycle detection for transient SQLAlchemy instances (use id(self) fallback) - Add docstrings to all RBAC model methods for coverage threshold - Add type annotations to migration functions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai please check the latest set of changes and make sure all your concerns are addressed. I have chose not to address the "brittle path" - I will adress that if it becomes a rel concern (flaky or failing test, etc) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 27734
All applicable concerns have been addressed. The PR looks good from a code review perspective. 🎉
|
- Add partial unique indexes to RBACUserRole model's __table_args__ to match migration definitions, fixing Alembic autogenerate mismatch - Fix test_temporal_role_assignment to use separate roles, avoiding unique constraint violation on global scope assignments Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Adrian Galvan <adrian@ethyca.com>
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
2 participants
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.
Ticket: ENG-2696
Description Of Changes
Adds database migrations and SQLAlchemy models to support the new dynamic Role-Based Access Control (RBAC) system. This creates the foundational database schema that enables:
Important: These migrations only CREATE new tables - no existing tables are modified.
Code Changes
New Database Tables (5):
rbac_role- Dynamic role definitions with hierarchy supportrbac_permission- Permission definitions (seeded from SCOPE_REGISTRY)rbac_role_permission- Role to permission mapping (many-to-many)rbac_user_role- User role assignments with resource scoping + temporal validityrbac_role_constraint- Separation of duties constraintsMigrations (3):
d9ee4ea46797_add_rbac_tables.py- Creates all 5 RBAC tables with indexesf5f526cbc35a_seed_rbac_defaults.py- Seeds permissions from SCOPE_REGISTRY and creates system roles9f6555f12ad1_add_rbac_management_scopes.py- Adds RBAC management scopes (idempotent)SQLAlchemy Models:
src/fides/api/models/rbac/- New model package with all RBAC entitiessql_models.pyto import RBAC modelsSteps to Confirm
alembic upgrade head\dt rbac_*in psqlSELECT COUNT(*) FROM rbac_permission;(should be ~135)SELECT * FROM rbac_role;(should have 7 system roles)alembic downgrade -1(repeat 3x), verify tables removedPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and worksSummary by CodeRabbit