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: don't refresh session if autoRefreshToken setting is set to false #925

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

thomasconner
Copy link

@thomasconner thomasconner commented Jun 17, 2024

What kind of change does this PR introduce?

Bug fix - Supabase client will obey autoRefreshToken setting when setting a session with an expired access token

What is the current behavior?

When a Supabase client is initialized with { auth: { autoRefreshToken: false } }

const client = createClient(options.supabaseUrl, options.supabaseKey, {
    auth: { autoRefreshToken: false }
});

but a session is set with client.setSession() and the access token has expired the client will disregard the autoRefreshToken setting and attempt to refresh the session.

What is the new behavior?

The client will not attempt to refresh an expired session when it is set with client.setSession() and autoRefreshToken is set to false.

@j4w8n
Copy link
Contributor

j4w8n commented Jun 17, 2024

What's your use case here, and what issue are you running into?

@thomasconner
Copy link
Author

thomasconner commented Jun 18, 2024

I am using the Supabase JavaScript client in a NestJS service. This service retrieves the access token from the incoming request to set the session. This setting of the session should not refresh the token automatically on the server side but instead just return an UnauthorizedError by the service to the requesting application. It is up to the requesting application to then properly refresh the access token.

@thomasconner
Copy link
Author

I removed the change that makes the refresh_token optional on a session. This minimizes the changes but still achieves the behavior I would expect.

@j4w8n
Copy link
Contributor

j4w8n commented Jun 19, 2024

After setting the session, do you use methods like getSession() and getUser()?

If not, then this PR would be more applicable once it's released: supabase/supabase-js#1004

@thomasconner
Copy link
Author

thomasconner commented Jun 19, 2024

In my opinion, supabase/supabase-js#1004 and my PR solve/fix two different things. supabase/supabase-js#1004 solves the problem of providing an access token to the Supabase client to make API requests. However, my PR I believe still is fixing a bug where the autoRefreshToken setting is not obeyed when calling supabase.setSession().

I would end up using the new API on the server side as described in that PR you linked to. That is a nice solution.

@thomasconner
Copy link
Author

Do you have anymore feedback regarding this PR?

@j4w8n
Copy link
Contributor

j4w8n commented Jun 27, 2024

Do you have anymore feedback regarding this PR?

Moving this PR forward isn't up to me, as I don't have approval permissions. I know the auth team can get pretty busy, so I'm not sure if/when they will comment.

I think there could be some more things to think about with this, because the _loadSession method also checks for token expiration.

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.

2 participants