Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
finnar-bin wants to merge 5 commits into
base: stage
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e842866
fix: allow users with a base role of admin or owner to change user roles
finnar-bin Nov 11, 2025
6cd3f8d
Merge branch 'stage' into fix/2514-enable-role-updates-for-custom-rol…
finnar-bin May 27, 2026
f3c17e1
Update src/pages/instances/[zuid]/users.js
finnar-bin May 27, 2026
3d2b7b3
Update src/pages/instances/[zuid]/users.js
finnar-bin May 27, 2026
18fac4b
Merge branch 'stage' into fix/2514-enable-role-updates-for-custom-rol…
finnar-bin Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions src/pages/instances/[zuid]/users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { useZestyStore } from 'store';
import { useRouter } from 'next/router';
import { Users } from 'views/accounts';
import { ErrorMsg, SuccessMsg } from 'components/accounts';
import * as helpers from 'utils';
import InstanceContainer from 'components/accounts/instances/InstanceContainer';

export { default as getServerSideProps } from 'lib/accounts/protectedRouteGetServerSideProps';
Expand Down Expand Up @@ -138,10 +137,17 @@ export default function UsersPage() {
}
}, [router.isReady]);

const isInstanceOwner = helpers.isInstanceOwner(
instanceUserWithRoles,
userInfo,
);
const canUpdateUsers = React.useMemo(() => {
if (!userInfo || !instanceUserWithRoles?.length) {
return false;
}

const updateUsersPermission = instanceUserWithRoles.find(
(user) => user.ZUID === userInfo.ZUID,
)?.role?.systemRole?.grant;

return updateUsersPermission || userInfo.staff;
}, [instanceUserWithRoles, userInfo]);
Comment thread
finnar-bin marked this conversation as resolved.
Comment thread
finnar-bin marked this conversation as resolved.
Comment on lines +140 to +150

@github-actions github-actions Bot Jun 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


const filteredUsers = instanceUserWithRoles?.filter((e) => {
const name = `${e?.firstName?.toLowerCase() || '-'} ${
Expand All @@ -155,7 +161,7 @@ export default function UsersPage() {
deleteUserRole,
instanceRoles,
createInvite,
isOwner: isInstanceOwner,
canUpdateUsers,
instanceZUID: zuid,
loading,
search,
Expand Down
19 changes: 11 additions & 8 deletions src/views/accounts/instances/Users.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const CustomTable = ({
handleUpdateRole,
handleDeleteRole,
instanceRoles,
isOwner,
canUpdateUsers,
loading,
}) => {
const ROWS = data?.map((e) => {
Expand Down Expand Up @@ -120,7 +120,7 @@ const CustomTable = ({
handleUpdateRole(val);
};

const role = isOwner
const role = canUpdateUsers
Comment thread
finnar-bin marked this conversation as resolved.

@github-actions github-actions Bot Jun 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

? RoleSwitcher({
role: e.role.name,
handleOnChange,
Expand Down Expand Up @@ -189,7 +189,10 @@ const CustomTable = ({
},
];
const actionOwner = [
{ title: 'Delete User', action: isOwner ? handleDeleteUser : null },
{
title: 'Delete User',
action: canUpdateUsers ? handleDeleteUser : null,
},
{
title: 'Email',
action: () => window.open(`mailto:${params.row.email}`),
Expand All @@ -205,7 +208,7 @@ const CustomTable = ({
</Button>
}
id={'actions'}
items={isOwner ? actionOwner : action}
items={canUpdateUsers ? actionOwner : action}
colorInvert={false}
/>
</>
Expand Down Expand Up @@ -276,7 +279,7 @@ const Index = ({
deleteUserRole,
instanceRoles,
createInvite,
isOwner,
canUpdateUsers,
instanceZUID,
loading,
search,
Expand Down Expand Up @@ -347,7 +350,7 @@ const Index = ({
handleUpdateRole={handleUpdateRole}
handleDeleteRole={handleDeleteRole}
instanceRoles={instanceRoles}
isOwner={isOwner}
canUpdateUsers={canUpdateUsers}
loading={loading}
/>
</Grid>
Expand All @@ -357,7 +360,7 @@ const Index = ({
data={pendingUsers}
instanceRoles={instanceRoles}
respondToInvite={respondToInvite}
isOwner={isOwner}
canUpdateUsers={canUpdateUsers}
loading={loading}
/>
</Grid>
Expand All @@ -369,7 +372,7 @@ export const Users = React.memo(Index);
const PendingTable = ({
data,
instanceRoles,
// isOwner,
// canUpdateUsers,
Comment thread
finnar-bin marked this conversation as resolved.
Comment thread
finnar-bin marked this conversation as resolved.

@github-actions github-actions Bot Jun 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

loading,
respondToInvite,
}) => {
Expand Down
Loading