-
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
// 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 comment
The 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 <Route>
that wraps this one and the other one and move this loader to there.
const { data: user, error } = useApiQuery( | ||
'sessionMe', | ||
{}, | ||
{ cacheTime: 0, refetchOnWindowFocus: false } |
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: false
to 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.
app/components/TopBar.tsx
Outdated
const isFleetViewer = !!systemPolicy | ||
|
||
const loggedIn = user && !error | ||
const isSystem = pb.isSystemPath(useLocation().pathname) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
8f8c327
to
8f85fde
Compare
8f85fde
to
d48b356
Compare
// 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) |
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.
Love to see this cleaned up!
<PageContainer> | ||
<TopBar /> | ||
<TopBar> | ||
{useSiloSystemPicker('silo')} |
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.
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 comment
The 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.
* 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. |
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.
Perhaps add a TODO?
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.
Problem is I have no idea how we’d track it, but we could totally have an API endpoint for analytics.
// 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', {}), |
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.
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 useSystemSiloPicker
which should also be from cache. From my understanding if we had it on /session/me
all that would do is remove it from the SystemLayout
loader. I still suspect that may be the better way to approach it long term, but this seems good enough for where we're at.
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.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Need a lint rule for this.
app/test/e2e/utils.ts
Outdated
// } | ||
// // if any of them showed up, we want to see that all of them did | ||
// expect(handles.every((h) => h != null)).toBe(true) | ||
// } |
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.
left in the first version to show how appalling it is
export async function expectSimultaneous(page: Page, sel1: string, sel2: string) { | ||
const [t1, t2] = await Promise.all([timeToAppear(page, sel1), timeToAppear(page, sel2)]) | ||
expect(Math.abs(t1 - t2)).toBeLessThan(20) | ||
} |
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.
this is my greatest creation
e6345e0
to
35e35a9
Compare
'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 comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The real trick here is doing the check for this in the loader, which means
Writing tests for both sides of this is hard because we don't have a way of logging as a less-privileged user in the mock server yet.