diff --git a/.changeset/22168-settings-user-rung-owner.md b/.changeset/22168-settings-user-rung-owner.md new file mode 100644 index 00000000000..65a134db6bb --- /dev/null +++ b/.changeset/22168-settings-user-rung-owner.md @@ -0,0 +1,19 @@ +--- +'@objectstack/service-settings': minor +--- + +fix(service-settings)!: the user rung answers only its owner, and a write of a user-scoped key must name its user + +Clause-②: no (narrowing) + + + +**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes. + +A settings key declared `scope: 'user'` belongs to one user. `SettingsService` now holds that on both its read and its write side. + +- **Reads.** The user rung answers only a caller whose `SettingsContext.userId` is present and equals the row's owner. A resolve whose context names no user (no `userId`, or an empty one) has no user rung: it answers from the tenant rung, then global, then the manifest default. This holds on every read path that reaches the cascade: `get`, `getMany`, `getNamespace`, `createClient`, `runAction` and `resetNamespace`. +- **Refused now.** `set` and `setMany` refuse a user-scoped key when the context names no user, a `null` reset of one included. The refusal is a `SettingsValidationError` (`code: 'SETTINGS_VALIDATION'`) with one `fields` entry per such key (`code: 'invalid_value'`, `constraint: { scope: 'user' }`). It refuses the whole batch, before anything is written. +- **Unchanged.** A caller with a user id reads and writes only their own rows, as before. Keys declared at `scope: 'tenant'` or `'global'` resolve and write the same with or without a user id. The settings HTTP routes resolve a signed-in user before they read or write any row, so they behave as before. + +What changes for you: when your code calls the settings service for a user-scoped key, pass the user id of the person the value belongs to. A value meant for everyone belongs on a key declared at `scope: 'tenant'` or `'global'`. None of the settings namespaces this repository ships declares a user-scoped key. diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index d3b0dd4d73f..3b142fbc142 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -106,6 +106,21 @@ const PLATFORM_SETTING_OBJECT = 'sys_platform_setting'; */ const SETTINGS_SYSTEM_CONTEXT = Object.freeze({ isSystem: true as const }); +/** + * The caller's user id, or `null` when the context names no user. + * + * The user rung is per owner: a `scope: 'user'` row belongs to the user its + * `user_id` names. So the rung answers a resolve only for that same user + * ({@link SettingsService.resolveKeyFromRows}), and a write of a user-scoped + * key needs a user to pin the row to ({@link SettingsService.setMany} refuses + * one without). An absent or empty id names nobody, and both read as "no user" + * here: the read falls through to the tenant rung, then global, then the + * default, and the write is refused. + */ +function callerUserIdOf(ctx: SettingsContext): string | null { + return typeof ctx.userId === 'string' && ctx.userId !== '' ? ctx.userId : null; +} + /** * Value-bearing specifier types — drives which entries we expect to * find in the K/V store. Keeps the resolver in sync with the spec @@ -1283,8 +1298,10 @@ export class SettingsService { const scope = reg.scopes.get(key)!; // For 'user' scope we pre-filter by user_id; for 'tenant' and 'global' // we load everything for the namespace and pick the right row below. - const rows = await this.loadRows(namespace, scope === 'user' ? ctx.userId ?? null : null); - return this.resolveKeyFromRows(reg, key, scope, rows); + // The user rung's pick compares the owner too (see `resolveKeyFromRows`). + const userId = scope === 'user' ? callerUserIdOf(ctx) : null; + const rows = await this.loadRows(namespace, userId); + return this.resolveKeyFromRows(reg, key, scope, rows, userId); } /** @@ -1379,18 +1396,19 @@ export class SettingsService { // [ADR-0131 D7] One load per required `loadRows` argument, as before — // and the global rung, which no argument changes, is read ONCE for both // (see `loadRowSets`). + const userId = callerUserIdOf(ctx); const groups: Array = [ - ...(userKeys.length > 0 ? [ctx.userId ?? null] : []), + ...(userKeys.length > 0 ? [userId] : []), ...(otherKeys.length > 0 ? [null] : []), ]; const sets = await this.loadRowSets(namespace, groups); const userRows = userKeys.length > 0 ? sets[0] : []; const otherRows = otherKeys.length > 0 ? sets[sets.length - 1] : []; for (const { key, scope } of userKeys) { - out[key] = await this.resolveKeyFromRows(reg, key, scope, userRows); + out[key] = await this.resolveKeyFromRows(reg, key, scope, userRows, userId); } for (const { key, scope } of otherKeys) { - out[key] = await this.resolveKeyFromRows(reg, key, scope, otherRows); + out[key] = await this.resolveKeyFromRows(reg, key, scope, otherRows, null); } } return out; @@ -1401,12 +1419,18 @@ export class SettingsService { * verbatim from {@link get} for #10826 so `get`, `getMany` and * `getNamespace` share ONE implementation of the resolution order * (env is handled by the callers BEFORE the row load, exactly as before). + * + * `userId` is the caller's ({@link callerUserIdOf}), or `null` for a caller + * that names no user and for every key not declared `scope: 'user'`. The user + * rung answers only a row whose `user_id` equals it; with `null` there is no + * user rung at all. */ private async resolveKeyFromRows( reg: RegisteredManifest, key: string, scope: SpecifierScope, rows: SettingsRow[], + userId: string | null, ): Promise> { // 2. cascade walk — OS_* env (handled by callers) > global > tenant > user > default // @@ -1438,8 +1462,12 @@ export class SettingsService { } } - if (scope === 'user') { - const userRow = rows.find((r) => r.key === key && r.scope === 'user'); + // The user rung answers its OWNER only. The pick compares `user_id` itself + // rather than trusting the row set to be the caller's alone, and a caller + // that names no user gets no user rung: the walk falls through to tenant, + // global, then the default, exactly as if no user row existed. + if (scope === 'user' && userId !== null) { + const userRow = rows.find((r) => r.key === key && r.scope === 'user' && r.user_id === userId); if (userRow) { chain.push({ scope: 'user', @@ -1674,6 +1702,30 @@ export class SettingsService { return null; } + /** + * The per-key entry {@link setMany} refuses a user-scoped key with when the + * caller names no user. `invalid_value` is ADR-0114's declared slot for + * "rejected for a reason no other member names" (the closed catalog has no + * member for a missing owner, and a service does not invent one); the + * constraint carries the declared scope, spelled as the specifier property + * it comes from. + */ + private ownerlessUserKeyError(reg: RegisteredManifest, key: string): FieldError { + const spec = ((reg.manifest.specifiers ?? []) as Array>) + .find((s) => s.key === key); + const label = typeof spec?.label === 'string' ? spec.label : key; + return { + field: key, + code: 'invalid_value', + message: + `${label} is a per-user setting, and this write names no user to store it for. ` + + 'Pass the user id of the person the value belongs to (`SettingsContext.userId`); ' + + "a value meant for everyone belongs on a key declared at scope 'tenant' or 'global'.", + label, + constraint: { scope: 'user' }, + }; + } + /** Persist a single key. Throws SettingsLockedError when env-locked. */ async set( namespace: string, @@ -1684,7 +1736,16 @@ export class SettingsService { return (await this.setMany(namespace, { [key]: value }, ctx))[key]; } - /** Persist multiple keys atomically (best-effort). */ + /** + * Persist multiple keys atomically (best-effort). + * + * A key declared `scope: 'user'` is written for one user, the caller + * ({@link callerUserIdOf}). A batch that writes one with no user id is + * refused WHOLE, before any write, with a {@link SettingsValidationError} + * naming each such key (`invalid_value`, `constraint: { scope: 'user' }`): + * the row it would store names no owner, and the user rung answers only its + * owner, so no reader would ever see it while the write reported success. + */ async setMany( namespace: string, patch: Record, @@ -1701,7 +1762,10 @@ export class SettingsService { // missing — the write path previously trusted a spoofable header identity). this.assertPermitted(reg.manifest, 'write', ctx); - // Pre-flight: reject the whole batch if any key is locked or unknown. + // Pre-flight: reject the whole batch if any key is locked or unknown, or + // is user-scoped while the caller names no user (collected, refused below). + const callerUserId = callerUserIdOf(ctx); + const ownerless: FieldError[] = []; for (const key of Object.keys(patch)) { if (!reg.scopes.has(key)) throw new UnknownKeyError(namespace, key); // An env override pins the key against writes — but only one that is IN @@ -1720,7 +1784,11 @@ export class SettingsService { // scope as the lock is still permitted (i.e. a platform admin // can edit a globally-locked value; a tenant admin cannot). const scope = reg.scopes.get(key)!; - const rows = await this.loadRows(namespace, scope === 'user' ? ctx.userId ?? null : null); + if (scope === 'user' && callerUserId === null) { + ownerless.push(this.ownerlessUserKeyError(reg, key)); + continue; + } + const rows = await this.loadRows(namespace, scope === 'user' ? callerUserId : null); const upper = rows.find( (r) => r.key === key && @@ -1731,6 +1799,9 @@ export class SettingsService { throw new SettingsLockedError(namespace, key, `locked-by-${upper.scope}`); } } + if (ownerless.length > 0) { + throw new SettingsValidationError(namespace, ownerless); + } // Reject writes that would leave the namespace in a known-broken // state (visible required field empty / pattern mismatch). See @@ -1761,9 +1832,10 @@ export class SettingsService { const scope = reg.scopes.get(key)!; // global rows are deployment-wide and land in `sys_platform_setting`, // which has no organization and no user column (ADR-0131 D7, see - // `rowIdentity`); user rows pin to ctx.userId; tenant rows leave user_id + // `rowIdentity`); user rows pin to the caller's user id, which the + // pre-flight above guarantees is present; tenant rows leave user_id // null and let the engine's tenant scoping fill in tenant_id from ctx. - const userId = scope === 'user' ? ctx.userId ?? null : null; + const userId = scope === 'user' ? callerUserId : null; const isEncrypted = reg.encryptedKeys.has(key); const isNull = rawValue === null || typeof rawValue === 'undefined'; diff --git a/packages/services/service-settings/src/settings-user-rung-owner.pin.test.ts b/packages/services/service-settings/src/settings-user-rung-owner.pin.test.ts new file mode 100644 index 00000000000..5ef1bc76a23 --- /dev/null +++ b/packages/services/service-settings/src/settings-user-rung-owner.pin.test.ts @@ -0,0 +1,274 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The user rung belongs to one user. + * + * A key declared `scope: 'user'` is stored per user: its rows carry the + * `user_id` of the person whose preference they are. Two halves keep that + * true at the service boundary, and each is pinned here on both stores the + * service can run over (an engine-bound `sys_setting`, and the no-engine + * in-memory fallback): + * + * - **Read.** The user rung answers only a caller whose user id is present + * and equals the row's `user_id`. A resolve with no user id falls through + * to the tenant rung, then global, then the manifest default, exactly as if + * no user row existed. That holds on every public read path that reaches + * the cascade (`get`, `getMany`, `getNamespace`, `createClient`, + * `runAction`, `resetNamespace`). + * - **Write.** `set` / `setMany` refuse a user-scoped key with no user id, + * with a typed refusal (`SETTINGS_VALIDATION`, the key named in `fields`) + * before any write, so the store never holds a user row with no owner. + * + * Controls: a caller with a user id reads and writes only their own row, and + * tenant / global keys answer and write the same with or without a user id. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SettingsService } from './settings-service.js'; +import { SettingsValidationError, type SettingsContext } from './settings-service.types.js'; + +type Row = Record; + +const NS = 'prefs_owner'; +const USER_A = 'usr_owner_a'; +const USER_B = 'usr_owner_b'; + +const MANIFEST = { + namespace: NS, + version: 1, + label: 'Preferences', + specifiers: [ + { type: 'text', key: 'theme', label: 'Theme', scope: 'user', default: 'system' }, + { type: 'text', key: 'density', label: 'Density', scope: 'tenant', default: 'comfortable' }, + { type: 'text', key: 'banner', label: 'Banner', scope: 'global', default: 'none' }, + ], +} as any; + +// The service's stored row shape, per rung. A user row with a `null` owner is +// the shape an ownerless write stored before the write half refused it; a +// database may still hold one. +const userRow = (userId: string | null, value: string): Row => ({ + namespace: NS, key: 'theme', scope: 'user', user_id: userId, value, + value_enc: null, encrypted: false, locked: false, locked_reason: null, +}); +const PEOPLE = [userRow(null, 'ownerless'), userRow(USER_A, 'dark'), userRow(USER_B, 'light')]; +const tenantRow = (key: string, value: string): Row => ({ + namespace: NS, key, scope: 'tenant', user_id: null, value, + value_enc: null, encrypted: false, locked: false, locked_reason: null, +}); +const globalRow = (key: string, value: string): Row => ({ + namespace: NS, key, scope: 'global', user_id: null, value, + value_enc: null, encrypted: false, locked: false, locked_reason: null, +}); + +// `$or` is the one combinator the settings reads write; any other is refused, +// so a predicate this double cannot express never reads as "no such row". +function matches(row: Row, where: Row): boolean { + return Object.entries(where).every(([k, v]) => { + if (k === '$or') return (v as Row[]).some((b) => matches(row, b)); + if (k.startsWith('$')) throw new Error(`fake engine: unimplemented combinator ${k}`); + return (row[k] ?? null) === (v ?? null); + }); +} + +/** + * A `SettingsEngine`-shaped double over two stores: the global rung's + * `sys_platform_setting` (no `scope`, no `user_id`) and `sys_setting` for the + * tenant and user rungs. Every user id resolves in `sys_user`, so the + * service's own user-reference refusal never decides a case here. + */ +function engineStack(seed: Row[], opts: { overAnswer?: boolean } = {}) { + const stores: Record = { + sys_platform_setting: seed + .filter((r) => r.scope === 'global') + .map(({ scope: _scope, user_id: _userId, ...stored }) => ({ ...stored })), + sys_setting: seed.filter((r) => r.scope !== 'global').map((r) => ({ ...r })), + }; + const find = vi.fn(async (object: string, query: any) => { + if (object === 'sys_user') return [{ id: query?.where?.id }]; + // `overAnswer`: a store that hands back every row of the namespace to any + // load, whatever else the predicate asked for. + const where = opts.overAnswer ? { namespace: query?.where?.namespace } : query?.where ?? {}; + const hits = (stores[object] ?? []).filter((r) => matches(r, where)); + return typeof query?.limit === 'number' ? hits.slice(0, query.limit) : hits; + }); + const insert = vi.fn(async (object: string, data: Row) => { + (stores[object] ??= []).push({ ...data }); + return { ...data }; + }); + const engine = { find, insert, update: vi.fn(), delete: vi.fn(), count: vi.fn() }; + const svc = new SettingsService({ env: {} }); + svc.registerManifest(MANIFEST); + svc.bindEngine(engine as any); + return { + svc, + wrote: () => insert.mock.calls.length + engine.update.mock.calls.length, + snapshot: () => JSON.stringify(stores), + }; +} + +/** The no-engine in-memory fallback, seeded store-level. */ +function memoryStack(seed: Row[]) { + const svc = new SettingsService({ env: {} }); + svc.registerManifest(MANIFEST); + const memory = (svc as any).memory as Row[]; + memory.push(...seed.map((r) => ({ ...r }))); + return { + svc, + wrote: () => (memory.length === seed.length ? 0 : 1), + snapshot: () => JSON.stringify(memory), + }; +} + +const STACKS = [ + ['engine-bound', engineStack], + ['in-memory', memoryStack], +] as const; + +/** Each public read path that reaches the cascade, answering `theme` for `ctx`. */ +const READ_PATHS: Array<[string, (svc: SettingsService, ctx: SettingsContext) => Promise]> = [ + ['get', async (svc, ctx) => (await svc.get(NS, 'theme', ctx)).value], + ['getMany', async (svc, ctx) => (await svc.getMany(NS, ['theme', 'density'], ctx)).theme.value], + ['getNamespace', async (svc, ctx) => (await svc.getNamespace(NS, ctx)).values.theme.value], + ['createClient', async (svc, ctx) => { + const client = await svc.createClient(NS, { ctx }); + try { return client.current.theme; } finally { client.dispose(); } + }], + ['runAction', async (svc, ctx) => { + let seen: unknown; + svc.registerAction(NS, 'probe', async ({ values }) => { + seen = values.theme; + return { ok: true, severity: 'info', message: 'ok' }; + }); + await svc.runAction(NS, 'probe', {}, ctx); + return seen; + }], +]; + +/** Contexts that carry no user id. */ +const NO_USER: Array<[string, SettingsContext]> = [ + ['an empty context', {}], + ['a tenant-only context', { tenantId: 'org_1' }], + ['an empty-string user id', { userId: '' }], +]; + +describe('the user rung answers only its owner', () => { + describe.each(STACKS)('%s', (_label, stack) => { + describe.each(READ_PATHS)('%s', (_path, read) => { + it.each(NO_USER)('a resolve with no user id never answers a user-scoped row (%s)', async (_c, ctx) => { + const { svc } = stack(PEOPLE); + expect(await read(svc, ctx)).toBe('system'); + }); + + it('control: a caller with a user id reads only their own row', async () => { + const { svc } = stack(PEOPLE); + expect(await read(svc, { userId: USER_A })).toBe('dark'); + expect(await read(svc, { userId: USER_B })).toBe('light'); + expect(await read(svc, { userId: 'usr_owner_c' })).toBe('system'); + }); + }); + + it('with no user id, a user-scoped key falls through to the tenant rung, then global, then the default', async () => { + const users = PEOPLE; + + const withTenant = await stack([...users, tenantRow('theme', 'tenant-theme')]).svc.get(NS, 'theme'); + expect(withTenant).toMatchObject({ value: 'tenant-theme', source: 'tenant' }); + expect(withTenant.cascadeChain?.map((e) => e.scope)).toEqual(['tenant', 'default']); + + const withGlobal = await stack([...users, globalRow('theme', 'global-theme')]).svc.get(NS, 'theme'); + expect(withGlobal).toMatchObject({ value: 'global-theme', source: 'global' }); + expect(withGlobal.cascadeChain?.map((e) => e.scope)).toEqual(['global', 'default']); + + const bare = await stack(users).svc.get(NS, 'theme'); + expect(bare).toMatchObject({ value: 'system', source: 'default' }); + expect(bare.cascadeChain?.map((e) => e.scope)).toEqual(['default']); + }); + + it('control: the owner still sees their row above the tenant rung, as before', async () => { + const { svc } = stack([userRow(USER_A, 'dark'), tenantRow('theme', 'tenant-theme')]); + const got = await svc.get(NS, 'theme', { userId: USER_A }); + expect(got.cascadeChain?.map((e) => e.scope)).toEqual(['tenant', 'user', 'default']); + expect(got.cascadeChain?.find((e) => e.scope === 'user')?.value).toBe('dark'); + }); + + it('a reset with no user id counts no user row and writes nothing', async () => { + const { svc, wrote, snapshot } = stack(PEOPLE); + const before = snapshot(); + expect(await svc.resetNamespace(NS)).toBe(0); + expect(wrote()).toBe(0); + expect(snapshot()).toBe(before); + }); + + it('control: tenant and global keys resolve the same with or without a user id', async () => { + const { svc } = stack([tenantRow('density', 'compact'), globalRow('banner', 'maintenance')]); + for (const ctx of [{}, { tenantId: 'org_1' }, { userId: USER_A }] as SettingsContext[]) { + expect(await svc.get(NS, 'density', ctx)).toMatchObject({ value: 'compact', source: 'tenant' }); + expect(await svc.get(NS, 'banner', ctx)).toMatchObject({ value: 'maintenance', source: 'global' }); + } + }); + }); +}); + +describe('the user rung compares the owner itself, whatever rows the store answers', () => { + it.each(READ_PATHS)('%s', async (_path, read) => { + const { svc } = engineStack(PEOPLE, { overAnswer: true }); + expect(await read(svc, { userId: USER_B })).toBe('light'); + expect(await read(svc, { userId: USER_A })).toBe('dark'); + expect(await read(svc, { userId: 'usr_owner_c' })).toBe('system'); + expect(await read(svc, {})).toBe('system'); + }); +}); + +describe('a user-scoped write names its owner', () => { + describe.each(STACKS)('%s', (_label, stack) => { + it.each(NO_USER)('a write of a user-scoped key with no user id is refused and stores nothing (%s)', async (_c, ctx) => { + const { svc, wrote, snapshot } = stack([]); + const events: unknown[] = []; + svc.subscribe(NS, (e) => events.push(e)); + const before = snapshot(); + + for (const value of ['dark', null]) { + const err: unknown = await svc.set(NS, 'theme', value, ctx).then(() => null, (e: unknown) => e); + // The envelope, not the bare throw: a service-layer error class carries + // `code` and the per-key `fields`, and no HTTP `status`. + expect(err).toBeInstanceOf(SettingsValidationError); + expect((err as SettingsValidationError).code).toBe('SETTINGS_VALIDATION'); + expect((err as SettingsValidationError).fields).toEqual([ + expect.objectContaining({ field: 'theme', code: 'invalid_value', constraint: { scope: 'user' } }), + ]); + } + + expect(wrote()).toBe(0); + expect(snapshot()).toBe(before); + expect(events).toEqual([]); + }); + + it('the refusal covers the whole batch: a tenant key beside it is not written either', async () => { + const { svc, wrote, snapshot } = stack([]); + const before = snapshot(); + const err: unknown = await svc + .setMany(NS, { density: 'compact', theme: 'dark' }, { tenantId: 'org_1' }) + .then(() => null, (e: unknown) => e); + expect(err).toBeInstanceOf(SettingsValidationError); + expect((err as SettingsValidationError).code).toBe('SETTINGS_VALIDATION'); + expect((err as SettingsValidationError).fields.map((f) => f.field)).toEqual(['theme']); + expect(wrote()).toBe(0); + expect(snapshot()).toBe(before); + }); + + it("control: a write with a user id stores that user's row, and another user does not read it", async () => { + const { svc, wrote } = stack([]); + const got = await svc.set(NS, 'theme', 'dark', { userId: USER_A }); + expect(got).toMatchObject({ value: 'dark', source: 'user' }); + expect(wrote()).toBe(1); + expect((await svc.get(NS, 'theme', { userId: USER_A })).value).toBe('dark'); + expect((await svc.get(NS, 'theme', { userId: USER_B })).value).toBe('system'); + }); + + it('control: tenant and global keys write the same with no user id', async () => { + const { svc } = stack([]); + expect(await svc.set(NS, 'density', 'compact')).toMatchObject({ value: 'compact', source: 'tenant' }); + expect(await svc.set(NS, 'banner', 'maintenance')).toMatchObject({ value: 'maintenance', source: 'global' }); + }); + }); +});