Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
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
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>
Adds system.operations data category annotations to db_dataset.yml for the five new RBAC tables to satisfy the fides_db_scan CI check. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Is this a duplicate of what's in https://github.com/ethyca/fides/pull/7285/changes#diff-1963b1998551549c30313328d4008ed8da2bbfc6e20c05141bdfe7d21b184e4dR153-R167?
There was a problem hiding this comment.
Yes, it should match 1:1 - otherwise this fails:
https://github.com/ethyca/fides/actions/runs/21926704315/job/63322448576
But its completely possible I'm doing this a non-standard way
| from fides.api.models.rbac.rbac_role import RBACRole | ||
|
|
||
|
|
||
| class RBACPermission(Base): |
There was a problem hiding this comment.
I see we're introducing the concept of permissions, which makes more sense than scopes to me. Do you see any issues with us renaming scopes to permissions? Or do you see this table name as just implementation detail?
There was a problem hiding this comment.
I think its a relatively minor nuance that we should watch out for if we are missing the boat on it but this is the way I see it:
Users have roles, and are assigned permissions, those permissions allow them to obtain a scope, and the scope defines what they can access in the application.
That being said, with fine grained access controls like this there should generally be a 1:1 mapping between a permission and its scope
- Add RBAC management scopes to scope_registry.py so they're part of the canonical SCOPE_REGISTRY (rbac_role:*, rbac_permission:read, rbac_user_role:*, rbac_constraint:*, rbac:evaluate) - Add TestRBACScopeRegistrySync test class that parses the RBAC seed migration and compares its SCOPE_DOCS against SCOPE_REGISTRY. This prevents drift where someone adds a scope without updating both locations, which could break the RBAC system. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
f5b41e3 to
7869909
Compare
RBAC management scopes don't need to be in the legacy SCOPE_REGISTRY since: - RBAC management only works when RBAC is enabled - When RBAC is enabled, permissions are checked against the database - The drift detection test only checks one direction (registry → migration) The RBAC scopes remain in the migration's SCOPE_DOCS to be seeded into the database, where they'll be used when RBAC is enabled. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
7327765 to
371fac1
Compare
Update the first RBAC migration (d9ee4ea46797) to depend on d304f57aea6d (stagedresourceancestor distance) instead of f85bd4c08401. This resolves the "Multiple head revisions" error caused by both migrations pointing to the same parent after merging main. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Ticket: N/A - Internal infrastructure for RBAC system
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 works