diff --git a/.changeset/system-config-admin-writes-authoritative.md b/.changeset/system-config-admin-writes-authoritative.md new file mode 100644 index 0000000..d958962 --- /dev/null +++ b/.changeset/system-config-admin-writes-authoritative.md @@ -0,0 +1,20 @@ +--- +'seamless-auth-api': minor +--- + +Make admin system-config writes authoritative so they survive a restart. + +Env-mapped `system_config` rows are re-seeded from their environment variable on +every boot for any row whose `updatedBy` is `NULL`. Admin console writes went +through the access-token path, which never populated `updatedBy` (only the +service-token path did), so a change made in the console (for example adding an +OAuth provider or enabling the `oauth` login method) was silently reverted on the +next restart, contradicting the documented contract. + +The whole-config `PATCH /system-config/admin` and the per-provider +`/system-config/oauth-providers` endpoints now record the acting admin's id in +`updatedBy` on the access-token path, in addition to the existing service-token +path, so an admin change is genuinely authoritative and is no longer overwritten +from env. Boot now also logs a warning when it overwrites a stored value that +differs from the env-derived one, so the reseed is no longer silent. +`docs/configuration.md` is updated to describe the real precedence. diff --git a/docs/configuration.md b/docs/configuration.md index 65c7728..a954b3b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -11,8 +11,9 @@ There are two configuration layers: OAuth providers, rate limits). These are seeded from environment variables on first boot and are the source of truth afterwards. -> Because `system_config` is seeded once, editing an already-seeded value in `.env` does not -> change running behavior. See [Environment vs system_config](#environment-vs-system_config). +> Once an admin changes an env-mapped value through the admin system-config endpoints, that row +> becomes authoritative and its environment variable is no longer consulted. Until then, the env +> var is re-applied on every boot. See [Environment vs system_config](#environment-vs-system_config). ## Minimal configuration to boot @@ -221,9 +222,23 @@ Validation is enforced by [`systemConfig.schema.ts`](../src/schemas/systemConfig ## Environment vs `system_config` -`system_config` is seeded **once**. After first boot, the row is authoritative and the mapped -environment variable is no longer consulted for that key. To change a seeded value you must update -the `system_config` row (via the admin system-config endpoints), not just `.env`. +Each env-mapped `system_config` row is seeded from its environment variable on first boot. After +that, precedence depends on whether the row has been changed through the admin API: + +- **Until an admin changes it** (`updatedBy IS NULL`), the mapped environment variable is + re-applied on **every** boot. This keeps env-driven config authoritative and stops a seeded + default from permanently shadowing a later `.env` change. When a boot overwrites a stored value + that differs, it is logged at `warn`. +- **Once an admin changes it** through the admin system-config endpoints, the row records who made + the change (`updatedBy`) and becomes authoritative. The mapped environment variable is no longer + consulted for that key, so the admin change survives restarts and later `.env` edits do not + affect it. + +Both the whole-config `PATCH /system-config/admin` and the per-provider `/system-config/oauth-providers` +endpoints set `updatedBy`, whether the caller authenticates with a service token or with an admin +access token. So a change made in the admin console sticks. To make a seeded value change through +env again after an admin has taken it over, clear that row's `updatedBy` (or delete the row and let +it re-seed). Reads are cached in-process, so a `system_config` write should invalidate the cache to take effect immediately. See [`getSystemConfig.ts`](../src/config/getSystemConfig.ts). diff --git a/src/config/bootstrapSystemConfig.ts b/src/config/bootstrapSystemConfig.ts index e410d43..349076c 100644 --- a/src/config/bootstrapSystemConfig.ts +++ b/src/config/bootstrapSystemConfig.ts @@ -6,10 +6,13 @@ import { SystemConfig } from '../models/systemConfig.js'; import { SystemConfigSchema } from '../schemas/systemConfig.schema.js'; +import getLogger from '../utils/logger.js'; import { parseSystemConfigEnvValue } from '../utils/parseEnvConfigs.js'; import { SYSTEM_CONFIG_DEFAULTS } from './systemConfig.defaults.js'; import { SYSTEM_CONFIG_ENV_MAP } from './systemConfig.envMap.js'; +const logger = getLogger('bootstrapSystemConfig'); + export async function bootstrapSystemConfig() { const resolvedConfig: Record = {}; @@ -29,6 +32,11 @@ export async function bootstrapSystemConfig() { ); if (JSON.stringify(existing.value) !== JSON.stringify(parsed)) { + logger.warn( + `Overwriting system_config "${key}" from env ${envVar}: the stored value differs and ` + + `the row is not marked admin-managed (updatedBy IS NULL). If this value was changed ` + + `through the admin console, that change is being reverted here.`, + ); await existing.update({ value: parsed }); } diff --git a/src/controllers/oauthProviders.ts b/src/controllers/oauthProviders.ts index 7b08a75..0cea763 100644 --- a/src/controllers/oauthProviders.ts +++ b/src/controllers/oauthProviders.ts @@ -7,6 +7,7 @@ import { Response } from 'express'; import { getSystemConfig, invalidateSystemConfigCache } from '../config/getSystemConfig.js'; +import { resolveSystemConfigUpdatedBy } from '../lib/systemConfigActor.js'; import { SystemConfig } from '../models/systemConfig.js'; import { OAuthProviderConfig, OAuthProviderConfigSchema } from '../schemas/systemConfig.schema.js'; import { AuthEventService } from '../services/authEventService.js'; @@ -29,7 +30,7 @@ async function persistProviders( req: ServiceRequest, audit: ProviderAudit, ) { - const updatedBy = typeof req.clientId === 'function' ? req.clientId() : (req.clientId ?? null); + const updatedBy = resolveSystemConfigUpdatedBy(req); await SystemConfig.sequelize!.transaction(async (tx) => { await SystemConfig.upsert( diff --git a/src/controllers/systemConfig.ts b/src/controllers/systemConfig.ts index 3e70ee5..81d479a 100644 --- a/src/controllers/systemConfig.ts +++ b/src/controllers/systemConfig.ts @@ -7,6 +7,7 @@ import { Response } from 'express'; import { getSystemConfig, invalidateSystemConfigCache } from '../config/getSystemConfig.js'; +import { resolveSystemConfigUpdatedBy } from '../lib/systemConfigActor.js'; import { SystemConfig } from '../models/systemConfig.js'; import { User } from '../models/users.js'; import { createPatchSystemConfigSchema } from '../schemas/systemConfig.patch.schema.js'; @@ -73,7 +74,7 @@ export async function updateSystemConfig(req: ServiceRequest, res: Response) { const existingMap = Object.fromEntries(existingRows.map((row) => [row.key, row.value])); - const updatedBy = typeof req.clientId === 'function' ? req.clientId() : (req.clientId ?? null); + const updatedBy = resolveSystemConfigUpdatedBy(req); await SystemConfig.sequelize!.transaction(async (tx) => { for (const [key, value] of Object.entries(updates)) { diff --git a/src/lib/systemConfigActor.ts b/src/lib/systemConfigActor.ts new file mode 100644 index 0000000..90c0c50 --- /dev/null +++ b/src/lib/systemConfigActor.ts @@ -0,0 +1,19 @@ +/* + * Copyright © 2026 Fells Code, LLC + * Licensed under the GNU Affero General Public License v3.0 + * See LICENSE file in the project root for full license information + */ + +import { AuthenticatedRequest, ServiceRequest } from '../types/types.js'; + +// Who a system_config write is attributed to. Service-token callers carry +// req.clientId; admin console callers authenticate with an access token and +// carry req.user instead. Either one sets updatedBy, which marks the row as +// admin-managed so bootstrap stops re-seeding it from env on the next boot. +export function resolveSystemConfigUpdatedBy(req: ServiceRequest): string | null { + const clientId = typeof req.clientId === 'function' ? req.clientId() : req.clientId; + if (clientId) return clientId; + + const user = (req as Partial>).user; + return user?.id ?? null; +} diff --git a/tests/integration/systemConfig/oauthProviders.spec.ts b/tests/integration/systemConfig/oauthProviders.spec.ts index 071deea..1c437d1 100644 --- a/tests/integration/systemConfig/oauthProviders.spec.ts +++ b/tests/integration/systemConfig/oauthProviders.spec.ts @@ -80,7 +80,7 @@ describe('POST /system-config/oauth-providers', () => { expect(res.status).toBe(201); expect(res.body.provider.id).toBe('github'); expect(SystemConfig.upsert).toHaveBeenCalledWith( - expect.objectContaining({ key: 'oauth_providers' }), + expect.objectContaining({ key: 'oauth_providers', updatedBy: 'user-1' }), expect.anything(), ); expect(invalidateSystemConfigCache).toHaveBeenCalled(); diff --git a/tests/integration/systemConfig/systemConfig.spec.ts b/tests/integration/systemConfig/systemConfig.spec.ts index 653c30e..613db6f 100644 --- a/tests/integration/systemConfig/systemConfig.spec.ts +++ b/tests/integration/systemConfig/systemConfig.spec.ts @@ -75,6 +75,10 @@ describe('PATCH /system-config/admin', () => { expect(res.status).toBe(200); expect(res.body.success).toBe(true); expect(invalidateSystemConfigCache).toHaveBeenCalled(); + expect(SystemConfig.upsert).toHaveBeenCalledWith( + expect.objectContaining({ updatedBy: 'user-1' }), + expect.anything(), + ); }); it('accepts scoped role names', async () => { diff --git a/tests/unit/config/bootstrapSystemConfig.spec.ts b/tests/unit/config/bootstrapSystemConfig.spec.ts index d524dc8..5117268 100644 --- a/tests/unit/config/bootstrapSystemConfig.spec.ts +++ b/tests/unit/config/bootstrapSystemConfig.spec.ts @@ -24,6 +24,17 @@ vi.mock('../../../src/schemas/systemConfig.schema', () => ({ }, })); +const { loggerWarn } = vi.hoisted(() => ({ loggerWarn: vi.fn() })); + +vi.mock('../../../src/utils/logger', () => ({ + default: () => ({ + warn: loggerWarn, + info: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + }), +})); + function resetEnv() { delete process.env.APP_NAME; delete process.env.RATE_LIMIT; @@ -101,6 +112,10 @@ describe('bootstrapSystemConfig', () => { await bootstrapSystemConfig(); expect(update).toHaveBeenCalledWith({ value: 'parsed' }); + expect(loggerWarn).toHaveBeenCalledTimes(2); + const warnedMessages = loggerWarn.mock.calls.map((call) => call[0]).join('\n'); + expect(warnedMessages).toContain('app_name'); + expect(warnedMessages).toContain('rate_limit'); }); it('does not rewrite an env-backed row whose value already matches', async () => { @@ -120,6 +135,7 @@ describe('bootstrapSystemConfig', () => { await bootstrapSystemConfig(); expect(update).not.toHaveBeenCalled(); + expect(loggerWarn).not.toHaveBeenCalled(); }); it('preserves a row that was changed via the admin API (updatedBy set)', async () => {