Delete button on silo images table

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[] => [

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<Image | null>(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 (

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 default-silo' })).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()
 

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,14 +125,59 @@ 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()
   await page.getByRole('button', { name: 'Confirm' }).click()
   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()
 })

app/test/e2e/utilization.e2e.ts

@@ -5,7 +5,7 @@
  *
  * Copyright Oxide Computer Company
  */
-import { expect, expectNotVisible, expectVisible, getDevUserPage, test } from './utils'
+import { expect, expectNotVisible, expectVisible, getPageAsUser, test } from './utils'
 
 // not trying to get elaborate here. just make sure the pages load, which
 // exercises the loader prefetches and invariant checks
@@ -24,7 +24,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()
   })
@@ -46,7 +46,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: 'Capacity & Utilization' }),

app/test/e2e/utils.ts

@@ -120,10 +120,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<Page> {
+export async function getPageAsUser(
+  browser: Browser,
+  user: 'Hans Jonas' | 'Simone de Beauvoir'
+): Promise<Page> {
   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()
 }

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)
 

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],
+    ])
+  })
 })

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'
 
@@ -292,24 +294,45 @@ export function currentUser(req: RestRequest): Json<User> {
 }
 
 /**
- * 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<User>): boolean {
+// could implement with `takeUntil(allRoles, r => r === role)`, but that is so
+// much harder to understand
+const roleOrStronger: Record<RoleKey, RoleKey[]> = {
+  viewer: ['viewer', 'collaborator', 'admin'],
+  collaborator: ['collaborator', 'admin'],
+  admin: ['admin'],
+}
+
+/**
+ * Determine whether a user has a role at least as strong as the specified role
+ * on the specified resource.
+ */
+export function userHasRole(
+  user: Json<User>,
+  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))
 }
 
 /**
@@ -318,6 +341,21 @@ export function userIsFleetViewer(user: Json<User>): 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
 }

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

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