Skip to content

Conversation

@grdsdev
Copy link
Contributor

@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
@grdsdev grdsdev force-pushed the feat/auto-refresh-token branch from 9dda668 to 17507dc Compare April 23, 2024 18:53
/// 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
Contributor 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
@grdsdev grdsdev requested review from J0, dshukertjr, hf and kangmingtay 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.

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
Contributor 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
Contributor 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

5 participants