mirror of
https://github.com/bulwarkmail/native.git
synced 2026-10-31 08:06:36 +00:00
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.
This commit is contained in:
@@ -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<typeof vi.fn>;
|
||||
|
||||
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');
|
||||
});
|
||||
});
|
||||
+21
-3
@@ -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<OAuthTokens> {
|
||||
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}`);
|
||||
|
||||
|
||||
+13
-2
@@ -107,11 +107,22 @@ function parseFragment(url: string): URLSearchParams {
|
||||
return new URLSearchParams(url.slice(hashIdx + 1));
|
||||
}
|
||||
|
||||
export async function runWebmailHandoff(webmailUrl: string): Promise<HandoffResult> {
|
||||
export async function runWebmailHandoff(
|
||||
webmailUrl: string,
|
||||
opts?: { addAccount?: boolean },
|
||||
): Promise<HandoffResult> {
|
||||
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();
|
||||
|
||||
@@ -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<typeof import('../../lib/oauth-native')>()),
|
||||
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<typeof vi.fn>).mockResolvedValue(metadata);
|
||||
(loginWithPkce as ReturnType<typeof vi.fn>).mockRejectedValue(new HandoffCancelledError());
|
||||
(probeWebmail as ReturnType<typeof vi.fn>).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,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -315,7 +315,7 @@ export const useAuthStore = create<AuthState>((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<AuthState>((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 });
|
||||
|
||||
Reference in New Issue
Block a user