Fix token refresh logic #113
Conversation
We were wrongly assuming that Session.ExpiresIn meant the users session was to expire at that time and we should destroy the session. Session.ExpiresIn property is actually meant to be the entire lifetime of the access token (in seconds). We shouldn't be concerned with "ending sessions" in this library. That is handled by Supabase invalidating refresh tokens. We also were providing the wrong value to ExpiresIn in the `SetSession` method.
On second thought, using the default assigned CreatedAt is correct for session creation.
|
Not sure what the status is for this project WRT maintainer(s). I did go ahead and kick off a build and it's got test failures. |
|
Any updates here? |
|
This is failing because on of a test that expects an exception to be thrown if the "session" is expired which is the logic error this PR is addressing. |
|
The test should be updated to reflect the desired behavior, not just left broken. I'm not actively working on this - not sure who is - but I can't in good conscience accept a PR that breaks the build, and I don't have time to go into the weeds on this. If there's nobody else left maintaining this I can barely justify hitting accept on a build based on code-only review, but not if the PR leads to a breaking build. Note that just deleting the test is also not acceptable. |
|
@azegallo Can you please fix the test? |
4 participants
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
#108 #112 #106 #109
What is the new behavior?
Tokens refresh correctly.
Additional context
We have been wrongly assuming that
Session.ExpiresInrepresented the users Supabase session expiry time. And that we should invalidate the users session when this time expired.The
Session.ExpiresInproperty actually represents the entire lifetime (in seconds) of the access token (if you set a JWT expiry in Supabase of 3600 thenExpiresInshould always be 3600).We should not be concerned with "ending sessions" in this library. That is handled by Supabase invalidating the refresh tokens. So I have removed all of the
Expired()checks.The
SetSessionmethod inClient.cswas also setting an incorrect value forExpiresIn, usingpayload.Expirationwhich is the timestamp for when the token expires, not the expectedexpires_intime. So I have calculated that as the payloads(exp - iat).After these changes the
ExpiresAt()andExpired()methods inSession.csare unused so I have removed them as well as theAnonKeyClientTests.csSessionCalculatesExpiresAtTimeCorrectly()method.