Fix: Adjusted BPN validations on different operators

edc-extensions/bpn-validation/bpn-validation-core/src/main/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunction.java

@@ -24,6 +24,7 @@
 import org.eclipse.edc.policy.model.Operator;
 import org.eclipse.edc.policy.model.Permission;
 import org.eclipse.edc.spi.agent.ParticipantAgent;
+import org.eclipse.edc.spi.result.StoreResult;
 import org.eclipse.tractusx.edc.validation.businesspartner.spi.BusinessPartnerStore;
 
 import java.util.Arrays;
@@ -78,6 +79,7 @@
 public class BusinessPartnerGroupFunction implements AtomicConstraintFunction<Permission> {
     public static final String BUSINESS_PARTNER_CONSTRAINT_KEY = TX_NAMESPACE + "BusinessPartnerGroup";
     private static final List<Operator> ALLOWED_OPERATORS = List.of(EQ, NEQ, IN, IS_ALL_OF, IS_ANY_OF, IS_NONE_OF);
+    private static final List<Operator> NEGATIVE_OPERATORS = List.of(NEQ, IS_NONE_OF);
     private static final Map<Operator, Function<BpnGroupHolder, Boolean>> OPERATOR_EVALUATOR_MAP = new HashMap<>();
     private final BusinessPartnerStore store;
 
@@ -86,12 +88,11 @@ public BusinessPartnerGroupFunction(BusinessPartnerStore store) {
         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::evaluateEquals);
+        OPERATOR_EVALUATOR_MAP.put(IS_ALL_OF, this::evaluateIsAllOf);
         OPERATOR_EVALUATOR_MAP.put(IS_ANY_OF, this::evaluateIn);
-        OPERATOR_EVALUATOR_MAP.put(IS_NONE_OF, this::evaluateNotEquals);
+        OPERATOR_EVALUATOR_MAP.put(IS_NONE_OF, this::evaluateIsNoneOf);
     }
 
-
     /**
      * Policy evaluation function that checks whether a given BusinessPartnerNumber is covered by a given policy.
      * The evaluation is prematurely aborted (returns {@code false}) if:
@@ -120,20 +121,13 @@ public boolean evaluate(Operator operator, Object rightValue, Permission rule, P
             return false;
         }
 
-
         var bpn = participantAgent.getIdentity();
         var groups = store.resolveForBpn(bpn);
 
-        // BPN not found in database
-        if (groups.failed()) {
-            policyContext.reportProblem(groups.getFailureDetail());
-            return false;
-        }
-        var assignedGroups = groups.getContent();
+        var errorMessage = evaluateEmptyGroups(groups, operator, bpn);
 
-        // BPN was found, but it does not have groups assigned.
-        if (assignedGroups.isEmpty()) {
-            policyContext.reportProblem("No groups were assigned to BPN " + bpn);
+        if (errorMessage != null) {
+            policyContext.reportProblem(errorMessage);
             return false;
         }
 
@@ -143,20 +137,57 @@ public boolean evaluate(Operator operator, Object rightValue, Permission rule, P
             return false;
         }
 
+        var assignedGroups = groups.getContent();
+
         //call evaluator function
         return OPERATOR_EVALUATOR_MAP.get(operator).apply(new BpnGroupHolder(assignedGroups, rightOperand));
     }
 
+    /**
+     * Verify if BPN was found in the database and if the BPN has any group assigned. If one of those validations fail,
+     * it will return an error message, otherwise it will return {@code null}.
+     * The evaluation is prematurely aborted (returns {@code null}) if:
+     * <ul>
+     *     <li>{@link Operator} is a negative operator. Check {@link BusinessPartnerGroupFunction#NEGATIVE_OPERATORS}</li>
+     * </ul>
+     *
+     * @param groups    result of the BPN search on the database
+     * @param operator  operator sent for evaluation
+     * @param bpn       participant agent identifier
+     * @return  an error message in case of empty groups or empty assigned groups, {@code null} otherwise.
+     */
+    private String evaluateEmptyGroups(StoreResult<List<String>> groups, Operator operator, String bpn) {
+        var isNegativeOperator = NEGATIVE_OPERATORS.contains(operator);
+
+        if (isNegativeOperator) {
+            return null;
+        }
+
+        // BPN not found in database
+        if (groups.failed()) {
+            return groups.getFailureDetail();
+        }
+
+        var assignedGroups = groups.getContent();
+
+        // BPN was found, but it does not have groups assigned.
+        if (assignedGroups.isEmpty()) {
+            return "No groups were assigned to BPN " + bpn;
+        }
+
+        return null;
+    }
+
     private List<String> parseRightOperand(Object rightValue, PolicyContext context) {
-        if (rightValue instanceof String) {
-            var tokens = ((String) rightValue).split(",");
+        if (rightValue instanceof String value) {
+            var tokens = value.split(",");
             return Arrays.asList(tokens);
         }
         if (rightValue instanceof Collection<?>) {
             return ((Collection<?>) rightValue).stream().map(Object::toString).toList();
         }
 
-        context.reportProblem(format("Right operand expected to be either String or a Collection, but was " + rightValue.getClass()));
+        context.reportProblem(format("Right operand expected to be either String or a Collection, but was %s", rightValue.getClass()));
         return null;
     }
 
@@ -170,13 +201,29 @@ private Boolean evaluateIn(BpnGroupHolder bpnGroupHolder) {
     }
 
     private Boolean evaluateNotEquals(BpnGroupHolder bpnGroupHolder) {
-        return !evaluateIn(bpnGroupHolder);
+        return !evaluateEquals(bpnGroupHolder);
     }
 
     private Boolean evaluateEquals(BpnGroupHolder bpnGroupHolder) {
         return bpnGroupHolder.allowedGroups.equals(bpnGroupHolder.assignedGroups);
     }
 
+    private boolean evaluateIsAllOf(BpnGroupHolder bpnGroupHolder) {
+        var assigned = bpnGroupHolder.assignedGroups;
+        return bpnGroupHolder.allowedGroups
+                .stream()
+                .distinct()
+                .allMatch(assigned::contains);
+    }
+
+    private boolean evaluateIsNoneOf(BpnGroupHolder bpnGroupHolder) {
+        if (bpnGroupHolder.assignedGroups == null) {
+            return true;
+        }
+
+        return !evaluateIn(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").
      */

edc-extensions/bpn-validation/bpn-validation-core/src/test/java/org/eclipse/tractusx/edc/validation/businesspartner/functions/BusinessPartnerGroupFunctionTest.java

@@ -35,6 +35,7 @@
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.ArgumentsProvider;
 import org.junit.jupiter.params.provider.ArgumentsSource;
+import org.junit.jupiter.params.provider.EnumSource;
 
 import java.util.Collections;
 import java.util.List;
@@ -50,6 +51,7 @@
 import static org.eclipse.edc.policy.model.Operator.IS_A;
 import static org.eclipse.edc.policy.model.Operator.IS_ALL_OF;
 import static org.eclipse.edc.policy.model.Operator.IS_ANY_OF;
+import static org.eclipse.edc.policy.model.Operator.IS_NONE_OF;
 import static org.eclipse.edc.policy.model.Operator.LEQ;
 import static org.eclipse.edc.policy.model.Operator.LT;
 import static org.eclipse.edc.policy.model.Operator.NEQ;
@@ -122,24 +124,46 @@ void evaluate_validOperator(String ignored, Operator operator, List<String> assi
         assertThat(function.evaluate(operator, allowedGroups, createPermission(operator, allowedGroups), context)).isEqualTo(expectedOutcome);
     }
 
-    @Test
-    void evaluate_noEntryForBpn() {
-        var operator = NEQ;
+    @EnumSource(value = Operator.class, names = {"NEQ", "IS_NONE_OF"})
+    @ParameterizedTest
+    void evaluate_noEntryForBpnWithNegativeOperator_shouldBeTrue(Operator operator) {
+        var allowedGroups = List.of(TEST_GROUP_1, TEST_GROUP_2);
+        when(context.getContextData(eq(ParticipantAgent.class))).thenReturn(new ParticipantAgent(Map.of(), Map.of(PARTICIPANT_IDENTITY, TEST_BPN)));
+        when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.notFound("foobar"));
+
+        assertThat(function.evaluate(operator, allowedGroups, createPermission(operator, allowedGroups), context)).isTrue();
+    }
+
+    @EnumSource(value = Operator.class, names = {"NEQ", "IS_NONE_OF"})
+    @ParameterizedTest
+    void evaluate_noGroupsAssignedToBpnWithNegativeOperator_shouldBeTrue(Operator operator) {
+        var allowedGroups = List.of(TEST_GROUP_1, TEST_GROUP_2);
+        when(context.getContextData(eq(ParticipantAgent.class))).thenReturn(new ParticipantAgent(Map.of(), Map.of(PARTICIPANT_IDENTITY, TEST_BPN)));
+        when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.success(Collections.emptyList()));
+
+        assertThat(function.evaluate(operator, allowedGroups, createPermission(operator, allowedGroups), context)).isTrue();
+    }
+
+    @EnumSource(value = Operator.class, names = {"EQ", "IS_ANY_OF", "IS_ALL_OF", "IN"})
+    @ParameterizedTest
+    void evaluate_noEntryForBpnWithPositiveOperator_shouldBeFalse(Operator operator) {
         var allowedGroups = List.of(TEST_GROUP_1, TEST_GROUP_2);
         when(context.getContextData(eq(ParticipantAgent.class))).thenReturn(new ParticipantAgent(Map.of(), Map.of(PARTICIPANT_IDENTITY, TEST_BPN)));
         when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.notFound("foobar"));
 
         assertThat(function.evaluate(operator, allowedGroups, createPermission(operator, allowedGroups), context)).isFalse();
+        verify(context).reportProblem("foobar");
     }
 
-    @Test
-    void evaluate_noGroupsAssignedToBpn() {
-        var operator = NEQ;
+    @EnumSource(value = Operator.class, names = {"EQ", "IS_ANY_OF", "IS_ALL_OF", "IN"})
+    @ParameterizedTest
+    void evaluate_noGroupsAssignedToBpnWithPositiveOperator_shouldBeFalse(Operator operator) {
         var allowedGroups = List.of(TEST_GROUP_1, TEST_GROUP_2);
         when(context.getContextData(eq(ParticipantAgent.class))).thenReturn(new ParticipantAgent(Map.of(), Map.of(PARTICIPANT_IDENTITY, TEST_BPN)));
         when(store.resolveForBpn(TEST_BPN)).thenReturn(StoreResult.success(Collections.emptyList()));
 
         assertThat(function.evaluate(operator, allowedGroups, createPermission(operator, allowedGroups), context)).isFalse();
+        verify(context).reportProblem("No groups were assigned to BPN " + TEST_BPN);
     }
 
     private Permission createPermission(Operator op, List<String> rightOperand) {
@@ -174,22 +198,29 @@ public Stream<? extends Arguments> provideArguments(ExtensionContext extensionCo
                     Arguments.of("Overlapping groups", EQ, List.of("different-group"), false),
 
                     Arguments.of("Disjoint groups", NEQ, List.of("different-group", "another-different-group"), true),
-                    Arguments.of("Overlapping groups", NEQ, List.of(TEST_GROUP_1, "different-group"), false),
+                    Arguments.of("Overlapping groups", NEQ, List.of(TEST_GROUP_1, "different-group"), true),
                     Arguments.of("Matching groups", NEQ, List.of(TEST_GROUP_1, TEST_GROUP_2), false),
+                    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"), 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), 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),
                     Arguments.of("Overlapping groups (1 overlap)", IS_ANY_OF, List.of(TEST_GROUP_1, "different-group", "another-different-group"), true),
-                    Arguments.of("Overlapping groups (2 overlap)", IS_ANY_OF, List.of(TEST_GROUP_1, TEST_GROUP_2, "different-group", "another-different-group"), true)
+                    Arguments.of("Overlapping groups (2 overlap)", IS_ANY_OF, List.of(TEST_GROUP_1, TEST_GROUP_2, "different-group", "another-different-group"), true),
+
+                    Arguments.of("Disjoint groups", IS_NONE_OF, List.of("different-group", "another-different-group"), true),
+                    Arguments.of("Matching groups", IS_NONE_OF, List.of(TEST_GROUP_1, TEST_GROUP_2), false),
+                    Arguments.of("Overlapping groups", IS_NONE_OF, List.of(TEST_GROUP_1, "another-different-group"), false),
+                    Arguments.of("Empty groups", IS_NONE_OF, List.of(), true)
             );
         }
     }