Repository navigation
Commit 07c842d
fix(plugin-security): a data-door edit of a permission set saved into a writable runtime package updates its own row instead of forking it (#21881)
Fixes #21861
Clause-②: no
## What this changes
Setup saves a permission set through the data door (`PATCH
/api/v1/data/sys_permission_set/:id`). The security plugin's
write-through (`createPermissionSetWriteThrough`,
`packages/plugins/plugin-security/src/permission-set-projection.ts`)
redirects that edit into the metadata store. Its update leg merged the
patch into the stored body and called `saveMetaItem` with no
`packageId`.
A `sys_metadata` row is keyed `(org, type, name, package_id)`, and a
save that names no package targets the package-less row
(`SysMetadataRepository.put`: an omitted `packageId` is the unbound
row). So for a set saved into a writable runtime package (`PUT
/api/v1/meta/permission/:name?package=PKG`), whose only stored row is
bound to that package, the edit answered `200` and minted a second,
package-less active row carrying the edit. The package-bound row stayed
as it was.
The update leg now reads the package binding of the row it edits and
passes it as `packageId`:
- The row is the `overlay` layer of the layered envelope the leg already
holds. `effectiveBodyForRow` takes that layer as the base the patch
merges into.
- Its binding is read from the metadata door's own single-item read,
`protocol.getMetaItem({ type: 'permission', name })`. That read serves
the row through the same served-row resolution as the layered read
(`findServedOverlayRow`, no package), and it states the row's
`package_id` on the item as `_packageId` ("surface the persisted
software-package binding", `protocol.ts`). The binding is not taken from
the patch, from the projected record, or from a registry item.
- No `overlay` layer means no stored row. Then there is nothing to fork,
and the call is unchanged.
- A package-less row carries no `_packageId`, so its save still names no
package.
- A code-shipped set is refused by the lock before this read
(`assertPermissionSetNotPackageDeclared` runs in the pre-pass). The lock
and its refusal do not move.
- A failed binding read is not caught. Guessing the binding would choose
which row the save lands in.
One file of production code, one new non-exported helper. No
`packages/spec` or `packages/metadata-protocol` edit, no new error code,
no new exported symbol, and no second classifier: every target reaching
the save has already been classified `org` by
`classifyPackagedPermissionSet`, so the binding read only tells a
package-bound `org` set from a package-less one.
## The dispatch hypotheses, measured
- **H1 (the update leg saves package-less and forks): confirmed.** On
`origin/main` `88a39c09` (this branch's base, which carries PR #21857),
through the new door pin's own steps, the runtime-package set's first
data-door edit answered `200`. Its active rows went from one, bound to
the package, to two: `{ package_id: null, description: "Edited at the
data door" }` and `{ package_id: "com.dogfood.bind21861", description:
undefined }`. The update leg is the `saveMetaItem` call at `:1374` on
this base. The `:1297` named in the dispatch is the insert leg's call.
- **H2 (where the binding can be read): measured before any edit.** The
projected `sys_permission_set` record reads `managed_by: "admin"`,
`package_id: null`, so the projection does not carry the binding. The
layered read's `overlay` layer has no `_packageId`, and its envelope
`packageId` is null: the code layer is the projection echo.
`getMetaItem`'s `item._packageId` is `com.dogfood.bind21861`, both
through `GET /api/v1/meta/permission/:name` and through
`protocol.getMetaItem` in-process. That existing reader is the one used
here.
- **H3 (only the runtime-package set needs the binding): held.** It is
pinned at the door: the package-less set saves package-less, and the
code-shipped `showcase_contributor` is still refused with `403
NOT_OVERRIDABLE` with its row count unchanged.
- **H4 (other legs): none forks the same way, so none is changed.**
Details are under Acceptance notes.
## Pins
- **Door pin** (new file
`packages/qa/dogfood/test/permission-set-write-through-package-binding.dogfood.test.ts`,
showcase). Every case counts the active `sys_metadata` rows for the name
under both type spellings and in every scope, before and after the edit:
- a runtime-package set: `200`, exactly one row before and after, still
bound to the package and carrying the edit. Edited twice, before and
after the list read (`GET /meta/permission`) that every Studio page load
issues;
- a package-less set: `200`, one package-less row before and after,
carrying the edit;
- a code-shipped set: `403` with `code: NOT_OVERRIDABLE`, and the stored
rows are unchanged.
- **Unit** (`permission-set-projection.test.ts`). The suite's existing
protocol double now keys a stored row by its package, as the repository
does, and serves the row's binding on `getMetaItem`. No new engine
double, so the engine-double ledger does not move. Four cases:
package-bound, package-less, a filtered edit spanning one of each (each
saved into its own row), and the code-shipped refusal (`code` and
`status` asserted, no save, the binding never read). Each case asserts
the row count before and after.
## Ablation (committed fix first, then mutated, then restored)
At `32048afa30`, `scripts/ablation-replace.mjs` replaced the anchor
`item: body, ...packageArg, ...actorArg });` (1 hit, then 0) with `item:
body, ...actorArg });` (0 hits, then 1). This puts the package-less save
back. Blob `e77bd871` became `4186f983`. The run was wrapped in a `trap`
restore on EXIT, INT and TERM.
- Rebuild: `pnpm turbo run build --filter=@objectstack/plugin-security`
exited 1, at DTS only (`TS6133: 'packageArg' is declared but its value
is never read`). The JS was already emitted, and `node
scripts/ablation-dist-preflight.mjs @objectstack/plugin-security "item:
body, ...actorArg }"` found the marker in 2 built files
(`dist/index.js`, `dist/index.mjs`). The ablation was live in the
artifact the door pin consumes.
- Unit: 2 failed, 72 passed. The first red is the row count: `expected [
…(2) ]`, the package-bound row plus `{ package_id: null, description:
"edited at the data door" }`.
- Door pin: 2 failed, 2 passed. The runtime-package set's two cases went
red with two active rows, `{ package_id: "com.dogfood.bind21861",
description: undefined }` and `{ package_id: null, description: "Edited
at the data door" }`. The package-less and code-shipped cases stayed
green, as they should.
- Restore: blob equal to HEAD (`e77bd871`), `git diff HEAD` empty, `git
status --porcelain` empty across the whole tree. After the rebuild,
`--absent` found the marker in 0 of 6 built files. Unit 74 of 74 passed,
door pin 4 of 4 passed.
## Tests (all at `32048afa30`)
- `pnpm --filter @objectstack/plugin-security exec vitest run
--maxWorkers=2`: 167 files, 3604 passed, 45 skipped.
- `pnpm --filter @objectstack/plugin-security run typecheck`, which
includes `check:test-typecheck` over `tsconfig.test.json`: pass. `pnpm
--filter @objectstack/dogfood run typecheck`: pass.
- After a fresh closure rebuild (`turbo run build
--filter=@objectstack/dogfood^...`, 63 tasks): dist preflight shows the
fix's spelling present and the ablation marker absent.
`permission-set-projection.test.ts` 74 of 74 passed. The door pins
`permission-set-write-through-package-binding` and
`permission-set-lock-row-provenance` (PR #21857's) passed 18 of 18.
- Narrowed lint: `eslint --no-inline-config --format json` over the
three changed `.ts` files reported 3 files, 0 errors, 0 warnings. The
population is the config's `**/*.{ts,…}` globs, and the fourth path is
the `.md` changeset. The config never enables type-aware linting (no
`parserOptions.project`), so the diff cannot move a verdict on an
untouched file. The repo-wide `pnpm lint` is left to CI.
- `integration`-tier tests are not relevant (no `packages/cli` path).
## Gates
`node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` derived 70 commands at `32048afa30`. All 70
were run, and each exit code was recorded. 69 exited 0 on the first
pass. `check:dual-build-cjs-loads` exited 3 (`PREREQUISITE NOT MET`: 8
packages outside the dogfood closure had no `dist/`). Those packages
were built (all cache hits), and the gate exited 0 on the rerun. `--ran`
reports: 70 derived, 70 run, 0 NOT-MEASURED, 0 UNRUN. The derivation
names 8 commands the dispatch-time list did not have, all
changeset-related because this PR adds a changeset:
`check-adr-0087-registration` (both), `check-empty-changeset` (both),
`release-rehearsal-clone --self-test`, `release-pending-publish
--self-test`, `check:objectui-changeset` and
`check:pm-changeset-deadline-census`.
## Acceptance notes
- **The write-through's other legs (H4), read rather than changed.**
- **Insert:** the duplicate-name check refuses whenever the name has a
record. A stored row with no record is only transient, because the
projector is awaited on every save and boot reconciliation creates a
record for every overlay. No door was measured to fork through it.
- **Restore:** it re-authors package-less after a delete. A
runtime-package set's delete removes its bound row, because
`deleteMetaItem` with no package matches the row in any package. So a
restore would leave one package-less row: a lost binding, not two rows.
That needs a different fix, since nothing holds the binding once the row
is gone. The leg is also unreachable on a real engine, because no trash
state exists (the leg's own comment). Carrier: none.
- **Reconcile backfill:** only names with no metadata presence, so there
is nothing to fork.
- **Delete:** matches any package, so no fork.
- **Degraded path, unchanged.** When the layered pre-read fails and the
lock answers from the engine registry alone, the leg has no envelope. It
reads no binding and saves package-less, as before. That needs a
transient read failure followed by a successful save. Carrier: none.
- **A stored row bound to a read-only package (a booted code package, or
a `system`/`cloud`-scoped one) with no artifact.** For such a row, the
save would now meet the protocol's own read-only-base refusal, which
`PUT /meta?package=` meets too, instead of forking silently. No door
mints such a row: `saveMetaItem`'s D1 check and
`SysMetadataRepository.assertAllowed` refuse that binding at write time.
Hence `Clause-②: no`. The accepted set does not move, and the edit still
answers `200`.
- **Base.** `origin/main` has moved past `88a39c09`. The incoming
commits include #21871, a `metadata-protocol` change to the
package-scoped list merge. It does not touch `getMetaItem`'s single-item
stamp or `findServedOverlayRow`, which this fix reads. #21860 (PR
#21873) has not landed, so main was not merged, per the dispatch. CI's
merge ref covers the joint state.
- **Docs.** No `content/docs` sentence describes a data-door edit
forking a package-bound set, so none is made false. The older
`permission-sets.mdx` drift about overlays of packaged sets is the one
PR #21857's review already noted. It is untouched.
---
_Generated by [Claude
Code](https://claude.ai/code/session_011K3zqE8Pv1Evw5hc8tZCnN)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent e864db5 commit 07c842d
4 files changed
Lines changed: 371 additions & 11 deletions
File tree
- .changeset
- packages
- plugins/plugin-security/src
- qa/dogfood/test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
110 | 110 | | |
111 | 111 | | |
112 | 112 | | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
113 | 122 | | |
114 | 123 | | |
115 | 124 | | |
116 | | - | |
117 | | - | |
118 | | - | |
119 | | - | |
120 | | - | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
121 | 130 | | |
122 | 131 | | |
123 | 132 | | |
124 | 133 | | |
125 | 134 | | |
126 | 135 | | |
127 | | - | |
| 136 | + | |
128 | 137 | | |
129 | 138 | | |
130 | 139 | | |
| |||
165 | 174 | | |
166 | 175 | | |
167 | 176 | | |
168 | | - | |
| 177 | + | |
| 178 | + | |
169 | 179 | | |
170 | 180 | | |
171 | 181 | | |
172 | | - | |
173 | | - | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
174 | 186 | | |
175 | 187 | | |
176 | 188 | | |
| |||
195 | 207 | | |
196 | 208 | | |
197 | 209 | | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
198 | 224 | | |
199 | 225 | | |
200 | 226 | | |
| |||
1071 | 1097 | | |
1072 | 1098 | | |
1073 | 1099 | | |
| 1100 | + | |
| 1101 | + | |
| 1102 | + | |
| 1103 | + | |
| 1104 | + | |
| 1105 | + | |
| 1106 | + | |
| 1107 | + | |
| 1108 | + | |
| 1109 | + | |
| 1110 | + | |
| 1111 | + | |
| 1112 | + | |
| 1113 | + | |
| 1114 | + | |
| 1115 | + | |
| 1116 | + | |
| 1117 | + | |
| 1118 | + | |
| 1119 | + | |
| 1120 | + | |
| 1121 | + | |
| 1122 | + | |
| 1123 | + | |
| 1124 | + | |
| 1125 | + | |
| 1126 | + | |
| 1127 | + | |
| 1128 | + | |
| 1129 | + | |
| 1130 | + | |
| 1131 | + | |
| 1132 | + | |
| 1133 | + | |
| 1134 | + | |
| 1135 | + | |
| 1136 | + | |
| 1137 | + | |
| 1138 | + | |
| 1139 | + | |
| 1140 | + | |
| 1141 | + | |
| 1142 | + | |
| 1143 | + | |
| 1144 | + | |
| 1145 | + | |
| 1146 | + | |
| 1147 | + | |
| 1148 | + | |
| 1149 | + | |
| 1150 | + | |
| 1151 | + | |
| 1152 | + | |
| 1153 | + | |
| 1154 | + | |
| 1155 | + | |
| 1156 | + | |
| 1157 | + | |
| 1158 | + | |
| 1159 | + | |
| 1160 | + | |
| 1161 | + | |
| 1162 | + | |
| 1163 | + | |
| 1164 | + | |
| 1165 | + | |
| 1166 | + | |
| 1167 | + | |
| 1168 | + | |
| 1169 | + | |
| 1170 | + | |
| 1171 | + | |
| 1172 | + | |
| 1173 | + | |
| 1174 | + | |
| 1175 | + | |
| 1176 | + | |
| 1177 | + | |
| 1178 | + | |
| 1179 | + | |
| 1180 | + | |
| 1181 | + | |
| 1182 | + | |
| 1183 | + | |
| 1184 | + | |
| 1185 | + | |
| 1186 | + | |
| 1187 | + | |
| 1188 | + | |
| 1189 | + | |
| 1190 | + | |
| 1191 | + | |
| 1192 | + | |
| 1193 | + | |
| 1194 | + | |
| 1195 | + | |
| 1196 | + | |
| 1197 | + | |
| 1198 | + | |
| 1199 | + | |
| 1200 | + | |
| 1201 | + | |
| 1202 | + | |
| 1203 | + | |
| 1204 | + | |
1074 | 1205 | | |
1075 | 1206 | | |
1076 | 1207 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1091 | 1091 | | |
1092 | 1092 | | |
1093 | 1093 | | |
| 1094 | + | |
| 1095 | + | |
| 1096 | + | |
| 1097 | + | |
| 1098 | + | |
| 1099 | + | |
| 1100 | + | |
| 1101 | + | |
| 1102 | + | |
| 1103 | + | |
| 1104 | + | |
| 1105 | + | |
| 1106 | + | |
| 1107 | + | |
| 1108 | + | |
| 1109 | + | |
| 1110 | + | |
| 1111 | + | |
| 1112 | + | |
| 1113 | + | |
| 1114 | + | |
| 1115 | + | |
| 1116 | + | |
| 1117 | + | |
| 1118 | + | |
| 1119 | + | |
| 1120 | + | |
| 1121 | + | |
| 1122 | + | |
| 1123 | + | |
| 1124 | + | |
| 1125 | + | |
| 1126 | + | |
| 1127 | + | |
| 1128 | + | |
| 1129 | + | |
| 1130 | + | |
| 1131 | + | |
| 1132 | + | |
| 1133 | + | |
| 1134 | + | |
1094 | 1135 | | |
1095 | 1136 | | |
1096 | 1137 | | |
| |||
1368 | 1409 | | |
1369 | 1410 | | |
1370 | 1411 | | |
1371 | | - | |
| 1412 | + | |
| 1413 | + | |
1372 | 1414 | | |
1373 | 1415 | | |
1374 | | - | |
| 1416 | + | |
| 1417 | + | |
| 1418 | + | |
1375 | 1419 | | |
1376 | 1420 | | |
1377 | 1421 | | |
| |||
0 commit comments