diff --git a/.changeset/sys-user-member-self-service-route.md b/.changeset/sys-user-member-self-service-route.md new file mode 100644 index 0000000000..57d2bff614 --- /dev/null +++ b/.changeset/sys-user-member-self-service-route.md @@ -0,0 +1,64 @@ +--- +"@objectstack/plugin-security": minor +--- + +feat(plugin-security): a rank-and-file member may edit their OWN `sys_user` row (#14959) + +Maintainer ruling 2026-09-03, decision batch #22, quoted verbatim and +untranslated as adopted: + +> 「同意」 + +The ruling that admitted `locale` to the ADR-0092 D2 column whitelist (#14787 / +PR #14958) opened **which columns** a permitted actor may touch. It did not open +**who**, and ADR-0092 D5 kept that with the permission layer, where +`member_default` still denied `allowEdit` on `sys_user`. The measured +consequence: a member's `PATCH /api/v1/data/sys_user/` was refused by the +object gate *before* the column guard was ever consulted, so `sys_user.locale` +shipped as a user-stated preference only a platform administrator could set — +with objectui#7501's "my language" form item waiting on a route that did not +exist, and #14788 having already ruled the stored value outranks +`Accept-Language` *because it is the user's own choice*. + +This opens the route, on the two axes that already existed and in the shape +`sys_api_key` has shipped since #8053: + +- **Which rows** — `member_default` gains an explicit `sys_user` entry + (`allowRead`/`allowEdit` true, create/delete **false**), and its + `sys_user_self` RLS carve-out (`id == current_user.id`) widens from `select` + to `all` so it reaches the by-id write pre-image check. `sys_user_org_members` + — the org-peer *visibility* policy — deliberately stays `select`-only: + policies OR-combine, so widening it would have composed + `id == me OR id IN ` and handed every member their + colleagues' profile rows. +- **Which columns** — unchanged. ADR-0092 D2's identity write guard still bounds + a user-context update to `SYS_USER_PROFILE_EDIT_FIELDS` + (`name`, `image`, `locale`); `email`, `role`, the ban columns and every system + stamp stay unwritable on this path. + +`allowCreate` / `allowDelete` stay false: accounts are minted and retired +through better-auth's own endpoints, and this set is bound to the `everyone` +anchor, which must remain anchor-safe (ADR-0090 D5). + +**ADR-0092 D5 is amended** by the same ruling — self-service edits of the +whitelisted columns route through the generic data path, with the D6 +`afterUpdate` hook as the session-cache refresh. `name` / `image` therefore +become editable there too, not only through better-auth `/update-user`. The +amendment ships as its own PR (`docs/adr/**` is governed and merged by hand). + +Rejected in the same ruling, recorded so they are not re-proposed: a dedicated +endpoint writing under system context (the "second stamping route" #14787's own +ruling rejected, one level up); leaving the column admin-only (a user-facing +setting only an administrator can set — ADR-0049's declared-not-reachable shape, +one step removed); and making `locale` a better-auth `additionalFields` entry +(#13881 measured that it breaks `getSession` on any environment that has not run +schema-sync). + +The pins are layer-attributed on purpose. Each of the four cases the ruling names +records *which* of the three layers produced its answer — object gate, row scope, +or identity guard — because before this change all four were refused by the +object gate, so "another member's row is refused" and "a non-whitelisted column +is refused by the guard" were both green while neither mechanism had run. A +two-leg ablation confirms it: reverting the permission-set entry drops the +non-whitelisted-column refusal from `identity-guard` to `object-gate`, and +reverting only the RLS widening drops it to `row-scope`. diff --git a/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts b/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts new file mode 100644 index 0000000000..d1aed3c796 --- /dev/null +++ b/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts @@ -0,0 +1,411 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The ROUTE contract for a member editing their own `sys_user` row — maintainer + * ruling 2026-09-03, decision batch #22, adopted 「同意」 (quoted verbatim and + * untranslated), which also AMENDED ADR-0092 D5. + * + * ## Why this file exists, and why it is not the sibling `sys-user-locale-write-contract` + * + * That file pins WHICH COLUMNS may be written. This one pins WHO may write them + * and TO WHICH ROW — the half D5 originally answered "nobody but a platform + * admin", and the half the amendment moved. The two are independent by + * construction (ADR-0092 D5), so neither file can stand in for the other: + * before this change the column was open and the route was shut, and every + * column-level pin was green the whole time. + * + * ## The failure mode this file is shaped against: a pin that passes at the + * ## wrong layer + * + * Three layers can refuse a member's `PATCH /api/v1/data/sys_user/`, in this + * order: + * + * 1. the CRUD **object gate** — `member_default.objects.sys_user.allowEdit`; + * 2. the **row scope** — the by-id write pre-image check, which re-reads the + * target row through the caller's compiled write-RLS filter and denies when + * it comes back empty (`sys_user_self`); + * 3. the ADR-0092 D2 **identity write guard** — an engine `beforeUpdate` hook + * that strips non-whitelisted columns and throws when nothing editable + * survives. + * + * A refusal assertion that does not say WHICH layer produced it proves almost + * nothing here, because layer 1 shadows the other two: before this change every + * one of these four cases was refused by the object gate, so "a member cannot + * edit someone else's row" and "a non-whitelisted column is refused by the + * guard" were both green while neither mechanism had run. That is the exact + * shape of the gap the card measured. + * + * So {@link route} returns the LAYER, established mechanically rather than + * inferred: + * + * - the middleware throws and `ql.findOne` was never called ⇒ `object-gate` + * (the pre-image re-read is the first thing past the CRUD check, so its + * absence dates the refusal); + * - the middleware throws and `ql.findOne` WAS called ⇒ `row-scope`; + * - the middleware passes and the guard hook throws ⇒ `identity-guard`. + * + * All four verdicts (`allowed` + the three layers) are produced by the cases + * below, which is this file's own non-vacuity control: a `route` that could only + * ever answer one of them would fail somewhere here. + * + * ## What is real and what is a fake + * + * Real: the shipped permission sets (`securityDefaultPermissionSets`), the real + * `SecurityPlugin` CRUD middleware, the real RLS compiler behind it, the real + * `SysUser` schema, the real identity write guard and its D6 companion hook. + * Faked: the storage engine (`findOne` answers out of a two-row fixture, by + * EVALUATING the filter the middleware actually composed) and better-auth's + * secondary storage. Nothing that decides authorization is faked. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { SysUser } from '@objectstack/platform-objects/identity'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { + registerIdentityWriteGuard, + registerManagedUpdateWhitelist, + type SecondaryStorageLike, +} from './identity-write-guard.js'; +import { SYS_USER_PROFILE_EDIT_FIELDS } from './sys-user-writable-fields.js'; + +// ── Fixture principals and rows ───────────────────────────────────────────── + +const ME = 'usr_me'; +const PEER = 'usr_peer'; + +/** A rank-and-file member: the `org_member` position, no app profile at all. */ +const MEMBER_CTX = { + userId: ME, + tenantId: 'org_1', + positions: ['org_member'], + permissions: [] as string[], + org_user_ids: [ME, PEER], +}; + +/** The two rows the fake storage holds. Same organization — colleagues. */ +const ROWS: Record> = { + [ME]: { id: ME, name: 'Me', email: 'me@example.com', locale: 'en-US', role: 'user' }, + [PEER]: { id: PEER, name: 'Peer', email: 'peer@example.com', locale: 'en-US', role: 'user' }, +}; + +// ── Filter evaluation ─────────────────────────────────────────────────────── + +/** + * Evaluate the `where` the middleware composed against a fixture row. + * + * Deliberately minimal, and it does NOT need to be general: the composed filter + * is itself asserted verbatim by the first test below, so this evaluator only + * has to be right about the shapes that appear there. It throws on anything + * else rather than guessing — a filter shape it does not understand must not + * silently read as "row visible". + */ +function matchesFilter(row: Record, filter: unknown): boolean { + if (filter == null) return true; + if (typeof filter !== 'object') throw new Error(`unhandled filter: ${String(filter)}`); + const f = filter as Record; + return Object.entries(f).every(([key, value]) => { + if (key === '$and') return (value as unknown[]).every((sub) => matchesFilter(row, sub)); + if (key === '$or') return (value as unknown[]).some((sub) => matchesFilter(row, sub)); + if (value && typeof value === 'object') { + const op = value as Record; + if (Array.isArray(op.$in)) return (op.$in as unknown[]).includes(row[key]); + throw new Error(`unhandled operator on '${key}': ${JSON.stringify(value)}`); + } + return row[key] === value; + }); +} + +// ── The route harness ─────────────────────────────────────────────────────── + +type Layer = 'object-gate' | 'row-scope' | 'identity-guard'; + +interface RouteResult { + /** `null` when the write was admitted end to end. */ + refusedBy: Layer | null; + error: any; + /** The payload as it stands after every layer ran (the guard strips in place). */ + data: Record; + /** Every `where` the pre-image re-read was called with, in order. */ + preImageWheres: unknown[]; + /** The secondary-storage writes the ADR-0092 D6 companion hook performed. */ + snapshotWrites: Array<{ key: string; value: any }>; +} + +/** + * Drive one `PATCH sys_user/` through the real middleware and then the + * real identity write guard, reporting which layer (if any) refused. + */ +async function route( + targetId: string, + patch: Record, + opts: { + context?: Record; + operation?: 'update' | 'insert' | 'delete'; + /** Seed better-auth's session cache so the D6 refresh has something to rewrite. */ + cachedSessionToken?: string; + } = {}, +): Promise { + const context = opts.context ?? MEMBER_CTX; + const operation = opts.operation ?? 'update'; + + // ── Layer 1 + 2: the real SecurityPlugin CRUD middleware ────────────────── + let middleware: any; + const preImageWheres: unknown[] = []; + const findOne = vi.fn(async (_object: string, query: any) => { + preImageWheres.push(query?.where); + const row = ROWS[targetId]; + if (!row) return null; + // The pre-image gate denies on `null`. Answering it by EVALUATING the + // filter the middleware built is what makes the row-scope layer a + // measurement rather than a stub: a filter that stopped naming the caller + // would let the peer row through here and redden the pin below. + return matchesFilter(row, query?.where) ? { ...row } : null; + }); + const ql = { + registerMiddleware: (mw: any) => { + if (!middleware) middleware = mw; + }, + getSchema: () => SysUser as any, + findOne, + }; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: ql, + metadata: { get: async () => SysUser as any, list: () => securityDefaultPermissionSets }, + 'org-scoping': { name: 'org-scoping' }, + }; + const pluginCtx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + getService: (name: string) => { + if (!(name in services)) throw new Error(`no service: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin(); + await plugin.init(pluginCtx); + await plugin.start(pluginCtx); + + const data: Record = { id: targetId, ...patch }; + const opCtx: any = { + object: 'sys_user', + operation, + data, + options: operation === 'insert' ? undefined : { where: { id: targetId } }, + context, + }; + + const snapshotWrites: Array<{ key: string; value: any }> = []; + try { + await middleware(opCtx, async () => {}); + } catch (error: any) { + return { + // The pre-image re-read is the first engine call past the CRUD gate, so + // "was it called" dates the refusal without reading the message. + refusedBy: findOne.mock.calls.length === 0 ? 'object-gate' : 'row-scope', + error, + data, + preImageWheres, + snapshotWrites, + }; + } + + // ── Layer 3: the real ADR-0092 D2 identity write guard ──────────────────── + const store = new Map(); + if (opts.cachedSessionToken) { + store.set( + `active-sessions-${targetId}`, + JSON.stringify([{ token: opts.cachedSessionToken, expiresAt: Date.now() + 3_600_000 }]), + ); + store.set( + opts.cachedSessionToken, + JSON.stringify({ + session: { id: 'ses_1', userId: targetId, expiresAt: new Date(Date.now() + 3_600_000).toISOString() }, + user: { id: targetId, name: ROWS[targetId]?.name, image: null, email: ROWS[targetId]?.email }, + }), + ); + } + const storage: SecondaryStorageLike = { + get: async (key) => store.get(key) ?? null, + set: async (key, value) => { + store.set(key, value); + snapshotWrites.push({ key, value: JSON.parse(value) }); + }, + delete: async (key) => void store.delete(key), + }; + + const handlers: Record Promise>> = {}; + const engine = { + getSchema: () => ({ name: 'sys_user', managedBy: SysUser.managedBy }), + registerHook: (event: string, handler: (ctx: any) => Promise) => { + (handlers[event] ??= []).push(handler); + }, + }; + registerManagedUpdateWhitelist('sys_user', SYS_USER_PROFILE_EDIT_FIELDS); + registerIdentityWriteGuard(engine as any, { + packageId: 'test.sys-user-self-service-route', + getSecondaryStorage: () => storage, + }); + + const hookCtx = { object: 'sys_user', session: context, input: { id: targetId, data } }; + const before = operation === 'insert' ? 'beforeInsert' : operation === 'delete' ? 'beforeDelete' : 'beforeUpdate'; + try { + for (const handler of handlers[before] ?? []) await handler(hookCtx); + } catch (error: any) { + return { refusedBy: 'identity-guard', error, data, preImageWheres, snapshotWrites }; + } + for (const handler of handlers.afterUpdate ?? []) await handler(hookCtx); + + return { refusedBy: null, error: null, data, preImageWheres, snapshotWrites }; +} + +// ── The composed write filter, read once and asserted verbatim ────────────── + +describe('sys_user self-service — the row scope the write actually composes', () => { + it('narrows a member’s by-id write to EXACTLY their own row', async () => { + const r = await route(ME, { locale: 'ja-JP' }); + expect(r.refusedBy).toBeNull(); + // One pre-image re-read, and its `where` is the `{id}` address ANDed with + // the compiled RLS parts. Asserted verbatim because every other assertion in + // this file leans on `matchesFilter` reading it correctly, and because it is + // the security property in one line: the caller's id, and nothing wider. + expect(r.preImageWheres).toHaveLength(1); + expect(r.preImageWheres[0]).toEqual({ $and: [{ id: ME }, { id: ME }] }); + }); + + it('the org-peer READ scope does not appear in the write filter', async () => { + // `sys_user_org_members` (`id in current_user.org_user_ids`) is what lets a + // member see colleagues at all. If it ever reached the write class, the + // filter above would carry an `$or` with a `$in` over `org_user_ids` and + // every member could edit every colleague. Asserted as an absence on the + // MEASURED filter rather than on the declaration, so a change anywhere in + // the composition path fails here. + const r = await route(ME, { name: 'Renamed' }); + // Non-vacuity: an absence assertion over an EMPTY list passes for free, and + // an object-gate refusal produces exactly that. Pin the write got as far as + // composing a filter before asserting what is not in it. + expect(r.refusedBy).toBeNull(); + expect(r.preImageWheres).toHaveLength(1); + expect(JSON.stringify(r.preImageWheres)).not.toContain('$in'); + expect(JSON.stringify(r.preImageWheres)).not.toContain(PEER); + }); +}); + +// ── The four pins the ruling names ────────────────────────────────────────── + +describe('sys_user self-service — the four pins, each attributed to a layer', () => { + it('PIN 1 — a member sets their OWN `locale`, and the value survives to the row', async () => { + const r = await route(ME, { locale: 'ja-JP' }); + expect(r.refusedBy, 'own-row locale must be admitted end to end').toBeNull(); + // Admitted is not enough: the guard strips in place, so a "success" that + // dropped the column would be a silent no-op at the driver. + expect(r.data).toEqual({ id: ME, locale: 'ja-JP' }); + }); + + it('PIN 2 — another member’s row is refused, and refused BY THE ROW SCOPE', async () => { + const r = await route(PEER, { locale: 'ja-JP' }); + expect(r.refusedBy).toBe('row-scope'); + // The discriminating half. Before this change the same expectation passed + // with `refusedBy === 'object-gate'` — the object bit was false, so the row + // scope never ran and this pin proved nothing about it. Now the pre-image + // re-read HAPPENED, was scoped to the caller, and came back empty. + expect(r.preImageWheres).toEqual([{ $and: [{ id: PEER }, { id: ME }] }]); + expect(r.error?.name).toBe('PermissionDeniedError'); + expect(r.error?.code).toBe('PERMISSION_DENIED'); + }); + + it('PIN 3 — `name` and `image` are editable on the generic path', async () => { + const r = await route(ME, { name: 'Renamed', image: 'https://example.com/a.png' }); + expect(r.refusedBy).toBeNull(); + expect(r.data).toEqual({ id: ME, name: 'Renamed', image: 'https://example.com/a.png' }); + }); + + it('PIN 3 — …and the ADR-0092 D6 session refresh is OBSERVED, not merely registered', async () => { + const r = await route(ME, { name: 'Renamed' }, { cachedSessionToken: 'tok_1' }); + expect(r.refusedBy).toBeNull(); + // The observable effect: better-auth's cached `{session, user}` snapshot is + // re-written with the new value, at the same key. Asserting that the hook is + // registered would pass over a hook that returned early on every input — + // which is precisely what it does when the mirror set excludes the column + // (see the `locale` case below). + const rewritten = r.snapshotWrites.filter((w) => w.key === 'tok_1'); + expect(rewritten).toHaveLength(1); + expect(rewritten[0].value.user).toMatchObject({ id: ME, name: 'Renamed' }); + // …and it REWRITES rather than deletes: the session survives the edit. + expect(rewritten[0].value.session).toMatchObject({ id: 'ses_1', userId: ME }); + }); + + it('PIN 3 — `locale` correctly does NOT touch the snapshot (D6 mirror ≠ whitelist)', async () => { + // Not a gap. better-auth carries no `locale` on its user model and it is + // deliberately not an `additionalFields` entry, so there is no stale cached + // copy to repair; merging one in would MANUFACTURE a key present only on + // sessions that happen to be cached. Pinned as an expected absence so a + // future reader does not "fix" it into an incoherence. + const r = await route(ME, { locale: 'ja-JP' }, { cachedSessionToken: 'tok_1' }); + expect(r.refusedBy).toBeNull(); + expect(r.snapshotWrites).toEqual([]); + }); + + it('PIN 4 — a non-whitelisted column is refused BY THE GUARD, not by the layers above it', async () => { + // The phantom-pin case, stated as the mechanism under test: `email` must get + // PAST the object gate and PAST the row scope — the caller is editing their + // own row, which both layers permit — and be stopped by the column + // whitelist. If either layer above refused instead, this pin would be green + // while the guard was dead code. + const r = await route(ME, { email: 'attacker@example.com' }); + expect(r.refusedBy).toBe('identity-guard'); + // The pre-image re-read ran and SUCCEEDED (the row was visible) — proof the + // first two layers admitted the write before the guard refused it. + expect(r.preImageWheres).toEqual([{ $and: [{ id: ME }, { id: ME }] }]); + // ADR-0112 envelope, both discriminators (the REST boundary derives 403 from + // `status`, and `mapDataError` keys on `code`). + expect(r.error?.code).toBe('PERMISSION_DENIED'); + expect(r.error?.status).toBe(403); + // The wording is load-bearing here: it is the only place the caller is told + // WHICH fields are editable, and it is what distinguishes this refusal from + // the two above in a log. + expect(r.error?.message).toContain('email'); + expect(r.error?.message).toContain('ADR-0092'); + expect(r.error?.message).toMatch(/Editable fields: .*locale/); + // And the column never reached the payload. + expect(r.data).toEqual({ id: ME }); + }); + + it('PIN 4 — the same holds for every other non-whitelisted column, one at a time', () => { + // A property over the tier list rather than one example, so a whitelist that + // grew by accident is caught here rather than in production. + const forbidden = ['email', 'role', 'banned', 'phone_number', 'email_verified', 'manager_id']; + for (const field of forbidden) { + expect(SYS_USER_PROFILE_EDIT_FIELDS.has(field), `${field} must not be self-editable`).toBe(false); + } + expect([...SYS_USER_PROFILE_EDIT_FIELDS].sort()).toEqual(['image', 'locale', 'name']); + }); + + it('PIN 4 — a mixed payload keeps the legal column and strips the rest, still admitted', async () => { + // The guard strips rather than refuses when SOMETHING editable survives. + // Worth pinning next to the refusal: the two behaviours are one branch apart + // and a change that made stripping silent-refuse (or refusal silent-strip) + // would be invisible to either test alone. + const r = await route(ME, { locale: 'zh-CN', role: 'admin' }); + expect(r.refusedBy).toBeNull(); + expect(r.data).toEqual({ id: ME, locale: 'zh-CN' }); + }); +}); + +// ── The axes the ruling did NOT open ──────────────────────────────────────── + +describe('sys_user self-service — create and delete stay shut, at the object gate', () => { + it.each([ + ['insert', 'insert' as const], + ['delete', 'delete' as const], + ])('a member’s %s on sys_user is refused before any row is read', async (_name, operation) => { + const r = await route(ME, { name: 'X' }, { operation }); + // These two cases are also this file's positive control for the + // `object-gate` verdict: without them, a `route` that could never answer + // `object-gate` would leave PIN 2 and PIN 4 unable to fail for the reason + // they are written to catch. + expect(r.refusedBy).toBe('object-gate'); + expect(r.preImageWheres).toEqual([]); + expect(r.error?.name).toBe('PermissionDeniedError'); + }); +}); diff --git a/packages/plugins/plugin-auth/src/sys-user-writable-fields.ts b/packages/plugins/plugin-auth/src/sys-user-writable-fields.ts index 4f9b3f55de..81d7bf1a9f 100644 --- a/packages/plugins/plugin-auth/src/sys-user-writable-fields.ts +++ b/packages/plugins/plugin-auth/src/sys-user-writable-fields.ts @@ -40,10 +40,17 @@ * would invent a field only cached sessions carry (see * `SESSION_SNAPSHOT_MIRRORED_FIELDS` in `identity-write-guard.ts`). * - * ⚠️ What this set does NOT decide is WHO. ADR-0092 D5 keeps that with the - * permission layer, and `member_default` still denies `allowEdit` on - * `sys_user`, so a rank-and-file member reaches this column through no shipped - * surface yet. That is a separate opening, deliberately not taken here. + * ⚠️ What this set does NOT decide is WHO — ADR-0092 D5 keeps that with the + * permission layer, and the two answers are independent. As of the 2026-09-03 + * ruling recorded on the D5 AMENDMENT, `member_default` names `sys_user` + * explicitly with `allowEdit: true`, row-scoped to the caller by the + * `sys_user_self` RLS carve-out (`plugin-security` + * `objects/default-permission-sets.ts`), so a rank-and-file member reaches + * these columns on their OWN row through the generic data path. Every other + * row, and every column not listed here, is still refused — by the RLS + * pre-image check and by this whitelist respectively, and the two refusals + * come from different layers. Widening THIS set does not widen who; widening + * the permission set does not widen which columns. */ /** Tier 1 — standard form / data-API editable (identity write guard whitelist). */ diff --git a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts index 382e0ff0e7..b7c51b0fd3 100644 --- a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts +++ b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts @@ -30,6 +30,19 @@ // Every OTHER change vs the pre-extraction snapshot is a same-visibility filter // simplification (duplicate-OR dedup; dead org-clause removal on non-tenant // objects) and is annotated inline. +// +// One LATER delta, authorized by its own ruling rather than by ADR-0095: +// (f) [#14959, maintainer ruling 2026-09-03 — ADR-0092 D5 amendment] every +// `better_auth` (`sys_user`) WRITE cell moves from `CRUD_DENY` to the +// caller's OWN ROW. `member_default` now names `sys_user` with +// `allowEdit: true` and its `sys_user_self` carve-out runs at +// `operation: 'all'`, so the CRUD gate admits the update and Layer 1 +// narrows it to `id == `. What this matrix shows, and the reason +// the cell is worth reading rather than just re-baselining: the widening +// is EXACTLY one row per principal — `org_admin` gets `oadmin` and not +// the org, `no_org_member` gets `u2` even with no active organization +// (`sys_user` is non-tenant, so Layer 0 is inert and cannot narrow it +// further). Nobody reaches anybody else's identity row on the write path. import { describe, it, expect, vi } from 'vitest'; import { derivePosture, POSTURE_RANK } from '@objectstack/core'; @@ -311,14 +324,29 @@ const EXPECTED_MATRIX: Record { } }); + // The two managed tables a member may UPDATE, and the reason each is not a + // hole in the "door is better-auth" rule. Both are self-service, both are + // narrowed on two axes no permission-set boolean can express, and both + // narrowings existed on that specific table BEFORE its edit bit flipped: + // + // - [#8053] `sys_api_key` — a member revokes their own personal key. The + // table is hand-rolled ObjectStack (better-auth's `apiKey` plugin is not + // loaded); rows are scoped by `sys_api_key_self` and columns by ADR-0092 + // D2's whitelist (`revoked` alone). + // - [#14959, maintainer ruling 2026-09-03] `sys_user` — a member edits + // their own profile row (`name` / `image` / `locale`). Rows are scoped by + // `sys_user_self`, columns by ADR-0092 D2's `SYS_USER_PROFILE_EDIT_FIELDS`. + // The ruling AMENDED ADR-0092 D5, which had said self-service stays on + // better-auth `/update-user` and that no RLS self-row edit carve-out + // would be built. + const UPDATABLE_MANAGED_OBJECTS = ['sys_api_key', 'sys_user']; + it('better-auth identity tables stay WRITE-DENIED (the door is better-auth, not CRUD)', () => { for (const object of BETTER_AUTH_MANAGED_OBJECTS) { expect(allows('insert', [MEMBER_DEFAULT], object), `${object} insert`).toBe(false); - // [#8053] `sys_api_key` is the ONE update exception: a member revokes - // their own personal key. It is not a hole in the "door is better-auth" - // rule — that table is hand-rolled ObjectStack (better-auth's `apiKey` - // plugin is not loaded), and the write is narrowed to the owner's rows by - // the `sys_api_key_self` RLS policy and to `revoked` by ADR-0092 D2's - // column whitelist. Neither narrowing is expressible as a permission-set - // boolean, which is why this axis has to be asserted per object here. expect(allows('update', [MEMBER_DEFAULT], object), `${object} update`).toBe( - object === 'sys_api_key', + UPDATABLE_MANAGED_OBJECTS.includes(object), ); expect(allows('delete', [MEMBER_DEFAULT], object), `${object} delete`).toBe(false); } }); - it('[#8053] the update exception is `sys_api_key` alone, and it does not leak onto the other axes', () => { + it('[#8053 / #14959] the update exceptions are those two alone, and neither leaks onto the other axes', () => { // Stated positively and separately so the loop above cannot be "fixed" by // widening the condition: every other managed table must still refuse - // update, and `sys_api_key` itself must still refuse insert and delete. + // update, and each exception must still refuse insert and delete. const alsoUpdatable = BETTER_AUTH_MANAGED_OBJECTS.filter( - (o) => o !== 'sys_api_key' && allows('update', [MEMBER_DEFAULT], o), + (o) => !UPDATABLE_MANAGED_OBJECTS.includes(o) && allows('update', [MEMBER_DEFAULT], o), ); expect(alsoUpdatable, 'no other managed identity table may become updatable').toEqual([]); - expect(allows('update', [MEMBER_DEFAULT], 'sys_api_key')).toBe(true); + for (const object of UPDATABLE_MANAGED_OBJECTS) { + expect(allows('update', [MEMBER_DEFAULT], object), `${object} update`).toBe(true); + expect(allows('find', [MEMBER_DEFAULT], object), `${object} read`).toBe(true); + } expect(allows('insert', [MEMBER_DEFAULT], 'sys_api_key'), 'minting stays POST /keys').toBe(false); expect(allows('delete', [MEMBER_DEFAULT], 'sys_api_key'), 'rows retire by revoking').toBe(false); - expect(allows('find', [MEMBER_DEFAULT], 'sys_api_key')).toBe(true); + // [#14959] Accounts are minted and retired through better-auth's own + // endpoints (sign-up, invite, admin remove-member) — the ruling opened the + // EDIT axis on `sys_user` and nothing else. + expect(allows('insert', [MEMBER_DEFAULT], 'sys_user'), 'sign-up is better-auth\'s').toBe(false); + expect(allows('delete', [MEMBER_DEFAULT], 'sys_user'), 'account deletion is better-auth\'s').toBe(false); + }); + + it('[#14959] the `sys_user` edit bit rides a WRITE-class row scope, and the org-peer scope stays read-only', () => { + // The edit bit alone would be table-wide. What bounds it to one row is the + // `sys_user_self` carve-out reaching the write class — a `select`-only + // policy contributes nothing to the by-id write pre-image check — and the + // org-peer policy NOT reaching it. Policies OR-combine, so an `all` on + // `sys_user_org_members` would compose `id == me OR id IN ` + // and hand every member their colleagues' profile rows. + const byName = (n: string) => (MEMBER_DEFAULT.rowLevelSecurity ?? []).find((p: any) => p.name === n); + const self = byName('sys_user_self') as any; + expect(self, 'sys_user_self is still shipped').toBeTruthy(); + expect(self.object).toBe('sys_user'); + expect(self.using).toBe('id == current_user.id'); + expect(['all', 'update'], 'sys_user_self must reach the write class').toContain(self.operation); + + const peers = byName('sys_user_org_members') as any; + expect(peers, 'sys_user_org_members is still shipped').toBeTruthy(); + expect(peers.operation, 'org-peer VISIBILITY must not become a write scope').toBe('select'); }); it('self-service preferences survive the wildcard removal as an EXPLICIT grant', () => { diff --git a/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts index bf48dfc975..7d1f917a2c 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts @@ -46,17 +46,25 @@ describe('BETTER_AUTH_MANAGED_OBJECTS ↔ schemas (drift pin, #3325)', () => { }); /** - * [#8053] The single, deliberate exception to the blanket managed-object edit - * deny: `member_default` may EDIT `sys_api_key`, so a member can revoke their - * own personal key. Bounded elsewhere and not by the permission-set boolean — - * the `sys_api_key_self` RLS carve-out decides which rows, ADR-0092 D2's column - * whitelist (`revoked` alone) decides which fields. + * The deliberate exceptions to the blanket managed-object edit deny. Both are + * on `member_default`, both are self-service, and neither is bounded by the + * permission-set boolean it sits on: * - * Encoded as an exact (set, object) pair rather than by loosening the loop, so - * a second entry — or the same one on another set — still fails this pin. The + * - [#8053] `sys_api_key` — a member may revoke their OWN personal key. The + * `sys_api_key_self` RLS carve-out decides which rows; ADR-0092 D2's column + * whitelist (`revoked` alone) decides which fields. + * - [#14959, maintainer ruling 2026-09-03] `sys_user` — a member may edit + * their OWN profile row. The `sys_user_self` RLS carve-out decides which + * rows; ADR-0092 D2's column whitelist (`SYS_USER_PROFILE_EDIT_FIELDS` — + * `name`, `image`, `locale`) decides which fields. The ruling amended + * ADR-0092 D5, whose original text said self-service stays on better-auth + * `/update-user` and that no RLS self-row EDIT carve-out would be built. + * + * Encoded as exact (set, object) pairs rather than by loosening the loop, so a + * third entry — or either of these on another set — still fails this pin. The * create/delete/read axes are NOT excepted and are still asserted below. */ -const EDIT_EXCEPTIONS = new Set(['member_default::sys_api_key']); +const EDIT_EXCEPTIONS = new Set(['member_default::sys_api_key', 'member_default::sys_user']); describe('default permission sets carry the managed denies (static baseline)', () => { it('each write-granting target set denies create/edit/delete on every managed object', () => { @@ -76,23 +84,47 @@ describe('default permission sets carry the managed denies (static baseline)', ( } }); - it('the edit exception is exactly one (set, object) pair, and it is the API-key one', () => { + it('the edit exceptions are exactly the two self-service pairs, and each rides its own row scope', () => { // The exception list is itself pinned: a future widening has to edit THIS // assertion, which is the moment someone is asked whether the new pair // really rides an owner-scoping RLS policy and a column whitelist the way - // `sys_api_key` does. Without this, `EDIT_EXCEPTIONS` could grow silently. - expect([...EDIT_EXCEPTIONS]).toEqual(['member_default::sys_api_key']); + // these two do. Without this, `EDIT_EXCEPTIONS` could grow silently. + expect([...EDIT_EXCEPTIONS]).toEqual([ + 'member_default::sys_api_key', + 'member_default::sys_user', + ]); const member = setByName('member_default'); - expect(member.objects.sys_api_key.allowEdit).toBe(true); - // The owner scoping the grant leans on must exist, or the edit bit is - // table-wide on a credential table. - const selfPolicy = (member.rowLevelSecurity ?? []).find( - (p: any) => p.object === 'sys_api_key' && p.name === 'sys_api_key_self', + + // The owner scoping each grant leans on must exist, or the edit bit is + // table-wide on an identity table. `operation` must reach the WRITE class: + // a `select`-only carve-out contributes nothing to the by-id write + // pre-image check, which is where the row scope is actually enforced. + const scopes: Array<[string, string, string]> = [ + ['sys_api_key', 'sys_api_key_self', 'user_id == current_user.id'], + ['sys_user', 'sys_user_self', 'id == current_user.id'], + ]; + for (const [object, policyName, predicate] of scopes) { + expect(member.objects[object].allowEdit, `${object} allowEdit`).toBe(true); + const selfPolicy = (member.rowLevelSecurity ?? []).find( + (p: any) => p.object === object && p.name === policyName, + ); + expect(selfPolicy, `member_default must keep the ${policyName} RLS carve-out`).toBeTruthy(); + expect(selfPolicy.using).toBe(predicate); + expect(['all', 'update'], `${policyName} operation`).toContain(selfPolicy.operation); + } + + // [#14959] The other `sys_user` policy in this set is the org-peer + // VISIBILITY scope, and it must stay read-only. Applicable policies + // OR-combine, so an `all` here would compose an update filter of + // `id == me OR id IN ` — i.e. the edit bit above + // would reach every colleague's profile row. Pinned next to the grant it + // bounds, because the two lines are 100+ apart in the source. + const orgPeers = (member.rowLevelSecurity ?? []).find( + (p: any) => p.object === 'sys_user' && p.name === 'sys_user_org_members', ); - expect(selfPolicy, 'member_default must keep the sys_api_key_self RLS carve-out').toBeTruthy(); - expect(selfPolicy.using).toBe('user_id == current_user.id'); - expect(['all', 'update']).toContain(selfPolicy.operation); + expect(orgPeers, 'member_default must keep sys_user_org_members').toBeTruthy(); + expect(orgPeers.operation, 'org-peer visibility must not become a write scope').toBe('select'); }); it('admin_full_access keeps its bare wildcard (zero per-object entries) — admin rescue path', () => { diff --git a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts index a700501227..3b91f54b29 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts @@ -477,6 +477,56 @@ const baseDefaultPermissionSets: PermissionSet[] = [ // target set does not already name (`name in objects` → skip), so this // line is preserved rather than overwritten. sys_api_key: { allowRead: true, allowCreate: false, allowEdit: true, allowDelete: false }, + // [#14959 — maintainer ruling 2026-09-03, decision batch #22, verbatim + // 「同意」] The SECOND override of the managed-object write deny, and it is + // deliberately shaped as a copy of the `sys_api_key` one above rather than + // as a new idea: a rank-and-file member may edit their OWN `sys_user` row. + // + // Why it had to be here and nowhere else. The 2026-09-03 ruling that + // admitted `locale` to the ADR-0092 D2 column whitelist opened WHICH + // COLUMNS a permitted actor may touch; it did not open WHO. With this + // entry absent, a member's `PATCH` to their own row was refused by THIS + // layer — the object gate — before the column guard was ever consulted, + // so `sys_user.locale` shipped as a user-facing preference only a platform + // admin could set. That is the "declared, not reachable" shape ADR-0049 + // exists to forbid, one step removed. + // + // The opening is bounded by the same TWO pre-existing mechanisms that + // bound `sys_api_key`, and by neither this line alone: + // - WHICH ROWS: the `sys_user_self` RLS carve-out below + // (`id == current_user.id`), widened from `select` to `all` by the + // same ruling and enforced on by-id writes through the security + // middleware's pre-image check. A member PATCHing ANOTHER member's + // row is still refused there — `sys_user_org_members` (the org-peer + // visibility policy) stays `select`-ONLY precisely so it cannot + // widen this write to the whole organization. + // - WHICH FIELDS: ADR-0092 D2's identity write guard, whose update + // whitelist for this table is `SYS_USER_PROFILE_EDIT_FIELDS` + // (plugin-auth — `name`, `image`, `locale`). `email`, `role`, the ban + // columns and every system stamp stay unwritable on this path; the + // guard strips them and throws when nothing editable survives. + // + // `allowCreate` / `allowDelete` stay false and are NOT an oversight: + // accounts are minted and retired through better-auth's own endpoints + // (sign-up, invite, admin remove-member), and this set is bound to the + // `everyone` anchor, which must remain anchor-safe (ADR-0090 D5). + // + // ⚠️ Still not a pattern to copy across the managed list. What makes this + // pair legitimate is that BOTH bounding mechanisms already existed for + // this exact table before the bit flipped. A managed object without an + // owner-scoped RLS carve-out and a registered column whitelist has + // nothing holding the opening, and `allowEdit: true` on it is table-wide. + // + // Being an EXPLICIT entry is what makes it survive `kernel:ready`: + // `applyManagedWriteDenies` injects its deny only for managed objects a + // target set does not already name (`name in objects` → skip). + // + // ADR-0092 D5 is AMENDED by the same ruling to say so: self-service edits + // of the whitelisted columns route through the generic data path, with the + // D6 `afterUpdate` hook as the session-cache refresh. `name` / `image` + // therefore become editable here too, not only through better-auth + // `/update-user`. + sys_user: { allowRead: true, allowCreate: false, allowEdit: true, allowDelete: false }, // Self-service preferences. NOT a better-auth table, so it is not covered // by the block above, and its `sys_user_preference_self` RLS policy below // (`operation: 'all'`) declares exactly this intent: a member reads and @@ -565,10 +615,27 @@ const baseDefaultPermissionSets: PermissionSet[] = [ operation: 'all', using: 'id == current_user.organization_id', }, + // [#14959 — maintainer ruling 2026-09-03, decision batch #22, verbatim + // 「同意」] `all`, not `select`. This is the WHICH-ROWS half of the + // member self-service opening declared by the explicit `sys_user` + // object entry above; neither half means anything without the other. + // + // `all` (rather than adding a second `update`-only policy) is the shape + // `sys_api_key_self` already ships for exactly this situation, and it is + // safe here for the same reason: the object entry keeps `allowCreate` / + // `allowDelete` false, so the insert and delete faces of `all` gate + // shut one layer up and this policy can only ever NARROW them. + // + // ⚠️ The sibling below (`sys_user_org_members`) must STAY `select`. + // Applicable policies OR-combine, so widening it to `all` would compose + // an update filter of `id == me OR id IN ` — a + // member editing any colleague's profile. The self scope and the + // org-peer VISIBILITY scope are two different questions, and only the + // first one was ruled. { name: 'sys_user_self', object: 'sys_user', - operation: 'select', + operation: 'all', using: 'id == current_user.id', }, // Org collaborators: members can see other users in the same