diff --git a/core/common/lib/policy-engine-lib/src/main/java/org/eclipse/edc/policy/engine/validation/PolicyValidator.java b/core/common/lib/policy-engine-lib/src/main/java/org/eclipse/edc/policy/engine/validation/PolicyValidator.java index 669bec470da..9cb46eb84ee 100644 --- a/core/common/lib/policy-engine-lib/src/main/java/org/eclipse/edc/policy/engine/validation/PolicyValidator.java +++ b/core/common/lib/policy-engine-lib/src/main/java/org/eclipse/edc/policy/engine/validation/PolicyValidator.java @@ -136,7 +136,7 @@ private Result validateLeftExpression(Rule rule, String leftOperand) { private Result 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)) @@ -187,14 +187,14 @@ private List getVali return functions; } - private interface PolicyValidation { - Result validate(String leftOperand, Operator operator, Object rightOperand, Rule rule); - } - private Rule currentRule() { return ruleContext.peek(); } + private interface PolicyValidation { + Result validate(String leftOperand, Operator operator, Object rightOperand, Rule rule); + } + public static class Builder { private final PolicyValidator validator; diff --git a/core/common/lib/policy-engine-lib/src/test/java/org/eclipse/edc/policy/engine/PolicyEngineImplValidationTest.java b/core/common/lib/policy-engine-lib/src/test/java/org/eclipse/edc/policy/engine/PolicyEngineImplValidationTest.java index 1314967a59e..614cc0fcfd2 100644 --- a/core/common/lib/policy-engine-lib/src/test/java/org/eclipse/edc/policy/engine/PolicyEngineImplValidationTest.java +++ b/core/common/lib/policy-engine-lib/src/test/java/org/eclipse/edc/policy/engine/PolicyEngineImplValidationTest.java @@ -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")); } @@ -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))); } @@ -181,7 +181,7 @@ void validate_shouldFail_whenSkippingDynamicFunction(Policy policy, Class // 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))); } diff --git a/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/ControlPlaneServicesExtension.java b/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/ControlPlaneServicesExtension.java index d466c3a1764..a0360d9aef8 100644 --- a/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/ControlPlaneServicesExtension.java +++ b/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/ControlPlaneServicesExtension.java @@ -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; @@ -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; @@ -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 diff --git a/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImpl.java b/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImpl.java index c06d406895f..b79e4f8b15e 100644 --- a/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImpl.java +++ b/core/control-plane/control-plane-aggregate-services/src/main/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImpl.java @@ -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 @@ -107,21 +110,23 @@ public ServiceResult> search(QuerySpec query) { @Override public @NotNull ServiceResult 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 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 @@ -138,6 +143,13 @@ public ServiceResult createEvaluationPlan(String scope, Po return ServiceResult.success(policyEngine.createEvaluationPlan(scope, policy)); } + private ServiceResult validatePolicyDefinition(PolicyDefinition policyDefinition) { + if (validatePolicy) { + return validate(policyDefinition.getPolicy()); + } + return ServiceResult.success(); + } + private List queryPolicyDefinitions(QuerySpec query) { return transactionContext.execute(() -> { try (var stream = policyStore.findAll(query)) { diff --git a/core/control-plane/control-plane-aggregate-services/src/test/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImplTest.java b/core/control-plane/control-plane-aggregate-services/src/test/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImplTest.java index 24715af2d05..faf67f51db8 100644 --- a/core/control-plane/control-plane-aggregate-services/src/test/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImplTest.java +++ b/core/control-plane/control-plane-aggregate-services/src/test/java/org/eclipse/edc/connector/controlplane/services/policydefinition/PolicyDefinitionServiceImplTest.java @@ -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() { @@ -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()); diff --git a/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/ManagementEndToEndExtension.java b/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/ManagementEndToEndExtension.java index 10c7789aa19..fb6ef9e2cac 100644 --- a/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/ManagementEndToEndExtension.java +++ b/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/ManagementEndToEndExtension.java @@ -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; @@ -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 config) { + super(context(config)); + } + + public static InMemory withConfig(Map config) { + return new InMemory(config); + } + + private static ManagementEndToEndTestContext context(Map config) { var managementPort = getFreePort(); var protocolPort = getFreePort(); @@ -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" diff --git a/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/PolicyDefinitionApiEndToEndTest.java b/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/PolicyDefinitionApiEndToEndTest.java index eb746db8332..6859cbe1659 100644 --- a/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/PolicyDefinitionApiEndToEndTest.java +++ b/system-tests/management-api/management-api-test-runner/src/test/java/org/eclipse/edc/test/e2e/managementapi/PolicyDefinitionApiEndToEndTest.java @@ -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; @@ -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 { @@ -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(); + } + + } + }