mirror of
https://github.com/bulwarkmail/webmail.git
synced 2026-10-31 00:12:39 +00:00
fix: renew a background OAuth account with its own token instead of the active account's #989
Co-authored-by: Almost-Senseless-Coder <109462027+Almost-Senseless-Coder@users.noreply.github.com>
This commit is contained in:
Linus Rath
co-authored by
Almost-Senseless-Coder
Almost-Senseless-Coder
parent
72faa57367
commit
b88397dfd2
@@ -0,0 +1,228 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
// Each test loads a fresh store: the clients and refresh timers live at module level.
|
||||
let useAuthStore: typeof import('../auth-store').useAuthStore;
|
||||
let useAccountStore: typeof import('../account-store').useAccountStore;
|
||||
|
||||
type FetchInput = Parameters<typeof fetch>[0];
|
||||
type FetchInit = Parameters<typeof fetch>[1];
|
||||
|
||||
const SERVER = 'https://mail.example.test';
|
||||
|
||||
function account(id: string, slot: number, active: boolean) {
|
||||
return {
|
||||
id,
|
||||
label: id,
|
||||
serverUrl: SERVER,
|
||||
username: `${id}@example.test`,
|
||||
authMode: 'oauth' as const,
|
||||
rememberMe: true,
|
||||
displayName: id,
|
||||
email: `${id}@example.test`,
|
||||
lastLoginAt: Date.now(),
|
||||
isConnected: false,
|
||||
hasError: false,
|
||||
isDefault: active,
|
||||
cookieSlot: slot,
|
||||
avatarColor: '#000',
|
||||
};
|
||||
}
|
||||
|
||||
function session(username: string) {
|
||||
return {
|
||||
username,
|
||||
accounts: { acc: { name: username } },
|
||||
primaryAccounts: { 'urn:ietf:params:jmap:mail': 'acc' },
|
||||
apiUrl: `${SERVER}/jmap`,
|
||||
downloadUrl: `${SERVER}/download`,
|
||||
uploadUrl: `${SERVER}/upload`,
|
||||
capabilities: {},
|
||||
};
|
||||
}
|
||||
|
||||
function jsonResponse(status: number, body: unknown) {
|
||||
const res = {
|
||||
ok: status >= 200 && status < 300,
|
||||
status,
|
||||
redirected: false,
|
||||
url: '',
|
||||
headers: { get: () => null },
|
||||
json: async () => body,
|
||||
text: async () => JSON.stringify(body),
|
||||
clone() { return res; },
|
||||
};
|
||||
return res;
|
||||
}
|
||||
|
||||
/**
|
||||
* Reproduction for the silent account drop (#989).
|
||||
*
|
||||
* Two OAuth accounts: A (active, slot 0) and B (background, slot 1). B's
|
||||
* cached access token is already expired as far as the mail server is
|
||||
* concerned, so B's first JMAP call gets a 401 and its client asks the store
|
||||
* for a fresh token. The store must refresh SLOT 1 (B's refresh token) and
|
||||
* hand B a token minted for B - not force-rotate the active account's
|
||||
* refresh token and hand B the active account's token.
|
||||
*/
|
||||
describe('auth-store: token refresh is scoped to the account that needs it', () => {
|
||||
const forcedRefreshSlots: number[] = [];
|
||||
const deletedSessions: string[] = [];
|
||||
let freshCounter = 0;
|
||||
let bCachedExpired = true;
|
||||
let slot1Refused = false;
|
||||
|
||||
beforeEach(async () => {
|
||||
vi.restoreAllMocks();
|
||||
vi.resetModules();
|
||||
({ useAuthStore } = await import('../auth-store'));
|
||||
({ useAccountStore } = await import('../account-store'));
|
||||
const browserNavigation = await import('@/lib/browser-navigation');
|
||||
sessionStorage.clear();
|
||||
localStorage.clear();
|
||||
forcedRefreshSlots.length = 0;
|
||||
deletedSessions.length = 0;
|
||||
freshCounter = 0;
|
||||
bCachedExpired = true;
|
||||
slot1Refused = false;
|
||||
window.history.pushState({}, '', '/en');
|
||||
vi.spyOn(browserNavigation, 'replaceWindowLocation').mockImplementation(() => {});
|
||||
|
||||
useAccountStore.setState({
|
||||
accounts: [account('A', 0, true), account('B', 1, false)],
|
||||
activeAccountId: 'A',
|
||||
defaultAccountId: 'A',
|
||||
});
|
||||
useAuthStore.setState({
|
||||
isAuthenticated: true,
|
||||
isLoading: false,
|
||||
error: null,
|
||||
serverUrl: SERVER,
|
||||
username: 'A@example.test',
|
||||
client: null,
|
||||
authMode: 'oauth',
|
||||
rememberMe: true,
|
||||
accessToken: null,
|
||||
tokenExpiresAt: null,
|
||||
connectionLost: false,
|
||||
activeAccountId: 'A',
|
||||
});
|
||||
|
||||
const fetchMock = vi.fn(async (input: FetchInput, init?: FetchInit) => {
|
||||
const url = String(input);
|
||||
const method = init?.method ?? 'GET';
|
||||
const headers = (init?.headers ?? {}) as Record<string, string>;
|
||||
|
||||
// Bulwark token route: plain PUT hands back the cached token for the
|
||||
// slot; force=true mints a fresh one for that slot.
|
||||
const m = url.match(/^\/api\/auth\/token\?slot=(\d+)(&force=true)?$/);
|
||||
if (m && method === 'PUT') {
|
||||
const slot = Number(m[1]);
|
||||
const who = slot === 0 ? 'A' : 'B';
|
||||
if (m[2]) {
|
||||
forcedRefreshSlots.push(slot);
|
||||
if (slot === 1 && slot1Refused) return jsonResponse(401, { error: 'invalid_grant' });
|
||||
freshCounter += 1;
|
||||
return jsonResponse(200, { access_token: `${who}-fresh-${freshCounter}`, expires_in: 3600 });
|
||||
}
|
||||
return jsonResponse(200, { access_token: `${who}-cached`, expires_in: 3600 });
|
||||
}
|
||||
if (url.startsWith('/api/auth/session') && method === 'DELETE') {
|
||||
deletedSessions.push(url);
|
||||
return jsonResponse(200, {});
|
||||
}
|
||||
if (url.startsWith('/api/auth/token') && method === 'DELETE') {
|
||||
return jsonResponse(200, {});
|
||||
}
|
||||
if (url === '/api/auth/stalwart-context') {
|
||||
return jsonResponse(200, { ok: true });
|
||||
}
|
||||
|
||||
// Mail server: B's cached token is expired; everything else is accepted
|
||||
// and answered with the session of the account the token belongs to.
|
||||
if (url === `${SERVER}/.well-known/jmap`) {
|
||||
const auth = headers.Authorization ?? '';
|
||||
if (auth === 'Bearer B-cached' && bCachedExpired) return jsonResponse(401, {});
|
||||
const owner = auth.includes('Bearer A') ? 'A' : 'B';
|
||||
return jsonResponse(200, session(`${owner}@example.test`));
|
||||
}
|
||||
// Anything else (display-name lookups etc.) is irrelevant here.
|
||||
return jsonResponse(404, {});
|
||||
});
|
||||
vi.stubGlobal('fetch', fetchMock);
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
for (const client of useAuthStore.getState().getAllConnectedClients().values()) {
|
||||
try { client.disconnect(); } catch { /* ignore */ }
|
||||
}
|
||||
vi.unstubAllGlobals();
|
||||
});
|
||||
|
||||
it('refreshes the background account\'s own slot and hands its client its own token', async () => {
|
||||
await useAuthStore.getState().checkAuth();
|
||||
|
||||
// Background restore of B runs fire-and-forget after A is up.
|
||||
await vi.waitFor(() => {
|
||||
expect(forcedRefreshSlots.length).toBeGreaterThan(0);
|
||||
});
|
||||
// Give the retry-after-refresh a moment to land.
|
||||
await vi.waitFor(() => {
|
||||
expect(useAuthStore.getState().getClientForAccount('B')).toBeDefined();
|
||||
});
|
||||
|
||||
// The refresh B asked for must be spent on B's slot, never on A's.
|
||||
expect(forcedRefreshSlots).toEqual([1]);
|
||||
|
||||
// B ends up holding a token minted for B, not A's.
|
||||
const clientB = useAuthStore.getState().getClientForAccount('B')!;
|
||||
expect(clientB.getAuthHeader()).toMatch(/^Bearer B-fresh-/);
|
||||
|
||||
// Nobody got evicted and the active session is untouched.
|
||||
const ids = useAccountStore.getState().accounts.map((a) => a.id).sort();
|
||||
expect(ids).toEqual(['A', 'B']);
|
||||
expect(useAuthStore.getState().isAuthenticated).toBe(true);
|
||||
expect(deletedSessions).toEqual([]);
|
||||
});
|
||||
|
||||
it('renews each account through its own slot when the scheduled refresh fires', async () => {
|
||||
bCachedExpired = false;
|
||||
vi.useFakeTimers({ shouldAdvanceTime: true });
|
||||
try {
|
||||
await useAuthStore.getState().checkAuth();
|
||||
await vi.waitFor(() => {
|
||||
expect(useAuthStore.getState().getClientForAccount('B')).toBeDefined();
|
||||
});
|
||||
expect(forcedRefreshSlots).toEqual([]);
|
||||
|
||||
await vi.advanceTimersByTimeAsync(3600_000);
|
||||
|
||||
expect([...forcedRefreshSlots].sort()).toEqual([0, 1]);
|
||||
expect(useAuthStore.getState().getClientForAccount('A')!.getAuthHeader()).toMatch(/^Bearer A-fresh-/);
|
||||
expect(useAuthStore.getState().getClientForAccount('B')!.getAuthHeader()).toMatch(/^Bearer B-fresh-/);
|
||||
expect(useAuthStore.getState().accessToken).toMatch(/^A-fresh-/);
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it('drops only the background account whose refresh token is refused', async () => {
|
||||
bCachedExpired = false;
|
||||
slot1Refused = true;
|
||||
vi.useFakeTimers({ shouldAdvanceTime: true });
|
||||
try {
|
||||
await useAuthStore.getState().checkAuth();
|
||||
await vi.waitFor(() => {
|
||||
expect(useAuthStore.getState().getClientForAccount('B')).toBeDefined();
|
||||
});
|
||||
|
||||
await vi.advanceTimersByTimeAsync(3600_000);
|
||||
|
||||
expect(useAccountStore.getState().accounts.map((a) => a.id)).toEqual(['A']);
|
||||
expect(useAuthStore.getState().isAuthenticated).toBe(true);
|
||||
expect(useAuthStore.getState().activeAccountId).toBe('A');
|
||||
expect(deletedSessions).not.toContain('/api/auth/session?slot=0');
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
});
|
||||
+46
-31
@@ -92,13 +92,13 @@ interface AuthState {
|
||||
loginWithServerSso: (code: string, state: string) => Promise<boolean>;
|
||||
loginDemo: () => Promise<boolean>;
|
||||
/**
|
||||
* Obtain a usable access token for the active account.
|
||||
* Obtain a usable access token for `accountId`, or the active account.
|
||||
*
|
||||
* Renews against the IdP by default. `allowCached` lets a session restore
|
||||
* reuse the token the server still holds for this slot - IdPs that gate
|
||||
* refresh tokens behind an `nbf` claim reject an early renewal outright.
|
||||
*/
|
||||
refreshAccessToken: (options?: { allowCached?: boolean }) => Promise<string | null>;
|
||||
refreshAccessToken: (options?: { allowCached?: boolean; accountId?: string }) => Promise<string | null>;
|
||||
/**
|
||||
* Sign out of the active account. Switches to the next signed-in account
|
||||
* when there is one; otherwise ends the identity provider's session too
|
||||
@@ -1383,7 +1383,7 @@ export const useAuthStore = create<AuthState>()(
|
||||
}
|
||||
|
||||
if (bearerToken) {
|
||||
client = JMAPClient.withBearer(serverUrl, bearerToken, username, () => get().refreshAccessToken());
|
||||
client = JMAPClient.withBearer(serverUrl, bearerToken, username, () => get().refreshAccessToken({ accountId }));
|
||||
await client.connect();
|
||||
oauthAccessToken = bearerToken;
|
||||
upgradedToOAuth = true;
|
||||
@@ -1517,7 +1517,7 @@ export const useAuthStore = create<AuthState>()(
|
||||
|
||||
// Schedule token refresh for TOTP-upgraded sessions
|
||||
if (upgradedToOAuth && oauthExpiresIn > 0) {
|
||||
scheduleRefresh(oauthExpiresIn, get().refreshAccessToken, accountId);
|
||||
scheduleRefresh(oauthExpiresIn, () => get().refreshAccessToken({ accountId }), accountId);
|
||||
}
|
||||
|
||||
// Sync settings from server (only if enabled)
|
||||
@@ -1775,8 +1775,10 @@ export const useAuthStore = create<AuthState>()(
|
||||
|
||||
const { access_token, expires_in } = await tokenRes.json();
|
||||
|
||||
const refreshFn = get().refreshAccessToken;
|
||||
const client = JMAPClient.withBearer(serverUrl, access_token, '', () => refreshFn());
|
||||
// The account id comes from the session, so the refresh hook reads
|
||||
// it once it is known below.
|
||||
let boundAccountId: string | undefined;
|
||||
const client = JMAPClient.withBearer(serverUrl, access_token, '', () => get().refreshAccessToken({ accountId: boundAccountId }));
|
||||
await client.connect();
|
||||
|
||||
const jmapUsername = client.getUsername();
|
||||
@@ -1793,6 +1795,7 @@ export const useAuthStore = create<AuthState>()(
|
||||
|
||||
// Register in account store
|
||||
const accountId = generateAccountId(username, serverUrl);
|
||||
boundAccountId = accountId;
|
||||
|
||||
// Snapshot current account if switching away and clear stores so
|
||||
// the new account starts with a clean email/contact/calendar state.
|
||||
@@ -1852,7 +1855,7 @@ export const useAuthStore = create<AuthState>()(
|
||||
});
|
||||
}).catch(() => {});
|
||||
|
||||
scheduleRefresh(expires_in, get().refreshAccessToken, accountId);
|
||||
scheduleRefresh(expires_in, () => get().refreshAccessToken({ accountId }), accountId);
|
||||
|
||||
notifyParent('sso:auth-success', { username });
|
||||
|
||||
@@ -1928,8 +1931,8 @@ export const useAuthStore = create<AuthState>()(
|
||||
throw new Error('Server URL not configured');
|
||||
}
|
||||
|
||||
const refreshFn = get().refreshAccessToken;
|
||||
const client = JMAPClient.withBearer(ssoServerUrl, access_token, '', () => refreshFn());
|
||||
let boundAccountId: string | undefined;
|
||||
const client = JMAPClient.withBearer(ssoServerUrl, access_token, '', () => get().refreshAccessToken({ accountId: boundAccountId }));
|
||||
await client.connect();
|
||||
|
||||
const jmapUsername = client.getUsername();
|
||||
@@ -1941,6 +1944,7 @@ export const useAuthStore = create<AuthState>()(
|
||||
initializeFeatureStores(client);
|
||||
|
||||
const accountId = generateAccountId(username, ssoServerUrl);
|
||||
boundAccountId = accountId;
|
||||
|
||||
const prevAccountId = get().activeAccountId;
|
||||
if (prevAccountId && prevAccountId !== accountId) {
|
||||
@@ -1996,7 +2000,7 @@ export const useAuthStore = create<AuthState>()(
|
||||
});
|
||||
}).catch(() => {});
|
||||
|
||||
scheduleRefresh(expires_in, get().refreshAccessToken, accountId);
|
||||
scheduleRefresh(expires_in, () => get().refreshAccessToken({ accountId }), accountId);
|
||||
|
||||
notifyParent('sso:auth-success', { username });
|
||||
|
||||
@@ -2027,15 +2031,20 @@ export const useAuthStore = create<AuthState>()(
|
||||
},
|
||||
|
||||
refreshAccessToken: async (options) => {
|
||||
if (refreshPromise) return refreshPromise;
|
||||
|
||||
const accountId = get().activeAccountId;
|
||||
if (accountId && refreshPromises.has(accountId)) {
|
||||
return refreshPromises.get(accountId)!;
|
||||
// Every multi-account client and timer passes its own account: renewing
|
||||
// the active slot on a background account's behalf would hand that
|
||||
// client another account's token (#989).
|
||||
const accountId = options?.accountId ?? get().activeAccountId;
|
||||
if (accountId) {
|
||||
const inFlight = refreshPromises.get(accountId);
|
||||
if (inFlight) return inFlight;
|
||||
} else if (refreshPromise) {
|
||||
return refreshPromise;
|
||||
}
|
||||
|
||||
const account = accountId ? useAccountStore.getState().getAccountById(accountId) : null;
|
||||
const slot = account?.cookieSlot ?? 0;
|
||||
const isActive = !accountId || accountId === get().activeAccountId;
|
||||
// The account is being signed out: a renewal now would only undo that.
|
||||
if (closingSlots.has(slot)) return null;
|
||||
|
||||
@@ -2054,6 +2063,11 @@ export const useAuthStore = create<AuthState>()(
|
||||
// maintenance windows and offline spells.
|
||||
if (res.status === 401) {
|
||||
resetRefreshBackoff(accountId ?? undefined);
|
||||
// Only this login's refresh token was refused.
|
||||
if (accountId && !isActive) {
|
||||
get().removeAccount(accountId);
|
||||
return null;
|
||||
}
|
||||
notifyParent('sso:session-expired');
|
||||
markSessionExpired();
|
||||
get().logout({ endProviderSession: false });
|
||||
@@ -2075,7 +2089,7 @@ export const useAuthStore = create<AuthState>()(
|
||||
if (shouldRetryRefresh(slot, accountId ?? undefined)) {
|
||||
const retryIn = nextRefreshRetrySeconds(accountId ?? undefined);
|
||||
debug.error(`Token refresh unavailable (${res.status}), retrying with backoff`);
|
||||
scheduleRefresh(retryIn, get().refreshAccessToken, accountId ?? undefined);
|
||||
scheduleRefresh(retryIn, () => get().refreshAccessToken({ accountId: accountId ?? undefined }), accountId ?? undefined);
|
||||
}
|
||||
return null;
|
||||
}
|
||||
@@ -2085,7 +2099,8 @@ export const useAuthStore = create<AuthState>()(
|
||||
// Signed out while the renewal was in flight: nothing to keep alive.
|
||||
if (closingSlots.has(slot)) return null;
|
||||
|
||||
get().client?.updateAccessToken(access_token);
|
||||
const owner = (accountId ? clients.get(accountId) : undefined) ?? (isActive ? get().client : null);
|
||||
owner?.updateAccessToken(access_token);
|
||||
|
||||
if (account) {
|
||||
await syncStalwartAuthContext(
|
||||
@@ -2096,13 +2111,15 @@ export const useAuthStore = create<AuthState>()(
|
||||
);
|
||||
}
|
||||
|
||||
set({
|
||||
accessToken: access_token,
|
||||
tokenExpiresAt: Date.now() + expires_in * 1000,
|
||||
});
|
||||
if (isActive) {
|
||||
set({
|
||||
accessToken: access_token,
|
||||
tokenExpiresAt: Date.now() + expires_in * 1000,
|
||||
});
|
||||
}
|
||||
|
||||
resetRefreshBackoff(accountId ?? undefined);
|
||||
scheduleRefresh(expires_in, get().refreshAccessToken, accountId ?? undefined);
|
||||
scheduleRefresh(expires_in, () => get().refreshAccessToken({ accountId: accountId ?? undefined }), accountId ?? undefined);
|
||||
if (accountId) void authHooks.onTokenRefresh.emit({ accountId });
|
||||
return access_token;
|
||||
} catch (error) {
|
||||
@@ -2110,17 +2127,17 @@ export const useAuthStore = create<AuthState>()(
|
||||
// not a rejection. Keep the session and retry with backoff.
|
||||
debug.error('Token refresh failed, retrying with backoff:', error);
|
||||
if (shouldRetryRefresh(slot, accountId ?? undefined)) {
|
||||
scheduleRefresh(nextRefreshRetrySeconds(accountId ?? undefined), get().refreshAccessToken, accountId ?? undefined);
|
||||
scheduleRefresh(nextRefreshRetrySeconds(accountId ?? undefined), () => get().refreshAccessToken({ accountId: accountId ?? undefined }), accountId ?? undefined);
|
||||
}
|
||||
return null;
|
||||
} finally {
|
||||
refreshPromise = null;
|
||||
if (accountId) refreshPromises.delete(accountId);
|
||||
else refreshPromise = null;
|
||||
}
|
||||
})();
|
||||
|
||||
refreshPromise = promise;
|
||||
if (accountId) refreshPromises.set(accountId, promise);
|
||||
else refreshPromise = promise;
|
||||
|
||||
return promise;
|
||||
},
|
||||
@@ -2371,12 +2388,11 @@ export const useAuthStore = create<AuthState>()(
|
||||
const res = await fetchSlotAccessToken(targetAccount.cookieSlot);
|
||||
if (res.ok) {
|
||||
const { access_token, expires_in } = await res.json();
|
||||
const refreshFn = get().refreshAccessToken;
|
||||
targetClient = JMAPClient.withBearer(targetAccount.serverUrl, access_token, targetAccount.username, () => refreshFn());
|
||||
targetClient = JMAPClient.withBearer(targetAccount.serverUrl, access_token, targetAccount.username, () => get().refreshAccessToken({ accountId }));
|
||||
bindClientStatusHandlers(targetClient, set, get, accountId);
|
||||
await targetClient.connect();
|
||||
clients.set(accountId, targetClient);
|
||||
scheduleRefresh(expires_in, get().refreshAccessToken, accountId);
|
||||
scheduleRefresh(expires_in, () => get().refreshAccessToken({ accountId }), accountId);
|
||||
await syncStalwartAuthContext(
|
||||
targetAccount.serverUrl,
|
||||
targetAccount.username,
|
||||
@@ -2654,13 +2670,12 @@ export const useAuthStore = create<AuthState>()(
|
||||
const res = await fetchSlotAccessToken(account.cookieSlot);
|
||||
if (res.ok) {
|
||||
const { access_token, expires_in } = await res.json();
|
||||
const refreshFn = get().refreshAccessToken;
|
||||
const client = JMAPClient.withBearer(account.serverUrl, access_token, account.username, () => refreshFn());
|
||||
const client = JMAPClient.withBearer(account.serverUrl, access_token, account.username, () => get().refreshAccessToken({ accountId: account.id }));
|
||||
bindClientStatusHandlers(client, set, get, account.id);
|
||||
const contextSync = syncStalwartAuthContext(account.serverUrl, account.username, client.getAuthHeader(), account.cookieSlot);
|
||||
await client.connect();
|
||||
clients.set(account.id, client);
|
||||
scheduleRefresh(expires_in, get().refreshAccessToken, account.id);
|
||||
scheduleRefresh(expires_in, () => get().refreshAccessToken({ accountId: account.id }), account.id);
|
||||
await contextSync;
|
||||
accountStore.updateAccount(account.id, { isConnected: true, hasError: false });
|
||||
void syncAccountDisplayName(account.id, client);
|
||||
|
||||
Reference in New Issue
Block a user