From 3bf2c30c994e58d494a4fe8bf3d09688c876d665 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Thu, 24 Sep 2026 19:37:46 +0200 Subject: [PATCH] fix: stop asking for the Stalwart principal on every start and message open Every sign-in and app start read the account's full name with `x:Account/get`, and the first message opened (invitation or not) looked up the account aliases with it too. Stalwart refuses that method to everyone but admins, so for most users both were wasted requests that always came back `forbidden`, and the full name was never refreshed. - The full name now comes from `x:AccountSettings/get` (its `description`), which every user can read, like the webmail (#900). - The alias lookup only runs for a message that carries a calendar invitation (and the calendar). - A refusal of `x:Account/get` is remembered per account for the session, so it is asked at most once. --- src/api/account-security.ts | 31 +++++++ .../email/CalendarInvitationBanner.tsx | 7 +- .../__tests__/calendar-user-addresses.test.ts | 86 +++++++++++++++++++ src/lib/calendar-user-addresses.ts | 17 ++-- .../__tests__/auth-store-display-name.test.ts | 53 ++++++++++++ src/stores/auth-store.ts | 16 ++-- 6 files changed, 190 insertions(+), 20 deletions(-) create mode 100644 src/lib/__tests__/calendar-user-addresses.test.ts create mode 100644 src/stores/__tests__/auth-store-display-name.test.ts diff --git a/src/api/account-security.ts b/src/api/account-security.ts index 01e3bb01..e0e41447 100644 --- a/src/api/account-security.ts +++ b/src/api/account-security.ts @@ -284,9 +284,40 @@ export async function removePublicKey(id: string): Promise { if (failure) throw new Error(failure.description || failure.type || 'Failed to remove public key'); } +/** + * The account's "Full name": the `description` of `x:AccountSettings`, which + * every user can read (webmail #900). Null when it is not set. + */ +export async function fetchAccountDisplayName(): Promise { + const accountId = jmapClient.accountId; + const responses = await send([['x:AccountSettings/get', { accountId, ids: ['singleton'] }, '0']]); + const result = resultFor<{ list?: Array<{ description?: string | null }> }>(responses, '0'); + const description = result.list?.[0]?.description; + return typeof description === 'string' && description.trim() ? description.trim() : null; +} + +// Stalwart only lets admins read their own principal with x:Account/get; +// everyone else is refused, every time. A refusal is remembered per account +// for the session, so the alias lookups don't keep asking. +const PRINCIPAL_REFUSALS = new Set(['forbidden', 'unknownMethod', 'accountNotSupportedByMethod']); +const principalRefused = new Set(); + +function principalKey(accountId: string): string { + return `${jmapClient.serverUrl ?? ''}|${accountId}`; +} + +/** Test hook. */ +export function resetPrincipalRefusals(): void { + principalRefused.clear(); +} + export async function fetchPrincipal(): Promise { const accountId = jmapClient.accountId; + const key = principalKey(accountId); + if (principalRefused.has(key)) throw new Error('forbidden'); const responses = await send([['x:Account/get', { accountId, ids: [accountId] }, '0']]); + const refusal = responses.find((r) => r[2] === '0' && r[0] === 'error')?.[1] as { type?: string } | undefined; + if (refusal?.type && PRINCIPAL_REFUSALS.has(refusal.type)) principalRefused.add(key); const result = resultFor<{ list?: Array<{ description?: string | null; diff --git a/src/components/email/CalendarInvitationBanner.tsx b/src/components/email/CalendarInvitationBanner.tsx index ba47a99a..4479c36f 100644 --- a/src/components/email/CalendarInvitationBanner.tsx +++ b/src/components/email/CalendarInvitationBanner.tsx @@ -68,11 +68,10 @@ export function CalendarInvitationBanner({ email, jmapAccountId }: Props) { const subscriptions = useCalendarSubscriptionsStore((s) => s.subscriptions); const importEvents = useCalendarStore((s) => s.importEvents); const rsvpEvent = useCalendarStore((s) => s.rsvpEvent); - // Login address + identities + aliases, so invitations addressed to an - // alias still show the RSVP buttons. - const currentUserEmails = useUserCalendarAddresses(); - const attachment = React.useMemo(() => findCalendarAttachment(email), [email]); + // Login address + identities + aliases, so invitations addressed to an + // alias still show the RSVP buttons. Only an invitation looks the aliases up. + const currentUserEmails = useUserCalendarAddresses(!!attachment && enabled); const [state, setState] = React.useState('loading'); const [event, setEvent] = React.useState | null>(null); diff --git a/src/lib/__tests__/calendar-user-addresses.test.ts b/src/lib/__tests__/calendar-user-addresses.test.ts new file mode 100644 index 00000000..2812642a --- /dev/null +++ b/src/lib/__tests__/calendar-user-addresses.test.ts @@ -0,0 +1,86 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +// x:Account/get (the principal, for the account aliases) is refused to +// everyone but admins: it used to go out on every start and on the first +// message opened, invitation or not (PF6). + +const request = vi.fn(); +vi.mock('../../api/jmap-client', () => ({ + jmapClient: { + isConnected: true, + accountId: 'acc-1', + serverUrl: 'https://mail.example.com', + currentSession: { apiUrl: 'https://mail.example.com/jmap/' }, + hasAccountCapability: () => true, + request: (...args: unknown[]) => request(...args), + }, +})); + +// Run effects straight away, like a mount. +vi.mock('react', () => ({ + default: { + useEffect: (effect: () => void) => { effect(); }, + useReducer: () => [0, () => undefined], + useMemo: (fn: () => T) => fn(), + }, +})); + +vi.mock('../../stores/account-store', () => ({ + useAccountStore: (select: (s: unknown) => unknown) => select({ getActiveAccount: () => ({ email: 'me@example.com' }) }), +})); +vi.mock('../../stores/settings-store', () => ({ + useSettingsStore: (select: (s: unknown) => unknown) => select({ identities: [] }), +})); + +import { useUserCalendarAddresses, resetUserCalendarAddressCache } from '../calendar-user-addresses'; +import { fetchAccountDisplayName, fetchPrincipal, resetPrincipalRefusals } from '../../api/account-security'; + +const forbidden = { methodResponses: [['error', { type: 'forbidden' }, '0']] }; + +beforeEach(() => { + request.mockReset(); + resetUserCalendarAddressCache(); + resetPrincipalRefusals(); +}); + +describe('account aliases for calendar invitations', () => { + it('does not look the aliases up for a message without an invitation', () => { + expect(useUserCalendarAddresses(false)).toEqual(['me@example.com']); + expect(request).not.toHaveBeenCalled(); + }); + + it('asks once, and never again this session once refused', async () => { + request.mockResolvedValue(forbidden); + useUserCalendarAddresses(true); + await new Promise((r) => setTimeout(r, 0)); + expect(request).toHaveBeenCalledTimes(1); + expect(request.mock.calls[0][0]).toEqual([['x:Account/get', { accountId: 'acc-1', ids: ['acc-1'] }, '0']]); + // Every later view needing them (another invitation, the calendar) reuses that. + useUserCalendarAddresses(true); + + // Even with the alias cache gone, the refusal is remembered. + resetUserCalendarAddressCache(); + useUserCalendarAddresses(true); + await expect(fetchPrincipal()).rejects.toThrow('forbidden'); + expect(request).toHaveBeenCalledTimes(1); + }); + + it('keeps asking after a failure that is not a refusal', async () => { + request.mockRejectedValueOnce(new Error('offline')); + await expect(fetchPrincipal()).rejects.toThrow('offline'); + request.mockResolvedValueOnce({ methodResponses: [['x:Account/get', { list: [{ name: 'me@example.com', aliases: { a: { name: 'alias@example.com' } } }] }, '0']] }); + await expect(fetchPrincipal()).resolves.toMatchObject({ emails: ['me@example.com', 'alias@example.com'] }); + expect(request).toHaveBeenCalledTimes(2); + }); +}); + +describe('account display name', () => { + it('reads the full name from x:AccountSettings, which every user may read', async () => { + request.mockResolvedValue({ methodResponses: [['x:AccountSettings/get', { list: [{ description: ' Ada Lovelace ' }] }, '0']] }); + await expect(fetchAccountDisplayName()).resolves.toBe('Ada Lovelace'); + expect(request).toHaveBeenCalledWith( + [['x:AccountSettings/get', { accountId: 'acc-1', ids: ['singleton'] }, '0']], + expect.anything(), + ); + }); +}); diff --git a/src/lib/calendar-user-addresses.ts b/src/lib/calendar-user-addresses.ts index 0d751913..40e2ceaa 100644 --- a/src/lib/calendar-user-addresses.ts +++ b/src/lib/calendar-user-addresses.ts @@ -7,13 +7,14 @@ import { collectUserCalendarAddresses } from './calendar-participants'; // Account aliases come from x:Account/get (Stalwart's principal object) and // only change when an admin edits the account, so they're fetched once per -// JMAP account and remembered for the session. Failure (older server, no -// permission) simply leaves the list at login address + identities. +// JMAP account and remembered for the session, and only by a view that needs +// them. Failure (older server, no permission: only admins may read it) simply +// leaves the list at login address + identities. const aliasCache = new Map(); const aliasInFlight = new Map>(); const aliasListeners = new Set<() => void>(); -function loadAliases(accountId: string): Promise { +function fetchAliases(accountId: string): Promise { const cached = aliasCache.get(accountId); if (cached) return Promise.resolve(cached); const pending = aliasInFlight.get(accountId); @@ -42,20 +43,22 @@ export function resetUserCalendarAddressCache(): void { * login address, the sending identities and the account aliases. Used to * find "me" among an event's participants (RSVP), to detect alias-organized * events as the user's own, and as the organizer address of new invites. + * `loadAliases: false` skips the alias lookup (a message that carries no + * invitation). */ -export function useUserCalendarAddresses(): string[] { +export function useUserCalendarAddresses(loadAliases = true): string[] { const activeEmail = useAccountStore((s) => s.getActiveAccount()?.email ?? null); const identities = useSettingsStore((s) => s.identities); const [, bump] = React.useReducer((n: number) => n + 1, 0); const accountId = jmapClient.isConnected ? jmapClient.accountId : null; React.useEffect(() => { - if (!accountId) return; + if (!accountId || !loadAliases) return; if (aliasCache.has(accountId)) return; aliasListeners.add(bump); - void loadAliases(accountId); + void fetchAliases(accountId); return () => { aliasListeners.delete(bump); }; - }, [accountId]); + }, [accountId, loadAliases]); const aliases = accountId ? aliasCache.get(accountId) ?? [] : []; return React.useMemo( diff --git a/src/stores/__tests__/auth-store-display-name.test.ts b/src/stores/__tests__/auth-store-display-name.test.ts new file mode 100644 index 00000000..37fa186b --- /dev/null +++ b/src/stores/__tests__/auth-store-display-name.test.ts @@ -0,0 +1,53 @@ +import { describe, it, expect, vi } from 'vitest'; + +// The display-name sync on every sign-in and start read the Stalwart +// principal with x:Account/get, which is refused to everyone but admins +// (PF6). The full name lives in x:AccountSettings, which every user reads. + +const request = vi.fn(async (calls: Array<[string, Record, string]>) => ({ + methodResponses: calls.map(([name, , id]) => { + if (name === 'Identity/get') return [name, { list: [{ id: 'i1', name: 'Old Name', email: 'user@example.com' }] }, id]; + if (name === 'x:AccountSettings/get') return [name, { list: [{ description: 'Ada Lovelace' }] }, id]; + return ['error', { type: 'forbidden' }, id]; + }), +})); + +vi.mock('../../api/jmap-client', () => ({ + jmapClient: { + connect: vi.fn(async () => ({ apiUrl: 'https://mail.example.com/jmap/' })), + snapshot: vi.fn(() => ({ session: null, credentials: null, accountId: null })), + restoreSnapshot: vi.fn(), + onAuthFailure: vi.fn(() => () => undefined), + onTokenRefresh: vi.fn(() => () => undefined), + hasAccountCapability: vi.fn(() => true), + request: (calls: Array<[string, Record, string]>) => request(calls), + accountId: 'acc-1', + isConnected: true, + currentSession: { apiUrl: 'https://mail.example.com/jmap/' }, + username: 'user@example.com', + serverUrl: 'https://mail.example.com', + }, + AuthenticationError: class AuthenticationError extends Error {}, + NetworkError: class NetworkError extends Error {}, +})); + +vi.mock('../../lib/push-notifications', () => ({ + teardownPushNotifications: vi.fn(async () => undefined), + teardownPushNotificationsForAccount: vi.fn(async () => undefined), +})); + +import { useAuthStore } from '../auth-store'; +import { useAccountStore } from '../account-store'; + +describe('display name sync on sign-in', () => { + it('reads the full name without asking for the principal', async () => { + await useAuthStore.getState().login('https://mail.example.com', 'user@example.com', 'pass'); + + await vi.waitFor(() => { + expect(useAccountStore.getState().getActiveAccount()?.displayName).toBe('Ada Lovelace'); + }); + const methods = request.mock.calls.flatMap(([calls]) => calls.map(([name]) => name)); + expect(methods).toContain('x:AccountSettings/get'); + expect(methods).not.toContain('x:Account/get'); + }); +}); diff --git a/src/stores/auth-store.ts b/src/stores/auth-store.ts index 423c5ef7..8c157802 100644 --- a/src/stores/auth-store.ts +++ b/src/stores/auth-store.ts @@ -2,7 +2,7 @@ import { create } from 'zustand'; import { jmapClient, AuthenticationError, NetworkError } from '../api/jmap-client'; import type { JMAPSession } from '../api/types'; import { getIdentities } from '../api/identity'; -import { fetchPrincipal, isStalwartSupported } from '../api/account-security'; +import { fetchAccountDisplayName, isStalwartSupported } from '../api/account-security'; import { useAccountStore } from './account-store'; import { useEmailStore } from './email-store'; import { useContactsStore } from './contacts-store'; @@ -153,9 +153,10 @@ async function revokeStoredRefreshToken(accountId: string): Promise { } // Refresh the registry's display name / address from the server (#900): the -// primary identity first, then the Stalwart principal's "Full name" when the -// account advertises the extension. Fire-and-forget; a failure keeps whatever -// the registry already had. +// primary identity first, then the Stalwart account's "Full name" when the +// account advertises the extension. Read from x:AccountSettings, which every +// user can read; x:Account/get is refused to everyone but admins. Fire-and- +// forget; a failure keeps whatever the registry already had. async function syncAccountDisplayName(accountId: string): Promise { try { const accountStore = useAccountStore.getState(); @@ -169,11 +170,8 @@ async function syncAccountDisplayName(accountId: string): Promise { if (primary?.name?.trim()) updates.displayName = primary.name.trim(); if (primary?.email && !entry.email.includes('@')) updates.email = primary.email; if (isStalwartSupported()) { - const principal = await fetchPrincipal().catch(() => null); - if (principal?.displayName?.trim()) updates.displayName = principal.displayName.trim(); - if (!updates.email && principal?.emails[0] && !entry.email.includes('@')) { - updates.email = principal.emails[0]; - } + const fullName = await fetchAccountDisplayName().catch(() => null); + if (fullName) updates.displayName = fullName; } if (Object.keys(updates).length === 0) return; if (useAccountStore.getState().getAccountById(accountId)) {