From db717dcd51ded4f78304414fc6b3467d9ea60499 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:08:27 +0100 Subject: [PATCH 1/7] fix: stop the app from crashing when switching to an account left in Trash --- .../use-deleted-items-recovery.test.ts | 47 +++++++++++++++++++ .../recovery/use-deleted-items-recovery.ts | 4 +- 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 src/lib/recovery/__tests__/use-deleted-items-recovery.test.ts diff --git a/src/lib/recovery/__tests__/use-deleted-items-recovery.test.ts b/src/lib/recovery/__tests__/use-deleted-items-recovery.test.ts new file mode 100644 index 00000000..52ccc0c1 --- /dev/null +++ b/src/lib/recovery/__tests__/use-deleted-items-recovery.test.ts @@ -0,0 +1,47 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const client = vi.hoisted(() => ({ id: null as string | null })); +vi.mock('../../../api/jmap-client', () => ({ + jmapClient: { + get accountId(): string { + if (!client.id) throw new Error('Not authenticated - call connect() first'); + return client.id; + }, + get connectedAccountId(): string | null { + return client.id; + }, + }, +})); +vi.mock('../../../api/archived-items', () => ({ + advertisesArchivedItems: vi.fn(() => true), + queryArchivedItemIds: vi.fn(async () => ({ ids: [], total: 0 })), + hasArchivedEmail: vi.fn(async () => false), +})); +const auth = vi.hoisted(() => ({ state: { session: {} as unknown, activeAccountId: 'a@host' } })); +vi.mock('../../../stores/auth-store', () => ({ + useAuthStore: (selector: (s: typeof auth.state) => unknown) => selector(auth.state), +})); + +import { renderHook } from '../../../components/email/structured-mail/__tests__/render-native'; +import { useDeletedItemsRecovery } from '../use-deleted-items-recovery'; + +afterEach(() => { + client.id = null; +}); + +describe('useDeletedItemsRecovery', () => { + it('renders without a connected client while an account switch reconnects', () => { + // The auth store still holds a session (the account being left, or the + // target one restored), but the client has no account id yet. + client.id = null; + const hook = renderHook(() => useDeletedItemsRecovery()); + expect(hook.current).toEqual({ available: false, accountId: null, key: null }); + }); + + it('names the connected account once the client is in', () => { + client.id = 'u1'; + const hook = renderHook(() => useDeletedItemsRecovery()); + expect(hook.current.accountId).toBe('u1'); + expect(hook.current.key).not.toBeNull(); + }); +}); diff --git a/src/lib/recovery/use-deleted-items-recovery.ts b/src/lib/recovery/use-deleted-items-recovery.ts index 4afd5a2d..6bee5c0e 100644 --- a/src/lib/recovery/use-deleted-items-recovery.ts +++ b/src/lib/recovery/use-deleted-items-recovery.ts @@ -38,7 +38,9 @@ export interface DeletedItemsRecovery { export function useDeletedItemsRecovery(): DeletedItemsRecovery { const session = useAuthStore((s) => s.session); const activeAccountId = useAuthStore((s) => s.activeAccountId); - const accountId = session ? jmapClient.accountId || null : null; + // connectedAccountId, not accountId: the client is between sessions while an + // account switch reconnects, and accountId throws then (a crash in render). + const accountId = session ? jmapClient.connectedAccountId : null; const off = recoveryPolicy().hidden; const key = accountId ? recoveryKey(activeAccountId, accountId) : null; const check = useRecoveryStore((s) => (key ? s.checks[key] : undefined)); From 688e8bef3b98ed9d96cb2687ce5fdd7fc166be0b Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:21:01 +0100 Subject: [PATCH 2/7] fix: keep two accounts on the same host apart by port --- App.tsx | 7 +- locales/rn/en.json | 5 +- src/components/SidebarDrawer.tsx | 13 +-- src/components/settings/AccountSettings.tsx | 4 +- src/lib/__tests__/account-id-port.test.ts | 95 ++++++++++++++++++++ src/lib/__tests__/push-notifications.test.ts | 40 +++++++++ src/lib/account-utils.ts | 70 ++++++++++++++- src/lib/push-background-task.ts | 5 ++ src/lib/push-notifications.ts | 43 +++++++-- src/stores/account-store.ts | 10 ++- src/stores/auth-store.ts | 16 ++++ 11 files changed, 287 insertions(+), 21 deletions(-) create mode 100644 src/lib/__tests__/account-id-port.test.ts diff --git a/App.tsx b/App.tsx index 4f459a38..e0729c85 100644 --- a/App.tsx +++ b/App.tsx @@ -654,7 +654,10 @@ export default function App() { ); const activeAccountId = useAuthStore((s) => s.activeAccountId); React.useEffect(() => { - if (!isAuthenticated || !client) return; + // The setup asks the server through the client: it waits for the + // session, and runs again once one comes up (an account added or + // switched to, a cold start that began offline). + if (!isAuthenticated || !client || !haveLiveSession) return; let cancelled = false; const doSetup = async () => { @@ -700,7 +703,7 @@ export default function App() { unsubscribe(); unsubscribeUp(); }; - }, [client, isAuthenticated, emailNotificationsEnabled, activeAccountId]); + }, [client, isAuthenticated, emailNotificationsEnabled, activeAccountId, haveLiveSession]); // Muting a folder or account changes the server-side push filter (#78), // which the setup above would only send at the next launch. Send it now, diff --git a/locales/rn/en.json b/locales/rn/en.json index fb90bce8..b89ab690 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -560,10 +560,10 @@ "export_failed": "Export failed", "forwarded": "Forwarded", "load_failed": "Failed to load email", - "next": "Next", - "not_spam_short": "Not spam", "new_tag": "New tag", "new_tag_placeholder": "Tag name", + "next": "Next", + "not_spam_short": "Not spam", "pause_animation": "Pause animation", "play_animation": "Play animation", "preview": { @@ -1341,6 +1341,7 @@ "accounts_desc_mobile": "Background notifications of each signed-in account on this device. Switch to an account to turn them on or off for it.", "confirm_disable_message_mobile": "This device will stop receiving new-mail alerts for this account.", "description_mobile": "Receive system notifications for new mail when the app is closed. Delivered via the Bulwark push relay; the relay never sees mail content.", + "err_account_changed": "The account changed while push was being set up. Try again.", "err_android_only": "Push notifications are only available on Android.", "err_apns_empty": "iOS returned an empty push token.", "err_apns_token": "iOS could not issue a push token ({detail}).", diff --git a/src/components/SidebarDrawer.tsx b/src/components/SidebarDrawer.tsx index 66dea0bd..937b4d95 100644 --- a/src/components/SidebarDrawer.tsx +++ b/src/components/SidebarDrawer.tsx @@ -21,7 +21,7 @@ import { useAccountStore } from '../stores/account-store'; import { useSettingsStore } from '../stores/settings-store'; import { useKeywordsStore, keywordToken } from '../stores/keywords-store'; import { useLocaleStore } from '../stores/locale-store'; -import { MAX_ACCOUNTS, accountRowText } from '../lib/account-utils'; +import { MAX_ACCOUNTS, accountRowText, serverHostLabel } from '../lib/account-utils'; import { toUnicodeEmail } from '../lib/idn'; import { buildMailboxTree, flattenVisible, mailboxSubtreeIds, ownMailboxes, type MailboxNode, @@ -788,6 +788,9 @@ export default function SidebarDrawer({ visible, onClose, docked = false }: Side if (!serverUrl) return ''; try { return new URL(serverUrl).hostname; } catch { return serverUrl; } }, [serverUrl]); + // The header names the server with its port when that isn't the default: + // two servers on one host are two accounts with the same address. + const hostLabel = React.useMemo(() => (serverUrl ? serverHostLabel(serverUrl) : ''), [serverUrl]); const accountEmailFull = React.useMemo(() => { if (!username) return hostname; if (username.includes('@')) return username; @@ -836,8 +839,8 @@ export default function SidebarDrawer({ visible, onClose, docked = false }: Side {username || t('settings.account.title', 'Account')} - {hostname ? ( - {hostname} + {hostLabel ? ( + {hostLabel} ) : null} {busy ? ( @@ -857,8 +860,8 @@ export default function SidebarDrawer({ visible, onClose, docked = false }: Side {accounts.map((acc) => { const isActive = acc.id === activeAccountId; - let accHost = ''; - try { accHost = new URL(acc.serverUrl).hostname; } catch { accHost = acc.serverUrl; } + // With the port when it isn't the default (see hostLabel). + const accHost = serverHostLabel(acc.serverUrl); // The address leads when another account has the same name // (webmail 3bb9f7df8). const rowText = accountRowText(acc, accounts); diff --git a/src/components/settings/AccountSettings.tsx b/src/components/settings/AccountSettings.tsx index e2c4a9ba..0be8994e 100644 --- a/src/components/settings/AccountSettings.tsx +++ b/src/components/settings/AccountSettings.tsx @@ -15,7 +15,7 @@ import { setPendingSettingsTab } from '../../navigation/pending-settings-tab'; import { sharedAccountSettingsTabs } from '../../lib/capabilities'; import { jmapClient } from '../../api/jmap-client'; import { fetchMailQuota, type MailQuota } from '../../api/quota'; -import { MAX_ACCOUNTS, accountRowText } from '../../lib/account-utils'; +import { MAX_ACCOUNTS, accountRowText, nonDefaultPort, serverHostLabel } from '../../lib/account-utils'; import { toUnicodeEmail } from '../../lib/idn'; import type { RootStackParamList } from '../../navigation/types'; @@ -189,6 +189,8 @@ export function AccountSettings() { {isActive && } {rowText.subtitle ? {toUnicodeEmail(rowText.subtitle)} : null} + {/* Two servers on one host are two accounts with the same address. */} + {nonDefaultPort(a.serverUrl) ? {serverHostLabel(a.serverUrl)} : null} {isDefault && ( {t('settings.account.accounts.default_badge', 'Default account')} diff --git a/src/lib/__tests__/account-id-port.test.ts b/src/lib/__tests__/account-id-port.test.ts new file mode 100644 index 00000000..558f8ce0 --- /dev/null +++ b/src/lib/__tests__/account-id-port.test.ts @@ -0,0 +1,95 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { generateAccountId, nonDefaultPort, serverHostLabel, setKnownAccounts } from '../account-utils'; +import { useAccountStore } from '../../stores/account-store'; + +// The same address on two servers on one host was one account: ids were +// username@host, so signing in on http://10.10.10.62:8016 replaced the +// account of http://10.10.10.62:8120. The port is part of the id now, while +// accounts already signed in keep the id they have. + +const entry = (username: string, serverUrl: string) => ({ + serverUrl, + username, + displayName: username, + email: username, + lastLoginAt: 1, + isConnected: true, + hasError: false, +}); + +beforeEach(() => { + useAccountStore.setState({ accounts: [], activeAccountId: null, defaultAccountId: null }); +}); + +afterEach(() => { + useAccountStore.setState({ accounts: [], activeAccountId: null, defaultAccountId: null }); + setKnownAccounts([]); +}); + +describe('generateAccountId', () => { + it('keeps the ids of accounts on the default ports', () => { + expect(generateAccountId('u@example.org', 'https://mail.example.org')).toBe('u@example.org@mail.example.org'); + expect(generateAccountId('u@example.org', 'https://mail.example.org:443/')).toBe('u@example.org@mail.example.org'); + expect(generateAccountId('u@example.org', 'http://mail.example.org:80/jmap')).toBe('u@example.org@mail.example.org'); + }); + + it('tells two servers on one host apart by port', () => { + const a = generateAccountId('usera@example.org', 'http://10.10.10.62:8120'); + const b = generateAccountId('usera@example.org', 'http://10.10.10.62:8016'); + expect(a).toBe('usera@example.org@10.10.10.62:8120'); + expect(b).toBe('usera@example.org@10.10.10.62:8016'); + }); + + it('keeps the host-only id of an account signed in before ports counted', () => { + setKnownAccounts([{ id: 'usera@example.org@10.10.10.62', username: 'usera@example.org', serverUrl: 'http://10.10.10.62:8120' }]); + expect(generateAccountId('usera@example.org', 'http://10.10.10.62:8120')).toBe('usera@example.org@10.10.10.62'); + expect(generateAccountId('usera@example.org', 'http://10.10.10.62:8120/')).toBe('usera@example.org@10.10.10.62'); + // Another server on the same host is another account. + expect(generateAccountId('usera@example.org', 'http://10.10.10.62:8016')).toBe('usera@example.org@10.10.10.62:8016'); + }); +}); + +describe('the account registry', () => { + it('adds the same address on another port of the same host as a second account', () => { + const store = useAccountStore.getState(); + const a = store.addAccount(entry('usera@example.org', 'http://10.10.10.62:8120')); + const b = useAccountStore.getState().addAccount(entry('usera@example.org', 'http://10.10.10.62:8016')); + expect(a).not.toBe(b); + const { accounts } = useAccountStore.getState(); + expect(accounts.map((x) => x.serverUrl)).toEqual(['http://10.10.10.62:8120', 'http://10.10.10.62:8016']); + }); + + it('keeps an existing account under its old id when another port is added', () => { + // As persisted by an earlier version. + useAccountStore.setState({ + accounts: [{ + ...entry('usera@example.org', 'http://10.10.10.62:8120'), + id: 'usera@example.org@10.10.10.62', + avatarColor: '#000', + isDefault: true, + }], + activeAccountId: 'usera@example.org@10.10.10.62', + defaultAccountId: 'usera@example.org@10.10.10.62', + }); + // Signing in on the old server again updates that account. + expect(useAccountStore.getState().addAccount(entry('usera@example.org', 'http://10.10.10.62:8120'))) + .toBe('usera@example.org@10.10.10.62'); + const b = useAccountStore.getState().addAccount(entry('usera@example.org', 'http://10.10.10.62:8016')); + expect(b).toBe('usera@example.org@10.10.10.62:8016'); + expect(useAccountStore.getState().accounts.map((x) => [x.id, x.serverUrl])).toEqual([ + ['usera@example.org@10.10.10.62', 'http://10.10.10.62:8120'], + ['usera@example.org@10.10.10.62:8016', 'http://10.10.10.62:8016'], + ]); + }); +}); + +describe('serverHostLabel', () => { + it('shows the port only when it is not the default', () => { + expect(serverHostLabel('https://mail.example.org')).toBe('mail.example.org'); + expect(serverHostLabel('https://mail.example.org:443')).toBe('mail.example.org'); + expect(serverHostLabel('http://10.10.10.62:8016/')).toBe('10.10.10.62:8016'); + expect(serverHostLabel('https://mail.example.org:8443/jmap')).toBe('mail.example.org:8443'); + expect(nonDefaultPort('http://mail.example.org:80')).toBeNull(); + expect(nonDefaultPort('https://mail.example.org:80')).toBe('80'); + }); +}); diff --git a/src/lib/__tests__/push-notifications.test.ts b/src/lib/__tests__/push-notifications.test.ts index 2b2e4edd..1c1989a6 100644 --- a/src/lib/__tests__/push-notifications.test.ts +++ b/src/lib/__tests__/push-notifications.test.ts @@ -80,6 +80,7 @@ vi.mock('../../api/jmap-client', () => ({ jmapClient: { username: 'user@example.com', serverUrl: 'https://mail.example.com', + isConnected: true, accountId: 'jmap-primary', currentSession: { capabilities: { 'urn:ietf:params:jmap:core': {} } }, }, @@ -1582,6 +1583,45 @@ describe('resyncPushNotifications', () => { expect(createMock).not.toHaveBeenCalled(); }); + it('waits for the session instead of asking the server without one', async () => { + // An account being added: the client holds its credentials, no session yet. + const client = jmapClient as unknown as { isConnected: boolean }; + client.isConnected = false; + try { + await AsyncStorage.setItem(SUB_KEY, 'existing'); + await AsyncStorage.setItem(EXPIRES_KEY, inDays(5)); + const result = await resyncPushNotifications({ relayBaseUrl: RELAY }); + expect(result).toBeNull(); + expect(listMock).not.toHaveBeenCalled(); + expect(await AsyncStorage.getItem(OPTED_OUT_KEY)).toBeNull(); + } finally { + client.isConnected = true; + } + }); + + it('keeps push on when the client moved to another account during the revocation check', async () => { + await AsyncStorage.setItem(SUB_KEY, 'existing'); + await AsyncStorage.setItem(EXPIRES_KEY, inDays(5)); + await AsyncStorage.setItem('push:accountIds:v1', JSON.stringify([ACCOUNT_ID])); + const client = jmapClient as unknown as { serverUrl: string }; + // The listing answers for the account added meanwhile, which has none of ours. + listMock.mockImplementation(async () => { + client.serverUrl = 'https://mail.example.com:8443'; + return []; + }); + try { + const result = await resyncPushNotifications({ relayBaseUrl: RELAY }); + expect(result).toBeNull(); + expect(await AsyncStorage.getItem(OPTED_OUT_KEY)).toBeNull(); + expect(await AsyncStorage.getItem(SUB_KEY)).toBe('existing'); + expect(await readPushAccountIds()).toEqual([ACCOUNT_ID]); + } finally { + client.serverUrl = 'https://mail.example.com'; + listMock.mockReset(); + listMock.mockResolvedValue([]); + } + }); + it('does not re-register a subscription another device revoked', async () => { await AsyncStorage.setItem(SUB_KEY, 'existing'); await AsyncStorage.setItem(EXPIRES_KEY, inDays(5)); diff --git a/src/lib/account-utils.ts b/src/lib/account-utils.ts index a330ecbc..13903bfd 100644 --- a/src/lib/account-utils.ts +++ b/src/lib/account-utils.ts @@ -13,7 +13,34 @@ export class AccountLimitError extends Error { } } -export function generateAccountId(username: string, serverUrl: string): string { +/** + * The port of `serverUrl` when it is not the scheme's default (443 for + * https, 80 for http), else null. Read from the text, since React Native's + * URL keeps a default port where the WHATWG one drops it. + */ +export function nonDefaultPort(serverUrl: string): string | null { + const m = /^([a-z][a-z0-9+.-]*):\/\/(?:[^@/?#]*@)?(?:\[[^\]]*\]|[^:/?#]*):(\d+)(?=[/?#]|$)/i.exec(serverUrl.trim()); + if (!m) return null; + const scheme = m[1].toLowerCase(); + const port = String(Number(m[2])); + if ((scheme === 'https' && port === '443') || (scheme === 'http' && port === '80')) return null; + return port; +} + +/** `serverUrl`'s host, with its port when that is not the default. */ +export function serverHostLabel(serverUrl: string): string { + let host: string; + try { + host = new URL(serverUrl).hostname; + } catch { + return serverUrl.replace(/^https?:\/\//, '').replace(/\/.*$/, ''); + } + const port = nonDefaultPort(serverUrl); + return port ? `${host}:${port}` : host; +} + +/** The id an account got before ports were part of it: username@host. */ +function hostOnlyAccountId(username: string, serverUrl: string): string { let host = serverUrl; try { host = new URL(serverUrl).hostname; @@ -23,6 +50,47 @@ export function generateAccountId(username: string, serverUrl: string): string { return `${username}@${host}`; } +/** What the registry tells generateAccountId (structural: no AccountEntry import). */ +export interface KnownAccount { + id: string; + username: string; + serverUrl: string; +} + +let knownAccounts: readonly KnownAccount[] = []; + +/** + * The registry's accounts, so generateAccountId keeps the ids accounts were + * signed in under. The account store feeds this whenever it changes; the + * headless push task, which reads the registry from storage, feeds it too. + */ +export function setKnownAccounts(accounts: readonly KnownAccount[]): void { + knownAccounts = accounts; +} + +/** + * The local id of the account `username` has on `serverUrl`: + * username@host, plus `:port` when the server is not on its scheme's + * default port. Two servers on one host are two accounts. + * + * Accounts were once keyed by username@host only. One signed in that way on + * a server with a port keeps that id, and with it its credentials, caches, + * outbox, push registration and settings: when the registry holds the + * host-only id for the same server, that is the answer. A second server on + * the same host then gets the id with its port, and the two stay apart. + * Accounts on the default ports have the same id either way. + */ +export function generateAccountId(username: string, serverUrl: string): string { + const legacy = hostOnlyAccountId(username, serverUrl); + const host = serverHostLabel(serverUrl); + const id = `${username}@${host}`; + if (id === legacy) return id; + const kept = knownAccounts.find((a) => a.id === legacy + && a.username === username + && `${a.username}@${serverHostLabel(a.serverUrl)}` === id); + return kept ? kept.id : id; +} + /** What naming an account in a list needs (structural: no AccountEntry import). */ export interface NamedAccount { id: string; diff --git a/src/lib/push-background-task.ts b/src/lib/push-background-task.ts index 53c09bf7..86f22ea2 100644 --- a/src/lib/push-background-task.ts +++ b/src/lib/push-background-task.ts @@ -5,6 +5,7 @@ import { CAPABILITIES } from '../api/types'; import type { Email, JMAPMethodCall, JMAPSession, Mailbox } from '../api/types'; import { detectDeviceLocale, isSupportedLocale, translate, type LocaleCode } from '../i18n'; import { secureFetch } from './client-cert'; +import { setKnownAccounts, type KnownAccount } from './account-utils'; import { freshStoredCredentials } from './stored-token-refresh'; import { generateEmailAvatarColor, @@ -194,6 +195,7 @@ export function parseRelayPushData(data: unknown): RelayPushData { interface RegistryAccount { id: string; username?: string; + serverUrl?: string; } async function readAccountRegistry(): Promise { @@ -452,6 +454,9 @@ export async function pushBackgroundTask(data: unknown): Promise { if (accountIds.length === 0) return; const registry = await readAccountRegistry(); + // Without the account store loaded, account ids are still the ones the + // registry holds (see generateAccountId). + setKnownAccounts(registry.filter((a): a is KnownAccount => !!a.username && !!a.serverUrl)); const jmapAccountIds = await readPushJmapAccountIds(); const match = matchAccountsForPush(payload, accountIds, jmapAccountIds, registry); if (match.ambiguous) await showPlainNotification(payload); diff --git a/src/lib/push-notifications.ts b/src/lib/push-notifications.ts index c9a61d50..1993c44e 100644 --- a/src/lib/push-notifications.ts +++ b/src/lib/push-notifications.ts @@ -1224,6 +1224,28 @@ async function pollVerificationCode( // PushVerification"). Callers share the first in-flight run instead. const inFlightSetups = new Map>(); +/** + * The local account the singleton client serves with a live session; null + * while it has none, such as during an account change, when it holds the + * credentials of the account being signed in to before its session is up. + * The setup asks the server through the client, so it must know the client + * still serves the account it is setting up: a listing of another account's + * subscriptions took the account's own for revoked and turned its push off. + */ +function servedAccountId(): string | null { + const username = jmapClient.username; + const serverUrl = jmapClient.serverUrl; + if (!jmapClient.isConnected || !username || !serverUrl) return null; + return generateAccountId(username, serverUrl); +} + +function accountChangedError(): PushSetupError { + return new PushSetupError( + 'account', + t('settings.notifications.push.err_account_changed', 'The account changed while push was being set up. Try again.'), + ); +} + /** * Full setup flow: ask permission, fetch the device's FCM token, register * with the relay, create a JMAP PushSubscription, poll for the verification @@ -1300,12 +1322,10 @@ async function setupPushNotificationsInner( // setupPushNotifications operates on the currently-loaded jmapClient. We // need its username/serverUrl up-front so we can key per-account state. - const username = jmapClient.username; - const serverUrl = jmapClient.serverUrl; - if (!username || !serverUrl) { + const accountId = servedAccountId(); + if (!accountId) { throw new PushSetupError('account', t('settings.notifications.push.err_no_account', 'No account loaded - cannot set up push.')); } - const accountId = generateAccountId(username, serverUrl); await migrateLegacyPushKeys(); @@ -1381,6 +1401,8 @@ async function setupPushNotificationsInner( // A failed listing ends the run here: taken for "the server has no // subscription", it would drop the key of a working registration. logPhase('jmap'); + // From here on the server is asked through the client (see servedAccountId). + if (servedAccountId() !== accountId) throw accountChangedError(); let existingSubs: PushSubscription[]; try { existingSubs = await listPushSubscriptions(); @@ -1868,12 +1890,15 @@ function isOwnSubscription( export async function resyncPushNotifications( params: PushSetupParams, ): Promise { - const username = jmapClient.username; - const serverUrl = jmapClient.serverUrl; - if (!username || !serverUrl) return null; - const accountId = generateAccountId(username, serverUrl); + // Without a live session (an account change under way) there is nothing + // to ask; the caller runs this again once the session is up. + const accountId = servedAccountId(); + if (!accountId) return null; if (await AsyncStorage.getItem(optedOutKey(accountId))) return null; - if (await wasRevokedOnServer(accountId)) { + const revoked = await wasRevokedOnServer(accountId); + // The listing went to whatever account the client served meanwhile. + if (servedAccountId() !== accountId) return null; + if (revoked) { logPhase('revoked', 'subscription gone before it was due to expire; leaving push off'); await disablePushForAccount(accountId); return null; diff --git a/src/stores/account-store.ts b/src/stores/account-store.ts index 58e9f946..e10a3110 100644 --- a/src/stores/account-store.ts +++ b/src/stores/account-store.ts @@ -1,7 +1,7 @@ import { create } from 'zustand'; import { persist } from 'zustand/middleware'; import { createPersistStorage } from './persist-storage'; -import { AccountLimitError, generateAccountId, MAX_ACCOUNTS } from '../lib/account-utils'; +import { AccountLimitError, generateAccountId, MAX_ACCOUNTS, setKnownAccounts } from '../lib/account-utils'; import { generateAvatarColor } from '../lib/avatar-utils'; export interface AccountEntry { @@ -58,6 +58,9 @@ export const useAccountStore = create()( a.id === id ? { ...a, + // The same server under another URL (a path, a scheme): + // the credentials just stored carry the new one. + serverUrl: entry.serverUrl || a.serverUrl, displayName: entry.displayName || a.displayName, email: entry.email || a.email, lastLoginAt: entry.lastLoginAt, @@ -180,3 +183,8 @@ export const useAccountStore = create()( }, ), ); + +// generateAccountId keeps the ids accounts were signed in under, which it +// reads from the registry (see lib/account-utils). +setKnownAccounts(useAccountStore.getState().accounts); +useAccountStore.subscribe((state) => setKnownAccounts(state.accounts)); diff --git a/src/stores/auth-store.ts b/src/stores/auth-store.ts index b7eac632..69d80591 100644 --- a/src/stores/auth-store.ts +++ b/src/stores/auth-store.ts @@ -203,6 +203,19 @@ async function checkServerChange(accountId: string, session: JMAPSession): Promi } } +// The registry names the server the account's stored credentials sign in +// to. Before ports were part of account ids, signing in to a second server +// on the same host stored its credentials under the first server's account +// and left the registry naming the first server. That account has been the +// second server's since (its mail, its sends), and now says so; the ids +// stay consistent with it (see generateAccountId). +function followStoredServer(accountId: string): void { + const served = jmapClient.serverUrl; + const accountStore = useAccountStore.getState(); + const entry = accountStore.getAccountById(accountId); + if (served && entry && entry.serverUrl !== served) accountStore.updateAccount(accountId, { serverUrl: served }); +} + // The outbox replays against whatever account the JMAP client serves, and a // replaced server may have given the queued ids to other messages. So it // runs only once the account's session is up and its server was checked. @@ -993,6 +1006,7 @@ export const useAuthStore = create((set, get) => ({ set({ isLoading: false, error: 'Failed to load session' }); return; } + followStoredServer(accountId); await checkServerChange(accountId, session); if (!isCurrent()) return; @@ -1139,6 +1153,7 @@ export const useAuthStore = create((set, get) => ({ }); const session = jmapClient.currentSession!; + followStoredServer(target.id); await checkServerChange(target.id, session); applyConnectedState(set, session, target.serverUrl, target.username, target.id); perfMark('restore-done'); @@ -1183,6 +1198,7 @@ export const useAuthStore = create((set, get) => ({ errorMessage: undefined, }); const fresh = jmapClient.currentSession!; + followStoredServer(activeAccountId); await checkServerChange(activeAccountId, fresh); if (!isCurrent()) return false; applyConnectedState(set, fresh, target.serverUrl, target.username, activeAccountId); From f9a36b28925774e64a845c2c319b0b47611b2854 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:24:56 +0100 Subject: [PATCH 3/7] fix: open a notification or link for another account over that account's inbox --- App.tsx | 39 ++++++++++----------- src/navigation/__tests__/linking.test.ts | 37 +++++++++++++++++++- src/navigation/linking.ts | 43 +++++++++++++++++++++--- 3 files changed, 92 insertions(+), 27 deletions(-) diff --git a/App.tsx b/App.tsx index e0729c85..27414f71 100644 --- a/App.tsx +++ b/App.tsx @@ -82,10 +82,12 @@ import { signOutWidgets, startWidgetSync } from './src/widgets/sync'; import { draftContextFromEmail } from './src/lib/draft-context'; import { acceptSignInLink, + enterLinkAccount, handleDeepLink, parseDeepLink, routeParkedSignInLink, shareToDeepLink, + type AccountSwitchResult, type DeepLink, } from './src/navigation/linking'; import { usePendingSignInLinkStore } from './src/navigation/pending-sign-in-link'; @@ -107,20 +109,26 @@ const navigationRef = createNavigationContainerRef(); // on every getInitialURL(), and a sign-in link must not run twice. let initialSignInLinkRead = false; +// Move to the account a notification, reminder or link names. An account +// that was signed out meanwhile has nothing safe to open. +async function switchToAccount(accountId: string): Promise { + const auth = useAuthStore.getState(); + if (auth.activeAccountId === accountId) return true; + if (!useAccountStore.getState().getAccountById(accountId)) return false; + await auth.switchAccount(accountId); + return useAuthStore.getState().activeAccountId === accountId ? 'switched' : false; +} + async function navigateToNotificationTap(payload: NotificationTapPayload): Promise { if (!navigationRef.isReady()) return; // The notification carries the account it was generated for. If the user // has since switched to a different account (or had a different one active // when the notification arrived), opening EmailThread under the active - // account would fetch the email from the wrong server and fail. - const auth = useAuthStore.getState(); - if (payload.accountId && payload.accountId !== auth.activeAccountId) { - const account = useAccountStore.getState().getAccountById(payload.accountId); - if (!account) return; // account was logged out — nothing safe to open. - await auth.switchAccount(payload.accountId); - if (useAuthStore.getState().activeAccountId !== payload.accountId) return; - } + // account would fetch the email from the wrong server and fail. After a + // switch the stack starts over on that account's inbox, so Back from the + // message lands there instead of on the old account's screens. + if (!(await enterLinkAccount(navigationRef, payload.accountId, switchToAccount))) return; prefetchMessage({ id: payload.emailId, threadId: payload.threadId }); navigationRef.navigate('EmailThread', { @@ -138,12 +146,7 @@ async function navigateToNotificationTap(payload: NotificationTapPayload): Promi // under the account it was scheduled for. The target is parked for // CalendarScreen, which also picks it up when it mounts after a cold start. async function openCalendarReminder(target: CalendarReminderTarget): Promise { - const auth = useAuthStore.getState(); - if (target.appAccountId && target.appAccountId !== auth.activeAccountId) { - if (!useAccountStore.getState().getAccountById(target.appAccountId)) return; - await auth.switchAccount(target.appAccountId); - if (useAuthStore.getState().activeAccountId !== target.appAccountId) return; - } + if (!(await enterLinkAccount(navigationRef, target.appAccountId, switchToAccount))) return; setPendingCalendarOpen(target); // On a cold start the navigator mounts right after the auth gate flips. for (let attempt = 0; attempt < 50 && !navigationRef.isReady(); attempt++) { @@ -178,13 +181,7 @@ async function openDeepLink(link: DeepLink): Promise { return false; } }, - switchAccount: async (accountId) => { - const auth = useAuthStore.getState(); - if (auth.activeAccountId === accountId) return true; - if (!useAccountStore.getState().getAccountById(accountId)) return false; - await auth.switchAccount(accountId); - return useAuthStore.getState().activeAccountId === accountId; - }, + switchAccount: switchToAccount, }); // A widget or a link elsewhere brought the app up for this; say it failed // rather than leave the user wondering why the app just opened. diff --git a/src/navigation/__tests__/linking.test.ts b/src/navigation/__tests__/linking.test.ts index be7c5cf3..f0f18907 100644 --- a/src/navigation/__tests__/linking.test.ts +++ b/src/navigation/__tests__/linking.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, vi } from 'vitest'; -import { acceptSignInLink, handleDeepLink, parseDeepLink, parseSignInLink, shareToDeepLink } from '../linking'; +import { acceptSignInLink, enterLinkAccount, handleDeepLink, parseDeepLink, parseSignInLink, shareToDeepLink } from '../linking'; +import { usePendingMailFolder } from '../pending-mail-folder'; import { usePendingSignInLinkStore } from '../pending-sign-in-link'; import { usePendingSettingsTab } from '../pending-settings-tab'; import { usePendingCalendarOpen } from '../pending-calendar-open'; @@ -168,6 +169,40 @@ describe('handleDeepLink', () => { })).toBe(false); }); + it('starts the stack over on the inbox of the account a link switched to', async () => { + const calls: unknown[][] = []; + const navigation = { + isReady: () => true, + navigate: vi.fn((...args: unknown[]) => { calls.push(['navigate', ...args]); }), + reset: vi.fn((state: unknown) => { calls.push(['reset', state]); }), + }; + const ok = await handleDeepLink( + { kind: 'message', emailId: 'M1', threadId: 'T1', accountId: 'b@example.org@mail' }, + { navigation: navigation as never, resolveThreadId: async () => null, switchAccount: async () => 'switched' }, + ); + expect(ok).toBe(true); + // The old account's screens go, the message opens over the new inbox. + expect(calls).toEqual([ + ['reset', { index: 0, routes: [{ name: 'MainTabs', params: { screen: 'Mail' } }] }], + ['navigate', 'EmailThread', { emailId: 'M1', threadId: 'T1' }], + ]); + expect(usePendingMailFolder.getState().consume()).toBe('inbox'); + }); + + it('leaves the stack alone when the link names the active account', async () => { + const navigation = { isReady: () => true, navigate: vi.fn(), reset: vi.fn() }; + await handleDeepLink( + { kind: 'message', emailId: 'M1', threadId: 'T1', accountId: 'a@example.org@mail' }, + { navigation: navigation as never, resolveThreadId: async () => null, switchAccount: async () => true }, + ); + expect(navigation.reset).not.toHaveBeenCalled(); + expect(navigation.navigate).toHaveBeenCalledWith('EmailThread', { emailId: 'M1', threadId: 'T1' }); + // A notification tap goes the same way (App navigateToNotificationTap). + expect(await enterLinkAccount(navigation as never, 'gone@example.org@mail', async () => false)).toBe(false); + expect(await enterLinkAccount(navigation as never, undefined, async () => 'switched')).toBe(true); + expect(navigation.reset).not.toHaveBeenCalled(); + }); + it('parks the settings tab and opens the Settings tab', async () => { const navigation = nav(); await handleDeepLink( diff --git a/src/navigation/linking.ts b/src/navigation/linking.ts index ddadeac4..a46efb38 100644 --- a/src/navigation/linking.ts +++ b/src/navigation/linking.ts @@ -276,6 +276,40 @@ export function routeParkedSignInLink(accountCount: number): 'add-account' | 'ac return 'add-account'; } +/** + * What switching to the account a link or notification names came to: false + * when that account is not signed in here (or the switch failed), true when + * it was the active one already, 'switched' when the app moved to it. + */ +export type AccountSwitchResult = boolean | 'switched'; + +/** + * After a link or notification switched accounts, the screens on the stack + * are the old account's. Start the stack over on the new account's inbox, so + * whatever the link opens goes on top of it and Back lands there. + */ +export function resetToAccountInbox(navigation: NavigationContainerRefWithCurrent): void { + setPendingMailFolder('inbox'); + navigation.reset({ index: 0, routes: [{ name: 'MainTabs', params: { screen: 'Mail' } }] } as never); +} + +/** + * Move to the account `accountId` names, if it is not the active one, and + * start the stack over when the app moved (resetToAccountInbox). False when + * the account is not signed in here: nothing may be opened then. + */ +export async function enterLinkAccount( + navigation: NavigationContainerRefWithCurrent, + accountId: string | undefined, + switchAccount: ((accountId: string) => Promise) | undefined, +): Promise { + if (!accountId || !switchAccount) return true; + const result = await switchAccount(accountId); + if (!result) return false; + if (result === 'switched' && navigation.isReady()) resetToAccountInbox(navigation); + return true; +} + export interface DeepLinkNavigator { navigation: NavigationContainerRefWithCurrent; // Resolve a message id to its thread (EmailThread needs both). Returns @@ -286,8 +320,9 @@ export interface DeepLinkNavigator { // be loaded. openDraft?: (emailId: string, jmapAccountId?: string) => Promise; // Switch to the account a permalink names (`?account=`); resolves false - // when that account is not signed in on this device. - switchAccount?: (accountId: string) => Promise; + // when that account is not signed in on this device, 'switched' when the + // app moved to it (see AccountSwitchResult). + switchAccount?: (accountId: string) => Promise; } /** Navigate for a parsed link. Returns false when nothing could be opened. */ @@ -295,9 +330,7 @@ export async function handleDeepLink(link: DeepLink, nav: DeepLinkNavigator): Pr const { navigation } = nav; if (!navigation.isReady()) return false; - if ('accountId' in link && link.accountId && nav.switchAccount) { - if (!(await nav.switchAccount(link.accountId))) return false; - } + if ('accountId' in link && !(await enterLinkAccount(navigation, link.accountId, nav.switchAccount))) return false; switch (link.kind) { case 'message': { From d00605492afd1c20d584b8f6c3b77573453fdf75 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:39:44 +0100 Subject: [PATCH 4/7] fix: keep an account whose saved sign-in stops working and ask to sign in again --- App.tsx | 18 ++ locales/rn/en.json | 3 + src/components/SidebarDrawer.tsx | 12 +- src/components/settings/AccountSettings.tsx | 11 +- src/screens/LoginScreen.tsx | 31 +++- src/screens/login/reauth-start.ts | 23 +++ .../__tests__/auth-store-logout.test.ts | 37 ++-- .../__tests__/auth-store-reauth.test.ts | 128 ++++++++++++++ src/stores/account-store.ts | 6 + src/stores/auth-store.ts | 160 +++++++++++++----- 10 files changed, 365 insertions(+), 64 deletions(-) create mode 100644 src/screens/login/reauth-start.ts create mode 100644 src/stores/__tests__/auth-store-reauth.test.ts diff --git a/App.tsx b/App.tsx index 27414f71..efb67f55 100644 --- a/App.tsx +++ b/App.tsx @@ -640,6 +640,24 @@ export default function App() { return () => { cancelled = true; }; }, [addAccountForLink]); + // The server refused the stored sign-in of an account (auth store + // keepForSignIn): open the sign-in to it, filled in, over what is cached. + const reauthTarget = useAuthStore((s) => s.reauth); + React.useEffect(() => { + if (!isAuthenticated || !reauthTarget) return; + let cancelled = false; + void (async () => { + for (let attempt = 0; attempt < 50 && !navigationRef.isReady(); attempt++) { + await new Promise((resolve) => setTimeout(resolve, 100)); + if (cancelled) return; + } + if (cancelled || !navigationRef.isReady()) return; + if (navigationRef.getCurrentRoute()?.name === 'AddAccount') return; + navigationRef.navigate('AddAccount'); + })(); + return () => { cancelled = true; }; + }, [isAuthenticated, reauthTarget]); + // Re-register the device with the configured relay once authenticated, // and whenever the FCM token rotates. Honours the user's notification // preference - flipping it off tears down THIS account's subscription so diff --git a/locales/rn/en.json b/locales/rn/en.json index b89ab690..727d039c 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -854,6 +854,8 @@ "qr_hint": "Settings → Security → Link Mobile App shows a code. Point the camera at it.", "qr_hint_title": "Open Bulwark on your computer", "qr_title": "Sign-in code", + "reauth_detail": "The server no longer accepts the saved sign-in for {account}. Sign in again to keep using this account.", + "reauth_title": "Sign in again", "scan_desc": "The fastest way in if you're signed in on the web", "scan_title": "Scan a sign-in code", "server_cert_active": "Signing in with the client certificate \"{alias}\". Tap to choose another.", @@ -1649,6 +1651,7 @@ }, "sidebar": { "account_error": "Needs attention", + "account_sign_in_again": "Sign in again", "remove_account_hint": "Long-press to remove this account", "remove_account_message": "Sign out of {account} on this device. Your mail stays on the server.", "remove_account_title": "Remove account?", diff --git a/src/components/SidebarDrawer.tsx b/src/components/SidebarDrawer.tsx index 937b4d95..e6354fdd 100644 --- a/src/components/SidebarDrawer.tsx +++ b/src/components/SidebarDrawer.tsx @@ -339,6 +339,7 @@ export default function SidebarDrawer({ visible, onClose, docked = false }: Side const logout = useAuthStore((s) => s.logout); const logoutAll = useAuthStore((s) => s.logoutAll); const switchAccount = useAuthStore((s) => s.switchAccount); + const requestReauth = useAuthStore((s) => s.requestReauth); const activeAccountId = useAuthStore((s) => s.activeAccountId); const accounts = useAccountStore((s) => s.accounts); const setDefaultAccount = useAccountStore((s) => s.setDefaultAccount); @@ -869,6 +870,13 @@ export default function SidebarDrawer({ visible, onClose, docked = false }: Side { + // The server refused its stored sign-in: sign in again. + if (acc.needsSignIn) { + setAccountMenuOpen(false); + onClose(); + void requestReauth(acc.id); + return; + } if (isActive) return; setAccountMenuOpen(false); onClose(); @@ -912,7 +920,9 @@ export default function SidebarDrawer({ visible, onClose, docked = false }: Side <> - {acc.errorMessage || t('sidebar.account_error', 'Needs attention')} + {acc.needsSignIn + ? t('sidebar.account_sign_in_again', 'Sign in again') + : acc.errorMessage || t('sidebar.account_error', 'Needs attention')} ) : ( diff --git a/src/components/settings/AccountSettings.tsx b/src/components/settings/AccountSettings.tsx index 0be8994e..32d4ede4 100644 --- a/src/components/settings/AccountSettings.tsx +++ b/src/components/settings/AccountSettings.tsx @@ -41,6 +41,7 @@ export function AccountSettings() { const activeAccountId = useAuthStore((s) => s.activeAccountId); const session = useAuthStore((s) => s.session); const switchAccount = useAuthStore((s) => s.switchAccount); + const requestReauth = useAuthStore((s) => s.requestReauth); const removeAccountAuth = useAuthStore((s) => s.removeAccount); const accounts = useAccountStore((s) => s.accounts); const defaultAccountId = useAccountStore((s) => s.defaultAccountId); @@ -178,7 +179,11 @@ export function AccountSettings() { { if (!isActive) void switchAccount(a.id); }} + onPress={() => { + // The server refused its stored sign-in: sign in again. + if (a.needsSignIn) void requestReauth(a.id); + else if (!isActive) void switchAccount(a.id); + }} accessibilityRole="button" accessibilityLabel={isActive ? t('settings.account.accounts.active', 'Currently active account') @@ -196,7 +201,9 @@ export function AccountSettings() { {t('settings.account.accounts.default_badge', 'Default account')} )} - {a.hasError && a.errorMessage ? ( + {a.needsSignIn ? ( + {t('sidebar.account_sign_in_again', 'Sign in again')} + ) : a.hasError && a.errorMessage ? ( {a.errorMessage} ) : null} diff --git a/src/screens/LoginScreen.tsx b/src/screens/LoginScreen.tsx index 822ca07e..9ec0dadb 100644 --- a/src/screens/LoginScreen.tsx +++ b/src/screens/LoginScreen.tsx @@ -34,6 +34,7 @@ import ConfirmStep from './login/ConfirmStep'; import PasswordStep from './login/PasswordStep'; import SigningInStep, { type SigningInPhase } from './login/SigningInStep'; import { loginBackAction } from './login/login-back'; +import { reauthStart, type LoginStepName } from './login/reauth-start'; import { isSignInCancelled } from '../lib/sign-in-abort'; interface LoginScreenProps { @@ -42,7 +43,7 @@ interface LoginScreenProps { onCancel?: () => void; } -type StepName = 'choose' | 'email' | 'server' | 'confirm' | 'password'; +type StepName = LoginStepName; const RECENT_EMAILS_KEY = 'login:recentEmails:v1'; @@ -92,11 +93,20 @@ export default function LoginScreen({ onLogin, isAddMode = false, onCancel }: Lo const accounts = useAccountStore((state) => state.accounts); const activeAccountId = useAccountStore((state) => state.activeAccountId); - const [step, setStep] = React.useState('choose'); + // Signing in to an account again (its stored sign-in was refused) starts + // on its form, filled in. Read once: the store clears it on success. + const [reauth] = React.useState(() => useAuthStore.getState().reauth); + const start = React.useMemo(() => reauthStart(reauth), [reauth]); + // Closed without signing in: the request is answered (the account row + // still offers it again). + React.useEffect(() => () => { + if (reauth && useAuthStore.getState().reauth === reauth) useAuthStore.getState().clearReauth(); + }, [reauth]); + const [step, setStep] = React.useState(start.step); const [history, setHistory] = React.useState([]); - const [email, setEmail] = React.useState(''); - const [serverInput, setServerInput] = React.useState(''); - const [serverUrl, setServerUrl] = React.useState(''); + const [email, setEmail] = React.useState(start.email); + const [serverInput, setServerInput] = React.useState(start.serverUrl); + const [serverUrl, setServerUrl] = React.useState(start.serverUrl); // Drives the confirm step's heading: we only claim to have "found" a server // when discovery actually found one. const [serverDiscovered, setServerDiscovered] = React.useState(false); @@ -107,10 +117,17 @@ export default function LoginScreen({ onLogin, isAddMode = false, onCancel }: Lo const [totpRequired, setTotpRequired] = React.useState(false); // Access-token (Bearer) sign-in in place of username and password, for // providers that hand out API tokens (Fastmail accepts nothing else). - const [tokenMode, setTokenMode] = React.useState(false); + const [tokenMode, setTokenMode] = React.useState(start.tokenMode); const [accessToken, setAccessToken] = React.useState(''); const [failedDomain, setFailedDomain] = React.useState(null); - const [notice, setNotice] = React.useState(null); + const [notice, setNotice] = React.useState(() => (reauth ? { + title: t('login.mobile.reauth_title', 'Sign in again'), + detail: t( + 'login.mobile.reauth_detail', + 'The server no longer accepts the saved sign-in for {account}. Sign in again to keep using this account.', + { account: reauth.username }, + ), + } : null)); const [searching, setSearching] = React.useState(false); const [busy, setBusy] = React.useState(null); const [scannerVisible, setScannerVisible] = React.useState(false); diff --git a/src/screens/login/reauth-start.ts b/src/screens/login/reauth-start.ts new file mode 100644 index 00000000..2e24b903 --- /dev/null +++ b/src/screens/login/reauth-start.ts @@ -0,0 +1,23 @@ +import type { ReauthTarget } from '../../stores/auth-store'; + +export type LoginStepName = 'choose' | 'email' | 'server' | 'confirm' | 'password'; + +/** + * Where signing in to an account again starts (auth store keepForSignIn): a + * pasted token or a password account at the form, with the token field or + * the address filled in; an OAuth account at the server's sign-in page. + */ +export function reauthStart(reauth: ReauthTarget | null): { + step: LoginStepName; + email: string; + serverUrl: string; + tokenMode: boolean; +} { + if (!reauth) return { step: 'choose', email: '', serverUrl: '', tokenMode: false }; + return { + step: reauth.method === 'oauth' ? 'confirm' : 'password', + email: reauth.method === 'token' ? '' : reauth.username, + serverUrl: reauth.serverUrl, + tokenMode: reauth.method === 'token', + }; +} diff --git a/src/stores/__tests__/auth-store-logout.test.ts b/src/stores/__tests__/auth-store-logout.test.ts index c0fe09f6..54698238 100644 --- a/src/stores/__tests__/auth-store-logout.test.ts +++ b/src/stores/__tests__/auth-store-logout.test.ts @@ -80,6 +80,7 @@ vi.mock('../../api/jmap-client', () => { clearAccountCredentials: vi.fn(async () => undefined), clearAllCredentials: vi.fn(async () => undefined), getStoredOAuthTokens: vi.fn(async () => null), + getStoredCredentials: vi.fn(async () => null), onAuthFailure: vi.fn(() => () => undefined), onTokenRefresh: vi.fn(() => () => undefined), hasAccountCapability: vi.fn(() => true), @@ -206,7 +207,7 @@ describe('signing out of the active account (Stalwart 1.0)', () => { expect(useOutboxStore.getState().activeAccountId).toBe(B); }); - it('skips a next account whose sign-in is refused', async () => { + it('skips a next account whose sign-in is refused, which stays for a new sign-in', async () => { await signedIn(A, B, C); client.loadErrors = { [B]: new AuthenticationError('Invalid credentials') }; @@ -214,7 +215,10 @@ describe('signing out of the active account (Stalwart 1.0)', () => { await settle(); expect(where()).toEqual({ signedIn: true, auth: C, registry: C, email: C, client: C }); - expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([C]); + expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([B, C]); + expect(useAccountStore.getState().getAccountById(B)?.needsSignIn).toBe(true); + // Landing on C does not open a sign-in to B; its row offers it. + expect(useAuthStore.getState().reauth).toBeNull(); }); }); @@ -281,18 +285,18 @@ describe('what a sign-out leaves behind', () => { expect(leftovers().feeds).toEqual([`sub-${A}`, `sub-${B}`]); }); - it('drops the feeds of an account whose sign-in is refused on the switch to it', async () => { + it('keeps the feeds of an account whose sign-in is refused on the switch to it', async () => { await signedIn(A, B); useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A), feed(B)] }); client.loadErrors = { [B]: new AuthenticationError('Invalid credentials') }; await useAuthStore.getState().switchAccount(B); - expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([A]); - expect(leftovers().feeds).toEqual([`sub-${A}`]); + expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([A, B]); + expect(leftovers().feeds).toEqual([`sub-${A}`, `sub-${B}`]); }); - it('clears them when a switch evicts the last account', async () => { + it('keeps them when a switch to the last account is refused', async () => { // Signed out of the active account while the last one's server was // away; it is refused when the user picks it. await signedIn(A); @@ -304,24 +308,26 @@ describe('what a sign-out leaves behind', () => { await useAuthStore.getState().switchAccount(A); - expect(useAccountStore.getState().accounts).toEqual([]); - expect(leftovers()).toEqual({ searches: [], feeds: [] }); + expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([A]); + expect(leftovers()).toEqual({ searches: ['invoice'], feeds: [`sub-${A}`] }); }); - it('clears them when the restore on launch evicts the last account', async () => { + it('keeps them when the restore on launch is refused', async () => { await signedIn(A); useAuthStore.setState({ isAuthenticated: false, session: null, activeAccountId: null, accountId: null }); useSearchHistoryStore.setState({ recentSearches: ['invoice'] }); useCalendarSubscriptionsStore.setState({ subscriptions: [feed(A)] }); client.loadErrors = { [A]: new AuthenticationError('Invalid credentials') }; - expect(await useAuthStore.getState().restoreSession()).toBe(false); + // The cached mail shows while the app asks for a new sign-in. + expect(await useAuthStore.getState().restoreSession()).toBe(true); - expect(useAccountStore.getState().accounts).toEqual([]); - expect(leftovers()).toEqual({ searches: [], feeds: [] }); + expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([A]); + expect(useAuthStore.getState().reauth?.accountId).toBe(A); + expect(leftovers()).toEqual({ searches: ['invoice'], feeds: [`sub-${A}`] }); }); - it('clears them when a session retry evicts the last account', async () => { + it('keeps them when a session retry is refused', async () => { await signedIn(A); useAuthStore.setState({ session: null }); useSearchHistoryStore.setState({ recentSearches: ['invoice'] }); @@ -330,8 +336,9 @@ describe('what a sign-out leaves behind', () => { expect(await useAuthStore.getState().retrySession()).toBe(false); - expect(useAccountStore.getState().accounts).toEqual([]); - expect(leftovers()).toEqual({ searches: [], feeds: [] }); + expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual([A]); + expect(useAuthStore.getState().reauth?.accountId).toBe(A); + expect(leftovers()).toEqual({ searches: ['invoice'], feeds: [`sub-${A}`] }); }); it('clears them when the last account signs out', async () => { diff --git a/src/stores/__tests__/auth-store-reauth.test.ts b/src/stores/__tests__/auth-store-reauth.test.ts new file mode 100644 index 00000000..ad996554 --- /dev/null +++ b/src/stores/__tests__/auth-store-reauth.test.ts @@ -0,0 +1,128 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// The server refusing an account's stored sign-in (a Stalwart 0.16 server +// moved to 1.0, an expired pasted token, a changed password) used to remove +// the account with "Your session has expired". It stays now, with its caches +// and credentials, and the app asks for a new sign-in to it. + +const stored = vi.hoisted(() => ({ creds: null as Record | null })); + +vi.mock('../../api/jmap-client', () => ({ + jmapClient: { + connect: vi.fn(), + logout: vi.fn(), + loadAccount: vi.fn(), + consumeLegacyCredentials: vi.fn(async () => null), + clearAccountCredentials: vi.fn(async () => undefined), + clearAllCredentials: vi.fn(async () => undefined), + getStoredCredentials: vi.fn(async () => stored.creds), + getStoredOAuthTokens: vi.fn(async () => null), + reset: vi.fn(), + snapshot: vi.fn(() => ({ session: 'live', credentials: 'live', accountId: 'live' })), + restoreSnapshot: vi.fn(), + onAuthFailure: vi.fn(() => () => undefined), + onTokenRefresh: vi.fn(() => () => undefined), + hasAccountCapability: vi.fn(() => false), + request: vi.fn(async () => { throw new Error('not mocked'); }), + accountId: 'acc-1', + currentSession: null, + }, + AuthenticationError: class AuthenticationError extends Error { + constructor(msg: string) { super(msg); this.name = 'AuthenticationError'; } + }, + NetworkError: class NetworkError extends Error { + constructor(msg: string) { super(msg); this.name = 'NetworkError'; } + }, + SessionSupersededError: class SessionSupersededError extends Error {}, +})); + +vi.mock('../../lib/push-notifications', () => ({ + teardownPushNotifications: vi.fn(async () => undefined), + teardownPushNotificationsForAccount: vi.fn(async () => undefined), +})); + +import { AuthenticationError, jmapClient } from '../../api/jmap-client'; +import { useAuthStore } from '../auth-store'; +import { useAccountStore, type AccountEntry } from '../account-store'; +import { reauthStart } from '../../screens/login/reauth-start'; + +const loadAccount = jmapClient.loadAccount as ReturnType; +const clearCredentials = jmapClient.clearAccountCredentials as ReturnType; + +const A = 'usera@example.org@mail.example.org'; +const B = 'userb@example.org@mail.example.org'; + +function entry(id: string, username: string): AccountEntry { + return { + id, username, serverUrl: 'https://mail.example.org', displayName: username, email: username, + avatarColor: '#000', lastLoginAt: 1, isConnected: false, hasError: false, isDefault: id === A, + }; +} + +beforeEach(() => { + vi.clearAllMocks(); + stored.creds = { serverUrl: 'https://mail.example.org', username: 'usera@example.org', password: 'old' }; + useAccountStore.setState({ + accounts: [entry(A, 'usera@example.org'), entry(B, 'userb@example.org')], + activeAccountId: A, + defaultAccountId: A, + }); + useAuthStore.setState({ + isAuthenticated: false, isLoading: false, hasRestoredSession: false, error: null, + serverUrl: null, username: null, session: null, accountId: null, activeAccountId: null, client: null, + reauth: null, + }); +}); + +describe('a refused stored sign-in', () => { + it('keeps the account on a cold start and asks for a new sign-in to it', async () => { + loadAccount.mockRejectedValue(new AuthenticationError('Session expired')); + + await useAuthStore.getState().restoreSession(); + + const account = useAccountStore.getState().getAccountById(A); + expect(account).toBeDefined(); + expect(account?.needsSignIn).toBe(true); + expect(clearCredentials).not.toHaveBeenCalled(); + const state = useAuthStore.getState(); + expect(state.error).toBeNull(); + expect(state.isAuthenticated).toBe(true); + expect(state.reauth).toEqual({ + accountId: A, serverUrl: 'https://mail.example.org', username: 'usera@example.org', method: 'password', + }); + }); + + it('asks a pasted-token account for a new token', async () => { + stored.creds = { serverUrl: 'https://mail.example.org', username: 'usera@example.org', password: '', accessToken: 'tok' }; + loadAccount.mockRejectedValue(new AuthenticationError('Session expired')); + + await useAuthStore.getState().restoreSession(); + + const reauth = useAuthStore.getState().reauth; + expect(reauth?.method).toBe('token'); + expect(reauthStart(reauth)).toEqual({ step: 'password', email: '', serverUrl: 'https://mail.example.org', tokenMode: true }); + }); + + it('keeps an account switched to and goes back to the one in use', async () => { + useAuthStore.setState({ isAuthenticated: true, session: { apiUrl: 'x' } as never, activeAccountId: A }); + loadAccount.mockRejectedValue(new AuthenticationError('Session expired')); + + await useAuthStore.getState().switchAccount(B); + + expect(useAccountStore.getState().getAccountById(B)?.needsSignIn).toBe(true); + expect(clearCredentials).not.toHaveBeenCalled(); + expect(jmapClient.restoreSnapshot).toHaveBeenCalled(); + expect(useAuthStore.getState().reauth?.accountId).toBe(B); + expect(useAuthStore.getState().error).toBeNull(); + }); +}); + +describe('reauthStart', () => { + it('opens a password account on the form and an OAuth one on its sign-in page, filled in', () => { + expect(reauthStart({ accountId: A, serverUrl: 'https://m', username: 'u@x', method: 'password' })) + .toEqual({ step: 'password', email: 'u@x', serverUrl: 'https://m', tokenMode: false }); + expect(reauthStart({ accountId: A, serverUrl: 'https://m', username: 'u@x', method: 'oauth' })) + .toEqual({ step: 'confirm', email: 'u@x', serverUrl: 'https://m', tokenMode: false }); + expect(reauthStart(null).step).toBe('choose'); + }); +}); diff --git a/src/stores/account-store.ts b/src/stores/account-store.ts index e10a3110..cf871cd3 100644 --- a/src/stores/account-store.ts +++ b/src/stores/account-store.ts @@ -15,6 +15,10 @@ export interface AccountEntry { isConnected: boolean; hasError: boolean; errorMessage?: string; + // The server refused the stored sign-in (a password changed, a token that + // expired, a server moved to Stalwart 1.0): the account stays, and the + // user is asked to sign in to it again. + needsSignIn?: boolean; isDefault: boolean; // JMAP primary account id (from the session's primaryAccounts). Recorded // when push is set up so a relay payload can be routed to this entry. @@ -67,6 +71,8 @@ export const useAccountStore = create()( isConnected: entry.isConnected, hasError: entry.hasError, errorMessage: entry.errorMessage, + // Signed in again. + needsSignIn: false, } : a, ), diff --git a/src/stores/auth-store.ts b/src/stores/auth-store.ts index 69d80591..9b599234 100644 --- a/src/stores/auth-store.ts +++ b/src/stores/auth-store.ts @@ -9,7 +9,7 @@ import { } from '../api/jmap-client'; import type { JMAPSession } from '../api/types'; import { fetchAccountDisplayName, isStalwartSupported } from '../api/account-security'; -import { useAccountStore } from './account-store'; +import { useAccountStore, type AccountEntry } from './account-store'; import { useEmailStore } from './email-store'; import { useContactsStore } from './contacts-store'; import { useCalendarStore } from './calendar-store'; @@ -77,6 +77,20 @@ async function waitForHydration(store: { }); } +/** + * An account whose stored sign-in the server refused, to be signed in to + * again: the sign-in screen opens on it with the server and the username + * filled in. The method is how it signed in: a pasted access token is asked + * for again, a password account goes to the password form, an OAuth one to + * the server's sign-in page. + */ +export interface ReauthTarget { + accountId: string; + serverUrl: string; + username: string; + method: 'password' | 'oauth' | 'token'; +} + export interface AuthState { isAuthenticated: boolean; isLoading: boolean; @@ -94,6 +108,8 @@ export interface AuthState { * on the next successful sign-in. */ pendingTotpLogin: { serverUrl: string; username: string; password: string } | null; + /** The account to sign in to again, set when its stored sign-in was refused. */ + reauth: ReauthTarget | null; login: ( serverUrl: string, @@ -115,6 +131,9 @@ export interface AuthState { restoreSession: () => Promise; retrySession: () => Promise; clearError: () => void; + /** Ask for a new sign-in to an account (sets reauth). */ + requestReauth: (accountId: string) => Promise; + clearReauth: () => void; } // The messages the viewer read, their rendered documents and their heights, @@ -216,6 +235,34 @@ function followStoredServer(accountId: string): void { if (served && entry && entry.serverUrl !== served) accountStore.updateAccount(accountId, { serverUrl: served }); } +// The message an account the server refused carries in the registry (the +// account rows translate it from needsSignIn). +const SIGN_IN_AGAIN = 'Sign in again'; + +/** + * The server refused an account's stored sign-in (401: the password was + * changed, a pasted token expired, the server moved from Stalwart 0.16 to + * 1.0 and lost the grant). The account stays, with its caches and settings, + * and is marked for a new sign-in; it used to be removed with "Your session + * has expired", which lost everything kept for it. Its credentials stay too + * until the new sign-in replaces them. Null when the account is gone. + */ +async function keepForSignIn(accountId: string): Promise { + const entry = useAccountStore.getState().getAccountById(accountId); + if (!entry) return null; + const creds = await jmapClient.getStoredCredentials(accountId).catch(() => null); + const method: ReauthTarget['method'] = creds?.accessToken ? (creds.tokenEndpoint ? 'oauth' : 'token') : 'password'; + if (useAccountStore.getState().getAccountById(accountId)) { + useAccountStore.getState().updateAccount(accountId, { + isConnected: false, + hasError: true, + errorMessage: SIGN_IN_AGAIN, + needsSignIn: true, + }); + } + return { accountId, serverUrl: entry.serverUrl, username: creds?.username || entry.username, method }; +} + // The outbox replays against whatever account the JMAP client serves, and a // replaced server may have given the queued ids to other messages. So it // runs only once the account's session is up and its server was checked. @@ -551,6 +598,7 @@ function applyConnectedState( accountId: jmapClient.accountId, activeAccountId: accountId, client: jmapClient, + reauth: null, }); } @@ -566,6 +614,7 @@ export const useAuthStore = create((set, get) => ({ activeAccountId: null, client: null, pendingTotpLogin: null, + reauth: null, login: async (serverUrl, typedUsername, password, opts) => { set({ isLoading: true, error: null }); @@ -824,16 +873,10 @@ export const useAuthStore = create((set, get) => ({ // afresh each time: the state taken above still lists the account just // removed, and signing out of the last one "switched" to it, which did // nothing, and left the app on an empty mail view. - for (let tries = useAccountStore.getState().accounts.length; tries > 0; tries--) { - const registry = useAccountStore.getState(); - const next = registry.getDefaultAccount() ?? registry.accounts[0]; - if (!next) break; - await get().switchAccount(next.id); - // Landed, or another switch has begun and decides. - if (get().activeAccountId === next.id || pendingSwitch) return; - if (!useAccountStore.getState().getAccountById(next.id)) continue; // its sign-in was refused - // Its server did not answer: show what is cached for it, as a cold - // start does, and let the session retry bring it up. + // Its server did not answer, or refused its stored sign-in: show what is + // cached for it, as a cold start does. The session retry brings an + // unreachable one up; a refused one asks for a new sign-in. + const showCached = (next: AccountEntry, reauth: ReauthTarget | null) => { useAccountStore.getState().setActiveAccount(next.id); useEmailStore.getState().setActiveAccount(next.id); set({ @@ -846,7 +889,35 @@ export const useAuthStore = create((set, get) => ({ accountId: null, activeAccountId: next.id, client: jmapClient, + reauth, }); + }; + // An account whose sign-in is refused stays for a new sign-in + // (keepForSignIn) and the next one is tried; it opens only when no + // other account comes up. + let refused: { account: AccountEntry; reauth: ReauthTarget | null } | null = null; + const tried = new Set(); + for (let tries = useAccountStore.getState().accounts.length; tries > 0; tries--) { + const registry = useAccountStore.getState(); + const next = [registry.getDefaultAccount(), ...registry.accounts] + .find((a): a is AccountEntry => !!a && !tried.has(a.id)); + if (!next) break; + tried.add(next.id); + await get().switchAccount(next.id); + // Landed, or another switch has begun and decides. + if (get().activeAccountId === next.id || pendingSwitch) return; + const after = useAccountStore.getState().getAccountById(next.id); + if (!after) continue; + if (after.needsSignIn) { + refused ??= { account: after, reauth: get().reauth }; + set({ reauth: null }); + continue; + } + showCached(after, null); + return; + } + if (refused) { + showCached(refused.account, refused.reauth); return; } @@ -978,13 +1049,12 @@ export const useAuthStore = create((set, get) => ({ } catch (err) { if (!isCurrent() || err instanceof SessionSupersededError) return; if (err instanceof AuthenticationError) { - await jmapClient.clearAccountCredentials(accountId).catch(() => undefined); + // Back to the account the switch left, and ask for a new sign-in + // to this one (see keepForSignIn). + const reauth = await keepForSignIn(accountId); if (!isCurrent()) return; - accountStore.removeAccount(accountId); - useEmailStore.getState().removeAccount(accountId); restorePrevious(); - set({ isLoading: false, error: 'Session expired for this account' }); - await forgetRemovedAccount(accountId); + set({ isLoading: false, error: null, reauth }); return; } // NetworkError or anything else - keep the previous active account @@ -1015,6 +1085,7 @@ export const useAuthStore = create((set, get) => ({ isConnected: true, hasError: false, errorMessage: undefined, + needsSignIn: false, lastLoginAt: Date.now(), }); applyConnectedState(set, session, target.serverUrl, target.username, accountId); @@ -1134,13 +1205,25 @@ export const useAuthStore = create((set, get) => ({ return true; } if (err instanceof AuthenticationError) { - // Server reachable but credentials rejected — drop them. - await jmapClient.clearAccountCredentials(target.id).catch(() => undefined); - accountStore.removeAccount(target.id); - useEmailStore.getState().removeAccount(target.id); - await forgetRemovedAccount(target.id); - set({ isLoading: false, hasRestoredSession: true, error: 'Session expired' }); - return false; + // Server reachable but the stored sign-in refused: keep the account + // and what is cached for it, as for an unreachable server, and ask + // for a new sign-in (see keepForSignIn). + const reauth = await keepForSignIn(target.id); + accountStore.setActiveAccount(target.id); + set({ + isAuthenticated: true, + isLoading: false, + hasRestoredSession: true, + error: null, + serverUrl: target.serverUrl, + username: target.username, + session: null, + accountId: null, + activeAccountId: target.id, + client: jmapClient, + reauth, + }); + return true; } throw err; } @@ -1150,6 +1233,7 @@ export const useAuthStore = create((set, get) => ({ isConnected: true, hasError: false, errorMessage: undefined, + needsSignIn: false, }); const session = jmapClient.currentSession!; @@ -1189,6 +1273,8 @@ export const useAuthStore = create((set, get) => ({ const accountStore = useAccountStore.getState(); const target = accountStore.getAccountById(activeAccountId); if (!target) return false; + // The server refused its sign-in: only a new one brings it back. + if (target.needsSignIn) return false; try { const ok = await jmapClient.loadAccount(activeAccountId); if (!ok || !isCurrent()) return false; @@ -1196,6 +1282,7 @@ export const useAuthStore = create((set, get) => ({ isConnected: true, hasError: false, errorMessage: undefined, + needsSignIn: false, }); const fresh = jmapClient.currentSession!; followStoredServer(activeAccountId); @@ -1207,22 +1294,10 @@ export const useAuthStore = create((set, get) => ({ } catch (err) { if (!isCurrent() || err instanceof SessionSupersededError) return false; if (err instanceof AuthenticationError) { - // Now we know the credentials are bad — fall back to logout flow. - await jmapClient.clearAccountCredentials(activeAccountId).catch(() => undefined); - accountStore.removeAccount(activeAccountId); - await forgetRemovedAccount(activeAccountId); - set({ - isAuthenticated: false, - isLoading: false, - hasRestoredSession: true, - error: 'Session expired', - serverUrl: null, - username: null, - session: null, - accountId: null, - activeAccountId: null, - client: null, - }); + // Now we know the stored sign-in is refused: the account stays on + // what is cached for it and asks for a new sign-in (keepForSignIn). + const reauth = await keepForSignIn(activeAccountId); + if (isCurrent()) set({ isLoading: false, error: null, reauth }); } // NetworkError or anything else: stay where we are. return false; @@ -1230,6 +1305,13 @@ export const useAuthStore = create((set, get) => ({ }, clearError: () => set({ error: null }), + + requestReauth: async (accountId) => { + const reauth = await keepForSignIn(accountId); + if (reauth) set({ reauth }); + }, + + clearReauth: () => set({ reauth: null }), })); // A 401 on a live session (revoked password/token, expired refresh token) From 149e5bcee5fd68143bed52a2fd8ada149b4c1883 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:42:37 +0100 Subject: [PATCH 5/7] fix: ask to sign in again when a password change ends the session --- locales/rn/en.json | 2 + .../settings/AccountSecuritySettings.tsx | 44 +++++++++++++++---- .../__tests__/password-change.test.ts | 24 ++++++++++ src/components/settings/password-change.ts | 24 ++++++++++ 4 files changed, 85 insertions(+), 9 deletions(-) create mode 100644 src/components/settings/__tests__/password-change.test.ts create mode 100644 src/components/settings/password-change.ts diff --git a/locales/rn/en.json b/locales/rn/en.json index 727d039c..5bad7fe9 100644 --- a/locales/rn/en.json +++ b/locales/rn/en.json @@ -1490,6 +1490,8 @@ "description": "Change the password for this account.", "error": "Failed to change password.", "mismatch": "Passwords do not match.", + "session_ended": "Changing the password ended this session. Sign in again with your new password.", + "session_ended_title": "Sign in again", "toggle_visibility": "Show or hide password", "too_short": "New password must be at least 8 characters." }, diff --git a/src/components/settings/AccountSecuritySettings.tsx b/src/components/settings/AccountSecuritySettings.tsx index 88ee9544..7c2d4185 100644 --- a/src/components/settings/AccountSecuritySettings.tsx +++ b/src/components/settings/AccountSecuritySettings.tsx @@ -18,6 +18,8 @@ import { pickClientCertAlias, } from '../../lib/client-cert'; import { useLocaleStore } from '../../stores/locale-store'; +import { useAuthStore } from '../../stores/auth-store'; +import { passwordChangeOutcome } from './password-change'; import { AccountSecurityForbiddenError, isLimitedStalwartSignIn, @@ -155,21 +157,45 @@ function PasswordChangeSection({ otpEnabled }: { otpEnabled: boolean }) { if (newPwd.length < 8) { setError(t('settings.security.password.too_short', "New password must be at least 8 characters.")); return; } if (newPwd !== confirmPwd) { setError(t('settings.security.password.mismatch', "Passwords do not match.")); return; } setSaving(true); + const tokenSignIn = jmapClient.usesBearerAuth; + let failure: unknown = null; try { - const tokenSignIn = jmapClient.usesBearerAuth; await changePassword(currentPwd, newPwd, otpEnabled ? otpCode : undefined); - setCurrentPwd(''); setOtpCode(''); setNewPwd(''); setConfirmPwd(''); - Alert.alert( - t('settings.security.password.changed_title', "Password changed"), - tokenSignIn - ? t('settings.security.password.changed_sign_in_again', "Your account password was updated. Sign in again with the new password.") - : t('settings.security.password.changed', "Your account password was updated."), - ); } catch (err) { - setError(err instanceof Error ? err.message : t('settings.security.password.error', "Failed to change password.")); + failure = err ?? new Error('Failed to change password'); } finally { setSaving(false); } + const outcome = passwordChangeOutcome(failure, tokenSignIn); + if (outcome.kind === 'error') { + setError(outcome.message || t('settings.security.password.error', "Failed to change password.")); + return; + } + setCurrentPwd(''); setOtpCode(''); setNewPwd(''); setConfirmPwd(''); + if (outcome.kind === 'changed') { + Alert.alert( + t('settings.security.password.changed_title', "Password changed"), + t('settings.security.password.changed', "Your account password was updated."), + ); + return; + } + // The change ended the session (see password-change): say so, then open + // the sign-in to this account, filled in, instead of letting the next + // request fail with a generic error. + const accountId = useAuthStore.getState().activeAccountId; + Alert.alert( + outcome.changed + ? t('settings.security.password.changed_title', "Password changed") + : t('settings.security.password.session_ended_title', "Sign in again"), + outcome.changed + ? t('settings.security.password.changed_sign_in_again', "Your account password was updated. Sign in again with the new password.") + : t('settings.security.password.session_ended', "Changing the password ended this session. Sign in again with your new password."), + [{ + text: t('login.mobile.continue', 'Continue'), + onPress: () => { if (accountId) void useAuthStore.getState().requestReauth(accountId); }, + }], + { cancelable: false }, + ); }; return ( diff --git a/src/components/settings/__tests__/password-change.test.ts b/src/components/settings/__tests__/password-change.test.ts new file mode 100644 index 00000000..73088cca --- /dev/null +++ b/src/components/settings/__tests__/password-change.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from 'vitest'; +import { AuthenticationError } from '../../../api/jmap-client'; +import { passwordChangeOutcome } from '../password-change'; + +// A password change on Stalwart revokes the session's tokens (0.16 with +// TOTP: the sign-in runs on OAuth tokens). The page says "sign in again" +// instead of showing the 401 as a failed change. + +describe('passwordChangeOutcome', () => { + it('asks a token session to sign in again after the change', () => { + expect(passwordChangeOutcome(null, true)).toEqual({ kind: 'sign-in-again', changed: true }); + expect(passwordChangeOutcome(null, false)).toEqual({ kind: 'changed' }); + }); + + it('takes a 401 on the change for the session it ended, not a failure', () => { + expect(passwordChangeOutcome(new AuthenticationError('Session expired'), true)) + .toEqual({ kind: 'sign-in-again', changed: false }); + }); + + it('shows what the server refused', () => { + expect(passwordChangeOutcome(new Error('Current secret is incorrect.'), true)) + .toEqual({ kind: 'error', message: 'Current secret is incorrect.' }); + }); +}); diff --git a/src/components/settings/password-change.ts b/src/components/settings/password-change.ts new file mode 100644 index 00000000..2580b0b1 --- /dev/null +++ b/src/components/settings/password-change.ts @@ -0,0 +1,24 @@ +/** + * What the Security page says after a password change. Stalwart revokes the + * account's tokens with the old password, refresh token included (webmail + * #1432, checked on 0.16.27), so a session on tokens (a password plus TOTP + * sign-in, OAuth) ends with the change. That is told as "sign in again with + * the new password", and the app opens the sign-in to the account. A 401 on + * the change itself is the same ending seen from the other side: the server + * dropped the session as it changed the password. Anything else is an error + * the form shows. + */ +export type PasswordChangeOutcome = + | { kind: 'changed' } + | { kind: 'sign-in-again'; changed: boolean } + | { kind: 'error'; message: string | null }; + +export function passwordChangeOutcome(error: unknown, tokenSignIn: boolean): PasswordChangeOutcome { + if (error === null || error === undefined) { + return tokenSignIn ? { kind: 'sign-in-again', changed: true } : { kind: 'changed' }; + } + if (error instanceof Error && error.name === 'AuthenticationError') { + return { kind: 'sign-in-again', changed: false }; + } + return { kind: 'error', message: error instanceof Error ? error.message : null }; +} From 2fe83669383110b67cc78c81c152858126d2e4d4 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:47:05 +0100 Subject: [PATCH 6/7] fix: let the notification Delete skip the unlock when the mail goes to Trash #85 --- .../__tests__/push-background-task.test.ts | 45 ++++++++++++++++--- src/lib/push-background-task.ts | 28 ++++++++++-- 2 files changed, 63 insertions(+), 10 deletions(-) diff --git a/src/lib/__tests__/push-background-task.test.ts b/src/lib/__tests__/push-background-task.test.ts index c90a53a6..6f3bf43e 100644 --- a/src/lib/__tests__/push-background-task.test.ts +++ b/src/lib/__tests__/push-background-task.test.ts @@ -218,6 +218,39 @@ describe('pushBackgroundTask notifications', () => { ); }); + // #85: Delete only moves to Trash, so with a Trash it needs no unlock and + // the shade stays open; without one the unlock stays. + it.each([ + ['without unlocking when the account has a Trash', [{ id: 'tr', role: 'trash' }], false], + ['behind the unlock when the account has no Trash', [{ id: 'in', role: 'inbox' }], true], + ])('offers Delete %s', async (_label, mailboxes, needsUnlock) => { + (secureFetch as ReturnType).mockImplementation(async (url: string, init?: { body?: string }) => { + if (url.endsWith('/.well-known/jmap')) { + return { + ok: true, + json: async () => ({ + apiUrl: 'https://mail.example.com/jmap/', + primaryAccounts: { 'urn:ietf:params:jmap:mail': 'jmap-primary' }, + accounts: { 'jmap-primary': {} }, + }), + }; + } + const calls = JSON.parse(init?.body ?? '{}').methodCalls as Array<[string, Record, string]>; + return { + ok: true, + json: async () => ({ + methodResponses: calls.map(([name, , id]) => (name === 'Mailbox/get' + ? [name, { list: mailboxes }, id] + : [name, { list: [{ id: 'm1', threadId: 't1', keywords: {}, subject: 'Hi', from: [{ email: 'bob@example.com' }] }] }, id])), + }), + }; + }); + + await pushBackgroundTask({ kind: 'jmap-email-push', accountLabel: 'alice', accountId: 'jmap-primary', emailIds: '["m1"]' }); + + expect(showNotification).toHaveBeenCalledWith(expect.objectContaining({ emailId: 'm1', deleteNeedsUnlock: needsUnlock })); + }); + it('shows a generic notification when the lookup fails, not none (#1041)', async () => { (secureFetch as ReturnType).mockImplementation(async (url: string) => { if (url.endsWith('/.well-known/jmap')) return { ok: false, status: 503 }; @@ -693,14 +726,14 @@ describe('pushBackgroundTask keeps the push subscription alive', () => { await recorded(2 * DAY); await push(); expect(showNotification).toHaveBeenCalled(); - expect(server.calls).toEqual(['Email/get', 'PushSubscription/get', 'PushSubscription/set', 'PushSubscription/get']); + expect(server.calls).toEqual(['Email/get', 'Mailbox/get', 'PushSubscription/get', 'PushSubscription/set', 'PushSubscription/get']); expect(await AsyncStorage.getItem(`push:subscriptionExpires:v1:${LOCAL}`)).toBe(server.expires); // Recorded now: the next pushes leave it alone for a day. server.calls = []; await AsyncStorage.multiRemove([`push:notifiedIds:v1:${LOCAL}`, `push:lastNotifiedEmailId:v2:${LOCAL}`]); await push(); - expect(server.calls).toEqual(['Email/get']); + expect(server.calls).toEqual(['Email/get', 'Mailbox/get']); }); it('extends it also when the push brings nothing new to show', async () => { @@ -721,7 +754,7 @@ describe('pushBackgroundTask keeps the push subscription alive', () => { await AsyncStorage.multiRemove([`push:notifiedIds:v1:${LOCAL}`, `push:lastNotifiedEmailId:v2:${LOCAL}`]); await setup(); await push(); - expect(server.calls).toEqual(['Email/get', 'PushSubscription/get', 'PushSubscription/set', 'PushSubscription/get']); + expect(server.calls).toEqual(['Email/get', 'Mailbox/get', 'PushSubscription/get', 'PushSubscription/set', 'PushSubscription/get']); expect(await AsyncStorage.getItem(`push:subscriptionExpires:v1:${LOCAL}`)).toBe(server.expires); // The server's copy has a week left: recorded, not written. @@ -730,7 +763,7 @@ describe('pushBackgroundTask keeps the push subscription alive', () => { await AsyncStorage.multiRemove([`push:notifiedIds:v1:${LOCAL}`, `push:lastNotifiedEmailId:v2:${LOCAL}`]); await setup(); await push(); - expect(server.calls).toEqual(['Email/get', 'PushSubscription/get']); + expect(server.calls).toEqual(['Email/get', 'Mailbox/get', 'PushSubscription/get']); expect(await AsyncStorage.getItem(`push:subscriptionExpires:v1:${LOCAL}`)).toBe(serverExpires); } }); @@ -748,7 +781,7 @@ describe('pushBackgroundTask keeps the push subscription alive', () => { await AsyncStorage.removeItem(`push:optedOut:v1:${LOCAL}`); await setup(); await push(); - expect(server.calls).toEqual(['Email/get']); + expect(server.calls).toEqual(['Email/get', 'Mailbox/get']); } }); @@ -756,7 +789,7 @@ describe('pushBackgroundTask keeps the push subscription alive', () => { await recorded(2 * DAY); server.deviceClientId = 'dcid-someone-else'; await push(); - expect(server.calls).toEqual(['Email/get', 'PushSubscription/get']); + expect(server.calls).toEqual(['Email/get', 'Mailbox/get', 'PushSubscription/get']); }); it('still notifies when the server cannot be asked about the subscription', async () => { diff --git a/src/lib/push-background-task.ts b/src/lib/push-background-task.ts index 86f22ea2..eeb5e238 100644 --- a/src/lib/push-background-task.ts +++ b/src/lib/push-background-task.ts @@ -128,6 +128,11 @@ interface ShowNotificationOptions { groupTitle: string; markReadLabel?: string; deleteLabel?: string; + // Whether Delete asks to unlock the phone first (#85). Delete only ever + // moves the message to Trash; with a Trash to move it to, nothing is lost + // and the action runs from the lock screen and the open shade. Without + // one, or when that is not known, the unlock stays. + deleteNeedsUnlock?: boolean; replyLabel?: string; } @@ -374,16 +379,29 @@ async function jmapPost( return body.methodResponses ?? []; } +/** + * The messages to notify, and whether their account has a Trash for the + * notification's Delete to move them to (asked in the same request). An + * answer without the folders counts as no Trash. + */ async function detachedGetEmails( session: DetachedSession, accountId: string, ids: string[], -): Promise { +): Promise<{ emails: Email[]; hasTrash: boolean }> { const responses = await jmapPost(session, [ ['Email/get', { accountId, ids, properties: EMAIL_PROPERTIES }, '0'], + ['Mailbox/get', { accountId, properties: ['id', 'role'] }, '1'], ]); const body = methodResult(responses, 'Email/get'); - return (body.list as Email[]) ?? []; + return { emails: (body.list as Email[]) ?? [], hasTrash: answerHasTrash(responses) }; +} + +/** Whether the Mailbox/get answer (call id '1') lists a Trash. */ +export function answerHasTrash(responses: Array<[string, Record, string]>): boolean { + const answer = responses.find(([name, , id]) => name === 'Mailbox/get' && id === '1'); + const list = answer?.[1]?.list; + return Array.isArray(list) && list.some((m: Mailbox) => m?.role === 'trash'); } async function detachedNewestUnreadInboxIds( @@ -543,6 +561,7 @@ async function processAccountForPush(accountId: string, payload: RelayPushData, async function notifyForPush(accountId: string, session: DetachedSession, payload: RelayPushData): Promise { const alreadyNotified = await readNotifiedIds(accountId); let candidates: Email[]; + let hasTrash = false; let emailAccountId = session.jmapAccountId; if (payload.emailIds.length > 0) { @@ -552,7 +571,7 @@ async function notifyForPush(accountId: string, session: DetachedSession, payloa const fresh = payload.emailIds.filter((id) => !alreadyNotified.includes(id)); if (fresh.length === 0) return; emailAccountId = payload.jmapAccountId ?? session.jmapAccountId; - candidates = await detachedGetEmails(session, emailAccountId, fresh); + ({ emails: candidates, hasTrash } = await detachedGetEmails(session, emailAccountId, fresh)); } else { // Legacy `jmap-state-change` payload without ids: look at the newest // unread inbox messages and notify the ones not shown before. The ring @@ -561,7 +580,7 @@ async function notifyForPush(accountId: string, session: DetachedSession, payloa const ids = await detachedNewestUnreadInboxIds(session, LEGACY_QUERY_LIMIT); const fresh = ids.filter((id) => !alreadyNotified.includes(id)); if (fresh.length === 0) return; - candidates = await detachedGetEmails(session, session.jmapAccountId, fresh); + ({ emails: candidates, hasTrash } = await detachedGetEmails(session, session.jmapAccountId, fresh)); } const toNotify = selectNotifiableEmails(candidates, alreadyNotified); @@ -613,6 +632,7 @@ async function notifyForPush(accountId: string, session: DetachedSession, payloa groupTitle, markReadLabel, deleteLabel, + deleteNeedsUnlock: !hasTrash, replyLabel, }); } From 5a301d62dcc5e9110a99a8595e44e753c5134e08 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sat, 31 Oct 2026 13:55:47 +0100 Subject: [PATCH 7/7] feat: sign out of the identity provider too when signing out of a browser sign-in --- .../live-stalwart-end-session.test.ts | 57 +++++++++ src/api/jmap-client.ts | 6 + src/lib/__tests__/oauth-end-session.test.ts | 121 ++++++++++++++++++ src/lib/oauth-native.ts | 66 ++++++++++ src/lib/oauth.ts | 5 + src/lib/totp-login.ts | 10 ++ src/stores/auth-store.ts | 7 +- 7 files changed, 270 insertions(+), 2 deletions(-) create mode 100644 src/api/__tests__/live-stalwart-end-session.test.ts create mode 100644 src/lib/__tests__/oauth-end-session.test.ts diff --git a/src/api/__tests__/live-stalwart-end-session.test.ts b/src/api/__tests__/live-stalwart-end-session.test.ts new file mode 100644 index 00000000..9b322a7e --- /dev/null +++ b/src/api/__tests__/live-stalwart-end-session.test.ts @@ -0,0 +1,57 @@ +/** + * Live check of ending the provider's session at sign-out (OIDC + * RP-Initiated Logout) against a real Stalwart. Opt-in like the other live + * tests: + * + * BULWARK_LIVE_JMAP=http://10.10.10.62:8120 \ + * BULWARK_LIVE_USER=e2e-perf@example.org BULWARK_LIVE_PASS=… \ + * npx vitest run src/api/__tests__/live-stalwart-end-session.test.ts + * + * 1.0 announces an end_session_endpoint and hands out an id token with the + * tokens; 0.16 has neither, and sign-out opens nothing there. The grant the + * run makes is revoked again. + */ +import { describe, it, expect, vi } from 'vitest'; + +vi.mock('expo-secure-store', () => ({ + setItemAsync: vi.fn(async () => undefined), + getItemAsync: vi.fn(async () => null), + deleteItemAsync: vi.fn(async () => undefined), +})); + +import { buildEndSessionUrl, discoverOAuthMetadata, revokeRefreshToken } from '../../lib/oauth-native'; +import { exchangePasswordForTokens } from '../../lib/totp-login'; + +const SERVER = (process.env.BULWARK_LIVE_JMAP ?? '').replace(/\/+$/, ''); +const USER = process.env.BULWARK_LIVE_USER ?? ''; +const PASS = process.env.BULWARK_LIVE_PASS ?? ''; +const live = SERVER && USER && PASS ? describe : describe.skip; + +live('ending the provider session at sign-out (live)', () => { + it('logs out with the id token where the server announces an end_session_endpoint', async () => { + const metadata = await discoverOAuthMetadata(SERVER); + expect(metadata).not.toBeNull(); + const tokens = await exchangePasswordForTokens(SERVER, USER, PASS); + try { + if (!metadata!.end_session_endpoint) { + // 0.16: nothing to open. + expect(buildEndSessionUrl(SERVER, metadata!, tokens)).toBeNull(); + return; + } + // 1.0: the token answer carries an id token (a JWT), kept for the hint. + expect(tokens.idToken?.split('.')).toHaveLength(3); + const target = buildEndSessionUrl(SERVER, metadata!, tokens); + expect(target).not.toBeNull(); + const url = new URL(target!.url); + expect(url.searchParams.get('id_token_hint')).toBe(tokens.idToken); + const response = await fetch(target!.url, { redirect: 'manual' }); + // Signed out: a page saying so, or back to where the app asked. + expect([200, 302, 303]).toContain(response.status); + if (response.status !== 200) { + expect(response.headers.get('location')).toBe(url.searchParams.get('post_logout_redirect_uri')); + } + } finally { + await revokeRefreshToken(SERVER, tokens); + } + }); +}); diff --git a/src/api/jmap-client.ts b/src/api/jmap-client.ts index 880f93d9..0a8d2ebb 100644 --- a/src/api/jmap-client.ts +++ b/src/api/jmap-client.ts @@ -59,6 +59,9 @@ export interface StoredCredentials { tokenEndpoint?: string; clientId?: string; tokenSource?: OAuthTokenSource; + // The OIDC id token of a browser sign-in, for ending the provider's + // session at sign-out (lib/oauth-native endProviderSession). + idToken?: string; } /** @@ -404,6 +407,7 @@ export class JMAPClient { tokenEndpoint: tokens.tokenEndpoint, clientId: tokens.clientId, tokenSource: tokens.source, + ...(tokens.idToken ? { idToken: tokens.idToken } : {}), }; const fetched = await this.fetchSession(baseUrl); @@ -460,6 +464,7 @@ export class JMAPClient { expiresAt: next.expiresAt, tokenEndpoint: next.tokenEndpoint, clientId: next.clientId, + idToken: next.idToken ?? base.idToken, }; if (inUse) this.credentials = updated; await writeSharedSecret(credentialsKey(accountId), JSON.stringify(updated)); @@ -529,6 +534,7 @@ export class JMAPClient { tokenEndpoint: creds.tokenEndpoint, clientId: creds.clientId, source: creds.tokenSource, + ...(creds.idToken ? { idToken: creds.idToken } : {}), }; } diff --git a/src/lib/__tests__/oauth-end-session.test.ts b/src/lib/__tests__/oauth-end-session.test.ts new file mode 100644 index 00000000..3644131b --- /dev/null +++ b/src/lib/__tests__/oauth-end-session.test.ts @@ -0,0 +1,121 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import * as WebBrowser from 'expo-web-browser'; + +// Sign-out ends the identity provider's own session too (OIDC RP-Initiated +// Logout, webmail #905): revoking the refresh token left the browser signed +// in at the provider, and the next browser sign-in went through unasked. + +vi.mock('../client-cert', () => { + const fetchFn = vi.fn(); + return { + secureFetch: fetchFn, + fetchWithoutForeignRedirect: fetchFn, + followedOnlyAllowedRedirects: () => true, + mayFollowRedirect: () => true, + }; +}); + +import { fetchWithoutForeignRedirect } from '../client-cert'; +import { buildEndSessionUrl, endProviderSession, loginWithPkce, type OAuthMetadata } from '../oauth-native'; +import { refreshOAuthAccessToken, resetRefreshRotations, type OAuthTokens } from '../oauth'; +import { DEFAULT_CLIENT_ID, MOBILE_CLIENT_ID, REGISTERED_LOGOUT_REDIRECT_URI } from '../totp-login'; + +const fetchMock = fetchWithoutForeignRedirect as unknown as ReturnType; +const browser = WebBrowser.openAuthSessionAsync as ReturnType; + +const SERVER = 'https://mail.example.org'; +const DISCOVERY = { + issuer: SERVER, + authorization_endpoint: `${SERVER}/login`, + token_endpoint: `${SERVER}/auth/token`, + end_session_endpoint: `${SERVER}/auth/logout`, +}; +const json = (body: unknown) => ({ ok: true, status: 200, url: '', json: async () => body, text: async () => JSON.stringify(body) }); + +function tokens(over: Partial = {}): OAuthTokens { + return { + accessToken: 'at', refreshToken: 'rt', tokenEndpoint: `${SERVER}/auth/token`, + clientId: 'registered-client', source: 'native', idToken: 'id.token.sig', ...over, + }; +} + +beforeEach(() => { + vi.clearAllMocks(); + resetRefreshRotations(); + browser.mockResolvedValue({ type: 'dismiss' }); +}); + +describe('buildEndSessionUrl', () => { + it('names the client, hints the id token and comes back to the app for a client the app registered', () => { + const target = buildEndSessionUrl(SERVER, DISCOVERY, tokens()); + const url = new URL(target!.url); + expect(url.origin + url.pathname).toBe(`${SERVER}/auth/logout`); + expect(url.searchParams.get('client_id')).toBe('registered-client'); + expect(url.searchParams.get('id_token_hint')).toBe('id.token.sig'); + expect(url.searchParams.get('post_logout_redirect_uri')).toBe(REGISTERED_LOGOUT_REDIRECT_URI); + expect(target!.returnUrl).toBe(REGISTERED_LOGOUT_REDIRECT_URI); + }); + + it('sends no return address for a client someone else registered', () => { + for (const clientId of [DEFAULT_CLIENT_ID, MOBILE_CLIENT_ID]) { + const url = new URL(buildEndSessionUrl(SERVER, DISCOVERY, tokens({ clientId }))!.url); + expect(url.searchParams.get('post_logout_redirect_uri')).toBeNull(); + expect(url.searchParams.get('client_id')).toBe(clientId); + } + }); + + it('skips a provider without an end_session_endpoint, or one on another host in the clear', () => { + expect(buildEndSessionUrl(SERVER, { end_session_endpoint: undefined }, tokens())).toBeNull(); + expect(buildEndSessionUrl(SERVER, { end_session_endpoint: 'http://idp.example.net/logout' }, tokens())).toBeNull(); + // The server's own host over http is what the user signed in to. + expect(buildEndSessionUrl('http://10.10.10.62:8120', { end_session_endpoint: 'http://10.10.10.62:8120/auth/logout' }, tokens())) + .not.toBeNull(); + }); +}); + +describe('endProviderSession', () => { + it("opens the provider's logout page after a browser sign-in", async () => { + fetchMock.mockResolvedValue(json(DISCOVERY)); + await endProviderSession(SERVER, tokens()); + expect(browser).toHaveBeenCalledTimes(1); + const [url, returnUrl] = browser.mock.calls[0] as [string, string]; + expect(new URL(url).searchParams.get('id_token_hint')).toBe('id.token.sig'); + expect(returnUrl).toBe(REGISTERED_LOGOUT_REDIRECT_URI); + }); + + it('opens nothing for sign-ins that left no browser session, or a provider without logout', async () => { + fetchMock.mockResolvedValue(json(DISCOVERY)); + await endProviderSession(SERVER, tokens({ source: 'totp' })); + await endProviderSession(SERVER, tokens({ source: 'pairing' })); + fetchMock.mockResolvedValue(json({ ...DISCOVERY, end_session_endpoint: undefined })); + await endProviderSession(SERVER, tokens()); + expect(browser).not.toHaveBeenCalled(); + }); + + it('never fails the sign-out', async () => { + fetchMock.mockRejectedValue(new Error('offline')); + await expect(endProviderSession(SERVER, tokens())).resolves.toBeUndefined(); + fetchMock.mockResolvedValue(json(DISCOVERY)); + browser.mockRejectedValueOnce(new Error('another session is open')); + await expect(endProviderSession(SERVER, tokens())).resolves.toBeUndefined(); + }); +}); + +describe('the id token', () => { + it('is kept from the browser sign-in', async () => { + const meta: OAuthMetadata = { authorization_endpoint: `${SERVER}/authorize`, token_endpoint: `${SERVER}/auth/token` }; + browser.mockImplementationOnce(async (authUrl: string) => { + const state = new URL(authUrl).searchParams.get('state'); + return { type: 'success', url: `bulwarkmobile://auth/callback?code=c&state=${state}` }; + }); + fetchMock.mockResolvedValue(json({ access_token: 'at', refresh_token: 'rt', expires_in: 300, id_token: 'id.token.sig' })); + const got = await loginWithPkce(SERVER, meta, { clientId: 'c1' }); + expect(got.idToken).toBe('id.token.sig'); + }); + + it('outlives a refresh that hands out none', async () => { + fetchMock.mockResolvedValue(json({ access_token: 'at2', refresh_token: 'rt2', expires_in: 300 })); + const next = await refreshOAuthAccessToken(tokens()); + expect(next.idToken).toBe('id.token.sig'); + }); +}); diff --git a/src/lib/oauth-native.ts b/src/lib/oauth-native.ts index 9b8ed8dd..3a0a2e3b 100644 --- a/src/lib/oauth-native.ts +++ b/src/lib/oauth-native.ts @@ -18,6 +18,9 @@ import { generateCodeVerifier, resolveLoginClient, DEFAULT_CLIENT_ID, + MOBILE_CLIENT_ID, + REGISTERED_LOGOUT_REDIRECT_URI, + REGISTERED_REDIRECT_URI, } from './totp-login'; import { randomHex } from './random'; import { NetworkError } from '../api/jmap-client'; @@ -27,6 +30,7 @@ export interface OAuthMetadata { authorization_endpoint: string; token_endpoint: string; revocation_endpoint?: string; + end_session_endpoint?: string; registration_endpoint?: string; scopes_supported?: string[]; prompt_values_supported?: string[]; @@ -264,6 +268,7 @@ export async function loginWithPkce( access_token?: string; refresh_token?: string; expires_in?: number; + id_token?: string; }; if (!data.access_token) throw new HandoffError('Token response missing access_token'); return { @@ -273,9 +278,70 @@ export async function loginWithPkce( tokenEndpoint: metadata.token_endpoint, clientId, source: 'native', + // Kept for ending the provider's session at sign-out (endProviderSession). + ...(typeof data.id_token === 'string' && data.id_token ? { idToken: data.id_token } : {}), }; } +/** + * The provider's logout URL for a sign-out (OIDC RP-Initiated Logout 1.0; + * webmail lib/oauth/end-session.ts buildEndSessionUrl), or null when the + * provider advertises no end_session_endpoint or an unusable one. + * + * `client_id` always goes along, `id_token_hint` when the sign-in's id token + * was kept. The browser carries the id token there, so the endpoint must be + * https, or on the mail server's own host, which the app already talks to in + * the clear when the user chose that. `post_logout_redirect_uri` is sent only + * for a client the app registered itself, with that URI on file: a provider + * refuses the whole logout for one it does not have. + */ +export function buildEndSessionUrl( + serverUrl: string, + metadata: Pick, + tokens: Pick, +): { url: string; returnUrl: string } | null { + const endpoint = metadata.end_session_endpoint; + if (typeof endpoint !== 'string' || !/^https?:\/\//i.test(endpoint)) return null; + if (!/^https:\/\//i.test(endpoint) && !sameHost(endpoint, serverUrl)) return null; + const registered = tokens.clientId !== DEFAULT_CLIENT_ID && tokens.clientId !== MOBILE_CLIENT_ID; + const params = new URLSearchParams({ client_id: tokens.clientId }); + if (tokens.idToken) params.set('id_token_hint', tokens.idToken); + if (registered) params.set('post_logout_redirect_uri', REGISTERED_LOGOUT_REDIRECT_URI); + return { + url: `${endpoint}${endpoint.includes('?') ? '&' : '?'}${params.toString()}`, + // The browser session closes when the provider sends it back to the app. + returnUrl: registered ? REGISTERED_LOGOUT_REDIRECT_URI : REGISTERED_REDIRECT_URI, + }; +} + +let endSessionQueue: Promise = Promise.resolve(); + +/** + * End the identity provider's own login session at sign-out, best effort. + * Revoking the refresh token ends the app's grant, but the session the + * provider keeps in the browser stays, and the next browser sign-in would go + * through without asking (webmail #905). Only a browser sign-in left one + * (source 'native'); the password and TOTP sign-ins and the webmail's + * hand-off and pairing did not, or own it. Opens the logout page in the + * auth browser session, one at a time. Never throws and is not awaited by + * the sign-out. + */ +export function endProviderSession(serverUrl: string, tokens: OAuthTokens): Promise { + if (tokens.source !== 'native') return Promise.resolve(); + const run = async () => { + try { + const metadata = await discoverOAuthMetadata(serverUrl); + const target = metadata ? buildEndSessionUrl(serverUrl, metadata, tokens) : null; + if (!target) return; + await WebBrowser.openAuthSessionAsync(target.url, target.returnUrl); + } catch { + // never block or fail a sign-out + } + }; + endSessionQueue = endSessionQueue.then(run, run); + return endSessionQueue; +} + /** * Best-effort RFC 7009 revocation of a refresh token on sign-out. Never * throws; the caller is already dropping the credentials. diff --git a/src/lib/oauth.ts b/src/lib/oauth.ts index bb2b772f..fb302726 100644 --- a/src/lib/oauth.ts +++ b/src/lib/oauth.ts @@ -26,6 +26,9 @@ export interface OAuthTokens { // the desktop's token, so sign-out leaves `pairing` bundles alone instead // of revoking them at the mail server; every other source is revoked. source?: OAuthTokenSource; + // The OIDC id token of the sign-in, kept for ending the provider's + // session at sign-out (id_token_hint). A refresh may hand out a new one. + idToken?: string; } export type HandoffResult = @@ -681,6 +684,7 @@ export async function refreshOAuthAccessToken(tokens: OAuthTokens): Promise) => void, opts?: Si // desktop's own refresh token, and newer ones hand out a separate grant whose // refresh token only the webmail's token proxy understands. The account's // chain also leaves the in-process rotation memory either way, so a stale copy -// refreshed after the sign-out can't pick up its tokens from there. +// refreshed after the sign-out can't pick up its tokens from there. A browser +// sign-in's provider session is ended too, in the background: the sign-out +// never waits for the logout page (see endProviderSession). async function revokeStoredRefreshToken(accountId: string): Promise { try { const entry = useAccountStore.getState().getAccountById(accountId); @@ -509,6 +511,7 @@ async function revokeStoredRefreshToken(accountId: string): Promise { forgetRefreshRotations(tokens?.refreshToken); if (!entry || !tokens || tokens.source === 'pairing') return; await revokeRefreshToken(entry.serverUrl, tokens); + void endProviderSession(entry.serverUrl, tokens); } catch { // never block sign-out }