From 9e4be2a48b6b99bc4bf0a1ad5c33a95a3d988256 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Sat, 13 Aug 2022 13:10:00 -0700 Subject: [PATCH] Fixes #6719 Validate rule conditions and return meaningful errors in API response (#6720) --- .../exception/CatalogExceptionMessage.java | 8 ++ .../catalog/jdbi3/PolicyRepository.java | 17 ++- .../policyevaluator/CompiledRule.java | 30 ++++- .../policyevaluator/RuleEvaluator.java | 10 +- .../policies/PolicyResourceTest.java | 56 +++++++++ .../policyevaluator/RuleEvaluatorTest.java | 107 +++++------------- ingestion-core/src/metadata/_version.py | 2 +- 7 files changed, 145 insertions(+), 85 deletions(-) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java index 00e04d4f99e..11285297f5b 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java +++ b/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); + } } 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 f63aa8c5067..a0986c869ab 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 @@ -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 { /* Get all the teams that use this policy */ private List getTeams(Policy policy) throws IOException { List records = findFrom(policy.getId(), POLICY, Relationship.HAS, Entity.TEAM); - List teams = EntityUtil.populateEntityReferences(records, Entity.TEAM); - return teams; + return EntityUtil.populateEntityReferences(records, Entity.TEAM); } /* Get all the roles that use this policy */ private List getRoles(Policy policy) throws IOException { List records = findFrom(policy.getId(), POLICY, Relationship.HAS, Entity.ROLE); - List roles = EntityUtil.populateEntityReferences(records, Entity.ROLE); - return roles; + return EntityUtil.populateEntityReferences(records, Entity.ROLE); } /** Generate EntityReference for a given Policy's Location. * */ @@ -137,7 +137,14 @@ public class PolicyRepository extends EntityRepository { 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 rules = EntityUtil.resolveRules(policy.getRules()); + + // Validate all the expressions in the rule + for (Rule rule : rules) { + CompiledRule.validateExpression(rule.getCondition(), Boolean.class); + } } public List getAccessControlPolicies() throws IOException { diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/CompiledRule.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/CompiledRule.java index 9b7b2868bb5..caac572fe65 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/CompiledRule.java +++ b/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 validateExpression(String condition, Class 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; } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluator.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluator.java index 46f17bd5607..0868f83455e 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluator.java +++ b/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 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 tagFQNs) throws IOException { + if (resourceContext == null) { + return false; + } List tags = resourceContext.getTags(); for (String tagFQN : tagFQNs) { TagLabel found = tags.stream().filter(t -> t.getTagFQN().equals(tagFQN)).findAny().orElse(null); 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 906af70ad6c..2fde89f08bf 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 @@ -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 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); diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluatorTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluatorTest.java index 5cecbf1bdde..5e970eba5c2 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/RuleEvaluatorTest.java +++ b/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 SubjectContext subjectContext; + private static EvaluationContext evaluationContext; @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)); - assertFalse(expressionParser.parseExpression("!noOwner()").getValue(evaluationContext, Boolean.class)); + assertEquals(Boolean.TRUE, evaluateExpression("noOwner()")); + 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( - Boolean.TRUE, expressionParser.parseExpression("noOwner()").getValue(evaluationContext, Boolean.class)); - assertEquals( - Boolean.TRUE, expressionParser.parseExpression("!noOwner()").getValue(evaluationContext, Boolean.class)); + assertNotEquals(Boolean.TRUE, evaluateExpression("noOwner()")); + assertEquals(Boolean.TRUE, evaluateExpression("!noOwner()")); } @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( - Boolean.TRUE, expressionParser.parseExpression("isOwner()").getValue(evaluationContext, Boolean.class)); - assertEquals( - Boolean.TRUE, expressionParser.parseExpression("!isOwner()").getValue(evaluationContext, Boolean.class)); + assertNotEquals(Boolean.TRUE, evaluateExpression("isOwner()")); + assertEquals(Boolean.TRUE, evaluateExpression("!isOwner()")); // 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( - Boolean.TRUE, expressionParser.parseExpression("isOwner()").getValue(evaluationContext, Boolean.class)); - assertNotEquals( - Boolean.TRUE, expressionParser.parseExpression("!isOwner()").getValue(evaluationContext, Boolean.class)); + assertEquals(Boolean.TRUE, evaluateExpression("isOwner()")); + assertNotEquals(Boolean.TRUE, evaluateExpression("!isOwner()")); // noOwner() || isOwner() - with noOwner being true and isOwner false table.setOwner(null); - assertEquals( - Boolean.TRUE, - expressionParser.parseExpression("noOwner() || isOwner()").getValue(evaluationContext, Boolean.class)); - assertNotEquals( - Boolean.TRUE, - expressionParser.parseExpression("!noOwner() && !isOwner()").getValue(evaluationContext, Boolean.class)); + assertEquals(Boolean.TRUE, evaluateExpression("noOwner() || isOwner()")); + assertNotEquals(Boolean.TRUE, evaluateExpression("!noOwner() && !isOwner()")); // noOwner() || isOwner() - with noOwner is false and isOwner true table.setOwner(new EntityReference().withId(user.getId()).withType(Entity.USER).withName(user.getName())); - assertEquals( - Boolean.TRUE, - expressionParser.parseExpression("noOwner() || isOwner()").getValue(evaluationContext, Boolean.class)); - assertNotEquals( - Boolean.TRUE, - expressionParser.parseExpression("!noOwner() && !isOwner()").getValue(evaluationContext, Boolean.class)); + assertEquals(Boolean.TRUE, evaluateExpression("noOwner() || isOwner()")); + assertNotEquals(Boolean.TRUE, evaluateExpression("!noOwner() && !isOwner()")); } @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( - Boolean.TRUE, - expressionParser - .parseExpression("matchAllTags('tag1', 'tag2', 'tag3')") - .getValue(evaluationContext, Boolean.class)); - assertNotEquals( - Boolean.TRUE, - expressionParser - .parseExpression("!matchAllTags('tag1', 'tag2', 'tag3')") - .getValue(evaluationContext, Boolean.class)); + assertEquals(Boolean.TRUE, evaluateExpression("matchAllTags('tag1', 'tag2', 'tag3')")); + assertNotEquals(Boolean.TRUE, evaluateExpression("!matchAllTags('tag1', 'tag2', 'tag3')")); // One tag `tag4` is missing table.withTags(getTags("tag1", "tag2", "tag4")); - assertNotEquals( - Boolean.TRUE, - expressionParser - .parseExpression("matchAllTags('tag1', 'tag2', 'tag3')") - .getValue(evaluationContext, Boolean.class)); - assertEquals( - Boolean.TRUE, - expressionParser - .parseExpression("!matchAllTags('tag1', 'tag2', 'tag3')") - .getValue(evaluationContext, Boolean.class)); + assertNotEquals(Boolean.TRUE, evaluateExpression("matchAllTags('tag1', 'tag2', 'tag3')")); + assertEquals(Boolean.TRUE, evaluateExpression("!matchAllTags('tag1', 'tag2', 'tag3')")); // Tag `tag1` is present - assertEquals( - Boolean.TRUE, - expressionParser.parseExpression("matchAnyTag('tag1')").getValue(evaluationContext, Boolean.class)); - assertNotEquals( - Boolean.TRUE, - expressionParser.parseExpression("!matchAnyTag('tag1')").getValue(evaluationContext, Boolean.class)); + assertEquals(Boolean.TRUE, evaluateExpression("matchAnyTag('tag1')")); + assertNotEquals(Boolean.TRUE, evaluateExpression("!matchAnyTag('tag1')")); // Tag `tag4` is not present - assertEquals( - Boolean.TRUE, - expressionParser.parseExpression("matchAnyTag('tag4')").getValue(evaluationContext, Boolean.class)); - assertNotEquals( - Boolean.TRUE, - expressionParser.parseExpression("!matchAnyTag('tag4')").getValue(evaluationContext, Boolean.class)); + assertEquals(Boolean.TRUE, evaluateExpression("matchAnyTag('tag4')")); + assertNotEquals(Boolean.TRUE, evaluateExpression("!matchAnyTag('tag4')")); + } + + private Boolean evaluateExpression(String condition) { + return parseExpression(condition).getValue(evaluationContext, Boolean.class); } private List getTags(String... tags) { diff --git a/ingestion-core/src/metadata/_version.py b/ingestion-core/src/metadata/_version.py index 7965b611a9b..37ebf67ffb4 100644 --- a/ingestion-core/src/metadata/_version.py +++ b/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__"]