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.

Sign up for GitHub

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

fix: token refresh retry offline + recover on visible #278

Merged
merged 10 commits into from
May 6, 2022
Merged

fix: token refresh retry offline + recover on visible #278

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3b506d3
fix: localStorage sync
thorwebdev May 4, 2022
4284a0c
fix: recover on visibility change
thorwebdev May 4, 2022
c64d8a2
fix: retry when failed to fetch
thorwebdev May 4, 2022
e7822f9
chore: clearTimeout for retires
thorwebdev May 4, 2022
c8c53d7
fix: more robust network failure handling
thorwebdev May 5, 2022
0875ffa
chore: default expiry margin to 10s
thorwebdev May 5, 2022
de9df53
chore: move localstorage acess to helpers
thorwebdev May 5, 2022
2b24a23
fix: localstorage null
thorwebdev May 5, 2022
4d253b6
fix: error handling
thorwebdev May 5, 2022
9b65103
chore: exponential backoff
thorwebdev May 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 56 additions & 34 deletions src/GoTrueClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,21 @@
import GoTrueApi from './GoTrueApi'
import { isBrowser, getParameterByName, uuid } from './lib/helpers'
import { GOTRUE_URL, DEFAULT_HEADERS, STORAGE_KEY } from './lib/constants'
import {
isBrowser,
getParameterByName,
uuid,
setItemAsync,
removeItemAsync,
getItemSynchronously,
getItemAsync,
} from './lib/helpers'
import {
GOTRUE_URL,
DEFAULT_HEADERS,
STORAGE_KEY,
EXPIRY_MARGIN,
NETWORK_FAILURE,
NETWORK_FAILURE_RETRY_INTERVAL,
} from './lib/constants'
import { polyfillGlobalThis } from './lib/polyfills'
import { Fetch } from './lib/fetch'

Expand All @@ -16,6 +31,7 @@ import type {
UserCredentials,
VerifyOTPParams,
OpenIDConnectCredentials,
SupportedStorage,
} from './lib/types'

polyfillGlobalThis() // Make "globalThis" available
Expand All @@ -29,17 +45,6 @@ const DEFAULT_OPTIONS = {
headers: DEFAULT_HEADERS,
}

type AnyFunction = (...args: any[]) => any
type MaybePromisify<T> = T | Promise<T>

type PromisifyMethods<T> = {
[K in keyof T]: T[K] extends AnyFunction
? (...args: Parameters<T[K]>) => MaybePromisify<ReturnType<T[K]>>
: T[K]
}

type SupportedStorage = PromisifyMethods<Pick<Storage, 'getItem' | 'setItem' | 'removeItem'>>

export default class GoTrueClient {
/**
* Namespace for the GoTrue API methods.
Expand Down Expand Up @@ -101,6 +106,7 @@ export default class GoTrueClient {
this._recoverSession()
this._recoverAndRefresh()
this._listenForMultiTabEvents()
this._handleVisibilityChange()

if (settings.detectSessionInUrl && isBrowser() && !!getParameterByName('access_token')) {
// Handle the OAuth redirect
Expand Down Expand Up @@ -184,7 +190,7 @@ export default class GoTrueClient {
* @param password The user's password.
* @param refreshToken A valid refresh token that was returned on login.
* @param provider One of the providers supported by GoTrue.
* @param redirectTo A URL to send the user to after they are confirmed (OAuth logins only).
* @param redirectTo A URL to send the user to after they are confirmed (OAuth logins only).
* @param shouldCreateUser A boolean flag to indicate whether to automatically create a user on magiclink / otp sign-ins if the user doesn't exist. Defaults to true.
* @param scopes A space-separated list of scopes granted to the OAuth application.
*/
Expand Down Expand Up @@ -404,7 +410,7 @@ export default class GoTrueClient {
...this.currentSession,
access_token,
token_type: 'bearer',
user: this.user()
user: this.user(),
}

this._notifyAllSubscribers('TOKEN_REFRESHED')
Expand Down Expand Up @@ -610,16 +616,12 @@ export default class GoTrueClient {
*/
private _recoverSession() {
try {
const json = isBrowser() && this.localStorage?.getItem(STORAGE_KEY)
if (!json || typeof json !== 'string') {
return null
}

const data = JSON.parse(json)
const data = getItemSynchronously(this.localStorage, STORAGE_KEY)
if (!data) return null
const { currentSession, expiresAt } = data
const timeNow = Math.round(Date.now() / 1000)

if (expiresAt >= timeNow && currentSession?.user) {
if (expiresAt >= timeNow + EXPIRY_MARGIN && currentSession?.user) {
this._saveSession(currentSession)
this._notifyAllSubscribers('SIGNED_IN')
}
Expand All @@ -634,20 +636,24 @@ export default class GoTrueClient {
*/
private async _recoverAndRefresh() {
try {
const json = isBrowser() && (await this.localStorage.getItem(STORAGE_KEY))
if (!json) {
return null
}

const data = JSON.parse(json)
const data = await getItemAsync(this.localStorage, STORAGE_KEY)
if (!data) return null
const { currentSession, expiresAt } = data
const timeNow = Math.round(Date.now() / 1000)

if (expiresAt < timeNow) {
if (expiresAt < timeNow + EXPIRY_MARGIN) {
Copy link
Member

Choose a reason for hiding this comment

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

a counter and stop pinging gotrue or stop pinging after expiry time is reached cos gotrue is not going to refresh that token anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

Added exponential backoff, starting with 200ms and then exponentially backing off for 10 retries

if (this.autoRefreshToken && currentSession.refresh_token) {
const { error } = await this._callRefreshToken(currentSession.refresh_token)
if (error) {
console.log(error.message)
if (error.message === NETWORK_FAILURE) {
if (this.refreshTokenTimer) clearTimeout(this.refreshTokenTimer)
this.refreshTokenTimer = setTimeout(
() => this._recoverAndRefresh(),
NETWORK_FAILURE_RETRY_INTERVAL * 1000
)
return
}
await this._removeSession()
}
} else {
Expand Down Expand Up @@ -703,7 +709,7 @@ export default class GoTrueClient {
if (expiresAt) {
const timeNow = Math.round(Date.now() / 1000)
const expiresIn = expiresAt - timeNow
const refreshDurationBeforeExpires = expiresIn > 60 ? 60 : 0.5
const refreshDurationBeforeExpires = expiresIn > EXPIRY_MARGIN ? EXPIRY_MARGIN : 0.5
this._startAutoRefreshToken((expiresIn - refreshDurationBeforeExpires) * 1000)
}

Copy link

@GaryAustin1 GaryAustin1 May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Edit:
It is not clear to me why you can not just call for a new refresh token if within EXPIRY_MARGIN+1 (+1 to not have to deal with fixing rounding and margin issues discussed below) and either way set the timer to expiresIn- EXPIRY_MARGIN. I think other changes require a minimum token expire length > EXPIRY_MARGIN by at least a few seconds to be useful, so the reason for the .5 (which was token life of less than 60 seconds) goes away.
::

I have an open issue that addresses in the first part .5 seconds as not enough time to complete refresh.
#272

I have a trace showing failure to do preflight and refresh request within 500msec in that issue (even though who ever came up with it felt it was enough time)

So if it is possible for the time left to expiration is EXPIRY_MARGIN + .5 or 1 or less, the token could expire for a brief amount of time if code can run and use the token before this task is finished (which I have not traced thru entirely).

I also think the rounding error mentioned in that issue still exists here and by cutting it so close with the .5 as Date.now() could get rounded up adding up .5 seconds you don't have if on the margin.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've made the following decisions:

  1. We will default JWT expiry to 15mins in the future (once we've verified all the token refresh issues have been resolved)
  2. We will allow a minimum JWT expiry of 30s.
  3. We will default the expiry margin to 10s.
  4. In case of network failure we will retry every second.

So if you set JWT to the smallest possible value (30s), the client library will refresh it every 20s.

Expand All @@ -716,14 +722,14 @@ export default class GoTrueClient {

private _persistSession(currentSession: Session) {
const data = { currentSession, expiresAt: currentSession.expires_at }
isBrowser() && this.localStorage.setItem(STORAGE_KEY, JSON.stringify(data))
setItemAsync(this.localStorage, STORAGE_KEY, data)
}

private async _removeSession() {
this.currentSession = null
this.currentUser = null
if (this.refreshTokenTimer) clearTimeout(this.refreshTokenTimer)
isBrowser() && (await this.localStorage.removeItem(STORAGE_KEY))
removeItemAsync(this.localStorage, STORAGE_KEY)
}

/**
Expand All @@ -734,7 +740,11 @@ export default class GoTrueClient {
if (this.refreshTokenTimer) clearTimeout(this.refreshTokenTimer)
if (value <= 0 || !this.autoRefreshToken) return

this.refreshTokenTimer = setTimeout(() => this._callRefreshToken(), value)
this.refreshTokenTimer = setTimeout(async () => {
const { error } = await this._callRefreshToken()
if (error?.message === NETWORK_FAILURE)
this._startAutoRefreshToken(NETWORK_FAILURE_RETRY_INTERVAL * 1000)
}, value)
if (typeof this.refreshTokenTimer.unref === 'function') this.refreshTokenTimer.unref()
}

Expand All @@ -752,7 +762,7 @@ export default class GoTrueClient {
if (e.key === STORAGE_KEY) {
const newSession = JSON.parse(String(e.newValue))
if (newSession?.currentSession?.access_token) {
this._recoverAndRefresh()
this._saveSession(newSession.currentSession)
this._notifyAllSubscribers('SIGNED_IN')
} else {
this._removeSession()
Expand All @@ -764,4 +774,16 @@ export default class GoTrueClient {
console.error('_listenForMultiTabEvents', error)
}
}

private _handleVisibilityChange() {
try {
window?.addEventListener('visibilitychange', () => {
if (document.visibilityState === 'visible') {
this._recoverAndRefresh()
}
})
} catch (error) {
console.error('_handleVisibilityChange', error)
}
}
}
4 changes: 3 additions & 1 deletion src/lib/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ import { version } from './version'
export const GOTRUE_URL = 'http://localhost:9999'
export const AUDIENCE = ''
export const DEFAULT_HEADERS = { 'X-Client-Info': `gotrue-js/${version}` }
export const EXPIRY_MARGIN = 60 * 1000
export const EXPIRY_MARGIN = 10 // in seconds
export const NETWORK_FAILURE = 'NETWORK_FAILURE'
export const NETWORK_FAILURE_RETRY_INTERVAL = 1 // in seconds
export const STORAGE_KEY = 'supabase.auth.token'
export const COOKIE_OPTIONS = {
name: 'sb',
Expand Down
24 changes: 8 additions & 16 deletions src/lib/fetch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { NETWORK_FAILURE } from './constants'

export type Fetch = typeof fetch

export interface FetchOptions {
Expand All @@ -9,21 +11,6 @@ export interface FetchOptions {

export type RequestMethodType = 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE'

const _getErrorMessage = (err: any): string =>
err.msg || err.message || err.error_description || err.error || JSON.stringify(err)

const handleError = (error: any, reject: any) => {
if (typeof error.json !== 'function') {
return reject(error)
}
error.json().then((err: any) => {
return reject({
message: _getErrorMessage(err),
status: error?.status || 500,
})
})
}

const _getRequestParams = (method: RequestMethodType, options?: FetchOptions, body?: object) => {
const params: { [k: string]: any } = { method, headers: options?.headers || {} }

Expand Down Expand Up @@ -52,7 +39,12 @@ async function _handleRequest(
return result.json()
})
.then((data) => resolve(data))
.catch((error) => handleError(error, reject))
.catch(() =>
reject({
message: NETWORK_FAILURE,
Copy link
Member

Choose a reason for hiding this comment

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

Since we are throwing even if result.ok is false, lets have a generic message like "Request Failed"? Cos this will get triggered both for network errors and server errors like timeouts etc

status: null,
})
)
})
}

Expand Down
36 changes: 36 additions & 0 deletions src/lib/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import crossFetch from 'cross-fetch'
import { SupportedStorage } from './types'

export function expiresAt(expiresIn: number) {
const timeNow = Math.round(Date.now() / 1000)
Expand Down Expand Up @@ -39,3 +40,38 @@ export const resolveFetch = (customFetch?: Fetch): Fetch => {
}
return (...args) => _fetch(...args)
}

// LocalStorage helpers
export const setItemAsync = async (
storage: SupportedStorage,
key: string,
data: any
): Promise<void> => {
isBrowser() && (await storage?.setItem(key, JSON.stringify(data)))
}

export const getItemAsync = async (storage: SupportedStorage, key: string): Promise<any | null> => {
const value = isBrowser() && (await storage?.getItem(key))
if (!value) return null
try {
return JSON.parse(value)
} catch {
return value
}
}

export const getItemSynchronously = (storage: SupportedStorage, key: string): any | null => {
const value = isBrowser() && storage?.getItem(key)
if (!value || typeof value !== 'string') {
return null
}
try {
return JSON.parse(value)
} catch {
return value
}
}

export const removeItemAsync = async (storage: SupportedStorage, key: string): Promise<void> => {
isBrowser() && (await storage?.removeItem(key))
}
11 changes: 11 additions & 0 deletions src/lib/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,3 +209,14 @@ export interface OpenIDConnectCredentials {
client_id?: string
issuer?: string
}

type AnyFunction = (...args: any[]) => any
type MaybePromisify<T> = T | Promise<T>

type PromisifyMethods<T> = {
[K in keyof T]: T[K] extends AnyFunction
? (...args: Parameters<T[K]>) => MaybePromisify<ReturnType<T[K]>>
: T[K]
}

export type SupportedStorage = PromisifyMethods<Pick<Storage, 'getItem' | 'setItem' | 'removeItem'>>