diff --git a/.changeset/25575-modify-all-allow-delete.md b/.changeset/25575-modify-all-allow-delete.md new file mode 100644 index 00000000000..b81946104d4 --- /dev/null +++ b/.changeset/25575-modify-all-allow-delete.md @@ -0,0 +1,31 @@ +--- +'@objectstack/spec': minor +'@objectstack/lint': minor +--- + +feat(spec,lint)!: a permission set can no longer declare `allowDelete: false` beside `modifyAllRecords: true` + +`modifyAllRecords` (Modify All Records) is the write bypass for edit, delete and transfer, as Salesforce's Modify All is. The permission evaluator grants delete on the bypass bit and never reads `allowDelete`, so an object-permission entry declaring `allowDelete: false` beside it parsed, was stored, and let its holder delete every record. The runtime does not change. What changes is what an author can write: + +- `ObjectPermissionSchema` refuses an explicit `allowDelete: false` in an entry that sets `modifyAllRecords: true`, with one `custom` issue at `objects..allowDelete` (the wildcard `*` entry included). The message names the route for "edit every record, delete none". +- An entry that sets `modifyAllRecords: true` and omits `allowDelete` now parses to `allowDelete: true` instead of `false`. That is what the evaluator already granted, and it keeps the parse idempotent: the output of `definePermissionSet` still parses when `defineStack`, a build or a save parses it again. +- `os lint` reports the pair as `security-modify-all-delete-withheld` (error) on a raw config that has not been through the schema. `os explain security-modify-all-delete-withheld` prints the route. +- The response-side `EffectiveObjectPermissionSchema` is unchanged and still accepts the pair. + +"Edit every record, delete none" is a `criteria` sharing rule with `accessLevel: 'edit'` shared with the position that holds the set, beside `allowEdit: true` and `allowDelete: false` with no `modifyAllRecords`. An edit share opens update and never delete (ADR-0111 D3). The object needs an owner field and an owner-write default (`private` or `public_read`), and the rule's `condition` must be a field predicate every row satisfies, such as `record.id != ''`: a literal `true` lowers to an empty filter, which the sharing service never serves (it logs the rule as not served at boot). + +Stored rows and older built artifacts: every earlier parse filled an omitted `allowDelete` with `false`, so the pair also sits where nobody wrote it. The ADR-0087 D2 conversion `permission-modify-all-allow-delete-false-to-true` rewrites it to `true` when a stored row is rehydrated and when a built artifact inside its declared-floor window is loaded. That changes no access decision. It is retired from the load path, so a source that still declares the pair is refused, not rewritten. The D3 entry `permission-modify-all-allow-delete-false-refused` carries the judgment for `os migrate meta`. + +Clause-②: no (narrowing) — the object-permission schema refuses a shape it used to accept (`allowDelete: false` beside `modifyAllRecords: true`). Nothing is widened. + +Metadata-protocol: narrowed (an object-permission entry may no longer declare `allowDelete: false` beside `modifyAllRecords: true`) + +Ruling-ref: 6377890121 (letter A, under 6377842908); retriage 6382861731 + + + +**BREAKING** for an author who declared the pair. + +- **Before.** `{ allowEdit: true, allowDelete: false, modifyAllRecords: true }` parsed, and the holder could delete every record. +- **Now.** The entry is refused at `objects..allowDelete`. +- **Fix.** If the set should delete every record, delete `allowDelete` from the entry. If it should edit records it does not own but delete none, remove `modifyAllRecords` and add a sharing rule: `{ type: 'criteria', name, object, condition: "record.id != ''", accessLevel: 'edit', sharedWith: { type: 'position', value: '' } }`. diff --git a/content/docs/permissions/permission-metadata.mdx b/content/docs/permissions/permission-metadata.mdx index bad26f4188f..e5738fee267 100644 --- a/content/docs/permissions/permission-metadata.mdx +++ b/content/docs/permissions/permission-metadata.mdx @@ -94,7 +94,7 @@ objects: { | `allowDelete` | Soft-delete records | | `allowTransfer` | Transfer record ownership | | `viewAllRecords` | View all records regardless of sharing rules | -| `modifyAllRecords` | Edit all records regardless of sharing rules | +| `modifyAllRecords` | Edit and delete all records regardless of sharing rules; `allowDelete: false` beside it is refused | > **Retired:** the former `allowRestore` / `allowPurge` keys were removed > (#12497, ADR-0049) — the `restore` / `purge` operations they claimed to gate diff --git a/content/docs/permissions/permission-sets.mdx b/content/docs/permissions/permission-sets.mdx index e780f28778a..4b9a5330ab1 100644 --- a/content/docs/permissions/permission-sets.mdx +++ b/content/docs/permissions/permission-sets.mdx @@ -53,7 +53,7 @@ export const SalesUser = definePermissionSet({ | `allowExport` | Bulk data egress — an opt-in grant on top of read, see below | | `allowTransfer` | Lifecycle class: change record ownership — enforced today via the `owner_id` guard (#3004). The former `allowRestore` / `allowPurge` keys were retired (#12497, ADR-0049 — the operations they claimed to gate do not exist yet) and return with the M2 lifecycle batch (#1883) | | `viewAllRecords` | Read ALL records regardless of ownership (super-user read) | -| `modifyAllRecords` | Edit ALL records regardless of ownership (super-user write) | +| `modifyAllRecords` | Edit and delete ALL records regardless of ownership (super-user write); `allowDelete: false` beside it is refused | ### `allowExport` — the export axis diff --git a/content/docs/permissions/permissions-matrix.mdx b/content/docs/permissions/permissions-matrix.mdx index b762f1ec7bd..fc0ce1f34ed 100644 --- a/content/docs/permissions/permissions-matrix.mdx +++ b/content/docs/permissions/permissions-matrix.mdx @@ -30,6 +30,12 @@ ObjectStack's `ObjectPermission` schema defines these boolean flags for object a **Super-user bypass:** When `modifyAllRecords` is set it satisfies write checks (`allowEdit`/`allowDelete`, and the lifecycle bit `allowTransfer`) on any record; `viewAllRecords` (or `modifyAllRecords`) satisfies `allowRead` on any record — both bypass ownership and sharing. See `packages/plugins/plugin-security/src/permission-evaluator.ts`. +**Modify All includes delete.** An entry that sets `modifyAllRecords: true` and +omits `allowDelete` parses to `allowDelete: true`, and an explicit +`allowDelete: false` beside it is refused at `objects..allowDelete`, +because the evaluator would never read it. To let a set edit every record but +delete none, use the [edit-share recipe](/docs/permissions/sharing-rules#recipe--edit-every-record-delete-none). + **The one exception is `allowExport`.** Neither super-user bit confers it: a principal with View/Modify All Data may read every record and still be refused a bulk copy of them. Export is an opt-in grant that must be stated explicitly — diff --git a/content/docs/permissions/sharing-rules.mdx b/content/docs/permissions/sharing-rules.mdx index bf09d759871..b5bb3f84d7c 100644 --- a/content/docs/permissions/sharing-rules.mdx +++ b/content/docs/permissions/sharing-rules.mdx @@ -347,6 +347,55 @@ grants it had issued rather than re-granting the whole object. If you really do want everyone to read every record of an object, that is the object's organization-wide default (`sharingModel`), not a sharing rule. +The gate judges the **lowered filter**, not the intent. A literal +`condition: 'true'` lowers to an empty filter, so the rule is not served (a +declared rule is logged at boot as `match-all-criteria`). A field predicate +that every row happens to satisfy, such as `record.id != ''`, is an ordinary +rule and is served. Use one only where the OWD would grant too much, as in the +recipe below. + +### Recipe — edit every record, delete none + +`modifyAllRecords` cannot express this: Modify All Records includes delete, and +the schema refuses `allowDelete: false` beside `modifyAllRecords: true`. A +sharing rule can, because an `edit` share opens update and never delete +(ADR-0111 D3). On a `public_read` object whose rows an administrator owns: + +```typescript +// The permission set: edit and read, no Modify All, delete withheld. +objects: { + srm_material: { allowRead: true, allowCreate: false, allowEdit: true, allowDelete: false }, +}, + +// The position distributes the set; the rule shares every row with it at `edit`. +definePosition({ name: 'srm_buyer', label: 'Buyer', permissionSets: ['srm_buyer_set'] }); + +defineSharingRule({ + type: 'criteria', + name: 'srm_material_edit_buyers', + object: 'srm_material', + condition: "record.id != ''", + accessLevel: 'edit', + sharedWith: { type: 'position', value: 'srm_buyer' }, +}); +``` + +Measured on a booted in-process stack (`@objectstack/verify`): a member who +holds `srm_buyer` updates a row the administrator owns, and their delete of +that row is refused `403 PERMISSION_DENIED`; a row inserted after the rule is +shared the same way; holding the position without a served rule, the same +update is refused at the record. The rule's grants follow the position's +holders and the object's writes, so nobody re-runs it by hand. Field-level +security narrows which fields the edit may touch. + +Three conditions, each one a reason the share would enforce nothing: + +- The object needs an **owner field** and an owner-write OWD (`private` or + `public_read`). Under `public_read_write` every member with `allowEdit` + already edits every row, so the share adds nothing. +- The condition must be a **field predicate** (see above). +- The permission set still grants `allowEdit`; a share widens rows, never verbs. + > **Retired shapes.** The pre-ADR-0090 `group` recipient was renamed to > `team`, and the `guest` recipient was removed — anonymous access is served > by the [public-form grant](/docs/permissions/authorization) and share diff --git a/content/docs/references/api/protocol.mdx b/content/docs/references/api/protocol.mdx index 7ba9c49b4d2..61aaff0db14 100644 --- a/content/docs/references/api/protocol.mdx +++ b/content/docs/references/api/protocol.mdx @@ -1196,7 +1196,7 @@ This schema accepts one of the following structures: | **allowRestore** | `never` | optional | [REMOVED] `objects..allowRestore` was removed in @objectstack/spec 17 (ADR-0049) — the `restore` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing. Delete the key — a dispatched `restore` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **allowPurge** | `never` | optional | [REMOVED] `objects..allowPurge` was removed in @objectstack/spec 17 (ADR-0049) — the `purge` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing (a compliance/GDPR erase the author believed was permission-locked was not — the operation itself does not exist). Delete the key — a dispatched `purge` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **viewAllRecords** | `boolean` | optional (default: `false`) | View All Data (Bypass Sharing) | -| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. | +| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. Includes delete: an omitted allowDelete parses to true, and an explicit allowDelete: false beside it is refused. | | **readScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Read depth: own\|unit\|unit_and_below\|org | | **writeScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Write depth: own\|unit\|unit_and_below\|org | | **apiOperations** | `Enum<'get' \| 'list' \| 'create' \| 'update' \| 'delete' \| 'upsert' \| 'bulk' \| …>[]` | optional | Server-resolved effective API operations for this object. Present only when the object tightens exposure via apiMethods; absent = default-allow. The frontend renders this effective set, never the raw whitelist. Vocabulary is the EFFECTIVE ApiOperation set (six primitives + eight derived verbs), not the authored six-value ApiMethod enum. | @@ -1648,7 +1648,7 @@ One changed key path of the pending draft, and the last write that changed it. | **allowRestore** | `never` | optional | [REMOVED] `objects..allowRestore` was removed in @objectstack/spec 17 (ADR-0049) — the `restore` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing. Delete the key — a dispatched `restore` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **allowPurge** | `never` | optional | [REMOVED] `objects..allowPurge` was removed in @objectstack/spec 17 (ADR-0049) — the `purge` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing (a compliance/GDPR erase the author believed was permission-locked was not — the operation itself does not exist). Delete the key — a dispatched `purge` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **viewAllRecords** | `boolean` | optional (default: `false`) | View All Data (Bypass Sharing) | -| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. | +| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. Includes delete: an omitted allowDelete parses to true, and an explicit allowDelete: false beside it is refused. | | **readScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Read depth: own\|unit\|unit_and_below\|org | | **writeScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Write depth: own\|unit\|unit_and_below\|org | diff --git a/content/docs/references/security/permission.mdx b/content/docs/references/security/permission.mdx index 07566f59695..0b8bcff05d9 100644 --- a/content/docs/references/security/permission.mdx +++ b/content/docs/references/security/permission.mdx @@ -53,7 +53,7 @@ const result = AdminScopeSchema.parse(data); | **allowRestore** | `never` | optional | [REMOVED] `objects..allowRestore` was removed in @objectstack/spec 17 (ADR-0049) — the `restore` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing. Delete the key — a dispatched `restore` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **allowPurge** | `never` | optional | [REMOVED] `objects..allowPurge` was removed in @objectstack/spec 17 (ADR-0049) — the `purge` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing (a compliance/GDPR erase the author believed was permission-locked was not — the operation itself does not exist). Delete the key — a dispatched `purge` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **viewAllRecords** | `boolean` | optional (default: `false`) | View All Data (Bypass Sharing) | -| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. | +| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. Includes delete: an omitted allowDelete parses to true, and an explicit allowDelete: false beside it is refused. | | **readScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Read depth: own\|unit\|unit_and_below\|org | | **writeScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Write depth: own\|unit\|unit_and_below\|org | | **apiOperations** | `Enum<'get' \| 'list' \| 'create' \| 'update' \| 'delete' \| 'upsert' \| 'bulk' \| 'aggregate' \| 'history' \| 'search' \| 'restore' \| 'purge' \| 'import' \| 'export'>[]` | optional | Server-resolved effective API operations for this object. Present only when the object tightens exposure via apiMethods; absent = default-allow. The frontend renders this effective set, never the raw whitelist. Vocabulary is the EFFECTIVE ApiOperation set (six primitives + eight derived verbs), not the authored six-value ApiMethod enum. | @@ -101,7 +101,7 @@ const result = AdminScopeSchema.parse(data); | **allowRestore** | `never` | optional | [REMOVED] `objects..allowRestore` was removed in @objectstack/spec 17 (ADR-0049) — the `restore` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing. Delete the key — a dispatched `restore` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **allowPurge** | `never` | optional | [REMOVED] `objects..allowPurge` was removed in @objectstack/spec 17 (ADR-0049) — the `purge` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing (a compliance/GDPR erase the author believed was permission-locked was not — the operation itself does not exist). Delete the key — a dispatched `purge` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **viewAllRecords** | `boolean` | optional (default: `false`) | View All Data (Bypass Sharing) | -| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. | +| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. Includes delete: an omitted allowDelete parses to true, and an explicit allowDelete: false beside it is refused. | | **readScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Read depth: own\|unit\|unit_and_below\|org | | **writeScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Write depth: own\|unit\|unit_and_below\|org | @@ -148,7 +148,7 @@ const result = AdminScopeSchema.parse(data); | **allowRestore** | `never` | optional | [REMOVED] `objects..allowRestore` was removed in @objectstack/spec 17 (ADR-0049) — the `restore` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing. Delete the key — a dispatched `restore` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **allowPurge** | `never` | optional | [REMOVED] `objects..allowPurge` was removed in @objectstack/spec 17 (ADR-0049) — the `purge` ObjectQL operation it claimed to gate has never shipped (roadmap M2), so granting the bit delivered nothing (a compliance/GDPR erase the author believed was permission-locked was not — the operation itself does not exist). Delete the key — a dispatched `purge` stays denied fail-closed by the permission evaluator's destructive-operation backstop, and the bit returns with the M2 lifecycle initiative alongside the operation it gates. Run `os migrate meta --from 17` to list the mechanical edits for existing sources; `--write` applies the ones it can prove, and you apply the rest by hand. | | **viewAllRecords** | `boolean` | optional (default: `false`) | View All Data (Bypass Sharing) | -| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. | +| **modifyAllRecords** | `boolean` | optional (default: `false`) | Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. Includes delete: an omitted allowDelete parses to true, and an explicit allowDelete: false beside it is refused. | | **readScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Read depth: own\|unit\|unit_and_below\|org | | **writeScope** | `Enum<'own' \| 'own_and_reports' \| 'unit' \| 'unit_and_below' \| 'org'>` | optional | [ADR-0057 D1] Write depth: own\|unit\|unit_and_below\|org | diff --git a/packages/lint/src/index.ts b/packages/lint/src/index.ts index e71c5206f08..ed988ea6952 100644 --- a/packages/lint/src/index.ts +++ b/packages/lint/src/index.ts @@ -327,6 +327,7 @@ export { SECURITY_OWD_UNSET, SECURITY_OWD_ALIAS, SECURITY_WILDCARD_VAMA, + SECURITY_MODIFY_ALL_DELETE_WITHHELD, SECURITY_ANCHOR_HIGH_PRIVILEGE, SECURITY_ROLE_WORD, SECURITY_BOOK_AUDIENCE_UNKNOWN_SET, diff --git a/packages/lint/src/rule-explanations.ts b/packages/lint/src/rule-explanations.ts index a6d3a1b91fd..2a2d7ecf920 100644 --- a/packages/lint/src/rule-explanations.ts +++ b/packages/lint/src/rule-explanations.ts @@ -756,6 +756,28 @@ const SECURITY_WILDCARD_VAMA_EXPLANATION: RuleExplanation = { ], }; +const SECURITY_MODIFY_ALL_DELETE_WITHHELD_EXPLANATION: RuleExplanation = { + rule: 'security-modify-all-delete-withheld', + covers: 'why Modify All cannot withhold delete, and what can', + paragraphs: [ + '`modifyAllRecords` (Modify All Records) is the write bypass for edit, delete and transfer, as ' + + 'Salesforce\'s Modify All is. The permission evaluator grants delete on the bypass bit before it reads ' + + '`allowDelete`, so `allowDelete: false` beside it is never enforced: the holder deletes every record ' + + 'while the stored set says they cannot.', + 'The schema refuses the pair at `objects..allowDelete`, so `os validate`, `os build`, ' + + '`defineStack` and a metadata save stop before this rule runs. The rule reaches the doors that do not ' + + 'parse first: `os lint` on a raw object-literal config, and a direct call of `validateSecurityPosture`.', + 'To fix it, decide what the set means. If it should delete every record, delete `allowDelete` (an entry ' + + 'that sets `modifyAllRecords: true` and omits it parses to `allowDelete: true`). If it should edit records ' + + 'it does not own and delete none, remove `modifyAllRecords`, keep `allowEdit: true` and ' + + '`allowDelete: false`, and grant the edit with a `criteria` sharing rule with `accessLevel: \'edit\'` ' + + 'shared with the position that holds the set. An edit share opens update and never delete (ADR-0111 D3). ' + + 'The object needs an owner field and an owner-write default (`private` or `public_read`), and the ' + + 'rule\'s `condition` must be a field predicate every row satisfies, such as `record.id != \'\'`: a literal ' + + '`true` lowers to an empty filter, which the sharing service never serves.', + ], +}; + const SECURITY_OWD_ALIAS_EXPLANATION: RuleExplanation = { rule: 'security-owd-alias', covers: 'which sharing values are retired or misplaced', @@ -4887,6 +4909,7 @@ export const RULE_EXPLANATIONS: Readonly> = Obje [SECURITY_FLS_UNKNOWN_FIELD_EXPLANATION.rule]: SECURITY_FLS_UNKNOWN_FIELD_EXPLANATION, [SECURITY_MASTER_DETAIL_UNGRANTED_EXPLANATION.rule]: SECURITY_MASTER_DETAIL_UNGRANTED_EXPLANATION, [SECURITY_WILDCARD_VAMA_EXPLANATION.rule]: SECURITY_WILDCARD_VAMA_EXPLANATION, + [SECURITY_MODIFY_ALL_DELETE_WITHHELD_EXPLANATION.rule]: SECURITY_MODIFY_ALL_DELETE_WITHHELD_EXPLANATION, [SECURITY_OWD_ALIAS_EXPLANATION.rule]: SECURITY_OWD_ALIAS_EXPLANATION, [SECURITY_DELEGATION_MISSING_REASON_EXPLANATION.rule]: SECURITY_DELEGATION_MISSING_REASON_EXPLANATION, [VISIBILITY_ROOT_MISLAYERED_EXPLANATION.rule]: VISIBILITY_ROOT_MISLAYERED_EXPLANATION, diff --git a/packages/lint/src/validate-security-posture.test.ts b/packages/lint/src/validate-security-posture.test.ts index 5cddc5f7b4d..7e910ba7837 100644 --- a/packages/lint/src/validate-security-posture.test.ts +++ b/packages/lint/src/validate-security-posture.test.ts @@ -29,6 +29,7 @@ import { SECURITY_DELEGATION_MISSING_REASON, SECURITY_CBP_NO_RELATION, SECURITY_CBP_AMBIGUOUS_RELATION, + SECURITY_MODIFY_ALL_DELETE_WITHHELD, } from './validate-security-posture.js'; import { lintDataModel } from './data-model-rules.js'; import { explainRule } from './rule-explanations.js'; @@ -194,6 +195,46 @@ describe('validateSecurityPosture (ADR-0090 D7)', () => { expect(findings[0].rule).toBe(SECURITY_WILDCARD_VAMA); }); + // ── Rule: security-modify-all-delete-withheld (#25575) ─────────────── + it('errors on allowDelete: false beside modifyAllRecords: true, at the allowDelete path', () => { + const findings = validateSecurityPosture({ + permissions: [ + { + name: 'buyer', + objects: { srm_material: { allowRead: true, allowEdit: true, allowDelete: false, modifyAllRecords: true } }, + }, + ], + }); + expect(findings.map((f) => [f.rule, f.severity, f.path])).toEqual([ + [SECURITY_MODIFY_ALL_DELETE_WITHHELD, 'error', 'permissions[0].objects.srm_material.allowDelete'], + ]); + }); + + it('does not fire where the false is enforced, or where delete is not withheld', () => { + expect(rulesOf({ + permissions: [{ + name: 'clerk', + objects: { + a: { allowEdit: true, allowDelete: false }, + b: { allowEdit: true, allowDelete: false, viewAllRecords: true }, + c: { allowEdit: true, allowDelete: true, modifyAllRecords: true }, + d: { allowEdit: true, modifyAllRecords: true }, + }, + }], + })).toEqual([]); + }); + + it('never sees the pair on a parsed door: the schema refuses it first', () => { + // The arm exists for the unparsed doors (`os lint` on a raw config, a + // direct call); on the parsed ones the schema is the gate. + const r = PermissionSetSchema.safeParse({ + name: 'buyer', + objects: { srm_material: { allowEdit: true, allowDelete: false, modifyAllRecords: true } }, + }); + expect(r.success).toBe(false); + expect(r.error!.issues.map((i) => i.path.join('.'))).toEqual(['objects.srm_material.allowDelete']); + }); + it("tolerates a plain '*' read wildcard without VAMA", () => { expect( rulesOf({ permissions: [{ name: 'reader', objects: { '*': { allowRead: true } } }] }), @@ -1581,6 +1622,13 @@ const READ_SURFACES: Array<{ receiver: string; expected: string[]; declaredBy: s declaredBy: 'ObjectPermissionSchema', keys: () => shapeKeysOf(ObjectPermissionSchema), }, + // [#25575] One object-permission entry, read by the Modify All delete arm. + { + receiver: 'grant', + expected: ['allowDelete', 'modifyAllRecords'], + declaredBy: 'ObjectPermissionSchema', + keys: () => shapeKeysOf(ObjectPermissionSchema), + }, { receiver: 'action', expected: ['label', 'name'], @@ -1821,6 +1869,7 @@ const RULE_IDS: Record = { SECURITY_DELEGATION_MISSING_REASON, SECURITY_CBP_NO_RELATION, SECURITY_CBP_AMBIGUOUS_RELATION, + SECURITY_MODIFY_ALL_DELETE_WITHHELD, }; const TEXT_FIELD = { a: { type: 'text', label: 'A' } } as const; @@ -1844,6 +1893,10 @@ const REACHABILITY_CORPUS: Array<{ label: string; stack: Record }, }, { label: 'wildcard-vama', stack: { permissions: [{ name: 'ps', label: 'PS', objects: { '*': { viewAllRecords: true } } }] } }, + { + label: 'modify-all-delete-withheld', + stack: { permissions: [{ name: 'ps', label: 'PS', objects: { item: { allowEdit: true, allowDelete: false, modifyAllRecords: true } } }] }, + }, { label: 'anchor-high-privilege', stack: { permissions: [{ name: 'ps', label: 'PS', isDefault: true, objects: { '*': { modifyAllRecords: true } } }] }, @@ -1929,7 +1982,8 @@ describe('validateSecurityPosture — every branch is reachable without an undec it('maps every `findings.push` site in the source', () => { // 16 since #23268: the external dial's alias and wider-than-internal sites left with the key. - expect(pushedRuleIds()).toHaveLength(16); + // 17 since #25575: allowDelete: false beside modifyAllRecords: true. + expect(pushedRuleIds()).toHaveLength(17); }); it('reaches every `findings.push` site from that corpus', () => { diff --git a/packages/lint/src/validate-security-posture.ts b/packages/lint/src/validate-security-posture.ts index c5a3dc770d8..5ba60477883 100644 --- a/packages/lint/src/validate-security-posture.ts +++ b/packages/lint/src/validate-security-posture.ts @@ -20,6 +20,7 @@ * | security-delegation-missing-reason(err) | ADR-0091 D3 dual audit | * | security-cbp-no-relation (error) | #7503 (runtime refusal #7474) | * | security-cbp-ambiguous-relation(error) | #14747 order-dependent master; runtime refusal since #23100 | + * | security-modify-all-delete-withheld(error) | #25575 Modify All includes delete — UNPARSED intakes only, see the arm | * * Per ADR-0049 discipline these are NOT advisory security: every `error` rule * mirrors a runtime enforcement point (D1 fail-closed OWD default, D4 @@ -136,6 +137,7 @@ export const SECURITY_GRANT_EXPIRED_AT_AUTHORING = 'security-grant-expired-at-au export const SECURITY_DELEGATION_MISSING_REASON = 'security-delegation-missing-reason'; export const SECURITY_CBP_NO_RELATION = 'security-controlled-by-parent-no-relation'; export const SECURITY_CBP_AMBIGUOUS_RELATION = 'security-controlled-by-parent-ambiguous-relation'; +export const SECURITY_MODIFY_ALL_DELETE_WITHHELD = 'security-modify-all-delete-withheld'; export type SecuritySeverity = 'error' | 'warning' | 'info'; @@ -913,6 +915,31 @@ export function validateSecurityPosture(stack: AnyRec, opts?: { nowMs?: number } }); } + // [#25575] `allowDelete: false` beside `modifyAllRecords: true`. Modify All + // is the write bypass for edit, delete and transfer, so the evaluator + // grants delete and never reads the false. The schema refuses the pair, so + // on a parsed door this arm never sees it (`ObjectPermissionSchema` refuses + // an authored false and writes `true` into an omitted key). It reaches the + // unparsed doors, as `security-owd-alias` does: `os lint` on a raw + // object-literal config, and a direct call of this exported function. + for (const [objName, entry] of Object.entries(objectsMap)) { + if (!entry || typeof entry !== 'object') continue; + const grant = entry as AnyRec; + if (grant.modifyAllRecords !== true || grant.allowDelete !== false) continue; + findings.push({ + severity: 'error', + rule: SECURITY_MODIFY_ALL_DELETE_WITHHELD, + where: `permission set "${psName}"`, + path: `${psPath}.objects.${objName}.allowDelete`, + message: + `objects.${objName} sets allowDelete: false beside modifyAllRecords: true, and Modify All ` + + `includes delete, so the holder can still delete every record`, + hint: + `Delete allowDelete if Modify All is meant; to edit every record but delete none, remove ` + + `modifyAllRecords and grant the edit with a criteria sharing rule (accessLevel: 'edit')`, + }); + } + const wildcard = objectsMap['*'] as AnyRec | undefined; if (wildcard && (wildcard.viewAllRecords === true || wildcard.modifyAllRecords === true)) { findings.push({ diff --git a/packages/metadata/src/plugin-artifact-forward-conversion.test.ts b/packages/metadata/src/plugin-artifact-forward-conversion.test.ts index a436ff248b7..f6ef964e395 100644 --- a/packages/metadata/src/plugin-artifact-forward-conversion.test.ts +++ b/packages/metadata/src/plugin-artifact-forward-conversion.test.ts @@ -120,8 +120,15 @@ describe('artifact door — 17.1-built artifact converts forward and registers ( // bits, and that pre-existing door behaviour is not under test // here (the conversion's own byte-preservation is pinned by // reference-identity in metadata-core's unit suite). + // + // [#25575] One bit is rewritten on purpose: HotCRM's source + // authors `sales_manager.crm_forecast` with `allowDelete: false` + // beside `modifyAllRecords: true`, the refused pair, so the + // `permission-modify-all-allow-delete-false-to-true` conversion + // writes the `true` the evaluator always acted on. const { allowPurge: _p, allowRestore: _r, ...rest } = grantRaw; - expect(grant).toMatchObject(rest); + const modifyAllDeleteRewritten = rest.modifyAllRecords === true && rest.allowDelete === false; + expect(grant).toMatchObject(modifyAllDeleteRewritten ? { ...rest, allowDelete: true } : rest); } } }); @@ -208,9 +215,14 @@ describe('artifact door — the conversion is versioned, not a blanket amnesty ( expect(current).toMatch(/^\d+\.\d+\.[1-9]\d*$/); const plugin = newPlugin(); const ctx = fakeCtx(); - const fixture = loadFixture(); // untouched: 150 × the emitted default + const fixture = loadFixture(); // 150 × the emitted default const before = loadFixture(); fixture.manifest.engines.protocol = `^${current}`; + // [#25575] An artifact authored against the current surface cannot + // carry `allowDelete: false` beside `modifyAllRecords: true` (the next + // case pins that refusal), so this one carries the bit that surface + // emits. Everything this case judges, the residue, is untouched. + fixture.permissions[2].objects.crm_forecast.allowDelete = true; const total = await plugin._parseAndRegisterArtifact(ctx, fixture, 'fixture-current-residue'); expect(total).toBe(before.permissions.length); @@ -234,6 +246,29 @@ describe('artifact door — the conversion is versioned, not a blanket amnesty ( .filter((m) => m.includes('permission-allow-restore-purge-removed')); expect(conversionWarns).toHaveLength(0); }); + + it('an artifact claiming the CURRENT spec version that carries allowDelete: false beside modifyAllRecords: true is refused, not converted (#25575)', async () => { + // The Modify All delete conversion is versioned like every retired + // entry: an artifact whose floor is past its `retiredAfter` claims a + // surface that already refused the pair, so the door does not rewrite + // it and the strict parse refuses it with its prescription. + const plugin = newPlugin(); + const ctx = fakeCtx(); + const fixture = loadFixture(); + expect(fixture.permissions[2].objects.crm_forecast).toMatchObject({ allowDelete: false, modifyAllRecords: true }); + fixture.manifest.engines.protocol = `^${currentSurfaceFloor()}`; + let refused: any; + try { + await plugin._parseAndRegisterArtifact(ctx, fixture, 'fixture-current-modify-all-delete'); + } catch (e) { + refused = e; + } + expect(refused, 'the strict parse must refuse the pair').toBeDefined(); + const issues = (refused.issues ?? []) as Array<{ code: string; path: PropertyKey[] }>; + expect(issues.map((i) => [i.code, i.path.join('.')])).toEqual([ + ['custom', 'permissions.2.objects.crm_forecast.allowDelete'], + ]); + }); }); describe('artifact door — environment-artifact envelope takes the same policy (#12772)', () => { diff --git a/packages/spec/dropped-refinements.baseline.json b/packages/spec/dropped-refinements.baseline.json index 736bf512801..d9bca71dedb 100644 --- a/packages/spec/dropped-refinements.baseline.json +++ b/packages/spec/dropped-refinements.baseline.json @@ -107,7 +107,7 @@ "manifest.objects.element.membershipKeys.valueType", "manifest.pages.element", "manifest.pages.element.slots.header.options[0].type", - "manifest.permissions.element.objects.valueType.out", + "manifest.permissions.element.objects.valueType.out.out", "manifest.permissions.element.rowLevelSecurity.element", "manifest.reports.element", "manifest.reports.element.blocks.element", @@ -229,7 +229,7 @@ "data.options[1].manifest.objects.element.membershipKeys.valueType", "data.options[1].manifest.pages.element", "data.options[1].manifest.pages.element.slots.header.options[0].type", - "data.options[1].manifest.permissions.element.objects.valueType.out", + "data.options[1].manifest.permissions.element.objects.valueType.out.out", "data.options[1].manifest.permissions.element.rowLevelSecurity.element", "data.options[1].manifest.reports.element", "data.options[1].manifest.reports.element.blocks.element", @@ -253,7 +253,7 @@ }, "api/GetObjectPermissionsResponse": { "sites": [ - "permissions.out" + "permissions.out.out" ] }, "api/GetPackageResponse": { @@ -348,7 +348,7 @@ "options[1].manifest.objects.element.membershipKeys.valueType", "options[1].manifest.pages.element", "options[1].manifest.pages.element.slots.header.options[0].type", - "options[1].manifest.permissions.element.objects.valueType.out", + "options[1].manifest.permissions.element.objects.valueType.out.out", "options[1].manifest.permissions.element.rowLevelSecurity.element", "options[1].manifest.reports.element", "options[1].manifest.reports.element.blocks.element", @@ -414,7 +414,7 @@ "data.packages.element.options[1].manifest.objects.element.membershipKeys.valueType", "data.packages.element.options[1].manifest.pages.element", "data.packages.element.options[1].manifest.pages.element.slots.header.options[0].type", - "data.packages.element.options[1].manifest.permissions.element.objects.valueType.out", + "data.packages.element.options[1].manifest.permissions.element.objects.valueType.out.out", "data.packages.element.options[1].manifest.permissions.element.rowLevelSecurity.element", "data.packages.element.options[1].manifest.reports.element", "data.packages.element.options[1].manifest.reports.element.blocks.element", @@ -1180,12 +1180,12 @@ }, "security/ObjectPermission": { "sites": [ - "out" + "out.out" ] }, "security/PermissionSet": { "sites": [ - "objects.valueType.out", + "objects.valueType.out.out", "rowLevelSecurity.element" ] }, diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index 0b59205d8aa..67a991df227 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -17596,6 +17596,103 @@ const currencyConfigPrecisionRemoved: MetadataConversion = { }, }; +/** + * `allowDelete: false` beside `modifyAllRecords: true` → `allowDelete: true` + * (protocol 18, #25575 — ruling letter A: keep the Modify All semantics, + * refuse the contradictory pair at authoring and publish; the D3 semantic entry + * `permission-modify-all-allow-delete-false-refused`). + * + * `modifyAllRecords` is the write bypass for edit, delete and transfer: the + * permission evaluator grants delete on the bypass bit and never reads + * `allowDelete`. An object-permission entry declaring both therefore parsed, + * was stored, and let its holder delete every record. The authoring schema now + * refuses the pair, and writes `allowDelete: true` into an entry that omits the + * key under `modifyAllRecords: true`. + * + * **Why data at rest needs this entry.** The pair reaches data at rest two + * ways. Authors wrote it: HotCRM's `sales_manager` profile declares + * `crm_forecast` with `allowDelete: false` beside `modifyAllRecords: true`, and + * its built 17.1 artifact carries that entry. And every parse before this + * release filled an omitted `allowDelete` with its `false` default, so every + * built artifact and every stored permission set whose source set + * `modifyAllRecords: true` without naming `allowDelete` carries the pair too + * (the showcase's `showcase_ops` among them). JSON cannot say which `false` was + * authored and which was the default, so the rewrite takes both; an author + * who rebuilds from a source that still declares the pair meets the refusal. + * + * **Lossless against the runtime.** The value written is the one the + * evaluator already acted on: the holder could delete before, and still can. + * No access decision changes. What changes is that the stored entry stops + * claiming a restriction nobody enforced. + * + * **Retired from the load path, from day one.** The authoring funnel does not + * replay it, so a live author meets the parse's refusal and its prescription + * (remove `modifyAllRecords` and grant the edit with a `criteria` sharing rule + * with `accessLevel: 'edit'`) instead of being rewritten to keep a delete grant + * they meant to withhold. Stored rows (`applyConversionsToStoredItem`) and a + * built artifact inside its declared-floor window get the rewrite. Idempotent: + * a converted entry carries `allowDelete: true`, which this transform never + * touches again. + */ +const permissionModifyAllAllowDeleteFalseToTrue: MetadataConversion = { + id: 'permission-modify-all-allow-delete-false-to-true', + toMajor: 18, + retiredFromLoadPath: true, + retiredAfter: '18.1.0', + surface: 'permission.objects.*.allowDelete — false beside modifyAllRecords: true', + summary: + "object-permission 'allowDelete: false' beside 'modifyAllRecords: true' → 'allowDelete: true' " + + '(Modify All Records includes delete: the evaluator never read the false, so the holder could ' + + 'always delete; the entry now says so, and no access decision changes)', + apply(stack, emit) { + return mapCollection(stack, 'permissions', (ps, path) => { + const objects = (ps as { objects?: unknown }).objects; + if (!isDict(objects)) return ps; + let touched = false; + const next: Record = {}; + for (const [name, entry] of Object.entries(objects)) { + if (isDict(entry) && entry.modifyAllRecords === true && entry.allowDelete === false) { + emit({ from: 'false', to: 'true', path: `${path}.objects.${name}.allowDelete` }); + next[name] = { ...entry, allowDelete: true }; + touched = true; + } else { + next[name] = entry; + } + } + return touched ? { ...ps, objects: next } : ps; + }); + }, + fixture: { + before: { + permissions: [{ + name: 'sales_manager', + label: 'Sales Manager', + objects: { + // The pair, authored or filled in by an earlier parse. + crm_forecast: { allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: true }, + // No Modify All: the false is enforced and is left alone. + crm_contact: { allowRead: true, allowEdit: true, allowDelete: false }, + // Already consistent: untouched. + crm_account: { allowRead: true, allowEdit: true, allowDelete: true, modifyAllRecords: true }, + }, + }], + }, + after: { + permissions: [{ + name: 'sales_manager', + label: 'Sales Manager', + objects: { + crm_forecast: { allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true, modifyAllRecords: true }, + crm_contact: { allowRead: true, allowEdit: true, allowDelete: false }, + crm_account: { allowRead: true, allowEdit: true, allowDelete: true, modifyAllRecords: true }, + }, + }], + }, + // One notice: the one entry carrying the pair. + expectedNotices: 1, + }, +}; + /** * RLS-policy `tags` removed (protocol 18, #20321 — ADR-0049 enforce-or-remove, * graded RETIRE by the maintainer's criterion for declared-but-unenforced @@ -18110,6 +18207,7 @@ const MAJOR_18_CONVERSIONS: readonly OrderedConversion[] = [ { conversion: pageTemplateFullWidthToDefault, order: 67 }, { conversion: permissionAdminScopeBindingAuthoringRemoved, order: 65 }, { conversion: permissionAllowRestorePurgeRemoved, order: 16 }, + { conversion: permissionModifyAllAllowDeleteFalseToTrue, order: 75 }, { conversion: permissionRlsTagsRemoved, order: 42 }, { conversion: permissionSetManagedByRemoved, order: 67 }, { conversion: recordChatterPositionVocabulary, order: 2 }, diff --git a/packages/spec/src/migrations/entries/semantic/18.permission-modify-all-allow-delete-false-refused.ts b/packages/spec/src/migrations/entries/semantic/18.permission-modify-all-allow-delete-false-refused.ts new file mode 100644 index 00000000000..9590cdb493a --- /dev/null +++ b/packages/spec/src/migrations/entries/semantic/18.permission-modify-all-allow-delete-false-refused.ts @@ -0,0 +1,61 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import type { SemanticMigration } from '../../types.js'; + +// The data-at-rest half is the D2 conversion +// `permission-modify-all-allow-delete-false-to-true`, retired from the load +// path from day one: stored rows and older built artifacts are rewritten, and a +// live author is refused with the prescription instead. +export const entry: SemanticMigration = { + id: 'permission-modify-all-allow-delete-false-refused', + // No backticks in `surface` — build-upgrade-guide.ts renders it inside a + // code span already, and a nested backtick would close it. + surface: + 'security.permission.objects.*.allowDelete (ObjectPermissionSchema) set to false in an entry that ' + + 'also sets modifyAllRecords: true. Modify All Records is the write bypass for edit, delete and ' + + 'transfer, so the permission evaluator granted delete on the bypass bit and never read the false', + replacement: + 'Decide which of the two the permission set means. To delete every record: delete `allowDelete` from ' + + 'the entry (an entry that sets `modifyAllRecords: true` and omits it now parses to ' + + '`allowDelete: true`), or write `allowDelete: true`. To edit records the holder does not own but ' + + 'delete none: remove `modifyAllRecords`, keep `allowEdit: true` and `allowDelete: false`, and grant ' + + 'the edit with a sharing rule: `{ type: \'criteria\', object, condition, accessLevel: \'edit\', ' + + 'sharedWith: { type: \'position\', value: \'\' } }`. An edit ' + + 'share opens update and never delete (ADR-0111 D3); the object needs an owner field and an ' + + 'owner-write default (`private` or `public_read`). The `condition` must be a field predicate that ' + + 'every row satisfies, such as `record.id != \'\'`: a literal `true` lowers to an empty filter, which ' + + 'the sharing service never serves (it logs the rule as not served at boot, because it would share ' + + 'every record).', + reason: + 'Maintainer ruling A, 2026-10-24: keep the Modify All semantics, which match Salesforce ' + + '"Modify All", and refuse the contradictory pair at publish time; "edit all, delete none" is ' + + 'expressed with row-level sharing. Measured on 18.0.0 by an app author: a buyer permission set ' + + 'declaring `allowEdit: true, allowDelete: false, modifyAllRecords: true` on a `public_read` object ' + + 'could delete every row. The pair parsed, was stored in sys_permission_set.object_permissions, and a ' + + 'census reading the stored entry counted a delete restriction the runtime never enforced ' + + '(ADR-0049 declared ≠ enforced). The refusal is a refinement on the AUTHORING object-permission ' + + 'schema at the allowDelete path; the response-side EffectiveObjectPermissionSchema stays tolerant ' + + '(the authorable/wire split). Because allowDelete defaults to false, the same parse writes ' + + '`allowDelete: true` into an entry that sets `modifyAllRecords: true` and omits the key; without ' + + 'that, every parse output of such an entry would be the refused pair and its next parse would fail. ' + + 'The runtime is unchanged: what the evaluator grants is exactly what it granted before. STORED ROWS ' + + 'AND OLDER BUILT ARTIFACTS: every earlier parse filled an omitted allowDelete with false, so the pair ' + + 'is also present where nobody wrote it. The D2 conversion ' + + '`permission-modify-all-allow-delete-false-to-true` rewrites it to true on stored-row rehydration and ' + + 'for a built artifact inside its declared-floor window, which changes no access decision. It is ' + + 'retired from the load path, so a source that still declares the pair is refused rather than ' + + 'rewritten. Ships at once, no grace window and no dual spelling. ADR-0049 / ADR-0087 / ADR-0111.', + // The rewrite `os migrate meta` applies to a source carrying the pair keeps + // the delete grant the runtime already gave; this entry is the judgment the + // author still owes on it. + conversionIds: ['permission-modify-all-allow-delete-false-to-true'], + acceptanceCriteria: + 'Search every authored permission set for an object-permission entry (wildcard `*` included) that sets ' + + 'both `modifyAllRecords: true` and `allowDelete: false`, and decide as the replacement says: remove ' + + '`allowDelete: false` where Modify All is meant, or replace `modifyAllRecords` with an `edit` sharing ' + + 'rule where delete must stay withheld. `PermissionSetSchema.safeParse` answers exactly one `custom` ' + + 'issue at `objects..allowDelete` for each such entry. Rebuilding an artifact from sources ' + + 'that omit `allowDelete` under Modify All needs no edit: the new parse writes `allowDelete: true`. ' + + 'The repository carried no authored pair at the time of the change; the showcase `showcase_ops` set ' + + 'and the verifier fixture omit `allowDelete` under Modify All and parse to `allowDelete: true`.', +}; diff --git a/packages/spec/src/security/permission.test.ts b/packages/spec/src/security/permission.test.ts index 41dc7ec9cc7..62d6c23ba2c 100644 --- a/packages/spec/src/security/permission.test.ts +++ b/packages/spec/src/security/permission.test.ts @@ -278,6 +278,98 @@ describe('a depth axis beside the super-user bit that short-circuits it is REFUS }); }); +describe('allowDelete: false beside modifyAllRecords: true is REFUSED (Modify All includes delete)', () => { + // The defect: `modifyAllRecords` is the write bypass for edit, delete and + // transfer (`objectPermissionGrants`, and plugin-security's + // `MODIFY_ALL_WRITE_KEYS`), so an entry declaring `allowDelete: false` beside + // it parsed, was stored, and the holder could still delete every record. + // Ruled: keep the Modify All semantics, refuse the pair at authoring and + // publish, and name the sharing-rule route for "edit all, delete none". + + // ── REFUSAL direction ──────────────────────────────────────────────── + it('an explicit allowDelete: false beside modifyAllRecords: true is refused, at the allowDelete path', () => { + const r = ObjectPermissionSchema.safeParse({ + allowRead: true, allowEdit: true, allowDelete: false, viewAllRecords: true, modifyAllRecords: true, + }); + expect(r.success).toBe(false); + const issues = r.error!.issues.filter((i) => i.path[i.path.length - 1] === 'allowDelete'); + expect(issues, 'exactly one issue, LOCATED on the key the author must change').toHaveLength(1); + expect(issues[0].code).toBe('custom'); + // The prescription names the route the author was looking for. + expect(issues[0].message).toContain('modifyAllRecords: true'); + expect(issues[0].message).toContain("accessLevel: 'edit'"); + }); + + it('the refusal reaches through a whole permission set, wildcard entry included', () => { + const r = PermissionSetSchema.safeParse({ + name: 'buyer', + objects: { + srm_material: { allowRead: true, allowEdit: true, allowDelete: false, modifyAllRecords: true }, + '*': { allowRead: true, allowDelete: false, modifyAllRecords: true }, + }, + }); + expect(r.success).toBe(false); + const paths = r.error!.issues.map((i) => i.path.join('.')); + expect(paths).toContain('objects.srm_material.allowDelete'); + expect(paths).toContain('objects.*.allowDelete'); + }); + + // ── The omitted key: the parse must stay idempotent ────────────────── + it('an entry that omits allowDelete under modifyAllRecords: true parses to allowDelete: true', () => { + // Without this, `.default(false)` would emit the refused pair, and the + // next parse of that output would refuse a set its author wrote correctly. + const parsed = ObjectPermissionSchema.parse({ allowRead: true, allowEdit: true, modifyAllRecords: true }); + expect(parsed.allowDelete).toBe(true); + expect(objectPermissionGrants(parsed, 'allowDelete'), 'the parsed bit now says what the fold grants') + .toBe(true); + }); + + it('parse output re-parses unchanged: definePermissionSet → PermissionSetSchema → the stack schema', () => { + const once = PermissionSetSchema.parse({ + name: 'ops', + objects: { announcement: { allowRead: true, allowCreate: true, allowEdit: true, modifyAllRecords: true } }, + }); + const twice = PermissionSetSchema.parse(once); + expect(twice).toEqual(once); + const stack = ObjectStackDefinitionSchema.safeParse({ + manifest: { id: 'com.example.ops', name: 'ops', version: '1.0.0', type: 'app' }, + permissions: [once], + }); + expect(stack.success, JSON.stringify(stack.error?.issues)).toBe(true); + }); + + // ── COST direction: what the refusal must NOT cost ─────────────────── + it('allowDelete: true beside modifyAllRecords: true stays accepted, byte-identical', () => { + const parsed = ObjectPermissionSchema.parse({ allowEdit: true, allowDelete: true, modifyAllRecords: true }); + expect(parsed.allowDelete).toBe(true); + }); + + it('allowDelete: false WITHOUT modifyAllRecords stays accepted and stays false', () => { + expect(ObjectPermissionSchema.parse({ allowEdit: true, allowDelete: false }).allowDelete).toBe(false); + expect(ObjectPermissionSchema.parse({ allowEdit: true, allowDelete: false, modifyAllRecords: false }).allowDelete) + .toBe(false); + // viewAllRecords is the READ bypass only; it does not imply delete. + expect(ObjectPermissionSchema.parse({ allowRead: true, allowDelete: false, viewAllRecords: true }).allowDelete) + .toBe(false); + }); + + it('an entry without modifyAllRecords keeps the false default for allowDelete', () => { + expect(ObjectPermissionSchema.parse({ allowEdit: true }).allowDelete).toBe(false); + }); + + it('the WIRE surface stays tolerant and writes nothing in', () => { + // #4001's authorable/wire split, as for the scope pairs above. + const parsed = EffectiveObjectPermissionSchema.parse({ allowDelete: false, modifyAllRecords: true }); + expect(parsed.allowDelete).toBe(false); + expect(EffectiveObjectPermissionSchema.parse({ modifyAllRecords: true }).allowDelete).toBe(false); + }); + + it('the stage in front keeps the shape read-through the schema walkers read', () => { + expect(Object.keys(ObjectPermissionSchema.shape)).toContain('allowDelete'); + expect(Object.keys(ObjectPermissionSchema.shape)).toContain('modifyAllRecords'); + }); +}); + describe('allowRestore / allowPurge are RETIRED (ADR-0049)', () => { // Removed by the 2026-08-26 maintainer ruling accepting #1883's // recommendation B: the `restore`/`purge` ObjectQL operations the bits diff --git a/packages/spec/src/security/permission.zod.ts b/packages/spec/src/security/permission.zod.ts index bdbca304488..6e772848d17 100644 --- a/packages/spec/src/security/permission.zod.ts +++ b/packages/spec/src/security/permission.zod.ts @@ -277,12 +277,17 @@ const OBJECT_PERMISSION_RETIRED_KEY_RESIDUE = { * semantics of ADR-0090, not a contradictory declaration, and is not judged * here. * + * [#25575] It also refuses an explicit `allowDelete: false` beside + * `modifyAllRecords: true`, for the same reason: the evaluator grants delete + * on the bypass bit and never reads the `false`. See the arm at the end. + * * Module-private on purpose: it has exactly one consumer, the schema below. * Exporting it would put a refinement helper on the published API surface (the * `security` barrel re-exports this module with `export *`) for no caller. */ function checkScopeAgainstSuperUserBits( value: { + allowDelete?: boolean; viewAllRecords?: boolean; modifyAllRecords?: boolean; readScope?: string; @@ -318,6 +323,60 @@ function checkScopeAgainstSuperUserBits( 'is a DIFFERENT shape and stays accepted — viewAllRecords does not bypass write.)', }); } + // [#25575] An explicit `allowDelete: false` beside `modifyAllRecords: true`. + // Only an AUTHORED false reaches here: the stage in front of this schema + // ({@link materializeModifyAllDelete}) writes `allowDelete: true` into an + // entry that sets `modifyAllRecords: true` and omits the key, before the + // `.default(false)` could fill it. + if (value.modifyAllRecords === true && value.allowDelete === false) { + ctx.addIssue({ + code: 'custom', + path: ['allowDelete'], + message: MODIFY_ALL_ALLOW_DELETE_FALSE_MESSAGE, + }); + } +} + +/** + * The refusal of `allowDelete: false` beside `modifyAllRecords: true` + * (#25575). The text names the route the author is looking for, because the + * author who writes this pair wants "edit every record, delete none", and + * Modify All cannot say that. + */ +const MODIFY_ALL_ALLOW_DELETE_FALSE_MESSAGE = + 'allowDelete: false is refused beside modifyAllRecords: true since @objectstack/spec 18 (ADR-0049): ' + + 'Modify All Records is the write bypass for edit, delete and transfer, and the permission evaluator ' + + 'grants delete on the bypass bit before it reads allowDelete, so this false is never enforced. If the set should delete every record, delete allowDelete (or set it to true). If it ' + + 'should edit records it does not own but delete none, remove modifyAllRecords, keep allowEdit: true ' + + "and allowDelete: false, and grant the edit with a sharing rule: a `criteria` rule with accessLevel: 'edit', " + + "a condition every row satisfies (record.id != ''), shared with the position that holds this set. An edit " + + 'share opens update and never delete; delete stays ' + + 'with the record owner. The object needs an owner field and an owner-write default (`private` or ' + + '`public_read`).'; + +/** + * [#25575] The stage in front of the object-permission schema that writes + * `allowDelete: true` into an entry that sets `modifyAllRecords: true` and + * omits `allowDelete`. + * + * `allowDelete` carries `.default(false)`, and a refinement only sees the + * parsed value, so without this stage `{ modifyAllRecords: true }` would parse + * to `{ allowDelete: false, modifyAllRecords: true }`. That output is the + * refused pair: parsing it again (the `definePermissionSet` result passed to + * `defineStack`, a built artifact read at boot, a stored row saved again) + * would refuse a set its author wrote correctly. Writing `true` makes the + * parsed entry say what the evaluator grants (`objectPermissionGrants` folds + * delete into Modify All), and makes the parse idempotent. + * + * Only an ABSENT key is written. An explicit `false` passes through and is + * refused by the refinement; an explicit `true` is already the answer. The + * input type is unchanged, so `allowDelete` stays optional to author. + */ +function materializeModifyAllDelete(body: unknown): unknown { + if (typeof body !== 'object' || body === null || Array.isArray(body)) return body; + const entry = body as Record; + if (entry.modifyAllRecords !== true || Object.prototype.hasOwnProperty.call(entry, 'allowDelete')) return body; + return { ...entry, allowDelete: true }; } /** @@ -504,8 +563,14 @@ const ObjectPermissionBaseSchema = lazySchema(() => strictObject( * RUNTIME change in `plugin-sharing` (option B on #6698) and is deliberately * not taken — what moved here is only the declaration, so that it stops * over-claiming (ADR-0049 `declared ≠ enforced`). + * + * [#25575] It includes DELETE (and transfer), as Salesforce's "Modify All" + * does. An entry that omits `allowDelete` parses to `allowDelete: true`, and + * an explicit `allowDelete: false` beside it is refused, because the + * evaluator never reads it. "Edit every record, delete none" is a `criteria` + * sharing rule with `accessLevel: 'edit'`, not this bit. */ - modifyAllRecords: z.boolean().default(false).describe('Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies.'), + modifyAllRecords: z.boolean().default(false).describe('Modify All Data (Bypass Sharing) — bypasses sharing rules and ownership on the objects record sharing enforces on; on an object with NO owner field sharing abstains, so the platform created_by write floor still applies. Includes delete: an omitted allowDelete parses to true, and an explicit allowDelete: false beside it is refused.'), /** * [ADR-0057 D1] Read access DEPTH (Dataverse-style access level), layered on @@ -561,7 +626,9 @@ const ObjectPermissionBaseSchema = lazySchema(() => strictObject( * them, and the same check on parsed output measures nothing at all. */ export const ObjectPermissionSchema = lazySchema(() => - acceptRetiredDefaultResidue( + // [#25575] The outermost stage sees the RAW entry, so it is the one place an + // omitted `allowDelete` is still distinguishable from an authored `false`. + withModifyAllDeleteMaterialized(acceptRetiredDefaultResidue( // [#16870] The contradictory-pair refusal rides on the BASE, inside the // residue stage, for two measured reasons. (1) In zod 4 `.superRefine()` // on a `ZodObject` returns a `ZodObject` that keeps `.shape`, while the @@ -574,9 +641,28 @@ export const ObjectPermissionSchema = lazySchema(() => // effective-permission response, and an older client must not crash on it. ObjectPermissionBaseSchema.superRefine(checkScopeAgainstSuperUserBits), OBJECT_PERMISSION_RETIRED_KEY_RESIDUE, - ), + )), ); +/** + * [#25575] Put {@link materializeModifyAllDelete} in front of the authoring + * schema, keeping the read-through `shape` the residue stage gives it (the + * schema walkers duck-test `shape`, and a `z.preprocess` pipe has none of its + * own). Only the authoring schema takes this stage: + * {@link EffectiveObjectPermissionSchema} is the tolerant wire surface and + * extends the unrefined base. + */ +function withModifyAllDeleteMaterialized( + schema: S, +): z.ZodType, z.input> & { readonly shape: S['shape'] } { + const pipe = z.preprocess(materializeModifyAllDelete, schema); + Object.defineProperty(pipe, 'shape', { + configurable: true, + get: () => schema.shape, + }); + return pipe as unknown as z.ZodType, z.input> & { readonly shape: S['shape'] }; +} + /** * RESPONSE-side extension of {@link ObjectPermissionSchema} carrying the * server-resolved effective API operation set for one object (#3391). diff --git a/packages/spec/src/shared/retired-key-removal-version.pin.test.ts b/packages/spec/src/shared/retired-key-removal-version.pin.test.ts index 6180307e6ab..6ad96ffefdd 100644 --- a/packages/spec/src/shared/retired-key-removal-version.pin.test.ts +++ b/packages/spec/src/shared/retired-key-removal-version.pin.test.ts @@ -250,6 +250,16 @@ const JOIN: Readonly> = { tombstone(AdminScopeSchema, 'permission.adminScope', 'authorEnvironmentSets'), ], 'permission-set-managed-by-removed': [tombstone(PermissionSetSchema, 'permission', 'managedBy')], + 'permission-modify-all-allow-delete-false-to-true': [ + refusal( + 'allowDelete: false beside modifyAllRecords: true', + () => PermissionSetSchema.safeParse({ + name: 'buyer', + objects: { srm_material: { allowEdit: true, allowDelete: false, modifyAllRecords: true } }, + }), + 'objects', + ), + ], 'manifest-permissions-string-list-removed': [ refusal('manifest.permissions as a list of strings', () => ManifestPermissionsSchema.safeParse(['system.user.read'])), ],