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
Conversation
9dda668
to
17507dc
Compare
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 |
My bad. I see it was added as configuration field. But I believe I should be |
@@ -75,7 +77,7 @@ extension Session { | |||
accessToken: "accesstoken", | |||
tokenType: "bearer", | |||
expiresIn: 60, | |||
expiresAt: Date().addingTimeInterval(60).timeIntervalSince1970, | |||
expiresAt: Date().addingTimeInterval(-60).timeIntervalSince1970, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch
There was a problem hiding this 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) |
There was a problem hiding this comment.
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)
}
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Closed in favor of #395 |
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.