Adapted TermsAggregationEvaluator to use ActionPrivileges

Signed-off-by: Nils Bandener <[email protected]>
nibix · May 31, 2024 · 8c89263 · 8c89263
1 parent 63e1648
commit 8c89263
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 22 deletions.
diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -348,7 +348,7 @@ public PrivilegesEvaluatorResponse evaluate(
                     "No cluster-level perm match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}",
                     user,
                     action0,
-                    securityRoles.getRoleNames(),
+                    mappedRoles,
                     presponse.getMissingPrivileges()
                 );
             }
@@ -527,7 +527,7 @@ public PrivilegesEvaluatorResponse evaluate(
         }
 
         // term aggregations
-        if (termsAggregationEvaluator.evaluate(requestedResolved, request, clusterService, user, securityRoles, resolver, presponse)
+        if (termsAggregationEvaluator.evaluate(requestedResolved, request, clusterService, context, actionPrivileges, resolver, presponse)
             .isComplete()) {
             return presponse;
         }
@@ -541,7 +541,7 @@ public PrivilegesEvaluatorResponse evaluate(
 
         if (isDebugEnabled) {
             log.debug("Requested resolved index types: {}", requestedResolved);
-            log.debug("Security roles: {}", securityRoles.getRoleNames());
+            log.debug("Security roles: {}", mappedRoles);
         }
 
         // TODO exclude Security index
@@ -614,7 +614,7 @@ public PrivilegesEvaluatorResponse evaluate(
                 user,
                 requestedResolved,
                 action0,
-                securityRoles.getRoleNames()
+                mappedRoles
             );
             log.info("Index to privilege matrix:\n{}", presponse.getCheckTable());
         }

diff --git a/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java b/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java
@@ -26,8 +26,7 @@
 
 package org.opensearch.security.privileges;
 
-import java.util.Set;
-
+import com.google.common.collect.ImmutableSet;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 
@@ -41,21 +40,24 @@
 import org.opensearch.search.aggregations.AggregationBuilder;
 import org.opensearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
 import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
-import org.opensearch.security.securityconf.SecurityRoles;
-import org.opensearch.security.user.User;
 
 public class TermsAggregationEvaluator {
 
     protected final Logger log = LogManager.getLogger(this.getClass());
 
-    private static final String[] READ_ACTIONS = new String[] {
+    private static final ImmutableSet<String> READ_ACTIONS = ImmutableSet.of(
         "indices:data/read/msearch",
         "indices:data/read/mget",
         "indices:data/read/get",
         "indices:data/read/search",
-        "indices:data/read/field_caps*"
-        // "indices:admin/mappings/fields/get*"
-    };
+        "indices:data/read/field_caps*"  // TODO this string likely does not what it is expected to do.
+        // SecurityRoles.getAllPermittedIndicesForDashboards() does not support patterns for the actions parameter -
+        // that is, the actions for which the privileges should be checked.
+        // In order to fulfill the requirements for the test to return true, the user needs to have a pattern
+        // permission either for "*", "indices:data/read/*" or "indices:data/read/field_caps*". Just
+        // "indices:data/read/field_caps" is however not sufficient, as this won't match the star (which will
+        // be just treated like a regular character). Thus, I am tending to just remove the star here.
+    );
 
     private static final QueryBuilder NONE_QUERY = new MatchNoneQueryBuilder();
 
@@ -65,8 +67,8 @@ public PrivilegesEvaluatorResponse evaluate(
         final Resolved resolved,
         final ActionRequest request,
         ClusterService clusterService,
-        User user,
-        SecurityRoles securityRoles,
+        PrivilegesEvaluationContext context,
+        ActionPrivileges actionPrivileges,
         IndexNameExpressionResolver resolver,
         PrivilegesEvaluatorResponse presponse
     ) {
@@ -86,17 +88,16 @@ public PrivilegesEvaluatorResponse evaluate(
                             && ab.getPipelineAggregations().isEmpty()
                             && ab.getSubAggregations().isEmpty()) {
 
-                            final Set<String> allPermittedIndices = securityRoles.getAllPermittedIndicesForDashboards(
-                                resolved,
-                                user,
+                            PrivilegesEvaluatorResponse subResponse = actionPrivileges.hasIndexPrivilege(
+                                context,
                                 READ_ACTIONS,
-                                resolver,
-                                clusterService
+                                Resolved._LOCAL_ALL
                             );
-                            if (allPermittedIndices == null || allPermittedIndices.isEmpty()) {
+
+                            if (subResponse.isPartiallyOk()) {
+                                sr.source().query(new TermsQueryBuilder("_index", subResponse.getAvailableIndices()));
+                            } else if (!subResponse.isAllowed()) {
                                 sr.source().query(NONE_QUERY);
-                            } else {
-                                sr.source().query(new TermsQueryBuilder("_index", allPermittedIndices));
                             }
 
                             presponse.allowed = true;