From 02809af01888977a457e3a8c28805f29c4504e25 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 14:12:50 +0000 Subject: [PATCH] fix(platform-objects,plugin-auth): give API-key revoke/restore a working product route (#7727) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sys_api_key` declared two row actions — `revoke_api_key` / `restore_api_key` — as `PATCH /api/v1/data/sys_api_key/{id}` with `bodyExtra: {revoked}`, while the same object set `enable.apiMethods = ['get','list']`. The declared PATCH died at the ADR-0049 method gate with 405 before any authorization ran, so no product route revoked an API key: the Setup UI's Revoke button errored, the row kept `revoked = false`, and the key kept authenticating. The write path had two gates, not one: - the method gate — `enable.apiMethods` now carries `update` (`create` / `delete` stay off: minting is `POST /api/v1/keys`, and keys are retired by revoking rather than deleting); - ADR-0103's `reconcileManagedApiMethods`, which strips any write verb a `managedBy` object's affordances do not grant and only warns. `apiMethods` alone would still have served 405 while the source read correctly, so `userActions: { edit: true }` declares the affordance — the ADR-0092 D4 pattern `sys_user` already uses. Opening the method does not open the columns. The object stays `managedBy: 'better-auth'`, so ADR-0092 D2's identity write guard still fail-closed rejects user-context writes and its per-object update whitelist stays the only opening; `revoked` is registered there and nothing else is. The guard itself is untouched — no general weakening, and every other identity table keeps its default-deny. Per D4's form-rendering constraint the columns outside the whitelist are now `readonly`, so the edit form this affordance turns on cannot offer a write the server refuses. Nothing pinned any of this: the existing tests exercise key resolution against a pre-revoked row and never call the route the actions declare. The new dogfood suite drives the real PATCH, asserts 200, then asserts the consequence — the key stops authenticating — and pins the refusals with code AND status. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SqARTSYRvgutYHaLXbx6f7 --- .changeset/api-key-revoke-product-route.md | 54 +++++ .../src/identity/sys-api-key.object.ts | 53 +++- .../src/managed-extension-fields.ts | 20 ++ .../api-key-revoke-lifecycle.dogfood.test.ts | 226 ++++++++++++++++++ ..._src__identity__sys-api-key.object.ts.json | 8 + 5 files changed, 356 insertions(+), 5 deletions(-) create mode 100644 .changeset/api-key-revoke-product-route.md create mode 100644 packages/qa/dogfood/test/api-key-revoke-lifecycle.dogfood.test.ts create mode 100644 scripts/adr-anchors/packages__platform-objects__src__identity__sys-api-key.object.ts.json diff --git a/.changeset/api-key-revoke-product-route.md b/.changeset/api-key-revoke-product-route.md new file mode 100644 index 0000000000..65c16ab22a --- /dev/null +++ b/.changeset/api-key-revoke-product-route.md @@ -0,0 +1,54 @@ +--- +"@objectstack/platform-objects": patch +"@objectstack/plugin-auth": patch +--- + +fix(platform-objects,plugin-auth): let the API-key revoke/restore actions actually run (#7727) + +`sys_api_key` contradicted itself. It declared two row actions — +`revoke_api_key` / `restore_api_key` — as `PATCH /api/v1/data/sys_api_key/{id}` +with `bodyExtra: { revoked: true|false }`, while the same object set +`enable.apiMethods = ['get', 'list']`. The declared PATCH was refused at the +ADR-0049 method gate with `405 OBJECT_API_METHOD_NOT_ALLOWED` before any +authorization ran, so **no product route revoked an API key**: the Setup → +API Keys → Revoke button produced an error toast, the row still read +`revoked = false`, and the key kept authenticating. A leaked key could only be +retired by writing the row out of band. + +Enforcement of the flag was never the problem — the verifier filters +`revoked: false` and re-checks the row, so a flipped bit takes effect on the +very next `x-api-key` call. The missing piece was purely the write path, and it +had **two** gates, not one: + +- **The method gate.** `enable.apiMethods` now carries `update`. `create` and + `delete` stay off: minting is `POST /api/v1/keys` (the only path that ever + returns the raw secret) and keys are retired by revoking, not deleting. +- **The affordance reconciler.** ADR-0103's `reconcileManagedApiMethods` strips + any write verb a `managedBy` object's resolved affordances do not grant — + warning, not failing. So `apiMethods` alone would still have served 405 while + the source read correctly. `userActions: { edit: true }` declares the + affordance, exactly as `sys_user` does under ADR-0092 D4. + +**Opening the method does not open the columns.** `sys_api_key` stays +`managedBy: 'better-auth'`, so ADR-0092 D2's identity write guard still +fail-closed rejects user-context writes, and its per-object update whitelist +remains the only opening. `revoked` is registered there and nothing else is: +`key` stays unwritable (a rotated hash would mint a credential nobody holds), +`user_id` stays unwritable (re-owning a key is privilege transfer), and +`expires_at` stays on the mint path. A PATCH carrying only non-whitelisted +columns is refused `403 PERMISSION_DENIED` rather than degrading into a +timestamp touch, and a mixed patch applies `revoked` while stripping the rest. +The guard itself is unchanged — no general weakening, and every other identity +table keeps its default-deny. + +Per ADR-0092 D4's form-rendering constraint, the columns outside the whitelist +(`name`, `prefix`, `user_id`, `scopes`, `expires_at`) are now `readonly`, so the +edit form this affordance turns on cannot offer a write the server refuses — +the declared-≠-enforced shape that caused the original defect. + +Nothing pinned any of this before: the existing tests exercise key *resolution* +against a pre-revoked row and never call the route the actions declare, which is +how a declared action and a method gate cancelled out unnoticed. The new +`api-key-revoke-lifecycle` dogfood suite drives the real PATCH, asserts `200`, +and then asserts the consequence — the key stops authenticating — because a 200 +that leaves the key working is the defect wearing a success code. diff --git a/packages/platform-objects/src/identity/sys-api-key.object.ts b/packages/platform-objects/src/identity/sys-api-key.object.ts index 66cf1d5270..52409f82c4 100644 --- a/packages/platform-objects/src/identity/sys-api-key.object.ts +++ b/packages/platform-objects/src/identity/sys-api-key.object.ts @@ -20,6 +20,22 @@ export const SysApiKey = ObjectSchema.create({ icon: 'key-round', isSystem: true, managedBy: 'better-auth', + // [ADR-0092 D4 / ADR-0103] Declares the generic EDIT affordance, which is + // what lets `enable.apiMethods` keep `update` below: `managedBy` objects run + // through `reconcileManagedApiMethods`, which strips any write verb the + // resolved affordances do not grant. Without this line the declaration and + // the runtime disagree again — silently, one layer deeper than #7727's + // method gate. `create` / `delete` stay bucket-default (off): minting is + // `POST /api/v1/keys` and rows are retired by revoking, not deleting. + // + // The affordance is safe to open only because the enforcement it fronts + // already exists (D4's sequencing rule — affordance never ships ahead of + // the guard): ADR-0092 D2's guard clamps every user-context update on this + // table to the registered column whitelist, which lists `revoked` alone. + // Per D4's form-rendering constraint, every column outside that whitelist + // is marked `readonly` below, so the edit form cannot offer a write the + // server will refuse. + userActions: { edit: true }, // ADR-0010 §3.7 — managed by better-auth; tenants may not edit schema, // but may add overlay row-level config. Use `no-overlay` if you need to // forbid sys_metadata overlays entirely. @@ -36,8 +52,9 @@ export const SysApiKey = ObjectSchema.create({ // Custom actions — sys_api_key is managed-by 'better-auth' but the // `revoked` boolean is a column we control via the data API. These row - // actions use the generic PATCH /api/v1/sys_api_key/{id} endpoint with - // `bodyExtra` to set the `revoked` flag explicitly. + // actions use the generic PATCH /api/v1/data/sys_api_key/{id} endpoint with + // `bodyExtra` to set the `revoked` flag explicitly. The `target` below is + // the authority on that path; this comment used to omit `/data/`. actions: [ { name: 'revoke_api_key', @@ -117,9 +134,16 @@ export const SysApiKey = ObjectSchema.create({ fields: { // ── Identity ───────────────────────────────────────────────── + // The five fields below are `readonly` for one reason (ADR-0092 D4's + // form-rendering constraint): they are set on the mint path and are NOT on + // the identity write guard's column whitelist, so a user-context write to + // any of them is refused 403. With `userActions.edit` open, leaving them + // writable in the form would advertise an edit the server rejects — the + // declared-≠-enforced shape this object already paid for once (#7727). name: Field.text({ label: 'Name', required: true, + readonly: true, searchable: true, maxLength: 255, description: 'Human-readable label for the API key', @@ -129,6 +153,7 @@ export const SysApiKey = ObjectSchema.create({ prefix: Field.text({ label: 'Prefix', required: false, + readonly: true, maxLength: 16, description: 'Visible prefix for identifying the key (e.g., "osk_")', group: 'Identity', @@ -137,6 +162,7 @@ export const SysApiKey = ObjectSchema.create({ user_id: Field.lookup('sys_user', { label: 'Owner', required: true, + readonly: true, description: 'User who owns this API key', group: 'Identity', }), @@ -145,6 +171,7 @@ export const SysApiKey = ObjectSchema.create({ scopes: Field.textarea({ label: 'Scopes', required: false, + readonly: true, description: 'JSON array of permission scopes', group: 'Access', }), @@ -153,6 +180,7 @@ export const SysApiKey = ObjectSchema.create({ expires_at: Field.datetime({ label: 'Expires At', required: false, + readonly: true, group: 'Lifecycle', }), @@ -214,8 +242,23 @@ export const SysApiKey = ObjectSchema.create({ trackHistory: true, searchable: false, apiEnabled: true, - // #1591 — reads only: writes are refused by the identity write guard - // (ADR-0092 D2) and owned by better-auth. HTTP answers 405 before the 403. - apiMethods: ['get', 'list'], + // #1591 / #7727 — reads, plus `update` for the revoke/restore lifecycle. + // + // `create` and `delete` stay off: minting is `POST /api/v1/keys` (the only + // path that can return the raw secret once) and rows are retired by + // revoking, not deleting, so history survives. + // + // `update` is here because the two row actions above declare a PATCH + // against the data API, and a method gate that answers 405 first makes + // those actions dead on arrival — a declared affordance the runtime never + // honours (#7727). Opening the METHOD does not open the COLUMNS: this + // object is `managedBy: 'better-auth'`, so ADR-0092 D2's identity write + // guard still fail-closed rejects user-context writes, and the only + // opening is its per-object update whitelist. `revoked` is registered + // there (plugin-auth `managed-extension-fields.ts`); every other column — + // `key`, `user_id`, `expires_at`, `name`, … — is stripped, and a PATCH + // that touches nothing else is refused 403 `PERMISSION_DENIED` rather + // than degrading into a silent no-op. + apiMethods: ['get', 'list', 'update'], }, }); diff --git a/packages/plugins/plugin-auth/src/managed-extension-fields.ts b/packages/plugins/plugin-auth/src/managed-extension-fields.ts index a11b6ffd4d..bdd4f99a5a 100644 --- a/packages/plugins/plugin-auth/src/managed-extension-fields.ts +++ b/packages/plugins/plugin-auth/src/managed-extension-fields.ts @@ -58,6 +58,15 @@ export const MANAGED_EXTENSION_FIELDS: Readonly { + let stack: VerifyStack; + let token: string; + + /** Mint a key through the ONE mint path; returns its id + raw secret. */ + const mintKey = async (name: string): Promise<{ id: string; raw: string }> => { + const res = await stack.apiAs(token, 'POST', '/keys', { name }); + expect(res.status).toBe(201); + const body: any = await res.json(); + expect(body?.data?.id).toBeTruthy(); + expect(body?.data?.key).toBeTruthy(); + return { id: String(body.data.id), raw: String(body.data.key) }; + }; + + /** + * Does this key still authenticate? Asked through a real authenticated + * read, with NO bearer token — the key is the only credential present, so + * a 200 means the key itself was accepted. + */ + const keyStillAuthenticates = async (raw: string): Promise => { + const res = await stack.api('/data/sys_api_key', { headers: { 'x-api-key': raw } }); + if (res.status === 200) return true; + expect(res.status).toBe(401); + return false; + }; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, {}); + token = await stack.signIn(); + }, 120_000); + + afterAll(async () => { await stack?.stop?.(); }); + + it('revokes through PATCH /data/sys_api_key/{id} and the key stops authenticating', async () => { + const { id, raw } = await mintKey('revoke-me'); + + // Baseline: the freshly minted key authenticates. Without this the + // post-revoke 401 would be unfalsifiable — a key that never worked also + // "stops working". + expect(await keyStillAuthenticates(raw)).toBe(true); + + // The exact request `revoke_api_key` declares. + const revoked = await stack.apiAs(token, 'PATCH', `/data/sys_api_key/${id}`, { revoked: true }); + expect(revoked.status).toBe(200); + + // The consequence — a 200 that leaves the key working is the defect. + expect(await keyStillAuthenticates(raw)).toBe(false); + + // And the row itself reads back revoked, so the 200 wasn't a no-op that + // stripped the only field it was supposed to write. + const row = await stack.apiAs(token, 'GET', `/data/sys_api_key/${id}`); + expect(row.status).toBe(200); + const rowBody: any = await row.json(); + expect(rowBody.record?.revoked ?? rowBody.data?.revoked).toBe(true); + }); + + it('restores through the same route and the key authenticates again', async () => { + const { id, raw } = await mintKey('restore-me'); + + const off = await stack.apiAs(token, 'PATCH', `/data/sys_api_key/${id}`, { revoked: true }); + expect(off.status).toBe(200); + expect(await keyStillAuthenticates(raw)).toBe(false); + + // The exact request `restore_api_key` declares. + const on = await stack.apiAs(token, 'PATCH', `/data/sys_api_key/${id}`, { revoked: false }); + expect(on.status).toBe(200); + expect(await keyStillAuthenticates(raw)).toBe(true); + }); + + it('opens the column, not the table: a non-revoked PATCH is refused 403 PERMISSION_DENIED', async () => { + // The negative direction matters as much as the positive one: if opening + // `update` let a GENERAL update through, that is a worse defect than the + // one being closed. `name` is an ordinary, otherwise-innocuous column — + // it is refused because it is not on the ADR-0092 D2 whitelist, not + // because it is dangerous. + const { id, raw } = await mintKey('rename-me'); + + const res = await stack.apiAs(token, 'PATCH', `/data/sys_api_key/${id}`, { name: 'renamed' }); + expect(res.status).toBe(403); + const body: any = await res.json(); + expect(body.code).toBe('PERMISSION_DENIED'); + + // Refused, not silently degraded into a timestamp touch. + const row = await stack.apiAs(token, 'GET', `/data/sys_api_key/${id}`); + const rowBody: any = await row.json(); + expect(rowBody.record?.name ?? rowBody.data?.name).toBe('rename-me'); + + // The key is untouched by a refused write. + expect(await keyStillAuthenticates(raw)).toBe(true); + }); + + it('refuses the credential columns even when smuggled alongside a legal `revoked`', async () => { + // The whitelist STRIPS non-listed keys rather than rejecting the whole + // payload, so a mixed patch is the shape that decides whether the + // opening is column-scoped in practice: `revoked` must land and `key` / + // `user_id` must not ride along with it. A rotated `key` would mint a + // credential nobody holds; a rewritten `user_id` is privilege transfer. + const { id, raw } = await mintKey('smuggle-me'); + const before = await stack.apiAs(token, 'GET', `/data/sys_api_key/${id}`); + const beforeBody: any = await before.json(); + const originalOwner = beforeBody.record?.user_id ?? beforeBody.data?.user_id; + expect(originalOwner).toBeTruthy(); + + const res = await stack.apiAs(token, 'PATCH', `/data/sys_api_key/${id}`, { + revoked: true, + key: 'forged-hash-value', + user_id: 'usr_someone_else', + }); + // The whitelisted field survives, so the write succeeds… + expect(res.status).toBe(200); + + // …but only that field was applied. + const after = await stack.apiAs(token, 'GET', `/data/sys_api_key/${id}`); + const afterBody: any = await after.json(); + expect(afterBody.record?.user_id ?? afterBody.data?.user_id).toBe(originalOwner); + expect(afterBody.record?.revoked ?? afterBody.data?.revoked).toBe(true); + + // The decisive proof that `key` was stripped: the ORIGINAL secret is what + // the row still hashes to. Had the forged value landed, this key would be + // unrecognised rather than recognised-and-revoked — both answer 401, so + // assert it from the other side by restoring and re-authenticating. + const restored = await stack.apiAs(token, 'PATCH', `/data/sys_api_key/${id}`, { revoked: false }); + expect(restored.status).toBe(200); + expect(await keyStillAuthenticates(raw)).toBe(true); + }); + + it('still refuses create and delete on the identity table (405, method gate)', async () => { + // `update` was opened; `create` / `delete` were not. Minting stays on + // `POST /api/v1/keys` (the only path that returns the raw secret once) + // and rows are retired by revoking, not deleting. + const { id } = await mintKey('immortal'); + + const created = await stack.apiAs(token, 'POST', '/data/sys_api_key', { name: 'forged' }); + expect(created.status).toBe(405); + const createdBody: any = await created.json(); + expect(createdBody.code).toBe('OBJECT_API_METHOD_NOT_ALLOWED'); + + const deleted = await stack.apiAs(token, 'DELETE', `/data/sys_api_key/${id}`); + expect(deleted.status).toBe(405); + const deletedBody: any = await deleted.json(); + expect(deletedBody.code).toBe('OBJECT_API_METHOD_NOT_ALLOWED'); + }); + + it('the declared row actions still match the route this file drives', async () => { + // The original defect was a DECLARATION disagreeing with the runtime, so + // pin the declaration too — read from the REGISTERED schema, i.e. what the + // runtime actually serves. If someone re-points these actions at another + // verb or path, every assertion above would keep passing while the button + // in the UI went dead again, which is exactly how #7727 stayed invisible. + const engine = await stack.kernel.getServiceAsync('objectql'); + const schema = engine?.getSchema?.('sys_api_key'); + expect(schema, 'sys_api_key schema must be registered').toBeTruthy(); + + // The REGISTERED apiMethods, i.e. post-`reconcileManagedApiMethods`. The + // object's own source saying `update` is not enough — the reconciler + // strips a write verb whose affordance is missing and only warns, which + // is how `update` can be declared and 405 anyway. + expect(schema.enable?.apiMethods).toContain('update'); + expect(schema.userActions?.edit).toBe(true); + // The opening is `update` only — `create` / `delete` must stay stripped. + expect(schema.enable?.apiMethods).not.toContain('create'); + expect(schema.enable?.apiMethods).not.toContain('delete'); + + const actions = schema.actions as any[] | undefined; + expect(actions, 'sys_api_key must keep its row actions').toBeTruthy(); + + for (const expected of [ + { name: 'revoke_api_key', revoked: true }, + { name: 'restore_api_key', revoked: false }, + ]) { + const declared = actions!.find((a) => a?.name === expected.name); + expect(declared, `${expected.name} must stay declared`).toBeTruthy(); + expect(declared.method).toBe('PATCH'); + expect(declared.target).toBe('/api/v1/data/sys_api_key/{id}'); + expect(declared.bodyExtra).toEqual({ revoked: expected.revoked }); + } + }); +}); diff --git a/scripts/adr-anchors/packages__platform-objects__src__identity__sys-api-key.object.ts.json b/scripts/adr-anchors/packages__platform-objects__src__identity__sys-api-key.object.ts.json new file mode 100644 index 0000000000..9881f09d88 --- /dev/null +++ b/scripts/adr-anchors/packages__platform-objects__src__identity__sys-api-key.object.ts.json @@ -0,0 +1,8 @@ +{ + "file": "packages/platform-objects/src/identity/sys-api-key.object.ts", + "adrs": [ + "ADR-0092", + "ADR-0103" + ], + "invariant": "`enable.apiMethods` carries `update` and `userActions.edit` is open, and neither is arbitrary. ADR-0092 D2's identity write guard clamps every user-context update on this better-auth-managed table to the registered column whitelist, which lists `revoked` alone — so opening the METHOD does not open the COLUMNS, and the readonly stamps on the non-whitelisted columns are D4's form-rendering constraint, not cosmetics. The affordance is what ADR-0103's `reconcileManagedApiMethods` requires before it will let a managed object keep a write verb: drop `userActions.edit` and the reconciler silently strips `update` again, which is how the declared revoke/restore row actions came to answer 405 (#7727)." +}