🐛 Makes `useSupabaseUser` an async function, to resolve the `missing subclaim` race condition when using backend calls after login

ammuench commented 1 year ago

Changes useSupabaseUser as a async function
Rewrite logic to properly encapsulate the current asynchronous logic within the composable with await functions instead

Types of changes

[x] Bug fix (a non-breaking change which fixes an issue)
[ ] New feature (a non-breaking change which adds functionality)
[X] Breaking change (fix or feature that would cause existing functionality to change)

Description

This should help address the problems in #238

Currently, the logic for the useSupabaseUser function calls a promise but does not await the outcome, leading to a potential race condition that may cause any actions based on the value of useSupabaseUser to not have the correct data if it's still pending;

In my current project, this logic causes an issue where I would get the invalid claim: missing sub claim error when I would make a call to an endpoint in my Nuxt /api that was using the serverSupabaseUser method--which is properly handled as an asynchronous method. This would usually only happen on the first call right on login, and resolve after refreshing the page.

Looking into the error, it looked to be caused by not awaiting session data correctly. Given that we use a cookie passed back to the Nuxt API routes to ensure that server-side auth works, I thought that having the composable immediately pass back the user session without properly waiting for the promise to resolve may result in conditions where the session cookie may or may not be properly set, but API calls that look for a cookie to be set may be called with the expectation that it has.

After making the changes below, I no longer have this issue in my project when watching the useSupabaseUser value to make API calls

HOWEVER, I'm aware this is a breaking change. I'm not sure why we do not currently await the promise within the composable as it stands, but if there is a legitimate reason, I'd kindly ask that we at least offer a new composable called useAsyncSupabaseUser or something similar to allow this functionality for those who want it, or are experiencing similar issues to mine

Checklist:

[X] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
- Happy to add these, I want to make sure these changes make sense before continuing
[ ] I have added tests to cover my changes (if not applicable, please state why)
- Happy to add these, I want to make sure these changes make sense before continuing

netlify[bot] commented 1 year ago

Deploy request for n3-supabase pending review.

Visit the deploys page to approve it

Name	Link
Latest commit	353bcb46ed9be3bfba48cb35aded04a459bfab24

ammuench commented 1 year ago

Also, while going through all of this, there appears to be inconsistencies with the code published on npm and the code tagged in the corresponding release.

For example, on a fresh install from npm the code for auth-redirect.mjs looks like this:

However, when we look at the code, that async call was removed from line 8 here prior to the v1.1.0 release: https://github.com/nuxt-modules/supabase/commit/4a35428e#diff-091d09dee0c693155b3a51eafd6b40bbb076227efb2d5964900088fb6587fc3cR9

This doesn't seem like the only instance of inconsistencies in the deployed code and the source (various imports seem to be inconstant with what's built locally from the 1.1.0 tag and what's on NPM currently), I would hope that it gets looked into to ensure there is consistency in the code being deployed and the corresponding source.

larbish commented 1 year ago

@ammuench Thanks for this PR and thanks for spotting the inconsistency between deployed version and tagged one. I should have missed something when I've published on npm. I've created a new version v1.1.2 to fix this. Could you please try with this version and tell me if it fixes your issue.

If it's not fixing this issue. Could you please update your PR and create a new useAsyncSupabaseUser to avoid breaking changes (just like you proposed). Could you also update the doc with this new composable and explain in what case it makes sense to use it. Thanks a lot for your contribution !

ammuench commented 1 year ago

@ammuench Thanks for this PR and thanks for spotting the inconsistency between deployed version and tagged one. I should have missed something when I've published on npm. I've created a new version v1.1.2 to fix this. Could you please try with this version and tell me if it fixes your issue.

If it's not fixing this issue. Could you please update your PR and create a new useAsyncSupabaseUser to avoid breaking changes (just like you proposed). Could you also update the doc with this new composable and explain in what case it makes sense to use it. Thanks a lot for your contribution !

Thanks for the response and the prompt updates!

Switching to v1.1.2 and updating my confirm page code to the example provided seems to have resolved the race condition issue on my end, which is great!

However, if it's okay, I still think there would be value in creating an async version of the useSupabaseUser composable. I know that the currently system is set around establishing the ref with the knowledge that a promise will resolve, update, and cause change effects when the promise eventually resolves and sets a new value for the ref, but for projects using the auth as a gateway to the backend, I think that having a true async method which reliably returns a resolved value the first time would be useful for building out flows in a project relying more on the backend.

I can rework those features into this PR or make a new one if you think there is value there.

larbish commented 1 year ago

Good news! Concerning the useAsyncSupabaseUser composable, it seems ok to create it. Only thing is: I don't want to add some code and extra feature if nobody is using it. If even the PR writer is not using the feature, I prefer to wait for a specific request. But if at some point you need it, don't hesitate to create a new PR.

ammuench commented 1 year ago

Sounds good, will close for now

juni0r commented 1 year ago

Ok, so +1 for adding useAsyncSupabaseUser.

Rationale:

I think it makes for a better API and DX if an operation that is inherently async will make this explicit.
Returning a user ref requires using watch to actually determine when the user has been loaded.
The useSupabaseUser composable can internally use useAsyncSupabaseUser and just update the ref and not duplicate code.

For now, I will just implement this composable myself since a Promise<User> makes way more sense over using watch in my code.

nuxt-modules / supabase