mitchelvanbever / remix-auth-supabase

MIT License
98 stars 14 forks source link

Refresh token in nested route #10

Closed jaPajaap closed 2 years ago

jaPajaap commented 2 years ago

When an access token is expired and check session is called from multiple nested routes loaders in parallel, the token is refreshed multiple times in parallel as well. This results in an error because each refresh token can only be used once. Any advice how to go about this case? Perhaps the strategy could redirect the user to a specific refresh token route and redirect to success or failure from there?

rphlmr commented 2 years ago

Hi @jaPajaap

I can reproduce what you describe and because I'll face the same issue soon, I propose https://github.com/mitchelvanbever/remix-auth-supabase/pull/11

Is it what you ask for ?

jaPajaap commented 2 years ago

Yep thanks that approach looks good

jaPajaap commented 2 years ago

I just tested your solution and still run into the same issue, I believe there is no pending promise yet because check session is called in parallel.

rphlmr commented 2 years ago

Hum, just to be sure, how did you test ? Npm link ?

jaPajaap commented 2 years ago

I think I found the issue, on a certain private page I was using multiple <Link prefetch="intent" />

rphlmr commented 2 years ago

Be sure to : On remix-auth-supabase folder pnpm install pnpm build npm link

On your project folder : npm link remix-auth-supabase

(npm unlink remix-auth-supabase to cancel).

By design, the fix "should" works most of the time but i fail design a unit test to be 💯 confident 😅 I hope event loop guarantee that the pending promise is set first and next tick only await it.

If you confirm it's still not working with the linking process I mention, i'll try to run remix with debugger.

(sorry for my English 😅)

edit : i'll try with link and prefetch 👍

jaPajaap commented 2 years ago

I can replicate the issue in this example app using nested routes and outlets. Login, get redirected to /private, wait for the token to expire, click the /private/1/details link

rphlmr commented 2 years ago

I'm sorry but there is no /private/1/details route 😬

image

Did I miss something ?

jaPajaap commented 2 years ago

Sorry I linked to the wrong branch 🙈 , here you go: https://github.com/jaPajaap/remix-auth-supabase/tree/fix/checkssion-concurrent-mode/examples/nested-routes

rphlmr commented 2 years ago

Thanks :D I can reproduce. I'm checking what happened :)

Edit : It's randomly working :O

jaPajaap commented 2 years ago

I was thinking to approach this as following, what do you think?

rphlmr commented 2 years ago

I see what you mean but the problem here is that, on dev mode, each route has it's own supabaseStrategy instance 😱 and didn't share pendingHandleRefreshToken

I think it's a remix behaviour on dev mode. (https://remix.run/docs/en/v1/other-api/serve) I see some export pattern for other package (prisma) that make it singleton to prevent multiple database connection in dev mode.

So, I push a fix that :

Let me know if you see a difference.

To test, I set my token validity to 1s (😂) and try to click multiple time every where. Have to notice that a high click rate on the same link (with a really short token expire time) will necessarily trigger a refresh error. But is it a real use case ?

mitchelvanbever commented 2 years ago

Thanks for opening the issue. This is probably something that one will run into when the applications get more complex.

Lot of activity going on here! I haven't had time this weekend to give it a try and come up with a solid solution, but to me it seems like @rphlmr is on the right path.

I'll have a look at the PR first thing after work!