-
Notifications
You must be signed in to change notification settings - Fork 13
Hide system/silo picker and 404 on system pages if /system/policy 403s
#1232
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
Changes from all commits
d856e30
c3d5694
f0cdf82
d48b356
02eac6f
1fae0a0
35e35a9
ec5bfc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import { Menu, MenuButton, MenuItem, MenuList } from '@reach/menu-button' | ||
| import { useLocation, 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,19 +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' | ||
|
|
||
| /** | ||
| * 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() { | ||
| export function TopBar({ children }: { children: React.ReactNode }) { | ||
| const navigate = useNavigate() | ||
| const logout = useApiMutation('logout', { | ||
| onSuccess: () => { | ||
|
|
@@ -31,29 +23,13 @@ export function TopBar() { | |
| navToLogin({ includeCurrent: false }) | ||
| }, | ||
| }) | ||
| const { data: user, error } = useApiQuery( | ||
| 'sessionMe', | ||
| {}, | ||
| { cacheTime: 0, refetchOnWindowFocus: false } | ||
| ) | ||
|
|
||
| const loggedIn = user && !error | ||
| const { data: user } = useApiQuery('sessionMe', {}, { cacheTime: 0 }) | ||
|
|
||
| const isSystem = useLocation().pathname.startsWith(pb.system()) // lol | ||
| const { projectName } = useParams() | ||
| const loggedIn = !!user | ||
|
|
||
| const [cornerPicker, ...otherPickers] = [ | ||
| hasSiloPerms && <SiloSystemPicker isSystem={isSystem} key={0} />, | ||
| // 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. | ||
| !isSystem && <OrgPicker key={1} />, | ||
| projectName && <ProjectPicker key={2} />, | ||
| ].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) | ||
|
Comment on lines
+30
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love to see this cleaned up! |
||
|
|
||
| // The height of this component is governed by the `PageContainer` | ||
| // It's important that this component returns two distinct elements (wrapped in a fragment). | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import { Access16Icon, Divider, Folder16Icon, Organization16Icon } from '@oxide/ui' | ||
|
|
||
| import { TopBar } from 'app/components/TopBar' | ||
| import { OrgPicker, useSiloSystemPicker } from 'app/components/TopBarPicker' | ||
| import { useRequiredParams } from 'app/hooks' | ||
| import { pb } from 'app/util/path-builder' | ||
|
|
||
|
|
@@ -12,7 +13,10 @@ const OrgLayout = () => { | |
|
|
||
| return ( | ||
| <PageContainer> | ||
| <TopBar /> | ||
| <TopBar> | ||
| {useSiloSystemPicker('silo')} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nit but it seems strange to me to see a hook inlined in the JSX. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I had it pulled out initially but I moved it inline because I could. I do like that it feels like calling a component, which is what I wish it was. |
||
| <OrgPicker /> | ||
| </TopBar> | ||
| <Sidebar> | ||
| <Sidebar.Nav> | ||
| <NavLinkItem to={pb.orgs()} end> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import { apiQueryClient } from '@oxide/api' | ||
| import { | ||
| Cloud16Icon, | ||
| Divider, | ||
|
|
@@ -10,16 +11,40 @@ 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 { SiloSystemPicker } from 'app/components/TopBarPicker' | ||
| import { pb } from 'app/util/path-builder' | ||
|
|
||
| import { ContentPane, PageContainer } from './helpers' | ||
|
|
||
| export default function SiloLayout() { | ||
| /** | ||
| * 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. | ||
|
Comment on lines
+27
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps add a TODO? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Problem is I have no idea how we’d track it, but we could totally have an API endpoint for analytics. |
||
| */ | ||
| SystemLayout.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 | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doing this here is awesome because it means we don't have to do any janky "is this a system route?" detection. This layout wraps all the system routes in the route config. If we ever want to change the scope of this check to include some other routes, all we have to do is add another |
||
|
|
||
| export default function SystemLayout() { | ||
| return ( | ||
| <PageContainer> | ||
| <TopBar /> | ||
| <TopBar> | ||
| {/* don't use the hook bc if we're here, this needs to show up */} | ||
| <SiloSystemPicker value="system" /> | ||
| </TopBar> | ||
| <Sidebar> | ||
| <Sidebar.Nav> | ||
| <DocsLinkItem /> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,13 @@ export const ContentPane = () => ( | |
| </div> | ||
| ) | ||
|
|
||
| export async function prefetchSessionMe() { | ||
| await apiQueryClient.prefetchQuery('sessionMe', {}) | ||
| /** Loader for the `<Route>` that wraps all authenticated routes. */ | ||
| export const userLoader = async () => { | ||
| await Promise.all([ | ||
| apiQueryClient.prefetchQuery('sessionMe', {}), | ||
| // 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', {}), | ||
|
Comment on lines
+31
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the comments. The query being spread across multiple loaders and a hook doesn't feel great to me, but I get why it's needed and the benefit of this approach. Prefetched here, fetched in the system loader (which could be from cache), and queried via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, it feels like a lot but also necessary. I’m thinking about how to test that these things are hooked up correctly, but it’s pretty tough to be thorough. Like we can obviously assert that the silo/system picker is there or not depending on the current user (at least, we can when we add some form of authz to the mock server) but testing whether there’s pop-in is pretty hard because the timings are tight. Working on it. |
||
| ]) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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: <Success16Icon />, | ||
| }) | ||
| navigate(searchParams.get('state') || '/') | ||
| navigate(searchParams.get('state') || pb.orgs()) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Need a lint rule for this. |
||
| }, | ||
| onError: () => { | ||
| addToast({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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"]', | ||
| ]) | ||
| }) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -77,3 +77,17 @@ export async function expectRowVisible( | |
| .poll(getRows) | ||
| .toEqual(expect.arrayContaining([expect.objectContaining(expectedRow)])) | ||
| } | ||
|
|
||
| async function timeToAppear(page: Page, selector: string): Promise<number> { | ||
| 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) | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is my greatest creation |
||

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved
refetchOnWindowFocus: falseto the global config. I am less nervous about making a move on #1178 now that the big demo is done.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which is a reminder @benjaminleonard that we should think more holistically about this from a design perspective. Having things pop out from under you is a bad experience but having to refresh over and over to see changes is also not great. I think the key here is giving explicit control in a way that's intuitive.
I think it'll be a big enough part of the experience that we need to communicate how to tweak this to users during on boarding.