From d856e3009494d322cf5e605dd35f8d09546f1668 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 14 Oct 2022 15:51:00 -0500 Subject: [PATCH 1/8] show/hide silo picker in top bar based on fleet policy --- app/components/TopBar.tsx | 30 +++++++++++++++++------------- app/layouts/helpers.tsx | 7 +++++-- app/routes.tsx | 4 ++-- libs/api-mocks/msw/handlers.ts | 22 ++++++++++++++++++++-- libs/api-mocks/role-assignment.ts | 16 ++++++++++++++-- libs/api/index.ts | 1 + 6 files changed, 59 insertions(+), 21 deletions(-) diff --git a/app/components/TopBar.tsx b/app/components/TopBar.tsx index 64cb3e8080..cdfd668a0b 100644 --- a/app/components/TopBar.tsx +++ b/app/components/TopBar.tsx @@ -15,12 +15,6 @@ import { pb } from 'app/util/path-builder' import { OrgPicker, ProjectPicker, SiloSystemPicker } from './TopBarPicker' -/** - * TODO: This is a temporary flag to disable the silo picker until we have - * the an API endpoint to support it. - */ -const hasSiloPerms = true - export function TopBar() { const navigate = useNavigate() const logout = useApiMutation('logout', { @@ -31,19 +25,29 @@ export function TopBar() { navToLogin({ includeCurrent: false }) }, }) - const { data: user, error } = useApiQuery( - 'sessionMe', - {}, - { cacheTime: 0, refetchOnWindowFocus: false } - ) + const { data: user } = useApiQuery('sessionMe', {}, { cacheTime: 0 }) + const { data: systemPolicy } = useApiQuery('systemPolicyView', {}) - const loggedIn = user && !error + const loggedIn = !!user + + // Whether the user is shown /system/* routes is governed by whether they have + // viewer perms (or better) on the fleet. The natural place to look for that + // is the fleet policy, but if the user doesn't have fleet read, they will get + // a 403 from that endpoint. So we simply check whether that endpoint 200s or + // not to determine whether the user is a fleet viewer. + // + // Note that we are able to ignore the possibility of `systemPolicy` being + // undefined because the request is still going because we have prefetched + // this request in the loader. If that prefetch were removed, fleet viewers + // would see the silo picker pop in when the request resolves. Bad. + const isFleetViewer = !!systemPolicy const isSystem = useLocation().pathname.startsWith(pb.system()) // lol + const { projectName } = useParams() const [cornerPicker, ...otherPickers] = [ - hasSiloPerms && , + isFleetViewer && , // TODO: This works ok in most situations, but when an operator user is on // the orgs page with no org selected, they see this picker, which is // redundant with the list of orgs. Overall this logic is starting to feel diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index 64466739cb..b1dce2a964 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -24,6 +24,9 @@ export const ContentPane = () => ( ) -export async function prefetchSessionMe() { - await apiQueryClient.prefetchQuery('sessionMe', {}) +export async function prefetchUserData() { + await Promise.all([ + apiQueryClient.prefetchQuery('sessionMe', {}), + apiQueryClient.prefetchQuery('systemPolicyView', {}), + ]) } diff --git a/app/routes.tsx b/app/routes.tsx index 5ec3636b9a..6a9d061519 100644 --- a/app/routes.tsx +++ b/app/routes.tsx @@ -10,7 +10,7 @@ import RootLayout from './layouts/RootLayout' import SettingsLayout from './layouts/SettingsLayout' import SiloLayout from './layouts/SiloLayout' import SystemLayout from './layouts/SystemLayout' -import { prefetchSessionMe } from './layouts/helpers' +import { prefetchUserData } from './layouts/helpers' import DeviceAuthSuccessPage from './pages/DeviceAuthSuccessPage' import DeviceAuthVerifyPage from './pages/DeviceAuthVerifyPage' import LoginPage from './pages/LoginPage' @@ -55,7 +55,7 @@ export const routes = createRoutesFromElements( {/* This wraps all routes that are supposed to be authenticated */} - }> + }> }> } /> } handle={{ crumb: 'Profile' }} /> diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index a807ce4f3b..b73430eb9f 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -5,6 +5,7 @@ import { pick, sortBy } from '@oxide/util' import type { Json } from '../json-type' import { genCumulativeI64Data, genI64Data } from '../metrics' +import { FLEET_ID } from '../role-assignment' import { serial } from '../serial' import { sessionMe } from '../session' import { defaultSilo } from '../silo' @@ -53,6 +54,10 @@ const unavailableBody = { error_code: 'ServiceUnavailable' } as const type Unavailable = typeof unavailableBody const unavailableErr = json(unavailableBody, { status: 503 }) +const forbiddenBody = { error_code: 'Forbidden' } as const +type Forbidden = typeof forbiddenBody +// const forbiddenErr = json(forbiddenBody, { status: 403 }) + const badRequest = (msg: string) => compose( context.status(400), @@ -63,8 +68,8 @@ const badRequest = (msg: string) => }) ) -type GetErr = NotFound | Unavailable -type PostErr = AlreadyExists | NotFound +type GetErr = NotFound | Unavailable | Forbidden +type PostErr = AlreadyExists | NotFound | Forbidden export const handlers = [ rest.get('/session/me', (req, res) => res(json(sessionMe))), @@ -1139,6 +1144,19 @@ export const handlers = [ } ), + rest.get | GetErr>( + '/system/policy', + (_req, res) => { + // manually uncomment to test non-operator user + // TODO: figure out authz in the mock server ugh + // return res(forbiddenErr) + const role_assignments = db.roleAssignments + .filter((r) => r.resource_type === 'fleet' && r.resource_id === FLEET_ID) + .map((r) => pick(r, 'identity_id', 'identity_type', 'role_name')) + return res(json({ role_assignments })) + } + ), + rest.get | GetErr>('/users', (req, res) => { return res(json(paginated(req.url.search, db.users))) }), diff --git a/libs/api-mocks/role-assignment.ts b/libs/api-mocks/role-assignment.ts index 7f008648d1..a565f33110 100644 --- a/libs/api-mocks/role-assignment.ts +++ b/libs/api-mocks/role-assignment.ts @@ -1,4 +1,5 @@ import type { + FleetRoleRoleAssignment, OrganizationRoleRoleAssignment, ProjectRoleRoleAssignment, SiloRoleRoleAssignment, @@ -18,12 +19,23 @@ import { userGroup1, userGroup2 } from './user-group' // assignments and then collecting them into a policy object at request time. // See https://github.com/oxidecomputer/omicron/issues/1165 type DbRoleAssignment = { resource_id: string } & ( - | ({ resource_type: 'project' } & Json) - | ({ resource_type: 'organization' } & Json) + | ({ resource_type: 'fleet' } & Json) | ({ resource_type: 'silo' } & Json) + | ({ resource_type: 'organization' } & Json) + | ({ resource_type: 'project' } & Json) ) +// this is hard-coded in the API. there can only be one +export const FLEET_ID = '001de000-1334-4000-8000-000000000000' + export const roleAssignments: DbRoleAssignment[] = [ + { + resource_type: 'fleet', + resource_id: FLEET_ID, + identity_id: user1.id, + identity_type: 'silo_user', + role_name: 'admin', + }, { resource_type: 'silo', resource_id: defaultSilo.id, diff --git a/libs/api/index.ts b/libs/api/index.ts index 87cde962b2..5329506014 100644 --- a/libs/api/index.ts +++ b/libs/api/index.ts @@ -26,6 +26,7 @@ export const queryClient = new QueryClient({ queries: { retry: false, staleTime: 10000, + refetchOnWindowFocus: false, }, }, }) From c3d56949c4ef36ceb1ddb3db246eedd95de93b5d Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 14 Oct 2022 16:44:25 -0500 Subject: [PATCH 2/8] 404 if user tries to hit system routes without fleet viewer perm --- app/components/ErrorBoundary.tsx | 2 ++ app/components/TopBar.tsx | 2 +- app/layouts/SystemLayout.tsx | 20 ++++++++++++++++++++ app/layouts/helpers.tsx | 3 ++- app/pages/LoginPage.tsx | 4 +++- app/routes.tsx | 2 +- app/util/path-builder.ts | 3 +++ package.json | 2 +- yarn.lock | 24 +++++++++--------------- 9 files changed, 42 insertions(+), 20 deletions(-) diff --git a/app/components/ErrorBoundary.tsx b/app/components/ErrorBoundary.tsx index cbbcb923bd..347e1f1265 100644 --- a/app/components/ErrorBoundary.tsx +++ b/app/components/ErrorBoundary.tsx @@ -5,6 +5,8 @@ import type { ErrorResult } from '@oxide/api' import NotFound from 'app/pages/NotFound' +export const trigger404 = { type: 'error', statusCode: 404 } + type Props = { error: Error | ErrorResult } function ErrorFallback({ error }: Props) { diff --git a/app/components/TopBar.tsx b/app/components/TopBar.tsx index cdfd668a0b..287454dfd4 100644 --- a/app/components/TopBar.tsx +++ b/app/components/TopBar.tsx @@ -42,7 +42,7 @@ export function TopBar() { // would see the silo picker pop in when the request resolves. Bad. const isFleetViewer = !!systemPolicy - const isSystem = useLocation().pathname.startsWith(pb.system()) // lol + const isSystem = pb.isSystemPath(useLocation().pathname) const { projectName } = useParams() diff --git a/app/layouts/SystemLayout.tsx b/app/layouts/SystemLayout.tsx index 932fa5cd25..7ac26b832f 100644 --- a/app/layouts/SystemLayout.tsx +++ b/app/layouts/SystemLayout.tsx @@ -1,3 +1,4 @@ +import { apiQueryClient } from '@oxide/api' import { Cloud16Icon, Divider, @@ -10,12 +11,31 @@ import { Storage16Icon, } from '@oxide/ui' +import { trigger404 } from 'app/components/ErrorBoundary' import { DocsLinkItem, NavLinkItem, Sidebar } from 'app/components/Sidebar' import { TopBar } from 'app/components/TopBar' import { pb } from 'app/util/path-builder' import { ContentPane, PageContainer } from './helpers' +/** + * If we can see the policy, we're a fleet viewer, same as in `TopBar`. We need + * to `fetchQuery` instead of `prefetchQuery` because the latter doesn't return + * the result, and then we need to `.catch()` because `fetchQuery` throws on + * request error. We're being a little cavalier here with the error. If it's + * something other than a 403, that would be strange and we would want to know. + */ +SiloLayout.loader = async () => { + const isFleetViewer = await apiQueryClient + .fetchQuery('systemPolicyView', {}) + .then(() => true) + .catch(() => false) + + // TODO: make sure 404 is the desired behavior. This situation should be + // pretty unlikely. + if (!isFleetViewer) throw trigger404 +} + export default function SiloLayout() { return ( diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index b1dce2a964..5b399de8ab 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -24,9 +24,10 @@ export const ContentPane = () => ( ) -export async function prefetchUserData() { +export const prefetchUserData = async () => { await Promise.all([ apiQueryClient.prefetchQuery('sessionMe', {}), + // also fetched by the SystemLayout loader, but RQ dedupes it apiQueryClient.prefetchQuery('systemPolicyView', {}), ]) } diff --git a/app/pages/LoginPage.tsx b/app/pages/LoginPage.tsx index a090adee4e..095dcc27f1 100644 --- a/app/pages/LoginPage.tsx +++ b/app/pages/LoginPage.tsx @@ -3,6 +3,8 @@ import { useNavigate, useSearchParams } from 'react-router-dom' import { useApiMutation } from '@oxide/api' import { Button, Success16Icon, Warning12Icon } from '@oxide/ui' +import { pb } from 'app/util/path-builder' + import { useToast } from '../hooks' /** @@ -28,7 +30,7 @@ export default function LoginPage() { title: 'Logged in', icon: , }) - navigate(searchParams.get('state') || '/') + navigate(searchParams.get('state') || pb.orgs()) }, onError: () => { addToast({ diff --git a/app/routes.tsx b/app/routes.tsx index 6a9d061519..c8a260ba04 100644 --- a/app/routes.tsx +++ b/app/routes.tsx @@ -73,7 +73,7 @@ export const routes = createRoutesFromElements( } handle={{ crumb: 'Hotkeys' }} /> - }> + } loader={SystemLayout.loader}> } loader={SilosPage.loader} /> '/settings/ssh-keys', deviceSuccess: () => '/device/success', + + // someday we'll be less stupid. not today + isSystemPath: (pathname: string) => pathname.startsWith(pb.system()), } // export const jelly = 'just kidding' diff --git a/package.json b/package.json index be1bb24140..6121026165 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "@reach/menu-button": "^0.16.2", "@reach/tabs": "^0.16.4", "@react-spring/web": "^9.4.5", - "@tanstack/react-query": "^4.0.10", + "@tanstack/react-query": "^4.12.0", "@tanstack/react-table": "^8.5.11", "classnames": "^2.3.1", "date-fns": "^2.28.0", diff --git a/yarn.lock b/yarn.lock index e0cac66049..d359ff16fb 100644 --- a/yarn.lock +++ b/yarn.lock @@ -2375,18 +2375,17 @@ "@svgr/hast-util-to-babel-ast" "^6.3.1" svg-parser "^2.0.4" -"@tanstack/query-core@^4.0.0-beta.1": - version "4.0.5" - resolved "https://registry.yarnpkg.com/@tanstack/query-core/-/query-core-4.0.5.tgz#70c7275a9f1fbc8e6520d895991f9bace0fd5bb8" - integrity sha512-QOJ2gLbwlf8p0487pMey6vv8EF5X2ib1zINayaD7mb9/LibUtXmZ12uJgTqcnjgNY/4tWZn5qJnEk2ePG5AVGA== +"@tanstack/query-core@4.12.0": + version "4.12.0" + resolved "https://registry.yarnpkg.com/@tanstack/query-core/-/query-core-4.12.0.tgz#0e96adcfe182efc4ea4c21802f7596d56c6cd60a" + integrity sha512-KEiFPNLMFByhNL2s6RBFL6Z5cNdwwQzFpW/II3GY+rEuQ343ZEoVyQ48zlUXXkEkbamQFIFg2onM8Pxf0Yo01A== -"@tanstack/react-query@^4.0.10": - version "4.0.10" - resolved "https://registry.yarnpkg.com/@tanstack/react-query/-/react-query-4.0.10.tgz#92c71a2632c06450d848d4964959bd216cde03c0" - integrity sha512-Wn5QhZUE5wvr6rGClV7KeQIUsdTmYR9mgmMZen7DSRWauHW2UTynFg3Kkf6pw+XlxxOLsyLWwz/Q6q1lSpM3TQ== +"@tanstack/react-query@^4.12.0": + version "4.12.0" + resolved "https://registry.yarnpkg.com/@tanstack/react-query/-/react-query-4.12.0.tgz#2cb233ef1ccf7537aeed61ca171fc3dcc52d9c57" + integrity sha512-prchV1q+CJ0ZVo8Rts2cOF3azDfQizZZySmH6XXsXRcPTbir0sgb9fp0vY/5l5ZkSYjTvWt/OL8WQhAhYMSvrA== dependencies: - "@tanstack/query-core" "^4.0.0-beta.1" - "@types/use-sync-external-store" "^0.0.3" + "@tanstack/query-core" "4.12.0" use-sync-external-store "^1.2.0" "@tanstack/react-table@^8.5.11": @@ -2771,11 +2770,6 @@ dependencies: "@types/node" "*" -"@types/use-sync-external-store@^0.0.3": - version "0.0.3" - resolved "https://registry.yarnpkg.com/@types/use-sync-external-store/-/use-sync-external-store-0.0.3.tgz#b6725d5f4af24ace33b36fafd295136e75509f43" - integrity sha512-EwmlvuaxPNej9+T4v5AuBPJa2x2UOJVdjCtDHgcDqitUeOtjnJKJ+apYjVcAoBEMjKW1VVFGZLUb5+qqa09XFA== - "@types/uuid@^8.3.0": version "8.3.0" resolved "https://registry.npmjs.org/@types/uuid/-/uuid-8.3.0.tgz" From f0cdf82c2fce6980612270e37bc7d9357bbd2fd7 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 14 Oct 2022 16:48:57 -0500 Subject: [PATCH 3/8] fix SystemLayout name --- app/layouts/SystemLayout.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/layouts/SystemLayout.tsx b/app/layouts/SystemLayout.tsx index 7ac26b832f..dd5dc96953 100644 --- a/app/layouts/SystemLayout.tsx +++ b/app/layouts/SystemLayout.tsx @@ -25,7 +25,7 @@ import { ContentPane, PageContainer } from './helpers' * request error. We're being a little cavalier here with the error. If it's * something other than a 403, that would be strange and we would want to know. */ -SiloLayout.loader = async () => { +SystemLayout.loader = async () => { const isFleetViewer = await apiQueryClient .fetchQuery('systemPolicyView', {}) .then(() => true) @@ -36,7 +36,7 @@ SiloLayout.loader = async () => { if (!isFleetViewer) throw trigger404 } -export default function SiloLayout() { +export default function SystemLayout() { return ( From d48b35618c9ece3537378767eda91729b4086820 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 14 Oct 2022 19:09:11 -0500 Subject: [PATCH 4/8] today is the day to be less stupid. don't detect system route by looking at strings --- app/components/TopBar.tsx | 25 ++++++++++++++++++------- app/components/TopBarPicker.tsx | 4 ++-- app/layouts/SystemLayout.tsx | 2 +- app/util/path-builder.ts | 3 --- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/app/components/TopBar.tsx b/app/components/TopBar.tsx index 287454dfd4..08eb61fcbf 100644 --- a/app/components/TopBar.tsx +++ b/app/components/TopBar.tsx @@ -1,5 +1,5 @@ import { Menu, MenuButton, MenuItem, MenuList } from '@reach/menu-button' -import { useLocation, useNavigate, useParams } from 'react-router-dom' +import { useNavigate, useParams } from 'react-router-dom' import { navToLogin, useApiMutation, useApiQuery } from '@oxide/api' import { @@ -15,7 +15,19 @@ import { pb } from 'app/util/path-builder' import { OrgPicker, ProjectPicker, SiloSystemPicker } from './TopBarPicker' -export function TopBar() { +type TopBarProps = { + /** + * Passing around flags like this is definitely a code smell, but this is the + * simplest thing for now. This allows `SystemLayout` to tell us when we're on + * a system route (instead of detecting it in an unreliable way by, for + * example, checking what the pathname starts with) while at the same time + * keeping the logic for showing/hiding the system/silo picker based on the + * current user's permissions bundled up in here. + */ + isSystemRoute?: boolean +} + +export function TopBar({ isSystemRoute = false }: TopBarProps) { const navigate = useNavigate() const logout = useApiMutation('logout', { onSuccess: () => { @@ -42,12 +54,10 @@ export function TopBar() { // would see the silo picker pop in when the request resolves. Bad. const isFleetViewer = !!systemPolicy - const isSystem = pb.isSystemPath(useLocation().pathname) - const { projectName } = useParams() const [cornerPicker, ...otherPickers] = [ - isFleetViewer && , + isFleetViewer && , // TODO: This works ok in most situations, but when an operator user is on // the orgs page with no org selected, they see this picker, which is // redundant with the list of orgs. Overall this logic is starting to feel @@ -55,8 +65,9 @@ export function TopBar() { // like we were doing before. That way, for example, we know whether we're // on a system situation because we're in SystemLayout. Seems pretty obvious // in hindsight. - !isSystem && , - projectName && , + !isSystemRoute && , + // doing both checks is redundant but let's be clear + !isSystemRoute && projectName && , ].filter(isTruthy) // The height of this component is governed by the `PageContainer` diff --git a/app/components/TopBarPicker.tsx b/app/components/TopBarPicker.tsx index 2e05a16cfe..43c259fc15 100644 --- a/app/components/TopBarPicker.tsx +++ b/app/components/TopBarPicker.tsx @@ -104,7 +104,7 @@ const NoOrgLogo = () => ( ) -export function SiloSystemPicker({ isSystem }: { isSystem: boolean }) { +export function SiloSystemPicker({ isSystemRoute }: { isSystemRoute: boolean }) { const commonProps = { items: [ { label: 'System', to: pb.silos() }, @@ -113,7 +113,7 @@ export function SiloSystemPicker({ isSystem }: { isSystem: boolean }) { 'aria-label': 'Switch between system and silo', } - return isSystem ? ( + return isSystemRoute ? ( { export default function SystemLayout() { return ( - + diff --git a/app/util/path-builder.ts b/app/util/path-builder.ts index 493981cb43..a18f10aee7 100644 --- a/app/util/path-builder.ts +++ b/app/util/path-builder.ts @@ -54,9 +54,6 @@ export const pb = { sshKeys: () => '/settings/ssh-keys', deviceSuccess: () => '/device/success', - - // someday we'll be less stupid. not today - isSystemPath: (pathname: string) => pathname.startsWith(pb.system()), } // export const jelly = 'just kidding' From 02eac6f4b015e7afcfca7ad2044ffb48fc18dd60 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 14 Oct 2022 21:49:21 -0500 Subject: [PATCH 5/8] FINALLY figured out how to handle the top bar pickers in the layouts --- app/components/TopBar.tsx | 51 ++++----------------------------- app/components/TopBarPicker.tsx | 28 +++++++++++++++--- app/layouts/OrgLayout.tsx | 6 +++- app/layouts/ProjectLayout.tsx | 7 ++++- app/layouts/SettingsLayout.tsx | 6 +++- app/layouts/SiloLayout.tsx | 6 +++- app/layouts/SystemLayout.tsx | 6 +++- 7 files changed, 56 insertions(+), 54 deletions(-) diff --git a/app/components/TopBar.tsx b/app/components/TopBar.tsx index 08eb61fcbf..291578c764 100644 --- a/app/components/TopBar.tsx +++ b/app/components/TopBar.tsx @@ -1,5 +1,6 @@ import { Menu, MenuButton, MenuItem, MenuList } from '@reach/menu-button' -import { useNavigate, useParams } from 'react-router-dom' +import React from 'react' +import { useNavigate } from 'react-router-dom' import { navToLogin, useApiMutation, useApiQuery } from '@oxide/api' import { @@ -9,25 +10,10 @@ import { Notifications16Icon, Profile16Icon, } from '@oxide/ui' -import { isTruthy } from '@oxide/util' import { pb } from 'app/util/path-builder' -import { OrgPicker, ProjectPicker, SiloSystemPicker } from './TopBarPicker' - -type TopBarProps = { - /** - * Passing around flags like this is definitely a code smell, but this is the - * simplest thing for now. This allows `SystemLayout` to tell us when we're on - * a system route (instead of detecting it in an unreliable way by, for - * example, checking what the pathname starts with) while at the same time - * keeping the logic for showing/hiding the system/silo picker based on the - * current user's permissions bundled up in here. - */ - isSystemRoute?: boolean -} - -export function TopBar({ isSystemRoute = false }: TopBarProps) { +export function TopBar({ children }: { children: React.ReactNode }) { const navigate = useNavigate() const logout = useApiMutation('logout', { onSuccess: () => { @@ -38,37 +24,12 @@ export function TopBar({ isSystemRoute = false }: TopBarProps) { }, }) const { data: user } = useApiQuery('sessionMe', {}, { cacheTime: 0 }) - const { data: systemPolicy } = useApiQuery('systemPolicyView', {}) const loggedIn = !!user - // Whether the user is shown /system/* routes is governed by whether they have - // viewer perms (or better) on the fleet. The natural place to look for that - // is the fleet policy, but if the user doesn't have fleet read, they will get - // a 403 from that endpoint. So we simply check whether that endpoint 200s or - // not to determine whether the user is a fleet viewer. - // - // Note that we are able to ignore the possibility of `systemPolicy` being - // undefined because the request is still going because we have prefetched - // this request in the loader. If that prefetch were removed, fleet viewers - // would see the silo picker pop in when the request resolves. Bad. - const isFleetViewer = !!systemPolicy - - const { projectName } = useParams() - - const [cornerPicker, ...otherPickers] = [ - isFleetViewer && , - // TODO: This works ok in most situations, but when an operator user is on - // the orgs page with no org selected, they see this picker, which is - // redundant with the list of orgs. Overall this logic is starting to feel - // silly, which points to a non-centralized approach handled in the layouts - // like we were doing before. That way, for example, we know whether we're - // on a system situation because we're in SystemLayout. Seems pretty obvious - // in hindsight. - !isSystemRoute && , - // doing both checks is redundant but let's be clear - !isSystemRoute && projectName && , - ].filter(isTruthy) + // toArray filters out nulls, which is essential because the silo/system + // picker is going to come in null when the user isn't supposed to see it + const [cornerPicker, ...otherPickers] = React.Children.toArray(children) // The height of this component is governed by the `PageContainer` // It's important that this component returns two distinct elements (wrapped in a fragment). diff --git a/app/components/TopBarPicker.tsx b/app/components/TopBarPicker.tsx index 43c259fc15..6b0b81215d 100644 --- a/app/components/TopBarPicker.tsx +++ b/app/components/TopBarPicker.tsx @@ -104,7 +104,29 @@ const NoOrgLogo = () => ( ) -export function SiloSystemPicker({ isSystemRoute }: { isSystemRoute: boolean }) { +/** + * Return `null` instead of the picker when the user doesn't have fleet viewer + * perms so it can get filtered out of the children array in ``. Having + * `` itself return `null` from render doesn't work because + * `` can't see that. + */ +export function useSiloSystemPicker(value: 'silo' | 'system') { + // User is only shown system routes if they have viewer perms (at least) on + // the fleet. The natural place to find out whether they have such perms is + // the fleet policy, but if the user doesn't have fleet read, we'll get a 403 + // from that endpoint. So we simply check whether that endpoint 200s or not to + // determine whether the user is a fleet viewer. + // + // Note that we are able to ignore the possibility of `systemPolicy` being + // undefined due to the request being in flight because we have prefetched + // this request in the loader. If that prefetch were removed, fleet viewers + // would see the silo picker pop in when the request resolves, which would be + // bad. + const { data: systemPolicy } = useApiQuery('systemPolicyView', {}) + return systemPolicy ? : null +} + +export function SiloSystemPicker({ value }: { value: 'silo' | 'system' }) { const commonProps = { items: [ { label: 'System', to: pb.silos() }, @@ -113,7 +135,7 @@ export function SiloSystemPicker({ isSystemRoute }: { isSystemRoute: boolean }) 'aria-label': 'Switch between system and silo', } - return isSystemRoute ? ( + return value === 'system' ? ( { return ( - + + {useSiloSystemPicker('silo')} + + diff --git a/app/layouts/ProjectLayout.tsx b/app/layouts/ProjectLayout.tsx index 8b73524dd7..afdb5afc8a 100644 --- a/app/layouts/ProjectLayout.tsx +++ b/app/layouts/ProjectLayout.tsx @@ -13,6 +13,7 @@ import { } from '@oxide/ui' import { TopBar } from 'app/components/TopBar' +import { OrgPicker, ProjectPicker, useSiloSystemPicker } from 'app/components/TopBarPicker' import { useQuickActions, useRequiredParams } from 'app/hooks' import { pb } from 'app/util/path-builder' @@ -49,7 +50,11 @@ const ProjectLayout = () => { return ( - + + {useSiloSystemPicker('silo')} + + + diff --git a/app/layouts/SettingsLayout.tsx b/app/layouts/SettingsLayout.tsx index 6e4d9f73b6..de8ac6f3d3 100644 --- a/app/layouts/SettingsLayout.tsx +++ b/app/layouts/SettingsLayout.tsx @@ -4,6 +4,7 @@ import { matchPath, useLocation, useNavigate } from 'react-router-dom' import { Divider, Key16Icon, Profile16Icon, Show16Icon } from '@oxide/ui' import { TopBar } from 'app/components/TopBar' +import { OrgPicker, useSiloSystemPicker } from 'app/components/TopBarPicker' import { useQuickActions } from 'app/hooks' import { pb } from 'app/util/path-builder' @@ -37,7 +38,10 @@ const SettingsLayout = () => { return ( - + + {useSiloSystemPicker('silo')} + + {/* TODO: what to link here? anything? */} diff --git a/app/layouts/SiloLayout.tsx b/app/layouts/SiloLayout.tsx index 93ba513305..bca1ac8545 100644 --- a/app/layouts/SiloLayout.tsx +++ b/app/layouts/SiloLayout.tsx @@ -2,6 +2,7 @@ import { Divider, Organization16Icon, Snapshots16Icon } from '@oxide/ui' import { DocsLinkItem, NavLinkItem, Sidebar } from 'app/components/Sidebar' import { TopBar } from 'app/components/TopBar' +import { OrgPicker, useSiloSystemPicker } from 'app/components/TopBarPicker' import { pb } from 'app/util/path-builder' import { ContentPane, PageContainer } from './helpers' @@ -9,7 +10,10 @@ import { ContentPane, PageContainer } from './helpers' export default function SiloLayout() { return ( - + + {useSiloSystemPicker('silo')} + + diff --git a/app/layouts/SystemLayout.tsx b/app/layouts/SystemLayout.tsx index 9be880e375..26658decb6 100644 --- a/app/layouts/SystemLayout.tsx +++ b/app/layouts/SystemLayout.tsx @@ -14,6 +14,7 @@ import { import { trigger404 } from 'app/components/ErrorBoundary' import { DocsLinkItem, NavLinkItem, Sidebar } from 'app/components/Sidebar' import { TopBar } from 'app/components/TopBar' +import { SiloSystemPicker } from 'app/components/TopBarPicker' import { pb } from 'app/util/path-builder' import { ContentPane, PageContainer } from './helpers' @@ -39,7 +40,10 @@ SystemLayout.loader = async () => { export default function SystemLayout() { return ( - + + {/* don't use the hook bc if we're here, this needs to show up */} + + From 1fae0a064130293fa76a1243b53e1f2e64270a50 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 14 Oct 2022 22:08:41 -0500 Subject: [PATCH 6/8] comments and naming tweaks --- app/layouts/SystemLayout.tsx | 11 ++++++----- app/layouts/helpers.tsx | 7 +++++-- app/routes.tsx | 4 ++-- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/app/layouts/SystemLayout.tsx b/app/layouts/SystemLayout.tsx index 26658decb6..0397bd7db2 100644 --- a/app/layouts/SystemLayout.tsx +++ b/app/layouts/SystemLayout.tsx @@ -20,11 +20,12 @@ import { pb } from 'app/util/path-builder' import { ContentPane, PageContainer } from './helpers' /** - * If we can see the policy, we're a fleet viewer, same as in `TopBar`. We need - * to `fetchQuery` instead of `prefetchQuery` because the latter doesn't return - * the result, and then we need to `.catch()` because `fetchQuery` throws on - * request error. We're being a little cavalier here with the error. If it's - * something other than a 403, that would be strange and we would want to know. + * If we can see the policy, we're a fleet viewer, and we need to be a fleet + * viewer in order to see any of the routes under this layout. We need to + * `fetchQuery` instead of `prefetchQuery` because the latter doesn't return the + * result, and then we need to `.catch()` because `fetchQuery` throws on request + * error. We're being a little cavalier here with the error. If it's something + * other than a 403, that would be strange and we would want to know. */ SystemLayout.loader = async () => { const isFleetViewer = await apiQueryClient diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index 5b399de8ab..e631e06640 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -24,10 +24,13 @@ export const ContentPane = () => ( ) -export const prefetchUserData = async () => { +/** Loader for the `` that wraps all authenticated routes. */ +export const userLoader = async () => { await Promise.all([ apiQueryClient.prefetchQuery('sessionMe', {}), - // also fetched by the SystemLayout loader, but RQ dedupes it + // Need to prefetch this because every layout hits it when deciding whether + // to show the silo/system picker. It's also fetched by the SystemLayout + // loader to figure out whether to 404, but RQ dedupes the request. apiQueryClient.prefetchQuery('systemPolicyView', {}), ]) } diff --git a/app/routes.tsx b/app/routes.tsx index c8a260ba04..a385d9070f 100644 --- a/app/routes.tsx +++ b/app/routes.tsx @@ -10,7 +10,7 @@ import RootLayout from './layouts/RootLayout' import SettingsLayout from './layouts/SettingsLayout' import SiloLayout from './layouts/SiloLayout' import SystemLayout from './layouts/SystemLayout' -import { prefetchUserData } from './layouts/helpers' +import { userLoader } from './layouts/helpers' import DeviceAuthSuccessPage from './pages/DeviceAuthSuccessPage' import DeviceAuthVerifyPage from './pages/DeviceAuthVerifyPage' import LoginPage from './pages/LoginPage' @@ -55,7 +55,7 @@ export const routes = createRoutesFromElements( {/* This wraps all routes that are supposed to be authenticated */} - }> + }> }> } /> } handle={{ crumb: 'Profile' }} /> From 35e35a927112a4c2d4d675147b1fc27cd53af4e0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Sat, 15 Oct 2022 22:33:46 -0500 Subject: [PATCH 7/8] let's be extra. test whether silo/system picker pops in --- app/pages/__tests__/top-bar.e2e.ts | 16 ++++++++++++++++ app/test/e2e/utils.ts | 28 ++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 app/pages/__tests__/top-bar.e2e.ts diff --git a/app/pages/__tests__/top-bar.e2e.ts b/app/pages/__tests__/top-bar.e2e.ts new file mode 100644 index 0000000000..b97d6daadd --- /dev/null +++ b/app/pages/__tests__/top-bar.e2e.ts @@ -0,0 +1,16 @@ +import { test } from '@playwright/test' + +import { expectSimultaneous } from 'app/test/e2e' +import { pb } from 'app/util/path-builder' + +test('Silo/system picker does not pop in', async ({ page }) => { + await page.goto(pb.orgs()) + + // make sure the system policy call is prefetched properly so that the + // silo/system picker doesn't pop in. if this turns out to be flaky, just + // throw it out. it's extra as hell + await expectSimultaneous(page, [ + 'role=button[name="Switch between system and silo"]', + 'role=button[name="Switch organization"]', + ]) +}) diff --git a/app/test/e2e/utils.ts b/app/test/e2e/utils.ts index 0de785463f..2b205c69bb 100644 --- a/app/test/e2e/utils.ts +++ b/app/test/e2e/utils.ts @@ -77,3 +77,31 @@ export async function expectRowVisible( .poll(getRows) .toEqual(expect.arrayContaining([expect.objectContaining(expectedRow)])) } + +// const sleep = async (ms: number) => new Promise((res) => setTimeout(res, ms)) +// +// export async function expectSimultaneous(page: Page, selectors: string[]) { +// const getHandles = () => Promise.all(selectors.map((s) => page.$(s))) +// let handles = new Array(selectors.length).fill(null) +// while (handles.every((h) => h == null)) { +// handles = await getHandles() +// // console.log(handles.map((h) => h != null)) +// await sleep(20) +// } +// // if any of them showed up, we want to see that all of them did +// expect(handles.every((h) => h != null)).toBe(true) +// } + +async function timeToAppear(page: Page, selector: string): Promise { + const start = Date.now() + await page.locator(selector).waitFor() + return Date.now() - start +} + +/** + * Assert two elements appeared within 20ms of each other + */ +export async function expectSimultaneous(page: Page, selectors: [string, string]) { + const [t1, t2] = await Promise.all(selectors.map((sel) => timeToAppear(page, sel))) + expect(Math.abs(t1 - t2)).toBeLessThan(20) +} From ec5bfc84d72e9c84affd548456e3fd2be3ef2b48 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Sat, 15 Oct 2022 23:27:48 -0500 Subject: [PATCH 8/8] delete the bad one --- app/test/e2e/utils.ts | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/app/test/e2e/utils.ts b/app/test/e2e/utils.ts index 2b205c69bb..bf56067a69 100644 --- a/app/test/e2e/utils.ts +++ b/app/test/e2e/utils.ts @@ -78,20 +78,6 @@ export async function expectRowVisible( .toEqual(expect.arrayContaining([expect.objectContaining(expectedRow)])) } -// const sleep = async (ms: number) => new Promise((res) => setTimeout(res, ms)) -// -// export async function expectSimultaneous(page: Page, selectors: string[]) { -// const getHandles = () => Promise.all(selectors.map((s) => page.$(s))) -// let handles = new Array(selectors.length).fill(null) -// while (handles.every((h) => h == null)) { -// handles = await getHandles() -// // console.log(handles.map((h) => h != null)) -// await sleep(20) -// } -// // if any of them showed up, we want to see that all of them did -// expect(handles.every((h) => h != null)).toBe(true) -// } - async function timeToAppear(page: Page, selector: string): Promise { const start = Date.now() await page.locator(selector).waitFor()