diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 2298f6851..0a54ecde5 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -dc2c66961be81b524836782ba24c60eee410f039 \ No newline at end of file +1beda0b8399a302c0efa1af1226a649180b01bd9 \ No newline at end of file diff --git a/app/pages/project/images/ImagesPage.tsx b/app/pages/project/images/ImagesPage.tsx index 637869b2e..60b9cca28 100644 --- a/app/pages/project/images/ImagesPage.tsx +++ b/app/pages/project/images/ImagesPage.tsx @@ -58,9 +58,6 @@ export function ImagesPage() { addToast({ content: `${variables.path.image} has been deleted` }) queryClient.invalidateQueries('imageList') }, - onError: (err) => { - addToast({ title: 'Error', content: err.message, variant: 'error' }) - }, }) const makeActions = (image: Image): MenuAction[] => [ diff --git a/app/pages/system/SiloImagesPage.tsx b/app/pages/system/SiloImagesPage.tsx index a1c639ebd..adefbd5f8 100644 --- a/app/pages/system/SiloImagesPage.tsx +++ b/app/pages/system/SiloImagesPage.tsx @@ -30,6 +30,8 @@ import { import { ListboxField, toListboxItem } from 'app/components/form' import { useForm, useToast } from 'app/hooks' +import { confirmDelete } from 'app/stores/confirm-delete' +import { addToast } from 'app/stores/toast' import { pb } from 'app/util/path-builder' const EmptyState = () => ( @@ -53,11 +55,26 @@ export function SiloImagesPage() { const [demoteImage, setDemoteImage] = useState(null) + const queryClient = useApiQueryClient() + const deleteImage = useApiMutation('imageDelete', { + onSuccess(_data, variables) { + addToast({ content: `${variables.path.image} has been deleted` }) + queryClient.invalidateQueries('imageList') + }, + }) + const makeActions = (image: Image): MenuAction[] => [ { label: 'Demote', onActivate: () => setDemoteImage(image), }, + { + label: 'Delete', + onActivate: confirmDelete({ + doDelete: () => deleteImage.mutateAsync({ path: { image: image.name } }), + label: image.name, + }), + }, ] return ( diff --git a/app/test/e2e/authz.e2e.ts b/app/test/e2e/authz.e2e.ts index 03122c077..701eaee6e 100644 --- a/app/test/e2e/authz.e2e.ts +++ b/app/test/e2e/authz.e2e.ts @@ -5,7 +5,7 @@ * * Copyright Oxide Computer Company */ -import { expect, getDevUserPage, test } from './utils' +import { expect, getPageAsUser, test } from './utils' test.describe('Silo/system picker', () => { test('appears for fleet viewer', async ({ page }) => { @@ -17,7 +17,7 @@ test.describe('Silo/system picker', () => { }) test('does not appear to for dev user', async ({ browser }) => { - const page = await getDevUserPage(browser) + const page = await getPageAsUser(browser, 'Hans Jonas') await page.goto('/projects') await expect(page.getByRole('link', { name: 'SILO maze-war' })).toBeVisible() await expect( @@ -27,7 +27,7 @@ test.describe('Silo/system picker', () => { }) test('dev user gets 404 on system pages', async ({ browser }) => { - const page = await getDevUserPage(browser) + const page = await getPageAsUser(browser, 'Hans Jonas') await page.goto('/system/silos') await expect(page.getByText('Page not found')).toBeVisible() diff --git a/app/test/e2e/images.e2e.ts b/app/test/e2e/images.e2e.ts index 12baef4b8..d1aebf32e 100644 --- a/app/test/e2e/images.e2e.ts +++ b/app/test/e2e/images.e2e.ts @@ -13,6 +13,7 @@ import { expect, expectNotVisible, expectVisible, + getPageAsUser, } from './utils' test('can promote an image from silo', async ({ page }) => { @@ -124,6 +125,9 @@ test('can demote an image from silo', async ({ page }) => { test('can delete an image from a project', async ({ page }) => { await page.goto('/projects/mock-project/images') + const cell = page.getByRole('cell', { name: 'image-3' }) + await expect(cell).toBeVisible() + await clickRowAction(page, 'image-3', 'Delete') const spinner = page.getByRole('dialog').getByLabel('Spinner') await expect(spinner).toBeHidden() @@ -131,7 +135,49 @@ test('can delete an image from a project', async ({ page }) => { await expect(spinner).toBeVisible() // Check deletion was successful - await expectVisible(page, ['text="image-3 has been deleted"']) - await expectNotVisible(page, ['role=cell[name="image-3"]']) + await expect(page.getByText('image-3 has been deleted', { exact: true })).toBeVisible() + await expect(cell).toBeHidden() + await expect(spinner).toBeHidden() +}) + +test('can delete an image from a silo', async ({ page }) => { + await page.goto('/images') + + const cell = page.getByRole('cell', { name: 'ubuntu-20-04' }) + await expect(cell).toBeVisible() + + await clickRowAction(page, 'ubuntu-20-04', 'Delete') + const spinner = page.getByRole('dialog').getByLabel('Spinner') + await expect(spinner).toBeHidden() + await page.getByRole('button', { name: 'Confirm' }).click() + await expect(spinner).toBeVisible() + + // Check deletion was successful + await expect( + page.getByText('ubuntu-20-04 has been deleted', { exact: true }) + ).toBeVisible() + await expect(cell).toBeHidden() + await expect(spinner).toBeHidden() +}) + +// this is to some extent a test of our mock server implementation, but I want +// to check the error handling as well because we expect people to run into this +test("Silo viewer can't delete silo image", async ({ browser }) => { + const page = await getPageAsUser(browser, 'Simone de Beauvoir') + + await page.goto('/images') + + const cell = page.getByRole('cell', { name: 'ubuntu-20-04' }) + await expect(cell).toBeVisible() + + await clickRowAction(page, 'ubuntu-20-04', 'Delete') + const spinner = page.getByRole('dialog').getByLabel('Spinner') + await expect(spinner).toBeHidden() + await page.getByRole('button', { name: 'Confirm' }).click() + await expect(spinner).toBeVisible() + + // Check deletion was successful + await expect(page.getByText('Could not delete resource', { exact: true })).toBeVisible() + await expect(cell).toBeVisible() await expect(spinner).toBeHidden() }) diff --git a/app/test/e2e/utilization.e2e.ts b/app/test/e2e/utilization.e2e.ts index 7a2876a6b..6a14d1394 100644 --- a/app/test/e2e/utilization.e2e.ts +++ b/app/test/e2e/utilization.e2e.ts @@ -10,7 +10,7 @@ import { expectNotVisible, expectRowVisible, expectVisible, - getDevUserPage, + getPageAsUser, test, } from './utils' @@ -46,7 +46,7 @@ test.describe('System utilization', () => { }) test('does not appear for dev user', async ({ browser }) => { - const page = await getDevUserPage(browser) + const page = await getPageAsUser(browser, 'Hans Jonas') await page.goto('/system/utilization') await expect(page.getByText('Page not found')).toBeVisible() }) @@ -66,7 +66,7 @@ test.describe('Silo utilization', () => { }) test('works for dev user', async ({ browser }) => { - const page = await getDevUserPage(browser) + const page = await getPageAsUser(browser, 'Hans Jonas') await page.goto('/utilization') await expectVisible(page, [page.getByRole('heading', { name: 'Utilization' })]) await expectNotVisible(page, [ diff --git a/app/test/e2e/utils.ts b/app/test/e2e/utils.ts index 99f9f69cb..94b1aa99c 100644 --- a/app/test/e2e/utils.ts +++ b/app/test/e2e/utils.ts @@ -126,10 +126,13 @@ export async function clickRowAction(page: Page, rowText: string, actionName: st await page.getByRole('menuitem', { name: actionName }).click() } -export async function getDevUserPage(browser: Browser): Promise { +export async function getPageAsUser( + browser: Browser, + user: 'Hans Jonas' | 'Simone de Beauvoir' +): Promise { const browserContext = await browser.newContext() await browserContext.addCookies([ - { name: MSW_USER_COOKIE, value: 'Hans Jonas', domain: 'localhost', path: '/' }, + { name: MSW_USER_COOKIE, value: user, domain: 'localhost', path: '/' }, ]) return await browserContext.newPage() } diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index 33b4620e6..ec9bafdcd 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -34,6 +34,7 @@ import { handleMetrics, paginated, requireFleetViewer, + requireRole, unavailableErr, } from './util' @@ -255,7 +256,12 @@ export const handlers = makeHandlers({ return json(newImage, { status: 201 }) }, imageView: ({ path, query }) => lookup.image({ ...path, ...query }), - imageDelete({ path, query }) { + imageDelete({ path, query, req }) { + // if it's a silo image, you need silo write to delete it + if (!query.project) { + requireRole(req, 'silo', defaultSilo.id, 'collaborator') + } + const image = lookup.image({ ...path, ...query }) db.images = db.images.filter((i) => i.id !== image.id) diff --git a/libs/api-mocks/msw/util.spec.ts b/libs/api-mocks/msw/util.spec.ts index 444ecc641..1fa0bc87f 100644 --- a/libs/api-mocks/msw/util.spec.ts +++ b/libs/api-mocks/msw/util.spec.ts @@ -7,8 +7,10 @@ */ import { describe, expect, it } from 'vitest' -import { users } from '..' -import { paginated, userIsFleetViewer } from './util' +import { FLEET_ID } from '@oxide/api' + +import { defaultSilo, users } from '..' +import { paginated, userHasRole } from './util' describe('paginated', () => { it('should return the first page', () => { @@ -81,11 +83,29 @@ describe('paginated', () => { }) }) -it('userIsFleetViewer', () => { - expect(users.map((u) => [u.display_name, userIsFleetViewer(u)])).toEqual([ - ['Hannah Arendt', true], - ['Hans Jonas', false], - ['Jacob Klein', false], - ['Simone de Beauvoir', false], - ]) +describe('userHasRole', () => { + it('fleet viewer', () => { + expect( + users.map((u) => [u.display_name, userHasRole(u, 'fleet', FLEET_ID, 'viewer')]) + ).toEqual([ + ['Hannah Arendt', true], + ['Hans Jonas', false], + ['Jacob Klein', false], + ['Simone de Beauvoir', false], + ]) + }) + + it('silo collaborator', () => { + expect( + users.map((u) => [ + u.display_name, + userHasRole(u, 'silo', defaultSilo.id, 'collaborator'), + ]) + ).toEqual([ + ['Hannah Arendt', true], + ['Hans Jonas', true], + ['Jacob Klein', false], + ['Simone de Beauvoir', false], + ]) + }) }) diff --git a/libs/api-mocks/msw/util.ts b/libs/api-mocks/msw/util.ts index 6a19bb672..c8c0198dd 100644 --- a/libs/api-mocks/msw/util.ts +++ b/libs/api-mocks/msw/util.ts @@ -10,16 +10,18 @@ import type { RestRequest } from 'msw' import type { DiskCreate, + RoleKey, Sled, SystemMetricName, SystemMetricQueryParams, User, } from '@oxide/api' -import { MAX_DISK_SIZE_GiB, MIN_DISK_SIZE_GiB, totalCapacity } from '@oxide/api' +import { FLEET_ID, MAX_DISK_SIZE_GiB, MIN_DISK_SIZE_GiB, totalCapacity } from '@oxide/api' import type { Json } from '@oxide/gen/msw-handlers' import { json } from '@oxide/gen/msw-handlers' import { GiB, TiB, isTruthy } from '@oxide/util' +import type { DbRoleAssignmentResourceType } from '..' import { genI64Data } from '../metrics' import { db } from './db' @@ -299,24 +301,48 @@ export function currentUser(req: RestRequest): Json { } /** - * Look for fleet roles in roles for the user as well as for groups the user is - * in. Testable core of `requireFleetViewer`. + * Given a role A, get a list of the roles (including A) that confer *at least* + * the powers of A. */ -export function userIsFleetViewer(user: Json): boolean { +// could implement with `takeUntil(allRoles, r => r === role)`, but that is so +// much harder to understand +const roleOrStronger: Record = { + viewer: ['viewer', 'collaborator', 'admin'], + collaborator: ['collaborator', 'admin'], + admin: ['admin'], +} + +/** + * Determine whether a user has a role at least as strong as `role` on the + * specified resource. Note that this does not yet do parent-child inheritance + * like Nexus does, i.e., if a user has collaborator on a silo, then it inherits + * collaborator on all projects in the silo even if it has no explicit role on + * those projects. This does NOT do that. + */ +export function userHasRole( + user: Json, + resourceType: DbRoleAssignmentResourceType, + resourceId: string, + role: RoleKey +): boolean { const userGroupIds = db.groupMemberships .filter((gm) => gm.userId === user.id) .map((gm) => db.userGroups.find((g) => g.id === gm.groupId)) .filter(isTruthy) .map((g) => g.id) - // don't need to filter by role because any role is at least viewer - const actorsWithFleetRole = db.roleAssignments - .filter((ra) => ra.resource_type === 'fleet') + /** All actors with *at least* the specified role on the resource */ + const actorsWithRole = db.roleAssignments + .filter( + (ra) => + ra.resource_type === resourceType && + ra.resource_id === resourceId && + roleOrStronger[role].includes(ra.role_name) + ) .map((ra) => ra.identity_id) - // user is a fleet viewer if their own ID or any of their groups is - // associated with any fleet role - return [user.id, ...userGroupIds].some((id) => actorsWithFleetRole.includes(id)) + // user has role if their own ID or any of their groups is associated with the role + return [user.id, ...userGroupIds].some((id) => actorsWithRole.includes(id)) } /** @@ -325,6 +351,21 @@ export function userIsFleetViewer(user: Json): boolean { * throw 403 if no. */ export function requireFleetViewer(req: RestRequest) { + requireRole(req, 'fleet', FLEET_ID, 'viewer') +} + +/** + * Determine whether current user has a role on a resource by looking roles + * for the user as well as for the user's groups. Do nothing if yes, throw 403 + * if no. + */ +export function requireRole( + req: RestRequest, + resourceType: DbRoleAssignmentResourceType, + resourceId: string, + role: RoleKey +) { const user = currentUser(req) - if (!userIsFleetViewer(user)) throw 403 // should it 404? I think the API is a mix + // should it 404? I think the API is a mix + if (!userHasRole(user, resourceType, resourceId, role)) throw 403 } diff --git a/libs/api-mocks/role-assignment.ts b/libs/api-mocks/role-assignment.ts index fbdc6c5a0..4589b3465 100644 --- a/libs/api-mocks/role-assignment.ts +++ b/libs/api-mocks/role-assignment.ts @@ -18,8 +18,11 @@ import { userGroup2, userGroup3 } from './user-group' // imitating the API's actual DB schema and behavior, storing individual role // assignments and then collecting them into a policy object at request time. // See https://github.com/oxidecomputer/omicron/issues/1165 + +export type DbRoleAssignmentResourceType = 'fleet' | 'silo' | 'project' + type DbRoleAssignment = { - resource_type: 'fleet' | 'silo' | 'project' + resource_type: DbRoleAssignmentResourceType resource_id: string identity_id: string identity_type: IdentityType diff --git a/libs/api/__generated__/OMICRON_VERSION b/libs/api/__generated__/OMICRON_VERSION index aa0feb393..e1afcaa52 100644 --- a/libs/api/__generated__/OMICRON_VERSION +++ b/libs/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -dc2c66961be81b524836782ba24c60eee410f039 +1beda0b8399a302c0efa1af1226a649180b01bd9 diff --git a/libs/api/roles.ts b/libs/api/roles.ts index f8cb8c2d2..c287fe7cc 100644 --- a/libs/api/roles.ts +++ b/libs/api/roles.ts @@ -21,7 +21,6 @@ import type { FleetRole, IdentityType, ProjectRole, SiloRole } from './__generat /** * Union of all the specific roles, which are all the same, which makes making * our methods generic on the *Role type is pointless (until they stop being the same). - * Only named `RoleName` because the API client already exports `Role`. */ export type RoleKey = FleetRole | SiloRole | ProjectRole diff --git a/package-lock.json b/package-lock.json index 32d5edf0b..93f506dcf 100644 --- a/package-lock.json +++ b/package-lock.json @@ -67,7 +67,6 @@ "@types/react-dom": "^18.2.13", "@types/react-is": "^18.2.2", "@types/semver": "^7.5.3", - "@types/testing-library__jest-dom": "^6.0.0", "@types/uuid": "^9.0.5", "@typescript-eslint/eslint-plugin": "~5.59.9", "@typescript-eslint/parser": "~5.59.9", @@ -5895,16 +5894,6 @@ "optional": true, "peer": true }, - "node_modules/@types/testing-library__jest-dom": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/@types/testing-library__jest-dom/-/testing-library__jest-dom-6.0.0.tgz", - "integrity": "sha512-bnreXCgus6IIadyHNlN/oI5FfX4dWgvGhOPvpr7zzCYDGAPIfvyIoAozMBINmhmsVuqV0cncejF2y5KC7ScqOg==", - "deprecated": "This is a stub types definition. @testing-library/jest-dom provides its own type definitions, so you do not need this installed.", - "dev": true, - "dependencies": { - "@testing-library/jest-dom": "*" - } - }, "node_modules/@types/unist": { "version": "2.0.6", "resolved": "https://registry.npmjs.org/@types/unist/-/unist-2.0.6.tgz", @@ -24737,15 +24726,6 @@ "optional": true, "peer": true }, - "@types/testing-library__jest-dom": { - "version": "6.0.0", - "resolved": "https://registry.npmjs.org/@types/testing-library__jest-dom/-/testing-library__jest-dom-6.0.0.tgz", - "integrity": "sha512-bnreXCgus6IIadyHNlN/oI5FfX4dWgvGhOPvpr7zzCYDGAPIfvyIoAozMBINmhmsVuqV0cncejF2y5KC7ScqOg==", - "dev": true, - "requires": { - "@testing-library/jest-dom": "*" - } - }, "@types/unist": { "version": "2.0.6", "resolved": "https://registry.npmjs.org/@types/unist/-/unist-2.0.6.tgz", diff --git a/package.json b/package.json index 82a069f10..a4b0e5064 100644 --- a/package.json +++ b/package.json @@ -87,7 +87,6 @@ "@types/react-dom": "^18.2.13", "@types/react-is": "^18.2.2", "@types/semver": "^7.5.3", - "@types/testing-library__jest-dom": "^6.0.0", "@types/uuid": "^9.0.5", "@typescript-eslint/eslint-plugin": "~5.59.9", "@typescript-eslint/parser": "~5.59.9",