diff --git a/app/(main)/admin/_pages/plugins.tsx b/app/(main)/admin/_pages/plugins.tsx index 74e9c19ab..eb09a808e 100644 --- a/app/(main)/admin/_pages/plugins.tsx +++ b/app/(main)/admin/_pages/plugins.tsx @@ -33,6 +33,8 @@ export function PluginsPage() { const [policy, setPolicy] = useState({ ...DEFAULT_POLICY }); const [savedPolicy, setSavedPolicy] = useState({ ...DEFAULT_POLICY }); const [savingPolicy, setSavingPolicy] = useState(false); + // The version of the policy this page loaded (see the policy page). + const policyETag = useRef(null); const iconFor = useExtensionIcons(); const fetchPlugins = useCallback(async () => { @@ -49,6 +51,7 @@ export function PluginsPage() { const res = await apiFetch('/api/admin/policy'); if (res.ok) { const data = await res.json(); + policyETag.current = res.headers.get('ETag'); setPolicy(data); setSavedPolicy(data); } @@ -77,10 +80,11 @@ export function PluginsPage() { try { const res = await apiFetch('/api/admin/policy', { method: 'PUT', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', ...(policyETag.current ? { 'If-Match': policyETag.current } : {}) }, body: JSON.stringify(policy), }); if (res.ok) { + policyETag.current = res.headers.get('ETag'); setSavedPolicy(policy); return null; } diff --git a/app/(main)/admin/_pages/policy.tsx b/app/(main)/admin/_pages/policy.tsx index 659aca2f0..3922976c4 100644 --- a/app/(main)/admin/_pages/policy.tsx +++ b/app/(main)/admin/_pages/policy.tsx @@ -1,6 +1,6 @@ 'use client'; -import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; import { Loader2, Plus, Trash2, ArrowUp, ArrowDown } from '@/components/icons'; import { iconForName } from '@/components/icons'; import type { SettingsPolicy, FeatureGates, PushRelayOption, AdminSidebarApp } from '@/lib/admin/types'; @@ -79,6 +79,9 @@ export function PolicyPage() { const [saving, setSaving] = useState(false); const setMessage = useAdminFeedback(); const [dirty, setDirty] = useState(false); + // The version of the policy this page loaded: a save based on an older + // one is refused instead of undoing someone else's change. + const policyETag = useRef(null); useEffect(() => { fetchPolicy(); }, []); @@ -88,6 +91,7 @@ export function PolicyPage() { const res = await apiFetch('/api/admin/policy'); if (res.ok) { const data = await res.json(); + policyETag.current = res.headers.get('ETag'); setPolicy(data); setSavedPolicy(data); } @@ -219,11 +223,12 @@ export function PolicyPage() { try { const res = await apiFetch('/api/admin/policy', { method: 'PUT', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', ...(policyETag.current ? { 'If-Match': policyETag.current } : {}) }, body: JSON.stringify(policy), }); if (res.ok) { + policyETag.current = res.headers.get('ETag'); setSavedPolicy(policy); setMessage({ type: 'success', text: 'Policy saved. Users see the change on their next login.' }); setDirty(false); diff --git a/app/(main)/admin/_pages/themes.tsx b/app/(main)/admin/_pages/themes.tsx index 387a9ddfa..8d472a43b 100644 --- a/app/(main)/admin/_pages/themes.tsx +++ b/app/(main)/admin/_pages/themes.tsx @@ -49,6 +49,8 @@ export function ThemesPage() { const [policy, setPolicy] = useState(() => withThemeDefaults({ ...DEFAULT_POLICY })); const [savedPolicy, setSavedPolicy] = useState(() => withThemeDefaults({ ...DEFAULT_POLICY })); const [savingPolicy, setSavingPolicy] = useState(false); + // The version of the policy this page loaded (see the policy page). + const policyETag = useRef(null); const fetchThemes = useCallback(async () => { try { @@ -64,6 +66,7 @@ export function ThemesPage() { const res = await apiFetch('/api/admin/policy'); if (res.ok) { const data = withThemeDefaults(await res.json()); + policyETag.current = res.headers.get('ETag'); setPolicy(data); setSavedPolicy(data); } @@ -152,10 +155,11 @@ export function ThemesPage() { try { const res = await apiFetch('/api/admin/policy', { method: 'PUT', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', ...(policyETag.current ? { 'If-Match': policyETag.current } : {}) }, body: JSON.stringify(policy), }); if (res.ok) { + policyETag.current = res.headers.get('ETag'); setSavedPolicy(policy); return null; } diff --git a/app/api/admin/__tests__/policy-if-match.test.ts b/app/api/admin/__tests__/policy-if-match.test.ts new file mode 100644 index 000000000..cc7bf3518 --- /dev/null +++ b/app/api/admin/__tests__/policy-if-match.test.ts @@ -0,0 +1,104 @@ +// @vitest-environment node +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { NextRequest } from 'next/server'; + +// The admin pages save the whole policy from a draft they loaded earlier. +// A save from a page opened before someone else's save (another tab, +// another admin) used to put the old policy back over theirs. + +vi.mock('@/lib/auth/instance-details', () => ({ + canSeeInstanceDetails: async () => true, + signedInIdentity: async () => ({ admin: true }), +})); +vi.mock('@/lib/admin/session', () => ({ + requireAdminAuth: async () => ({ payload: { role: 'admin' } }), + getClientIP: () => '127.0.0.1', +})); +vi.mock('@/lib/admin/audit', () => ({ auditLog: async () => {} })); + +import { GET, PUT } from '@/app/api/admin/policy/route'; +import { forgetExtensionInPolicy } from '@/lib/admin/extension-cleanup'; +import { configManager } from '@/lib/admin/config-manager'; + +let dir: string; +const URL = 'https://mail.example/api/admin/policy'; +const stored = async () => JSON.parse(await readFile(path.join(dir, 'policy.json'), 'utf-8')); + +async function load(): Promise<{ policy: Record; etag: string }> { + const res = await GET(new NextRequest(URL)); + expect(res.status).toBe(200); + const etag = res.headers.get('ETag'); + expect(etag).toMatch(/^".+"$/); + return { policy: await res.json(), etag: etag! }; +} + +function save(policy: Record, etag?: string) { + return PUT(new NextRequest(URL, { + method: 'PUT', + headers: { 'content-type': 'application/json', ...(etag ? { 'if-match': etag } : {}) }, + body: JSON.stringify(policy), + })); +} + +beforeEach(async () => { + dir = await mkdtemp(path.join(tmpdir(), 'bw-policy-if-match-')); + vi.stubEnv('ADMIN_CONFIG_DIR', dir); + await writeFile(path.join(dir, 'policy.json'), JSON.stringify({ features: { calendarEnabled: true }, forceEnabledPlugins: ['demo'] })); + await configManager.reload(); +}); + +afterEach(async () => { + vi.unstubAllEnvs(); + await rm(dir, { recursive: true, force: true }); +}); + +describe('policy save with If-Match', () => { + it('refuses a save based on an older policy and keeps the newer one', async () => { + const tabA = await load(); + const tabB = await load(); + + const first = await save({ ...tabA.policy, features: { ...(tabA.policy.features as object), calendarEnabled: false } }, tabA.etag); + expect(first.status).toBe(200); + expect(first.headers.get('ETag')).not.toBe(tabA.etag); + + const stale = await save({ ...tabB.policy, features: { ...(tabB.policy.features as object), filesEnabled: false } }, tabB.etag); + expect(stale.status).toBe(409); + expect((await stale.json()).error).toBe( + 'Someone else changed the policy after you opened this page. Reload to see their change; your edits stay here until you reload.', + ); + expect((await stored()).features.calendarEnabled).toBe(false); + expect((await stored()).features.filesEnabled).not.toBe(false); + expect(configManager.getPolicy().features.calendarEnabled).toBe(false); + + // Saving again with the version the first save answered works. + const again = await save({ ...tabA.policy, features: { ...(tabA.policy.features as object), calendarEnabled: true } }, first.headers.get('ETag')!); + expect(again.status).toBe(200); + }); + + it('sees a change saved to policy.json by another replica on the same volume', async () => { + const page = await load(); + await writeFile(path.join(dir, 'policy.json'), JSON.stringify({ features: { calendarEnabled: false } })); + const res = await save({ ...page.policy, features: { ...(page.policy.features as object), filesEnabled: false } }, page.etag); + expect(res.status).toBe(409); + expect((await stored()).features).toEqual({ calendarEnabled: false }); + }); + + it('refuses a page loaded before a plugin was dropped from the policy', async () => { + const page = await load(); + await forgetExtensionInPolicy('plugin', 'demo'); + expect((await stored()).forceEnabledPlugins).toEqual([]); + expect((await save(page.policy, page.etag)).status).toBe(409); + expect((await stored()).forceEnabledPlugins).toEqual([]); + }); + + it('replaces the policy without If-Match, as before', async () => { + await load(); + await configManager.setPolicy({ ...configManager.getPolicy(), forceEnabledPlugins: [] }); + const res = await save({ features: { calendarEnabled: false } }); + expect(res.status).toBe(200); + expect((await stored()).features.calendarEnabled).toBe(false); + }); +}); diff --git a/app/api/admin/__tests__/policy-scopes-route.test.ts b/app/api/admin/__tests__/policy-scopes-route.test.ts index 3c27c1320..70c6f3b70 100644 --- a/app/api/admin/__tests__/policy-scopes-route.test.ts +++ b/app/api/admin/__tests__/policy-scopes-route.test.ts @@ -28,6 +28,7 @@ vi.mock('@/lib/admin/config-manager', () => ({ organisation: { disclaimer: { html: '

Acme GmbH

', text: 'Acme GmbH' } }, scopes: SCOPES, }), + getPolicyVersion: () => 'v1', }, })); vi.mock('@/lib/logger', () => ({ logger: { error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn() } })); diff --git a/app/api/admin/plugins/__tests__/route.test.ts b/app/api/admin/plugins/__tests__/route.test.ts index 9d2ce079b..bfb4ec855 100644 --- a/app/api/admin/plugins/__tests__/route.test.ts +++ b/app/api/admin/plugins/__tests__/route.test.ts @@ -18,6 +18,7 @@ vi.mock('@/lib/admin/config-manager', () => ({ get: vi.fn(), getPolicy: vi.fn(() => policy), setPolicy: vi.fn(async (p: Record) => { policy = p; }), + updatePolicy: vi.fn(async (change: (p: Record) => Record | undefined) => { policy = change(policy) ?? policy; }), }, })); diff --git a/app/api/admin/policy/file/route.ts b/app/api/admin/policy/file/route.ts index b1448ad0b..62d7bee0c 100644 --- a/app/api/admin/policy/file/route.ts +++ b/app/api/admin/policy/file/route.ts @@ -1,5 +1,5 @@ import { NextRequest, NextResponse } from 'next/server'; -import { completePolicy, configManager } from '@/lib/admin/config-manager'; +import { completePolicy, configManager, PolicyConflictError } from '@/lib/admin/config-manager'; import { requireAdminAuth, getClientIP } from '@/lib/admin/session'; import { auditLog } from '@/lib/admin/audit'; import { logger } from '@/lib/logger'; @@ -57,12 +57,24 @@ export async function POST(request: NextRequest) { await configManager.ensureLoaded(); const next = completePolicy(raw as Partial); - const summary = summarizePolicyChange(configManager.getPolicy(), next); if (request.nextUrl.searchParams.get('preview') === '1') { - return NextResponse.json({ summary }); + return NextResponse.json({ summary: summarizePolicyChange(configManager.getPolicy(), next) }); } - await configManager.setPolicy(next); + // The import replaces the policy it was compared with: a save that lands + // in between is compared again, so the summary says what was replaced. + let summary: ReturnType; + for (let attempt = 1; ; attempt++) { + const version = configManager.getPolicyVersion(); + summary = summarizePolicyChange(configManager.getPolicy(), next); + try { + await configManager.setPolicy(next, { ifVersion: version }); + break; + } catch (error) { + if (!(error instanceof PolicyConflictError)) throw error; + if (attempt >= 3) return NextResponse.json({ error: error.message }, { status: 409 }); + } + } await auditLog('policy.import', { scopeCount: next.scopes?.length ?? 0, scopesAdded: summary.scopes.added.length, diff --git a/app/api/admin/policy/route.ts b/app/api/admin/policy/route.ts index 205c66f24..512307784 100644 --- a/app/api/admin/policy/route.ts +++ b/app/api/admin/policy/route.ts @@ -1,5 +1,5 @@ import { NextRequest, NextResponse } from 'next/server'; -import { configManager } from '@/lib/admin/config-manager'; +import { configManager, PolicyConflictError } from '@/lib/admin/config-manager'; import { requireAdminAuth, getClientIP } from '@/lib/admin/session'; import { auditLog } from '@/lib/admin/audit'; import { logger } from '@/lib/logger'; @@ -9,6 +9,21 @@ import { bearerIdentity } from '@/lib/auth/bearer-identity'; import { scopesVisibleTo } from '@/lib/admin/policy-scopes'; import { configReadOnlyResponse } from '@/lib/admin/config-read-only-response'; +/** The policy version as an HTTP entity tag. */ +function policyETag(version: string): string { + return `"${version}"`; +} + +/** + * The version an If-Match header names, or undefined for none (or `*`). + * The admin pages send the ETag of the policy they loaded. + */ +function ifMatchVersion(request: NextRequest): string | undefined { + const raw = request.headers.get('if-match')?.trim(); + if (!raw || raw === '*') return undefined; + return raw.split(',')[0].trim().replace(/^W\//, '').replace(/^"(.*)"$/, '$1'); +} + /** * GET /api/admin/policy - Get settings policy (NOT admin-protected - users read this; * visitors who are not signed in get the subset the login page needs) @@ -52,8 +67,10 @@ export async function GET(request: NextRequest) { // answers with all of them (documented). const identity = await signedInIdentity(); if (identity !== null && !identity.admin) return userPolicyResponse(policy, identity.addresses); + // The whole policy, as the admin pages edit it: with its version, which + // they send back as If-Match when they save. return NextResponse.json({ ...policy, scopes: policy.scopes ?? [] }, { - headers: { 'Cache-Control': 'no-store' }, + headers: { 'Cache-Control': 'no-store', ETag: policyETag(configManager.getPolicyVersion()) }, }); } catch (error) { logger.error('Policy read error', { error: error instanceof Error ? error.message : 'Unknown error' }); @@ -74,6 +91,10 @@ function userPolicyResponse(policy: SettingsPolicy, addresses: string[]): NextRe /** * PUT /api/admin/policy - Update settings policy (admin-protected) + * + * With `If-Match: ` the save is refused with 409 when the + * policy changed since it was read, so a page opened earlier can't undo + * someone else's save. Without it the policy is replaced, as it always was. */ export async function PUT(request: NextRequest) { try { @@ -110,14 +131,22 @@ export async function PUT(request: NextRequest) { return NextResponse.json({ error: 'organisation must be an object' }, { status: 400 }); } - await configManager.setPolicy(policy); + let version: string; + try { + version = await configManager.setPolicy(policy, { ifVersion: ifMatchVersion(request) }); + } catch (error) { + if (error instanceof PolicyConflictError) { + return NextResponse.json({ error: error.message }, { status: 409 }); + } + throw error; + } await auditLog('policy.update', { restrictionCount: Object.keys(policy.restrictions || {}).length, // Entries the sanitizer dropped never reach users, so log what stuck. defaultSidebarAppCount: configManager.getPolicy().defaultSidebarApps?.length ?? 0, }, ip); - return NextResponse.json({ ok: true }); + return NextResponse.json({ ok: true }, { headers: { ETag: policyETag(version) } }); } catch (error) { const readOnly = configReadOnlyResponse(error); if (readOnly) return readOnly; diff --git a/app/api/admin/themes/__tests__/route.test.ts b/app/api/admin/themes/__tests__/route.test.ts index 89a60618a..b69d04b25 100644 --- a/app/api/admin/themes/__tests__/route.test.ts +++ b/app/api/admin/themes/__tests__/route.test.ts @@ -17,6 +17,7 @@ vi.mock('@/lib/admin/config-manager', () => ({ get: vi.fn(), getPolicy: vi.fn(() => policy), setPolicy: vi.fn(async (p: Record) => { policy = p; }), + updatePolicy: vi.fn(async (change: (p: Record) => Record | undefined) => { policy = change(policy) ?? policy; }), }, })); const saved: Array<{ theme: Record; css: string; skin: string | null }> = []; diff --git a/components/admin/org/use-org-policy-draft.ts b/components/admin/org/use-org-policy-draft.ts index 411be8639..283f0d831 100644 --- a/components/admin/org/use-org-policy-draft.ts +++ b/components/admin/org/use-org-policy-draft.ts @@ -1,6 +1,6 @@ 'use client'; -import { useCallback, useEffect, useMemo, useState } from 'react'; +import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { create } from 'zustand'; import type { FeatureGates, SettingRestriction, SettingsPolicy } from '@/lib/admin/types'; import { DEFAULT_POLICY } from '@/lib/admin/types'; @@ -50,6 +50,9 @@ export function useOrgPolicyDraft(dirtyId: string) { const [policy, setPolicy] = useState(null); const [saved, setSaved] = useState(null); const [saving, setSaving] = useState(false); + // The version of the policy this draft started from: a save based on an + // older one is refused instead of undoing someone else's change. + const policyETag = useRef(null); // Outcomes show in a toast, like every admin page's. const setMessage = useAdminFeedback(); const scopeId = useOrgScopeStore((s) => s.scopeId); @@ -60,6 +63,7 @@ export function useOrgPolicyDraft(dirtyId: string) { const res = await apiFetch('/api/admin/policy'); if (!res.ok) throw new Error(); const data = (await res.json()) as SettingsPolicy; + policyETag.current = res.headers.get('ETag'); const normalised = { ...DEFAULT_POLICY, ...data, organisation: sanitizeOrganisationPolicy(data.organisation), scopes: data.scopes ?? [] }; setPolicy(normalised); setSaved(clone(normalised)); @@ -186,7 +190,7 @@ export function useOrgPolicyDraft(dirtyId: string) { try { const res = await apiFetch('/api/admin/policy', { method: 'PUT', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', ...(policyETag.current ? { 'If-Match': policyETag.current } : {}) }, body: JSON.stringify(policy), }); if (res.ok) { diff --git a/lib/__tests__/instance-details-gates.test.ts b/lib/__tests__/instance-details-gates.test.ts index 5e2d91c92..2da5a6969 100644 --- a/lib/__tests__/instance-details-gates.test.ts +++ b/lib/__tests__/instance-details-gates.test.ts @@ -32,6 +32,7 @@ vi.mock('@/lib/admin/config-manager', () => ({ ensureLoaded: async () => {}, get: (key: string, fallback: unknown) => (key in config.values ? config.values[key] : fallback), getPolicy: () => POLICY, + getPolicyVersion: () => 'v1', }, })); vi.mock('@/lib/logger', () => ({ logger: { error: vi.fn(), warn: vi.fn(), info: vi.fn(), debug: vi.fn() } })); diff --git a/lib/admin/config-manager.ts b/lib/admin/config-manager.ts index 5f6697918..a2289c1aa 100644 --- a/lib/admin/config-manager.ts +++ b/lib/admin/config-manager.ts @@ -8,6 +8,8 @@ import { sanitizeDefaultSidebarApps } from '@/lib/sidebar-apps'; import { sanitizePolicyDefaults } from './policy-defaults'; import { sanitizeOrganisationPolicy } from './organisation-policy'; import { sanitizeRestrictions, sanitizeScopes } from './policy-scopes'; +import { versionOf } from '@/lib/shared-store/keys'; +import { serializeLocal } from '@/lib/shared-store/local'; function parseEnvValue(value: string, type: string): unknown { switch (type) { @@ -39,6 +41,34 @@ type WatchedFile = typeof WATCHED_FILES[number]; */ const CHANGE_CHECK_INTERVAL_MS = 5_000; +/** The policy version while no policy has been saved (the defaults apply). */ +export const DEFAULT_POLICY_VERSION = 'default'; + +/** + * A policy save based on a version that is no longer the current one: + * someone else saved in between (another tab, another admin, a plugin + * deletion). The caller's draft would undo their change. + */ +export class PolicyConflictError extends Error { + constructor() { + super('Someone else changed the policy after you opened this page. Reload to see their change; your edits stay here until you reload.'); + this.name = 'PolicyConflictError'; + } +} + +/** Policy writes in this process, one after another, so a version check and its write can't interleave. */ +const POLICY_WRITE_QUEUE = 'admin-config\0policy.json'; + +interface ParsedDoc { + data: Record; + /** As the shared store computes it: changes whenever the bytes do. */ + version: string; +} + +function parseDoc(raw: string): ParsedDoc { + return { data: JSON.parse(raw), version: versionOf(Buffer.from(raw, 'utf-8')) }; +} + /** Changes whenever the file is replaced or rewritten; null when it is missing. */ async function fileStamp(filename: string): Promise { try { @@ -52,6 +82,8 @@ async function fileStamp(filename: string): Promise { class ConfigManager { private adminConfig: Record = {}; private policyCache: SettingsPolicy = { ...DEFAULT_POLICY }; + /** Version of the stored policy the cache came from; null while none is stored. */ + private policyVersion: string | null = null; private loaded = false; private fileStamps: Partial> = {}; private lastChangeCheck = 0; @@ -67,11 +99,12 @@ class ConfigManager { logger.debug('ConfigManager loaded', { configKeys: Object.keys(this.adminConfig).length }); } - private apply(config: Record | null, policy: Record | null): void { - this.adminConfig = config || {}; - this.policyCache = policy - ? ConfigManager.completePolicy(policy as Partial) + private apply(config: ParsedDoc | null, policy: ParsedDoc | null): void { + this.adminConfig = config?.data || {}; + this.policyCache = policy?.data + ? ConfigManager.completePolicy(policy.data as Partial) : { ...DEFAULT_POLICY }; + this.policyVersion = policy?.version ?? null; this.loaded = true; this.applyLoggerSettings(); } @@ -273,12 +306,47 @@ class ConfigManager { } /** - * Update the settings policy. Writes to disk. + * The version of the policy getPolicy() returns, for an ETag. Changes with + * every save; DEFAULT_POLICY_VERSION while none was saved. */ - async setPolicy(policy: SettingsPolicy): Promise { + getPolicyVersion(): string { + return this.policyVersion ?? DEFAULT_POLICY_VERSION; + } + + /** + * Update the settings policy. Writes to disk. With `ifVersion` (from + * getPolicyVersion when the caller read the policy) it refuses with a + * PolicyConflictError when the stored policy has changed since, so a + * draft based on an older policy can't undo someone else's save. + * Returns the new version. + */ + async setPolicy(policy: SettingsPolicy, options: { ifVersion?: string } = {}): Promise { assertWritable('update settings policy'); - this.policyCache = ConfigManager.completePolicy(policy); - await this.writeJsonFile('policy.json', this.policyCache as unknown as Record); + return serializeLocal(POLICY_WRITE_QUEUE, async () => { + if (options.ifVersion !== undefined) { + // Compare with what is on disk now, not with what this process saw last. + await this.reloadIfChanged(); + if (options.ifVersion !== this.getPolicyVersion()) throw new PolicyConflictError(); + } + this.policyCache = ConfigManager.completePolicy(policy); + await this.writeJsonFile('policy.json', this.policyCache as unknown as Record); + return this.getPolicyVersion(); + }); + } + + /** + * Change the stored policy with `change` (return undefined to leave it). + * For changes to one part of the policy that must not undo a save that + * lands at the same time, such as dropping a deleted plugin's id. + */ + async updatePolicy(change: (current: SettingsPolicy) => SettingsPolicy | undefined): Promise { + assertWritable('update settings policy'); + await serializeLocal(POLICY_WRITE_QUEUE, async () => { + const next = change(this.policyCache); + if (next === undefined) return; + this.policyCache = ConfigManager.completePolicy(next); + await this.writeJsonFile('policy.json', this.policyCache as unknown as Record); + }); } /** @@ -353,7 +421,7 @@ class ConfigManager { await this.load(); } - private async readJsonFile(filename: string): Promise | null> { + private async readJsonFile(filename: string): Promise { try { return await this.readJsonFileOrThrow(filename); } catch (error) { @@ -363,9 +431,9 @@ class ConfigManager { } /** null when the file doesn't exist; throws when it can't be read or parsed. */ - private async readJsonFileOrThrow(filename: string): Promise | null> { + private async readJsonFileOrThrow(filename: string): Promise { try { - return JSON.parse(await readFile(getConfigPath(filename), 'utf-8')); + return parseDoc(await readFile(getConfigPath(filename), 'utf-8')); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; throw error; @@ -376,9 +444,11 @@ class ConfigManager { await ensureConfigDir(); const targetPath = getConfigPath(filename); const tmpPath = targetPath + '.tmp'; + const text = JSON.stringify(data, null, 2); // Owner-only: config.json carries the session secret and OAuth client secrets. - await writeFile(tmpPath, JSON.stringify(data, null, 2), { encoding: 'utf-8', mode: 0o600 }); + await writeFile(tmpPath, text, { encoding: 'utf-8', mode: 0o600 }); await rename(tmpPath, targetPath); + if (filename === 'policy.json') this.policyVersion = versionOf(Buffer.from(text, 'utf-8')); // Our own write is not a change to pick up. this.fileStamps[filename] = await fileStamp(filename); } diff --git a/lib/admin/extension-cleanup.ts b/lib/admin/extension-cleanup.ts index 00b70529c..91065f41c 100644 --- a/lib/admin/extension-cleanup.ts +++ b/lib/admin/extension-cleanup.ts @@ -8,11 +8,14 @@ import { deletePluginConfig } from './plugin-config'; */ export async function forgetExtensionInPolicy(kind: 'plugin' | 'theme', id: string): Promise { await configManager.ensureLoaded(); - const policy = configManager.getPolicy(); const key = kind === 'plugin' ? 'forceEnabledPlugins' : 'forceEnabledThemes'; - const list = policy[key] ?? []; - if (!list.includes(id)) return; - await configManager.setPolicy({ ...policy, [key]: list.filter((x) => x !== id) }); + // Against the stored policy, so a policy save landing at the same time + // keeps its changes. + await configManager.updatePolicy((policy) => { + const list = policy[key] ?? []; + if (!list.includes(id)) return undefined; + return { ...policy, [key]: list.filter((x) => x !== id) }; + }); } /**