Throw error if multiple rules within policy have same operation (#2344)

catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java

@@ -19,7 +19,9 @@ import java.io.IOException;
 import java.net.URI;
 import java.text.ParseException;
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 import java.util.UUID;
 import lombok.SneakyThrows;
 import lombok.extern.slf4j.Slf4j;
@@ -34,6 +36,7 @@ import org.openmetadata.catalog.security.policyevaluator.PolicyEvaluator;
 import org.openmetadata.catalog.type.ChangeDescription;
 import org.openmetadata.catalog.type.EntityReference;
 import org.openmetadata.catalog.type.Include;
+import org.openmetadata.catalog.type.MetadataOperation;
 import org.openmetadata.catalog.type.PolicyType;
 import org.openmetadata.catalog.util.EntityInterface;
 import org.openmetadata.catalog.util.EntityUtil;
@@ -191,18 +194,32 @@ public class PolicyRepository extends EntityRepository<Policy> {
       return;
     }
     LOG.debug("Validating rules for {} policy: {}", PolicyType.AccessControl, policy.getName());
+
+    Set<MetadataOperation> operations = new HashSet<>();
     for (Object ruleObject : policy.getRules()) {
       // Cast to access control policy Rule.
       Rule rule = JsonUtils.readValue(JsonUtils.getJsonStructure(ruleObject).toString(), Rule.class);
-      // If the operation is not specified or if all user (subject) and entity (object) attributes are null, the rule
+
-      // is invalid.
+      if (rule.getOperation() == null) {
-      if (rule.getOperation() == null
-          || (rule.getEntityTagAttr() == null && rule.getEntityTypeAttr() == null && rule.getUserRoleAttr() == null)) {
         throw new IllegalArgumentException(
             String.format(
-                "Found invalid rule %s within policy %s. Check if operation is non-null "
+                "Found invalid rule %s within policy %s. Please ensure operation is non-null",
-                    + "and at least one among the user (subject) and entity (object) attributes is specified",
+                rule.getName(), policy.getName()));
-                rule, policy.getName()));
+      }
+
+      if (!operations.add(rule.getOperation())) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Found multiple rules with operation %s within policy %s. Please ensure that operation across all rules within the policy are distinct",
+                rule.getOperation(), policy.getName()));
+      }
+
+      // If all user (subject) and entity (object) attributes are null, the rule is invalid.
+      if (rule.getEntityTagAttr() == null && rule.getEntityTypeAttr() == null && rule.getUserRoleAttr() == null) {
+        throw new IllegalArgumentException(
+            String.format(
+                "Found invalid rule %s within policy %s. Please ensure that at least one among the user (subject) and entity (object) attributes is specified",
+                rule.getName(), policy.getName()));
       }
     }
     // No validation errors, if execution reaches here.

catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java

@@ -160,20 +160,52 @@ public class PolicyResourceTest extends EntityResourceTest<Policy> {
 
   @Test
   void post_AccessControlPolicyWithValidRules_200_ok(TestInfo test) throws IOException {
-    CreatePolicy create = createAccessControlPolicyWithValidRules(test);
+    List<Rule> rules = new ArrayList<>();
+    rules.add(
+        PolicyUtils.accessControlRule(null, null, "DataConsumer", MetadataOperation.UpdateDescription, true, 0, true));
+    rules.add(PolicyUtils.accessControlRule(null, null, "DataConsumer", MetadataOperation.UpdateTags, true, 1, true));
+    CreatePolicy create = createAccessControlPolicyWithRules(getEntityName(test), rules);
     createAndCheckEntity(create, adminAuthHeaders());
   }
 
   @Test
-  void post_AccessControlPolicyWithInvalidRules_400_error(TestInfo test) throws IOException {
+  void post_AccessControlPolicyWithInvalidRules_400_error(TestInfo test) {
-    CreatePolicy create = createAccessControlPolicyWithInvalidRules(test);
+    List<Rule> rules = new ArrayList<>();
+    rules.add(
+        PolicyUtils.accessControlRule("rule21", null, null, null, MetadataOperation.UpdateDescription, true, 0, true));
+    CreatePolicy create = createAccessControlPolicyWithRules(getEntityName(test), rules);
     HttpResponseException exception =
         assertThrows(HttpResponseException.class, () -> createEntity(create, adminAuthHeaders()));
     assertResponseContains(
         exception,
         BAD_REQUEST,
-        "Check if operation is non-null and at least one among the user (subject) "
+        String.format(
-            + "and entity (object) attributes is specified");
+            "Found invalid rule rule21 within policy %s. Please ensure that at least one among the user "
+                + "(subject) and entity (object) attributes is specified",
+            getEntityName(test)));
+  }
+
+  @Test
+  void post_AccessControlPolicyWithDuplicateRules_400_error(TestInfo test) {
+    List<Rule> rules = new ArrayList<>();
+    rules.add(
+        PolicyUtils.accessControlRule(
+            "rule1", null, null, "DataConsumer", MetadataOperation.UpdateDescription, true, 0, true));
+    rules.add(
+        PolicyUtils.accessControlRule(
+            "rule2", null, null, "DataConsumer", MetadataOperation.UpdateTags, true, 1, true));
+    rules.add(
+        PolicyUtils.accessControlRule(
+            "rule3", null, null, "DataConsumer", MetadataOperation.UpdateTags, true, 1, true));
+    CreatePolicy create = createAccessControlPolicyWithRules(getEntityName(test), rules);
+    HttpResponseException exception =
+        assertThrows(HttpResponseException.class, () -> createEntity(create, adminAuthHeaders()));
+    assertResponseContains(
+        exception,
+        BAD_REQUEST,
+        String.format(
+            "Found multiple rules with operation UpdateTags within policy %s. Please ensure that operation across all rules within the policy are distinct",
+            getEntityName(test)));
   }
 
   @Test
@@ -416,20 +448,6 @@ public class PolicyResourceTest extends EntityResourceTest<Policy> {
         .withOwner(USER_OWNER1);
   }
 
-  private CreatePolicy createAccessControlPolicyWithInvalidRules(TestInfo test) {
-    List<Rule> rules = new ArrayList<>();
-    rules.add(PolicyUtils.accessControlRule(null, null, null, MetadataOperation.UpdateDescription, true, 0, true));
-    return createAccessControlPolicyWithRules(getEntityName(test), rules);
-  }
-
-  private CreatePolicy createAccessControlPolicyWithValidRules(TestInfo test) {
-    List<Rule> rules = new ArrayList<>();
-    rules.add(
-        PolicyUtils.accessControlRule(null, null, "DataConsumer", MetadataOperation.UpdateDescription, true, 0, true));
-    rules.add(PolicyUtils.accessControlRule(null, null, "DataConsumer", MetadataOperation.UpdateTags, true, 1, true));
-    return createAccessControlPolicyWithRules(getEntityName(test), rules);
-  }
-
   private static Location createLocation() throws HttpResponseException {
     CreateLocation createLocation = LocationResourceTest.create(LOCATION_NAME, AWS_STORAGE_SERVICE_REFERENCE);
     return TestUtils.post(getResource("locations"), createLocation, Location.class, adminAuthHeaders());