From 3dde9f4ffbb6645d89ed6afadd4eed5f1fa36fc6 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Fri, 22 Dec 2023 11:58:55 -0800 Subject: [PATCH] Minor: Fix context evaluation to use the class as context (#14490) --- .../events/subscription/AlertUtil.java | 4 +- .../EventSubscriptionResource.java | 25 ++-------- .../service/security/JwtFilter.java | 6 ++- .../policyevaluator/CompiledRule.java | 7 +-- .../subscription/AlertsRuleEvaluatorTest.java | 46 ++++++++----------- .../glossary/GlossaryResourceTest.java | 6 +-- .../policyevaluator/RuleEvaluatorTest.java | 13 +++--- 7 files changed, 38 insertions(+), 69 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertUtil.java b/openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertUtil.java index e1faf1c191c..c83cb181833 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertUtil.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertUtil.java @@ -49,7 +49,6 @@ import org.openmetadata.service.resources.CollectionRegistry; import org.openmetadata.service.search.models.IndexMapping; import org.springframework.expression.Expression; -import org.springframework.expression.spel.support.StandardEvaluationContext; @Slf4j public final class AlertUtil { @@ -169,9 +168,8 @@ public static boolean evaluateAlertConditions(ChangeEvent changeEvent, List listEventSubscriptionFunctions( @Context UriInfo uriInfo, @Context SecurityContext securityContext) { + authorizer.authorizeAdmin(securityContext); return new ArrayList<>(AlertUtil.getAlertFilterFunctions().values()); } @@ -518,6 +497,7 @@ public List listEventSubscriptionFunctions( description = "Get list of EventSubscription functions used in filtering conditions in Event Subscription") public ResultList listEventSubResources( @Context UriInfo uriInfo, @Context SecurityContext securityContext) { + authorizer.authorizeAdmin(securityContext); return new ResultList<>(EventsSubscriptionRegistry.listResourceDescriptors()); } @@ -536,6 +516,7 @@ public void validateCondition( @Context SecurityContext securityContext, @Parameter(description = "Expression to validate", schema = @Schema(type = "string")) @PathParam("expression") String expression) { + authorizer.authorizeAdmin(securityContext); AlertUtil.validateExpression(expression, Boolean.class); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/JwtFilter.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/JwtFilter.java index 27606503c19..c53541a38b7 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/JwtFilter.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/JwtFilter.java @@ -65,7 +65,9 @@ public class JwtFilter implements ContainerRequestFilter { private AuthProvider providerType; public static final List EXCLUDED_ENDPOINTS = List.of( - "v1/system/config", + "v1/system/config/jwks", + "v1/system/config/authorizer", + "v1/system/config/customLogoConfiguration", "v1/users/signup", "v1/system/version", "v1/users/registrationConfirmation", @@ -110,7 +112,7 @@ public JwtFilter( @Override public void filter(ContainerRequestContext requestContext) { UriInfo uriInfo = requestContext.getUriInfo(); - if (EXCLUDED_ENDPOINTS.stream().anyMatch(endpoint -> uriInfo.getPath().contains(endpoint))) { + if (EXCLUDED_ENDPOINTS.stream().anyMatch(endpoint -> uriInfo.getPath().equalsIgnoreCase(endpoint))) { return; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java index 5fd336372fb..46280edfcae 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java @@ -18,7 +18,6 @@ import org.openmetadata.service.security.policyevaluator.SubjectContext.PolicyContext; import org.springframework.expression.Expression; import org.springframework.expression.spel.standard.SpelExpressionParser; -import org.springframework.expression.spel.support.StandardEvaluationContext; /** This class is used in a single threaded model and hence does not have concurrency support */ @Slf4j @@ -54,9 +53,8 @@ public static void validateExpression(String condition, Class clz) { } Expression expression = parseExpression(condition); RuleEvaluator ruleEvaluator = new RuleEvaluator(); - StandardEvaluationContext evaluationContext = new StandardEvaluationContext(ruleEvaluator); try { - expression.getValue(evaluationContext, clz); + 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 .*$", ""); @@ -207,8 +205,7 @@ private boolean matchExpression( return true; } RuleEvaluator ruleEvaluator = new RuleEvaluator(policyContext, subjectContext, resourceContext); - StandardEvaluationContext evaluationContext = new StandardEvaluationContext(ruleEvaluator); - return Boolean.TRUE.equals(expr.getValue(evaluationContext, Boolean.class)); + return Boolean.TRUE.equals(expr.getValue(ruleEvaluator, Boolean.class)); } public static boolean overrideAccess(Access newAccess, Access currentAccess) { diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluatorTest.java index 1d71d1db198..0d74e976cf9 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluatorTest.java @@ -27,8 +27,6 @@ import org.openmetadata.service.OpenMetadataApplicationTest; import org.openmetadata.service.resources.EntityResourceTest; import org.openmetadata.service.resources.databases.TableResourceTest; -import org.springframework.expression.EvaluationContext; -import org.springframework.expression.spel.support.StandardEvaluationContext; class AlertsRuleEvaluatorResourceTest extends OpenMetadataApplicationTest { private static TableResourceTest tableResourceTest; @@ -45,9 +43,8 @@ void test_matchAnySource() { ChangeEvent changeEvent = new ChangeEvent(); changeEvent.setEntityType("alert"); AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); - assertTrue(evaluateExpression("matchAnySource('alert')", evaluationContext)); - assertFalse(evaluateExpression("matchAnySource('bot')", evaluationContext)); + assertTrue(evaluateExpression("matchAnySource('alert')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchAnySource('bot')", alertsRuleEvaluator)); } @Test @@ -65,10 +62,9 @@ void test_matchAnyOwnerName(TestInfo test) throws IOException { // Test Owner Name Present in list and not present in list AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); assertTrue( - evaluateExpression("matchAnyOwnerName('" + EntityResourceTest.USER1.getName() + "')", evaluationContext)); - assertFalse(evaluateExpression("matchAnyOwnerName('tempName')", evaluationContext)); + evaluateExpression("matchAnyOwnerName('" + EntityResourceTest.USER1.getName() + "')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchAnyOwnerName('tempName')", alertsRuleEvaluator)); } // issue: https://github.com/open-metadata/OpenMetadata/issues/10376 @@ -85,10 +81,9 @@ void test_matchAnyEntityFqn(TestInfo test) throws IOException { // Test Entity Fqn in List of match and not present in list AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); String fqn = createdTable.getFullyQualifiedName(); - assertTrue(evaluateExpression("matchAnyEntityFqn('" + fqn + "')", evaluationContext)); - assertFalse(evaluateExpression("matchAnyEntityFqn('testFQN1')", evaluationContext)); + assertTrue(evaluateExpression("matchAnyEntityFqn('" + fqn + "')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchAnyEntityFqn('testFQN1')", alertsRuleEvaluator)); } @Test @@ -105,10 +100,9 @@ void test_matchAnyEntityId(TestInfo test) throws IOException { // Test Entity Id in List of match and not present in list AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); String id = createdTable.getId().toString(); - assertTrue(evaluateExpression("matchAnyEntityId('" + id + "')", evaluationContext)); - assertFalse(evaluateExpression("matchAnyEntityId('" + UUID.randomUUID() + "')", evaluationContext)); + assertTrue(evaluateExpression("matchAnyEntityId('" + id + "')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchAnyEntityId('" + UUID.randomUUID() + "')", alertsRuleEvaluator)); } @Test @@ -119,9 +113,8 @@ void test_matchAnyEventType() { // Check if eventType present in list or absent from the list AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); - assertTrue(evaluateExpression("matchAnyEventType('entityCreated')", evaluationContext)); - assertFalse(evaluateExpression("matchAnyEventType('entityUpdated')", evaluationContext)); + assertTrue(evaluateExpression("matchAnyEventType('entityCreated')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchAnyEventType('entityUpdated')", alertsRuleEvaluator)); } @Test @@ -142,9 +135,8 @@ void test_matchTestResult() { // Test If Test Result status matches in list and if status not matches AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); - assertTrue(evaluateExpression("matchTestResult('Success')", evaluationContext)); - assertFalse(evaluateExpression("matchTestResult('Failed')", evaluationContext)); + assertTrue(evaluateExpression("matchTestResult('Success')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchTestResult('Failed')", alertsRuleEvaluator)); } @Test @@ -155,9 +147,8 @@ void test_matchUpdatedBy() { // Test if the username is in list or not AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); - assertTrue(evaluateExpression("matchUpdatedBy('test')", evaluationContext)); - assertFalse(evaluateExpression("matchUpdatedBy('test1')", evaluationContext)); + assertTrue(evaluateExpression("matchUpdatedBy('test')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchUpdatedBy('test1')", alertsRuleEvaluator)); } @Test @@ -172,12 +163,11 @@ void test_matchAnyFieldChange() { // Test if the updated field matches from the list or not AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent); - EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator); - assertTrue(evaluateExpression("matchAnyFieldChange('test')", evaluationContext)); - assertFalse(evaluateExpression("matchAnyFieldChange('temp')", evaluationContext)); + assertTrue(evaluateExpression("matchAnyFieldChange('test')", alertsRuleEvaluator)); + assertFalse(evaluateExpression("matchAnyFieldChange('temp')", alertsRuleEvaluator)); } - private Boolean evaluateExpression(String condition, EvaluationContext evaluationContext) { - return parseExpression(condition).getValue(evaluationContext, Boolean.class); + private Boolean evaluateExpression(String condition, AlertsRuleEvaluator alertsRuleEvaluator) { + return parseExpression(condition).getValue(alertsRuleEvaluator, Boolean.class); } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java index f0bb505deb7..70425a1ca81 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java @@ -380,7 +380,7 @@ void testGlossaryImportExport() throws IOException { List createRecords = listOf( String.format( - ",g1,dsp1,\"dsc1,1\",h1;h2;h3,,term1;http://term1,Tier.Tier1,%s;%s,user;%s,%s", + ",g1,dsp1,\"dsc1,1\",h1;h2;h3,,term1;https://term1,Tier.Tier1,%s;%s,user;%s,%s", user1, user2, user1, "Approved"), String.format( ",g2,dsp2,dsc3,h1;h3;h3,,term2;https://term2,Tier.Tier2,%s,user;%s,%s", user1, user2, "Approved"), @@ -390,14 +390,14 @@ void testGlossaryImportExport() throws IOException { List updateRecords = listOf( String.format( - ",g1,dsp1,new-dsc1,h1;h2;h3,,term1;http://term1,Tier.Tier1,%s;%s,user;%s,%s", + ",g1,dsp1,new-dsc1,h1;h2;h3,,term1;https://term1,Tier.Tier1,%s;%s,user;%s,%s", user1, user2, user1, "Approved"), String.format( ",g2,dsp2,new-dsc3,h1;h3;h3,,term2;https://term2,Tier.Tier2,%s,user;%s,%s", user1, user2, "Approved"), String.format("importExportTest.g1,g11,dsp2,new-dsc11,h1;h3;h3,,,,%s,team;%s,%s", user1, team11, "Draft")); // Add new row to existing rows - List newRecords = listOf(",g3,dsp0,dsc0,h1;h2;h3,,term0;http://term0,Tier.Tier3,,,Approved"); + List newRecords = listOf(",g3,dsp0,dsc0,h1;h2;h3,,term0;https://term0,Tier.Tier3,,,Approved"); testImportExport(glossary.getName(), GlossaryCsv.HEADERS, createRecords, updateRecords, newRecords); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java index e73ea73f5ae..0c452bc9ccc 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java @@ -34,7 +34,7 @@ import org.openmetadata.service.jdbi3.TeamRepository; import org.openmetadata.service.security.policyevaluator.SubjectContext.PolicyContext; import org.springframework.expression.EvaluationContext; -import org.springframework.expression.spel.support.StandardEvaluationContext; +import org.springframework.expression.spel.support.SimpleEvaluationContext; class RuleEvaluatorTest { private static final Table table = new Table().withName("table"); @@ -43,6 +43,8 @@ class RuleEvaluatorTest { private static SubjectContext subjectContext; private static ResourceContext resourceContext; + private static RuleEvaluator ruleEvaluator; + @BeforeAll public static void setup() { TeamRepository teamRepository = mock(TeamRepository.class); @@ -73,8 +75,8 @@ public static void setup() { user = new User().withId(UUID.randomUUID()).withName("user"); resourceContext = new ResourceContext("table", table, mock(TableRepository.class)); subjectContext = new SubjectContext(user); - RuleEvaluator ruleEvaluator = new RuleEvaluator(null, subjectContext, resourceContext); - evaluationContext = new StandardEvaluationContext(ruleEvaluator); + ruleEvaluator = new RuleEvaluator(null, subjectContext, resourceContext); + evaluationContext = SimpleEvaluationContext.forReadOnlyDataBinding().withRootObject(ruleEvaluator).build(); } @Test @@ -249,7 +251,7 @@ void test_hasAnyRole() { } private Boolean evaluateExpression(String condition) { - return parseExpression(condition).getValue(evaluationContext, Boolean.class); + return parseExpression(condition).getValue(ruleEvaluator, Boolean.class); } private List getTags(String... tags) { @@ -294,7 +296,6 @@ private Role createRole(String roleName) { private void updatePolicyContext(String team) { PolicyContext policyContext = new PolicyContext(Entity.TEAM, team, null, null, null); - RuleEvaluator ruleEvaluator = new RuleEvaluator(policyContext, subjectContext, resourceContext); - evaluationContext = new StandardEvaluationContext(ruleEvaluator); + ruleEvaluator = new RuleEvaluator(policyContext, subjectContext, resourceContext); } }