Skip to content

Commit

Permalink
fix: don't call removeSession prematurely (#915)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?
* Replaces #854 
* Fixes #853, #904 
* We don't need to remove the existing session prematurely. This causes
some issues when users want to implement some sort of switch-account
functionality since the existing session will always be removed
regardless of whether the signup / sign-in attempt succeeds.
* It's safe to remove `_removeSession` since calling `_saveSession`
multiple times will just replace the existing session
  • Loading branch information
kangmingtay committed Jun 7, 2024
1 parent 5bda912 commit e0dc518
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 22 deletions.
22 changes: 0 additions & 22 deletions src/GoTrueClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -369,8 +369,6 @@ export default class GoTrueClient {
*/
async signInAnonymously(credentials?: SignInAnonymouslyCredentials): Promise<AuthResponse> {
try {
await this._removeSession()

const res = await _request(this.fetch, 'POST', `${this.url}/signup`, {
headers: this.headers,
body: {
Expand Down Expand Up @@ -414,8 +412,6 @@ export default class GoTrueClient {
*/
async signUp(credentials: SignUpWithPasswordCredentials): Promise<AuthResponse> {
try {
await this._removeSession()

let res: AuthResponse
if ('email' in credentials) {
const { email, password, options } = credentials
Expand Down Expand Up @@ -495,8 +491,6 @@ export default class GoTrueClient {
credentials: SignInWithPasswordCredentials
): Promise<AuthTokenResponsePassword> {
try {
await this._removeSession()

let res: AuthResponsePassword
if ('email' in credentials) {
const { email, password, options } = credentials
Expand Down Expand Up @@ -557,8 +551,6 @@ export default class GoTrueClient {
* This method supports the PKCE flow.
*/
async signInWithOAuth(credentials: SignInWithOAuthCredentials): Promise<OAuthResponse> {
await this._removeSession()

return await this._handleProviderSignIn(credentials.provider, {
redirectTo: credentials.options?.redirectTo,
scopes: credentials.options?.scopes,
Expand Down Expand Up @@ -621,8 +613,6 @@ export default class GoTrueClient {
* should be enabled and configured.
*/
async signInWithIdToken(credentials: SignInWithIdTokenCredentials): Promise<AuthTokenResponse> {
await this._removeSession()

try {
const { options, provider, token, access_token, nonce } = credentials

Expand Down Expand Up @@ -679,8 +669,6 @@ export default class GoTrueClient {
*/
async signInWithOtp(credentials: SignInWithPasswordlessCredentials): Promise<AuthOtpResponse> {
try {
await this._removeSession()

if ('email' in credentials) {
const { email, options } = credentials
let codeChallenge: string | null = null
Expand Down Expand Up @@ -734,11 +722,6 @@ export default class GoTrueClient {
*/
async verifyOtp(params: VerifyOtpParams): Promise<AuthResponse> {
try {
if (params.type !== 'email_change' && params.type !== 'phone_change') {
// we don't want to remove the authenticated session if the user is performing an email_change or phone_change verification
await this._removeSession()
}

let redirectTo = undefined
let captchaToken = undefined
if ('options' in params) {
Expand Down Expand Up @@ -800,7 +783,6 @@ export default class GoTrueClient {
*/
async signInWithSSO(params: SignInWithSSO): Promise<SSOResponse> {
try {
await this._removeSession()
let codeChallenge: string | null = null
let codeChallengeMethod: string | null = null
if (this.flowType === 'pkce') {
Expand Down Expand Up @@ -874,10 +856,6 @@ export default class GoTrueClient {
*/
async resend(credentials: ResendParams): Promise<AuthOtpResponse> {
try {
if (credentials.type != 'email_change' && credentials.type != 'phone_change') {
await this._removeSession()
}

const endpoint = `${this.url}/resend`
if ('email' in credentials) {
const { email, type, options } = credentials
Expand Down
49 changes: 49 additions & 0 deletions test/GoTrueClient.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON,
} from './lib/clients'
import { mockUserCredentials } from './lib/utils'
import { Session } from '../src'

describe('GoTrueClient', () => {
// @ts-expect-error 'Allow access to private _refreshAccessToken'
Expand Down Expand Up @@ -1016,4 +1017,52 @@ describe('GoTrueClient with storageisServer = true', () => {
expect(sessionUser).not.toBeNull()
expect(warnings.length).toEqual(0)
})

test('saveSession should overwrite the existing session', async () => {
const store = memoryLocalStorageAdapter()
const client = new GoTrueClient({
url: GOTRUE_URL_SIGNUP_ENABLED_AUTO_CONFIRM_ON,
storageKey: 'test-storage-key',
autoRefreshToken: false,
persistSession: true,
storage: {
...store,
},
})
const initialSession: Session = {
access_token: 'test-access-token',
refresh_token: 'test-refresh-token',
expires_in: 3600,
token_type: 'bearer',
user: {
id: 'test-user-id',
aud: 'test-audience',
user_metadata: {},
app_metadata: {},
created_at: new Date().toISOString(),
},
}

// @ts-ignore 'Allow access to private _saveSession'
await client._saveSession(initialSession)
expect(store.getItem('test-storage-key')).toEqual(JSON.stringify(initialSession))

const newSession: Session = {
access_token: 'test-new-access-token',
refresh_token: 'test-new-refresh-token',
expires_in: 3600,
token_type: 'bearer',
user: {
id: 'test-user-id',
aud: 'test-audience',
user_metadata: {},
app_metadata: {},
created_at: new Date().toISOString(),
},
}

// @ts-ignore 'Allow access to private _saveSession'
await client._saveSession(newSession)
expect(store.getItem('test-storage-key')).toEqual(JSON.stringify(newSession))
})
})

0 comments on commit e0dc518

Please sign in to comment.