Repository navigation
Add full User and Groups lists to Access page - #3211
charliepark wants to merge 86 commits into
Conversation
…nks to direct paths
…ilo, and group names
|
The warning here about removing your own silo access could be worded more clearly and made more visible, maybe with a red or yellow message box. @fakemonster also suggests the possibility of making the user type something. It really is an action you will usually want to avoid, so I think I like that. Joe also noticed that the case where you remove it from a group you're in (as opposed to a direct assignment) doesn't get a warning. 
|
|
Like we talked about, I think it's a blocker that when a user has a role both directly and from a group, "remove silo role" and "change silo role" are very misleading about what will happen when you click — they will in fact keep that role because it's from the group. We decided to try removing those from the row menu and only let you manipulate the assignments in the detail modal, where it's clear which one you are acting on. Change and remove can be actions on the direct assignment. For the group assignment, not sure whether it should have the row actions but disable them with a message explaining that you have to go the group instead, or maybe it doesn't have the actions at all. Probably the former I guess for symmetry. I think I'd also like the group name in the list to be a link that takes you to the group detail, so you can go make changes there if you want. 
|
|
The CI test will fail until I merge this branch, but holding off until we're sure it's the right path, to reduce noise on Omicron. https://github.com/oxidecomputer/omicron/tree/routes-for-access-v2 |
|
Nexus updates are in oxidecomputer/omicron#11700 |
|
Have added a modal to highlight the policy at the silo / project level: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| export function AccessUsersTab({ scope }: { scope: PolicyScope }) { | ||
| const { siloPolicy, projectPolicy, userPath } = scope | ||
|
|
||
| const { data: users } = usePrefetchedQuery(userListAll) |
There was a problem hiding this comment.
🔴 Silo and project admins can no longer remove a role assignment held by an identity missing from the user/group lists, such as a deleted user. The Users and Groups tabs build rows only from userListAll/groupListAll, which are capped at ALL_ISH. Unresolved assignments appear only in the read-only PolicyModal, which has no actions. Fix: give every policy assignment a remove action. For example, add a Remove row action in PolicyModal, or list unresolved assignments in the tabs with an ID fallback.
Why this was flagged
Trigger: a policy assignment whose identityId is not returned by userList/groupList with limit: ALL_ISH. That covers a user or group removed from the IdP, or one past the 1000-item cap. PolicyModal.tsx even shows a tooltip for this case: "Can't resolve name because the user is no longer in this silo". AccessUsersTab.tsx:51-58 and AccessGroupsTab.tsx:53-58 build rows only from those lists, so such an assignment gets no row and no remove action. The detail route's userView/groupView fails for a deleted identity, so that path is closed too. PolicyModal (app/components/access/PolicyModal.tsx) lists the assignment with no actions. On the base branch, SiloAccessPage and ProjectAccessPage built rows from policy.roleAssignments via useUserRows, falling back to the ID. Each row had an enabled Delete action calling deleteRole. After this change an admin cannot remove the stale assignment anywhere in the console.
Verification: Trigger: a silo or project policy holds an assignment whose identityId is not in userList/groupList at limit: ALL_ISH. Base roles.ts:131 falls back to the raw ID. AccessUsersTab.tsx:54-58 builds rows only from usePrefetchedQuery(userListAll). PolicyModal.tsx:71-119 does list unresolved assignments, but it is read-only and has no actions.
There was a problem hiding this comment.
Two parts to this. One is that we now have a "remove {silo/project} role" action button when the user has been deleted and left an orphan role:
(open to alternate copy ideas)
The other is that I have code in oxidecomputer/omicron#11713 that will clean up some of that data, and a possible migration that would go through and clean up other orphaned data so the concern highlighted here wouldn't actually come up.
I'm not marking this particular comment as resolved as I still want to talk through it with Crespo/Joe.
| await Promise.all([ | ||
| queryClient.fetchQuery(groupListAll).then((groups) => { | ||
| groups.items.forEach((g) => | ||
| queryClient.prefetchQuery(groupMemberList({ groupId: g.id })) | ||
| ) |
There was a problem hiding this comment.
🔴 Admins of silos with many groups now trigger one userList request per group on every Access page visit and every user-detail open. One failed request leaves every user's role unreadable and uneditable. The loader fires groupMemberList for each group, and the Users tab and user detail mount useQueries over all groups. The global staleTime is 2000 ms with retry: false, so any observer mounting after 2 s refetches all G lists. Fix: give groupMemberList a long staleTime, invalidated only when memberships could change, so opening a user detail reuses the cached lists. A failed group should only mark the users it covers as unknown, not the whole map.
Why this was flagged
The trigger is a silo with G groups, up to ALL_ISH = 1000, all JIT-provisioned from SAML. prefetchUsersAndGroups (app/components/access/policy-scope.ts:49-53) calls prefetchQuery(groupMemberList(...)) for every group. useGroupsByUserId (app/api/roles.ts:241) mounts G observers in AccessUsersTab, SiloUserDetail.tsx:48 and ProjectUserDetail.tsx:47, and the Groups tab mounts one MemberCountCell per group. app/api/client.ts:73-74 sets retry: false and staleTime: 2000, so each new observer refetches its stale query. Clicking a user name more than 2 s after the page loaded re-fires all G requests. useGroupsByUserId returns { status: 'error' } if any single result errors. The Users tab then shows Error in every Role/Groups cell. UserDetailsSideModal shows "Could not load group memberships", with no Change/Remove/Add actions. The base silo and project Access pages issued a fixed 3-4 requests and their role actions did not depend on membership fetches.
Verification: prefetchUsersAndGroups (app/components/access/policy-scope.ts:49-53) prefetches groupMemberList for every group, which is G requests per visit that the base did not make. With retry: false, one failed member request makes the whole map { status: 'error' } (roles.ts:245). In AccessUsersTab.tsx:80-83, every user's Role and Groups cell becomes <ErrorCell />.
| export function useCanEditPolicy(siloPolicy: Policy, projectPolicy?: Policy): boolean { | ||
| const { me, myGroups } = useCurrentUser() | ||
| if (!projectPolicy) return me.siloAdmin | ||
| const entries = userScopedRoleEntries(me.id, myGroups.items, siloPolicy, projectPolicy) |
There was a problem hiding this comment.
🟡 (optional) Project editors whose admin or collaborator access comes from a group past the first page of their groups will find every project role action disabled. useCanEditPolicy builds the project permission check from myGroups.items. That list is currentUserGroups fetched with no limit, so it holds only Nexus's default first page. Fix: compute the gate from the full membership, for example by fetching currentUserGroups with limit: ALL_ISH or using useGroupsByUserId for me.id, or leave actions enabled and let the API reject unauthorized changes, as FleetAccessPage does.
Why this was flagged
The trigger is a user in more groups than Nexus's default page size, where the group that grants project admin or silo collaborator/admin sorts past page 1 by id. I could not count how many users this affects. Groups are IdP-provisioned, so heavy AD/SAML users are the likely population. myGroups comes from q(api.currentUserGroups, {}) in app/hooks/use-current-user.ts:19 and app/layouts/AuthenticatedLayout.tsx:30, with no limit. app/components/access/use-can-edit-policy.ts:37 passes myGroups.items to userScopedRoleEntries, and canEditScope then returns false. UserDetailsSideModal and GroupMembersSideModal feed that to roleActions(managedScope, canEdit), which disables Add/Change/Remove with "You don't have permission to ... project roles". The base ProjectAccessPage never gated these actions and left enforcement to the API, so this user could edit before the change. Meanwhile the Users tab resolves their memberships through useGroupsByUserId with ALL_ISH and still shows the role that grants access.
Verification: app/hooks/use-current-user.ts:19 fetches myGroups with no limit, so myGroups.items holds only the first page of /v1/me/groups. app/components/access/use-can-edit-policy.ts:37 passes myGroups.items to userScopedRoleEntries. roleActions.tsx:43 and :75 disable the add, change and remove actions. The base ProjectAccessPage did not gate edits on the viewer's own permissions.
|
Prompted by the Claude review from yesterday, I also have some work in oxidecomputer/omicron#11713 looking at doing a better job cleaning up role rows when deleting users and groups. |
Currently, the Silo Access and Project Access page don't show all users, except when you open up the "Add user or group" form and access the dropdown:
In issue #2887, @askfongjojo requested a view where all users and groups would be displayed. This PR does that, adding a tabbed interface to show Groups / Users, and, then, on those pages, shows all groups and users.
From this tabbed view, an admin can make edits to a specific user / group's permissions, as before:
One ongoing situation that I want to mention is that we don't have a way to order / filter the responses from the API, and the default ordering from the API is by User ID. Instead of having pagination with 50 users per page, I have it selecting all-ish users (1,000) and then sorting them by name on the client. This deviates from the vanilla API response, but gives, I think, a more useful listing of users. I'm open to other approaches, if people have suggestions. I checked with Will about whether that 1,000 user cap would be a problem, and he said that in the far future it might be something to address, but that there are no current customers where that limit would be problematic, based on how they configure users/silos.
Closes #2887
Closes #1146
Closes #1359