Skip to content

Commit

Permalink
fix: handle multiple consequences in ODRL duty (#4085)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ndr-brt
ndr-brt authored Apr 5, 2024
1 parent 0b83737 commit f890a36
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 100 deletions.
2 changes: 1 addition & 1 deletion DEPENDENCIES
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ maven/mavencentral/org.slf4j/slf4j-api/2.0.9, MIT, approved, #5915
maven/mavencentral/org.testcontainers/database-commons/1.19.7, Apache-2.0, approved, #10345
maven/mavencentral/org.testcontainers/jdbc/1.19.7, Apache-2.0, approved, #10348
maven/mavencentral/org.testcontainers/junit-jupiter/1.19.7, MIT, approved, #10344
maven/mavencentral/org.testcontainers/kafka/1.19.7, None, restricted, #14177
maven/mavencentral/org.testcontainers/kafka/1.19.7, MIT, approved, #14177
maven/mavencentral/org.testcontainers/postgresql/1.19.7, MIT, approved, #10350
maven/mavencentral/org.testcontainers/testcontainers/1.19.7, Apache-2.0 AND MIT, approved, #10347
maven/mavencentral/org.testcontainers/vault/1.19.7, MIT, approved, #10852
Expand Down
28 changes: 15 additions & 13 deletions ...common/lib/policy-engine-lib/src/main/java/org/eclipse/edc/policy/engine/ScopeFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,7 @@ public Policy applyScope(Policy policy, String scope) {
var filteredObligations = policy.getObligations().stream().map(d -> applyScope(d, scope)).filter(Objects::nonNull).collect(toList());
var filteredPermissions = policy.getPermissions().stream().map(p -> applyScope(p, scope)).filter(Objects::nonNull).collect(toList());
var filteredProhibitions = policy.getProhibitions().stream().map(p -> applyScope(p, scope)).filter(Objects::nonNull).collect(toList());
var policyBuilder = Policy.Builder.newInstance();
policyBuilder
return Policy.Builder.newInstance()
.type(policy.getType())
.assignee(policy.getAssignee())
.assigner(policy.getAssigner())
Expand All @@ -69,8 +68,8 @@ public Policy applyScope(Policy policy, String scope) {
.duties(filteredObligations)
.permissions(filteredPermissions)
.prohibitions(filteredProhibitions)
.extensibleProperties(policy.getExtensibleProperties());
return policyBuilder.build();
.extensibleProperties(policy.getExtensibleProperties())
.build();
}

@Nullable
Expand All @@ -79,7 +78,7 @@ Permission applyScope(Permission permission, String scope) {
return null;
}
var filteredConstraints = applyScope(permission.getConstraints(), scope);
var filteredDuties = permission.getDuties().stream().map(d -> applyScope(d, scope)).filter(Objects::nonNull).collect(toList());
var filteredDuties = permission.getDuties().stream().map(d -> applyScope(d, scope)).filter(Objects::nonNull).toList();

return Permission.Builder.newInstance()
.action(permission.getAction())
Expand All @@ -93,14 +92,17 @@ Duty applyScope(Duty duty, String scope) {
if (actionNotInScope(duty, scope)) {
return null;
}
var filteredConsequence = duty.getConsequence() != null ? applyScope(duty.getConsequence(), scope) : null;
var filteredConsequences = duty.getConsequences().stream()
.map(consequence -> applyScope(consequence, scope))
.filter(Objects::nonNull)
.toList();
var filteredConstraints = applyScope(duty.getConstraints(), scope);

return Duty.Builder.newInstance()
.action(duty.getAction())
.constraints(filteredConstraints)
.parentPermission(duty.getParentPermission())
.consequence(filteredConsequence)
.consequences(filteredConsequences)
.build();
}

Expand All @@ -119,10 +121,10 @@ Prohibition applyScope(Prohibition prohibition, String scope) {

@Nullable
Constraint applyScope(Constraint rootConstraint, String scope) {
if (rootConstraint instanceof AtomicConstraint) {
return applyScope((AtomicConstraint) rootConstraint, scope);
if (rootConstraint instanceof AtomicConstraint atomicConstraint) {
return applyScope(atomicConstraint, scope);
} else if (rootConstraint instanceof MultiplicityConstraint multiplicityConstraint) {
var filteredConstraints = multiplicityConstraint.getConstraints().stream().map(c -> applyScope(c, scope)).filter(Objects::nonNull).collect(toList());
var filteredConstraints = multiplicityConstraint.getConstraints().stream().map(c -> applyScope(c, scope)).filter(Objects::nonNull).toList();
return filteredConstraints.isEmpty() ? null : multiplicityConstraint.create(filteredConstraints);
}
return rootConstraint;
Expand All @@ -133,13 +135,13 @@ private boolean actionNotInScope(Rule rule, String scope) {
}

private List<Constraint> applyScope(List<Constraint> constraints, String scope) {
return constraints.stream().map(constraint -> applyScope(constraint, scope)).filter(Objects::nonNull).collect(toList());
return constraints.stream().map(constraint -> applyScope(constraint, scope)).filter(Objects::nonNull).toList();
}

@Nullable
private Constraint applyScope(AtomicConstraint constraint, String scope) {
if (constraint.getLeftExpression() instanceof LiteralExpression) {
return registry.isInScope(((LiteralExpression) constraint.getLeftExpression()).asString(), scope) ? constraint : null;
if (constraint.getLeftExpression() instanceof LiteralExpression literalExpression) {
return registry.isInScope(literalExpression.asString(), scope) ? constraint : null;
} else {
return constraint;
}
Expand Down
4 changes: 2 additions & 2 deletions ...on/lib/policy-engine-lib/src/test/java/org/eclipse/edc/policy/engine/ScopeFilterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import static org.assertj.core.api.Assertions.assertThat;

class ScopeFilterTest {

public static final String BOUND_SCOPE = "scope1";
private static final Action REPORT_ACTION = Action.Builder.newInstance().type("report").build();
private static final Action SUB_ACTION = Action.Builder.newInstance().type("subaction").build();
Expand Down Expand Up @@ -80,7 +81,6 @@ void verifyFiltersPolicy() {
assertThat(filteredPolicy.getObligations()).isNotEmpty();
assertThat(filteredPolicy.getProhibitions()).isNotEmpty();
assertThat(filteredPolicy.getExtensibleProperties()).isNotEmpty();

}

@Test
Expand Down Expand Up @@ -163,7 +163,7 @@ void verifyFiltersDutyType() {

assertThat(filteredDuty).isNotNull();
assertThat(filteredDuty.getAction()).isNotNull();
assertThat(filteredDuty.getConsequence()).isNotNull();
assertThat(filteredDuty.getConsequences()).hasSize(1);
assertThat(filteredDuty.getConstraints().size()).isEqualTo(1); // verify that the unbound constraint was removed
assertThat(filteredDuty.getConstraints()).contains(BOUND_CONSTRAINT);
}
Expand Down
7 changes: 4 additions & 3 deletions ...lipse/edc/connector/controlplane/transform/odrl/from/JsonObjectFromPolicyTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,10 @@ public JsonObject visitProhibition(Prohibition prohibition) {
public JsonObject visitDuty(Duty duty) {
var obligationBuilder = visitRule(duty);

if (duty.getConsequence() != null) {
var consequence = visitDuty(duty.getConsequence());
obligationBuilder.add(ODRL_CONSEQUENCE_ATTRIBUTE, consequence);
var consequences = duty.getConsequences();
if (consequences != null && !consequences.isEmpty()) {
var consequencesJson = consequences.stream().map(this::visitDuty).collect(toJsonArray());
obligationBuilder.add(ODRL_CONSEQUENCE_ATTRIBUTE, consequencesJson);
}

return obligationBuilder.build();
Expand Down
2 changes: 1 addition & 1 deletion ...org/eclipse/edc/connector/controlplane/transform/odrl/to/JsonObjectToDutyTransformer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public JsonObjectToDutyTransformer() {
visitProperties(object, key -> switch (key) {
case ODRL_ACTION_ATTRIBUTE -> value -> builder.action(transformObject(value, Action.class, context));
case ODRL_CONSTRAINT_ATTRIBUTE -> value -> builder.constraints(transformArray(value, Constraint.class, context));
case ODRL_CONSEQUENCE_ATTRIBUTE -> value -> builder.consequence(transformObject(value, Duty.class, context));
case ODRL_CONSEQUENCE_ATTRIBUTE -> value -> builder.consequences(transformArray(value, Duty.class, context));
default -> doNothing();
});

Expand Down
12 changes: 8 additions & 4 deletions ...e/edc/connector/controlplane/transform/odrl/from/JsonObjectFromPolicyTransformerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,11 +273,13 @@ void transform_prohibitionWithConstraint_returnJsonObject() {
void transform_dutyWithConstraintAndConsequence_returnJsonObject() {
var constraint = getConstraint();
var action = getAction();
var consequence = Duty.Builder.newInstance().action(action).build();
var firstConsequence = Duty.Builder.newInstance().action(action).build();
var secondConsequence = Duty.Builder.newInstance().action(action).build();
var duty = Duty.Builder.newInstance()
.action(action)
.constraint(constraint)
.consequence(consequence)
.consequence(firstConsequence)
.consequence(secondConsequence)
.build();
var policy = Policy.Builder.newInstance().duty(duty).build();

Expand All @@ -294,8 +296,10 @@ void transform_dutyWithConstraintAndConsequence_returnJsonObject() {
assertThat(constraintJson.getJsonObject(ODRL_RIGHT_OPERAND_ATTRIBUTE).getJsonString(VALUE).getString())
.isEqualTo(((LiteralExpression) constraint.getRightExpression()).getValue());

var consequenceJson = dutyJson.getJsonObject(ODRL_CONSEQUENCE_ATTRIBUTE);
assertThat(consequenceJson.getJsonObject(ODRL_ACTION_ATTRIBUTE)).isNotNull();
var consequencesJson = dutyJson.getJsonArray(ODRL_CONSEQUENCE_ATTRIBUTE);
assertThat(consequencesJson).hasSize(2).map(JsonValue::asJsonObject).allSatisfy(consequenceJson -> {
assertThat(consequenceJson.getJsonObject(ODRL_ACTION_ATTRIBUTE)).isNotNull();
});

verify(context, never()).reportProblem(anyString());
}
Expand Down
57 changes: 20 additions & 37 deletions ...eclipse/edc/connector/controlplane/transform/odrl/to/JsonObjectToDutyTransformerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@

package org.eclipse.edc.connector.controlplane.transform.odrl.to;

import jakarta.json.Json;
import jakarta.json.JsonObject;
import org.eclipse.edc.connector.controlplane.transform.TestInput;
import org.eclipse.edc.policy.model.Action;
Expand All @@ -23,12 +22,10 @@
import org.eclipse.edc.policy.model.Duty;
import org.eclipse.edc.transform.spi.TransformerContext;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.Map;
import java.util.stream.Stream;
import org.junit.jupiter.api.Test;

import static jakarta.json.Json.createArrayBuilder;
import static jakarta.json.Json.createObjectBuilder;
import static org.assertj.core.api.Assertions.assertThat;
import static org.eclipse.edc.jsonld.spi.JsonLdKeywords.TYPE;
import static org.eclipse.edc.jsonld.spi.PropertyAndTypeNames.ODRL_ACTION_ATTRIBUTE;
Expand All @@ -47,7 +44,7 @@
class JsonObjectToDutyTransformerTest {
private static final String TARGET = "target";

private final TransformerContext context = mock(TransformerContext.class);
private final TransformerContext context = mock();

private final Action action = Action.Builder.newInstance().type("type").build();
private final Constraint constraint = AtomicConstraint.Builder.newInstance().build();
Expand All @@ -64,46 +61,32 @@ void setUp() {
when(context.transform(isA(JsonObject.class), eq(Duty.class))).thenReturn(consequence);
}

@ParameterizedTest
@MethodSource("jsonSource")
void transform_returnDuty(JsonObject jsonDuty) {
@Test
void transform_returnDuty() {
var actionJson = createObjectBuilder().add(TYPE, "action");
var constraintJson = createObjectBuilder().add(TYPE, "constraint");
var consequencesJson = createArrayBuilder()
.add(createObjectBuilder().add(TYPE, "consequence"))
.add(createObjectBuilder().add(TYPE, "consequence"));
var jsonDuty = createObjectBuilder()
.add(ODRL_ACTION_ATTRIBUTE, actionJson)
.add(ODRL_CONSTRAINT_ATTRIBUTE, constraintJson)
.add(ODRL_CONSEQUENCE_ATTRIBUTE, consequencesJson)
.add(ODRL_TARGET_ATTRIBUTE, TARGET)
.build();

var result = transformer.transform(TestInput.getExpanded(jsonDuty), context);

assertThat(result).isNotNull();
assertThat(result.getAction()).isEqualTo(action);
assertThat(result.getConstraints()).hasSize(1);
assertThat(result.getConstraints().get(0)).isEqualTo(constraint);
assertThat(result.getConsequence()).isEqualTo(consequence);
assertThat(result.getConsequences()).hasSize(2).first().isEqualTo(consequence);

verify(context, never()).reportProblem(anyString());
verify(context, times(1)).transform(isA(JsonObject.class), eq(Action.class));
verify(context, times(1)).transform(isA(JsonObject.class), eq(Constraint.class));
verify(context, times(1)).transform(isA(JsonObject.class), eq(Duty.class));
}

static Stream<JsonObject> jsonSource() {
var jsonFactory = Json.createBuilderFactory(Map.of());
var actionJson = jsonFactory.createObjectBuilder().add(TYPE, "action");
var constraintJson = jsonFactory.createObjectBuilder().add(TYPE, "constraint");
var consequenceJson = jsonFactory.createObjectBuilder().add(TYPE, "consequence");

return Stream.of(
// as object
jsonFactory.createObjectBuilder()
.add(ODRL_ACTION_ATTRIBUTE, actionJson)
.add(ODRL_CONSTRAINT_ATTRIBUTE, constraintJson)
.add(ODRL_CONSEQUENCE_ATTRIBUTE, consequenceJson)
.add(ODRL_TARGET_ATTRIBUTE, TARGET)
.build(),

// as arrays
jsonFactory.createObjectBuilder()
.add(ODRL_ACTION_ATTRIBUTE, jsonFactory.createArrayBuilder().add(actionJson))
.add(ODRL_CONSTRAINT_ATTRIBUTE, jsonFactory.createArrayBuilder().add(constraintJson))
.add(ODRL_CONSEQUENCE_ATTRIBUTE, jsonFactory.createArrayBuilder().add(consequenceJson))
.add(ODRL_TARGET_ATTRIBUTE, jsonFactory.createArrayBuilder().add(TARGET))
.build()
);
verify(context, times(2)).transform(isA(JsonObject.class), eq(Duty.class));
}

}
23 changes: 14 additions & 9 deletions spi/common/policy-model/src/main/java/org/eclipse/edc/policy/model/Duty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,24 +21,20 @@
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import org.jetbrains.annotations.Nullable;

import java.util.ArrayList;
import java.util.List;

import static java.util.stream.Collectors.joining;

/**
* An obligation that must be performed if all its constraints are satisfied.
* TODO: Do we need to support deserializing the parent permission setting?
*/
@JsonDeserialize(builder = Duty.Builder.class)
@JsonTypeName("dataspaceconnector:duty")
public class Duty extends Rule {

private Permission parentPermission;

@Nullable
private Duty consequence;

public Duty getConsequence() {
return consequence;
}
private final List<Duty> consequences = new ArrayList<>();

/**
* If this duty is part of a permission, returns the parent permission; otherwise returns null.
Expand All @@ -48,6 +44,10 @@ public Permission getParentPermission() {
return parentPermission;
}

public List<Duty> getConsequences() {
return consequences;
}

void setParentPermission(Permission permission) {
parentPermission = permission;
}
Expand Down Expand Up @@ -80,7 +80,12 @@ public Builder parentPermission(Permission parentPermission) {
}

public Builder consequence(Duty consequence) {
rule.consequence = consequence;
rule.consequences.add(consequence);
return this;
}

public Builder consequences(List<Duty> consequences) {
rule.consequences.addAll(consequences);
return this;
}

Expand Down
22 changes: 6 additions & 16 deletions .../org/eclipse/edc/connector/controlplane/policy/spi/PolicyDefinitionSerializationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ class PolicyDefinitionSerializationTest {

private final TypeManager typeManager = new TypeManager();


@Test
void verifySerialization() {
var policyDef = PolicyDefinition.Builder.newInstance()
Expand All @@ -42,31 +41,22 @@ void verifySerialization() {
.build();

var json = typeManager.writeValueAsString(policyDef);
assertThat(json).isNotNull();
assertThat(json).contains("createdAt");
assertThat(json).contains("sampleInheritsFrom");
assertThat(json).isNotNull().contains("createdAt").contains("sampleInheritsFrom");

var deserialized = typeManager.readValue(json, PolicyDefinition.class);
assertThat(deserialized).usingRecursiveComparison().isEqualTo(policyDef);
assertThat(deserialized.getCreatedAt()).isEqualTo(12345L);
}

private Policy createPolicy() {
var permission = Permission.Builder.newInstance().build();

var prohibition = Prohibition.Builder.newInstance().build();

var duty = Duty.Builder.newInstance().build();

var p = Policy.Builder.newInstance()
.permission(permission)
.prohibition(prohibition)
.duties(List.of(duty))
return Policy.Builder.newInstance()
.permission(Permission.Builder.newInstance().build())
.prohibition(Prohibition.Builder.newInstance().build())
.duties(List.of(Duty.Builder.newInstance().build()))
.inheritsFrom("sampleInheritsFrom")
.assigner("sampleAssigner")
.assignee("sampleAssignee")
.target("sampleTarget")
.type(PolicyType.SET);
return p.build();
.type(PolicyType.SET).build();
}
}
Loading

0 comments on commit f890a36

Please sign in to comment.