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

@@ -78,6 +78,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> POSITIVE_OPERATORS = List.of(EQ, IN, IS_ALL_OF, IS_ANY_OF);
     private static final Map<Operator, Function<BpnGroupHolder, Boolean>> OPERATOR_EVALUATOR_MAP = new HashMap<>();
     private final BusinessPartnerStore store;
 
@@ -86,20 +87,19 @@ 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:
      * <ul>
      *     <li>No {@link ParticipantAgent} was found on the {@link PolicyContext}</li>
      *     <li>The operator is invalid. Check {@link BusinessPartnerGroupFunction#ALLOWED_OPERATORS} for valid operators.</li>
-     *     <li>No database entry was found for the BPN (taken from the {@link ParticipantAgent})</li>
-     *     <li>A database entry was found, but no group-IDs were assigned</li>
+     *     <li>No database entry was found for the BPN (taken from the {@link ParticipantAgent}) and the operator is 'positive' {@link BusinessPartnerGroupFunction#POSITIVE_OPERATORS}</li>
+     *     <li>A database entry was found, but no group-IDs were assigned and the operator is 'positive' {@link BusinessPartnerGroupFunction#POSITIVE_OPERATORS}</li>
      *     <li>The right value is anything other than {@link String} or {@link Collection}</li>
      * </ul>
      */
@@ -120,20 +120,23 @@ public boolean evaluate(Operator operator, Object rightValue, Permission rule, P
             return false;
         }
 
-
         var bpn = participantAgent.getIdentity();
         var groups = store.resolveForBpn(bpn);
 
+        var isPositiveOperator = POSITIVE_OPERATORS.contains(operator);
+        var errorMessage = "";
+
         // BPN not found in database
         if (groups.failed()) {
-            policyContext.reportProblem(groups.getFailureDetail());
-            return false;
-        }
-        var assignedGroups = groups.getContent();
+            errorMessage = groups.getFailureDetail();
 
         // BPN was found, but it does not have groups assigned.
-        if (assignedGroups.isEmpty()) {
-            policyContext.reportProblem("No groups were assigned to BPN " + bpn);
+        } else if (groups.getContent().isEmpty()) {
+            errorMessage = "No groups were assigned to BPN " + bpn;
+        }
+
+        if (!errorMessage.isBlank() && isPositiveOperator) {
+            policyContext.reportProblem(errorMessage);
             return false;
         }
 
@@ -143,20 +146,22 @@ 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));
     }
 
     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 +175,33 @@ 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) {
+        if (bpnGroupHolder.allowedGroups.isEmpty()) {
+            return false;
+        }
+
+        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;
@@ -115,31 +117,64 @@ void evaluate_rightOperandNotStringOrCollection() {
     @ArgumentsSource(ValidOperatorProvider.class)
     @DisplayName("Valid operators, evaluating different circumstances")
     void evaluate_validOperator(String ignored, Operator operator, List<String> assignedBpn, boolean expectedOutcome) {
-
         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(assignedBpn));
         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);
+    }
+
+    @ArgumentsSource(OperatorForEmptyGroupsProvider.class)
+    @ParameterizedTest
+    void evaluate_groupsAssignedButNoGroupsSentToEvaluate(Operator operator, boolean expectedOutcome) {
+        var allowedGroups = List.<String>of();
+        var assignedBpnGroups = 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(assignedBpnGroups));
+
+        assertThat(function.evaluate(operator, allowedGroups, createPermission(operator, allowedGroups), context)).isEqualTo(expectedOutcome);
     }
 
     private Permission createPermission(Operator op, List<String> rightOperand) {
@@ -174,22 +209,43 @@ 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)
+            );
+        }
+    }
+
+    private static class OperatorForEmptyGroupsProvider implements ArgumentsProvider {
+        @Override
+        public Stream<? extends Arguments> provideArguments(ExtensionContext extensionContext) throws Exception {
+            return Stream.of(
+                    Arguments.of(EQ, false),
+                    Arguments.of(NEQ, true),
+                    Arguments.of(IN, false),
+                    Arguments.of(IS_ALL_OF, false),
+                    Arguments.of(IS_ANY_OF, false),
+                    Arguments.of(IS_NONE_OF, true)
             );
         }
     }