From 8c892638c55f75090cc8f0e0f10fb6f3f39ab2d6 Mon Sep 17 00:00:00 2001 From: Nils Bandener Date: Fri, 31 May 2024 09:17:31 +0200 Subject: [PATCH] Adapted TermsAggregationEvaluator to use ActionPrivileges Signed-off-by: Nils Bandener --- .../privileges/PrivilegesEvaluator.java | 8 ++-- .../privileges/TermsAggregationEvaluator.java | 37 ++++++++++--------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 2103dc58e8..988775ddf5 100644 --- 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 index d06a45726a..e53ba5f712 100644 --- 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 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 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;