From ef8189a33dc0478ae48b31273369e14ad1b2cf45 Mon Sep 17 00:00:00 2001 From: bmg13 <18561736+bmg13@users.noreply.github.com> Date: Tue, 26 Nov 2024 07:34:05 +0000 Subject: [PATCH] Fix BPNL Group validation operators logic (#1673) * Fix BPNL Group Validation Operator's Logic and update doc and tests. * Update method doc. * Fix failing test. * Change EQ and NEQ to deprecated. * Update EQ and NEQ to deprecated. * Changes from PR. * Changes from PR. * Changes from PR. * Revert EQ and NEQ test removals and other updates. * Update DEPENDENCIES file. * Update DEPENDENCIES file. * Update DEPENDENCIES file. * Update DEPENDENCIES file. * Update DEPENDENCIES file. --- edc-extensions/bpn-validation/README.md | 4 +- .../BusinessPartnerValidationExtension.java | 8 ++-- .../BusinessPartnerGroupFunction.java | 46 +++++++++++-------- .../BusinessPartnerGroupFunctionTest.java | 16 +++---- .../retirement/RetireAgreementTest.java | 2 +- 5 files changed, 42 insertions(+), 34 deletions(-) diff --git a/edc-extensions/bpn-validation/README.md b/edc-extensions/bpn-validation/README.md index b947e75f5..d2b8a87ad 100644 --- a/edc-extensions/bpn-validation/README.md +++ b/edc-extensions/bpn-validation/README.md @@ -28,7 +28,7 @@ Both previously mentioned evaluation functions are bound to the following scopes This policy states, that a certain BPN must, may or must not be member of a certain group. Groups may be represented as scalar, or as comma-separated lists. For semantic expression, the following ODRL operators are -supported: +supported: - `eq` - `neq` - `isPartOf` @@ -36,6 +36,8 @@ supported: - `isAnyOf` - `isNoneOf` +The `eq` and `neq` operators are **deprecated** in favor of `isAllOf`, `isAnyOf` and `isNoneOf` operators. + The following example demonstrates a full JSON-LD structure in expanded form, containing such a constraint. ### Example diff --git a/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/BusinessPartnerValidationExtension.java b/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/BusinessPartnerValidationExtension.java index 69360d574..c12ffabd1 100644 --- a/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/BusinessPartnerValidationExtension.java +++ b/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/BusinessPartnerValidationExtension.java @@ -75,10 +75,10 @@ public class BusinessPartnerValidationExtension implements ServiceExtension { @Override public void initialize(ServiceExtensionContext context) { - - bindToScope(TRANSFER_SCOPE, TransferProcessPolicyContext.class, new BusinessPartnerGroupFunction<>(store)); - bindToScope(NEGOTIATION_SCOPE, ContractNegotiationPolicyContext.class, new BusinessPartnerGroupFunction<>(store)); - bindToScope(CATALOG_SCOPE, CatalogPolicyContext.class, new BusinessPartnerGroupFunction<>(store)); + var monitor = context.getMonitor().withPrefix("BusinessPartnerGroupFunction"); + bindToScope(TRANSFER_SCOPE, TransferProcessPolicyContext.class, new BusinessPartnerGroupFunction<>(store, monitor)); + bindToScope(NEGOTIATION_SCOPE, ContractNegotiationPolicyContext.class, new BusinessPartnerGroupFunction<>(store, monitor)); + bindToScope(CATALOG_SCOPE, CatalogPolicyContext.class, new BusinessPartnerGroupFunction<>(store, monitor)); } private void bindToScope(String scope, Class contextType, AtomicConstraintRuleFunction function) { diff --git a/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunction.java b/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunction.java index ad199ad15..c5e2043b4 100644 --- a/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunction.java +++ b/edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunction.java @@ -25,14 +25,15 @@ import org.eclipse.edc.policy.engine.spi.PolicyContext; import org.eclipse.edc.policy.model.Operator; import org.eclipse.edc.policy.model.Permission; +import org.eclipse.edc.spi.monitor.Monitor; import org.eclipse.tractusx.edc.validation.businesspartner.spi.BusinessPartnerStore; -import java.util.Arrays; import java.util.Collection; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; @@ -82,13 +83,15 @@ public class BusinessPartnerGroupFunction ALLOWED_OPERATORS = List.of(EQ, NEQ, IN, IS_ALL_OF, IS_ANY_OF, IS_NONE_OF); private static final Map> OPERATOR_EVALUATOR_MAP = new HashMap<>(); private final BusinessPartnerStore store; + private final Monitor monitor; - public BusinessPartnerGroupFunction(BusinessPartnerStore store) { + public BusinessPartnerGroupFunction(BusinessPartnerStore store, Monitor monitor) { this.store = store; + this.monitor = monitor; OPERATOR_EVALUATOR_MAP.put(EQ, this::evaluateEquals); OPERATOR_EVALUATOR_MAP.put(NEQ, this::evaluateNotEquals); - OPERATOR_EVALUATOR_MAP.put(IN, this::evaluateIn); - OPERATOR_EVALUATOR_MAP.put(IS_ALL_OF, this::evaluateIn); + OPERATOR_EVALUATOR_MAP.put(IN, this::evaluateIsAnyOf); + OPERATOR_EVALUATOR_MAP.put(IS_ALL_OF, this::evaluateIsAllOf); OPERATOR_EVALUATOR_MAP.put(IS_ANY_OF, this::evaluateIsAnyOf); OPERATOR_EVALUATOR_MAP.put(IS_NONE_OF, this::evaluateIsNoneOf); } @@ -115,42 +118,46 @@ public boolean evaluate(Operator operator, Object rightOperand, Permission permi } // right-operand is anything other than String or Collection - var rightOperand1 = parseRightOperand(rightOperand, context); - if (rightOperand1 == null) { + var parsedRightOperand = parseRightOperand(rightOperand, context); + if (parsedRightOperand == null) { return false; } //call evaluator function - return OPERATOR_EVALUATOR_MAP.get(operator).apply(new BpnGroupHolder(assignedGroups, rightOperand1)); + return OPERATOR_EVALUATOR_MAP.get(operator).apply(new BpnGroupHolder(new HashSet<>(assignedGroups), parsedRightOperand)); } - private List parseRightOperand(Object rightValue, PolicyContext context) { + private Set parseRightOperand(Object rightValue, PolicyContext context) { if (rightValue instanceof String value) { var tokens = value.split(","); - return Arrays.asList(tokens); + return Set.of(tokens); } if (rightValue instanceof Collection) { - return ((Collection) rightValue).stream().map(Object::toString).toList(); + return ((Collection) rightValue).stream().map(Object::toString).collect(Collectors.toSet()); } context.reportProblem(format("Right operand expected to be either String or a Collection, but was %s", rightValue.getClass())); return null; } - private Boolean evaluateIn(BpnGroupHolder bpnGroupHolder) { - var assigned = bpnGroupHolder.assignedGroups; - // checks whether both lists overlap - return new HashSet<>(bpnGroupHolder.allowedGroups).containsAll(assigned); - } - + @Deprecated(since = "0.9.0") private Boolean evaluateNotEquals(BpnGroupHolder bpnGroupHolder) { - return !evaluateEquals(bpnGroupHolder); + monitor.warning("%s is a deprecated operator, in future please use %s operator.".formatted(NEQ, IS_NONE_OF)); + return !bpnGroupHolder.allowedGroups.equals(bpnGroupHolder.assignedGroups); } + @Deprecated(since = "0.9.0") private Boolean evaluateEquals(BpnGroupHolder bpnGroupHolder) { + monitor.warning("%s is a deprecated operator, in future please use %s operator.".formatted(EQ, IS_ALL_OF)); return bpnGroupHolder.allowedGroups.equals(bpnGroupHolder.assignedGroups); } + private Boolean evaluateIsAllOf(BpnGroupHolder bpnGroupHolder) { + var assigned = bpnGroupHolder.assignedGroups; + var allowed = bpnGroupHolder.allowedGroups; + return (assigned.isEmpty() || !allowed.isEmpty()) && assigned.containsAll(allowed); + } + private boolean evaluateIsAnyOf(BpnGroupHolder bpnGroupHolder) { if (bpnGroupHolder.allowedGroups.isEmpty() && bpnGroupHolder.assignedGroups.isEmpty()) { return true; @@ -159,7 +166,6 @@ private boolean evaluateIsAnyOf(BpnGroupHolder bpnGroupHolder) { var allowedGroups = bpnGroupHolder.allowedGroups; return bpnGroupHolder.assignedGroups .stream() - .distinct() .anyMatch(allowedGroups::contains); } @@ -168,9 +174,9 @@ private boolean evaluateIsNoneOf(BpnGroupHolder bpnGroupHolder) { } /** - * Internal utility class to hold the list of assigned groups for a BPN, and the list of groups specified in the policy ("allowed groups"). + * Internal utility class to hold the collection of assigned groups for a BPN, and the collection of groups specified in the policy ("allowed groups"). */ - private record BpnGroupHolder(List assignedGroups, List allowedGroups) { + private record BpnGroupHolder(Set assignedGroups, Set allowedGroups) { } } diff --git a/edc-extensions/bpn-validation/bpn-validation-core/src/test/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunctionTest.java b/edc-extensions/bpn-validation/bpn-validation-core/src/test/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunctionTest.java index f37e8f40f..460d7fd35 100644 --- a/edc-extensions/bpn-validation/bpn-validation-core/src/test/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunctionTest.java +++ b/edc-extensions/bpn-validation/bpn-validation-core/src/test/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunctionTest.java @@ -23,6 +23,7 @@ import org.eclipse.edc.participant.spi.ParticipantAgentPolicyContext; import org.eclipse.edc.policy.model.Operator; import org.eclipse.edc.policy.model.Permission; +import org.eclipse.edc.spi.monitor.Monitor; import org.eclipse.edc.spi.result.StoreResult; import org.eclipse.tractusx.edc.validation.businesspartner.spi.BusinessPartnerStore; import org.junit.jupiter.api.DisplayName; @@ -34,7 +35,6 @@ import org.junit.jupiter.params.provider.ArgumentsSource; import java.util.List; -import java.util.Map; import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; @@ -61,15 +61,15 @@ class BusinessPartnerGroupFunctionTest { private static final String TEST_BPN = "BPN000TEST"; private final BusinessPartnerStore store = mock(); private final ParticipantAgent agent = mock(); + private final Monitor monitor = mock(); private final Permission unusedPermission = Permission.Builder.newInstance().build(); private final ParticipantAgentPolicyContext context = new TestParticipantAgentPolicyContext(agent); - private final BusinessPartnerGroupFunction function = new BusinessPartnerGroupFunction<>(store); + private final BusinessPartnerGroupFunction function = new BusinessPartnerGroupFunction<>(store, monitor); @ParameterizedTest(name = "Invalid operator {0}") @ArgumentsSource(InvalidOperatorProvider.class) @DisplayName("Invalid operators, expect report in policy context") void evaluate_invalidOperator(Operator invalidOperator) { - var agent = new ParticipantAgent(Map.of(), Map.of()); var result = function.evaluate(invalidOperator, "test-group", unusedPermission, context); @@ -84,7 +84,7 @@ void evaluate_rightOperandNotStringOrCollection(Object rightValue) { when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.success(List.of("test-group"))); when(agent.getIdentity()).thenReturn(TEST_BPN); - var result = function.evaluate(EQ, rightValue, unusedPermission, context); + var result = function.evaluate(IS_ANY_OF, rightValue, unusedPermission, context); assertThat(result).isFalse(); assertThat(context.getProblems()).containsOnly("Right operand expected to be either String or a Collection, but was " + rightValue.getClass()); @@ -109,7 +109,7 @@ void evaluate_failedResolveForBpn_shouldBeFalse() { when(agent.getIdentity()).thenReturn(TEST_BPN); when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.notFound("foobar")); - var result = function.evaluate(EQ, allowedGroups, unusedPermission, context); + var result = function.evaluate(IS_ANY_OF, allowedGroups, unusedPermission, context); assertThat(result).isFalse(); assertThat(context.getProblems()).containsOnly("foobar"); @@ -156,14 +156,14 @@ public Stream provideArguments(ExtensionContext extensionCo Arguments.of("Empty groups", NEQ, List.of(), true), Arguments.of("Matching groups", IN, List.of(TEST_GROUP_1, TEST_GROUP_2), true), - Arguments.of("Overlapping groups", IN, List.of(TEST_GROUP_1, "different-group"), false), + Arguments.of("Overlapping groups", IN, List.of(TEST_GROUP_1, "different-group"), true), Arguments.of("Disjoint groups", IN, List.of("different-group", "another-different-group"), false), Arguments.of("Disjoint groups", IS_ALL_OF, List.of("different-group", "another-different-group"), false), Arguments.of("Matching groups", IS_ALL_OF, List.of(TEST_GROUP_1, TEST_GROUP_2), true), - Arguments.of("Overlapping groups", IS_ALL_OF, List.of(TEST_GROUP_1, TEST_GROUP_2, "different-group", "another-different-group"), false), + Arguments.of("Overlapping groups", IS_ALL_OF, List.of(TEST_GROUP_1, TEST_GROUP_2, "different-group", "another-different-group"), true), Arguments.of("Overlapping groups (1 overlap)", IS_ALL_OF, List.of(TEST_GROUP_1, "different-group"), false), - Arguments.of("Overlapping groups (1 overlap)", IS_ALL_OF, List.of(TEST_GROUP_1), true), + Arguments.of("Overlapping groups (1 overlap)", IS_ALL_OF, List.of(TEST_GROUP_1), false), Arguments.of("Disjoint groups", IS_ANY_OF, List.of("different-group", "another-different-group"), false), Arguments.of("Matching groups", IS_ANY_OF, List.of(TEST_GROUP_1, TEST_GROUP_2), true), diff --git a/edc-tests/edc-controlplane/agreement-retirement-tests/src/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java b/edc-tests/edc-controlplane/agreement-retirement-tests/src/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java index d208a941f..0ce7bdfa8 100644 --- a/edc-tests/edc-controlplane/agreement-retirement-tests/src/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java +++ b/edc-tests/edc-controlplane/agreement-retirement-tests/src/test/java/org/eclipse/tractusx/edc/tests/agreement/retirement/RetireAgreementTest.java @@ -91,7 +91,7 @@ void retireAgreement_shouldCloseTransferProcesses() { PROVIDER.createAsset(assetId, Map.of(), dataAddress); PROVIDER.storeBusinessPartner(CONSUMER.getBpn(), "test-group1"); - var accessPolicy = PROVIDER.createPolicyDefinition(PolicyHelperFunctions.bpnGroupPolicy(Operator.EQ, "test-group1")); + var accessPolicy = PROVIDER.createPolicyDefinition(PolicyHelperFunctions.bpnGroupPolicy(Operator.IS_ALL_OF, "test-group1")); var policy = PolicyHelperFunctions.frameworkPolicy(Map.of()); var contractPolicy = PROVIDER.createPolicyDefinition(policy); PROVIDER.createContractDefinition(assetId, "def-1", accessPolicy, contractPolicy);