diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java index 722c10afda3..1468feb2ec2 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java +++ b/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 { return; } LOG.debug("Validating rules for {} policy: {}", PolicyType.AccessControl, policy.getName()); + + Set 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 - || (rule.getEntityTagAttr() == null && rule.getEntityTypeAttr() == null && rule.getUserRoleAttr() == null)) { + + if (rule.getOperation() == null) { throw new IllegalArgumentException( String.format( - "Found invalid rule %s within policy %s. Check if operation is non-null " - + "and at least one among the user (subject) and entity (object) attributes is specified", - rule, policy.getName())); + "Found invalid rule %s within policy %s. Please ensure operation is non-null", + rule.getName(), 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. diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java index 59e8c232b62..1e4bf29bb9e 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java @@ -160,20 +160,52 @@ public class PolicyResourceTest extends EntityResourceTest { @Test void post_AccessControlPolicyWithValidRules_200_ok(TestInfo test) throws IOException { - CreatePolicy create = createAccessControlPolicyWithValidRules(test); + List 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 { - CreatePolicy create = createAccessControlPolicyWithInvalidRules(test); + void post_AccessControlPolicyWithInvalidRules_400_error(TestInfo test) { + List 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) " - + "and entity (object) attributes is specified"); + String.format( + "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 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 { .withOwner(USER_OWNER1); } - private CreatePolicy createAccessControlPolicyWithInvalidRules(TestInfo test) { - List 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 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());