diff --git a/app/api/admin/__tests__/config-merge-shared.test.ts b/app/api/admin/__tests__/config-merge-shared.test.ts new file mode 100644 index 000000000..c6f94b10e --- /dev/null +++ b/app/api/admin/__tests__/config-merge-shared.test.ts @@ -0,0 +1,99 @@ +// @vitest-environment node +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdtemp, rm } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { NextRequest } from 'next/server'; + +// Some admin saves change one entry of a list: the per-domain branding, the +// client secrets of the JMAP servers. Built on this instance's copy of the +// list, they dropped what another instance (or a parallel upload here) had +// saved a moment before (#1066). + +vi.mock('@/lib/admin/session', () => ({ + requireAdminAuth: async () => ({ payload: { role: 'admin' } }), + getClientIP: () => '127.0.0.1', +})); +vi.mock('@/lib/admin/audit', () => ({ auditLog: async () => {} })); + +import { POST as uploadBranding, DELETE as deleteBranding } from '../branding/route'; +import { PATCH as patchConfig } from '../config/route'; +import { configManager } from '@/lib/admin/config-manager'; +import { adminConfigDocs } from '@/lib/admin/admin-store'; +import { setStoreForTests } from '@/lib/shared-store'; +import { resetLocalStoreForTests } from '@/lib/shared-store/local'; +import { INIT_EPOCH, markEpochsSeen, resetChangesForTests } from '@/lib/shared-store/changes'; +import { separateFakeStore } from '@/lib/admin/__tests__/shared-store-fake'; + +let dir: string; +const stored = async () => JSON.parse((await adminConfigDocs().get('config.json'))!.data.toString('utf-8')); +/** Another instance saved this; its notice hasn't arrived here yet. */ +const savedElsewhere = (config: Record) => adminConfigDocs().put('config.json', JSON.stringify(config)); + +function upload(host: string) { + const form = new FormData(); + form.set('file', new File([new Uint8Array([0x89, 0x50, 0x4e, 0x47])], 'logo.png', { type: 'image/png' })); + form.set('slot', 'faviconUrl'); + form.set('host', host); + return uploadBranding(new NextRequest('https://mail.example/api/admin/branding', { method: 'POST', body: form })); +} + +beforeEach(async () => { + dir = await mkdtemp(path.join(tmpdir(), 'bw-config-merge-')); + vi.stubEnv('ADMIN_CONFIG_DIR', path.join(dir, 'config')); + vi.stubEnv('ADMIN_STATE_DIR', path.join(dir, 'state')); + resetLocalStoreForTests(); + resetChangesForTests(); + const store = separateFakeStore(path.join(dir, 'store')); + setStoreForTests(store); + await store.epochs.bump(INIT_EPOCH); + await savedElsewhere({ setupComplete: true }); + await markEpochsSeen(); + await configManager.load(); +}); + +afterEach(async () => { + setStoreForTests(null); + resetChangesForTests(); + resetLocalStoreForTests(); + vi.unstubAllEnvs(); + await rm(dir, { recursive: true, force: true }); +}); + +describe('list saves with a shared store', () => { + it('keeps another instance\'s domain branding when uploading for another domain', async () => { + await savedElsewhere({ setupComplete: true, domainBranding: [{ host: 'a.example', faviconUrl: '/api/admin/branding/a.png' }] }); + expect((await upload('b.example')).status).toBe(200); + const hosts = (await stored()).domainBranding.map((e: { host: string }) => e.host).sort(); + expect(hosts).toEqual(['a.example', 'b.example']); + + await savedElsewhere({ ...(await stored()), domainBranding: [...(await stored()).domainBranding, { host: 'c.example', faviconUrl: '/x.png' }] }); + const res = await deleteBranding(new NextRequest('https://mail.example/api/admin/branding', { + method: 'DELETE', + body: JSON.stringify({ slot: 'faviconUrl', host: 'b.example' }), + })); + expect(res.status).toBe(200); + expect((await stored()).domainBranding.map((e: { host: string }) => e.host)).toContain('c.example'); + }); + + it('keeps both of two uploads for different domains at once', async () => { + const results = await Promise.all([upload('a.example'), upload('b.example')]); + expect(results.map((r) => r.status)).toEqual([200, 200]); + expect((await stored()).domainBranding.map((e: { host: string }) => e.host).sort()).toEqual(['a.example', 'b.example']); + }); + + it('keeps a JMAP server\'s client secret another instance just saved', async () => { + const server = { id: 'corp', label: 'Corp', url: 'https://mail.corp.example', oauth: { clientId: 'webmail' } }; + await savedElsewhere({ setupComplete: true, jmapServers: [{ ...server, oauth: { clientId: 'webmail', clientSecret: 'secret-saved-elsewhere' } }] }); + // The dashboard never has the secret, so it sends the entry without it. + const res = await patchConfig(new NextRequest('https://mail.example/api/admin/config', { + method: 'PATCH', + headers: { 'content-type': 'application/json' }, + body: JSON.stringify({ jmapServers: [{ ...server, label: 'Corp mail' }], appName: 'Renamed' }), + })); + expect(res.status).toBe(200); + const config = await stored(); + expect(config.appName).toBe('Renamed'); + expect(config.jmapServers[0]).toMatchObject({ label: 'Corp mail', oauth: { clientId: 'webmail', clientSecret: 'secret-saved-elsewhere' } }); + }); +}); diff --git a/app/api/admin/branding/route.ts b/app/api/admin/branding/route.ts index b7d5e4ff5..14c0864d2 100644 --- a/app/api/admin/branding/route.ts +++ b/app/api/admin/branding/route.ts @@ -193,9 +193,10 @@ export async function POST(request: NextRequest) { await configManager.ensureLoaded(); if (host) { - const current = parseDomainBranding(configManager.get('domainBranding', [])); - const next = mergeDomainEntry(current, host, { [slot]: servedUrl }); - await configManager.setAdminConfig({ domainBranding: next }); + // Built on the stored list, so a save of another domain at the same + // time (here or on another instance) stays. + await configManager.updateAdminConfig('domainBranding', (current) => + mergeDomainEntry(parseDomainBranding(current), host, { [slot]: servedUrl }), []); } else { await configManager.setAdminConfig({ [slot]: servedUrl }); } @@ -269,9 +270,8 @@ export async function DELETE(request: NextRequest) { await configManager.ensureLoaded(); if (host) { - const current = parseDomainBranding(configManager.get('domainBranding', [])); - const next = clearDomainKeys(current, host, [slot as BrandingOverrideKey]); - await configManager.setAdminConfig({ domainBranding: next }); + await configManager.updateAdminConfig('domainBranding', (current) => + clearDomainKeys(parseDomainBranding(current), host, [slot as BrandingOverrideKey]), []); } else { await configManager.removeAdminOverride(slot); } diff --git a/app/api/admin/config/route.ts b/app/api/admin/config/route.ts index 33e3cd203..dd223fafd 100644 --- a/app/api/admin/config/route.ts +++ b/app/api/admin/config/route.ts @@ -135,6 +135,7 @@ export async function PATCH(request: NextRequest) { // Normalize jmapServers: pass through the parser so invalid entries are // rejected (bad ids, duplicate ids, non-HTTP URLs) before they're persisted. + let jmapServers: unknown[] | null = null; if ('jmapServers' in updates) { const incoming = updates.jmapServers; if (incoming != null && !Array.isArray(incoming)) { @@ -151,6 +152,8 @@ export async function PATCH(request: NextRequest) { error: 'One or more jmapServers entries are invalid (each needs a unique id, label, and HTTP(S) url).', }, { status: 400 }); } + // Saved on its own below, with the secrets of the stored list. + jmapServers = Array.isArray(incoming) ? incoming : []; updates.jmapServers = sanitized; } @@ -179,7 +182,18 @@ export async function PATCH(request: NextRequest) { oldValues[key] = configManager.get(key); } - await configManager.setAdminConfig(updates); + if (jmapServers) { + // The secrets the dashboard didn't send come from the stored list, not + // this instance's copy: a secret saved elsewhere a moment ago stays. + const { jmapServers: _servers, ...rest } = updates; + void _servers; + if (Object.keys(rest).length > 0) await configManager.setAdminConfig(rest); + const incoming = jmapServers; + updates.jmapServers = await configManager.updateAdminConfig('jmapServers', (current) => + parseJmapServers(withStoredClientSecrets(incoming, parseJmapServers(current))), []); + } else { + await configManager.setAdminConfig(updates); + } await auditLog('config.update', { changes: Object.keys(updates).map(k => ({ key: k, old: oldValues[k], new: updates[k] })) }, ip); return NextResponse.json({ ok: true });