diff --git a/packages/spec/src/contracts/sharing-service.test.ts b/packages/spec/src/contracts/sharing-service.test.ts index ddc6c877ca..3ec4956931 100644 --- a/packages/spec/src/contracts/sharing-service.test.ts +++ b/packages/spec/src/contracts/sharing-service.test.ts @@ -51,3 +51,78 @@ describe('Sharing Service Contract — recipient vocabularies (#4539)', () => { expect(ruleRecipient).toBe(notAuthorable); }); }); + +/** + * [#5125] The three WRITE gates must all document the `modifyAllRecords` + * super-user bypass. + * + * #4647 made the bypass EXPLICIT on the enforcement side: `canEdit`, + * `canDelete` and `canManageShares` all fold through the one + * `ISecurityService.hasWriteBypass` predicate (`hasModifyAllBypass` in + * `@objectstack/plugin-sharing`'s `SharingService`). Two of the three + * docstrings said so; `canEdit`'s did not, and its omission read as a + * deliberate exclusion — the exact opposite of the implementation, and + * strictly worse than silence because `canDelete` sits four lines below + * naming the bypass it supposedly does not share. + * + * A prose pin rather than a behavioural one, because prose is what drifted: + * nothing type-checks a doc comment, and this interface's whole job is to be + * the thing cross-package callers read instead of the plugin. Deleting the + * sentence again turns this red. + */ +describe('[#5125] ISharingService write-gate bypass documentation parity', () => { + it('canEdit / canDelete / canManageShares each name the `modifyAllRecords` bypass', async () => { + const ts = (await import('typescript')).default; + const { readFileSync } = await import('node:fs'); + const { dirname, resolve } = await import('node:path'); + const { fileURLToPath } = await import('node:url'); + + const file = resolve(dirname(fileURLToPath(import.meta.url)), 'sharing-service.ts'); + const source = ts.createSourceFile( + file, + readFileSync(file, 'utf8'), + ts.ScriptTarget.Latest, + /* setParentNodes */ true, + ); + + const iface = source.statements.find( + (s): s is import('typescript').InterfaceDeclaration => + ts.isInterfaceDeclaration(s) && s.name.text === 'ISharingService', + ); + expect(iface, 'ISharingService must still be an interface in this file').toBeDefined(); + + // `getFullText` carries a member's LEADING TRIVIA — its doc comment — so + // the pin reads exactly what an IDE shows on hover, with no assumption + // about how the comment is wrapped. + const docOf = new Map(); + for (const member of iface!.members) { + if (!ts.isMethodSignature(member) || !member.name || !ts.isIdentifier(member.name)) continue; + docOf.set(member.name.text, member.getFullText(source)); + } + + // Anti-vacuity 1: the enumeration found the real contract surface, so a + // rename cannot quietly empty the assertions below. + expect([...docOf.keys()].sort()).toEqual([ + 'buildReadFilter', + 'canDelete', + 'canEdit', + 'canManageShares', + 'grant', + 'listShares', + 'revoke', + ]); + + for (const gate of ['canEdit', 'canDelete', 'canManageShares'] as const) { + expect(docOf.get(gate), `${gate} must document the modifyAllRecords bypass`) + .toContain('modifyAllRecords'); + } + + // Anti-vacuity 2: the search DISCRIMINATES — it is not matching text every + // member happens to carry. `buildReadFilter` is the honest negative: the + // read path has no `hasWriteBypass` branch at all (a View/Modify All Data + // holder reaches every row because the security layer resolves read DEPTH + // to `org`, which short-circuits the filter before sharing is consulted), + // so naming the write bypass there would itself be drift. + expect(docOf.get('buildReadFilter')).not.toContain('modifyAllRecords'); + }); +}); diff --git a/packages/spec/src/contracts/sharing-service.ts b/packages/spec/src/contracts/sharing-service.ts index c25ff95589..ff46c0ca0f 100644 --- a/packages/spec/src/contracts/sharing-service.ts +++ b/packages/spec/src/contracts/sharing-service.ts @@ -126,9 +126,18 @@ export interface ISharingService { /** * Return `true` when the principal in `context` may UPDATE the record - * `(object, recordId)`. Ownership (widened by write DEPTH) OR a write-level - * ({@link ShareAccessLevel} `edit`) share. Always true for system context, + * `(object, recordId)`. Ownership (widened by write DEPTH), a write-level + * ({@link ShareAccessLevel} `edit`) share, OR — [#4647] — the + * `modifyAllRecords` super-user bypass. Always true for system context, * `public` objects, and objects with no owner field. + * + * The bypass is the same `ISecurityService.hasWriteBypass` predicate + * {@link canDelete} and {@link canManageShares} consult, so the three write + * gates cannot drift from each other or from what `security/explain` + * reports. It is asked LAST — after ownership and shares — so an ordinary + * write costs no extra resolution, and it **fails CLOSED** (ADR-0111 D2): + * no security service, a throwing probe, or a principal-less / + * on-behalf-of context leaves the answer at owner-plus-share only. */ canEdit( object: string,