diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java index 463a93280d8..22a8399a6b1 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java @@ -354,6 +354,8 @@ public class ElasticSearchClient implements SearchClient { } } + buildSearchRBACQuery(subjectContext, searchSourceBuilder); + // Add Filter if (!nullOrEmpty(request.getQueryFilter()) && !request.getQueryFilter().equals("{}")) { try { @@ -526,7 +528,6 @@ public class ElasticSearchClient implements SearchClient { } searchSourceBuilder.timeout(new TimeValue(30, TimeUnit.SECONDS)); - buildSearchRBACQuery(subjectContext, searchSourceBuilder); try { @@ -2311,8 +2312,8 @@ public class ElasticSearchClient implements SearchClient { private void buildSearchRBACQuery( SubjectContext subjectContext, SearchSourceBuilder searchSourceBuilder) { if (subjectContext != null && !subjectContext.isAdmin()) { - if (rbacConditionEvaluator != null) { - OMQueryBuilder rbacQuery = rbacConditionEvaluator.evaluateConditions(subjectContext); + OMQueryBuilder rbacQuery = rbacConditionEvaluator.evaluateConditions(subjectContext); + if (rbacQuery != null) { searchSourceBuilder.query( QueryBuilders.boolQuery() .must(searchSourceBuilder.query()) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchClient.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchClient.java index 034f0f29e4d..cfd64096999 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchClient.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchClient.java @@ -348,6 +348,8 @@ public class OpenSearchClient implements SearchClient { } } + buildSearchRBACQuery(subjectContext, searchSourceBuilder); + // Add Query Filter if (!nullOrEmpty(request.getQueryFilter()) && !request.getQueryFilter().equals("{}")) { try { @@ -520,7 +522,7 @@ public class OpenSearchClient implements SearchClient { } else { searchSourceBuilder.trackTotalHitsUpTo(MAX_RESULT_HITS); } - buildSearchRBACQuery(subjectContext, searchSourceBuilder); + searchSourceBuilder.timeout(new TimeValue(30, TimeUnit.SECONDS)); try { SearchResponse searchResponse = @@ -2261,8 +2263,8 @@ public class OpenSearchClient implements SearchClient { private void buildSearchRBACQuery( SubjectContext subjectContext, SearchSourceBuilder searchSourceBuilder) { if (subjectContext != null && !subjectContext.isAdmin()) { - if (rbacConditionEvaluator != null) { - OMQueryBuilder rbacQuery = rbacConditionEvaluator.evaluateConditions(subjectContext); + OMQueryBuilder rbacQuery = rbacConditionEvaluator.evaluateConditions(subjectContext); + if (rbacQuery != null) { searchSourceBuilder.query( QueryBuilders.boolQuery() .must(searchSourceBuilder.query()) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java index 28b3445d52b..6bcb33bc874 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java @@ -4,6 +4,7 @@ import java.util.*; import java.util.stream.Collectors; import org.openmetadata.schema.entity.teams.User; import org.openmetadata.schema.type.EntityReference; +import org.openmetadata.schema.type.MetadataOperation; import org.openmetadata.service.Entity; import org.openmetadata.service.search.queries.OMQueryBuilder; import org.openmetadata.service.search.queries.QueryBuilderFactory; @@ -24,6 +25,8 @@ public class RBACConditionEvaluator { private final QueryBuilderFactory queryBuilderFactory; private final ExpressionParser spelParser = new SpelExpressionParser(); private final StandardEvaluationContext spelContext; + private static final Set SEARCH_RELEVANT_OPS = + Set.of(MetadataOperation.VIEW_BASIC, MetadataOperation.VIEW_ALL); public RBACConditionEvaluator(QueryBuilderFactory queryBuilderFactory) { this.queryBuilderFactory = queryBuilderFactory; @@ -40,7 +43,8 @@ public class RBACConditionEvaluator { it.hasNext(); ) { SubjectContext.PolicyContext context = it.next(); for (CompiledRule rule : context.getRules()) { - if (rule.getCondition() != null) { + if (rule.getCondition() != null + && rule.getOperations().stream().anyMatch(SEARCH_RELEVANT_OPS::contains)) { ConditionCollector ruleCollector = new ConditionCollector(queryBuilderFactory); if (!rule.getResources().isEmpty() && !rule.getResources().contains("All")) { OMQueryBuilder indexFilter = getIndexFilter(rule.getResources()); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java index 08424f5f654..30da785f07d 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java @@ -2,6 +2,7 @@ package org.openmetadata.service.search.security; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; @@ -62,7 +63,10 @@ class ElasticSearchRBACConditionEvaluatorTest { } private void setupMockPolicies( - List expressions, String effect, List> resourcesList) { + List expressions, + String effect, + List> resourcesList, + List> operationsList) { // Mock the user mockUser = mock(User.class); EntityReference mockUserReference = mock(EntityReference.class); @@ -86,6 +90,10 @@ class ElasticSearchRBACConditionEvaluatorTest { List resources = (resourcesList.size() > i) ? resourcesList.get(i) : List.of("All"); when(mockRule.getResources()).thenReturn(resources); + List operations = + (operationsList.size() > i) ? operationsList.get(i) : List.of(MetadataOperation.VIEW_ALL); + when(mockRule.getOperations()).thenReturn(operations); + CompiledRule.Effect mockEffect = CompiledRule.Effect.valueOf(effect.toUpperCase()); when(mockRule.getEffect()).thenReturn(mockEffect); mockRules.add(mockRule); @@ -100,7 +108,11 @@ class ElasticSearchRBACConditionEvaluatorTest { } private void setupMockPolicies(String expression, String effect, List resources) { - setupMockPolicies(List.of(expression), effect, List.of(resources)); + setupMockPolicies( + List.of(expression), + effect, + List.of(resources), + List.of(List.of(MetadataOperation.VIEW_ALL))); } private void setupMockPolicies(String expression, String effect) { @@ -247,7 +259,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testComplexConditionWithRolesDomainTagsTeams() throws IOException { + void testComplexConditionWithRolesDomainTagsTeams() { setupMockPolicies( "hasAnyRole('Admin', 'DataSteward') && hasDomain() && (matchAnyTag('Sensitive') || inAnyTeam('Interns'))", "ALLOW"); @@ -294,7 +306,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testConditionUserDoesNotHaveRole() throws IOException { + void testConditionUserDoesNotHaveRole() { setupMockPolicies("hasAnyRole('Admin', 'DataSteward') && isOwner()", "ALLOW"); EntityReference role = new EntityReference(); role.setName("DataConsumer"); @@ -312,7 +324,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testNegationWithRolesAndTeams() throws IOException { + void testNegationWithRolesAndTeams() { setupMockPolicies("!(hasAnyRole('Viewer') || inAnyTeam('Marketing')) && isOwner()", "ALLOW"); EntityReference role = new EntityReference(); role.setName("Viewer"); @@ -332,7 +344,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testComplexConditionUserMeetsAllCriteria() throws IOException { + void testComplexConditionUserMeetsAllCriteria() { setupMockPolicies( "hasDomain() && inAnyTeam('Engineering', 'Analytics') && matchAllTags('Confidential', 'Internal')", "ALLOW"); @@ -363,7 +375,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testConditionUserLacksDomain() throws IOException { + void testConditionUserLacksDomain() { setupMockPolicies("hasDomain() && isOwner() && matchAnyTag('Public')", "ALLOW"); when(mockUser.getDomain()).thenReturn(null); OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); @@ -383,7 +395,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testNestedLogicalOperators() throws IOException { + void testNestedLogicalOperators() { setupMockPolicies( "(hasAnyRole('Admin') || inAnyTeam('Engineering')) && (matchAnyTag('Sensitive') || isOwner())", "ALLOW"); @@ -428,7 +440,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testComplexNestedConditions() throws IOException { + void testComplexNestedConditions() { setupMockPolicies( "(hasAnyRole('Admin') || (matchAnyTag('Confidential') && hasDomain())) && (!inAnyTeam('HR') || noOwner())", "ALLOW"); @@ -471,7 +483,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testMultipleNestedNotOperators() throws IOException { + void testMultipleNestedNotOperators() { setupMockPolicies( "!(matchAllTags('Sensitive', 'Internal') && (!hasDomain()) || isOwner())", "ALLOW"); @@ -498,7 +510,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testComplexOrConditionsWithNegations() throws IOException { + void testComplexOrConditionsWithNegations() { setupMockPolicies( "(hasAnyRole('Analyst') && matchAnyTag('Public')) || (!hasAnyRole('Viewer') && !inAnyTeam('Finance'))", "ALLOW"); @@ -528,7 +540,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testNestedAndOrWithMultipleMethods() throws IOException { + void testNestedAndOrWithMultipleMethods() { setupMockPolicies( "(hasDomain() || matchAnyTag('External')) && (inAnyTeam('Engineering', 'Analytics') || hasAnyRole('DataEngineer')) && !noOwner()", "ALLOW"); @@ -568,7 +580,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testComplexConditionWithAllMethods() throws IOException { + void testComplexConditionWithAllMethods() { setupMockPolicies( "(hasAnyRole('Admin') && hasDomain() && matchAllTags('PII', 'Sensitive')) || (isOwner() && !matchAnyTag('Restricted'))", "ALLOW"); @@ -626,7 +638,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testMultipleOrConditionsWithNestedAnd() throws IOException { + void testMultipleOrConditionsWithNestedAnd() { setupMockPolicies( "(hasAnyRole('Admin') || hasAnyRole('DataSteward')) && (matchAnyTag('Finance') || matchAllTags('Confidential', 'Internal')) && !inAnyTeam('Data')", "ALLOW"); @@ -675,7 +687,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testComplexConditionWithMultipleNegations() throws IOException { + void testComplexConditionWithMultipleNegations() { setupMockPolicies( "!((hasAnyRole('Admin') || inAnyTeam('Engineering')) && matchAnyTag('Confidential') && matchAllTags('Sensitive', 'Classified')) && hasDomain() && isOwner()", "ALLOW"); @@ -732,7 +744,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testNotHasDomainWhenUserHasNoDomain() throws IOException { + void testNotHasDomainWhenUserHasNoDomain() { setupMockPolicies("!hasDomain() && isOwner()", "ALLOW"); when(mockUser.getDomain()).thenReturn(null); when(mockUser.getId()).thenReturn(UUID.randomUUID()); @@ -755,7 +767,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testIndexFilteringBasedOnResource() throws IOException { + void testIndexFilteringBasedOnResource() { // Assume the rule applies to 'Table' resource setupMockPolicies("hasAnyRole('Admin') && matchAnyTag('Sensitive')", "ALLOW", List.of("Table")); @@ -782,7 +794,7 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testComplexConditionWithIndexFiltering() throws IOException { + void testComplexConditionWithIndexFiltering() { // Assume the rule applies to 'Database' and 'Table' resources setupMockPolicies( "hasDomain() && matchAnyTag('Sensitive')", "ALLOW", List.of("Database", "Table")); @@ -818,14 +830,15 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testMultipleRulesInPolicy() throws IOException { + void testMultipleRulesInPolicy() { // Set up policies with multiple rules setupMockPolicies( List.of( "hasAnyRole('Admin') && matchAnyTag('Sensitive')", "inAnyTeam('Engineering') && matchAllTags('Confidential', 'Internal')"), "ALLOW", - List.of(List.of("Table"), List.of("Dashboard"))); + List.of(List.of("Table"), List.of("Dashboard")), + List.of(List.of(MetadataOperation.VIEW_BASIC))); // Mock user roles and teams EntityReference role = new EntityReference(); @@ -879,12 +892,13 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testMultiplePoliciesInRole() throws IOException { + void testMultiplePoliciesInRole() { // Mock multiple policies in a single role setupMockPolicies( List.of("hasDomain() && matchAnyTag('Public')", "!inAnyTeam('HR') || isOwner()"), "ALLOW", - List.of(List.of("Table"), List.of("Dashboard"))); + List.of(List.of("Table"), List.of("Dashboard")), + List.of(List.of(MetadataOperation.VIEW_BASIC))); // Mock user roles EntityReference role = new EntityReference(); @@ -928,14 +942,15 @@ class ElasticSearchRBACConditionEvaluatorTest { } @Test - void testRoleAndPolicyInheritanceFromTeams() throws IOException { + void testRoleAndPolicyInheritanceFromTeams() { // Mock policies inherited through team hierarchy setupMockPolicies( List.of( "hasAnyRole('Manager') && hasDomain()", "inAnyTeam('Engineering') && matchAnyTag('Critical')"), "ALLOW", - List.of(List.of("All"), List.of("All"))); + List.of(List.of("All"), List.of("All")), + List.of(List.of(MetadataOperation.VIEW_BASIC))); // Mock user teams with inherited roles EntityReference team = new EntityReference(); @@ -981,4 +996,45 @@ class ElasticSearchRBACConditionEvaluatorTest { "$.bool.must[1].bool.should[?(@.term['tags.tagFQN'].value=='Critical')]", "Critical tag"); } + + @Test + void testRuleWithNonViewOperationIgnored() { + // Rule with operation EditDescription, which should be ignored in search RBAC + setupMockPolicies( + List.of("isOwner()"), + "ALLOW", + List.of(List.of("All")), + List.of(List.of(MetadataOperation.EDIT_DESCRIPTION))); + + // Mock user ownership + when(mockUser.getId()).thenReturn(UUID.randomUUID()); + + // Evaluate the condition + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + assertNull(finalQuery, "The query should be null since the rule is ignored"); + } + + @Test + void testRuleWithViewBasicOperationApplied() { + // Rule with operation ViewBasic, which should affect search results + setupMockPolicies( + List.of("isOwner()"), + "ALLOW", + List.of(List.of("All")), + List.of(List.of(MetadataOperation.VIEW_BASIC))); + + // Mock user ownership + UUID userId = UUID.randomUUID(); + when(mockUser.getId()).thenReturn(userId); + + // Evaluate the condition + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + + // The rule should affect the search query + assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'"); + assertTrue( + generatedQuery.contains(userId.toString()), "The query should contain the user's ID"); + } }