Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: getUser returns null if there is no session #876

Merged
merged 1 commit into from
Apr 10, 2024

Conversation

hf
Copy link
Contributor

@hf hf commented Apr 6, 2024

If there is no session (i.e. no JWT/access token), getUser() cannot possibly work. It now returns null user.

Previously, it just sent out a request without the JWT. When combined with @supabase/supabase-js which does some clever tricks with fetch by adding a default Authorization header using the Supabase anon API key, if you called getUser() at the wrong time an error such as missing sub claim error message would be thrown by Supabase Auth.

(Unfortunately the Supabase anon API key is signed with the same JWT secret, so it's hard to disambiguate why this is happening.)

Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also add this check for all other auth authorized endpoints that require the access token and is wrapped by useSession or else they will return the same invalid sub claim error if no session is present

@@ -1198,6 +1198,11 @@ export default class GoTrueClient {
throw error
}

if (!data.session?.access_token) {
// if there's no access token, the user can't be fetched
return { data: { user: null }, error: null }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm should we consider returning the AuthSessionMissingError instead of null? i feel like that makes the DX better because the developer can check against that and redirect the user to the login page

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say returning null is best here, because getUser is just trying to read information; similar to getSession.

Currently, the AuthSessionMissingError is only thrown for methods which try to change or set something - update a user, refresh a session, set a session, reauthenticate; none of which will succeed unless there is a session. By contrast, getUser has no real success/fail characteristic - the user info is either there or it isn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this can't return an error, or it's going to cause issues with #874

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll return error, and deal with #874 differently...

@hf hf force-pushed the hf/fix-getUser-with-no-session branch from d5ed6f9 to 8c4a6a7 Compare April 10, 2024 13:54
@hf hf merged commit 6adf8ca into master Apr 10, 2024
3 checks passed
@hf hf deleted the hf/fix-getUser-with-no-session branch April 10, 2024 13:58
kangmingtay pushed a commit that referenced this pull request Apr 18, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.63.1](v2.63.0...v2.63.1)
(2024-04-18)


### Bug Fixes

* `getUser` returns null if there is no session
([#876](#876))
([6adf8ca](6adf8ca))
* implement exponential back off on the retries of `_refreshAccessToken`
method ([#869](#869))
([f66711d](f66711d))
* update session warning
([#879](#879))
([3661130](3661130))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
J0 added a commit that referenced this pull request Apr 25, 2024
J0 added a commit that referenced this pull request Apr 25, 2024
J0 added a commit that referenced this pull request Apr 25, 2024
…" (#889)

This reverts commit 6adf8ca.

Revert check for access token before fetching user. While potentially
beneficial, we'll need to update our guides and address a few other
issues before we can proceed with this change
J0 pushed a commit that referenced this pull request Apr 25, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.64.0](v2.63.2...v2.64.0)
(2024-04-25)


### Features

* remove `cache: no-store` as it breaks cloudflare
([#886](#886))
([10e9d38](10e9d38))


### Bug Fixes

* Revert "fix: `getUser` returns null if there is no session
([#876](#876))"
([#889](#889))
([6755fef](6755fef))
* revert check for access token in header
([#885](#885))
([03d8ba7](03d8ba7))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
hf added a commit that referenced this pull request Apr 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants