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

Jump to bottom

[fix] Keep existing session on auth functions if they fail #854

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

[fix] Keep existing session on auth functions if they fail #854

Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
afb0ae8
fix: keep existing session on auth functions if they fail
bombillazo Feb 16, 2024
09df436
Merge branch 'supabase:master' into master
bombillazo Apr 2, 2024
6f2b4d6
Merge branch 'supabase:master' into master
bombillazo Apr 19, 2024
1d5dace
Merge branch 'supabase:master' into master
bombillazo Apr 30, 2024
75b5877
Merge branch 'supabase:master' into master
bombillazo May 8, 2024
b6a0847
Merge branch 'supabase:master' into master
bombillazo May 9, 2024
88cedd7
chore: update function description comment
bombillazo May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 81 additions & 11 deletions src/GoTrueClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,9 @@ export default class GoTrueClient {
* @returns A user if the server has "autoconfirm" OFF
*/
async signUp(credentials: SignUpWithPasswordCredentials): Promise<AuthResponse> {
const currentSession = await this._useSession(async (result) => {
return result.data.session
})
try {
await this._removeSession()

Expand Down Expand Up @@ -471,14 +474,18 @@ export default class GoTrueClient {
if (data.session) {
await this._saveSession(data.session)
await this._notifyAllSubscribers('SIGNED_IN', session)
} else if (currentSession) {
await this._saveSession(currentSession)
}

return { data: { user, session }, error: null }
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
Comment on lines +483 to +485
Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure why it's necessary to preserve the existing session when one calls the signup method - we always want to remove the session when signup is called to prevent any possibilities of the user logging in as someone else.

Copy link
Member

Choose a reason for hiding this comment

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

this applies to all the other sign-in methods too - if you're signing in, there won't be any session in the first place, else you'd be logged in already

Copy link
Author

@bombillazo bombillazo May 9, 2024 •
edited

Choose a reason for hiding this comment

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

This truly depends on the UI implementation. We may need to allow logged-in users to switch accounts directly so it is possible to call sign-in/sign-up. Currently, if something fails, the user is kicked out of their current session.

In addition, I don't see how this would log a user into someone else's account. Either the sign-in is successful, and the current session switches to the new one, or it fails, and you keep the existing one. If you were never logged in, you stay logged out.

Copy link
Contributor

@j4w8n j4w8n May 10, 2024 •
edited

Choose a reason for hiding this comment

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

If the team wanted to go down this path, you could simplify a lot of these methods by moving the _removeSession call to right before the _saveSession calls. There would be a few exceptions to that though.

I've seen more and more discussions about people trying to implement a "switch account" feature; or they already have, but recent changes have broken their implementations.

Copy link
Author

Choose a reason for hiding this comment

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

I agree. I just wasn't sure if some logic was dependent on having no session while executing, so I preferred keeping it and reverting at the end.

Hopefully, we can remove the current limitation, be it by removing _removeSession where not necessary or, as my PR suggests, restoring the session on fail.

This is already working for us in production, we patch this library in our app for this behavior, but we'd prefer if the lib had it working natively.

if (isAuthError(error)) {
return { data: { user: null, session: null }, error }
}

throw error
}
}
Expand All @@ -494,6 +501,9 @@ export default class GoTrueClient {
async signInWithPassword(
credentials: SignInWithPasswordCredentials
): Promise<AuthTokenResponsePassword> {
const currentSession = await this._useSession(async (result) => {
return result.data.session
})
try {
await this._removeSession()

Expand Down Expand Up @@ -535,7 +545,10 @@ export default class GoTrueClient {
if (data.session) {
await this._saveSession(data.session)
await this._notifyAllSubscribers('SIGNED_IN', data.session)
} else if (currentSession) {
await this._saveSession(currentSession)
}

return {
data: {
user: data.user,
Expand All @@ -545,6 +558,9 @@ export default class GoTrueClient {
error,
}
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
if (isAuthError(error)) {
return { data: { user: null, session: null }, error }
}
Expand All @@ -557,14 +573,24 @@ 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,
queryParams: credentials.options?.queryParams,
skipBrowserRedirect: credentials.options?.skipBrowserRedirect,
const currentSession = await this._useSession(async (result) => {
return result.data.session
})
try {
await this._removeSession()

return await this._handleProviderSignIn(credentials.provider, {
redirectTo: credentials.options?.redirectTo,
scopes: credentials.options?.scopes,
queryParams: credentials.options?.queryParams,
skipBrowserRedirect: credentials.options?.skipBrowserRedirect,
})
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
throw error
}
}

/**
Expand Down Expand Up @@ -621,9 +647,11 @@ export default class GoTrueClient {
* should be enabled and configured.
*/
async signInWithIdToken(credentials: SignInWithIdTokenCredentials): Promise<AuthTokenResponse> {
await this._removeSession()

const currentSession = await this._useSession(async (result) => {
return result.data.session
})
try {
await this._removeSession()
const { options, provider, token, access_token, nonce } = credentials

const res = await _request(this.fetch, 'POST', `${this.url}/token?grant_type=id_token`, {
Expand All @@ -650,9 +678,15 @@ export default class GoTrueClient {
if (data.session) {
await this._saveSession(data.session)
await this._notifyAllSubscribers('SIGNED_IN', data.session)
} else if (currentSession) {
await this._saveSession(currentSession)
}

return { data, error }
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
if (isAuthError(error)) {
return { data: { user: null, session: null }, error }
}
Expand All @@ -678,6 +712,9 @@ export default class GoTrueClient {
* This method supports PKCE when an email is passed.
*/
async signInWithOtp(credentials: SignInWithPasswordlessCredentials): Promise<AuthOtpResponse> {
const currentSession = await this._useSession(async (result) => {
return result.data.session
})
try {
await this._removeSession()

Expand Down Expand Up @@ -721,6 +758,9 @@ export default class GoTrueClient {
}
throw new AuthInvalidCredentialsError('You must provide either an email or phone number.')
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
if (isAuthError(error)) {
return { data: { user: null, session: null }, error }
}
Expand All @@ -733,6 +773,9 @@ export default class GoTrueClient {
* Log in a user given a User supplied OTP or TokenHash received through mobile or email.
*/
async verifyOtp(params: VerifyOtpParams): Promise<AuthResponse> {
const currentSession = await this._useSession(async (result) => {
return result.data.session
})
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
Expand Down Expand Up @@ -772,10 +815,15 @@ export default class GoTrueClient {
params.type == 'recovery' ? 'PASSWORD_RECOVERY' : 'SIGNED_IN',
session
)
} else if (currentSession) {
await this._saveSession(currentSession)
}

return { data: { user, session }, error: null }
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
if (isAuthError(error)) {
return { data: { user: null, session: null }, error }
}
Expand All @@ -799,6 +847,9 @@ export default class GoTrueClient {
* organization's SSO Identity Provider UUID directly instead.
*/
async signInWithSSO(params: SignInWithSSO): Promise<SSOResponse> {
const currentSession = await this._useSession(async (result) => {
return result.data.session
})
try {
await this._removeSession()
let codeChallenge: string | null = null
Expand Down Expand Up @@ -826,6 +877,9 @@ export default class GoTrueClient {
xform: _ssoResponse,
})
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
if (isAuthError(error)) {
return { data: null, error }
}
Expand Down Expand Up @@ -873,6 +927,9 @@ export default class GoTrueClient {
* Resends an existing signup confirmation email, email change email, SMS OTP or phone change OTP.
*/
async resend(credentials: ResendParams): Promise<AuthOtpResponse> {
const currentSession = await this._useSession(async (result) => {
return result.data.session
})
try {
if (credentials.type != 'email_change' && credentials.type != 'phone_change') {
await this._removeSession()
Expand All @@ -890,6 +947,9 @@ export default class GoTrueClient {
},
redirectTo: options?.emailRedirectTo,
})
if (currentSession) {
await this._saveSession(currentSession)
}
return { data: { user: null, session: null }, error }
} else if ('phone' in credentials) {
const { phone, type, options } = credentials
Expand All @@ -901,12 +961,18 @@ export default class GoTrueClient {
gotrue_meta_security: { captcha_token: options?.captchaToken },
},
})
if (currentSession) {
await this._saveSession(currentSession)
}
return { data: { user: null, session: null, messageId: data?.message_id }, error }
}
throw new AuthInvalidCredentialsError(
'You must provide either an email or phone number and a type'
)
} catch (error) {
if (currentSession) {
await this._saveSession(currentSession)
}
if (isAuthError(error)) {
return { data: { user: null, session: null }, error }
}
Expand Down Expand Up @@ -1876,7 +1942,7 @@ export default class GoTrueClient {
}

/**
* Recovers the session from LocalStorage and refreshes
* Recovers the session from LocalStorage and refreshes the token
* Note: this method is async to accommodate for AsyncStorage e.g. in React native.
*/
private async _recoverAndRefresh() {
Expand All @@ -1891,6 +1957,7 @@ export default class GoTrueClient {
this._debug(debugName, 'session is not valid')
if (currentSession !== null) {
await this._removeSession()
await this._notifyAllSubscribers('SIGNED_OUT', null)
}

return
Expand Down Expand Up @@ -1918,6 +1985,7 @@ export default class GoTrueClient {
error
)
await this._removeSession()
await this._notifyAllSubscribers('SIGNED_OUT', null)
}
}
}
Expand Down Expand Up @@ -2085,10 +2153,12 @@ export default class GoTrueClient {
// finished and tests run endlessly. This can be prevented by calling
// `unref()` on the returned object.
ticker.unref()
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
} else if (typeof Deno !== 'undefined' && typeof Deno.unrefTimer === 'function') {
// similar like for NodeJS, but with the Deno API
// https://deno.land/api@latest?unstable&s=Deno.unrefTimer
// eslint-disable-next-line @typescript-eslint/ban-ts-comment
// @ts-ignore
Deno.unrefTimer(ticker)
}
Expand Down