feat(auth): gate staff features on staffPermissions instead of role names - #1623
Conversation
…ames /User/current has returned a staffPermissions array since the API's granular staff roles landed, but the UI still gated everything on fabricRole === 'fabric_admin' — narrower staff roles saw nothing, and admin mode promised actions (org/cluster delete) the API has since withheld. hasStaffPermission() reads the array (falling back to the old role-name behavior when the API predates the field) and every staff surface now names the permission it needs. The /admin section admits any account holding at least one of its pages' permissions; the API Token page stays hidden from super_user, whose password sessions can't satisfy the mint's SSO requirement. Closes #1622 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request transitions the frontend authorization model from coarse-grained role checks (such as isFabricAdmin and useAdminMode) to a fine-grained, permission-based system using a new staffPermissions array. It introduces the hasStaffPermission and useStaffPermission hooks, updates UI components (like the Navbar, AdminShell, OrgCard, and Regions index) to gate features based on specific permissions, and refactors permission hooks to union staff permissions with organization membership permissions. Comprehensive unit tests have also been added to verify the new permission-checking logic. I have no feedback to provide as there are no review comments.
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Sounds good. A few codex suggestions if you are interested, while this sits in the queue.
🤖 Reviewed with Codex
| const [search, setSearch] = useState(''); | ||
| // The page itself only needs region:read; creating/editing posts to the | ||
| // region:write-gated endpoints. | ||
| const canWriteRegions = useStaffPermission('region:write'); |
There was a problem hiding this comment.
A user with only region:read is now intentionally admitted to this page, but the component unconditionally runs getOrganizationsQueryOptions(), whose /Admin/Organization/ request is gated by org:read. The explicit fabric_readonly test case therefore gets a global 403 toast on page load, loses organization names, and cannot filter scopes by name. Please either require org:read for this page or conditionally enable the organization query and provide a read-only fallback for region-only users; the modal should apply the same rule. — KrAIs (GPT-5)
| // straight to an org from an id copied out of a log or ticket. | ||
| const searchEntityId = useMemo(() => { | ||
| if (!isAdminMode) { | ||
| if (!canReadAllOrgs) { |
There was a problem hiding this comment.
This single org:read check also enables pasted cluster-ID searches, but that branch first calls /Cluster/{id} before looking up the organization. An account with org:read but no cluster:read will therefore get a 403 instead of the promised exact-ID lookup. Please gate cluster-ID detection on cluster:read as well, or resolve the cluster through an organization-admin endpoint authorized by org:read. — KrAIs (GPT-5)
| // instance:create / instance:delete permissions), so those verbs follow | ||
| // the cluster grants. | ||
| create: hasStaffPermission(user, 'cluster:update') || member.create, | ||
| remove: hasStaffPermission(user, 'cluster:delete') || member.remove, |
There was a problem hiding this comment.
The adjacent comment correctly says adding and removing instances happens through cluster updates, but remove is mapped to cluster:delete. That denies instance removal to staff who can update a cluster and grants it to staff who may delete a cluster but cannot update it; the latter operation would then be rejected by the update endpoint. Please map both instance create and remove to cluster:update and add the symmetric test assertion. — KrAIs (GPT-5)
3 participants
Closes #1622
What
GET /User/currentreturns astaffPermissionsarray describing exactly what a staff account may do, but the UI never read it — staff gating was hardcoded tofabricRole === 'fabric_admin'(plussuper_user). That was wrong in both directions: the narrower staff roles saw no staff features at all, and admin mode rendered actions the API has since withheld fromfabric_admin(org delete, cluster delete), which 403 on click.This replaces every role-name gate with the specific permission its surface needs, via a new
hasStaffPermission(user, permission):org:readbilling:writeorg:delete/org:update(unioned with the member role)role:read/create/update/deletecluster:read/create/update/deleteinstance:read/update; create/remove follow the cluster grants (instances are added/removed through cluster updates)instance:updatesystemStatus:writeregion:read(page),region:write(create/edit buttons)apiToken:createBehavior notes
usePermissionshooks union, not shadow: a staff account that is also an org member keeps whatever its member role grants, and vice versa — matching the API's own semantics./adminindex redirect is unchanged.isFabricAdmingate encoded, but per-page instead of hiding the whole section.staffPermissionsis absent from the response,hasStaffPermissionfalls back to the legacy role-name check, so deploying this UI ahead of the API changes nothing.isAdminMode/useAdminMode/isFabricAdminare gone — nothing references role names for gating anymore.Testing
hasStaffPermissionunit tests (granted/withheld/customer/local/legacy-fallback),/adminvisibility per role incl. the super_user carve-out, and cluster/instance hook mapping + union tests.stagebaseline plus the new tests.tsc, oxlint, and dprint clean.🤖 Generated with Claude Code