Fixes #6719 Validate rule conditions and return meaningful errors in API response (#6720)

catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java

@@ -169,4 +169,12 @@ public final class CatalogExceptionMessage {
   public static String announcementInvalidStartTime() {
     return "Announcement start time must be earlier than the end time";
   }
+
+  public static String failedToParse(String message) {
+    return String.format("Failed to parse - %s", message);
+  }
+
+  public static String failedToEvaluate(String message) {
+    return String.format("Failed to evaluate - %s", message);
+  }
 }

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

@@ -27,9 +27,11 @@ import org.jdbi.v3.sqlobject.transaction.Transaction;
 import org.openmetadata.catalog.Entity;
 import org.openmetadata.catalog.entity.data.Location;
 import org.openmetadata.catalog.entity.policies.Policy;
+import org.openmetadata.catalog.entity.policies.accessControl.Rule;
 import org.openmetadata.catalog.exception.CatalogExceptionMessage;
 import org.openmetadata.catalog.jdbi3.CollectionDAO.EntityRelationshipRecord;
 import org.openmetadata.catalog.resources.policies.PolicyResource;
+import org.openmetadata.catalog.security.policyevaluator.CompiledRule;
 import org.openmetadata.catalog.type.EntityReference;
 import org.openmetadata.catalog.type.PolicyType;
 import org.openmetadata.catalog.type.Relationship;
@@ -72,15 +74,13 @@ public class PolicyRepository extends EntityRepository<Policy> {
   /* Get all the teams that use this policy */
   private List<EntityReference> getTeams(Policy policy) throws IOException {
     List<EntityRelationshipRecord> records = findFrom(policy.getId(), POLICY, Relationship.HAS, Entity.TEAM);
-    List<EntityReference> teams = EntityUtil.populateEntityReferences(records, Entity.TEAM);
+    return EntityUtil.populateEntityReferences(records, Entity.TEAM);
-    return teams;
   }
 
   /* Get all the roles that use this policy */
   private List<EntityReference> getRoles(Policy policy) throws IOException {
     List<EntityRelationshipRecord> records = findFrom(policy.getId(), POLICY, Relationship.HAS, Entity.ROLE);
-    List<EntityReference> roles = EntityUtil.populateEntityReferences(records, Entity.ROLE);
+    return EntityUtil.populateEntityReferences(records, Entity.ROLE);
-    return roles;
   }
 
   /** Generate EntityReference for a given Policy's Location. * */
@@ -137,7 +137,14 @@ public class PolicyRepository extends EntityRepository<Policy> {
     if (!policy.getPolicyType().equals(PolicyType.AccessControl)) {
       return;
     }
-    EntityUtil.resolveRules(policy.getRules()); // Resolve rules performs JSON schema constraint based validation
+
+    // Resolve JSON blobs into Rule object and perform schema based validation
+    List<Rule> rules = EntityUtil.resolveRules(policy.getRules());
+
+    // Validate all the expressions in the rule
+    for (Rule rule : rules) {
+      CompiledRule.validateExpression(rule.getCondition(), Boolean.class);
+    }
   }
 
   public List<Policy> getAccessControlPolicies() throws IOException {

catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/CompiledRule.java

@@ -7,6 +7,7 @@ import java.util.Iterator;
 import java.util.List;
 import lombok.extern.slf4j.Slf4j;
 import org.openmetadata.catalog.entity.policies.accessControl.Rule;
+import org.openmetadata.catalog.exception.CatalogExceptionMessage;
 import org.openmetadata.catalog.security.AuthorizationException;
 import org.openmetadata.catalog.security.policyevaluator.SubjectContext.PolicyContext;
 import org.openmetadata.catalog.type.MetadataOperation;
@@ -30,12 +31,39 @@ public class CompiledRule extends Rule {
         .withResources(rule.getResources());
   }
 
+  public static Expression parseExpression(String condition) {
+    if (condition == null) {
+      return null;
+    }
+    try {
+      return EXPRESSION_PARSER.parseExpression(condition);
+    } catch (Exception exception) {
+      throw new IllegalArgumentException(CatalogExceptionMessage.failedToParse(exception.getMessage()));
+    }
+  }
+
+  /** Used only for validating the expressions when new rule is created */
+  public static <T> T validateExpression(String condition, Class<T> clz) {
+    if (condition == null) {
+      return null;
+    }
+    Expression expression = parseExpression(condition);
+    RuleEvaluator ruleEvaluator = new RuleEvaluator(null, null, null);
+    try {
+      return expression.getValue(ruleEvaluator, clz);
+    } catch (Exception exception) {
+      // Remove unnecessary class details in the exception message
+      String message = exception.getMessage().replaceAll("on type .*$", "").replaceAll("on object .*$", "");
+      throw new IllegalArgumentException(CatalogExceptionMessage.failedToEvaluate(message));
+    }
+  }
+
   public Expression getExpression() {
     if (this.getCondition() == null) {
       return null;
     }
     if (expression == null) {
-      expression = EXPRESSION_PARSER.parseExpression(this.getCondition());
+      expression = parseExpression(getCondition());
     }
     return expression;
   }

catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluator.java

@@ -24,16 +24,19 @@ public class RuleEvaluator {
 
   /** Returns true if the resource being accessed has no owner */
   public boolean noOwner() throws IOException {
-    return resourceContext.getOwner() == null;
+    return resourceContext != null && resourceContext.getOwner() == null;
   }
 
   /** Returns true if the resource is owned by the subject/user */
   public boolean isOwner() throws IOException {
-    return subjectContext.isOwner(resourceContext.getOwner());
+    return subjectContext != null && subjectContext.isOwner(resourceContext.getOwner());
   }
 
   /** Returns true if the tags of a resource being accessed matches all the tags provided as parameters */
   public boolean matchAllTags(String... tagFQNs) throws IOException {
+    if (resourceContext == null) {
+      return false;
+    }
     List<TagLabel> tags = resourceContext.getTags();
     for (String tagFQN : tagFQNs) {
       TagLabel found = tags.stream().filter(t -> t.getTagFQN().equals(tagFQN)).findAny().orElse(null);
@@ -46,6 +49,9 @@ public class RuleEvaluator {
 
   /** Returns true if the tags of a resource being accessed matches at least one tag provided as parameters */
   public boolean matchAnyTag(List<String> tagFQNs) throws IOException {
+    if (resourceContext == null) {
+      return false;
+    }
     List<TagLabel> tags = resourceContext.getTags();
     for (String tagFQN : tagFQNs) {
       TagLabel found = tags.stream().filter(t -> t.getTagFQN().equals(tagFQN)).findAny().orElse(null);

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

@@ -22,6 +22,7 @@ import static org.openmetadata.catalog.util.TestUtils.UpdateType.MINOR_UPDATE;
 import static org.openmetadata.catalog.util.TestUtils.assertListNotNull;
 import static org.openmetadata.catalog.util.TestUtils.assertListNull;
 import static org.openmetadata.catalog.util.TestUtils.assertResponse;
+import static org.openmetadata.catalog.util.TestUtils.assertResponseContains;
 
 import java.io.IOException;
 import java.net.URI;
@@ -163,6 +164,61 @@ public class PolicyResourceTest extends EntityResourceTest<Policy, CreatePolicy>
     assertResponse(() -> createEntity(create2, ADMIN_AUTH_HEADERS), BAD_REQUEST, "[resources must not be null]");
   }
 
+  @Test
+  void post_policiesWithInvalidConditions(TestInfo test) {
+    String policyName = getEntityName(test);
+    Rule rule = accessControlRule(List.of("all"), List.of(MetadataOperation.ALL), ALLOW);
+    CreatePolicy create = createAccessControlPolicyWithRules(policyName, List.of(rule));
+
+    // No ending parenthesis
+    rule.withCondition("!matchAnyTag('tag1'");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to parse");
+
+    // No starting parenthesis
+    rule.withCondition("!matchAnyTag'tag1')");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to parse");
+
+    // Non-terminating quoted string 'unexpectedParam (missing end quote) or unexpectedParam' (missing beginning quote)
+    rule.withCondition("!isOwner('unexpectedParam)");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to parse");
+    rule.withCondition("!isOwner(unexpectedParam')");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to parse");
+
+    // Incomplete expressions - right operand problem
+    rule.withCondition("!isOwner() ||");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to parse");
+    rule.withCondition("|| isOwner()");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to parse");
+
+    // Incomplete expressions
+    rule.withCondition("!");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to parse");
+
+    // matchAnyTag() method does not input parameters
+    rule.withCondition("!matchAnyTag()");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to evaluate");
+
+    // isOwner() has Unexpected input parameter
+    rule.withCondition("!isOwner('unexpectedParam')");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to evaluate");
+
+    // Invalid function name
+    rule.withCondition("invalidFunction()");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to evaluate");
+    rule.withCondition("isOwner() || invalidFunction()");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to evaluate");
+
+    // Function matchTags() has no input parameter
+    rule.withCondition("matchTags()");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to evaluate");
+
+    // Invalid text
+    rule.withCondition("a");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to evaluate");
+    rule.withCondition("abc");
+    assertResponseContains(() -> createEntity(create, ADMIN_AUTH_HEADERS), BAD_REQUEST, "Failed to evaluate");
+  }
+
   @Test
   void patch_PolicyAttributes_200_ok(TestInfo test) throws IOException {
     Policy policy = createAndCheckEntity(createRequest(test), ADMIN_AUTH_HEADERS).withLocation(null);

catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluatorTest.java

@@ -1,9 +1,8 @@
 package org.openmetadata.catalog.security.policyevaluator;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
-import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.openmetadata.catalog.security.policyevaluator.CompiledRule.parseExpression;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -22,134 +21,90 @@ import org.openmetadata.catalog.jdbi3.TeamRepository;
 import org.openmetadata.catalog.jdbi3.UserRepository;
 import org.openmetadata.catalog.type.EntityReference;
 import org.openmetadata.catalog.type.TagLabel;
-import org.springframework.expression.spel.standard.SpelExpressionParser;
+import org.springframework.expression.EvaluationContext;
 import org.springframework.expression.spel.support.StandardEvaluationContext;
 
 class RuleEvaluatorTest {
-  private static SpelExpressionParser expressionParser;
   private static Table table;
   private static User user;
-  private static ResourceContext resourceContext;
+  private static EvaluationContext evaluationContext;
-  private static SubjectContext subjectContext;
 
   @BeforeAll
   public static void setup() throws NoSuchMethodException {
     Entity.registerEntity(User.class, Entity.USER, Mockito.mock(UserDAO.class), Mockito.mock(UserRepository.class));
     Entity.registerEntity(Team.class, Entity.TEAM, Mockito.mock(TeamDAO.class), Mockito.mock(TeamRepository.class));
-    expressionParser = new SpelExpressionParser();
     table = new Table().withName("table");
     user = new User().withId(UUID.randomUUID()).withName("user");
-    resourceContext =
+    ResourceContext resourceContext =
         ResourceContext.builder()
             .resource("table")
             .entity(table)
             .entityRepository(Mockito.mock(TableRepository.class))
             .build();
-    subjectContext = new SubjectContext(user);
+    SubjectContext subjectContext = new SubjectContext(user);
+    RuleEvaluator ruleEvaluator = new RuleEvaluator(null, subjectContext, resourceContext);
+    evaluationContext = new StandardEvaluationContext(ruleEvaluator);
   }
 
   @Test
   void test_noOwner() {
-    RuleEvaluator policyContext = new RuleEvaluator(null, subjectContext, resourceContext);
-    StandardEvaluationContext evaluationContext = new StandardEvaluationContext(policyContext);
-
     // Set no owner to the entity and test noOwner method
     table.setOwner(null);
-    assertTrue(expressionParser.parseExpression("noOwner()").getValue(evaluationContext, Boolean.class));
+    assertEquals(Boolean.TRUE, evaluateExpression("noOwner()"));
-    assertFalse(expressionParser.parseExpression("!noOwner()").getValue(evaluationContext, Boolean.class));
+    assertNotEquals(Boolean.TRUE, evaluateExpression("!noOwner()"));
 
     // Set owner to the entity and test noOwner method
     table.setOwner(new EntityReference().withId(UUID.randomUUID()).withType(Entity.USER));
-    assertNotEquals(
+    assertNotEquals(Boolean.TRUE, evaluateExpression("noOwner()"));
-        Boolean.TRUE, expressionParser.parseExpression("noOwner()").getValue(evaluationContext, Boolean.class));
+    assertEquals(Boolean.TRUE, evaluateExpression("!noOwner()"));
-    assertEquals(
-        Boolean.TRUE, expressionParser.parseExpression("!noOwner()").getValue(evaluationContext, Boolean.class));
   }
 
   @Test
   void test_isOwner() {
-    RuleEvaluator policyContext = new RuleEvaluator(null, subjectContext, resourceContext);
-    StandardEvaluationContext evaluationContext = new StandardEvaluationContext(policyContext);
-
     // Table owner is a different user (random ID) and hence isOwner returns false
     table.setOwner(new EntityReference().withId(UUID.randomUUID()).withType(Entity.USER).withName("otherUser"));
-    assertNotEquals(
+    assertNotEquals(Boolean.TRUE, evaluateExpression("isOwner()"));
-        Boolean.TRUE, expressionParser.parseExpression("isOwner()").getValue(evaluationContext, Boolean.class));
+    assertEquals(Boolean.TRUE, evaluateExpression("!isOwner()"));
-    assertEquals(
-        Boolean.TRUE, expressionParser.parseExpression("!isOwner()").getValue(evaluationContext, Boolean.class));
 
     // Table owner is same as the user in subjectContext and hence isOwner returns true
     table.setOwner(new EntityReference().withId(user.getId()).withType(Entity.USER).withName(user.getName()));
-    assertEquals(
+    assertEquals(Boolean.TRUE, evaluateExpression("isOwner()"));
-        Boolean.TRUE, expressionParser.parseExpression("isOwner()").getValue(evaluationContext, Boolean.class));
+    assertNotEquals(Boolean.TRUE, evaluateExpression("!isOwner()"));
-    assertNotEquals(
-        Boolean.TRUE, expressionParser.parseExpression("!isOwner()").getValue(evaluationContext, Boolean.class));
 
     // noOwner() || isOwner() - with noOwner being true and isOwner false
     table.setOwner(null);
-    assertEquals(
+    assertEquals(Boolean.TRUE, evaluateExpression("noOwner() || isOwner()"));
-        Boolean.TRUE,
+    assertNotEquals(Boolean.TRUE, evaluateExpression("!noOwner() && !isOwner()"));
-        expressionParser.parseExpression("noOwner() || isOwner()").getValue(evaluationContext, Boolean.class));
-    assertNotEquals(
-        Boolean.TRUE,
-        expressionParser.parseExpression("!noOwner() && !isOwner()").getValue(evaluationContext, Boolean.class));
 
     // noOwner() || isOwner() - with noOwner is false and isOwner true
     table.setOwner(new EntityReference().withId(user.getId()).withType(Entity.USER).withName(user.getName()));
-    assertEquals(
+    assertEquals(Boolean.TRUE, evaluateExpression("noOwner() || isOwner()"));
-        Boolean.TRUE,
+    assertNotEquals(Boolean.TRUE, evaluateExpression("!noOwner() && !isOwner()"));
-        expressionParser.parseExpression("noOwner() || isOwner()").getValue(evaluationContext, Boolean.class));
-    assertNotEquals(
-        Boolean.TRUE,
-        expressionParser.parseExpression("!noOwner() && !isOwner()").getValue(evaluationContext, Boolean.class));
   }
 
   @Test
   void test_allTagsOrAnyTag() {
-    RuleEvaluator policyContext = new RuleEvaluator(null, subjectContext, resourceContext);
-    StandardEvaluationContext evaluationContext = new StandardEvaluationContext(policyContext);
-
     // All tags present
     table.withTags(getTags("tag1", "tag2", "tag3"));
-    assertEquals(
+    assertEquals(Boolean.TRUE, evaluateExpression("matchAllTags('tag1', 'tag2', 'tag3')"));
-        Boolean.TRUE,
+    assertNotEquals(Boolean.TRUE, evaluateExpression("!matchAllTags('tag1', 'tag2', 'tag3')"));
-        expressionParser
-            .parseExpression("matchAllTags('tag1', 'tag2', 'tag3')")
-            .getValue(evaluationContext, Boolean.class));
-    assertNotEquals(
-        Boolean.TRUE,
-        expressionParser
-            .parseExpression("!matchAllTags('tag1', 'tag2', 'tag3')")
-            .getValue(evaluationContext, Boolean.class));
 
     // One tag `tag4` is missing
     table.withTags(getTags("tag1", "tag2", "tag4"));
-    assertNotEquals(
+    assertNotEquals(Boolean.TRUE, evaluateExpression("matchAllTags('tag1', 'tag2', 'tag3')"));
-        Boolean.TRUE,
+    assertEquals(Boolean.TRUE, evaluateExpression("!matchAllTags('tag1', 'tag2', 'tag3')"));
-        expressionParser
-            .parseExpression("matchAllTags('tag1', 'tag2', 'tag3')")
-            .getValue(evaluationContext, Boolean.class));
-    assertEquals(
-        Boolean.TRUE,
-        expressionParser
-            .parseExpression("!matchAllTags('tag1', 'tag2', 'tag3')")
-            .getValue(evaluationContext, Boolean.class));
 
     // Tag `tag1` is present
-    assertEquals(
+    assertEquals(Boolean.TRUE, evaluateExpression("matchAnyTag('tag1')"));
-        Boolean.TRUE,
+    assertNotEquals(Boolean.TRUE, evaluateExpression("!matchAnyTag('tag1')"));
-        expressionParser.parseExpression("matchAnyTag('tag1')").getValue(evaluationContext, Boolean.class));
-    assertNotEquals(
-        Boolean.TRUE,
-        expressionParser.parseExpression("!matchAnyTag('tag1')").getValue(evaluationContext, Boolean.class));
 
     // Tag `tag4` is not present
-    assertEquals(
+    assertEquals(Boolean.TRUE, evaluateExpression("matchAnyTag('tag4')"));
-        Boolean.TRUE,
+    assertNotEquals(Boolean.TRUE, evaluateExpression("!matchAnyTag('tag4')"));
-        expressionParser.parseExpression("matchAnyTag('tag4')").getValue(evaluationContext, Boolean.class));
+  }
-    assertNotEquals(
+
-        Boolean.TRUE,
+  private Boolean evaluateExpression(String condition) {
-        expressionParser.parseExpression("!matchAnyTag('tag4')").getValue(evaluationContext, Boolean.class));
+    return parseExpression(condition).getValue(evaluationContext, Boolean.class);
   }
 
   private List<TagLabel> getTags(String... tags) {

ingestion-core/src/metadata/_version.py

@@ -7,5 +7,5 @@ Provides metadata version information.
 
 from incremental import Version
 
-__version__ = Version("metadata", 0, 12, 0, dev=11)
+__version__ = Version("metadata", 0, 12, 0, dev=12)
 __all__ = ["__version__"]