Skip to content
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.

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

Better error for admin group name misconfig #2671

Merged
merged 9 commits into from
Feb 5, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
51 changes: 48 additions & 3 deletions app/components/ErrorBoundary.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,30 +5,75 @@
*
* 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 { Message } from '~/ui/lib/Message'
import { links } from '~/util/links'

import { ErrorPage, NotFound } from './ErrorPage'

const IdpMisconfig = () => (
<Message
variant="notice"
className="!mt-6"
showIcon={false}
content={
<p>
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{' '}
<a
href={links.troubleshootingAccess}
className="underline"
target="_blank"
rel="noreferrer"
>
docs
</a>{' '}
for more information.
</p>
}
/>
)

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 <NotFound />
}
if (statusCode === 404) return <NotFound />

return (
<ErrorPage>
<h1 className="text-sans-2xl">Something went wrong</h1>
<p className="text-secondary">
Please try again. If the problem persists, contact your administrator.
</p>
{showIdpMisconfig && <IdpMisconfig />}
</ErrorPage>
)
}
Expand Down
1 change: 1 addition & 0 deletions app/forms/image-upload.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,7 @@ export function Component() {
await onSubmit(values)
} catch (e) {
if (e !== ABORT_ERROR) {
console.error(e)
setModalError('Something went wrong. Please try again.')
// abort anything in flight in case
cancelEverything()
Expand Down
6 changes: 5 additions & 1 deletion app/pages/ProjectsPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines +42 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

this is helpful; thank you

await queryClient.fetchQuery(projectList.optionsFn())
return null
}

Expand Down
15 changes: 7 additions & 8 deletions app/ui/lib/Message.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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<Variant, ReactElement> = {
Expand Down Expand Up @@ -73,21 +72,21 @@ export const Message = ({
className,
variant = 'info',
cta,
icon,
showIcon = true,
}: MessageProps) => {
return (
<div
className={cn(
'relative flex items-start overflow-hidden rounded-lg p-4 elevation-1',
'relative flex items-start gap-2.5 overflow-hidden rounded-lg p-4 elevation-1',
color[variant],
textColor[variant],
className
)}
>
<div className="mt-[2px] flex [&>svg]:h-3 [&>svg]:w-3">
{icon || defaultIcon[variant]}
</div>
<div className="flex-1 pl-2.5">
{showIcon && (
<div className="mt-[2px] flex [&>svg]:h-3 [&>svg]:w-3">{defaultIcon[variant]}</div>
)}
<div className="flex-1">
{title && <div className="text-sans-semi-md">{title}</div>}
<div
className={cn(
Expand Down
2 changes: 2 additions & 0 deletions app/util/links.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
}
Expand Down
14 changes: 10 additions & 4 deletions mock-api/msw/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand Down Expand Up @@ -189,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' }
Expand All @@ -203,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' }
Expand Down
9 changes: 9 additions & 0 deletions test/e2e/error-pages.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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('identity provider is not set up correctly')).toBeVisible()
})
3 changes: 2 additions & 1 deletion test/e2e/image-upload.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 })
})
}

Expand Down
13 changes: 9 additions & 4 deletions test/e2e/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -185,7 +190,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<Page> {
const browserContext = await browser.newContext()
await browserContext.addCookies([
Expand Down
Loading