fix: allow users with a systemRole.grant = true to update user roles

[finnar-bin]

Description

Allows users with a systemRole.grant = true to update user roles. Originally it only allows users with a role name of Admin or Owner which makes it not flexible and doesn't take into consideration created custom roles.

Fixes #2514

Type of change

• Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

• Manual Test
• Unit Test
• E2E Test

Screenshots / Screen recording

Screencast_20251111_100718.webm

[github-actions]

Review Summary

The rename from isInstanceOwner → canUpdateUsers and the switch to systemRole.grant is a sensible, well-scoped fix for the custom-admin-role case. A few notes (left inline):

Correctness

• ?.role.systemRole.grant only optional-chains through the find() result. If the matched user object lacks role or role.systemRole, this throws at runtime. Worth chaining all the way through.

Scope / consistency

• The same helpers.isInstanceOwner check still gates apis.js, teams.js, and webhooks.js. Custom admin-grant roles will continue to hit the same bug there. Consider updating the shared helper instead of inlining the new logic, so all four pages benefit.
• PendingTable receives canUpdateUsers but the destructure is commented out — pre-existing, but a good time to either wire it up or drop the prop.

Other

• Cleanup of the now-unused helpers import is good.
• The useMemo deps look correct (instanceUserWithRoles, userInfo).
• No security/perf concerns beyond what's noted above.

Nothing blocking — happy to see this go in once the optional-chaining and (ideally) the shared-helper question are addressed.

[github-actions]

Review summary

Blocking

• Syntax error in src/pages/instances/[zuid]/users.js (lines 145–149) — the find() call and const updateUsersPermission declaration are duplicated with mismatched parentheses. The file will not parse, so the build is broken. Inline suggestion left on the diff. Please run npm run check-lint / npm run build locally before re-requesting review — this would have been caught immediately by either.

Non-blocking, but worth considering

• Same bug exists in adjacent pages. The permission helper at src/utils/index.js:210 (isInstanceOwner) still gates the role-management UI in src/pages/instances/[zuid]/teams.js, apis.js, and webhooks.js, and it still uses the name-based 'Admin'/'Owner' check that 🐛 Account UI: custom roles with admin base role can't update another user's role #2514 reports as broken. Users with a custom role where systemRole.grant = true will continue to be locked out of those screens. Consider either fixing isInstanceOwner itself (and renaming it, e.g. canManageInstanceUsers) so all four pages share one source of truth, or filing a follow-up to cover the others. Inlining the check in users.js only leaves the logic divergent across four pages.

• Consider a unit test. The PR description notes "Manual Test" only. Given this is a permissions check with two failure modes (the original 'Admin'/'Owner' regression, and now this PR's syntax break), a small unit test on the memoized predicate would protect against both.

Looks good

• Renaming the prop from isOwner → canUpdateUsers across Users.js is consistent and reads better given the new semantics.
• Falling back to userInfo.staff preserves the existing staff-override behavior from isInstanceOwner.
• The early-return guard on !userInfo || !instanceUserWithRoles?.length is good — avoids running find on an empty array during initial render.

[github-actions]

Review Summary

Nice, focused fix — switching from a hard-coded Owner/Admin name check to the underlying systemRole.grant permission is the right direction and is the correct semantic gate for "can manage other users' roles." Manual test video confirms the happy path. A few things to consider:

Scope inconsistency across the app

The same helpers.isInstanceOwner (name-based) check is still used on the sibling pages: src/pages/instances/[zuid]/webhooks.js:167, src/pages/instances/[zuid]/teams.js:109, and src/pages/instances/[zuid]/apis.js:142. Those pages will still mis-gate users with custom Admin/Owner-based roles. If the goal of this fix is to unblock custom roles broadly, it would be worth either following up on those pages or being explicit in the PR that this scope is intentionally narrow (since grant is specifically about granting roles, while those other pages gate different actions and may need different systemRole.* checks like update/create/delete).

A medium-term cleanup might be to replace utils/isInstanceOwner with a small set of permission helpers driven by systemRole flags rather than name strings, so this pattern doesn't get repeated.

Backward compatibility

Worth confirming with the API that the seeded Owner and Admin system roles have grant: true — otherwise this is a regression for existing owners/admins. Based on the PR description it sounds like you've validated this manually; might be worth a unit test around the memo for the common shapes (admin, owner, custom-with-grant, custom-without-grant, staff, unauthenticated/empty) so this doesn't regress silently.

Minor inline notes

• Variable naming (updateUsersPermission) and a Boolean(...) wrap on the memo return — see inline comment.
• RoleSwitcher still does a string match on 'Owner', so custom roles backed by the Owner base role would render as editable — see inline comment.
• Leftover // canUpdateUsers, comment in PendingTable destructure suggests the pending-invites table was meant to be gated too but isn't — see inline comment.

Security

No new auth surface introduced — the client is just deciding what UI affordances to show; the API still enforces. That said, please double-check that the backend authorizes updateUserRole / deleteUserRole calls based on the same grant permission, so that the UI gate and server gate stay in sync.

[github-actions]

Two small issues with this memoized value:

1. The return can be undefined instead of false. If updateUsersPermission is undefined (user not found in instanceUserWithRoles, or no systemRole) and userInfo.staff is also undefined, the expression undefined || undefined returns undefined. It coerces to falsy in JSX but it's safer to return a strict boolean so downstream ?:/equality checks behave predictably:

   return Boolean(updateUsersPermission || userInfo.staff);

2. The name canUpdateUsers is a bit narrow — this same flag also gates the Delete User action (Users.js:194) and the entire popover menu choice (Users.js:211). Consider canManageUsers to better reflect the scope, or align with the underlying permission name (grant).

[github-actions]

This commented-out destructure can be removed. The parent passes canUpdateUsers={canUpdateUsers} to PendingTable (line 363) but the component never reads it — the prop is unused on both ends. Either destructure and use it (e.g. gate the "Cancel Invite" action below at line 436 the same way Delete User is gated), or drop the prop pass-through entirely. Leaving a stale comment and dead prop is misleading for future readers.

[github-actions]

Review summary

Nice, focused fix — replacing the hard-coded Owner/Admin name check with the underlying systemRole.grant permission is the right direction for supporting custom roles, and the rename from isOwner → canUpdateUsers is consistently propagated through Users.js.

A few things to consider:

Same bug class exists in sibling pages

The legacy helpers.isInstanceOwner (in src/utils/index.js:210) is still in use on three other instance pages, each of which will exhibit the same limitation this PR fixes:

• src/pages/instances/[zuid]/apis.js:142
• src/pages/instances/[zuid]/teams.js:109
• src/pages/instances/[zuid]/webhooks.js:167

A user with a custom role that has systemRole.grant = true but a non-Owner/Admin name still can't manage APIs, teams, or webhooks. Consider either (a) updating helpers.isInstanceOwner itself to also honor systemRole.grant (smallest change, fixes all callers in one place), or (b) following up with the same pattern in those pages. Option (a) is appealing because it keeps the logic in one place and would let you drop the new inline useMemo here in favor of the helper.

Permission semantics

The new check uses role.systemRole.grant. Make sure that's the right field for "can manage users" rather than something more specific (e.g. grant may semantically mean "can grant roles" which is roughly equivalent, but worth confirming against the Zesty auth model since this same flag now controls both the role dropdown and the delete-user action).

Test coverage

Per the PR description this is manual-test only. Given this is a permissions check, a small unit test around the canUpdateUsers predicate (or the shared helper if you go with option (a)) would protect against future regressions for: custom-role-with-grant=true, custom-role-with-grant=false, staff override, and the user-not-in-roles edge case.

Inline comments left on specific lines for the smaller nits.

[github-actions]

Heads-up on a residual gap: RoleSwitcher (lines 34–48, above) still gates editability by hard-coded role name (case 'Owner'). After this PR, a custom role with systemRole.grant = true will see the editable dropdown for every user — including users whose role is named "Owner" — and could attempt to demote them. Whether the API rejects that or not, the UI behavior should probably match the new permission model (e.g. also block edits where the target row's role.systemRole.name === 'Owner' or role.systemRole.super is true). Not blocking for this PR, but worth a follow-up.