feat: add auth fallback logic for when official Coder components are not mounted #128
Conversation
| @@ -46,7 +46,8 @@ | |||
| "valibot": "^0.28.1" | |||
| }, | |||
| "peerDependencies": { | |||
| "react": "^16.13.1 || ^17.0.0 || ^18.0.0" | |||
| "react": "^16.13.1 || ^17.0.0 || ^18.0.0", | |||
| "react-dom": "^16.13.1 || ^17.0.0 || ^18.0.0" | |||
There was a problem hiding this comment.
Needed to bring in React DOM as a peer dependency so I could start using React portals
There was a problem hiding this comment.
All the test cases are still here – it's just that they've now been split up between CoderAuthForm and CoderAuthFormCardWrapper
| let Wrapper: FC<PropsWithChildren<unknown>>; | ||
| switch (type) { | ||
| case 'card': { | ||
| Wrapper = CoderAuthCard; | ||
| break; | ||
| } | ||
| default: { | ||
| assertExhaustion(type); | ||
| } | ||
| } |
There was a problem hiding this comment.
This was my original attempt at trying to make CoderAuthWrapper be the one-stop-shop component for every auth component, but it just ended up being too wonky and restrictive:
- Every component either has to accept zero props or the exact same props, no matter the use case
- Up until now, I didn't have another component to try building with, so I never had a way to test if this design would actually hold up
Ended up making it so that there's a general CoderAuthForm component that offers the main functionality, and then it can be composed into more specialized components (so far that's CoderAuthFormDialog and CoderAuthFormCardWrapper)
| throw new Error( | ||
| 'Tried to process authenticated user after main content should already be shown', | ||
| ); | ||
| return <CoderAuthSuccessStatus />; |
There was a problem hiding this comment.
I shout this out in one of the comments, but this change was made so that if anything ever goes slightly awry with some of the auth checks, there's no way that we'll throw an error directly in the UI
If all goes well, the user will never see this component, but always good to have some double bookkeeping
There was a problem hiding this comment.
Good call, makes a lot of sense
| @@ -65,44 +66,49 @@ const InnerRoot = ({ | |||
| const headerId = `${hookId}-header`; | |||
|
|
|||
| return ( | |||
| <CoderAuthWrapper type="card"> | |||
There was a problem hiding this comment.
This was one of the big problems with the previous design: you had a Card wrapper, but then inside it, you had another card.
Not terrible, but the problem was that you had to make sure that the auth card wrapper and the main content card wrapper were styled exactly the same and had to stay in sync. Obviously not great, so I refactored it so that there's only one source of the card styling now
| export const CoderAuthWrapper = coderPlugin.provide( | ||
| createComponentExtension({ | ||
| name: 'CoderAuthWrapper', | ||
| component: { | ||
| lazy: () => | ||
| import('./components/CoderAuthWrapper').then(m => m.CoderAuthWrapper), | ||
| }, | ||
| }), | ||
| ); | ||
|
|
There was a problem hiding this comment.
Still undecided on whether I'll be exposing the new auth components to the end user; going to wait until the vendored SDK is plugged in, so I can get a sense for whether they're really needed
| const [authToken, setAuthToken] = useState( | ||
| () => window.localStorage.getItem(TOKEN_STORAGE_KEY) ?? '', | ||
| ); |
There was a problem hiding this comment.
Inlined the previous function because it was only used in one spot, and also, with the file getting so big, I wanted as few top-level values hanging around the module as possible
| @@ -152,50 +140,140 @@ export const CoderAuthProvider = ({ children }: CoderAuthProviderProps) => { | |||
| // outside React because we let the user connect their own queryClient | |||
| const queryClient = useQueryClient(); | |||
| useEffect(() => { | |||
There was a problem hiding this comment.
Effect logic is exactly the same as before; just refactored it slightly for readability
|
|
||
| type CoderAuthProviderProps = Readonly<PropsWithChildren<unknown>>; | ||
| function useAuthState(): CoderAuth { |
There was a problem hiding this comment.
I know Kayla wouldn't be the biggest fan of having a bunch of ad-hoc custom hooks that are only used in one spot, but just because there's so much state, and the different "clusters" of state generally don't interact with each other, it felt better to extract them, to limit how much you have to keep in your mind at once
There was a problem hiding this comment.
There's a lot going on in this file; might be better to start at the bottom to see how things end up coming together
There was a problem hiding this comment.
The main thing to keep in mind is that we basically have three "categories" of component that can interact with the rest of the Coder plugin logic:
- Official coder components that should be tracked. If none of these are screen, we know we should display a fallback auth UI
- Official coder components that should not be tracked. These are the components used in the fallback auth UI.
- The key thing, though, is that we need to make sure that we don't accidentally track any of these components. If the condition for displaying the auth fallback is that we have no tracked components, then the moment we put a tracked component on the screen that's supposed to help with the auth fallback, then the fallback will immediately disappear
- Components made by the user that use the Coder SDK. Should not interact with the tracking logic at all, ever
I mention this in one of my code comments, but one of my main goals was to make it so that someone could make a single component that can satisfy (1) and (2) – without having to do anything different, or even being aware that the categories even exist. This file gets a little chaotic to accommodate that, but better to keep all the chaos in one file
| * there are no official Coder components that are able to give the user a way | ||
| * to do that through normal user flows. | ||
| */ | ||
| export function useInternalCoderAuth(): CoderAuth { |
There was a problem hiding this comment.
Not sure if there are better names for useInternalCoderAuth and useEndUserCoderAuth, but I wanted the names to communicate that only one of them should be used for the internal plugin code, and the other one is strictly for the end user
There was a problem hiding this comment.
__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED
In seriousness, sounds like reasonable names to me. Part of me wants to use External to parallel Internal but I feel like "external auth" already has a different meaning so "end user" seems better.
| /** | ||
| * Add additional padding to the bottom of the main app to make sure that even | ||
| * with the fallback UI in place, it won't permanently cover up any of the | ||
| * other content as long as the user scrolls down far enough. | ||
| * | ||
| * Involves jumping through a bunch of hoops since we don't have 100% control | ||
| * over the Backstage application. Need to minimize risks of breaking existing | ||
| * Backstage styling or other plugins | ||
| */ | ||
| const fallbackRef = useRef<HTMLElement>(null); | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
The things you do to avoid breaking other people's code
|
About to review, but I wanted to check if my intuition here is right: when we switch to OAuth, I imagine some/most of this goes away? Since by just asking for the token, that triggers Backstage's UI for the OAuth flow, and it resolves once that flow is done or canceled. |
There was a problem hiding this comment.
Looks good to me! Some really neat ideas here, and I felt like it was organized in a way that made it easy to digest.
I am tempted to say we should have only the standalone/popup version, and it appears when something tries to get a token. If the user closes out of it, that token call resolves to undefined (or throws), and the dialog does not reappear until you refresh/renavigate (since that causes the component to request the token again, triggering the popup again, although we could also have a "try again" button for our components at least that could be used re-request a token, thus triggering the popup again). That would match how the OAuth stuff will work (at least I think that was how it worked, last I tried it).
@code-asher Possibly? I was under the impression that we'd be keeping both forms of auth (though obviously pushing users to prefer the OAuth version). In which case, this might not go away, but just become more deemphasized Depending on what oauth looks like, we could make it so that these main auth components never actually mount, and you don't have to worry about the fallback UI |
Closes #111
Did not expect this to the hardest part of the umbrella issue.
Video demo: https://github.com/coder/backstage-plugins/assets/28937484/1a859f52-168c-436e-aa60-014bc36c97f1
Changes made
CoderProviderbeing rendered, a fallback auth input will appear while the user is not authenticatedNotes
Remaining pieces for umbrella issue
useQuery/useMutationthat are pre-wired with auth for all relevant properties)