Skip to content

Commit

Permalink
feat: adds flag to enable validation on create/update policy definiti…
Browse files Browse the repository at this point in the history 
…ons (#4621)

* feat: add flag to enable validation on create/update policy definitions

* Update core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/ControlPlaneServicesExtension.java

Co-authored-by: andrea bertagnolli <andrea.bertagnolli@gmail.com>

---------

Co-authored-by: andrea bertagnolli <andrea.bertagnolli@gmail.com>
  • Loading branch information
@wolf4ood @ndr-brt
wolf4ood and ndr-brt authored Nov 13, 2024
1 parent cfc8e6f commit fdf6427
Show file tree
Hide file tree
Showing 7 changed files with 184 additions and 23 deletions.
10 changes: 5 additions & 5 deletions ...cy-engine-lib/src/main/java/org/eclipse/edc/policy/engine/validation/PolicyValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ private Result<Void> validateLeftExpression(Rule rule, String leftOperand) {
private Result<Void> validateConstraint(String leftOperand, Operator operator, Object rightOperand, Rule rule) {
var functions = getValidations(leftOperand, rule.getClass());
if (functions.isEmpty()) {
return Result.failure("left operand '%s' is not bound to any functions: Rule { %s }".formatted(leftOperand, rule));
return Result.failure("leftOperand '%s' is not bound to any functions: Rule { %s }".formatted(leftOperand, rule));
} else {
return functions.stream()
.map(f -> f.validate(leftOperand, operator, rightOperand, rule))
Expand Down Expand Up @@ -187,14 +187,14 @@ private <R extends Rule, C extends PolicyContext> List<PolicyValidation> getVali
return functions;
}

private interface PolicyValidation {
Result<Void> validate(String leftOperand, Operator operator, Object rightOperand, Rule rule);
}

private Rule currentRule() {
return ruleContext.peek();
}

private interface PolicyValidation {
Result<Void> validate(String leftOperand, Operator operator, Object rightOperand, Rule rule);
}

public static class Builder {
private final PolicyValidator validator;

Expand Down
6 changes: 3 additions & 3 deletions ...ngine-lib/src/test/java/org/eclipse/edc/policy/engine/PolicyEngineImplValidationTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ void validate_whenKeyNotBoundInTheRegistryAndToFunctions() {
// The foo key is not bound nor to function nor in the RuleBindingRegistry
assertThat(result).isFailed().messages().hasSize(2)
.anyMatch(s -> s.startsWith("leftOperand 'foo' is not bound to any scopes"))
.anyMatch(s -> s.startsWith("left operand 'foo' is not bound to any functions"));
.anyMatch(s -> s.startsWith("leftOperand 'foo' is not bound to any functions"));

}

Expand Down Expand Up @@ -143,7 +143,7 @@ void validate_shouldFail_withDynamicFunction() {

assertThat(result).isFailed()
.messages()
.anyMatch(s -> s.startsWith("left operand '%s' is not bound to any functions".formatted(leftOperand)));
.anyMatch(s -> s.startsWith("leftOperand '%s' is not bound to any functions".formatted(leftOperand)));

}

Expand Down Expand Up @@ -181,7 +181,7 @@ void validate_shouldFail_whenSkippingDynamicFunction(Policy policy, Class<Rule>

// The input key is not bound any functions , the dynamic one cannot handle the input key
assertThat(result).isFailed().messages().hasSize(1)
.anyMatch(s -> s.startsWith("left operand '%s' is not bound to any functions".formatted(key)));
.anyMatch(s -> s.startsWith("leftOperand '%s' is not bound to any functions".formatted(key)));

}

Expand Down
6 changes: 5 additions & 1 deletion ...n/java/org/eclipse/edc/connector/controlplane/services/ControlPlaneServicesExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
import org.eclipse.edc.runtime.metamodel.annotation.Extension;
import org.eclipse.edc.runtime.metamodel.annotation.Inject;
import org.eclipse.edc.runtime.metamodel.annotation.Provider;
import org.eclipse.edc.runtime.metamodel.annotation.Setting;
import org.eclipse.edc.spi.command.CommandHandlerRegistry;
import org.eclipse.edc.spi.event.EventRouter;
import org.eclipse.edc.spi.iam.IdentityService;
Expand All @@ -99,6 +100,9 @@ public class ControlPlaneServicesExtension implements ServiceExtension {

public static final String NAME = "Control Plane Services";

@Setting(key = "edc.policy.validation.enabled", description = "If true enables the policy validation when creating and updating policy definitions", defaultValue = "false")
private Boolean validatePolicy;

@Inject
private Clock clock;

Expand Down Expand Up @@ -253,7 +257,7 @@ public PolicyDefinitionService policyDefinitionService() {
var policyDefinitionObservable = new PolicyDefinitionObservableImpl();
policyDefinitionObservable.registerListener(new PolicyDefinitionEventListener(clock, eventRouter));
return new PolicyDefinitionServiceImpl(transactionContext, policyDefinitionStore, contractDefinitionStore,
policyDefinitionObservable, policyEngine, QueryValidators.policyDefinition());
policyDefinitionObservable, policyEngine, QueryValidators.policyDefinition(), validatePolicy);
}

@Provider
Expand Down
34 changes: 23 additions & 11 deletions ...pse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,19 @@ public class PolicyDefinitionServiceImpl implements PolicyDefinitionService {
private final QueryValidator queryValidator;
private final PolicyEngine policyEngine;

private final boolean validatePolicy;


public PolicyDefinitionServiceImpl(TransactionContext transactionContext, PolicyDefinitionStore policyStore,
ContractDefinitionStore contractDefinitionStore, PolicyDefinitionObservable observable,
PolicyEngine policyEngine, QueryValidator queryValidator) {
PolicyEngine policyEngine, QueryValidator queryValidator, boolean validatePolicy) {
this.transactionContext = transactionContext;
this.policyStore = policyStore;
this.contractDefinitionStore = contractDefinitionStore;
this.observable = observable;
this.policyEngine = policyEngine;
this.queryValidator = queryValidator;
this.validatePolicy = validatePolicy;
}

@Override
Expand Down Expand Up @@ -107,21 +110,23 @@ public ServiceResult<List<PolicyDefinition>> search(QuerySpec query) {

@Override
public @NotNull ServiceResult<PolicyDefinition> create(PolicyDefinition policyDefinition) {
return transactionContext.execute(() -> {
var saveResult = policyStore.create(policyDefinition);
saveResult.onSuccess(v -> observable.invokeForEach(l -> l.created(policyDefinition)));
return ServiceResult.from(saveResult);
});
return transactionContext.execute(() -> validatePolicyDefinition(policyDefinition)
.compose(s -> {
var saveResult = policyStore.create(policyDefinition);
saveResult.onSuccess(v -> observable.invokeForEach(l -> l.created(policyDefinition)));
return ServiceResult.from(saveResult);
}));
}


@Override
public ServiceResult<PolicyDefinition> update(PolicyDefinition policyDefinition) {
return transactionContext.execute(() -> {
var updateResult = policyStore.update(policyDefinition);
updateResult.onSuccess(p -> observable.invokeForEach(l -> l.updated(p)));
return ServiceResult.from(updateResult);
});
return transactionContext.execute(() -> validatePolicyDefinition(policyDefinition)
.compose(s -> {
var updateResult = policyStore.update(policyDefinition);
updateResult.onSuccess(p -> observable.invokeForEach(l -> l.updated(p)));
return ServiceResult.from(updateResult);
}));
}

@Override
Expand All @@ -138,6 +143,13 @@ public ServiceResult<PolicyEvaluationPlan> createEvaluationPlan(String scope, Po
return ServiceResult.success(policyEngine.createEvaluationPlan(scope, policy));
}

private ServiceResult<Void> validatePolicyDefinition(PolicyDefinition policyDefinition) {
if (validatePolicy) {
return validate(policyDefinition.getPolicy());
}
return ServiceResult.success();
}

private List<PolicyDefinition> queryPolicyDefinitions(QuerySpec query) {
return transactionContext.execute(() -> {
try (var stream = policyStore.findAll(query)) {
Expand Down
33 changes: 32 additions & 1 deletion ...edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ class PolicyDefinitionServiceImplTest {
private final PolicyDefinitionObservable observable = mock(PolicyDefinitionObservable.class);
private final PolicyEngine policyEngine = mock();
private final QueryValidator queryValidator = mock();
private final PolicyDefinitionServiceImpl policyServiceImpl = new PolicyDefinitionServiceImpl(dummyTransactionContext, policyStore, contractDefinitionStore, observable, policyEngine, queryValidator);
private final PolicyDefinitionServiceImpl policyServiceImpl = new PolicyDefinitionServiceImpl(dummyTransactionContext, policyStore, contractDefinitionStore, observable, policyEngine, queryValidator, false);

@Test
void findById_shouldRelyOnPolicyStore() {
Expand Down Expand Up @@ -113,6 +113,37 @@ void createPolicy_shouldNotCreatePolicyIfItAlreadyExists() {
verifyNoMoreInteractions(policyStore);
}

@Test
void createPolicy_shouldCreatePolicyIfValidationSucceed() {

var policyServiceImpl = new PolicyDefinitionServiceImpl(dummyTransactionContext, policyStore, contractDefinitionStore, observable, policyEngine, queryValidator, true);

var policy = createPolicy("policyId");
when(policyStore.create(policy)).thenReturn(StoreResult.success(policy));
when(policyEngine.validate(policy.getPolicy())).thenReturn(Result.success());

var inserted = policyServiceImpl.create(policy);

assertThat(inserted).isSucceeded();
verify(policyStore).create(policy);
verifyNoMoreInteractions(policyStore);
}

@Test
void createPolicy_shouldNotCreatePolicyIfValidationFails() {

var policyServiceImpl = new PolicyDefinitionServiceImpl(dummyTransactionContext, policyStore, contractDefinitionStore, observable, policyEngine, queryValidator, true);

var policy = createPolicy("policyId");
when(policyStore.create(policy)).thenReturn(StoreResult.success(policy));
when(policyEngine.validate(policy.getPolicy())).thenReturn(Result.failure("validation failure"));

var inserted = policyServiceImpl.create(policy);

assertThat(inserted).isFailed().messages().contains("validation failure");
verifyNoMoreInteractions(policyStore);
}

@Test
void delete_shouldDeletePolicyIfItsNotReferencedByAnyContractDefinition() {
when(contractDefinitionStore.findAll(any())).thenReturn(Stream.empty(), Stream.empty());
Expand Down
14 changes: 12 additions & 2 deletions ...ner/src/test/java/org/eclipse/edc/test/e2e/managementapi/ManagementEndToEndExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.junit.jupiter.api.extension.ParameterResolutionException;

import java.util.HashMap;
import java.util.Map;

import static org.eclipse.edc.sql.testfixtures.PostgresqlEndToEndInstance.createDatabase;
import static org.eclipse.edc.util.io.Ports.getFreePort;
Expand Down Expand Up @@ -56,10 +57,18 @@ public Object resolveParameter(ParameterContext parameterContext, ExtensionConte
static class InMemory extends ManagementEndToEndExtension {

protected InMemory() {
super(context());
super(context(Map.of()));
}

private static ManagementEndToEndTestContext context() {
protected InMemory(Map<String, String> config) {
super(context(config));
}

public static InMemory withConfig(Map<String, String> config) {
return new InMemory(config);
}

private static ManagementEndToEndTestContext context(Map<String, String> config) {
var managementPort = getFreePort();
var protocolPort = getFreePort();

Expand All @@ -75,6 +84,7 @@ private static ManagementEndToEndTestContext context() {
put("edc.dsp.callback.address", "http://localhost:" + protocolPort + "/protocol");
put("web.http.management.path", "/management");
put("web.http.management.port", String.valueOf(managementPort));
putAll(config);
}
},
":system-tests:management-api:management-api-test-runtime"
Expand Down
104 changes: 104 additions & 0 deletions ...src/test/java/org/eclipse/edc/test/e2e/managementapi/PolicyDefinitionApiEndToEndTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.extension.RegisterExtension;

import java.util.HashMap;
import java.util.Map;
Expand All @@ -45,6 +46,7 @@
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.startsWith;

public class PolicyDefinitionApiEndToEndTest {

Expand Down Expand Up @@ -402,4 +404,106 @@ class InMemory extends Tests {
class Postgres extends Tests {
}


@Nested
class ValidationTests {

@RegisterExtension
static ManagementEndToEndExtension.InMemory runtime = ManagementEndToEndExtension.InMemory.withConfig(Map.of(
"edc.policy.validation.enabled", "true"
));

@Test
void shouldNotStorePolicyDefinition_whenValidationFails(ManagementEndToEndTestContext context) {
var requestBody = createObjectBuilder()
.add(CONTEXT, createObjectBuilder()
.add(VOCAB, EDC_NAMESPACE)
.build())
.add(TYPE, "PolicyDefinition")
.add("policy", sampleOdrlPolicy())
.build();

context.baseRequest()
.body(requestBody)
.contentType(JSON)
.post("/v3/policydefinitions")
.then()
.log().ifError()
.statusCode(400)
.contentType(JSON)
.body("size()", is(2))
.body("[0].message", startsWith("leftOperand 'https://w3id.org/edc/v0.0.1/ns/left' is not bound to any scopes"))
.body("[1].message", startsWith("leftOperand 'https://w3id.org/edc/v0.0.1/ns/left' is not bound to any functions"));
}

@Test
void shouldNotUpdatePolicyDefinition_whenValidationFails(ManagementEndToEndTestContext context) {
var validRequestBody = createObjectBuilder()
.add(CONTEXT, createObjectBuilder()
.add(VOCAB, EDC_NAMESPACE)
.build())
.add(TYPE, "PolicyDefinition")
.add("policy", emptyOdrlPolicy())
.build();

var id = context.baseRequest()
.body(validRequestBody)
.contentType(JSON)
.post("/v3/policydefinitions")
.then()
.contentType(JSON)
.extract().jsonPath().getString(ID);

context.baseRequest()
.contentType(JSON)
.get("/v3/policydefinitions/" + id)
.then()
.statusCode(200);

var inValidRequestBody = createObjectBuilder()
.add(CONTEXT, createObjectBuilder()
.add(VOCAB, EDC_NAMESPACE)
.build())
.add(TYPE, "PolicyDefinition")
.add("policy", sampleOdrlPolicy())
.build();

context.baseRequest()
.contentType(JSON)
.body(inValidRequestBody)
.put("/v3/policydefinitions/" + id)
.then()
.statusCode(400)
.contentType(JSON)
.body("size()", is(2))
.body("[0].message", startsWith("leftOperand 'https://w3id.org/edc/v0.0.1/ns/left' is not bound to any scopes"))
.body("[1].message", startsWith("leftOperand 'https://w3id.org/edc/v0.0.1/ns/left' is not bound to any functions"));

}

private JsonObject emptyOdrlPolicy() {
return createObjectBuilder()
.add(CONTEXT, "http://www.w3.org/ns/odrl.jsonld")
.add(TYPE, "Set")
.build();
}

private JsonObject sampleOdrlPolicy() {
return createObjectBuilder()
.add(CONTEXT, "http://www.w3.org/ns/odrl.jsonld")
.add(TYPE, "Set")
.add("permission", createArrayBuilder()
.add(createObjectBuilder()
.add("action", "use")
.add("constraint", createObjectBuilder()
.add("leftOperand", "left")
.add("operator", "eq")
.add("rightOperand", "value"))
.build())
.build())
.build();
}

}

}

0 comments on commit fdf6427

Please sign in to comment.