Simplify policy import model: replace entriesAdditions and importsAliases with entry-level references#2424
Conversation
hu-ahmed
left a comment
There was a problem hiding this comment.
Thanks @thjaeckle — the 4 design answers all verified against code and runtime:
- A1 (one-level resolution, no cycles) — confirmed at
PolicyImporter.java:307-324 - A2 (409 on delete of referenced entry) — verified live:
"policies:entry.referenceconflict"with clear message naming both entries - A3 (self-reference rejection at write time) — verified on
PUT /references,PUT /entries/{label}, andPUT /policies/{id} - A4 (
allowedImportAdditionsonly for import refs) — confirmed atPolicyImporter.java:366-368
Four follow-up items from live testing against the branch:
1. Local-ref-only policies silently lose authorization
This is the one I'd most like to address before merge. Reproduced live:
Same policy structure; only difference is top-level imports{} being empty vs populated.
imports: {} → Alice GET /features/temp → HTTP 404
imports: {ns:template-empty: {}} → Alice GET /features/temp → HTTP 200
Root cause: Policy.java:470 gates resolveReferences behind !imports.isEmpty(). A policy that uses only local references — exactly the shape in the power-plant example in this issue's body — never triggers reference resolution. Suggest calling resolveReferences whenever any entry has non-empty references, regardless of whether imports is empty.
2. Status code / exception type inconsistency
The issue body describes rule 1 as "400 Bad Request." Actual behavior:
| Scenario | Status | Error code |
|---|---|---|
PUT /entries/{label}/references with undeclared import |
400 | policies:import.invalid |
CreatePolicy / ModifyPolicy / ModifyPolicyEntry with invalid reference |
403 | policies:entry.modificationinvalid |
| Self-reference (any path) | 403 | policies:entry.modificationinvalid |
| Ref to non-existent local entry | 403 | policies:entry.modificationinvalid |
PolicyEntryModificationInvalidException is hardcoded HttpStatus.FORBIDDEN at line 57 with a DEFAULT_DESCRIPTION about WRITE-on-policy:/ — it's the "no-writer-left" exception. Reusing it for reference validation produces a 403 where 400 is more appropriate, and a cross-command inconsistency (400 via one endpoint, 403 via others). Suggest PolicyEntryInvalidException (400) or a reference-specific exception.
3. Silent drop of non-object elements in references arrays
PUT /entries/operator/references
Body: [{"entry":"shared"}, 42, "bogus"]
Response: 204
Stored: [{"entry":"shared"}]
Six call sites using .filter(JsonValue::isObject) — ImmutablePolicyEntry.readReferences, ModifyPolicyEntryReferences.fromJson/setEntity, PolicyEntryReferencesModified.fromJson/setEntity,
RetrievePolicyEntryReferencesResponse, AbstractPolicyMappingStrategies.entryReferencesFrom. Same antipattern class flagged before — worth an explicit validating loop that throws PolicyEntryInvalidException naming the bad element.
4. Cross-command READ authorization for references
PolicyImportsPreEnforcer.apply (lines 94-108) routes only ModifyPolicyEntryReferences to validateReferencesModification. For CreatePolicy, ModifyPolicy, ModifyPolicyEntries, and ModifyPolicyEntry — only top-level imports are READ-checked, per-entry reference targets are not. A caller can add an import reference to a template entry they don't have READ on by going through ModifyPolicyEntry rather than /references. Impact scope: information disclosure at resolution time rather than privilege escalation (import refs don't merge subjects), but a cross-command discipline gap worth closing.
Smaller notes:
PolicyImporter.rewriteLabeldropsreferencesfrom imported entries (6-param factory) while the merge path preserves them (7-param). Either rejectreferenceson non-NEVERentries at write time, or extract a
cloneEntryhelper so no path reconstructs aPolicyEntrywith fewer than 7 fields.- Duplicate references (
[{entry:"x"},{entry:"x"}]) are accepted as-is; resolution merges twice. - No ETag support on
/referencessub-resource. ImmutablePolicyBuilder.setPolicyEntryonly putsentryReferenceswhen non-empty — re-setting with an empty list doesn't clear prior value (latent).
78fc0c7 to
ce0ecd0
Compare
|
System tests passed: https://github.com/eclipse-ditto/ditto/actions/runs/24784871727 |
PR eclipse-ditto/ditto#2424 makes allowedImportAdditions a runtime filter rather than a write-time contract: own subjects/resources that exceed the strictest allowedImportAdditions across an entry's import refs are silently stripped at enforcement time, not rejected on PUT. - Rewrite the two ITs that previously expected BAD_REQUEST on the PUT and instead verify the timeline: write succeeds, runtime strips the disallowed own addition, thing access flips accordingly. - Strengthen removingResourcesFromAllowedAdditions... from a getPolicy smoke check to a real NO_CONTENT->FORBIDDEN attribute write flip. - Add a new IT covering the multi-import intersection path: two templates with disjoint allowed sets reduce the intersection to empty, so the consumer's own subject and own resource are both stripped at runtime. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1297d0b to
ca017aa
Compare
|
Ready FMPOV - System tests passed: |
…ases with entry-level references Resolves eclipse-ditto#2423 Replace the `entriesAdditions`, `importsAliases`, and per-import alias concepts with a single `references` array on policy entries. Each reference is either an import reference (`{"import": "policyId", "entry": "label"}`) or a local reference (`{"entry": "label"}`), inheriting subjects, resources, and namespaces from the referenced entry. Local and import references share the same merge semantics; only the lookup site differs. The previously-introduced `allowedImportAdditions` field is renamed to `allowedAdditions` (the field now governs both local and import references) and gains explicit three-tier semantics: - absent → no restriction (upgrade-friendly default) - empty `[]` → deny all own additions - list → only the listed kinds (subjects/resources/namespaces) of own additions survive at enforcement time `allowedAdditions` is enforced at resolution time as a runtime filter on the referencing entry's own additions. The strictest set across all references on an entry applies. The write itself is not rejected for own additions that violate the filter — they are silently stripped at enforcement time. This is intentional: effective permissions depend on the live state of every referenced entry, which can change. Model: - Add `EntryReference` with import and local reference variants - Add `references` array on `PolicyEntry`; subjects and resources become optional when references are present - Rename `AllowedImportAddition` → `AllowedAddition` and the corresponding constants/methods/signal types/HTTP paths - Strict parsing of `references` and `allowedAdditions` arrays via shared `PoliciesModelFactory` helpers (non-object/non-string elements rejected with `PolicyEntryInvalidException`) - Reject `{"import": null}` and `{"import": ""}` explicitly in `ImmutableEntryReference.fromJson` - Lenient parsing for legacy `entriesAdditions`/`importsAliases` fields on snapshot recovery (silently ignored — no operator migration step required) Resolution (`PolicyImporter`): - One-level resolution: local refs do not chain transitively; cross-policy reach requires `transitiveImports` - `importable: never` skips both local and import refs - Multiple import references from different policies resolve independently, then merge against the strictest `allowedAdditions` - 3-level reference resolution: imported policy's references are resolved before being imported; transitive entries get their import prefix; subjects merge additively for import refs - Mutual local references (A→B, B→A) terminate after one pass - First-listed reference wins on subject ID collisions - Fail loud on `MAX_TRANSITIVE_RESOLUTION_DEPTH` (`PolicyImportInvalidException`, HTTP 400) instead of silent truncation - WARN-logging callback fires once per missing reference at enforcement time so silent permission degradation is visible Validation (write time): - Local refs must target existing entries; import refs must target declared imports and pass READ authorization - Reject self-references and duplicate references (same import+entry pair) - Reject local refs to `importable: never` entries - Reject reference modifications that would dangle on import filter shrinkage (409 PolicyImportReferenceConflictException) - `PoliciesValidator` resolves local references before validating the WRITE-on-`policy:/` invariant, so a policy that splits the admin subject and the WRITE grant across two mutually-referencing entries is now correctly accepted Pre-enforcer: - `validateReferencesIntegrity` shared helper for all reference-carrying commands (CreatePolicy, ModifyPolicy, ModifyPolicyEntries, ModifyPolicyEntry, ModifyPolicyEntryReferences) - Synchronous declared-imports check for CreatePolicy/ModifyPolicy (no cache lookup) - READ-access enforcement on import-reference targets HTTP API: - `GET/PUT/DELETE /policies/{id}/entries/{label}/references` - `GET/PUT/DELETE /policies/{id}/entries/{label}/allowedAdditions` - 201 Created on first PUT /references; 204 on subsequent updates - ETag support on /references retrievals - Removed: /entriesAdditions, /importsAliases routes (and signals) Signals & strategies: - New: ModifyPolicyEntryReferences(Response), RetrievePolicyEntry- References(Response), PolicyEntryReferencesModified - Renamed: ModifyPolicyEntryAllowed{Import,}Additions and friends - Strategies delegate duplicate-ref / self-ref / local-existence / import-existence checks to the shared validator - Removed: all entriesAdditions and importsAliases signals, strategies, and protocol mappings Protocol adapter: - Full Ditto-Protocol round-trip mappings for the new signals - `PolicyResource` enum and `PolicyPathMatcher` register `/entries/{label}/references` and `/entries/{label}/allowedAdditions` Documentation: - `basic-policy.md`: replace the entriesAdditions/importsAliases sections with a unified "Entry references" section; document one-level resolution, first-wins merge, runtime-filter semantics of allowedAdditions, and the WRITE-on-references == WRITE-on-entry authority equivalence; update the power-plant template example and the three-level transitive hierarchy example - OpenAPI: new `references.yml` and `allowedAdditions.yml` schemas/paths; api-2-index updated; `ditto-api-2.yml` regenerated via the documented npm build - JSON schema: `policy.json` updated; description on `allowedAdditions` matches the three-tier semantics Tests: - `PolicyImporterTest`: 2/3-level reference resolution, cycle/depth termination, NEVER-marker uniformity, NAMESPACES gating, multiple import refs from different policies, conflicting allowedAdditions across imports, empty allowedAdditions strips all own additions, subject overlap (template wins), mutual local-ref cycle, missing-reference callback - `ImmutableEntryReferenceTest`: null/empty `import` rejection - `ImmutablePolicy(Import)Test`: legacy field lenient acceptance - `ReferencesValidationStrategyTest`, `PoliciesValidatorTest`, `ModifyPolicyImportStrategyTest`, `ModifyPolicyEntryReferencesStrategyTest`: reference write-time validation paths Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ca017aa to
7dd7eb2
Compare
…ing for entry references
- ModifyPolicyEntriesStrategy now validates against a Policy candidate so
PoliciesValidator runs local-reference resolution before checking the
WRITE-on-policy:/ invariant; previously the /entries endpoint rejected
payloads that the equivalent full /policies PUT accepted (BLOCKER 1)
- ModifyPolicyImportEntriesStrategy preserves transitiveImports through
PUT /imports/{id}/entries, captures the rebuilt policy for the response
ETag, and implements previous/nextEntityTag; the event applier
PolicyImportEntriesModifiedStrategy was also dropping transitiveImports
on recovery and now preserves them (BLOCKER 2)
- Add orphan-reference checks for the three sibling endpoints that
previously bypassed validation: ModifyPolicyEntryImportable rejects
flipping a referenced entry to importable=never (HIGH N1);
ModifyPolicyImportEntries rejects narrowing the labels filter when an
entry reference targets a removed label (HIGH N2); ModifyPolicyImports
applies the same label-narrow check to retained imports (HIGH N3).
Lift the shared logic into AbstractPolicyCommandStrategy.
checkLabelNarrowingDoesNotOrphan
- AbstractPolicyMongoEventAdapter overrides fromJournal to detect manifests
for removed legacy event types (importsAlias*, policyEntryAllowedImport-
AdditionsModified, policyImportEntry/EntriesAdditions*) and skip them
with a controlled INFO log instead of the parent's per-row ERROR; this
keeps recovery quiet on clusters that emitted these events under the
previous version
- DELETE /entries/{label}/allowedAdditions is now wired end-to-end. New
command DeletePolicyEntryAllowedAdditions(Response), event
PolicyEntryAllowedAdditionsDeleted, command + event strategies, route
binding in PolicyEntriesRoute, and protocol-mapping registrations.
PolicyBuilder gains removeAllowedAdditionsFor for the event applier
- RetrievePolicyEntryAllowedAdditionsStrategy returns 200 + [] for an
entry whose allowedAdditions field is absent (was 404, conflating
"entry missing" with "field unset"); nextEntityTag now emits an ETag
on the GET so conditional reads work, mirroring /references
- Strict parsing aligns transports: ModifyPolicyEntryAllowedAdditions
fromJson + setEntity, PolicyEntryAllowedAdditionsModified fromJson +
setEntity, and RetrievePolicyEntryAllowedAdditionsResponse fromJson
now route through PoliciesModelFactory.parseAllowedAdditions; non-string
array elements and unknown names are rejected with PolicyEntryInvalid-
Exception, matching the strict HTTP route
- ImmutableEntryReference.fromJson emits a ref-specific JsonParseException
for non-string import values ("Received: 42.") instead of the framework's
generic type-mismatch message; null and empty-string cases now share the
same dedicated builder
- PolicyEntry.getAllowedAdditions javadoc inverted the three-tier rule
(absent meant "no restriction", not "deny all"); corrected
- PolicyEnforcer.missingReferenceLogger gains a 5-minute dedup window per
(policyId, entry, target) tuple so a dangling reference does not flood
WARN on every enforcer cache miss; bounded by a 10000-entry size cap
- PolicyImporter.resolveReferences gains an onOwnAdditionsStripped
callback so callers can surface the second cause of silent permission
degradation (the first being missing references). Drop the dead
isEmpty?null:list ternary on getReferences (resolveReferences only
invokes the merge path when references is non-empty)
- PolicyImportsPreEnforcer: replace inline FQN imports
(java.util.stream.StreamSupport, java.util.ArrayList) with regular
imports
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tus alignment - ModifyPolicyEntryAllowedAdditionsStrategy implements previous/nextEntityTag using a shared helper on AbstractPolicyCommandStrategy. PUT now emits an ETag and conditional updates with If-Match round-trip correctly; previously both tags returned Optional.empty(), so a GET-mutate-PUT-If-Match flow always failed with 412 - RetrievePolicyEntryAllowedAdditionsStrategy reuses the same helper so the absent state (Optional.empty allowedAdditions) also emits an ETag. The three semantic tiers (absent, present-empty, present-non-empty) now all produce stable, distinct opaque tags so caches/proxies don't conflate them - DeletePolicyEntryAllowedAdditionsStrategy uses the same helper for its prev/next tags so a subsequent GET produces a matching ETag, and DELETE on an already-absent field is now a true no-op: 204 with the current ETag, no PolicyEntryAllowedAdditionsDeleted event, no revision bump. Repeated DELETEs no longer pollute the journal or produce phantom modifications on the change feed - ModifyPolicyEntryImportableStrategy raises a 409 PolicyEntryReference- ConflictException when flipping an entry to importable=never would orphan a local reference, replacing the 400 PolicyEntryInvalidException so all four orphan-check endpoints (N1 importable flip, N2 import-entries narrow, N3 imports narrow, plus DeletePolicyEntry) share the same status code and error code shape - RetrievePolicyEntryAllowedAdditionsResponse.of(JsonArray, ...) now validates strictly via PoliciesModelFactory.parseAllowedAdditions; the deserializer routes through the same factory so there is one validation site for every transport. setEntity inherits the validation by routing through of(...). Three new tests cover non-string array elements, unknown-name elements, and the setEntity path - AbstractPolicyMongoEventAdapter drops the dead performFromJournalMigration override (it logged but did not actually rewrite, so the parent's path still threw on legacy types) and the unused isLegacyDiscardedEventType helper. The manifest-based fromJournal short-circuit is the only legacy- event path; rare manifest-null rows fall through to the parent's catch-and-log path Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
hu-ahmed
left a comment
There was a problem hiding this comment.
LGTM!
Thanks for that massive work
Summary
Resolves #2423
Replace
entriesAdditionsandimportsAliaseswith a singlereferencesarray on policy entries. Each reference is either:{"import": "policyId", "entry": "label"}) — refers to an entry in an imported policy{"entry": "label"}) — refers to an entry within the same policyLocal and import references share the same merge semantics (subjects, resources, and namespaces are inherited additively from the referenced entry); only the lookup site differs.
The previously-introduced
allowedImportAdditionsfield is renamed toallowedAdditions(it now governs both local and import references) and gains explicit three-tier semantics:[]→ deny all own additionssubjects/resources/namespaces) of own additions surviveallowedAdditionsis enforced at resolution time as a runtime filter on the referencing entry's own additions. The strictest set across all references on an entry applies. Own additions that violate the filter are silently stripped at enforcement time — the write itself is not rejected. This is intentional: effective permissions depend on the live state of every referenced entry, which can change.What's new
EntryReferencemodel +referencesarray onPolicyEntrysubjectsandresourcesare now optional on aPolicyEntry(an entry can consist of references only)GET/PUT/DELETE /policies/{id}/entries/{label}/referencesGET/PUT/DELETE /policies/{id}/entries/{label}/allowedAdditionsallowedAdditions, with full Ditto Protocol adapter round-trip coveragetransitiveImportsimportable: neverskips and rejects both local and import refs uniformly (the policy author's "do-not-inherit-from-me" signal)allowedAdditionsMAX_TRANSITIVE_RESOLUTION_DEPTHfails loud (HTTP 400) instead of silent truncationimport+entrypair), and local refs toimportable: neverentries409 PolicyImportReferenceConflictException)PoliciesValidatorresolves local references before checking the WRITE-on-policy:/invariant — a policy that splits the admin subject and the WRITE grant across two mutually-referencing entries is now correctly acceptedreferencesandallowedAdditionsarrays (non-object / non-string elements rejected withPolicyEntryInvalidException){"import": null}and{"import": ""}explicitly inImmutableEntryReference.fromJsonWhat's removed
entriesAdditionson policy imports (model, signals, strategies, routes, tests)importsAliaseson policies (model, signals, strategies, routes, tests)What's preserved / backward-compatible
entriesfilter on import (whitelist of import labels)importable(implicit/explicit/never)transitiveImportsentriesAdditions/importsAliasesfields on snapshot recovery — existing on-disk policies load without an operator migration step (legacy fields are silently dropped from the response)Documentation
basic-policy.mdrewritten around entry references; adds the unifiedallowedAdditionssemantics, the WRITE-on-references≡ WRITE-on-entry note, and refreshed power-plant template + 3-level transitive hierarchy examplesditto-api-2.ymlregenerated via the documented npm buildpolicy.jsonJSON schema updatedTest plan
PolicyImporterTest— 2 / 3-level resolution, NEVER uniformity, NAMESPACES gating, multiple import refs from different policies, conflictingallowedAdditionsacross imports, emptyallowedAdditionsstrips all own additions, subject-overlap (template wins), mutual local-ref cycle, missing-reference callback, depth-limit failureImmutableEntryReferenceTest— null / emptyimportrejectionImmutablePolicy(Import)Test— legacy field lenient acceptancePoliciesValidatorTest— split-admin policy with mutual local refs is validReferencesValidationStrategyTest— local-ref-to-NEVER write-time rejection; structured-key dedupModifyPolicyImportStrategyTest— shrinking the filter while a reference depends on a removed label returns 409ModifyPolicyEntryReferencesStrategyTest— duplicate-ref / self-ref / local-existence / import-existenceCreatePolicy/ModifyPolicyrejected; declared import target accepted