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

feat(auth): auto refresh token #353

Closed
wants to merge 12 commits into from
Closed

feat(auth): auto refresh token #353

wants to merge 12 commits into from

Conversation

grdsdev
Copy link
Collaborator

@grdsdev grdsdev commented Apr 23, 2024

What kind of change does this PR introduce?

Fix #77

What is the current behavior?

An access token is currently refreshed only when retrieved before calling an endpoint.

What is the new behavior?

A part of the behavior specified above, now periodically checks for the token in the background and refreshes it upfront, the same behavior as JS and Flutter libs.

@grdsdev grdsdev marked this pull request as draft April 23, 2024 17:06
@@ -71,7 +71,7 @@ public struct Session: Codable, Hashable, Sendable {

/// UNIX timestamp after which the ``Session/accessToken`` should be renewed by using the refresh
/// token with the `refresh_token` grant type.
public var expiresAt: TimeInterval?
public var expiresAt: TimeInterval
Copy link
Collaborator Author

@grdsdev grdsdev Apr 24, 2024

Choose a reason for hiding this comment

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

@hf @kangmingtay or @J0

Do you know if this is guarantee to always be returned in the Session? It was optional for some reason on Swift.

Copy link

Choose a reason for hiding this comment

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

Should be - or at least we guarantee it in the spec: https://github.com/supabase/auth/blob/master/openapi.yaml#L1920

I think we added the ExpiresAt after the other properties which might be why the author defined it as optional then for backward compatibility during the transition from an older Auth version to the newer one

@grdsdev grdsdev marked this pull request as ready for review April 24, 2024 11:26
@ypotsiah
Copy link

ypotsiah commented Apr 24, 2024

If we have infinite lifetime for the refresh token (according to docs) seems better to verify/refresh access token just on session touch (as it was initially implemented). Nobody knows when it will be used next time so looks strange to auto refresh it by default. At least we can introduce something like:

enum SessionRefreshStrategy {
    case manual, auto
}

with manual option by default.

@ypotsiah
Copy link

If we have infinite lifetime for the refresh token (according to docs) seems better to verify/refresh access token just on session touch (as it was initially implemented). Nobody knows when it will be used next time so looks strange to auto refresh it by default. At least we can introduce something like:

enum SessionRefreshStrategy {
    case manual, auto
}

with manual option by default.

My bad. I see it was added as configuration field. But I believe I should be false by default.

@@ -75,7 +77,7 @@ extension Session {
accessToken: "accesstoken",
tokenType: "bearer",
expiresIn: 60,
expiresAt: Date().addingTimeInterval(60).timeIntervalSince1970,
expiresAt: Date().addingTimeInterval(-60).timeIntervalSince1970,
Copy link

Choose a reason for hiding this comment

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

Nice catch

Copy link

@JOsacky JOsacky left a comment

Choose a reason for hiding this comment

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

Thanks for adding this change, this is exactly what we need

task = Task {
while !Task.isCancelled {
await autoRefreshTokenTick()
try? await Task.sleep(nanoseconds: UInt64(AutoRefreshToken.tickDuration) * NSEC_PER_SEC)
Copy link

Choose a reason for hiding this comment

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

I was curious what your thoughts were on using this Task.sleep to precisely schedule the time that the token should be refreshed instead of checking every 30 seconds. This would match the behavior of the kotlin library.

Here is an example of an implementation: f62567e

 func scheduleSessionRefresh(_ session: Session) throws {
    if autoRefreshTask != nil {
      return
    }

    autoRefreshTask = Task {
      defer { autoRefreshTask = nil }

      guard let expiresAt = session.expiresAt else {
        return
      }
      let expiryDate = Date(timeIntervalSince1970: expiresAt)

      let timeIntervalToExpiry = expiryDate.timeIntervalSinceNow

      // if negative then token is expired and will refresh right away
      let timeIntervalToRefresh = max(timeIntervalToExpiry * 0.8, 0)

      try await Task.sleep(nanoseconds: UInt64(timeIntervalToRefresh * 1_000_000_000))
      let session = try await sessionRefresher.refreshSession(session.refreshToken)
    }
  }

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @JOsacky the 30-second interval follows the implementation of the official JS and Flutter libraries, but in Swift, we could do it the way you suggested.

I'd like input from @hf @J0 or @kangmingtay on this.

By checking every 30 seconds, we can detect sessions logged out early, this is the only thing I can think of that would benefit from the current implementation.

Copy link

Choose a reason for hiding this comment

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

Just seeing this comment. Thank you for thinking of the tradeoffs between the two! It seems like either would be a good solution and I saw that you chose the scheduled approach. Thank you for all the thought you put into this, I'm really excited for the next version of the swift app, I think this will greatly reduce the number of expired sessions.

@grdsdev
Copy link
Collaborator Author

grdsdev commented May 21, 2024

Closed in favor of #395

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.

Auth session doesn't refresh when using database
4 participants