Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle multiple consequences in ODRL duty #4085

Merged
merged 1 commit into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DEPENDENCIES
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
maven/mavencentral/com.apicatalog/carbon-did/0.0.2, Apache-2.0, approved, #9239

Check warning on line 1 in DEPENDENCIES

View workflow job for this annotation

GitHub Actions / check / Dash-Verify-Licenses

Restricted Dependencies found

Some dependencies are marked 'restricted' - please review them
maven/mavencentral/com.apicatalog/iron-ed25519-cryptosuite-2020/0.8.1, Apache-2.0, approved, #11157
maven/mavencentral/com.apicatalog/iron-verifiable-credentials/0.8.1, Apache-2.0, approved, #9234
maven/mavencentral/com.apicatalog/titanium-json-ld/1.0.0, Apache-2.0, approved, clearlydefined
Expand Down Expand Up @@ -348,7 +348,7 @@
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
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
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
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
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
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
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));
}

}
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
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
Loading