Skip to content

Commit ce0ecd0

Browse files
thjaeckleclaude
andcommitted
Fix 3-level reference resolution and add comprehensive tests
Three interconnected fixes for multi-level import reference resolution: 1. Resolve imported policy's references before importing: when importing a policy whose entries use references (e.g. intermediate's driver references template's driver for resources), resolveReferences is called BEFORE entries are imported and references stripped. This materializes the inherited values. 2. Apply import prefix to transitive entries: transitive entries now get their import prefix so they don't collide with the loaded policy's own entries (e.g. both having "driver"). This allows resolveReferences to find transitive entries by their prefixed labels. 3. Merge subjects for import references: import references now merge subjects additively (same as local references). Without this, the leaf in a 3-level hierarchy could never inherit subjects from the intermediate level. New tests: - testThreeLevelReferenceResolution: full 3-level hierarchy (template → intermediate → leaf) verifying resources and subjects propagate - testLocalRefOnlyPolicyResolvesViaWithResolvedImports: verifies the fix from the previous commit (no-imports path) - testImportedPolicyReferencesAreResolvedBeforeImport: 2-level scenario verifying resources inherited via references survive import Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 02aa727 commit ce0ecd0

2 files changed

Lines changed: 235 additions & 20 deletions

File tree

policies/model/src/main/java/org/eclipse/ditto/policies/model/PolicyImporter.java

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,16 @@ private static CompletionStage<Set<PolicyEntry>> resolveImport(
140140
resolvedEntriesCs = CompletableFuture.completedFuture(loadedPolicy.getEntriesSet());
141141
}
142142
return resolvedEntriesCs.thenApply(resolvedEntries -> {
143+
// Resolve the loaded policy's own entry references before importing.
144+
// This ensures entries that inherit resources/subjects via references
145+
// have those values materialized before the references are stripped
146+
// during label rewriting.
147+
final Set<PolicyEntry> withResolvedRefs =
148+
resolveReferences(loadedPolicy, resolvedEntries);
143149
final ImportedLabels importedLabels = policyImport.getEffectedImports()
144150
.map(EffectedImports::getImportedLabels)
145151
.orElse(ImportedLabels.none());
146-
return rewriteImportedLabels(importedPolicyId, resolvedEntries,
152+
return rewriteImportedLabels(importedPolicyId, withResolvedRefs,
147153
importedLabels, applyImportPrefix);
148154
});
149155
}).orElse(CompletableFuture.completedFuture(Collections.emptySet())));
@@ -202,11 +208,12 @@ private static CompletionStage<Set<PolicyEntry>> resolveTransitiveImports(
202208
newVisited.addAll(transitiveIdSet);
203209
final Set<PolicyId> unmodifiableVisited = Collections.unmodifiableSet(newVisited);
204210

205-
// Resolve filtered imports without label prefixing — the entries are merged into the
206-
// loaded policy's entries as if they were its own inline entries. The outer resolution
207-
// applies the prefix.
211+
// Resolve filtered imports WITH label prefixing so transitive entries don't collide
212+
// with the loaded policy's own entries (e.g. both having a "driver" entry). The prefix
213+
// allows resolveReferences to find the transitive entries by their import-prefixed labels.
214+
// The outer resolution applies its own prefix on top.
208215
return mergeImportedPolicyEntries(loadedPolicy.getEntriesSet(), filteredImportsList,
209-
policyLoader, depth + 1, false, unmodifiableVisited);
216+
policyLoader, depth + 1, true, unmodifiableVisited);
210217
}
211218

212219
private static Set<PolicyEntry> rewriteImportedLabels(final PolicyId importedPolicyId,
@@ -356,13 +363,8 @@ private static PolicyEntry resolveOneReference(final Policy importingPolicy,
356363
referencedEntry.getNamespaces().orElse(null),
357364
ownEntry.getNamespaces().orElse(Collections.emptyList()));
358365

359-
// For local references, also merge subjects additively
360-
final Subjects mergedSubjects;
361-
if (ref.isLocalReference()) {
362-
mergedSubjects = mergeSubjects(referencedEntry.getSubjects(), ownEntry.getSubjects());
363-
} else {
364-
mergedSubjects = ownEntry.getSubjects();
365-
}
366+
// Merge subjects additively for both local and import references
367+
final Subjects mergedSubjects = mergeSubjects(referencedEntry.getSubjects(), ownEntry.getSubjects());
366368

367369
// Narrow allowedImportAdditions for import references
368370
final Set<AllowedImportAddition> narrowedAllowed = ref.isImportReference()

policies/model/src/test/java/org/eclipse/ditto/policies/model/PolicyImporterTest.java

Lines changed: 221 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -337,9 +337,10 @@ public void transitiveResolutionDoesNotAffectOtherImports() {
337337
final Label importedDLabel = PoliciesModelFactory.newImportedLabel(policyIdD, dLabel);
338338
assertThat(entries.stream().anyMatch(e -> e.getLabel().equals(importedDLabel))).isTrue();
339339

340-
// B's transitive entries from C should also be resolved (single prefix — no double-prefix)
341-
final Label importedRoleLabel = PoliciesModelFactory.newImportedLabel(policyIdB, roleLabel);
342-
assertThat(entries.stream().anyMatch(e -> e.getLabel().equals(importedRoleLabel))).isTrue();
340+
// B's transitive entries from C get double-prefixed: C-prefix inside B-prefix
341+
final Label cPrefixedRole = PoliciesModelFactory.newImportedLabel(policyIdC, roleLabel);
342+
final Label bPrefixedCRole = PoliciesModelFactory.newImportedLabel(policyIdB, cPrefixedRole);
343+
assertThat(entries.stream().anyMatch(e -> e.getLabel().equals(bPrefixedCRole))).isTrue();
343344

344345
// A's own entry should be preserved
345346
assertThat(entries.stream().anyMatch(e -> e.getLabel().equals(END_USER_LABEL))).isTrue();
@@ -447,9 +448,11 @@ public void transitiveResolutionWorksWithExplicitImportableType() {
447448
PoliciesModelFactory.newPolicyImport(policyIdC, bEffected))),
448449
Collections.emptyList());
449450

450-
// A imports B with entries=["ROLE"] and transitiveImports=["C"]
451+
// A imports B with transitiveImports=["C"]. The entries filter includes the
452+
// prefixed label since transitive entries carry import prefixes.
453+
final Label cPrefixedRole = PoliciesModelFactory.newImportedLabel(policyIdC, roleLabel);
451454
final EffectedImports aEffected = PoliciesModelFactory.newEffectedImportedLabels(
452-
Collections.singletonList(roleLabel), Collections.singletonList(policyIdC));
455+
Collections.singletonList(cPrefixedRole), Collections.singletonList(policyIdC));
453456
final Policy policyA = PoliciesModelFactory.newPolicyBuilder(POLICY_ID)
454457
.setPolicyImport(PoliciesModelFactory.newPolicyImport(policyIdB, aEffected))
455458
.build();
@@ -466,10 +469,11 @@ public void transitiveResolutionWorksWithExplicitImportableType() {
466469
final Set<PolicyEntry> entries =
467470
PolicyImporter.mergeImportedPolicyEntries(policyA, loader).toCompletableFuture().join();
468471

469-
// The EXPLICIT "ROLE" entry from C should be imported via B with a single prefix
470-
final Label importedRoleLabel = PoliciesModelFactory.newImportedLabel(policyIdB, roleLabel);
472+
// The EXPLICIT "ROLE" entry from C gets double-prefixed: C-prefix inside B-prefix
473+
final Label expectedLabel = PoliciesModelFactory.newImportedLabel(policyIdB,
474+
PoliciesModelFactory.newImportedLabel(policyIdC, roleLabel));
471475
final PolicyEntry mergedEntry = entries.stream()
472-
.filter(e -> e.getLabel().equals(importedRoleLabel))
476+
.filter(e -> e.getLabel().equals(expectedLabel))
473477
.findFirst()
474478
.orElseThrow(() -> new AssertionError(
475479
"Expected EXPLICIT ROLE entry not found. Available labels: " +
@@ -989,6 +993,215 @@ private static Policy createImportedPolicyWithAdditions(final PolicyId importedP
989993
null, emptyPolicyImports(), policyEntries);
990994
}
991995

996+
/**
997+
* 3-level hierarchy: template → intermediate → leaf.
998+
* Template defines resources on "driver" entry.
999+
* Intermediate imports template and has a "driver" entry with references to template's driver + own subjects.
1000+
* Leaf imports intermediate with transitiveImports to template and references intermediate's driver.
1001+
*
1002+
* Verifies that the leaf's resolved driver entry has:
1003+
* - resources from the template (inherited via intermediate's reference, materialized during import)
1004+
* - subjects from the intermediate (alice, bob)
1005+
* - subjects from the leaf (charlie)
1006+
*/
1007+
@Test
1008+
public void testThreeLevelReferenceResolution() {
1009+
final PolicyId templateId = PolicyId.of("com.example", "template");
1010+
final PolicyId intermediateId = PolicyId.of("com.example", "intermediate");
1011+
final PolicyId leafId = PolicyId.of("com.example", "leaf");
1012+
1013+
final ResourceKey locationResource = ResourceKey.newInstance("thing", JsonPointer.of("features/location"));
1014+
final ResourceKey fuelResource = ResourceKey.newInstance("thing", JsonPointer.of("features/fuel"));
1015+
1016+
// Template: driver entry with resources, no subjects
1017+
final PolicyEntry templateDriver = PoliciesModelFactory.newPolicyEntry(Label.of("driver"),
1018+
PoliciesModelFactory.emptySubjects(),
1019+
Resources.newInstance(
1020+
Resource.newInstance("thing", JsonPointer.of("features/location"),
1021+
EffectedPermissions.newInstance(
1022+
Permissions.newInstance("READ"), Permissions.none())),
1023+
Resource.newInstance("thing", JsonPointer.of("features/fuel"),
1024+
EffectedPermissions.newInstance(
1025+
Permissions.newInstance("READ"), Permissions.none()))),
1026+
null, ImportableType.IMPLICIT,
1027+
Collections.singleton(AllowedImportAddition.SUBJECTS), null);
1028+
1029+
final Policy templatePolicy = PoliciesModelFactory.newPolicyBuilder(templateId)
1030+
.set(templateDriver)
1031+
.build();
1032+
1033+
// Intermediate: imports template, driver entry with subjects + reference to template driver
1034+
final PolicyEntry intermediateDriver = PoliciesModelFactory.newPolicyEntry(Label.of("driver"),
1035+
Subjects.newInstance(
1036+
Subject.newInstance(SubjectIssuer.GOOGLE, "alice"),
1037+
Subject.newInstance(SubjectIssuer.GOOGLE, "bob")),
1038+
PoliciesModelFactory.emptyResources(),
1039+
null, ImportableType.IMPLICIT,
1040+
Collections.singleton(AllowedImportAddition.SUBJECTS),
1041+
Collections.singletonList(
1042+
PoliciesModelFactory.newEntryReference(templateId, Label.of("driver"))));
1043+
1044+
final Policy intermediatePolicy = PoliciesModelFactory.newPolicyBuilder(intermediateId)
1045+
.set(intermediateDriver)
1046+
.setPolicyImport(PoliciesModelFactory.newPolicyImport(templateId, (EffectedImports) null))
1047+
.build();
1048+
1049+
// Leaf: imports intermediate with transitiveImports to template
1050+
final PolicyEntry leafDriver = PoliciesModelFactory.newPolicyEntry(Label.of("driver"),
1051+
Subjects.newInstance(Subject.newInstance(SubjectIssuer.GOOGLE, "charlie")),
1052+
PoliciesModelFactory.emptyResources(),
1053+
null, ImportableType.IMPLICIT, null,
1054+
Collections.singletonList(
1055+
PoliciesModelFactory.newEntryReference(intermediateId, Label.of("driver"))));
1056+
1057+
final EffectedImports intermediateImport = PoliciesModelFactory.newEffectedImportedLabels(
1058+
Collections.emptyList(), Collections.singletonList(templateId));
1059+
1060+
final Policy leafPolicy = PoliciesModelFactory.newPolicyBuilder(leafId)
1061+
.set(leafDriver)
1062+
.setPolicyImport(PoliciesModelFactory.newPolicyImport(intermediateId, intermediateImport))
1063+
.build();
1064+
1065+
// Policy loader knows all three
1066+
final Function<PolicyId, CompletionStage<Optional<Policy>>> loader = id -> {
1067+
if (templateId.equals(id)) {
1068+
return CompletableFuture.completedFuture(Optional.of(templatePolicy));
1069+
} else if (intermediateId.equals(id)) {
1070+
return CompletableFuture.completedFuture(Optional.of(intermediatePolicy));
1071+
}
1072+
return CompletableFuture.completedFuture(Optional.empty());
1073+
};
1074+
1075+
// Resolve via withResolvedImports (the full pipeline)
1076+
final Policy resolved = leafPolicy.withResolvedImports(loader).toCompletableFuture().join();
1077+
1078+
// Find the leaf's resolved driver entry
1079+
final PolicyEntry resolvedDriver = resolved.getEntryFor(Label.of("driver"))
1080+
.orElseThrow(() -> new AssertionError("driver entry not found in resolved policy"));
1081+
1082+
// Verify resources from template are present (inherited via intermediate's reference)
1083+
assertThat(resolvedDriver.getResources().getResource(locationResource)).isPresent();
1084+
assertThat(resolvedDriver.getResources().getResource(fuelResource)).isPresent();
1085+
1086+
// Import references merge subjects additively — leaf's driver inherits alice+bob
1087+
// from the intermediate's resolved driver, plus its own charlie.
1088+
final Set<String> subjectIds = StreamSupport.stream(resolvedDriver.getSubjects().spliterator(), false)
1089+
.map(s -> s.getId().toString())
1090+
.collect(java.util.stream.Collectors.toSet());
1091+
assertThat(subjectIds).contains("google:alice", "google:bob", "google:charlie");
1092+
}
1093+
1094+
/**
1095+
* Verifies that a policy using only local references (no imports) correctly resolves
1096+
* references via withResolvedImports. This was a bug where the else-branch skipped
1097+
* resolveReferences entirely when imports were empty.
1098+
*/
1099+
@Test
1100+
public void testLocalRefOnlyPolicyResolvesViaWithResolvedImports() {
1101+
final Label sharedLabel = Label.of("operators");
1102+
final Label consumerLabel = Label.of("reactor-op");
1103+
1104+
final SubjectId alice = SubjectId.newInstance(SubjectIssuer.GOOGLE, "alice");
1105+
final ResourceKey reactorResource = ResourceKey.newInstance("thing", JsonPointer.of("features/reactor"));
1106+
1107+
final PolicyEntry sharedEntry = ImmutablePolicyEntry.of(sharedLabel,
1108+
Subjects.newInstance(Subject.newInstance(alice)),
1109+
PoliciesModelFactory.emptyResources(),
1110+
ImportableType.IMPLICIT);
1111+
1112+
final PolicyEntry consumerEntry = PoliciesModelFactory.newPolicyEntry(consumerLabel,
1113+
PoliciesModelFactory.emptySubjects(),
1114+
Resources.newInstance(Resource.newInstance("thing", JsonPointer.of("features/reactor"),
1115+
EffectedPermissions.newInstance(
1116+
Permissions.newInstance("READ", "WRITE"), Permissions.none()))),
1117+
null, ImportableType.IMPLICIT, null,
1118+
Collections.singletonList(PoliciesModelFactory.newLocalEntryReference(sharedLabel)));
1119+
1120+
// No imports — only local references
1121+
final Policy policy = PoliciesModelFactory.newPolicyBuilder(POLICY_ID)
1122+
.set(sharedEntry)
1123+
.set(consumerEntry)
1124+
.build();
1125+
1126+
// Use withResolvedImports (with a no-op loader since there are no imports)
1127+
final Function<PolicyId, CompletionStage<Optional<Policy>>> noOpLoader =
1128+
id -> CompletableFuture.completedFuture(Optional.empty());
1129+
1130+
final Policy resolved = policy.withResolvedImports(noOpLoader).toCompletableFuture().join();
1131+
1132+
final PolicyEntry resolvedConsumer = resolved.getEntryFor(consumerLabel)
1133+
.orElseThrow(() -> new AssertionError("consumer entry not found"));
1134+
1135+
// Verify alice from shared entry is merged into consumer's subjects
1136+
final Set<String> subjectIds = StreamSupport.stream(resolvedConsumer.getSubjects().spliterator(), false)
1137+
.map(s -> s.getId().toString())
1138+
.collect(java.util.stream.Collectors.toSet());
1139+
assertThat(subjectIds).contains(alice.toString());
1140+
1141+
// Verify consumer's own resources are still there
1142+
assertThat(resolvedConsumer.getResources().getResource(reactorResource)).isPresent();
1143+
}
1144+
1145+
/**
1146+
* Verifies that intermediate policy's references are resolved before its entries are
1147+
* imported. Without this, resources inherited via references at the intermediate level
1148+
* would be lost during import (since rewriteLabel strips references).
1149+
*/
1150+
@Test
1151+
public void testImportedPolicyReferencesAreResolvedBeforeImport() {
1152+
final PolicyId templateId = PolicyId.of("com.example", "tmpl");
1153+
final PolicyId importingId = PolicyId.of("com.example", "importer");
1154+
1155+
final ResourceKey attrResource = ResourceKey.newInstance("thing", JsonPointer.of("attributes"));
1156+
1157+
// Template: entry with resources
1158+
final PolicyEntry templateEntry = PoliciesModelFactory.newPolicyEntry(Label.of("role"),
1159+
PoliciesModelFactory.emptySubjects(),
1160+
Resources.newInstance(Resource.newInstance("thing", JsonPointer.of("attributes"),
1161+
EffectedPermissions.newInstance(
1162+
Permissions.newInstance("READ"), Permissions.none()))),
1163+
null, ImportableType.IMPLICIT,
1164+
Collections.singleton(AllowedImportAddition.SUBJECTS), null);
1165+
1166+
final Policy templatePolicy = PoliciesModelFactory.newPolicyBuilder(templateId)
1167+
.set(templateEntry)
1168+
.build();
1169+
1170+
// Importing policy: imports template, has entry referencing template's role
1171+
final PolicyEntry importingEntry = PoliciesModelFactory.newPolicyEntry(Label.of("role"),
1172+
Subjects.newInstance(Subject.newInstance(SubjectIssuer.GOOGLE, "user")),
1173+
PoliciesModelFactory.emptyResources(),
1174+
null, ImportableType.IMPLICIT, null,
1175+
Collections.singletonList(
1176+
PoliciesModelFactory.newEntryReference(templateId, Label.of("role"))));
1177+
1178+
final Policy importingPolicy = PoliciesModelFactory.newPolicyBuilder(importingId)
1179+
.set(importingEntry)
1180+
.setPolicyImport(PoliciesModelFactory.newPolicyImport(templateId, (EffectedImports) null))
1181+
.build();
1182+
1183+
final Function<PolicyId, CompletionStage<Optional<Policy>>> loader = id -> {
1184+
if (templateId.equals(id)) {
1185+
return CompletableFuture.completedFuture(Optional.of(templatePolicy));
1186+
}
1187+
return CompletableFuture.completedFuture(Optional.empty());
1188+
};
1189+
1190+
final Policy resolved = importingPolicy.withResolvedImports(loader).toCompletableFuture().join();
1191+
1192+
final PolicyEntry resolvedRole = resolved.getEntryFor(Label.of("role"))
1193+
.orElseThrow(() -> new AssertionError("role entry not found"));
1194+
1195+
// Resources from template must be inherited via resolved reference
1196+
assertThat(resolvedRole.getResources().getResource(attrResource)).isPresent();
1197+
1198+
// Subject from importing policy's own entry
1199+
final Set<String> subjectIds = StreamSupport.stream(resolvedRole.getSubjects().spliterator(), false)
1200+
.map(s -> s.getId().toString())
1201+
.collect(java.util.stream.Collectors.toSet());
1202+
assertThat(subjectIds).contains("google:user");
1203+
}
1204+
9921205
private static Policy createPolicy() {
9931206
final List<PolicyEntry> policyEntries = Collections.singletonList(KNOWN_POLICY_ENTRY_OWN);
9941207
return ImmutablePolicy.of(POLICY_ID, PolicyLifecycle.ACTIVE, PolicyRevision.newInstance(1), null, null, null,

0 commit comments

Comments
 (0)