diff --git a/.changeset/30440-managed-clamp-modify-all.md b/.changeset/30440-managed-clamp-modify-all.md new file mode 100644 index 00000000000..a78d2b5400f --- /dev/null +++ b/.changeset/30440-managed-clamp-modify-all.md @@ -0,0 +1,15 @@ +--- +'@objectstack/core': patch +--- + +fix(core): effective object permissions on a guarded managed object grant no edit, delete or transfer the write guard refuses, `modifyAllRecords` included + +Clause-②: yes (widening) +Metadata-protocol: none + +The managed write clamp behind `/auth/me/permissions` and `ISecurityService.getEffectiveObjectPermissions` cleared `allowEdit`, `allowCreate` and `allowDelete` on an object in a guarded `managedBy` bucket (`better-auth`, `engine-owned`, `append-only`) that does not opt the verb in through `userActions`, but left two grants the write guard refuses: + +- **`modifyAllRecords`**, which grants edit, delete and transfer on its own. A permission set naming such an object with `{ modifyAllRecords: true }` made `current_user.can(object, 'edit' | 'delete' | 'transfer')` answer `true` while every one of those writes was refused. Where the guard refuses `edit` or `delete`, it is now lowered to `viewAllRecords` (the read it implies, so read is unchanged), and each verb the object still opts in is kept as its plain bit: an object opting `edit` in (such as `sys_user`) keeps edit and transfer and loses delete; an object opting nothing in keeps read only. An object opting both `edit` and `delete` in keeps `modifyAllRecords`. The external write clamp lowers it through the same helper. +- **`allowTransfer`**, which now follows `edit`: an owner change is an update, refused unless `edit` is opted in. This also corrects a super-user wildcard admin, who was answered `can(object, 'transfer')` `true` on every guarded object that does not opt `edit` in. + +No write changes: the guard refused each of these before and still does. Only the served capability answer, and the console buttons that read it, now match it. diff --git a/packages/core/src/security/effective-object-permissions.test.ts b/packages/core/src/security/effective-object-permissions.test.ts index a83f1b2fe98..3b9ec5bd108 100644 --- a/packages/core/src/security/effective-object-permissions.test.ts +++ b/packages/core/src/security/effective-object-permissions.test.ts @@ -555,6 +555,73 @@ describe('[#30370] an object whose external write gate is closed grants no write }); }); +describe('[#30440] a guarded managed object grants no verb its write guard refuses, modifyAllRecords included', () => { + const SCHEMAS: Record = { + // engine-owned, nothing opted in: the guard refuses every user-context write. + sys_automation_run: { name: 'sys_automation_run', managedBy: 'engine-owned' }, + // better-auth with only `edit` opted in (the sys_user shape). + sys_user: { name: 'sys_user', managedBy: 'better-auth', userActions: { edit: true } }, + // append-only with both row verbs opted in (object form for delete). + sys_feed_note: { name: 'sys_feed_note', managedBy: 'append-only', userActions: { edit: true, delete: { enabled: true } } }, + // no guard covers it. + crm_lead: { name: 'crm_lead' }, + }; + const source = { allSchemas: () => Object.values(SCHEMAS), schemaOf: (name: string) => SCHEMAS[name] }; + const ROW_VERBS = ['allowEdit', 'allowDelete', 'allowTransfer'] as const; + const can = (entry: any) => Object.fromEntries( + (['allowRead', 'allowCreate', ...ROW_VERBS] as const).map((verb) => [verb, objectPermissionGrants(entry, verb)]), + ); + + it('an explicit { modifyAllRecords: true } on an object opting nothing in answers no edit, delete or transfer, and the same read', () => { + const map: any = buildEffectiveObjectPermissions([{ objects: { sys_automation_run: { modifyAllRecords: true } } }], source); + expect(can(map.sys_automation_run)).toEqual({ allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false, allowTransfer: false }); + expect(map.sys_automation_run).toMatchObject({ modifyAllRecords: false, viewAllRecords: true }); + }); + + it('an object opting edit in keeps edit and transfer through it, and loses delete', () => { + const map: any = buildEffectiveObjectPermissions([{ objects: { sys_user: { modifyAllRecords: true } } }], source); + expect(can(map.sys_user)).toEqual({ allowRead: true, allowCreate: false, allowEdit: true, allowDelete: false, allowTransfer: true }); + expect(map.sys_user).toMatchObject({ modifyAllRecords: false, viewAllRecords: true, allowEdit: true, allowTransfer: true }); + }); + + it('an object opting edit and delete in keeps modifyAllRecords as granted', () => { + const map: any = buildEffectiveObjectPermissions([{ objects: { sys_feed_note: { modifyAllRecords: true } } }], source); + expect(map.sys_feed_note.modifyAllRecords).toBe(true); + expect(map.sys_feed_note).not.toHaveProperty('viewAllRecords'); + expect(can(map.sys_feed_note)).toEqual({ allowRead: true, allowCreate: false, allowEdit: true, allowDelete: true, allowTransfer: true }); + }); + + it('a plain allowTransfer on an object not opting edit in answers no transfer; opting edit in keeps it', () => { + const map: any = buildEffectiveObjectPermissions( + [{ objects: { sys_automation_run: { allowRead: true, allowTransfer: true }, sys_user: { allowRead: true, allowTransfer: true } } }], + source, + ); + expect(objectPermissionGrants(map.sys_automation_run, 'allowTransfer')).toBe(false); + expect(objectPermissionGrants(map.sys_automation_run, 'allowRead')).toBe(true); + expect(objectPermissionGrants(map.sys_user, 'allowTransfer')).toBe(true); + }); + + it('CONTROL: the same entry on an unguarded object keeps every verb modifyAllRecords grants', () => { + const map: any = buildEffectiveObjectPermissions([{ objects: { crm_lead: { modifyAllRecords: true } } }], source); + expect(map.crm_lead.modifyAllRecords).toBe(true); + expect(can(map.crm_lead)).toEqual({ allowRead: true, allowCreate: false, allowEdit: true, allowDelete: true, allowTransfer: true }); + }); + + it('CONTROL: the super-user wildcard fold answers as before, transfer on the object opting nothing in aside', () => { + const map: any = buildEffectiveObjectPermissions( + [{ objects: { '*': { allowRead: true, viewAllRecords: true, modifyAllRecords: true } } }], + source, + ); + expect(can(map.crm_lead)).toEqual({ allowRead: true, allowCreate: false, allowEdit: true, allowDelete: true, allowTransfer: true }); + expect(can(map.sys_user)).toEqual({ allowRead: true, allowCreate: false, allowEdit: true, allowDelete: false, allowTransfer: true }); + expect(can(map.sys_feed_note)).toEqual({ allowRead: true, allowCreate: false, allowEdit: true, allowDelete: true, allowTransfer: true }); + // The fold copies the bits the wildcard grants, never modifyAllRecords itself. + for (const name of Object.keys(SCHEMAS)) expect(map[name], name).not.toHaveProperty('modifyAllRecords'); + // The one answer that moves: the folded transfer bit, on the object whose guard refuses the update. + expect(can(map.sys_automation_run)).toEqual({ allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false, allowTransfer: false }); + }); +}); + // ── [#25866 / ruling of record 6408558941] A field permission entry's row // condition (`when`): the ONE vote per verdict, and the row-blind merge behind // `/auth/me/permissions`' `fields` slot. diff --git a/packages/core/src/security/effective-object-permissions.ts b/packages/core/src/security/effective-object-permissions.ts index 0c7fdba6e4e..83183d154da 100644 --- a/packages/core/src/security/effective-object-permissions.ts +++ b/packages/core/src/security/effective-object-permissions.ts @@ -239,6 +239,18 @@ const GUARDED_WRITE_BUCKETS: ReadonlySet = new Set(['better-auth', 'engi * objects are NOT clamped — no guard covers them, so their permission-set result * stands (an admin CAN write them via the data API, and the hint must not * under-report that). + * + * [#30440] Every verb the guard refuses is refused whatever grants it: + * - `allowTransfer` follows `edit` — an owner change is an update, which the + * guard refuses unless `edit` is opted in; the super-user wildcard fold + * copies the transfer bit onto every entry it covers; + * - `modifyAllRecords` grants edit, delete and transfer on its own (the + * spec's `objectPermissionGrants`), so no `allow*: false` beside it takes + * any of them away. Where the guard refuses `edit` or `delete` it is lowered + * by {@link lowerModifyAllRecords}: to `viewAllRecords`, the read it + * implies, plus the plain bit of each verb the object still opts in. Where + * `edit` and `delete` are both opted in it grants nothing the guard + * refuses, and stays. */ export function clampManagedObjectWrites( objects: Record, @@ -249,15 +261,42 @@ export function clampManagedObjectWrites( const schema = schemaOf(obj); if (!schema?.managedBy || !GUARDED_WRITE_BUCKETS.has(schema.managedBy)) continue; const ua = schema.userActions ?? {}; - if (!isWriteOptedIn(ua.edit)) acc.allowEdit = false; + const edit = isWriteOptedIn(ua.edit); + const del = isWriteOptedIn(ua.delete); + if (!edit) acc.allowEdit = false; // `create` reads through the same opt-in helper as edit/delete since // #7692 widened it to the object form — a bare `ua.create !== true` // would clamp away a legitimate `{ enabled: true, visibleWhen: … }`. if (!isWriteOptedIn(ua.create)) acc.allowCreate = false; - if (!isWriteOptedIn(ua.delete)) acc.allowDelete = false; + if (!del) acc.allowDelete = false; + if (!edit) acc.allowTransfer = false; + lowerModifyAllRecords(acc, { allowEdit: edit, allowDelete: del, allowTransfer: edit }); } } +/** + * Lower an entry's `modifyAllRecords` where a server-side layer refuses a verb + * it grants. The spec's `objectPermissionGrants` answers edit, delete and + * transfer `true` on `modifyAllRecords` alone, so no `allow*: false` beside it + * takes any of them away: the bit itself has to go. It becomes + * `viewAllRecords` — the read it implies, so the entry keeps exactly the read + * it had — and each verb `kept` names is carried over as its plain bit, so a + * verb the layer still admits is not lost with it. An entry the layer admits + * every one of those verbs on is left as it is. + */ +function lowerModifyAllRecords( + acc: Record, + kept: { allowEdit: boolean; allowDelete: boolean; allowTransfer: boolean }, +): void { + if (acc.modifyAllRecords !== true) return; + if (kept.allowEdit && kept.allowDelete && kept.allowTransfer) return; + acc.modifyAllRecords = false; + acc.viewAllRecords = true; + if (kept.allowEdit) acc.allowEdit = true; + if (kept.allowDelete) acc.allowDelete = true; + if (kept.allowTransfer) acc.allowTransfer = true; +} + /** The API-exposure-relevant slice of a registered object schema. */ export interface ApiExposureSchemaLike { name?: string; @@ -551,7 +590,7 @@ const EXTERNAL_WRITE_GATED_OPERATIONS: ReadonlySet = new Set, forbidden: Re acc.allowEdit = false; acc.allowDelete = false; acc.allowTransfer = false; - if (acc.modifyAllRecords === true) { - acc.modifyAllRecords = false; - acc.viewAllRecords = true; - } + lowerModifyAllRecords(acc, { allowEdit: false, allowDelete: false, allowTransfer: false }); } } diff --git a/packages/plugins/plugin-security/src/get-effective-object-permissions.test.ts b/packages/plugins/plugin-security/src/get-effective-object-permissions.test.ts index 14bffb0cc3a..6811596d655 100644 --- a/packages/plugins/plugin-security/src/get-effective-object-permissions.test.ts +++ b/packages/plugins/plugin-security/src/get-effective-object-permissions.test.ts @@ -354,7 +354,8 @@ describe('[#18783] the engine is handed the same producer', () => { * the create / edit / delete verbs may read NARROWER than the evaluator: that * is the managed-write clamp, the engine write guard the permission sets do not * model. There the pin holds the refuse direction only — the map never grants - * what the evaluator refuses. + * what the evaluator refuses. [#30440] `transfer` is one of them: an owner + * change is an update, which the guard refuses unless the object opts `edit` in. * * [#20134] Subjects whose sets carry a SUPER-USER wildcard are held by the same * table, one row per shape: the shipped platform admin and walled org admin, a @@ -473,6 +474,15 @@ describe('[#20083] parity: can() over the member\'s map answers what checkObject shipped('admin_full_access'), shipped('organization_admin'), shipped('member_default'), ], // [#20135] The export slot's own shapes. + // [#30440] An explicit modify-all entry on guarded managed objects: one opting + // `edit` in (sys_user), one opting nothing in (sys_member), one private (sys_secret). + '[#30440] explicit modifyAllRecords entries on guarded managed objects': [ + authored('managed_modify_all', { + [SysUser.name]: { modifyAllRecords: true }, + [SysMember.name]: { modifyAllRecords: true }, + [SysSecret.name]: { modifyAllRecords: true }, + }), + ], 'platform admin beside a plain export-only wildcard': [shipped('admin_full_access'), authored('exporter', { '*': { allowExport: true } })], 'one set: an exporting wildcard AND an explicit entry without the grant': [ authored('same_export', { '*': { allowRead: true, allowExport: true }, crm_lead: { allowRead: true } }), @@ -484,7 +494,7 @@ describe('[#20083] parity: can() over the member\'s map answers what checkObject allowTransfer: 'transfer', allowExport: 'export', }; const GUARDED = new Set(['better-auth', 'engine-owned', 'append-only']); - const CLAMPED = new Set(['allowCreate', 'allowEdit', 'allowDelete']); + const CLAMPED = new Set(['allowCreate', 'allowEdit', 'allowDelete', 'allowTransfer']); const USER = { id: 'u_parity', positions: [] as string[] }; const evaluator = new PermissionEvaluator();