689 authentication #1320
689 authentication #1320
Conversation
…for managing user session
…ess token when it has expired
| @@ -74,6 +74,7 @@ module.exports = { | |||
| "@typescript-eslint/require-await": "off", | |||
| "@typescript-eslint/restrict-template-expressions": "off", | |||
| "@typescript-eslint/unbound-method": "off", | |||
| "arrow-body-style": "off", | |||
There was a problem hiding this comment.
I think this is a not necessarily helpful, and I am getting tired of the linter yelling at me every time I need to change a function from being implicitly returned to being in block body when extra logic is needed
app/components/AppProvider.tsx
Outdated
| const contextValue = useMemo(() => { | ||
| const authClient = new auth0.WebAuth({ | ||
| audience: config.AUTH0_AUDIENCE, | ||
| clientID: config.AUTH0_CLIENT_ID, | ||
| domain: config.AUTH0_DOMAIN, | ||
| redirectUri: config.AUTH0_REDIRECT_URI, | ||
| responseType: "token id_token", | ||
| }); | ||
|
|
||
| return { | ||
| userLocation, | ||
| authState, | ||
| setAuthState, | ||
| authClient, | ||
| }; | ||
| }, [authState, userLocation]); |
There was a problem hiding this comment.
Two things: 1) I think we probably don't want to put the entire thing in the useMemo(), and 2) I don't think we actually want to use useMemo() at all for this.
The reason we shouldn't put the entire context in the useMemo() is because we'll be needlessly invalidating the memoization any time authState or userLocation changes, but I believe the real thing you're trying to not run multiple times is the WebAuth() object. Since it looks like that object's credentials are all static, not influenced by any of the props or state in this component, we should really just be memoizing that and not the other, cheaper to create values.
The reason we shouldn't use useMemo() here is because it isn't guaranteed to actually cache at all. If the WebAuth object contains any mutable state, we really ought to be storing this in something that is stored globally outside of React, either as a global variable, or possibly with a storage slot created with useRef(). Initializing and access the global variable/ref would have to be done with useEffect(), similar to this example in the docs for useEffect(): https://react.dev/reference/react/useEffect#controlling-a-non-react-widget
If it doesn't have any mutable state, then it might be fine to just store it in a useState(), where we never call the setter method, so it'll always use the initial value.
Actually, it looks like there's an official auth0-react library, which basically handles all this stuff for us: https://github.com/auth0/auth0-react. It's probably worth using, since it takes care of this state management for us, and it seems to provide nice hooks for us to use, which might even mean that we might not need most of the changes in this file.
There was a problem hiding this comment.
The one unfortunate issue with Auth0-React for our purposes is that it does not support embedded passwordless login; i.e., an embedded form that sends user credentials to Auth0's auth servers without a redirect to an Auth0 interstitial page. Auth0 is really pushing their universal login flow, which involves the user being redirected to an interstitial page under Auth0's domain where they enter email/verification code credentials; thus, they do not support it in their React SDK. That was my reason for not using the Auth0-React library since our designs use an embedded form.
Below is some discussion on auth0-react and embedded passwordless login. It looks like they have no intention of allowing it. Anyhow, in the meantime, I will look into replacing the useMemo hook with useState or useRef.
There was a problem hiding this comment.
In hindsight it probably would have been a good bit easier had we chosen the universal flow, but here we are...
There was a problem hiding this comment.
FWIW, I was racking my brain trying to remember why/how I decided to use useMemo. I believe that I lifted it from Auth0-React's implementation of their AppContext object. See their code here. That said, I'm still looking into doing this without useMemo
There was a problem hiding this comment.
I tried defining the context value with useState, but when I do this, the user's authentication state does not propagate downwards after a successful login; this is evident because the "Log Out" button does not appear after auth'ing the user (note: it does propagate on refresh). I have confirmed that the authState is updated in the Context component but not in its children components. I'm not quite sure why this is. My only guess is that there is some kind of weirdness going on given that the context value basically a state value that is referencing other state values and that when these other state values update, it does not update for some reason.
For experimental purposes I tried defining the context as a simple const var. Then, I got an interesting linter error: The 'contextValue' object (at line 74) passed as the value prop to the Context provider (at line 129) changes every render. To fix this consider wrapping it in a useMemo hook.
This makes me think that maybe useMemo is a more common practice for the React AppContext API? This is admittedly getting to the edge of my React knowledge, but I have also seen some discussion online about using useMemo in the AppContext provider. In addition to the Auth0 react app using it in their AppContext provider (see my link above).
|
Also, it was neat checking out the version of this that you deployed to qaone! However, I noticed that email for passwordless signup went to my spam folder: 
I think this means that the email wasn't sent from a domain with email authentication stuff like SPF and DKIM. Are the emails like this only because they're on some development-mode account, or is there extra work we'll need to do to ensure that the emails being sent out will be properly authenticated? |
…s security reasons related to Auth0's passwordless protocol, we will just log the user in without incident
… into 689-authentication
There was a problem hiding this comment.
Sorry for being late to leave another round of review, but thanks for addressing my last round of feedback! I managed to sit down and look through this some more. I'm afraid that we may need a third round after this, since I still haven't gotten a chance to take a look at every file yet.
I think there's still something that's a bit off about how state is being managed and synchronized between the Auth0 client, local storage, the AppProvider component and the AppContext, but I'm still sort of wrapping my head around it so I don't have any concrete suggestions other than my various inline comments. I've been trying to focus my review on this layer, since I assume most of the actual UI components for signing up, logging in, and logging out are relatively straightforward.
Overall, though, this looks great! If it would help with speeding up the review and revision feedback loops, I'm totally down to spend some time pair reviewing this during our next 1-1. If you feel like this is starting to take too long, then I also think this is probably in decent enough shape to merge, since we can still iterate on the auth stuff even after this merges.
…to exported top-level functions. Other misc PR cleanup.
Thanks so much for the comprehensive review! Your suggestions are very helpful and much appreciated. I've made most of your suggested changes. As for the more complex changes like combining AppProvder/UseContext files and removing the |
…ed bool for authState interface. Set authState object to null if user is logged out rather than use isAuthenticated bool.
Alright! I think made almost all of your suggested changes and added some more cleanup of my own after fixing up some TypeScript definitions. The one thing I didn't change was |
Sweet! I've made a few more changes in light of your comments and will merge it in now |
Adds auth functionality to front-end. UI styling is not complete but that will come in a future PR. Since the auth pages are not known to users yet, I believe that this is okay to merge into Master. Check https://qaone.sfserviceguide.org/log-in and https://qaone.sfserviceguide.org/sign-up to test it out (though it will fully work until this PR is merged in: ShelterTechSF/askdarcel-api#732)