Chore/update remix auth GitHub #944
Conversation
Update authentication libraries to latest versions: - remix-auth from 3.7.0 to 4.1.0 - remix-auth-github from 1.7.0 to 3.0.2 Refactor authentication implementation to match new library requirements, including: - Removing connection session storage - Updating GitHub strategy configuration - Adjusting authentication callback handling
|
Sorry, there is one test failing, for some reason we don't receive the throw from the loader... |
| export const connectionSessionStorage = createCookieSessionStorage({ | ||
| cookie: { | ||
| name: 'en_connection', | ||
| sameSite: 'lax', // CSRF protection is advised if changing to 'none' | ||
| path: '/', | ||
| httpOnly: true, | ||
| maxAge: 60 * 10, // 10 minutes | ||
| secrets: process.env.SESSION_SECRET.split(','), | ||
| secure: process.env.NODE_ENV === 'production', | ||
| }, | ||
| }) | ||
|
|
There was a problem hiding this comment.
I'm confused why changing this was necessary because it looks like we've now lost the ability to display errors as the user goes between different pages in the OAuth flow. And I don't think that this change was necessary for upgrading to Remix auth. Can you help me understand why this change was made?
There was a problem hiding this comment.
This is no needed anymore since Remix Auth doesn't receive a session storage instance anymore, instead the strategy uses @mjackson/headers to save the data it needs in a cookie during the auth process.
Then after the user is back in the app from GitHub it you need to save the data in your session storage yourself, AFAIK you already did this anyway so there's no change in that part.
There was a problem hiding this comment.
Also related to error handling, now the strategies always throw an error, so you handle them with a try/catch
| @@ -35,7 +35,7 @@ test('when auth fails, send the user to login with a toast', async () => { | |||
| consoleError.mockImplementation(() => {}) | |||
| server.use( | |||
| http.post('https://github.com/login/oauth/access_token', async () => { | |||
| return new Response('error', { status: 400 }) | |||
| return new Response(null, { status: 400 }) | |||
There was a problem hiding this comment.
I had to change from "error" to null because the test was in timeout, stuck in the await response.body.cancel() call inside the arctic library. Cf. screenshots.
|
Thanks for your feedback. Grad to help :) Of course this still has to be tested on a deployed server ;) |
|
Well, it took a couple tries, but it's working in production now 😅 |
|
Well done 👍 |
|
Seriously, thank you! I had tried upgrading remix auth once or twice and I just never dug in deep enough to figure out what changes needed to happen so it's been in the back of my mind for a while. I'm so glad to have this now! |
This PR update remix-auth and remix-auth-github to the latest version, to close #932 .
The main chalenge in this PR was to update the mock implementation done by remix-auth-github.
Test Plan
This needs to be deployed and tested with Github.
Checklist
Screenshots