Fix Search RBAC to consider only ViewAll, ViewBasic (#17865)

* Fix Search RBAC to consider only ViewAll, ViewBasic

* Fix Search RBAC to consider only ViewAll, ViewBasic

* Fix Search RBAC to consider only ViewAll, ViewBasic

openmetadata-service/src/main/java/org/openmetadata/service/search/elasticsearch/ElasticSearchClient.java

@@ -354,6 +354,8 @@ public Response search(SearchRequest request, SubjectContext subjectContext) thr
       }
     }
 
+    buildSearchRBACQuery(subjectContext, searchSourceBuilder);
+
     // Add Filter
     if (!nullOrEmpty(request.getQueryFilter()) && !request.getQueryFilter().equals("{}")) {
       try {
@@ -526,7 +528,6 @@ public Response search(SearchRequest request, SubjectContext subjectContext) thr
     }
 
     searchSourceBuilder.timeout(new TimeValue(30, TimeUnit.SECONDS));
-    buildSearchRBACQuery(subjectContext, searchSourceBuilder);
 
     try {
 
@@ -2311,8 +2312,8 @@ public Object getLowLevelClient() {
   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())

openmetadata-service/src/main/java/org/openmetadata/service/search/opensearch/OpenSearchClient.java

@@ -348,6 +348,8 @@ public Response search(SearchRequest request, SubjectContext subjectContext) thr
       }
     }
 
+    buildSearchRBACQuery(subjectContext, searchSourceBuilder);
+
     // Add Query Filter
     if (!nullOrEmpty(request.getQueryFilter()) && !request.getQueryFilter().equals("{}")) {
       try {
@@ -520,7 +522,7 @@ public Response search(SearchRequest request, SubjectContext subjectContext) thr
     } else {
       searchSourceBuilder.trackTotalHitsUpTo(MAX_RESULT_HITS);
     }
-    buildSearchRBACQuery(subjectContext, searchSourceBuilder);
+
     searchSourceBuilder.timeout(new TimeValue(30, TimeUnit.SECONDS));
     try {
       SearchResponse searchResponse =
@@ -2261,8 +2263,8 @@ public Object getLowLevelClient() {
   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())

openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java

@@ -4,6 +4,7 @@
 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<MetadataOperation> SEARCH_RELEVANT_OPS =
+      Set.of(MetadataOperation.VIEW_BASIC, MetadataOperation.VIEW_ALL);
 
   public RBACConditionEvaluator(QueryBuilderFactory queryBuilderFactory) {
     this.queryBuilderFactory = queryBuilderFactory;
@@ -40,7 +43,8 @@ public OMQueryBuilder evaluateConditions(SubjectContext subjectContext) {
         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());

openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java

@@ -2,6 +2,7 @@
 
 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 @@ public void tearDown() {
   }
 
   private void setupMockPolicies(
-      List<String> expressions, String effect, List<List<String>> resourcesList) {
+      List<String> expressions,
+      String effect,
+      List<List<String>> resourcesList,
+      List<List<MetadataOperation>> operationsList) {
     // Mock the user
     mockUser = mock(User.class);
     EntityReference mockUserReference = mock(EntityReference.class);
@@ -86,6 +90,10 @@ private void setupMockPolicies(
       List<String> resources = (resourcesList.size() > i) ? resourcesList.get(i) : List.of("All");
       when(mockRule.getResources()).thenReturn(resources);
 
+      List<MetadataOperation> 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 @@ private void setupMockPolicies(
   }
 
   private void setupMockPolicies(String expression, String effect, List<String> 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 @@ void testHasDomain() {
   }
 
   @Test
-  void testComplexConditionWithRolesDomainTagsTeams() throws IOException {
+  void testComplexConditionWithRolesDomainTagsTeams() {
     setupMockPolicies(
         "hasAnyRole('Admin', 'DataSteward') && hasDomain() && (matchAnyTag('Sensitive') || inAnyTeam('Interns'))",
         "ALLOW");
@@ -294,7 +306,7 @@ void testComplexConditionWithRolesDomainTagsTeams() throws IOException {
   }
 
   @Test
-  void testConditionUserDoesNotHaveRole() throws IOException {
+  void testConditionUserDoesNotHaveRole() {
     setupMockPolicies("hasAnyRole('Admin', 'DataSteward') && isOwner()", "ALLOW");
     EntityReference role = new EntityReference();
     role.setName("DataConsumer");
@@ -312,7 +324,7 @@ void testConditionUserDoesNotHaveRole() throws IOException {
   }
 
   @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 @@ void testNegationWithRolesAndTeams() throws IOException {
   }
 
   @Test
-  void testComplexConditionUserMeetsAllCriteria() throws IOException {
+  void testComplexConditionUserMeetsAllCriteria() {
     setupMockPolicies(
         "hasDomain() && inAnyTeam('Engineering', 'Analytics') && matchAllTags('Confidential', 'Internal')",
         "ALLOW");
@@ -363,7 +375,7 @@ void testComplexConditionUserMeetsAllCriteria() throws IOException {
   }
 
   @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 @@ void testConditionUserLacksDomain() throws IOException {
   }
 
   @Test
-  void testNestedLogicalOperators() throws IOException {
+  void testNestedLogicalOperators() {
     setupMockPolicies(
         "(hasAnyRole('Admin') || inAnyTeam('Engineering')) && (matchAnyTag('Sensitive') || isOwner())",
         "ALLOW");
@@ -428,7 +440,7 @@ private void countBoolQueries(JsonNode node, AtomicInteger count) {
   }
 
   @Test
-  void testComplexNestedConditions() throws IOException {
+  void testComplexNestedConditions() {
     setupMockPolicies(
         "(hasAnyRole('Admin') || (matchAnyTag('Confidential') && hasDomain())) && (!inAnyTeam('HR') || noOwner())",
         "ALLOW");
@@ -471,7 +483,7 @@ void testComplexNestedConditions() throws IOException {
   }
 
   @Test
-  void testMultipleNestedNotOperators() throws IOException {
+  void testMultipleNestedNotOperators() {
     setupMockPolicies(
         "!(matchAllTags('Sensitive', 'Internal') && (!hasDomain()) || isOwner())", "ALLOW");
 
@@ -498,7 +510,7 @@ void testMultipleNestedNotOperators() throws IOException {
   }
 
   @Test
-  void testComplexOrConditionsWithNegations() throws IOException {
+  void testComplexOrConditionsWithNegations() {
     setupMockPolicies(
         "(hasAnyRole('Analyst') && matchAnyTag('Public')) || (!hasAnyRole('Viewer') && !inAnyTeam('Finance'))",
         "ALLOW");
@@ -528,7 +540,7 @@ void testComplexOrConditionsWithNegations() throws IOException {
   }
 
   @Test
-  void testNestedAndOrWithMultipleMethods() throws IOException {
+  void testNestedAndOrWithMultipleMethods() {
     setupMockPolicies(
         "(hasDomain() || matchAnyTag('External')) && (inAnyTeam('Engineering', 'Analytics') || hasAnyRole('DataEngineer')) && !noOwner()",
         "ALLOW");
@@ -568,7 +580,7 @@ void testNestedAndOrWithMultipleMethods() throws IOException {
   }
 
   @Test
-  void testComplexConditionWithAllMethods() throws IOException {
+  void testComplexConditionWithAllMethods() {
     setupMockPolicies(
         "(hasAnyRole('Admin') && hasDomain() && matchAllTags('PII', 'Sensitive')) || (isOwner() && !matchAnyTag('Restricted'))",
         "ALLOW");
@@ -626,7 +638,7 @@ void testComplexConditionWithAllMethods() throws IOException {
   }
 
   @Test
-  void testMultipleOrConditionsWithNestedAnd() throws IOException {
+  void testMultipleOrConditionsWithNestedAnd() {
     setupMockPolicies(
         "(hasAnyRole('Admin') || hasAnyRole('DataSteward')) && (matchAnyTag('Finance') || matchAllTags('Confidential', 'Internal')) && !inAnyTeam('Data')",
         "ALLOW");
@@ -675,7 +687,7 @@ void testMultipleOrConditionsWithNestedAnd() throws IOException {
   }
 
   @Test
-  void testComplexConditionWithMultipleNegations() throws IOException {
+  void testComplexConditionWithMultipleNegations() {
     setupMockPolicies(
         "!((hasAnyRole('Admin') || inAnyTeam('Engineering')) && matchAnyTag('Confidential') && matchAllTags('Sensitive', 'Classified')) && hasDomain() && isOwner()",
         "ALLOW");
@@ -732,7 +744,7 @@ void testComplexConditionWithMultipleNegations() throws IOException {
   }
 
   @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 @@ void testNotHasDomainWhenUserHasNoDomain() throws IOException {
   }
 
   @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 @@ void testIndexFilteringBasedOnResource() throws IOException {
   }
 
   @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 @@ void testComplexConditionWithIndexFiltering() throws IOException {
   }
 
   @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 @@ void testMultipleRulesInPolicy() throws IOException {
   }
 
   @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 @@ void testMultiplePoliciesInRole() throws IOException {
   }
 
   @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 @@ void testRoleAndPolicyInheritanceFromTeams() throws IOException {
         "$.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");
+  }
 }