Membership form #7902
Membership form #7902
Conversation
|
Size Change: +14.8 kB (+1%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
| const newToken = generateNewToken(decodedToken, state, formData); | ||
|
|
||
| res.redirect( | ||
| 308, |
There was a problem hiding this comment.
Why a 308 here? I think browsers will cache this very aggressively, which might cause issues
There was a problem hiding this comment.
http status codes aren't my strong suit, but I think you'd previously suggested a 301, and a 308 is equivalent, but as the result of a POST from what I read. But maybe I had the 301 wrong in the first place? Maybe I want 307 instead? Or more likely 302 since the method can (should) change from POST to GET?
There was a problem hiding this comment.
Yeah, not sure why I said 301 lol 🙃 This is interesting - it's definitely not a permanent redirect, so looking at this it's not actually clear that there's anything which is a good fit. This is, I think, a good clue that we should tweak the approach...
-
We're making a POST to our endpoint with our form data (this is fine and good)
-
We want to Do Some Stuff with that data and then POST to the
/continueendpoint of Auth0 (I think?) as per these docs -
We are currently trying to make that POST by giving the client a redirect to it, but that seems hard. Looking into it further, it's explicitly disallowed by the spec!
in response to a request other than GET or HEAD, the user agent MUST NOT automatically redirect the request unless it can be confirmed by the user
-
Solution: I think we should just make that request ourselves in the server-side code (ie with axios or whatever), and then redirect with a 302 to the account page (or an error page).
There was a problem hiding this comment.
I think I get what you mean, so line 21 is currently a redirect - we need that to be a post so we can use something like axios.post() then redirect to error/success page based on that
| const { | ||
| state, | ||
| redirectUri, | ||
| firstName, | ||
| lastName, | ||
| termsAndConditions, | ||
| sessionToken, | ||
| } = req.body; |
There was a problem hiding this comment.
We should be checking that these are populated as expected and returning a 400 if not
|
|
||
| res.redirect( | ||
| 308, | ||
| `${redirectUri}?state=${state}&session_token=${newToken}` |
There was a problem hiding this comment.
Need to consider whether passing an arbitrary redirectUri here could be harmful - elsewhere (callback and login URLs) we have to provide a whitelist. Not sure whether that's required here.
There was a problem hiding this comment.
That's a good point – we wouldn't expect the redirectUri to change would we?
There was a problem hiding this comment.
True, it's always going to be this /continue endpoint right? I think we should be able to construct that from the Auth0 config variables that we already have
There was a problem hiding this comment.
I think we can do as Jamie suggests and construct this from the Auth0 config variables
| }, | ||
| }); | ||
|
|
||
| const RegistrationPage: NextPage<Props> = ({ |
There was a problem hiding this comment.
I think this page probably needs to be split up into a few components
| | 'iat' | ||
| | 'iss' | ||
| | 'sub' | ||
| | 'exp' | ||
| | 'aud' | ||
| | 'state' | ||
| | 'https://wellcomecollection.org/terms_agreed' | ||
| | 'https://wellcomecollection.org/first_name' | ||
| | 'https://wellcomecollection.org/last_name' |
There was a problem hiding this comment.
Fun TS tip (not necessarily better than this, just worth knowing):
const jwtRequiredFields = [
'iat',
'iss',
'sub',
'exp',
'aud',
'state',
'https://wellcomecollection.org/terms_agreed',
'https://wellcomecollection.org/first_name',
'https://wellcomecollection.org/last_name'
] as const;
type RegistrationJwtPayload = Pick<JwtPayload, typeof jwtRequiredFields[number]>There was a problem hiding this comment.
neat – I prefer writing commas at the ends of lines to pipes and spaces at the beginning!
There was a problem hiding this comment.
is the as const important? IDE not shouting at me if I take it off. I guess it must be otherwise TS would just thing jwtRequiredFields was an array of any strings
| error: 'Missing required fields', | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const decodedToken = decodeToken(sessionToken); |
There was a problem hiding this comment.
What if this throws? Something to bikeshed as to whether that's a 401 or a 400
There was a problem hiding this comment.
Part of #7808 to be integrated with #7896
Who is this for?
Self-registered library members
What is it doing for them?
Giving them a page to fill out their names and accept terms once they've initiated the signup process (by entering an email/password in the Auth0 Universal login screen).
The call to
/api/users/:user_iddoesn't work at present. As discussed in standup, not sure whether it is a problem/security concern for this page to be live (either on stage or prod) in a state where it doesn't currently work while we connect it to the identity repo in order to do the updating of the user metadata. What are the options? I had thought of putting it behind a toggle (but if there were security concerns, that wouldn't solve them).TODO
Work out what to send back to Auth0. This PR won't work because it attempts to call the
updateUsermethod via the/api/users/:user_idroute, but that would need to have a password sent along with it