Skip to content

Commit 7518ee3

Browse files
fix(service-settings)!: the user rung answers only its owner; a user-scoped write must name its user (#22246)
Fixes #22168 Clause-②: no (narrowing) `SettingsService` now treats the user rung as belonging to one user, on both its read side and its write side. This is the defensive fix the seat review on the card ruled, in two halves. One file changes (`packages/services/service-settings/src/settings-service.ts`), with a pin file and a `minor` changeset that declares the narrowing. ## What changes **Read half: `resolveKeyFromRows`.** The cascade walk now takes the caller's user id. The user rung answers only a row whose `user_id` equals that id. 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, exactly as if no user row existed. `get` and `getMany` pass the id through, with one helper (`callerUserIdOf`) as the single definition of "present". `getNamespace`, `createClient`, `runAction`, `resetNamespace` and `setMany`'s own re-resolve all reach the cascade through `get` or `getMany`, so every read path gets the same answer. The engine-bound store and the no-engine in-memory fallback share the pick, so both are covered. **Write half: `setMany`, ruled A.** A key declared `scope: 'user'` in a batch whose context names no user is refused with `SettingsValidationError` (`SETTINGS_VALIDATION`). Each such key gets one `fields` entry: `field` names the key, `code: 'invalid_value'` and `constraint: { scope: 'user' }`. The check runs in the pre-flight, before validation, before any row load for that key and before any write, and it refuses the whole batch. A `null` reset of such a key is refused too: the only row it could touch has no owner, and no reader returns that row any more. **Unchanged:** - A caller with a user id reads and writes only their own rows. - Tenant and global keys resolve and write the same with or without a user id. - The settings HTTP routes are unaffected. Their context comes from the verified authz resolution, which either names a user or carries no permissions, and the manifest permission gate runs before any row load. - The tenant rung's organization comparison is untouched. #15211 is not addressed here. ## Why `SETTINGS_VALIDATION` and not a new code I checked the service's existing error vocabulary first, as the dispatch asked. `SettingsValidationError` already refuses a whole batch before any write, with one `FieldError` per key. It also already carries a refusal that is not about the submitted value: the manifest-side `visible` predicate, under `invalid_value`, ADR-0114's slot for "rejected for a reason no other member names". `SETTINGS_FORBIDDEN` is defined as the HTTP-boundary capability refusal, and its message names a capability. `SETTINGS_LOCKED` drives the lock affordances. A new code would also need a ledger row in `packages/spec`, which is outside this card's fence. ## Reach (the round-1 census, re-read on this base) The shipped settings manifests declare no `scope: 'user'` key at manifest or specifier level. That was true at the claim's base and still is at `0e9371f0c6`. So no in-repo caller meets either half today. The in-process writers are the HTTP routes and the service's own `set` and `resetNamespace`, and the HTTP door always carries a user. The change is latent and defensive. It takes effect the first time a manifest declares a user-scoped key. ## Tests Measured at `fd24546953`. Every run went through `scripts/pm/os-verify-lock.sh`. - New pin file `settings-user-rung-owner.pin.test.ts`, 65 cases, run on both stores: - a resolve with no user id never answers a user-scoped row, on each read path (`get`, `getMany`, `getNamespace`, `createClient`, `runAction`), for an empty context, a tenant-only context and an empty-string user id; - the fall-through order (tenant, then global, then the default) for a resolve with no user id; - a reset with no user id counts no user row and writes nothing; - the user-rung pick compares the owner itself, even when the store returns more rows than the query asked for; - a write of a user-scoped key with no user id is refused, with the envelope (`code` plus `fields`), stores nothing and emits no change event, and a mixed batch is refused whole; - controls: per-user reads and writes, the owner's row above the tenant rung, and tenant and global keys with and without a user id. - Before the fix, on the base source: 34 failed and 26 passed. Every negative pin was red and every control was green. One control was split before commit because it also asserted a read with no user. The two load-bearing pins were added in the second commit; ablations M1 and M2 below make them red. - With the fix: `pnpm --filter @objectstack/service-settings exec vitest run --maxWorkers=2` gave 39 files and 702 tests passed. `pnpm --filter @objectstack/service-settings typecheck` gave exit 0, and `--listFiles` shows the pin file is part of that program. - Ablation, run from the committed state through `scripts/ablation-replace.mjs` (anchor hit once, blob changed, restore proven equal to HEAD, `git diff HEAD` empty), with a bash `trap` restore as well. Every leg turned red, and no control failed: - M1, the presence check removed from the user-rung pick: 39 of 65 failed. These were the no-user read pins on both stores, the fall-through, the reset and the over-answer pin. An ownerless legacy row is what makes this check load-bearing. - M2, the owner comparison removed: 5 of 65 failed. These were the over-answer pin on each read path. With the current loaders a present user id only loads its own rows, so this is the pin that holds the comparison. - M12, both removed: 39 of 65 failed. - M3, the refusal disarmed: 8 of 65 failed. These were the no-user write pins on both stores and the batch pin. ## Acceptance notes - **The load with no user id still reads every user's user rows** (`loadScopedRows` and the in-memory filter). With the pick narrowed, those rows are inert for the cascade. Narrowing the load itself is defence in depth and is outside this card's fence. Carrier: none. - **Ownerless user rows** written before this change, by a write with no user id, stay in the store. No reader returns them now. The service cannot clear one either, because a `null` write with no user id is refused. No in-repo writer could have produced one, since no shipped manifest declares a user-scoped key. - **`resetNamespace` on a user-scoped key whose effective value comes from the tenant or global rung** writes `null` at the key's declared (user) rung, so the upper value stays in force even though it is counted as cleared. This was there before this change and is a reading, not measured. With no user id, that path now meets the write refusal instead of storing an ownerless row. Carrier: none. - **Docs:** the "Writes fail loudly" table in `content/docs/protocol/kernel/config-resolution.mdx` has no row for this refusal. That file is outside this card's fence. Carrier: none. --- _Generated by [Claude Code](https://claude.ai/code/session_01WkL6Eijt432S1Y7ekb6ovQ)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent dbb7be2 commit 7518ee3

3 files changed

Lines changed: 377 additions & 12 deletions

File tree

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
---
2+
'@objectstack/service-settings': minor
3+
---
4+
5+
fix(service-settings)!: the user rung answers only its owner, and a write of a user-scoped key must name its user
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) A runtime narrowing inside SettingsService, not a metadata change: no spec key, export, option, response field or stored shape is removed, renamed or re-shaped, so there is no tombstone and nothing for `objectstack migrate meta` to rewrite. What narrows is the service's accept set: `set` and `setMany` now refuse a key declared `scope: 'user'` when the caller's context names no user. The other categories are closed on facts: the package publishes (not unpublished); no ADR-0087 id covers this service and this diff adds none (not registered / already-registered); and no published interface or type changes (not runtime-interface-only / type-surface-only). -->
10+
11+
**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes.
12+
13+
A settings key declared `scope: 'user'` belongs to one user. `SettingsService` now holds that on both its read and its write side.
14+
15+
- **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`.
16+
- **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.
17+
- **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.
18+
19+
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.

‎packages/services/service-settings/src/settings-service.ts‎

Lines changed: 84 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,21 @@ const PLATFORM_SETTING_OBJECT = 'sys_platform_setting';
106106
*/
107107
const SETTINGS_SYSTEM_CONTEXT = Object.freeze({ isSystem: true as const });
108108

109+
/**
110+
* The caller's user id, or `null` when the context names no user.
111+
*
112+
* The user rung is per owner: a `scope: 'user'` row belongs to the user its
113+
* `user_id` names. So the rung answers a resolve only for that same user
114+
* ({@link SettingsService.resolveKeyFromRows}), and a write of a user-scoped
115+
* key needs a user to pin the row to ({@link SettingsService.setMany} refuses
116+
* one without). An absent or empty id names nobody, and both read as "no user"
117+
* here: the read falls through to the tenant rung, then global, then the
118+
* default, and the write is refused.
119+
*/
120+
function callerUserIdOf(ctx: SettingsContext): string | null {
121+
return typeof ctx.userId === 'string' && ctx.userId !== '' ? ctx.userId : null;
122+
}
123+
109124
/**
110125
* Value-bearing specifier types — drives which entries we expect to
111126
* find in the K/V store. Keeps the resolver in sync with the spec
@@ -1283,8 +1298,10 @@ export class SettingsService {
12831298
const scope = reg.scopes.get(key)!;
12841299
// For 'user' scope we pre-filter by user_id; for 'tenant' and 'global'
12851300
// we load everything for the namespace and pick the right row below.
1286-
const rows = await this.loadRows(namespace, scope === 'user' ? ctx.userId ?? null : null);
1287-
return this.resolveKeyFromRows<T>(reg, key, scope, rows);
1301+
// The user rung's pick compares the owner too (see `resolveKeyFromRows`).
1302+
const userId = scope === 'user' ? callerUserIdOf(ctx) : null;
1303+
const rows = await this.loadRows(namespace, userId);
1304+
return this.resolveKeyFromRows<T>(reg, key, scope, rows, userId);
12881305
}
12891306

12901307
/**
@@ -1379,18 +1396,19 @@ export class SettingsService {
13791396
// [ADR-0131 D7] One load per required `loadRows` argument, as before —
13801397
// and the global rung, which no argument changes, is read ONCE for both
13811398
// (see `loadRowSets`).
1399+
const userId = callerUserIdOf(ctx);
13821400
const groups: Array<string | null> = [
1383-
...(userKeys.length > 0 ? [ctx.userId ?? null] : []),
1401+
...(userKeys.length > 0 ? [userId] : []),
13841402
...(otherKeys.length > 0 ? [null] : []),
13851403
];
13861404
const sets = await this.loadRowSets(namespace, groups);
13871405
const userRows = userKeys.length > 0 ? sets[0] : [];
13881406
const otherRows = otherKeys.length > 0 ? sets[sets.length - 1] : [];
13891407
for (const { key, scope } of userKeys) {
1390-
out[key] = await this.resolveKeyFromRows(reg, key, scope, userRows);
1408+
out[key] = await this.resolveKeyFromRows(reg, key, scope, userRows, userId);
13911409
}
13921410
for (const { key, scope } of otherKeys) {
1393-
out[key] = await this.resolveKeyFromRows(reg, key, scope, otherRows);
1411+
out[key] = await this.resolveKeyFromRows(reg, key, scope, otherRows, null);
13941412
}
13951413
}
13961414
return out;
@@ -1401,12 +1419,18 @@ export class SettingsService {
14011419
* verbatim from {@link get} for #10826 so `get`, `getMany` and
14021420
* `getNamespace` share ONE implementation of the resolution order
14031421
* (env is handled by the callers BEFORE the row load, exactly as before).
1422+
*
1423+
* `userId` is the caller's ({@link callerUserIdOf}), or `null` for a caller
1424+
* that names no user and for every key not declared `scope: 'user'`. The user
1425+
* rung answers only a row whose `user_id` equals it; with `null` there is no
1426+
* user rung at all.
14041427
*/
14051428
private async resolveKeyFromRows<T = unknown>(
14061429
reg: RegisteredManifest,
14071430
key: string,
14081431
scope: SpecifierScope,
14091432
rows: SettingsRow[],
1433+
userId: string | null,
14101434
): Promise<ResolvedSettingValue<T>> {
14111435
// 2. cascade walk — OS_* env (handled by callers) > global > tenant > user > default
14121436
//
@@ -1438,8 +1462,12 @@ export class SettingsService {
14381462
}
14391463
}
14401464

1441-
if (scope === 'user') {
1442-
const userRow = rows.find((r) => r.key === key && r.scope === 'user');
1465+
// The user rung answers its OWNER only. The pick compares `user_id` itself
1466+
// rather than trusting the row set to be the caller's alone, and a caller
1467+
// that names no user gets no user rung: the walk falls through to tenant,
1468+
// global, then the default, exactly as if no user row existed.
1469+
if (scope === 'user' && userId !== null) {
1470+
const userRow = rows.find((r) => r.key === key && r.scope === 'user' && r.user_id === userId);
14431471
if (userRow) {
14441472
chain.push({
14451473
scope: 'user',
@@ -1674,6 +1702,30 @@ export class SettingsService {
16741702
return null;
16751703
}
16761704

1705+
/**
1706+
* The per-key entry {@link setMany} refuses a user-scoped key with when the
1707+
* caller names no user. `invalid_value` is ADR-0114's declared slot for
1708+
* "rejected for a reason no other member names" (the closed catalog has no
1709+
* member for a missing owner, and a service does not invent one); the
1710+
* constraint carries the declared scope, spelled as the specifier property
1711+
* it comes from.
1712+
*/
1713+
private ownerlessUserKeyError(reg: RegisteredManifest, key: string): FieldError {
1714+
const spec = ((reg.manifest.specifiers ?? []) as Array<Record<string, unknown>>)
1715+
.find((s) => s.key === key);
1716+
const label = typeof spec?.label === 'string' ? spec.label : key;
1717+
return {
1718+
field: key,
1719+
code: 'invalid_value',
1720+
message:
1721+
`${label} is a per-user setting, and this write names no user to store it for. ` +
1722+
'Pass the user id of the person the value belongs to (`SettingsContext.userId`); ' +
1723+
"a value meant for everyone belongs on a key declared at scope 'tenant' or 'global'.",
1724+
label,
1725+
constraint: { scope: 'user' },
1726+
};
1727+
}
1728+
16771729
/** Persist a single key. Throws SettingsLockedError when env-locked. */
16781730
async set(
16791731
namespace: string,
@@ -1684,7 +1736,16 @@ export class SettingsService {
16841736
return (await this.setMany(namespace, { [key]: value }, ctx))[key];
16851737
}
16861738

1687-
/** Persist multiple keys atomically (best-effort). */
1739+
/**
1740+
* Persist multiple keys atomically (best-effort).
1741+
*
1742+
* A key declared `scope: 'user'` is written for one user, the caller
1743+
* ({@link callerUserIdOf}). A batch that writes one with no user id is
1744+
* refused WHOLE, before any write, with a {@link SettingsValidationError}
1745+
* naming each such key (`invalid_value`, `constraint: { scope: 'user' }`):
1746+
* the row it would store names no owner, and the user rung answers only its
1747+
* owner, so no reader would ever see it while the write reported success.
1748+
*/
16881749
async setMany(
16891750
namespace: string,
16901751
patch: Record<string, unknown>,
@@ -1701,7 +1762,10 @@ export class SettingsService {
17011762
// missing — the write path previously trusted a spoofable header identity).
17021763
this.assertPermitted(reg.manifest, 'write', ctx);
17031764

1704-
// Pre-flight: reject the whole batch if any key is locked or unknown.
1765+
// Pre-flight: reject the whole batch if any key is locked or unknown, or
1766+
// is user-scoped while the caller names no user (collected, refused below).
1767+
const callerUserId = callerUserIdOf(ctx);
1768+
const ownerless: FieldError[] = [];
17051769
for (const key of Object.keys(patch)) {
17061770
if (!reg.scopes.has(key)) throw new UnknownKeyError(namespace, key);
17071771
// An env override pins the key against writes — but only one that is IN
@@ -1720,7 +1784,11 @@ export class SettingsService {
17201784
// scope as the lock is still permitted (i.e. a platform admin
17211785
// can edit a globally-locked value; a tenant admin cannot).
17221786
const scope = reg.scopes.get(key)!;
1723-
const rows = await this.loadRows(namespace, scope === 'user' ? ctx.userId ?? null : null);
1787+
if (scope === 'user' && callerUserId === null) {
1788+
ownerless.push(this.ownerlessUserKeyError(reg, key));
1789+
continue;
1790+
}
1791+
const rows = await this.loadRows(namespace, scope === 'user' ? callerUserId : null);
17241792
const upper = rows.find(
17251793
(r) =>
17261794
r.key === key &&
@@ -1731,6 +1799,9 @@ export class SettingsService {
17311799
throw new SettingsLockedError(namespace, key, `locked-by-${upper.scope}`);
17321800
}
17331801
}
1802+
if (ownerless.length > 0) {
1803+
throw new SettingsValidationError(namespace, ownerless);
1804+
}
17341805

17351806
// Reject writes that would leave the namespace in a known-broken
17361807
// state (visible required field empty / pattern mismatch). See
@@ -1761,9 +1832,10 @@ export class SettingsService {
17611832
const scope = reg.scopes.get(key)!;
17621833
// global rows are deployment-wide and land in `sys_platform_setting`,
17631834
// which has no organization and no user column (ADR-0131 D7, see
1764-
// `rowIdentity`); user rows pin to ctx.userId; tenant rows leave user_id
1835+
// `rowIdentity`); user rows pin to the caller's user id, which the
1836+
// pre-flight above guarantees is present; tenant rows leave user_id
17651837
// null and let the engine's tenant scoping fill in tenant_id from ctx.
1766-
const userId = scope === 'user' ? ctx.userId ?? null : null;
1838+
const userId = scope === 'user' ? callerUserId : null;
17671839
const isEncrypted = reg.encryptedKeys.has(key);
17681840
const isNull = rawValue === null || typeof rawValue === 'undefined';
17691841

0 commit comments

Comments
 (0)