Fix token refresh logic

[azegallo]

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

• Token refresh logic rejects refreshes if the access token is expired.
• In many (most?) use cases the token will never refresh.

#108 #112 #106 #109

What is the new behavior?

Tokens refresh correctly.

Additional context

We have been wrongly assuming that Session.ExpiresIn represented the users Supabase session expiry time. And that we should invalidate the users session when this time expired.
The Session.ExpiresIn property actually represents the entire lifetime (in seconds) of the access token (if you set a JWT expiry in Supabase of 3600 then ExpiresIn should 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 SetSession method in Client.cs was also setting an incorrect value for ExpiresIn, using payload.Expiration which is the timestamp for when the token expires, not the expected expires_in time. So I have calculated that as the payloads (exp - iat).

After these changes the ExpiresAt() and Expired() methods in Session.cs are unused so I have removed them as well as the AnonKeyClientTests.cs SessionCalculatesExpiresAtTimeCorrectly() method.

[Geek-bule]

i need this fix

[wiverson]

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.

[jaimeatsherpa]

Any updates here?

[azegallo]

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.

[wiverson]

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.

[jaimeatsherpa]

@azegallo Can you please fix the test?

[azegallo]

@wiverson @jaimeatsherpa I've updated logic around refresh to destroy sessions when invalid refresh tokens are attempted and updated the failing tests. Please let me know if you need anything else.

[wiverson]

@acupofjose - I haven't been working on this repo for some time. Two users submitted PRs that seem fine. I went ahead and accepted them. It looks like the repo isn't set up to release w/o credentials, I'm guess you did that in the past via local updates. Are you up for a) going ahead and kicking off a release build to pick up those changes and b) adding a note to the README WRT current status for the project?

[wiverson]

@jaimeatsherpa @azegallo FYI I went ahead and accepted two PRs, this one an another minor one adding an error code. Waiting to see if there is a working release process...

[acupofjose]

Thanks y’all for your work! I don’t have admin access to the repos anymore, so I’m not able to help.

However, usually a fresh release would be done by pushing to semantically versioned branch (release/x.x.x) and then GitHub actions would follow up on the push to nuget (provided the api key is still valid).

Then an update of dependencies on the main supabase-csharp and another release branch in the same way.

[wiverson]

@acupofjose Is a anyone else officially in charge now? Or am I the closest thing?

[acupofjose]

As far as I know, yes, but I've been out of context on this for the last year and a half. I reached out to the supabase staff when I left to let them know, but it's all in their hands.