Skip to content

Commit 120e6df

Browse files
baozhoutaoclaude
andauthored
docs(spec): ISharingService.canEdit documents the modifyAllRecords bypass (#5125) (#5818)
`canEdit`'s contract doc listed ownership and an `edit`-level share and stopped there, while the implementation has carried a third branch since #4647: the `modifyAllRecords` super-user write bypass, probed through `ISecurityService.hasWriteBypass` after ownership and shares both fail (`packages/plugins/plugin-sharing/src/sharing-service.ts:398-400`, via `hasModifyAllBypass` at :341-351). The omission was worse than silence because `canDelete` sits four lines below naming the same bypass ("ownership (widened by write DEPTH) or the `modifyAllRecords` super-user bypass ONLY"), so the pair read as a deliberate exclusion on the update gate -- the exact opposite of the code. The wording now matches `canDelete`'s, and names the same permission the implementation actually checks (`bit: 'modify'` -> `op.modifyAllRecords`, `permission-evaluator.ts:superuserBypassSets`). Adds a parity pin over the interface's own JSDoc: the three write gates (`canEdit` / `canDelete` / `canManageShares`) must each name the bypass, `buildReadFilter` must not (the read path has no `hasWriteBypass` branch), and the member enumeration is asserted whole so a rename cannot empty it. Nothing type-checks a doc comment, and prose is what drifted here. Comment + test only: no schema, type, export or behaviour change. Claude-Session: https://claude.ai/code/session_01559M8FVm6W6vDLABL3jvdW Co-authored-by: Claude <noreply@anthropic.com>
1 parent 846ed1f commit 120e6df

2 files changed

Lines changed: 86 additions & 2 deletions

File tree

packages/spec/src/contracts/sharing-service.test.ts

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,3 +51,78 @@ describe('Sharing Service Contract — recipient vocabularies (#4539)', () => {
5151
expect(ruleRecipient).toBe(notAuthorable);
5252
});
5353
});
54+
55+
/**
56+
* [#5125] The three WRITE gates must all document the `modifyAllRecords`
57+
* super-user bypass.
58+
*
59+
* #4647 made the bypass EXPLICIT on the enforcement side: `canEdit`,
60+
* `canDelete` and `canManageShares` all fold through the one
61+
* `ISecurityService.hasWriteBypass` predicate (`hasModifyAllBypass` in
62+
* `@objectstack/plugin-sharing`'s `SharingService`). Two of the three
63+
* docstrings said so; `canEdit`'s did not, and its omission read as a
64+
* deliberate exclusion — the exact opposite of the implementation, and
65+
* strictly worse than silence because `canDelete` sits four lines below
66+
* naming the bypass it supposedly does not share.
67+
*
68+
* A prose pin rather than a behavioural one, because prose is what drifted:
69+
* nothing type-checks a doc comment, and this interface's whole job is to be
70+
* the thing cross-package callers read instead of the plugin. Deleting the
71+
* sentence again turns this red.
72+
*/
73+
describe('[#5125] ISharingService write-gate bypass documentation parity', () => {
74+
it('canEdit / canDelete / canManageShares each name the `modifyAllRecords` bypass', async () => {
75+
const ts = (await import('typescript')).default;
76+
const { readFileSync } = await import('node:fs');
77+
const { dirname, resolve } = await import('node:path');
78+
const { fileURLToPath } = await import('node:url');
79+
80+
const file = resolve(dirname(fileURLToPath(import.meta.url)), 'sharing-service.ts');
81+
const source = ts.createSourceFile(
82+
file,
83+
readFileSync(file, 'utf8'),
84+
ts.ScriptTarget.Latest,
85+
/* setParentNodes */ true,
86+
);
87+
88+
const iface = source.statements.find(
89+
(s): s is import('typescript').InterfaceDeclaration =>
90+
ts.isInterfaceDeclaration(s) && s.name.text === 'ISharingService',
91+
);
92+
expect(iface, 'ISharingService must still be an interface in this file').toBeDefined();
93+
94+
// `getFullText` carries a member's LEADING TRIVIA — its doc comment — so
95+
// the pin reads exactly what an IDE shows on hover, with no assumption
96+
// about how the comment is wrapped.
97+
const docOf = new Map<string, string>();
98+
for (const member of iface!.members) {
99+
if (!ts.isMethodSignature(member) || !member.name || !ts.isIdentifier(member.name)) continue;
100+
docOf.set(member.name.text, member.getFullText(source));
101+
}
102+
103+
// Anti-vacuity 1: the enumeration found the real contract surface, so a
104+
// rename cannot quietly empty the assertions below.
105+
expect([...docOf.keys()].sort()).toEqual([
106+
'buildReadFilter',
107+
'canDelete',
108+
'canEdit',
109+
'canManageShares',
110+
'grant',
111+
'listShares',
112+
'revoke',
113+
]);
114+
115+
for (const gate of ['canEdit', 'canDelete', 'canManageShares'] as const) {
116+
expect(docOf.get(gate), `${gate} must document the modifyAllRecords bypass`)
117+
.toContain('modifyAllRecords');
118+
}
119+
120+
// Anti-vacuity 2: the search DISCRIMINATES — it is not matching text every
121+
// member happens to carry. `buildReadFilter` is the honest negative: the
122+
// read path has no `hasWriteBypass` branch at all (a View/Modify All Data
123+
// holder reaches every row because the security layer resolves read DEPTH
124+
// to `org`, which short-circuits the filter before sharing is consulted),
125+
// so naming the write bypass there would itself be drift.
126+
expect(docOf.get('buildReadFilter')).not.toContain('modifyAllRecords');
127+
});
128+
});

packages/spec/src/contracts/sharing-service.ts

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -126,9 +126,18 @@ export interface ISharingService {
126126

127127
/**
128128
* Return `true` when the principal in `context` may UPDATE the record
129-
* `(object, recordId)`. Ownership (widened by write DEPTH) OR a write-level
130-
* ({@link ShareAccessLevel} `edit`) share. Always true for system context,
129+
* `(object, recordId)`. Ownership (widened by write DEPTH), a write-level
130+
* ({@link ShareAccessLevel} `edit`) share, OR — [#4647] — the
131+
* `modifyAllRecords` super-user bypass. Always true for system context,
131132
* `public` objects, and objects with no owner field.
133+
*
134+
* The bypass is the same `ISecurityService.hasWriteBypass` predicate
135+
* {@link canDelete} and {@link canManageShares} consult, so the three write
136+
* gates cannot drift from each other or from what `security/explain`
137+
* reports. It is asked LAST — after ownership and shares — so an ordinary
138+
* write costs no extra resolution, and it **fails CLOSED** (ADR-0111 D2):
139+
* no security service, a throwing probe, or a principal-less /
140+
* on-behalf-of context leaves the answer at owner-plus-share only.
132141
*/
133142
canEdit(
134143
object: string,

0 commit comments

Comments
 (0)