From 70203893dbd58f43e854a1c9e00f7d5a587c9e19 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 27 Jan 2025 23:33:49 -0600 Subject: [PATCH 1/8] better error for group misconfig --- app/components/ErrorBoundary.tsx | 42 +++++++++++++++++++++++++++++--- app/pages/ProjectsPage.tsx | 6 ++++- mock-api/msw/handlers.ts | 10 ++++++-- test/e2e/error-pages.e2e.ts | 9 +++++++ test/e2e/utils.ts | 2 +- 5 files changed, 62 insertions(+), 7 deletions(-) diff --git a/app/components/ErrorBoundary.tsx b/app/components/ErrorBoundary.tsx index 32baf81ded..938f2b16ec 100644 --- a/app/components/ErrorBoundary.tsx +++ b/app/components/ErrorBoundary.tsx @@ -5,23 +5,58 @@ * * Copyright Oxide Computer Company */ +import { useQuery } from '@tanstack/react-query' import { ErrorBoundary as BaseErrorBoundary } from 'react-error-boundary' import { useRouteError } from 'react-router' +import { apiq } from '~/api' import { type ApiError } from '~/api/errors' import { ErrorPage, NotFound } from './ErrorPage' +const IdpMisconfig = () => ( +

+ Hint: You are not in any user groups and you + have no assigned role on the silo. This usually means the{' '} + + admin group name + {' '} + is not set correctly for the silo. Contact your administrator. +

+) + +function useDetectNoSiloRole(enabled: boolean): boolean { + // this is kind of a hail mary, so if any of this goes wrong we need to ignore it + const options = { enabled, throwOnError: false } + const { data: me } = useQuery(apiq('currentUserView', {}, options)) + const { data: myGroups } = useQuery(apiq('currentUserGroups', {}, options)) + const { data: siloPolicy } = useQuery(apiq('policyView', {}, options)) + + if (!me || !myGroups || !siloPolicy) return false + + const noGroups = myGroups.items.length === 0 + const hasDirectRoleOnSilo = siloPolicy.roleAssignments.some((r) => r.identityId === me.id) + return noGroups && !hasDirectRoleOnSilo +} + export const trigger404 = { type: 'error', statusCode: 404 } type Props = { error: Error | ApiError } function ErrorFallback({ error }: Props) { console.error(error) + const statusCode = 'statusCode' in error ? error.statusCode : undefined + + // if the error is a 403, make API calls to check whether the user has any + // groups or any roles directly on the silo + const showIdpMisconfig = useDetectNoSiloRole(statusCode === 403) - if ('statusCode' in error && error.statusCode === 404) { - return - } + if (statusCode === 404) return return ( @@ -29,6 +64,7 @@ function ErrorFallback({ error }: Props) {

Please try again. If the problem persists, contact your administrator.

+ {showIdpMisconfig && } ) } diff --git a/app/pages/ProjectsPage.tsx b/app/pages/ProjectsPage.tsx index 9359e471f2..5e059cf1af 100644 --- a/app/pages/ProjectsPage.tsx +++ b/app/pages/ProjectsPage.tsx @@ -39,7 +39,11 @@ const EmptyState = () => ( const projectList = getListQFn('projectList', {}) export async function loader() { - await queryClient.prefetchQuery(projectList.optionsFn()) + // fetchQuery instead of prefetchQuery means errors blow up here instead of + // waiting to hit the invariant in the useQuery call. We may end up doing this + // everywhere, but here in particular it is needed to trigger the silo group + // misconfig detection case in ErrorBoundary. + await queryClient.fetchQuery(projectList.optionsFn()) return null } diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 05c5dbfb91..88e1763b12 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -72,8 +72,14 @@ export const handlers = makeHandlers({ loginLocal: ({ body: { password } }) => (password === 'bad' ? 401 : 200), groupList: (params) => paginated(params.query, db.userGroups), groupView: (params) => lookupById(db.userGroups, params.path.groupId), - - projectList: (params) => paginated(params.query, db.projects), + projectList: ({ query, cookies }) => { + // this is used to test for the IdP misconfig situation where the user has + // no role on the silo (see error-pages.e2e.ts). requireRole checks for _at + // least_ viewer, and viewer is the weakest role, so checking for viewer + // effectively means "do I have any role at all" + requireRole(cookies, 'silo', defaultSilo.id, 'viewer') + return paginated(query, db.projects) + }, projectCreate({ body }) { errIfExists(db.projects, { name: body.name }, 'project') diff --git a/test/e2e/error-pages.e2e.ts b/test/e2e/error-pages.e2e.ts index d697e5cf5b..1f5117e0c9 100644 --- a/test/e2e/error-pages.e2e.ts +++ b/test/e2e/error-pages.e2e.ts @@ -7,6 +7,8 @@ */ import { expect, test } from '@playwright/test' +import { getPageAsUser } from './utils' + test('Shows 404 page when a resource is not found', async ({ page }) => { await page.goto('/nonexistent') await expect(page.locator('text=Page not found')).toBeVisible() @@ -42,3 +44,10 @@ test('Shows something went wrong page on other errors', async ({ page }) => { // up at the right URL await expect(page).toHaveURL('/login') }) + +test('error page for user with no groups or silo role', async ({ browser }) => { + const page = await getPageAsUser(browser, 'Jacob Klein') + await page.goto('/projects') + await expect(page.getByText('Something went wrong')).toBeVisible() + await expect(page.getByText('admin group name is not set correctly')).toBeVisible() +}) diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index 4bb0638434..e5fb912cea 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -185,7 +185,7 @@ export async function selectOption( export async function getPageAsUser( browser: Browser, - user: 'Hans Jonas' | 'Simone de Beauvoir' + user: 'Hans Jonas' | 'Simone de Beauvoir' | 'Jacob Klein' ): Promise { const browserContext = await browser.newContext() await browserContext.addCookies([ From 719bcc56afcb0be9e19bb52ef25c8e8947a7f8cb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 31 Jan 2025 14:45:38 -0600 Subject: [PATCH 2/8] no "hint:" --- app/components/ErrorBoundary.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/components/ErrorBoundary.tsx b/app/components/ErrorBoundary.tsx index 938f2b16ec..6e7659db2a 100644 --- a/app/components/ErrorBoundary.tsx +++ b/app/components/ErrorBoundary.tsx @@ -16,8 +16,8 @@ import { ErrorPage, NotFound } from './ErrorPage' const IdpMisconfig = () => (

- Hint: You are not in any user groups and you - have no assigned role on the silo. This usually means the{' '} + You are not in any user groups and you have no assigned role on the silo. This usually + means the{' '} Date: Fri, 31 Jan 2025 15:52:12 -0600 Subject: [PATCH 3/8] much better look after a ben consultation --- app/components/ErrorBoundary.tsx | 34 ++++++++++++++++++++------------ app/ui/lib/Message.tsx | 15 +++++++------- 2 files changed, 28 insertions(+), 21 deletions(-) diff --git a/app/components/ErrorBoundary.tsx b/app/components/ErrorBoundary.tsx index 6e7659db2a..d3ec480e0d 100644 --- a/app/components/ErrorBoundary.tsx +++ b/app/components/ErrorBoundary.tsx @@ -11,23 +11,31 @@ import { useRouteError } from 'react-router' import { apiq } from '~/api' import { type ApiError } from '~/api/errors' +import { Message } from '~/ui/lib/Message' import { ErrorPage, NotFound } from './ErrorPage' const IdpMisconfig = () => ( -

- You are not in any user groups and you have no assigned role on the silo. This usually - means the{' '} - - admin group name - {' '} - is not set correctly for the silo. Contact your administrator. -

+ + You are not in any user groups and you have no assigned role on the silo. This + usually means the{' '} + + admin group name + {' '} + is not set correctly for the silo. + + } + /> ) function useDetectNoSiloRole(enabled: boolean): boolean { diff --git a/app/ui/lib/Message.tsx b/app/ui/lib/Message.tsx index d28641410f..0f48a57cc6 100644 --- a/app/ui/lib/Message.tsx +++ b/app/ui/lib/Message.tsx @@ -27,8 +27,7 @@ export interface MessageProps { text: string link: To } - // try to use icons from the ___12Icon set, rather than forcing a 16px or 24px icon - icon?: ReactElement + showIcon?: boolean } const defaultIcon: Record = { @@ -73,21 +72,21 @@ export const Message = ({ className, variant = 'info', cta, - icon, + showIcon = true, }: MessageProps) => { return (
-
- {icon || defaultIcon[variant]} -
-
+ {showIcon && ( +
{defaultIcon[variant]}
+ )} +
{title &&
{title}
}
Date: Mon, 3 Feb 2025 15:58:05 -0600 Subject: [PATCH 4/8] move docs link to links.ts, update URL --- app/components/ErrorBoundary.tsx | 15 ++++++++------- app/util/links.ts | 2 ++ test/e2e/error-pages.e2e.ts | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/app/components/ErrorBoundary.tsx b/app/components/ErrorBoundary.tsx index d3ec480e0d..be419ffa09 100644 --- a/app/components/ErrorBoundary.tsx +++ b/app/components/ErrorBoundary.tsx @@ -12,6 +12,7 @@ import { useRouteError } from 'react-router' import { apiq } from '~/api' import { type ApiError } from '~/api/errors' import { Message } from '~/ui/lib/Message' +import { links } from '~/util/links' import { ErrorPage, NotFound } from './ErrorPage' @@ -21,19 +22,19 @@ const IdpMisconfig = () => ( className="!mt-6" showIcon={false} content={ - <> - You are not in any user groups and you have no assigned role on the silo. This - usually means the{' '} +

+ You are not in any groups and have no role on the silo. This usually means the + identity provider is not set up correctly. Read the{' '} - admin group name + docs {' '} - is not set correctly for the silo. - + for more information. +

} /> ) diff --git a/app/util/links.ts b/app/util/links.ts index b3bc96256f..159f0aa790 100644 --- a/app/util/links.ts +++ b/app/util/links.ts @@ -50,6 +50,8 @@ export const links = { systemSiloDocs: 'https://docs.oxide.computer/guides/operator/silo-management', transitIpsDocs: 'https://docs.oxide.computer/guides/configuring-guest-networking#_example_4_software_routing_tunnels', + troubleshootingAccess: + 'https://docs.oxide.computer/guides/operator/faq#_how_do_i_fix_the_something_went_wrong_error', instancesDocs: 'https://docs.oxide.computer/guides/deploying-workloads', vpcsDocs: 'https://docs.oxide.computer/guides/configuring-guest-networking', } diff --git a/test/e2e/error-pages.e2e.ts b/test/e2e/error-pages.e2e.ts index 1f5117e0c9..93340de431 100644 --- a/test/e2e/error-pages.e2e.ts +++ b/test/e2e/error-pages.e2e.ts @@ -49,5 +49,5 @@ test('error page for user with no groups or silo role', async ({ browser }) => { const page = await getPageAsUser(browser, 'Jacob Klein') await page.goto('/projects') await expect(page.getByText('Something went wrong')).toBeVisible() - await expect(page.getByText('admin group name is not set correctly')).toBeVisible() + await expect(page.getByText('identity provider is not set up correctly')).toBeVisible() }) From 2195cf275bad2d73af8a71c1c90207b09f0b795a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 4 Feb 2025 10:55:13 -0600 Subject: [PATCH 5/8] try to reduce test flake with slower mock image upload --- app/forms/image-upload.tsx | 1 + mock-api/msw/handlers.ts | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/forms/image-upload.tsx b/app/forms/image-upload.tsx index 9a77b95dd6..262d3cb849 100644 --- a/app/forms/image-upload.tsx +++ b/app/forms/image-upload.tsx @@ -536,6 +536,7 @@ export function Component() { await onSubmit(values) } catch (e) { if (e !== ABORT_ERROR) { + console.error(e) setModalError('Something went wrong. Please try again.') } cancelEverything() diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 88e1763b12..3ad00a5dca 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -218,7 +218,7 @@ export const handlers = makeHandlers({ const disk = lookup.disk({ ...path, ...query }) const diskImport = db.diskBulkImportState.get(disk.id) if (!diskImport) throw notFoundErr(`disk import for disk '${disk.id}'`) - await delay(1000) // slow it down for the tests + await delay(2000) // slow it down for the tests // if (Math.random() < 0.01) throw 400 diskImport.blocks[body.offset] = true return 204 From f30d2e9fe770030032c215d396b33538777d1d7f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 4 Feb 2025 10:55:13 -0600 Subject: [PATCH 6/8] add .first() to closeToast helper --- test/e2e/utils.ts | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index e5fb912cea..10c719a44e 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -133,11 +133,16 @@ export async function expectNoToast(page: Page, expectedText: string | RegExp) { } /** - * Close toast and wait for it to fade out. For some reason it prevents things - * from working, but only in tests as far as we can tell. + * Close first toast and wait for it to fade out. For some reason it prevents + * things from working, but only in tests as far as we can tell. */ export async function closeToast(page: Page) { - await page.getByRole('button', { name: 'Dismiss notification' }).click() + // first() is a hack aimed at situations where we're testing an error + // response, which usually means we have an initial "creating..." toast + // followed by an error toast. Sometimes the error toast shows up so fast that + // we don't have time to close the first one. Without first(), this errors out + // because there are two toasts. + await page.getByRole('button', { name: 'Dismiss notification' }).first().click() await sleep(1000) } From 85f9682c73f86f65798dcbd9f35676e251acf3f9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 4 Feb 2025 16:19:00 -0600 Subject: [PATCH 7/8] give e2e a chance to catch upload states --- mock-api/msw/handlers.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 96dc7d2ea8..299ff6a860 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -195,7 +195,7 @@ export const handlers = makeHandlers({ throw 'Can only enter state importing_from_bulk_write from import_ready' } - await delay(1000) // slow it down for the tests + await delay(2000) // slow it down for the tests db.diskBulkImportState.set(disk.id, { blocks: {} }) disk.state = { state: 'importing_from_bulk_writes' } @@ -209,7 +209,7 @@ export const handlers = makeHandlers({ if (disk.state.state !== 'importing_from_bulk_writes') { throw 'Can only stop import for disk in state importing_from_bulk_write' } - await delay(1000) // slow it down for the tests + await delay(2000) // slow it down for the tests db.diskBulkImportState.delete(disk.id) disk.state = { state: 'import_ready' } From 28abda92d95adf303534d1a1fef1b5bbbf6778e8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 4 Feb 2025 18:01:00 -0600 Subject: [PATCH 8/8] fight the flakes --- mock-api/msw/handlers.ts | 2 +- test/e2e/image-upload.e2e.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 299ff6a860..2cffe08e11 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -219,7 +219,7 @@ export const handlers = makeHandlers({ const disk = lookup.disk({ ...path, ...query }) const diskImport = db.diskBulkImportState.get(disk.id) if (!diskImport) throw notFoundErr(`disk import for disk '${disk.id}'`) - await delay(2000) // slow it down for the tests + await delay(1000) // slow it down for the tests // if (Math.random() < 0.01) throw 400 diskImport.blocks[body.offset] = true return 204 diff --git a/test/e2e/image-upload.e2e.ts b/test/e2e/image-upload.e2e.ts index a1d8315a13..28e188eb91 100644 --- a/test/e2e/image-upload.e2e.ts +++ b/test/e2e/image-upload.e2e.ts @@ -159,7 +159,8 @@ test.describe('Image upload', () => { await page.getByRole('button', { name: 'Cancel' }).click() await page.getByRole('link', { name: 'Disks' }).click() await expect(page.getByRole('cell', { name: 'disk-1', exact: true })).toBeVisible() - await expect(page.getByRole('cell', { name: 'tmp' })).toBeHidden() + // needs a little extra time for delete to go through + await expect(page.getByRole('cell', { name: 'tmp' })).toBeHidden({ timeout: 10000 }) }) }