diff --git a/.changeset/18783-server-can-option-visibility.md b/.changeset/18783-server-can-option-visibility.md index f357292a9fe..29a6ba2e551 100644 --- a/.changeset/18783-server-can-option-visibility.md +++ b/.changeset/18783-server-can-option-visibility.md @@ -34,6 +34,6 @@ stage: Field.select({ - If the security service cannot resolve the map, a write that needs it is refused with the resolution's own error — fail closed. It is never read as "no grants". - With no security plugin, or an engine older than the seam, there is no permission data. The gate stays unevaluable and the value is admitted with the same `warn` as before, which names the missing input. The security plugin logs one `warn` at start when the engine lacks the seam. -**Plain-wildcard coverage, closed in this release.** `can()` reads only the per-object entries of the map. Before #20083, `/auth/me/permissions` listed an object for a `'*'` wildcard grant only when that grant carried a super-user bit, so a subject whose access to an object came only from a plain wildcard — for example `organization_admin_no_bypass`, which a deployment without an organization wall grants to organization owners and admins — got `false` from `current_user.can()` for that object, although the data plane admits the write, and was refused on a `can`-gated option. That gap is closed in this same release by #20083 (`.changeset/20083-effective-map-plain-wildcard.md`): `buildEffectiveObjectPermissions` now puts each set's plain `'*'` on the registered public objects that set does not name, so that population's map — and any client that answers `can()` from the same `/auth/me/permissions` map — carries an entry for each object the wildcard covers, with the wildcard's grants, narrowed on a guarded managed object by the same managed-write clamp as every other entry. The map still differs from `PermissionEvaluator.checkObjectPermission` for subjects holding a super-user wildcard: an entry the super-user set itself names narrower can read as granted, and an entry reached through a super-user wildcard carries no `transfer`. +**Plain-wildcard coverage, closed in this release.** `can()` reads only the per-object entries of the map. Before #20083, `/auth/me/permissions` listed an object for a `'*'` wildcard grant only when that grant carried a super-user bit, so a subject whose access to an object came only from a plain wildcard — for example `organization_admin_no_bypass`, which a deployment without an organization wall grants to organization owners and admins — got `false` from `current_user.can()` for that object, although the data plane admits the write, and was refused on a `can`-gated option. That gap is closed in this same release by #20083 (`.changeset/20083-effective-map-plain-wildcard.md`): `buildEffectiveObjectPermissions` now puts each set's plain `'*'` on the registered public objects that set does not name, so that population's map — and any client that answers `can()` from the same `/auth/me/permissions` map — carries an entry for each object the wildcard covers, with the wildcard's grants, narrowed on a guarded managed object by the same managed-write clamp as every other entry. The map still differs from `PermissionEvaluator.checkObjectPermission` for subjects holding a super-user wildcard: an entry the super-user set itself names narrower can read as granted. Its missing `transfer` is closed in this same release (`.changeset/20134-super-user-entries-every-bit.md`). **No spec key, route or config key is added or removed.** diff --git a/.changeset/18931-me-permissions-unrestricted-export-annotation.md b/.changeset/18931-me-permissions-unrestricted-export-annotation.md index 38bfb50fcbe..6eac37164a9 100644 --- a/.changeset/18931-me-permissions-unrestricted-export-annotation.md +++ b/.changeset/18931-me-permissions-unrestricted-export-annotation.md @@ -10,7 +10,7 @@ The endpoint builds its per-object map in four passes — seed, fold, clamp, ann For that principal an unrestricted object got no entry, so annotate never saw it and the response said nothing about it at all. The client reads `apiOperations: undefined`, takes the default-allow path #3391 gave it, renders **Export**, and the click is refused. A sibling object declaring `apiMethods` got an entry, an `apiOperations` without `export`, and no button — the same principal, the same session, two answers. -- **The seed now applies annotate's own predicate**: resolve with the export slot annotate will read for the entry being seeded, and skip only an object that is unrestricted **and** keeps `export`. A seeded entry carries no `allowExport` of its own and `foldWildcardSuperUser` does not add one, so annotate's `acc.allowExport ?? wildExport` resolves to the same wildcard bit the seed read — the two passes cannot diverge again. +- **The seed and annotate cannot diverge again.** Since #20134 the seed places an entry for every registered object a super-user wildcard reaches that the merge left without one, and `annotateEffectiveApiOperations` alone decides which entries carry `apiOperations`: an object that is unrestricted **and** keeps `export` gets none. - **The export axis is the only axis this reaches.** Measured across the `enable` shapes an unrestricted object can carry: withholding `export` subtracts `export` and nothing else, and `mode` stays `unrestricted` either way — which is why the old `mode`-only guard could not tell the two cases apart. The CRUD axis needed no annotation and still gets none. - **What the response gains**: for such a principal, one entry per unrestricted object, each the full closure minus `export`. Its CRUD bits are folded `true` — the same answer the client already computed by falling back to `'*'`, now stated explicitly rather than inherited. - **Denial is unchanged.** `enforceExportPermission` → `security.canExport` still answers `403 EXPORT_NOT_PERMITTED`, and no request that was refused is now accepted. This is the affordance half: the channel that is supposed to tell the client now does. diff --git a/.changeset/18990-viewall-only-permissions-seed.md b/.changeset/18990-viewall-only-permissions-seed.md index 3ed4a3fe8b3..261e412fe05 100644 --- a/.changeset/18990-viewall-only-permissions-seed.md +++ b/.changeset/18990-viewall-only-permissions-seed.md @@ -9,6 +9,6 @@ - **One predicate admits both classes.** The seed now asks the wildcard READ bypass — `viewAllRecords || modifyAllRecords` — which is the same question `foldWildcardSuperUser` already asks to decide whose `allowRead` it pulls true, and the same one `PermissionEvaluator` applies server-side. It is now a single module-local reading both call sites share, so the seed can never materialise an entry for a principal the fold leaves entirely false. - **A plain wildcard grant carrying neither bypass bit is still not seeded.** That is what makes the admission the read bypass rather than "any wildcard": the fold pulls nothing true for it, so a seeded entry would be an all-false claim with no server behaviour behind it. - **The seeded entry is the truth, not an overreach.** It starts `{allow*: false}`, the fold pulls `allowRead` true, and the write bits stay false. The seed only ever touches objects with **no explicit entry**, and on those a viewAll-only principal really can only read — so "explicit false" for edit is what is true about it, where the silence it replaces was not. -- **`apiOperations` is attached through the predicate already shared with the modify-all class** — an unrestricted object whose export stays allowed is still skipped, because for it the client's default-allow path is already right. +- **`apiOperations` is attached through the predicate already shared with the modify-all class** — an unrestricted object whose export stays allowed still gets no `apiOperations` (the entry itself is seeded since #20134), because for it the client's default-allow path is already right. ⚠️ **This is a deliberate behaviour change on an existing published channel, ruled rather than inferred.** For a viewAll-only principal a client that reads "no entry" as default-allow now reads an explicit `allowEdit: false` instead. Two pins asserting the old silence (`toBeUndefined` for the viewAll-only principal, one of them added by the framework#18931 PR that pinned this boundary while saying the pin was not a ruling that the silence was correct) are inverted on purpose under that ruling. Payload growth is the same one-entry-per-object framework#18931 accepted, now also for viewAll principals. diff --git a/.changeset/20134-super-user-entries-every-bit.md b/.changeset/20134-super-user-entries-every-bit.md new file mode 100644 index 00000000000..a113da40a18 --- /dev/null +++ b/.changeset/20134-super-user-entries-every-bit.md @@ -0,0 +1,17 @@ +--- +'@objectstack/core': minor +--- + +fix(core): an effective-map entry reached through a super-user `'*'` carries every bit the server grants, so `current_user.can(object, 'transfer')` agrees with `checkObjectPermission` for a platform admin (#20134) + +`buildEffectiveObjectPermissions` builds the `objects` slot of `GET /auth/me/permissions` (`@objectstack/plugin-hono-server`) and the map `ISecurityService.getEffectiveObjectPermissions` returns (`@objectstack/plugin-security`), which the engine hands to `current_user.can(object, verb)` on the write path. For a subject holding a super-user wildcard — a `'*'` carrying `viewAllRecords` or `modifyAllRecords`, as `admin_full_access` and `organization_admin` do — its entries diverged from `PermissionEvaluator.checkObjectPermission` in three places, each in the refuse direction: + +- **`transfer`.** The fold put only read, create, edit and delete on an entry. `modifyAllRecords` also grants `transfer` on the server, so `can(object, 'transfer')` answered `false` for `admin_full_access` on every object, and for the walled `organization_admin` on every object its own set does not name. +- **A super-read wildcard's own bits.** A `'*'` carrying `viewAllRecords` beside plain bits (`allowEdit`, `allowTransfer`, …) put only the read on an entry, so `can(object, 'edit')` answered `false` where the server edits. +- **A super-user wildcard carrying `allowExport`.** The super-user seed skipped every unrestricted object whose export stays allowed, because it needs no `apiOperations`. That left no entry at all, and `can()` reads an absent entry as "no grant", so every verb answered `false` on those objects. + +**What changes.** The seed now places an entry for every registered object the merged map does not already carry. A new step after the fold then applies each set's super-user `'*'` to every entry that set does not name, and sets every bit the spec's `objectPermissionGrants` says that wildcard grants: `transfer` through `modifyAllRecords`, the wildcard's own plain bits, and its `allowExport`. This is the per-set reading `checkObjectPermission` applies. A set that names an object keeps its explicit entry as its whole answer for that object, and a private object is covered, as on the server. Only `true` bits are set. The step runs before the managed-write clamp, which still narrows create, edit and delete on a guarded managed object. No exported name or type changes. + +**What a reader of `/auth/me/permissions` sees.** For a subject holding a super-user wildcard, entries gain `true` bits (`allowTransfer`, and the wildcard's own plain and export bits). Where that subject's wildcards also grant `allowExport`, the map gains an entry for each registered unrestricted object that had none. That entry carries no `apiOperations`, exactly as the operation channel said nothing about the object before, so a client's default-allow path for the operation set is unchanged. Nothing is removed and no `true` bit turns `false`. The response is byte-identical for a subject holding no super-user wildcard: `member_default` alone, and `viewer_readonly` or `organization_admin_no_bypass` beside it. The response shape, its keys and the route are unchanged. On the write path, a `can(object, 'transfer')`-gated option or default is now admitted for these subjects wherever the server grants `transfer`. + +**Still broader than the server, unchanged here.** The fold still folds the merged super-user bits into an entry the super-user set itself names narrower. It also still pulls `allowCreate` on `modifyAllRecords` alone, which the server does not grant. Both over-grants are left exactly as they were. diff --git a/packages/core/src/security/effective-object-permissions.test.ts b/packages/core/src/security/effective-object-permissions.test.ts index 45ffe2462c5..cfe74b39a0c 100644 --- a/packages/core/src/security/effective-object-permissions.test.ts +++ b/packages/core/src/security/effective-object-permissions.test.ts @@ -187,8 +187,11 @@ describe('[#20083] plain wildcard coverage', () => { [{ objects: { '*': { ...WILD, viewAllRecords: true, modifyAllRecords: true } } }], source, ); - // Seeded all-false, then folded: the super-user entry shape, not the wildcard's own bits. - expect(Object.keys(map.crm_account)).toEqual(['allowCreate', 'allowRead', 'allowEdit', 'allowDelete', 'apiOperations']); + // Seeded all-false, then folded: the super-user entry shape, not the wildcard's own key + // order — [#20134] with `allowTransfer`, which `modifyAllRecords` grants, appended by the + // per-set fold (the wildcard's own `allowTransfer: false` grants nothing and is not copied). + expect(Object.keys(map.crm_account)).toEqual(['allowCreate', 'allowRead', 'allowEdit', 'allowDelete', 'allowTransfer', 'apiOperations']); + expect(map.crm_account.allowTransfer).toBe(true); // …and the private object is the seed's, as before. expect(map.crm_secret).toMatchObject({ allowRead: true, allowEdit: true }); }); @@ -200,3 +203,83 @@ describe('[#20083] plain wildcard coverage', () => { expect(map).toEqual({ '*': WILD }); }); }); + +/** + * [#20134] A SUPER-USER `'*'` — one carrying `viewAllRecords` or + * `modifyAllRecords` — puts on the map every bit it grants, per set, the way + * `PermissionEvaluator.checkObjectPermission` resolves it: the seed places an + * entry for every registered object the merge left absent, and the per-set + * fold reads each set's wildcard through the spec's `objectPermissionGrants`. + * The map used to hold only the four bits the merged fold pulls, so + * `current_user.can(object, 'transfer')` answered `false` for a platform admin + * the server lets transfer, a super-read wildcard lost its own plain bits, and + * a super-user wildcard carrying `allowExport` left every unrestricted object + * with no entry at all. + * + * The enforcement-side half — every verb, the shipped super-user sets, the + * real `can()` against the real evaluator — is pinned table-driven in + * plugin-security's `get-effective-object-permissions.test.ts`. + */ +describe('[#20134] super-user wildcard: every bit it grants, per set', () => { + const SCHEMAS: Record = { + crm_account: { name: 'crm_account' }, + crm_lead: { name: 'crm_lead', enable: { apiMethods: ['get', 'list'] } }, + crm_secret: { name: 'crm_secret', access: { default: 'private' } }, + }; + const source = { allSchemas: () => Object.values(SCHEMAS), schemaOf: (n: string) => SCHEMAS[n] }; + + it('the write bypass carries `transfer` onto every entry, a private object\'s included', () => { + const map: any = buildEffectiveObjectPermissions( + [{ objects: { '*': { allowRead: true, allowCreate: true, modifyAllRecords: true } } }], + source, + ); + for (const name of Object.keys(SCHEMAS)) { + expect(map[name], name).toMatchObject({ allowRead: true, allowEdit: true, allowDelete: true, allowTransfer: true }); + } + }); + + it('a super-read wildcard keeps its own plain bits — and nothing it does not grant', () => { + const map: any = buildEffectiveObjectPermissions( + [{ objects: { '*': { viewAllRecords: true, allowEdit: true } } }], + source, + ); + for (const name of Object.keys(SCHEMAS)) { + expect(map[name], name).toMatchObject({ allowRead: true, allowEdit: true, allowCreate: false, allowDelete: false }); + expect(map[name], name).not.toHaveProperty('allowTransfer'); + } + }); + + it('a set that names the object contributes its explicit entry, never its own wildcard\'s grants', () => { + const map: any = buildEffectiveObjectPermissions( + [{ objects: { '*': { viewAllRecords: true, allowTransfer: true }, crm_account: { allowRead: true } } }], + source, + ); + expect(map.crm_account).not.toHaveProperty('allowTransfer'); + expect(map.crm_lead).toMatchObject({ allowRead: true, allowTransfer: true }); + }); + + it('ANOTHER set\'s super-user wildcard widens a present entry, bit by bit', () => { + const map: any = buildEffectiveObjectPermissions( + [ + { objects: { crm_account: { allowRead: true } } }, + { objects: { '*': { viewAllRecords: true, allowTransfer: true, allowExport: true } } }, + ], + source, + ); + // Unrestricted and export-allowed, so annotate has nothing to add to it. + expect(map.crm_account).toEqual({ allowRead: true, allowTransfer: true, allowExport: true }); + }); + + it('a super-user wildcard carrying `allowExport` seeds every registered object — annotate keeps its own skip', () => { + const map: any = buildEffectiveObjectPermissions( + [{ objects: { '*': { allowRead: true, allowEdit: true, modifyAllRecords: true, allowExport: true } } }], + source, + ); + expect(Object.keys(map)).toEqual(['*', 'crm_account', 'crm_lead', 'crm_secret']); + expect(map.crm_account).toMatchObject({ allowRead: true, allowEdit: true, allowTransfer: true, allowExport: true }); + // An unrestricted object whose export stays allowed: an entry, and no operation set on it. + expect(map.crm_account).not.toHaveProperty('apiOperations'); + // A narrowed one is annotated exactly as before, `export` kept. + expect(map.crm_lead.apiOperations).toEqual(['get', 'list', 'aggregate', 'search', 'export']); + }); +}); diff --git a/packages/core/src/security/effective-object-permissions.ts b/packages/core/src/security/effective-object-permissions.ts index f8e0efad472..0955a452451 100644 --- a/packages/core/src/security/effective-object-permissions.ts +++ b/packages/core/src/security/effective-object-permissions.ts @@ -76,8 +76,24 @@ function wildcardGrantsSuperRead(objects: Record): boolean { * the client is told must be derived from the server's actual effective * enforcement, never from an independent reading of the declarations. * - * The super-user grant covers private/managed objects on the server, so folding - * it here is exactly as broad as real enforcement — never broader. + * The super-user grant covers private/managed objects on the server, so the + * read and write bypass reach every entry here. + * + * [#20134] This reads the MERGED wildcard, so it knows only the two bypass + * bits, and only four entry bits. The per-set half, + * {@link foldSuperUserWildcardGrants}, carries what that reading cannot: the + * `transfer` the write bypass grants, and a super-user wildcard's own plain + * bits (`'*': { viewAllRecords, allowEdit }` edits). Two cells here are known + * to be BROADER than enforcement, and are left exactly as they are for the + * over-grant card of this family (#20136): + * - the merged bypass is folded into an entry the super-user set itself names + * narrower — `resolveObjectPermission` answers that set with its explicit + * entry, so the walled `organization_admin` is granted edit on + * `sys_position` here and refused it by the server; + * - `allowCreate` is pulled on the write bypass, which the spec's + * `objectPermissionGrants` deliberately gives no create cell: a + * `'*': { modifyAllRecords: true }` without `allowCreate` is granted create + * here and refused it by the server. */ export function foldWildcardSuperUser(objects: Record): void { const wild = objects?.['*']; @@ -96,6 +112,64 @@ export function foldWildcardSuperUser(objects: Record): void { } } +/** + * [#20134] The per-set half of the super-user fold: each set's SUPER-USER + * `'*'` puts every bit it grants on each entry that set does not name, mutating + * the map in place. Runs after the seed (so every registered object has an + * entry) and BEFORE the managed-write clamp. + * + * Without it the map held only the four bits {@link foldWildcardSuperUser} + * pulls, so `current_user.can(object, 'transfer')` answered `false` for + * `admin_full_access` and the walled `organization_admin` on every object + * where `PermissionEvaluator.checkObjectPermission('transfer', …)` answers + * `true` through `modifyAllRecords`, and a super-read wildcard lost its own + * plain bits (`'*': { viewAllRecords, allowEdit }` read only). + * + * The bits are DERIVED, not listed: for each grant bit, the spec's + * `objectPermissionGrants` — the one fold `checkObjectPermission` itself asks + * — is read on the set's wildcard. For a super-user wildcard its read cell is + * always `true`, so its export cell is exactly the wildcard's own + * `allowExport`, the grant half the evaluator's export conjunction reads. + * + * Exactly as broad as `resolveObjectPermission`, never broader: + * - a set that names the object contributes nothing here: for that set the + * explicit entry is the whole answer, and the merge already carries it; + * - a super-user wildcard covers a private object as much as a public one, + * so no posture is read; + * - only `true` bits are set — a bit the wildcard does not grant is left + * as the other passes left it. + * + * A plain wildcard is {@link materializePlainWildcardCoverage}'s, and a map + * holding no super-user wildcard leaves this pass without a single write. + */ +function foldSuperUserWildcardGrants( + objects: Record, + sets: ReadonlyArray, +): void { + const superWildcards: Array<{ named: Record; bits: readonly GrantBit[] }> = []; + for (const ps of sets) { + const named = ps?.objects as Record | null | undefined; + const wild = named?.['*'] as Record | null | undefined; + if (!named || !wild || typeof wild !== 'object') continue; + // The file's one reading of "is this a super-user wildcard?", asked of the set. + if (!wildcardGrantsSuperRead(named)) continue; + const bits = GRANT_BITS.filter((bit) => objectPermissionGrants(wild as EffectiveObjectPermission, bit)); + superWildcards.push({ named, bits }); + } + if (superWildcards.length === 0) return; + for (const [obj, acc] of Object.entries(objects) as Array<[string, any]>) { + if (obj === '*' || !acc) continue; + for (const { named, bits } of superWildcards) { + // `resolveObjectPermission`'s own test: a set's explicit entry, when it + // has one, is that set's whole answer for the object. + if (named[obj]) continue; + for (const bit of bits) { + if (acc[bit] !== true) acc[bit] = true; + } + } + } +} + /** Minimal schema shape the managed-write clamp needs. */ export interface ManagedSchemaLike { managedBy?: string; @@ -200,6 +274,7 @@ function isPrivatePosture(schema: ObjectAccessPostureLike | null | undefined): b /** The `allow*` bits {@link objectPermissionGrants} reads, i.e. every bit a `can()` verb resolves to. */ const GRANT_BITS = ['allowRead', 'allowCreate', 'allowEdit', 'allowDelete', 'allowTransfer', 'allowExport'] as const; +type GrantBit = (typeof GRANT_BITS)[number]; const GRANT_BIT_SET: ReadonlySet = new Set(GRANT_BITS); /** Does the entry grant any verb on its own? (`objectPermissionGrants` over every verb target.) */ @@ -285,14 +360,14 @@ function materializePlainWildcardCoverage( /** * [#3391] Seed false-initialized per-object entries for a wildcard SUPER-USER, - * for every registered object whose `apiMethods` whitelist tightens exposure. + * for every registered object the merged map does not already carry. * * A super-user's grant is usually the `'*'` wildcard, not explicit per-object - * entries — so restricting objects never appear in the merged `objects` map and - * would miss their `apiOperations` annotation. Seeding a `{allow*: false}` entry - * lets {@link foldWildcardSuperUser} pull it true (super-user reads/writes - * everything) and lets {@link annotateEffectiveApiOperations} attach the effective - * set. Runs BEFORE fold. + * entries — so the objects it reaches never appear in the merged `objects` map. + * Seeding a `{allow*: false}` entry lets {@link foldWildcardSuperUser} and + * {@link foldSuperUserWildcardGrants} pull its grants true and lets + * {@link annotateEffectiveApiOperations} attach the effective operation set + * where the object narrows it. Runs BEFORE both folds. * * [#18990] Admitted by {@link wildcardGrantsSuperRead} — the READ bypass, so * BOTH super-user classes are seeded, and a plain wildcard grant carrying @@ -311,33 +386,37 @@ function materializePlainWildcardCoverage( * (`PermissionEvaluator.checkObjectPermission`), so the entry is exactly as * broad as real enforcement, never broader. * - * [#18931] A schema is skipped only when it needs NO annotation, which is the - * predicate {@link annotateEffectiveApiOperations} itself applies: unrestricted - * AND the export axis leaves `export` in place. Resolving WITHOUT the export - * slot made this pass disagree with annotate's — an unrestricted object whose - * `export` the axis withholds got no entry, annotate (which iterates existing - * entries only) never saw it, and `/me/permissions` stayed silent for exactly - * the population #8681 created: a wildcard-only admin holding no `allowExport`. - * The client's `apiOperations` is then `undefined`, its default-allow path - * renders an Export button, and the click is refused `403 EXPORT_NOT_PERMITTED`. + * [#18931] An unrestricted object whose `export` the export axis withholds is + * seeded too: annotate iterates existing entries only, so without an entry + * `/me/permissions` stayed silent for exactly the population #8681 created — a + * wildcard-only admin holding no `allowExport` — the client's `apiOperations` + * was `undefined`, its default-allow path rendered an Export button, and the + * click was refused `403 EXPORT_NOT_PERMITTED`. + * + * [#20134] And so is EVERY other registered object absent from the map — the + * name keeps its #3391 origin, the scope does not. The pass used to skip an + * unrestricted object whose `export` stays allowed, the one object that needs + * no `apiOperations` annotation. But the entry is not only the annotation's + * carrier: `current_user.can()` reads the map entry by entry and an absent one + * as "no grant", so a super-user wildcard that also carries `allowExport` left + * every such object reading `false` for every verb the server lets that + * principal perform. The entry is the truth the #18990 ruling asks for — every + * object a principal can reach gets one — and annotate keeps its own skip: + * an entry seeded for an unrestricted, export-allowed object carries no + * `apiOperations`, so the client's default-allow path for the operation set is + * exactly as it was. */ export function seedSuperUserRestrictedObjects( objects: Record, allSchemas: readonly ApiExposureSchemaLike[], ): void { if (!wildcardGrantsSuperRead(objects)) return; - // [#18931] The export slot annotate will read for an entry seeded here. A - // seeded entry carries no `allowExport` of its own and `foldWildcardSuperUser` - // does not add one, so annotate's `acc.allowExport ?? wildExport` resolves to - // exactly this wildcard bit — the two passes cannot diverge again. - const userExportAllowed = objects['*']?.allowExport === true; + // A super-user wildcard covers every registered object, private ones + // included (`PermissionEvaluator`'s `resolveObjectPermission`), so every + // registered object the merge left absent gets an entry. for (const schema of allSchemas) { const name = schema?.name; if (!name || name === '*' || objects[name]) continue; - const eff = resolveEffectiveApiMethods(schema.enable ?? undefined, { userExportAllowed }); - // Same skip predicate as annotate: there is nothing to say about an - // unrestricted object that keeps its full operation closure. - if (eff.mode === 'unrestricted' && userExportAllowed) continue; objects[name] = { allowCreate: false, allowRead: false, allowEdit: false, allowDelete: false }; } } @@ -432,7 +511,9 @@ export interface EffectiveObjectPermissionsInputSet { * 3. {@link materializePlainWildcardCoverage} — [#20083] each set's plain * `'*'` onto the registered objects it covers for that set, so the map is * as broad as `checkObjectPermission` there; guarded like (2); - * 4. {@link foldWildcardSuperUser}; + * 4. {@link foldWildcardSuperUser}, then {@link foldSuperUserWildcardGrants} — + * [#20134] each set's super-user `'*'` onto the entries it covers for that + * set, every bit it grants; * 5. {@link clampManagedObjectWrites}; * 6. {@link annotateEffectiveApiOperations} — guarded like (2). * @@ -466,9 +547,10 @@ export function buildEffectiveObjectPermissions( try { return source.allSchemas?.() ?? []; } catch { return [] as ApiExposureSchemaLike[]; } })(); // [#3391] For a wildcard super-user — [#18990] either bypass bit, not - // modify-all alone — seed restricting objects absent from the merged map so - // fold pulls what it pulls and annotate can attach their effective - // apiOperations. Guarded — a failure here must never drop the whole map. + // modify-all alone — seed [#20134] every registered object absent from the + // merged map, so the folds pull what they pull and annotate can attach the + // effective apiOperations where the object narrows them. Guarded — a failure + // here must never drop the whole map. try { seedSuperUserRestrictedObjects(objects, allSchemas); } catch (e: any) { @@ -486,9 +568,12 @@ export function buildEffectiveObjectPermissions( // Make the per-object map reflect the server's ACTUAL effective enforcement // = permission-set grant ∩ identity write guard (ADR-0057 D10, cited as an // attribution, #9628): (1) fold the `'*'` super-user grant into every object - // so an admin's wildcard is not shadowed by another set's explicit deny; + // so an admin's wildcard is not shadowed by another set's explicit deny — + // the merged bypass bits, then [#20134] every bit each set's super-user + // wildcard grants, `transfer` and its own plain bits included; // (2) re-clamp guarded managed objects by their write affordance. foldWildcardSuperUser(objects); + foldSuperUserWildcardGrants(objects, sets); clampManagedObjectWrites(objects, schemaOf); // [#3391] Annotate the per-object effective API operation set. Guarded: on // failure `apiOperations` is simply omitted and a client falls back to its diff --git a/packages/plugins/plugin-hono-server/src/current-user-endpoints-effective-objects.test.ts b/packages/plugins/plugin-hono-server/src/current-user-endpoints-effective-objects.test.ts index a400c6a7007..e1e7ea6a8f9 100644 --- a/packages/plugins/plugin-hono-server/src/current-user-endpoints-effective-objects.test.ts +++ b/packages/plugins/plugin-hono-server/src/current-user-endpoints-effective-objects.test.ts @@ -91,6 +91,10 @@ describe('[#18783] /auth/me/permissions `objects` is the one effective-map funct expect(body.objects.sys_member).toMatchObject({ allowEdit: false }); // clamp over the fold expect(body.objects.report).toMatchObject({ allowRead: true }); // seed for a super-user expect(Array.isArray(body.objects.report.apiOperations)).toBe(true); // annotation + // [#20134] the per-set super-user fold: `modifyAllRecords` grants transfer, on a seeded + // entry and on one another set names narrower alike. + expect(body.objects.report).toMatchObject({ allowTransfer: true }); + expect(body.objects.deal).toMatchObject({ allowTransfer: true }); }); it('[#20083] a PLAIN wildcard reaches every registered public object it covers — the same bytes', async () => { diff --git a/packages/plugins/plugin-hono-server/src/effective-api-operations.test.ts b/packages/plugins/plugin-hono-server/src/effective-api-operations.test.ts index a77c0c48830..1a29ebde30a 100644 --- a/packages/plugins/plugin-hono-server/src/effective-api-operations.test.ts +++ b/packages/plugins/plugin-hono-server/src/effective-api-operations.test.ts @@ -173,21 +173,24 @@ describe('seedSuperUserRestrictedObjects (#3391)', () => { { name: 'locked', enable: { apiMethods: [] } }, // deny-all (restricting) ]; - it('for a modify-all super-user, seeds false-init entries for restricting objects only', () => { - // [#18931] `allowExport: true` is what keeps this case about the `apiMethods` - // DERIVATION — the same reason the annotate block above grants it. Without - // it the export axis also withholds `export`, which is its own reason to - // seed `open_obj`, and this assertion would be reading that instead. The - // withheld-export case is pinned separately below. + // [#20134] INVERTED on purpose. This read "seeds false-init entries for + // restricting objects only" and asserted `open_obj` undefined: an unrestricted + // object whose export stays allowed needs no `apiOperations`, so the seed + // skipped it. But `current_user.can()` reads the entry, and an absent one as + // "no grant", so every such object answered `false` for every verb this + // principal holds. The #18990 ruling's own headline — every object a + // principal can reach gets an entry — now holds for it too; the annotation + // keeps its skip, pinned in the #18931 block below. + it('for a modify-all super-user, seeds a false-init entry for every registered object', () => { + // [#18931] `allowExport: true` is what keeps `open_obj` the export-allowed, + // unrestricted case — the one the seed used to skip. const objects: Record = { '*': { modifyAllRecords: true, viewAllRecords: true, allowExport: true }, }; seedSuperUserRestrictedObjects(objects, schemas); expect(objects.widget).toEqual({ allowCreate: false, allowRead: false, allowEdit: false, allowDelete: false }); expect(objects.locked).toBeDefined(); - // an unrestricted object that keeps its FULL closure is NOT seeded — for it - // the client's default-allow path is already the right answer - expect(objects.open_obj).toBeUndefined(); + expect(objects.open_obj).toEqual({ allowCreate: false, allowRead: false, allowEdit: false, allowDelete: false }); }); // [#18990] INVERTED on purpose. This used to read "does not seed for a @@ -287,15 +290,20 @@ describe('seedSuperUserRestrictedObjects (#3391)', () => { ); }); - it('stays silent for the same object once the principal really may export', () => { + // [#20134] INVERTED on purpose: this read "stays silent" and asserted no + // entry at all. The OPERATION channel still says nothing — the control + // below — but the entry exists, because `current_user.can()` reads it and + // reads an absent one as "no grant" for every verb. + it('once the principal really may export: an entry, and no operation set on it', () => { // The control: same schema, same super-user bits, `allowExport` granted. - // Nothing is withheld, so there is nothing to say and the client's - // default-allow path is correct — no entry, exactly as before #18931. + // Nothing is withheld, so there is no `apiOperations` to say and the + // client's default-allow path for the operation set is correct. const objects: Record = wildcardOnlyAdmin(); objects['*'].allowExport = true; seedSuperUserRestrictedObjects(objects, unrestricted); annotateEffectiveApiOperations(objects, (name) => unrestricted.find((s) => s.name === name)); - expect(objects.crm_lead).toBeUndefined(); + expect(objects.crm_lead).toBeDefined(); + expect(objects.crm_lead).not.toHaveProperty('apiOperations'); }); // [#18990] INVERTED on purpose, under the ruling on #18990 (batch #159 @@ -326,15 +334,17 @@ describe('seedSuperUserRestrictedObjects (#3391)', () => { ); }); - it('stays silent for a viewAll-only principal once it really may export', () => { + it('a viewAll-only principal once it really may export: an entry read true, and no operation set on it', () => { // The ruling's own carve-out — "skip only an unrestricted object whose - // export stays allowed" — is the SAME skip the modify-all control above - // pins, reached through the same predicate and not a second one. + // export stays allowed" — is annotate's skip, and it is the SAME skip the + // modify-all control above pins; [#20134] the entry itself is no longer + // skipped, so `can()` reads the read the server grants. const objects: Record = { '*': { viewAllRecords: true, allowExport: true } }; seedSuperUserRestrictedObjects(objects, unrestricted); foldWildcardSuperUser(objects); annotateEffectiveApiOperations(objects, (name) => unrestricted.find((s) => s.name === name)); - expect(objects.crm_lead).toBeUndefined(); + expect(objects.crm_lead).toMatchObject({ allowRead: true, allowEdit: false }); + expect(objects.crm_lead).not.toHaveProperty('apiOperations'); }); }); }); 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 aef13e0e9dc..b3e9cdc71e2 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 @@ -299,10 +299,21 @@ describe('[#18783] the engine is handed the same producer', () => { * model. There the pin holds the refuse direction only — the map never grants * what the evaluator refuses. * - * Subjects whose sets carry a SUPER-USER wildcard are outside this table: their - * entries are the super-user seed's and fold's, which this card leaves as they - * were, and which the plain-wildcard pass does not touch (pinned in core's - * `effective-object-permissions.test.ts`). + * [#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 + * bare `modifyAllRecords` wildcard, a super-read wildcard carrying plain bits, a + * super-user wildcard carrying `allowExport` (which the seed used to skip), and + * a super-user wildcard beside a set naming an object narrower. Their entries + * used to lack `transfer` (the write bypass grants it) and a super-read + * wildcard's own plain bits, and the `allowExport` shape had no entry at all for + * an unrestricted object; each row is now 0 under-granted. + * + * The super-user fold is also BROADER than the evaluator in two known places. + * Both belong to this family's over-grant card (#20136), and both are + * pre-registered below cell for cell in `KNOWN_OVER_GRANT` — never absorbed + * into the table's rule: the merged bypass folded into an entry the super-user + * set itself names narrower, and `allowCreate` pulled on `modifyAllRecords` + * alone, which the spec's `objectPermissionGrants` gives no create cell. */ describe('[#20083] parity: can() over the member\'s map answers what checkObjectPermission answers', () => { const REGISTERED: Record = { @@ -344,6 +355,68 @@ describe('[#20083] parity: can() over the member\'s map answers what checkObject authored('exporter', { '*': { allowExport: true } }), ], 'a wildcard that grants nothing': [authored('none', { '*': {} })], + // [#20134] Super-user wildcards. + 'platform admin': [shipped('admin_full_access'), shipped('member_default')], + 'platform admin who is also a wall-less org admin': [ + shipped('admin_full_access'), shipped('organization_admin_no_bypass'), shipped('member_default'), + ], + 'walled org admin': [shipped('organization_admin'), shipped('member_default')], + 'a bare modify-all wildcard': [authored('modify_all', { '*': { modifyAllRecords: true } })], + 'a super-read wildcard carrying plain bits': [ + authored('view_all_editor', { '*': { viewAllRecords: true, allowEdit: true, allowTransfer: true } }), + ], + 'a super-user wildcard carrying the export grant': [ + authored('exporting_admin', { + '*': { + allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, + viewAllRecords: true, modifyAllRecords: true, allowExport: true, + }, + }), + ], + 'a super-read wildcard carrying the export grant': [ + authored('exporting_viewer', { '*': { viewAllRecords: true, allowExport: true } }), + ], + 'an explicit entry beside another set\'s super-user wildcard': [ + authored('reader', { crm_account: { allowRead: true } }), + authored('super', { '*': { viewAllRecords: true, allowTransfer: true, allowExport: true } }), + ], + 'one set: a super-user wildcard AND a narrower explicit entry': [ + authored('same_super', { + '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, + crm_account: { allowRead: true }, + }), + ], + }; + + /** + * [#20134] KNOWN DIVERGENCE, pre-registered — the super-user fold's two + * over-grants, both owned by #20136 and both left exactly as they were: + * + * - the merged bypass folded into an entry the super-user set ITSELF names + * narrower (`resolveObjectPermission` answers that set with its explicit + * entry): the walled org admin's read-only RBAC rows, and the one-set row; + * - `allowCreate` pulled on `modifyAllRecords` alone. + * + * The assertion below is EXACT, both ways: a cell missing here fails the row, + * and a cell listed here that no longer diverges fails it too. ⛔ FLIP TRIGGER: + * when #20136 lands, these rows go red; delete each entry it empties — never + * widen one to absorb a new cell. + */ + const WRITE_VERBS = ['create', 'delete', 'edit', 'import', 'remove', 'update', 'write'] as const; + const CREATE_VERBS = ['create', 'import'] as const; + const cellsOf = (objects: readonly string[], verbs: readonly string[]) => + objects.flatMap((object) => verbs.map((verb) => `${object}.${verb}`)); + const KNOWN_OVER_GRANT: Record = { + // `organization_admin` names its RBAC rows read-only; its own `'*'` is folded over them. + 'walled org admin': cellsOf([SysPosition.name, SysPermissionSet.name], WRITE_VERBS), + // The same shape, authored: the set's `crm_account` entry is its whole answer there. + 'one set: a super-user wildcard AND a narrower explicit entry': cellsOf(['crm_account'], WRITE_VERBS), + // `modifyAllRecords` has no create cell; the fold pulls `allowCreate` on every unguarded entry + // (the clamp takes it back on a guarded one). + 'a bare modify-all wildcard': cellsOf( + ['crm_account', 'crm_lead', 'crm_secret', 'crm_hidden', SysAttachment.name, SysUserPreference.name, SysPosition.name, SysPermissionSet.name], + CREATE_VERBS, + ), }; const OPERATION: Record = { @@ -378,6 +451,7 @@ describe('[#20083] parity: can() over the member\'s map answers what checkObject const permissions = toEvalPermissions(await svc.getEffectiveObjectPermissions!({ userId: USER.id })); const wrong: string[] = []; + const overGranted: string[] = []; let cells = 0; for (const schema of Object.values(REGISTERED)) { const isPrivate = schema.access?.default === 'private'; @@ -390,14 +464,54 @@ describe('[#20083] parity: can() over the member\'s map answers what checkObject if (map === server) continue; // The managed-write clamp may only NARROW, and only on its own verbs. if (clamped && CLAMPED.has(target) && server && !map) continue; + // [#20134] A pre-registered over-grant is collected, then held EXACTLY below. + if (map && (KNOWN_OVER_GRANT[label] ?? []).includes(`${schema.name}.${verb}`)) { + overGranted.push(`${schema.name}.${verb}`); + continue; + } wrong.push(`${schema.name}.${verb}: can()=${map} checkObjectPermission=${server}`); } } expect(cells).toBe(Object.keys(REGISTERED).length * OBJECT_PERMISSION_VERB_NAMES.length); expect(wrong).toEqual([]); + // The flip trigger: the known divergence is exactly what it was registered as. + expect([...overGranted].sort()).toEqual([...(KNOWN_OVER_GRANT[label] ?? [])].sort()); }); } + it('[#20134] every pre-registered over-grant names a row of the table', () => { + for (const label of Object.keys(KNOWN_OVER_GRANT)) expect(SUBJECTS, label).toHaveProperty([label]); + }); + + it('[#20134] the reported case, spelled out: the platform admin may transfer, and can() says so', async () => { + const sets = SUBJECTS['platform admin']; + const { svc, plugin } = await locate({ schemas: REGISTERED }); + vi.spyOn(plugin as any, 'resolvePermissionSetsForContext').mockResolvedValue(sets); + const permissions = toEvalPermissions(await svc.getEffectiveObjectPermissions!({ userId: USER.id })); + // `modifyAllRecords` is the write bypass, and `transfer` is in its class. + expect(evaluator.checkObjectPermission('transfer', 'crm_account', sets)).toBe(true); + expect(can(permissions, 'crm_account', 'transfer')).toBe(true); + // …on a private object too: a super-user wildcard covers it. + expect(evaluator.checkObjectPermission('transfer', 'crm_secret', sets, { isPrivate: true })).toBe(true); + expect(can(permissions, 'crm_secret', 'transfer')).toBe(true); + }); + + it('[#20134] a super-user wildcard carrying the export grant: an unrestricted object has an entry, and it answers', async () => { + const sets = SUBJECTS['a super-user wildcard carrying the export grant']; + const { svc, plugin } = await locate({ schemas: REGISTERED }); + vi.spyOn(plugin as any, 'resolvePermissionSetsForContext').mockResolvedValue(sets); + const map: any = await svc.getEffectiveObjectPermissions!({ userId: USER.id }); + const permissions = toEvalPermissions(map); + for (const [verb, operation] of [['read', 'find'], ['edit', 'update'], ['export', 'export']] as const) { + expect(evaluator.checkObjectPermission(operation, 'crm_account', sets)).toBe(true); + expect(can(permissions, 'crm_account', verb), verb).toBe(true); + } + // The operation channel still says nothing about an object that keeps its whole closure… + expect(map.crm_account).not.toHaveProperty('apiOperations'); + // …and still speaks for one whose `apiMethods` narrow it, `export` included. + expect(map.crm_lead.apiOperations).toEqual(expect.arrayContaining(['get', 'list', 'export'])); + }); + it('the wall-less org admin\'s reported case, spelled out: edit on an app object reached only through `*`', async () => { const sets = SUBJECTS['wall-less org admin']; const { svc, plugin } = await locate({ schemas: REGISTERED });