diff --git a/app/forms/project-access.tsx b/app/forms/project-access.tsx index 15566bc56..0752fbf8b 100644 --- a/app/forms/project-access.tsx +++ b/app/forms/project-access.tsx @@ -13,6 +13,7 @@ import { updateRole, useActorsNotInPolicy, useApiMutation, + type RoleKey, } from '@oxide/api' import { Access16Icon } from '@oxide/design-system/icons/react' @@ -88,7 +89,11 @@ export function ProjectAccessEditUserSideModal({ identityType, policy, defaultValues, -}: EditRoleModalProps) { +}: Omit & { + // roleName is optional so the form can open with no role selected for a user + // who has only a silo role today; choosing one adds a new project assignment + defaultValues: { roleName?: RoleKey } +}) { const { project } = useProjectSelector() const updatePolicy = useApiMutation(api.projectPolicyUpdate, { @@ -106,13 +111,15 @@ export function ProjectAccessEditUserSideModal({ form={form} formType="edit" resourceName="role" - title="Edit role" + title={`${defaultValues.roleName ? 'Edit' : 'Add'} project role`} subtitle={ {name} } onSubmit={({ roleName }) => { + // required validation prevents submitting without a selection + if (!roleName) return updatePolicy.mutate({ path: { project }, body: updateRole({ identityId, identityType, roleName }, policy), diff --git a/app/pages/project/access/ProjectAccessPage.tsx b/app/pages/project/access/ProjectAccessPage.tsx index bcf10c78b..f1622d0d5 100644 --- a/app/pages/project/access/ProjectAccessPage.tsx +++ b/app/pages/project/access/ProjectAccessPage.tsx @@ -20,6 +20,7 @@ import { roleOrder, useApiMutation, usePrefetchedQuery, + userRoleFromPolicies, useUserRows, type IdentityType, type RoleKey, @@ -34,6 +35,7 @@ import { ProjectAccessAddUserSideModal, ProjectAccessEditUserSideModal, } from '~/forms/project-access' +import { useCurrentUser } from '~/hooks/use-current-user' import { getProjectSelector, useProjectSelector } from '~/hooks/use-params' import { useQuickActions } from '~/hooks/use-quick-actions' import { confirmDelete } from '~/stores/confirm-delete' @@ -103,6 +105,17 @@ export default function ProjectAccessPage() { const { data: projectPolicy } = usePrefetchedQuery(projectPolicyView(projectSelector)) const projectRows = useUserRows(projectPolicy.roleAssignments, 'project') + // Changing a project role assignment requires `modify` on the project, i.e. + // the project `admin` role. Per the authz policy, that role is conferred + // either by a direct project admin role or by a silo collaborator/admin role + // (a silo collaborator is an admin on every project in the silo). + // https://github.com/oxidecomputer/omicron/blob/main/nexus/auth/src/authz/omicron.polar + const { me, myGroups } = useCurrentUser() + const myProjectRole = userRoleFromPolicies(me, myGroups.items, [projectPolicy]) + const mySiloRole = userRoleFromPolicies(me, myGroups.items, [siloPolicy]) + const canEditRoles = + myProjectRole === 'admin' || mySiloRole === 'admin' || mySiloRole === 'collaborator' + const rows = useMemo(() => { return groupBy(siloRows.concat(projectRows), (u) => u.id) .map(([userId, userAssignments]) => { @@ -166,15 +179,16 @@ export default function ProjectAccessPage() { ), }), - // TODO: tooltips on disabled elements explaining why getActionsCol((row: UserRow) => [ { - label: 'Change role', + // An admin can change any row's project role, even one that only has + // a silo role, because doing so adds a new project assignment. + label: `${row.projectRole ? 'Change' : 'Add'} project role`, onActivate: () => setEditingUserRow(row), disabled: - !row.projectRole && "You don't have permission to change this user's role", + !canEditRoles && + `You don't have permission to ${row.projectRole ? 'change' : 'add'} project roles`, }, - // TODO: only show if you have permission to do this { label: 'Delete', onActivate: confirmDelete({ @@ -194,11 +208,17 @@ export default function ProjectAccessPage() { ), resourceKind: 'role assignment', }), - disabled: !row.projectRole && "You don't have permission to delete this user", + disabled: !canEditRoles + ? "You don't have permission to remove roles" + : // There's no project assignment to delete; the role is inherited + // from the silo and can only be changed on the silo access page. + !row.projectRole + ? 'This role is inherited from the silo' + : undefined, }, ]), ], - [projectPolicy, projectSelector.project, updatePolicy] + [canEditRoles, projectPolicy, projectSelector.project, updatePolicy] ) const tableInstance = useReactTable({ @@ -239,13 +259,14 @@ export default function ProjectAccessPage() { policy={projectPolicy} /> )} - {projectPolicy && editingUserRow?.projectRole && ( + {projectPolicy && editingUserRow && ( setEditingUserRow(null)} policy={projectPolicy} name={editingUserRow.name} identityId={editingUserRow.id} identityType={editingUserRow.identityType} + // leave unselected when the user has no project role yet defaultValues={{ roleName: editingUserRow.projectRole }} /> )} diff --git a/test/e2e/project-access.e2e.ts b/test/e2e/project-access.e2e.ts index d4f34d8ce..4b39685b8 100644 --- a/test/e2e/project-access.e2e.ts +++ b/test/e2e/project-access.e2e.ts @@ -7,7 +7,14 @@ */ import { user3, user4 } from '@oxide/api-mocks' -import { expect, expectNotVisible, expectRowVisible, expectVisible, test } from './utils' +import { + expect, + expectNotVisible, + expectRowVisible, + expectVisible, + getPageAsUser, + test, +} from './utils' test('Click through project access page', async ({ page }) => { await page.goto('/projects/mock-project') @@ -47,6 +54,18 @@ test('Click through project access page', async ({ page }) => { `role=cell[name="Simone de Beauvoir"]`, ]) + // Hannah Arendt has only a silo role, so the action reads "Add project role" + // and the form opens with nothing preselected + await page + .getByRole('row', { name: 'Hannah Arendt', exact: false }) + .getByRole('button', { name: 'Row actions' }) + .click() + await page.click('role=menuitem[name="Add project role"]') + const editDialog = page.getByRole('dialog') + await expect(editDialog.getByRole('heading', { name: /Add project role/ })).toBeVisible() + await expect(editDialog.getByRole('radio', { checked: true })).toHaveCount(0) + await editDialog.getByRole('button', { name: 'Cancel' }).click() + // Add user 4 as collab await page.click('role=button[name="Add user or group"]') await expectVisible(page, ['role=heading[name*="Add user or group"]']) @@ -80,9 +99,9 @@ test('Click through project access page', async ({ page }) => { .locator('role=row', { hasText: user4.display_name }) .locator('role=button[name="Row actions"]') .click() - await page.click('role=menuitem[name="Change role"]') + await page.click('role=menuitem[name="Change project role"]') - await expectVisible(page, ['role=heading[name*="Edit role"]']) + await expectVisible(page, ['role=heading[name*="Edit project role"]']) // Verify Collaborator is currently selected await expect(page.getByRole('radio', { name: /^Collaborator / })).toBeChecked() @@ -116,3 +135,32 @@ test('Click through project access page', async ({ page }) => { Role: 'silo.admin+1', }) }) + +test('Non-admin cannot change or remove roles', async ({ browser }) => { + // Jacob Klein is only a project collaborator (not project admin or silo + // collaborator/admin), so he lacks `modify` on the project and can't change + // role assignments. Both row actions should be disabled. + const page = await getPageAsUser(browser, 'Jacob Klein') + await page.goto('/projects/mock-project/access') + + await expect(page.getByRole('heading', { name: /Access/ })).toBeVisible() + + await page + .getByRole('row', { name: 'Herbert Marcuse', exact: false }) + .getByRole('button', { name: 'Row actions' }) + .click() + + const changeRole = page.getByRole('menuitem', { name: 'Change project role' }) + await expect(changeRole).toBeDisabled() + await changeRole.hover() + await expect(page.getByRole('tooltip')).toHaveText( + "You don't have permission to change project roles" + ) + + const deleteItem = page.getByRole('menuitem', { name: 'Delete' }) + await expect(deleteItem).toBeDisabled() + await deleteItem.hover() + await expect(page.getByRole('tooltip')).toHaveText( + "You don't have permission to remove roles" + ) +})