Add RBAC management UI with alpha feature flag#7284
Add RBAC management UI with alpha feature flag#7284thabofletcher wants to merge 19 commits intomainfrom
Conversation
This commit adds the admin UI components for the new database-driven RBAC system: - Add rbacManagement feature flag (development/test only) - Add TypeScript types for RBAC roles, permissions, constraints, and evaluation - Add RBAC RTK Query slice with all API endpoints - Add RBAC tag types to api.slice.ts - Add RBAC routes to navigation config - Add RBAC pages: - /settings/rbac - Role management dashboard - /settings/rbac/roles/new - Create custom role - /settings/rbac/roles/[id] - Edit role and permissions The RBAC management feature is protected by the rbacManagement feature flag and requires Plus license. UI supports: - Viewing all roles (system and custom) - Creating new custom roles with optional parent inheritance - Editing role details and permissions - Permission matrix with inherited permissions display - Deleting custom roles (system roles protected) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
When the rbacManagement feature flag is enabled, the Users page now: - Fetches roles from the RBAC system instead of using legacy permissions - Creates RBAC user role assignments when saving - Removes RBAC role assignments when roles are deselected - Falls back to legacy permission system when flag is disabled Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Move useMemo hooks (initialValues, targetUserIsOwner) before early returns to comply with React's Rules of Hooks. Hooks must be called unconditionally and in the same order on every render. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The UserPermissionsCell component now checks the rbacManagement feature flag and fetches user roles from RBAC API instead of legacy permissions when enabled. Falls back to legacy permissions when disabled. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The updatePermissionsRbac function was not saving managed systems, causing system assignments to not persist. Added the same system saving logic that exists in updatePermissionsLegacy. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Change scopes from USER_PERMISSION_UPDATE/READ to USER_PERMISSION_ASSIGN_OWNERS to ensure only Owners can access the RBAC management UI. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Create new RolesForm.tsx that displays database roles (including custom roles) - Update UserManagementTabs to conditionally show "Roles" tab when RBAC enabled - Tab uses RolesForm (RBAC) or PermissionsForm (legacy) based on feature flag - Fix Tag color values to use valid fidesui color names - RolesForm shows role descriptions, system/custom badges, and handles role assignment Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add system management hooks (useGetUserManagedSystemsQuery, useUpdateUserManagedSystemsMutation) - Add AssignSystemsModal and AssignSystemsDeleteTable components - Show "Assign systems +" button when a role that supports system assignment is selected - Save system assignments along with role changes - Define ROLES_WITHOUT_SYSTEM_ASSIGNMENT for roles that cannot have systems assigned Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Stop event propagation on checkbox click to prevent double-toggle when clicking the checkbox (card onClick was also firing). Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add useGetMyRBACPermissionsQuery to rbac.slice.ts for fetching user's RBAC permissions - Update CommonSubscriptions to fetch RBAC permissions when rbacManagement flag is enabled - Update selectThisUsersScopes selector to prefer RBAC permissions when available This ensures the UI shows correct navigation items based on RBAC-derived permissions rather than legacy role-based permissions. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add read-only mode to SystemInformationForm for users without SYSTEM_UPDATE permission, preventing accidental API errors and providing clear UX feedback - Add warning alert in RBAC role detail page explaining that system:read permission currently grants access to ALL systems (no per-system filtering) - Wrap form fields in fieldset to automatically disable all inputs when read-only - Hide save button when user doesn't have update permissions Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add curly braces to single-line if statements - Replace for-of loops with Promise.all for async operations - Fix no-param-reassign by using spread operator in reducers - Add aria-labels to form controls for accessibility - Replace anchor element with Button for proper semantics - Escape apostrophe in JSX text - Fix prettier formatting issues Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The rbacManagement feature flag was set to test: true, which caused the UserManagementTabs component to display "Roles" instead of "Permissions". This broke existing cypress tests that look for the "Permissions" tab. Setting test: false ensures existing tests pass while RBAC development continues behind the feature flag. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Rename feature flag from rbacManagement to alphaRbac to follow alpha naming convention (like alphaPrivacyNoticesSandbox) - Remove "Management" from name since RBAC covers roles, permissions, and user assignments - not just role management - Remove "Fine-grained per-system access control is not yet implemented" sentence from system permissions alert Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Clarify that all system:* permissions (create, read, update, delete) apply globally, not just system:read. Point users to system_manager permissions for per-system access control. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
thabofletcher
requested review from
lucanovera
and removed request for
a team
Greptile OverviewGreptile SummaryThis PR adds an alpha-gated RBAC management UI to the admin client, including:
Most changes fit cleanly into existing patterns (feature-flagged nav, RTK Query baseApi injection, separate pages under Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
clients/admin-ui/src/features/user-management/user-management.slice.ts
Outdated
Show resolved
Hide resolved
1. Use proper RTK Query selector instead of fragile string-based cache key lookup in selectThisUsersScopes. No circular import risk exists. 2. Add "RBAC Roles" to providesTags for getMyRBACPermissions so user permissions are invalidated when role definitions change. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Addressed both Greptile comments in commit 94ef4f2:
|
| export const selectThisUsersScopes: (state: RootState) => ScopeRegistryEnum[] = | ||
| createSelector([(RootState) => RootState, selectUser], (RootState, user) => { | ||
| if (!user) { | ||
| return emptyScopes; | ||
| } | ||
| const permissions = userApi.endpoints.getUserPermissions.select(user.id)( | ||
| RootState, | ||
| ).data; | ||
| createSelector( | ||
| [(RootState) => RootState, selectUser, selectEnvFlags], | ||
| (RootState, user, flags) => { | ||
| // When RBAC is enabled, prefer RBAC-derived permissions | ||
| if (flags.alphaRbac) { | ||
| const rbacPermissions = | ||
| rbacApi.endpoints.getMyRBACPermissions.select(undefined)( | ||
| RootState, | ||
| )?.data; | ||
|
|
||
| if (rbacPermissions && rbacPermissions.length > 0) { | ||
| return rbacPermissions as ScopeRegistryEnum[]; | ||
| } |
There was a problem hiding this comment.
[P1] selectThisUsersScopes treats an empty RBAC permission set as “not loaded” and silently falls back to legacy permissions, which can grant UI access when RBAC is enabled but the user legitimately has 0 permissions (or the RBAC endpoint returns []). This is most likely for brand-new users/roles or when the RBAC service is up but returns no scopes.
Concretely, the gate if (rbacPermissions && rbacPermissions.length > 0) means [] will fall back to user/{id}/permission, potentially rendering screens based on legacy scopes even though RBAC is enabled.
Suggestion: distinguish “query not loaded / errored” from “loaded empty”, and if RBAC is enabled + query succeeded, return the RBAC permissions even if empty.
Also appears in: clients/admin-ui/src/features/common/CommonSubscriptions.tsx (RBAC prefetch assumes selector uses cache).
| // Use RBAC roles when enabled, otherwise fall back to legacy permissions | ||
| // Must be defined here (before early returns) to satisfy React's Rules of Hooks | ||
| const initialValues = useMemo(() => { | ||
| if (isRbacEnabled) { | ||
| return currentRbacRoleKeys.length > 0 | ||
| ? { roles: currentRbacRoleKeys as RoleRegistryEnum[] } | ||
| : defaultInitialValues; | ||
| } | ||
| return userPermissions?.roles | ||
| ? { roles: userPermissions.roles } | ||
| : defaultInitialValues; | ||
| }, [isRbacEnabled, currentRbacRoleKeys, userPermissions?.roles]); |
There was a problem hiding this comment.
[P1] In RBAC mode, initialValues casts role keys to RoleRegistryEnum[] (currentRbacRoleKeys as RoleRegistryEnum[]). This is unsound: RBAC supports custom roles whose key is not in RoleRegistryEnum, and even for system roles this relies on string compatibility. It can lead to form state/type mismatches and incorrect filtering/selection behavior.
This is most visible if a user has a custom role assigned: the form will still treat it as a RoleRegistryEnum and downstream logic (e.g., role filtering and RoleOption rendering keyed off ROLES) won’t represent it correctly.
Consider either (a) keeping a separate RBAC-only form value type of string[] role keys, or (b) gating this legacy form entirely when alphaRbac is on (since RolesForm exists).
| return permissions.reduce<Record<string, RBACPermission[]>>((acc, perm) => { | ||
| const resourceType = perm.resource_type || "general"; | ||
| const result = { ...acc }; | ||
| if (!result[resourceType]) { | ||
| result[resourceType] = []; | ||
| } | ||
| result[resourceType].push(perm); | ||
| return result; | ||
| }, {}); | ||
| }, [permissions]); |
There was a problem hiding this comment.
[P2] groupedPermissions rebuilds a new object on every reduce iteration via const result = { ...acc }, which is unnecessarily expensive and defeats the point of reducing.
You can safely mutate the accumulator in a reducer here (since it’s local) to avoid O(n^2) object copying. Also, this pattern makes it harder to preserve stable references for memoization.
This same pattern shows up in a few other new reducers/maps in this PR (e.g., building role maps) where mutation would be clearer and cheaper.
| { | ||
| title: "Actions", | ||
| key: "actions", | ||
| render: (_: unknown, record: RBACRole) => ( | ||
| <Space> | ||
| <Button | ||
| size="small" | ||
| onClick={() => router.push(`/settings/rbac/roles/${record.id}`)} | ||
| > | ||
| Edit | ||
| </Button> | ||
| {!record.is_system_role && ( | ||
| <Button | ||
| size="small" | ||
| danger | ||
| onClick={() => handleDeleteRole(record)} | ||
| loading={isDeleting} | ||
| > | ||
| Delete | ||
| </Button> | ||
| )} | ||
| </Space> |
There was a problem hiding this comment.
[P2] isDeleting is a single mutation-level loading flag but it’s used for every row’s Delete button. If you click delete on one role, all Delete buttons will show loading (and will stay loading if multiple deletes are triggered). This is confusing UX and can also allow accidental double-click deletes on other rows.
Consider tracking the deletingRoleId locally (or using originalArgs from the mutation state) so only the clicked row shows a spinner/disabled state.
| // Filter out system roles from parent options | ||
| const parentRoleOptions = useMemo(() => { | ||
| if (!allRoles) { | ||
| return []; | ||
| } | ||
| return allRoles.map((r) => ({ | ||
| label: `${r.name} (${r.key})`, | ||
| value: r.id, | ||
| })); | ||
| }, [allRoles]); |
There was a problem hiding this comment.
[P2] Comment says “Filter out system roles from parent options”, but the implementation includes all roles (return allRoles.map(...)). If system roles should not be selectable as parents, this currently violates that rule.
This becomes user-visible when creating a custom role: system roles will appear in the Parent Role dropdown despite the comment/intent.
If system roles are actually allowed as parents, the comment should be corrected to avoid misleading future changes.
Additional Comments (2)
Even if flags typically change only on reload, the codebase supports toggling flags via localStorage; this makes the missing dependency observable. Suggestion: include
Prefer closing only the toast(s) you created for this submission, or avoid closing all toasts globally. |
1 participant
Ticket: N/A - Internal infrastructure for RBAC system
Description Of Changes
Adds the admin UI components for the new database-driven RBAC system, protected by the
alphaRbacfeature flag (disabled by default).Related PRs (merge order):
Testing: Full end-to-end testing should be done with fidesplus#3028 after PRs #7296 and #7285 are merged.
Safe to merge independently: Yes. The UI is protected by the
alphaRbacfeature flag which is disabled by default (development: true,test: false,production: false). Users will not see any changes until the flag is explicitly enabled.Code Changes
alphaRbacfeature flag (development only, follows alpha naming convention)/api/v1/plus/rbac/*)/settings/rbac- Role management dashboard/settings/rbac/roles/new- Create custom role/settings/rbac/roles/[id]- Edit role and permissionsUserManagementTabs.tsx- Shows "Roles" tab instead of "Permissions" when RBAC enabledRolesForm.tsx- New role assignment form using RBAC APIsuseUserManagementTable.tsx- Displays RBAC roles in user tablePermissionsForm.tsx- Conditionally hides legacy permission selectorCommonSubscriptions.tsxintegration to prefetch RBAC permissionsSteps to Confirm
flags.jsonhasalphaRbac.test: falsecd clients && npm run test:cicd clients && npm run lint:fixTo test with RBAC enabled:
FIDESPLUS__RBAC__ENABLED=truePre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works