From ab845783ab074dbc41a81a43328ca4a5749fecbf Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 18:35:05 +0200 Subject: [PATCH] fix: stop adding an OAuth account from signing in the current user again Adding a second account through the server's own OAuth opened the authorization endpoint in the shared system browser with no `prompt`, so the identity provider found the session of the account that was already signed in and quietly handed back the same user. When adding an account, the PKCE flow now sends `prompt=select_account` like the webmail does (5771a213), or `prompt=login` when the server's metadata lists no account picker, and on iOS it runs the auth session ephemerally so the browser's cookies don't carry over. The webmail hand-off gets the ephemeral session too. Android's Custom Tabs have no private mode, so there the prompt is the only guard. --- src/lib/__tests__/oauth-native.test.ts | 63 +++++++++++++++ src/lib/oauth-native.ts | 24 +++++- src/lib/oauth.ts | 15 +++- .../__tests__/auth-store-add-account.test.ts | 76 +++++++++++++++++++ src/stores/auth-store.ts | 4 +- 5 files changed, 175 insertions(+), 7 deletions(-) create mode 100644 src/lib/__tests__/oauth-native.test.ts create mode 100644 src/stores/__tests__/auth-store-add-account.test.ts diff --git a/src/lib/__tests__/oauth-native.test.ts b/src/lib/__tests__/oauth-native.test.ts new file mode 100644 index 00000000..3f0b5bde --- /dev/null +++ b/src/lib/__tests__/oauth-native.test.ts @@ -0,0 +1,63 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import * as WebBrowser from 'expo-web-browser'; + +vi.mock('../client-cert', () => ({ + secureFetch: vi.fn(), +})); + +import { loginWithPkce, type OAuthMetadata } from '../oauth-native'; +import { HandoffCancelledError } from '../oauth'; + +const mockOpenAuthSession = WebBrowser.openAuthSessionAsync as ReturnType; + +const metadata: OAuthMetadata = { + authorization_endpoint: 'https://mail.example.com/authorize/code', + token_endpoint: 'https://mail.example.com/auth/token', +}; + +// The browser mock answers every session with `cancel`, so the flow stops +// right after opening the authorization URL - which is all these tests need. +async function openedSession(meta: OAuthMetadata, addAccount?: boolean) { + await expect(loginWithPkce('https://mail.example.com', meta, { addAccount })).rejects.toBeInstanceOf( + HandoffCancelledError, + ); + expect(mockOpenAuthSession).toHaveBeenCalledTimes(1); + const [authUrl, redirectUri, options] = mockOpenAuthSession.mock.calls[0]; + return { params: new URL(authUrl as string).searchParams, redirectUri, options }; +} + +describe('loginWithPkce', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('reuses the browser session and sends no prompt for the first sign-in', async () => { + const { params, redirectUri, options } = await openedSession(metadata); + + expect(params.get('prompt')).toBeNull(); + expect(params.get('code_challenge_method')).toBe('S256'); + expect(redirectUri).toBe('bulwarkmobile://auth/callback'); + expect(options).toBeUndefined(); + }); + + it('asks for the account picker in a private session when adding an account', async () => { + const { params, options } = await openedSession(metadata, true); + + expect(params.get('prompt')).toBe('select_account'); + expect(options).toEqual({ preferEphemeralSession: true }); + }); + + it('asks for a fresh login when the server advertises no account picker', async () => { + const keycloak = { ...metadata, prompt_values_supported: ['none', 'login', 'consent'] }; + const { params } = await openedSession(keycloak, true); + + expect(params.get('prompt')).toBe('login'); + }); + + it('keeps the account picker when the server advertises it', async () => { + const picker = { ...metadata, prompt_values_supported: ['none', 'login', 'select_account'] }; + const { params } = await openedSession(picker, true); + + expect(params.get('prompt')).toBe('select_account'); + }); +}); diff --git a/src/lib/oauth-native.ts b/src/lib/oauth-native.ts index 707b5bd4..7f7d09de 100644 --- a/src/lib/oauth-native.ts +++ b/src/lib/oauth-native.ts @@ -22,6 +22,7 @@ export interface OAuthMetadata { token_endpoint: string; revocation_endpoint?: string; scopes_supported?: string[]; + prompt_values_supported?: string[]; } const DISCOVERY_TIMEOUT_MS = 8_000; @@ -108,14 +109,26 @@ function parseCallbackParams(url: string): URLSearchParams { return params; } +// Adding an account must not quietly reuse the identity provider's session +// of the account that is already signed in (webmail 5771a213). Ask for the +// account picker, or for a fresh login where the server says it has no +// picker (Keycloak advertises only none/login/consent). +function addAccountPrompt(metadata: OAuthMetadata): string { + const supported = metadata.prompt_values_supported; + if (supported?.length && !supported.includes('select_account') && supported.includes('login')) return 'login'; + return 'select_account'; +} + /** * Run the authorization-code + PKCE flow in the system browser. Throws - * `HandoffCancelledError` when the user closes the browser. + * `HandoffCancelledError` when the user closes the browser. With `addAccount` + * the provider is asked to pick or sign in another account, and iOS runs the + * session without the browser's cookies so the current one can't carry over. */ export async function loginWithPkce( serverUrl: string, metadata: OAuthMetadata, - opts?: { clientId?: string; scopes?: string; redirectUri?: string }, + opts?: { clientId?: string; scopes?: string; redirectUri?: string; addAccount?: boolean }, ): Promise { const clientId = opts?.clientId ?? DEFAULT_CLIENT_ID; const redirectUri = opts?.redirectUri ?? HANDOFF_REDIRECT_URI; @@ -136,9 +149,14 @@ export async function loginWithPkce( code_challenge: challenge, code_challenge_method: 'S256', }); + if (opts?.addAccount) params.set('prompt', addAccountPrompt(metadata)); const authUrl = `${metadata.authorization_endpoint}${metadata.authorization_endpoint.includes('?') ? '&' : '?'}${params.toString()}`; - const result = await WebBrowser.openAuthSessionAsync(authUrl, redirectUri); + const result = await WebBrowser.openAuthSessionAsync( + authUrl, + redirectUri, + opts?.addAccount ? { preferEphemeralSession: true } : undefined, + ); if (result.type === 'cancel' || result.type === 'dismiss') throw new HandoffCancelledError(); if (result.type !== 'success' || !result.url) throw new HandoffError(`Sign-in failed: ${result.type}`); diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index 31ba28d2..ccf0c92e 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -107,11 +107,22 @@ function parseFragment(url: string): URLSearchParams { return new URLSearchParams(url.slice(hashIdx + 1)); } -export async function runWebmailHandoff(webmailUrl: string): Promise { +export async function runWebmailHandoff( + webmailUrl: string, + opts?: { addAccount?: boolean }, +): Promise { const state = randomState(); const handoffUrl = buildHandoffUrl(webmailUrl, state); - const result = await WebBrowser.openAuthSessionAsync(handoffUrl, HANDOFF_REDIRECT_URI); + // Adding an account: on iOS, keep the browser's webmail and identity + // provider cookies out of the session so the signed-in account can't + // silently come back as the "new" one. Android's Custom Tabs have no such + // mode. + const result = await WebBrowser.openAuthSessionAsync( + handoffUrl, + HANDOFF_REDIRECT_URI, + opts?.addAccount ? { preferEphemeralSession: true } : undefined, + ); if (result.type === 'cancel' || result.type === 'dismiss') { throw new HandoffCancelledError(); diff --git a/src/stores/__tests__/auth-store-add-account.test.ts b/src/stores/__tests__/auth-store-add-account.test.ts new file mode 100644 index 00000000..554493e9 --- /dev/null +++ b/src/stores/__tests__/auth-store-add-account.test.ts @@ -0,0 +1,76 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import * as WebBrowser from 'expo-web-browser'; + +vi.mock('../../api/jmap-client', () => ({ + jmapClient: { + snapshot: vi.fn(() => ({ session: null, credentials: null, accountId: null })), + restoreSnapshot: vi.fn(), + onAuthFailure: vi.fn(() => () => undefined), + onTokenRefresh: vi.fn(() => () => undefined), + }, + AuthenticationError: class AuthenticationError extends Error {}, + NetworkError: class NetworkError extends Error {}, +})); + +vi.mock('../../lib/push-notifications', () => ({ + teardownPushNotifications: vi.fn(async () => undefined), + teardownPushNotificationsForAccount: vi.fn(async () => undefined), +})); + +vi.mock('../../lib/oauth-native', async (importOriginal) => ({ + ...(await importOriginal()), + discoverOAuthMetadata: vi.fn(), + loginWithPkce: vi.fn(), + probeWebmail: vi.fn(), +})); + +import { discoverOAuthMetadata, loginWithPkce, probeWebmail } from '../../lib/oauth-native'; +import { HandoffCancelledError } from '../../lib/oauth'; +import { useAuthStore } from '../auth-store'; + +const metadata = { + authorization_endpoint: 'https://mail.example.com/authorize/code', + token_endpoint: 'https://mail.example.com/auth/token', +}; + +beforeEach(() => { + vi.clearAllMocks(); + useAuthStore.setState({ isAuthenticated: true, isLoading: false, error: null }); + (discoverOAuthMetadata as ReturnType).mockResolvedValue(metadata); + (loginWithPkce as ReturnType).mockRejectedValue(new HandoffCancelledError()); + (probeWebmail as ReturnType).mockResolvedValue(true); +}); + +describe('adding another OAuth account', () => { + it('tells the PKCE flow it is adding an account', async () => { + await useAuthStore.getState().loginViaOAuth('https://mail.example.com/', { addAccount: true }); + + expect(loginWithPkce).toHaveBeenCalledWith('https://mail.example.com', metadata, { addAccount: true }); + }); + + it('does not ask for another account on a first sign-in', async () => { + await useAuthStore.getState().loginViaOAuth('https://mail.example.com'); + + expect(loginWithPkce).toHaveBeenCalledWith('https://mail.example.com', metadata, { addAccount: undefined }); + }); + + it('opens the webmail hand-off in a private browser session', async () => { + await useAuthStore.getState().loginViaWebmail('https://mail.example.com', { addAccount: true }); + + expect(WebBrowser.openAuthSessionAsync).toHaveBeenCalledWith( + expect.stringContaining('https://mail.example.com/login?'), + 'bulwarkmobile://auth/callback', + { preferEphemeralSession: true }, + ); + }); + + it('keeps the shared browser session for a first webmail sign-in', async () => { + await useAuthStore.getState().loginViaWebmail('https://mail.example.com'); + + expect(WebBrowser.openAuthSessionAsync).toHaveBeenCalledWith( + expect.stringContaining('https://mail.example.com/login?'), + 'bulwarkmobile://auth/callback', + undefined, + ); + }); +}); diff --git a/src/stores/auth-store.ts b/src/stores/auth-store.ts index b9753590..20adc411 100644 --- a/src/stores/auth-store.ts +++ b/src/stores/auth-store.ts @@ -315,7 +315,7 @@ export const useAuthStore = create((set, get) => ({ } let result; try { - result = await runWebmailHandoff(webmailUrl); + result = await runWebmailHandoff(webmailUrl, { addAccount: opts?.addAccount }); } catch (err) { if (err instanceof HandoffCancelledError) { // User closed the browser tab — quiet exit, no error banner. @@ -359,7 +359,7 @@ export const useAuthStore = create((set, get) => ({ try { const metadata = await discoverOAuthMetadata(base); if (!metadata) throw new HandoffError('This server does not offer OAuth sign-in'); - tokens = await loginWithPkce(base, metadata); + tokens = await loginWithPkce(base, metadata, { addAccount: opts?.addAccount }); } catch (err) { if (err instanceof HandoffCancelledError) { set({ isLoading: false, error: null });