Skip to content

Commit 490d136

Browse files
fix(plugin-security): every member reads and posts comments, and reads the activity history, of the records they can read (#30222)
<!-- claude-projects-attribution: {"github_login":"os-bill"} --> _Requested by **Bill** · [project thread](https://claude.ai/code/project/chan_016iMXWSZDzqWRWvWzdNo8wg?thread=cmsg_016iMXWSZDzqWRWvWzdNo8wgFMKmBZQVKU38CQ6a3dqyHp)_ domain:server#2 · os-bill · session_01Utp6Cbvcmv8YWFhFJwzjKL ## 维护者速读 - **改了什么:** 平台基线 `member_default` 加两行授权:`sys_comment`(读、建、改)和 `sys_activity`(读)。没有删除位、没有行策略、没有导出。 - **为什么改:** 评论和动态默认开在每个对象上,却没有任何出厂权限集授予这两个对象,新应用的普通成员在自己能看的工单上也只看到「没有权限」,不能评论,也看不到谁改了什么。您已「同意 A」。 - **风险与回滚:** 看得到哪些行仍由父记录门决定:成员只能看、只能评论自己能读的记录;外部成员拿不到这个基线,不受影响;`enable.feeds: false` 的对象仍拒评论;动态读取不刷拒绝日志。8 条消融全部转红后还原。回滚即撤本 PR。 - **先看:** `packages/plugins/plugin-security/src/objects/default-permission-sets.ts` 新增的两行和上方注释,再看 `member-default-feed-grants.integration.test.ts`。 - **两分钟验证:** `pnpm --filter @objectstack/plugin-security exec vitest run src/member-default-feed-grants.integration.test.ts`,预期 20 条全绿。 - **需要您做的:** 无,席位落地。 Fixes #30108 Clause-②: no — runtime permission behaviour (human-floor security/permission class, ruled by the maintainer), not a published accept-set or public-surface widening Metadata-protocol: none ## Why: the ruling, and its three conditions measured > 「同意 A」 (maintainer, recorded on #30108 in comment 6633054352). The platform baseline `member_default` grants `sys_comment` (read, create, edit) and `sys_activity` (read). The parent-record gates decide which rows. | Condition (verbatim from 6633054352) | Measured on this branch | Ablation that turns it red | |:--|:--|:--| | "The parent gates hold for an external member, because `member_default` also reaches `everyone`." | **Holds; its premise is corrected.** `member_default` does not reach an external member: a holder of `org_external` is not given the `everyone` anchor (`resolveAuthzContext`, step 5b, `@objectstack/core`), and `humanBaselineFor` (`external-baseline.ts`) resolves `external_default` in the baseline's place. On the real resolver an external with only its baseline is refused both objects (`403 PERMISSION_DENIED`), so the grant widens nothing for externals. An external holding an explicit grant that names both objects reads and posts on its own ticket only: two other tickets and a vault record serve no comment and no activity row, a by-id read finds nothing, and a post to another ticket is refused (`403 RECORD_NOT_ACCESSIBLE`, nothing written). | A6 (external handed the member baseline): the baseline-only external case goes red. A2, A3, A4: the explicit-grant external cases go red. | | "An object with `enable.feeds: false` still refuses on both verbs." | **Holds.** The switch's gate, `enforceFeedsCapability` (plugin-audit `audit-writers.ts`), is registered on two verbs: `beforeInsert`, and since #10170 `beforeUpdate` for a re-thread. Those are also the two write verbs this grant adds, and both still refuse a member: a comment create answers `403 FEEDS_DISABLED` and writes nothing; moving the member's own comment into that object answers `403 FEEDS_DISABLED` and the row keeps its thread. Measured on a record the member reads and edits, so the refusal is the switch and not the row scope. Reading is not one of the switch's verbs, before or after this change: see Acceptance notes. | A5 (feeds gate off): both cases go red. | | "The grant does not bring back the per-object refusal log noise #23279 measured." | **Holds.** The console header's feed read (`find` on `sys_activity`, top 20 by `timestamp` desc, then its `count`) as a member logs **0** `[Security] Access denied` lines, and serves rows. Armed twice in the same file: the member's own read of the vault logs exactly 1 denial, and the same feed read with `canReadObject` withdrawn from the security service logs denials naming the vault. | A4 (activity gate off) reds the "rows only about readable records" half; the `canReadObject` withdrawal is the in-file arm of the log-count half. | No condition failed, so there is no fork. The grant is exactly the ruling's: not narrowed, and option B not taken. ## What changes **`@objectstack/plugin-security` — `member_default`** (`src/objects/default-permission-sets.ts`) names two more objects: ```ts sys_comment: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: false }, sys_activity: { allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false }, ``` - **Which rows is not decided here, and no row policy is added.** Neither object has an owner or user column to scope by, and each row's parent is a different object. plugin-audit's parent-record gates decide the rows: `comment-access-hooks.ts` lists, reads and accepts a post only on a thread whose parent record the caller can read; `activity-read-visibility.ts` serves an activity row only when its record is readable, and skips a parent object the caller cannot read through `canReadObject`, without reading it as the caller. - **Edit** stays under the wildcard `owner_only_writes` floor (the `sys_comment_moderation` carve-out is the delete limb only), so a member edits the comments they wrote and is refused on another member's (`403 PERMISSION_DENIED`). - **No delete bit** on either (ADR-0090 D5: this set is bound to the `everyone` anchor). Deleting a comment stays the moderation path, through a delete bit an ordinary set grants. - The comment above the inbox pair said "`sys_activity` is deliberately NOT included … a separate question if it ever matters". It is rewritten to say why it was left out of that pair (no `user_id` for a `_self` policy) and that it is granted below on its own ruling, so the file no longer contradicts itself. - The new block cites the ruling (6633054352) and names both gates, the capability switches, the anchor rule and the external member's baseline. **Tests** (`packages/plugins/plugin-security/src/`): - `member-default-feed-grants.integration.test.ts` (new, 20 cases): the real `SecurityPlugin` with the shipped sets, the real `AuditPlugin`, SQL over in-memory better-sqlite3, and contexts from the real `resolveAuthzContext` and `assembleExecutionContext` over seeded identity rows (`sys_member.role` `member` and `external`). The app set names only the app's own objects, the shape the card measured. Blocks: (1) a readable record: read, post (author stamped), edit own and refused on another's, read history, no delete; (2) an unreadable record, both by row scope (another requester's ticket) and by object (a vault the member does not read): an unfiltered list serves none of its comments or history and its `count` agrees, a by-id read finds nothing, a post is refused and writes nothing; (3) external members, with and without an explicit grant; (4) `enable.feeds: false`; (5) the feed read's denial-log count, armed both ways. - `member-default-explicit-allow.test.ts`: the pin "`sys_activity` is deliberately NOT granted" is flipped to the new semantics and asserts it: `sys_comment` read, create and edit and no delete; `sys_activity` read only; no select- or insert-class row policy of the baseline on either; the `sys_comment` edit limb is exactly `owner_only_writes`; no export, view-all or modify-all; `external_default` names neither. The inbox block keeps its "no `sys_activity_self`" assertion. - `comment-reaction-own-record-floor.test.ts`: header prose only. Its "(as an app grants `sys_comment` today)" now says the app grants the delete bit and the baseline grants read, create and edit. **Changeset:** `.changeset/30108-member-default-feed-grants.md`, `patch` for `@objectstack/plugin-security`, both declaration lines. ## Acceptance notes - **Reading comments on an `enable.feeds: false` object is not gated by the switch**, before or after this change, and this PR does not change that. It is by design: the switch closes new comments and re-points, and a row written before the flag was flipped off stays workable (#10170's header on `enforceFeedsCapability`); the record page suppresses the discussion panel for such an object (`content/docs/ui/pages.mdx`). So such an object holds only comments written before it was turned off, and a caller who holds `sys_comment` read (whoever an app grants it to today, every member after this PR) can list those through the API, under the parent gate like any other row (read from the code: neither `enforceFeedsCapability` nor `comment-access-hooks.ts` consults the switch on a read; deliberately not pinned here, so a later decision is free to change it). The dispatch's suggested route listed "refuses create and read"; I read the ruling's "both verbs" as the switch's two registered verbs above. If a read refusal is wanted, it is a change to the switch in plugin-audit, not to this grant; listed for the seat as an open question. `enable.activities: false` writes no activity row for its object (measured: zero rows). - **External members still get no comments or activity from the platform.** Measured above; this PR widens nothing for them. A portal app grants them explicitly, under the same gates. - **Reactions are not in the ruling.** `member_default` does not name `sys_comment_reaction`, so an ordinary member reads and posts comments but the record page does not offer them the reaction control (it needs create on `sys_comment_reaction`, `content/docs/frontend/plugins/plugin-detail.mdx`). Listed for the seat; not changed here. - The record page and the Console are not touched: the card's own measurement showed the timeline, comment box and mention delivery working once these two grants are in a set. ## Verification Suites and gates ran on the final tree, head `d22f921b`. The ablations ran on `2086b2d8`, the change before the `main` merge; the merge (#30165) changed three other plugin-security files (`delegated-admin-gate.ts`, `security-plugin-authz-middleware.ts`, a posture test) and nothing in plugin-audit, and the integration file stays 20/20 on `d22f921b`. <details><summary>Ablations (8 legs, each restored and proven by blob hash against HEAD)</summary> Driven through `scripts/ablation-replace.mjs` (WRAP mode; anchor hit exactly once, mutation proven on disk, restore proven by `git hash-object` == the HEAD blob and an empty `git diff HEAD`). `@objectstack/plugin-audit` resolves to its source in this package's vitest config, so no build sits between the mutation and the run. | Leg | Mutation | Red cases | |:--|:--|:--| | A1 | the two grant lines removed (the pre-PR baseline) | 18 of 66 across the integration and explicit-allow files: every positive case, every list/by-id/post case (refused at the object instead), both feeds cases (refused at the object instead of `FEEDS_DISABLED`), the feed-read cases | | A2 | comment read gate's `andIn` skipped | 3: list, by-id read, the explicit-grant external's comment list | | A3 | comment create's parent-read check skipped | 2: the member's and the external's post to an unreadable thread | | A4 | activity read gate's `andIntoWhere` skipped | 4: list, by-id read, the external's history, the feed serving vault rows | | A5 | `enforceFeedsCapability` condition false | 2: create and re-thread into the feeds-off object | | A6 | external handed the member baseline | 1: the baseline-only external reads both objects | | A7 | `allowDelete: true` on `sys_comment` | 4: the integration "deletes nothing" case, the new structural pin, and the pre-existing "the set stays anchor-safe for `everyone`" pin | | A8 | `sys_comment_moderation` widened from `delete` to `all` | 4: the "edits … not one somebody else wrote" case, plus three cases the `all` class reaches through insert and select (observed direction: more red than the one targeted) | Full output: project file `server-seat2/30108/ablations-ran.txt`. </details> <details><summary>Suites</summary> Before the `main` merge (head `2086b2d8`): `pnpm --filter @objectstack/plugin-security test`: Test Files 243 passed (243), Tests 5312 passed, 45 skipped. `pnpm --filter @objectstack/plugin-audit test`: Test Files 70 passed (70), Tests 1128 passed. After merging `main` (`474c030b`) into the branch, on head `d22f921b`, under `scripts/pm/os-verify-lock.sh`: `pnpm install` exit 0; the package's build closure exit 0; `pnpm --filter @objectstack/plugin-security test`: Test Files 243 passed (243), Tests 5316 passed, 45 skipped; `pnpm --filter @objectstack/plugin-audit test`: Test Files 70 passed (70), Tests 1128 passed; the package typecheck exit 0 (the test tsconfig includes all three changed test files). </details> <details><summary>Gates</summary> `node scripts/pm/dispatch-gates.mjs --commands --repo objectstack-ai/objectstack` on head `d22f921b` derived 60 commands (change set: 5 paths, +607 / -13, under the 3000-line human-merge threshold). Each ran with its exit code recorded before any pipe. - 57 exited 0 on the first pass. - 3 exited 3 (prerequisite not met, nothing measured), and each was re-run after its prerequisite was met: - `node scripts/check-plugin-teardown-shape.mjs --self-test`: its positive-control fixture is pinned to commit `621a4876`, which this shallow clone lacked. After `git fetch --depth=1 origin` of that commit: exit 0 (48 cases). - `pnpm check:dual-build-cjs-loads` and `pnpm check:i18n`: both read built output. After a full `pnpm build` (119 of 119 tasks): exit 0 each. - `node scripts/pm/dispatch-gates.mjs --ran` over the final record: "60 derived famil(ies) accounted for — 60 run, 0 NOT-MEASURED", exit 0. - `pnpm lint` (not derived by the tool, run by the dispatch's instruction): exit 0. CI's own families (artifact roster, wide-population families, path-scheduled jobs, type-check lanes) run on this PR's check runs. </details> ## Human Minimum - [x] 1. Schemas start as Zod; `packages/spec` holds no runtime logic (`packages/spec` not touched). - [x] 2. Every name I added uses the casing for its kind. - [x] 3. No formatter ran; `pnpm lint` passes (reading under Gates). - [x] 4. One issue, one branch off an up-to-date `main`, one pull request. - [x] 5. A change that publishes carries a changeset, and both declaration lines are filled in here and in the changeset. - [x] 6. `packages/spec` touched: n/a. - [x] 7. No hand edits to `content/docs/references/`, `packages/*/CHANGELOG.md` or `content/docs/releases/`. - [x] 8. Title, body and comments are in English (the 维护者速读 block and the quoted ruling are Chinese by the Communication rule). --- _Generated by [Claude Code](https://claude.ai/code/session_01Utp6CatT2ajve5PJf1Hhzj)_ Co-authored-by: Claude <noreply@anthropic.com>
1 parent 314ad95 commit 490d136

5 files changed

Lines changed: 607 additions & 13 deletions

File tree

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
---
4+
5+
fix(plugin-security): every member reads and posts comments, and reads the activity history, of the records they can read — the platform baseline `member_default` now grants `sys_comment` (read, create, edit) and `sys_activity` (read)
6+
7+
Clause-②: no — runtime permission behaviour (human-floor security/permission class, ruled by the maintainer), not a published accept-set or public-surface widening
8+
Metadata-protocol: none
9+
10+
Comments (`enable.feeds`) and the activity timeline (`enable.activities`) are on by default on every object, but no shipped permission set named `sys_comment` or `sys_activity`. A member of an app whose permission sets name only the app's own objects — what `create-objectstack` generates — was refused both (`403` on `GET /api/v1/data/sys_comment` and `GET /api/v1/data/sys_activity`), on records they can read: the record page showed "no permission" for comments and activity, and offered no comment box. An app author no longer needs to grant the two objects by hand.
11+
12+
- **Which rows is unchanged.** The grant opens the two objects; plugin-audit's parent-record gates still decide the rows, so a member reads and posts comments, and reads the history, of exactly the records they can read, and nothing of any other record — a record another user owns under the app's row scope, or a record of an object the member does not read. A member edits the comments they wrote. The grant carries no delete bit (deleting a comment stays the moderation path, through a delete bit an app grants), no export and no view-all or modify-all.
13+
- **`enable.feeds: false` still closes comments** on that object: creating a comment there, or moving one into it, answers `403 FEEDS_DISABLED`. `enable.activities: false` still writes no activity row for the object.
14+
- **External members are not reached.** A holder of the `external` organization role does not hold the `everyone` anchor and resolves `external_default` in place of `member_default`, so a portal user still gets comments and activity only through an explicit grant, under the same two gates.
15+
- **No log noise.** A member's activity read skips the parent objects they cannot read without reading them, so the console header's feed logs no `[Security] Access denied` line per skipped object.
16+
- An app that already grants the two objects to its members keeps working unchanged: permission sets merge most-permissively, so its own lines (a delete bit included) still apply beside the baseline's.

‎packages/plugins/plugin-security/src/comment-reaction-own-record-floor.test.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,9 @@
3333
* ## Who grants the bits
3434
*
3535
* The object-level create / read / delete bits come from an ordinary
36-
* position-distributed set an application ships (as an app grants
37-
* `sys_comment` today), never from the `everyone`-bound baseline, which
36+
* position-distributed set an application ships (as an app grants the
37+
* `sys_comment` delete bit; the baseline grants comments' read, create and
38+
* edit, never delete), never from the `everyone`-bound baseline, which
3839
* ADR-0090 D5 keeps free of delete bits. Which shipped set — if any — carries
3940
* them is an open decision on #22566; the floor pinned here holds whatever set
4041
* hands the delete bit out.

‎packages/plugins/plugin-security/src/member-default-explicit-allow.test.ts‎

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { PermissionSetSchema } from '@objectstack/spec/security';
2828
import type { PermissionSet } from '@objectstack/spec/security';
2929
import { PermissionEvaluator } from './permission-evaluator.js';
3030
import { RLSCompiler, RLS_DENY_FILTER } from './rls-compiler.js';
31-
import { defaultPermissionSets, BETTER_AUTH_MANAGED_OBJECTS } from './objects/default-permission-sets.js';
31+
import { defaultPermissionSets, BETTER_AUTH_MANAGED_OBJECTS, EXTERNAL_DEFAULT_PERMISSION_SET } from './objects/default-permission-sets.js';
3232

3333
const evaluator = new PermissionEvaluator();
3434
const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!;
@@ -267,13 +267,10 @@ describe('[#7344] the personal inbox is readable by a member, scoped to their ow
267267
expect(rls.compileFilter(policiesFor(object, 'select') as any, {} as any)).toEqual(RLS_DENY_FILTER);
268268
});
269269

270-
it('`sys_activity` is deliberately NOT granted — the ruling excludes it', () => {
271-
// Named as an explicit negative so a future "while we are here" sweep has to
272-
// argue with the ruling rather than quietly widen past it. It is not a
273-
// per-user-scoped shape; a separate question if it ever matters.
274-
for (const { operation } of AXES) {
275-
expect(allows(operation, [MEMBER_DEFAULT], 'sys_activity'), `sys_activity ${operation}`).toBe(false);
276-
}
270+
it('`sys_activity` rides on its own ruling, not on this one: no `_self` policy scopes it to the caller', () => {
271+
// The inbox ruling left it out because it has no `user_id` to scope by. Its
272+
// grant (the `[#30108]` block below) is scoped by its parent-record gate, so
273+
// no inbox-shaped `_self` carve-out may appear for it.
277274
const names = (MEMBER_DEFAULT.rowLevelSecurity ?? []).map((p: any) => p.name);
278275
expect(names).not.toContain('sys_activity_self');
279276
});
@@ -291,6 +288,60 @@ describe('[#7344] the personal inbox is readable by a member, scoped to their ow
291288
});
292289
});
293290

291+
// [#30108 — maintainer ruling 6633054352, 「同意 A」] A record's comments and its
292+
// activity timeline, for every member who can read the record. The baseline
293+
// opens the two OBJECTS and nothing more; which rows is plugin-audit's
294+
// parent-record gates' answer, driven on a real stack in
295+
// `member-default-feed-grants.integration.test.ts`. What is pinned here is the
296+
// shape of the opening: the part that would quietly widen if it moved.
297+
const FEED_OBJECTS = ['sys_comment', 'sys_activity'] as const;
298+
299+
describe('[#30108] the baseline opens comments and activity, and leaves the rows to the parent gates', () => {
300+
it('sys_comment: a member reads, posts and edits — and holds no delete bit', () => {
301+
expect(allows('find', [MEMBER_DEFAULT], 'sys_comment'), 'read').toBe(true);
302+
expect(allows('insert', [MEMBER_DEFAULT], 'sys_comment'), 'create').toBe(true);
303+
expect(allows('update', [MEMBER_DEFAULT], 'sys_comment'), 'edit').toBe(true);
304+
expect(allows('delete', [MEMBER_DEFAULT], 'sys_comment'), 'delete').toBe(false);
305+
});
306+
307+
it('sys_activity: a member reads — the audit mirror is its only writer', () => {
308+
expect(allows('find', [MEMBER_DEFAULT], 'sys_activity'), 'read').toBe(true);
309+
for (const operation of ['insert', 'update', 'delete']) {
310+
expect(allows(operation, [MEMBER_DEFAULT], 'sys_activity'), operation).toBe(false);
311+
}
312+
});
313+
314+
it.each(FEED_OBJECTS)('%s: no read or create row policy of the baseline stands in for the parent gate', (object) => {
315+
// A select- or insert-class policy here would answer the gate's question with
316+
// a column predicate neither object can answer (no owner or user column, and
317+
// a different parent object on every row).
318+
expect(policiesFor(object, 'select').map((p: any) => p.name), 'select').toEqual([]);
319+
expect(policiesFor(object, 'insert').map((p: any) => p.name), 'insert').toEqual([]);
320+
});
321+
322+
it('sys_comment: an edit stays under the ownership floor — a member edits the comments they wrote', () => {
323+
// The moderation carve-out is the DELETE limb only; the edit limb is the
324+
// wildcard floor, so a member's edit reaches their own comments alone.
325+
expect(policiesFor('sys_comment', 'update').map((p: any) => p.name)).toEqual(['owner_only_writes']);
326+
});
327+
328+
it.each(FEED_OBJECTS)('%s: no export, view-all or modify-all rides along', (object) => {
329+
const perm = (MEMBER_DEFAULT.objects as any)[object];
330+
expect(perm, `${object} is named`).toBeTruthy();
331+
expect(perm.allowDelete ?? false).toBe(false);
332+
expect(perm.allowExport ?? false).toBe(false);
333+
expect(perm.viewAllRecords ?? false).toBe(false);
334+
expect(perm.modifyAllRecords ?? false).toBe(false);
335+
});
336+
337+
it.each(FEED_OBJECTS)('%s: the external baseline does not name it — the grant reaches no external member', (object) => {
338+
const external = defaultPermissionSets.find((p) => p.name === EXTERNAL_DEFAULT_PERMISSION_SET)!;
339+
expect(external, `${EXTERNAL_DEFAULT_PERMISSION_SET} is shipped`).toBeTruthy();
340+
expect(allows('find', [external], object)).toBe(false);
341+
expect(Object.keys(external.objects ?? {})).not.toContain(object);
342+
});
343+
});
344+
294345
describe('[#5491] the admin sets keep their wildcards (this is a BASELINE change only)', () => {
295346
it.each(['admin_full_access', 'organization_admin', 'viewer_readonly'])(
296347
'%s still carries a `*` entry',

0 commit comments

Comments
 (0)