Cleaned up use of PrivilegesEvaluatorResponse

Signed-off-by: Nils Bandener <[email protected]>

Author: nibix

CHANGELOG.md

@@ -40,6 +40,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - [GRPC] Fix compilation errors from core protobuf version bump to 0.23.0 ([#5763](https://github.com/opensearch-project/security/pull/5763))
 - Modularized PrivilegesEvaluator ([#5791](https://github.com/opensearch-project/security/pull/5791))
 - [Resource Sharing] Adds post support for update sharing info API ([#5799](https://github.com/opensearch-project/security/pull/5799))
+- Cleaned up use of PrivilegesEvaluatorResponse ([#5804](https://github.com/opensearch-project/security/pull/5804))
 
 ### Maintenance
 - Bump `org.junit.jupiter:junit-jupiter` from 5.13.4 to 5.14.1 ([#5678](https://github.com/opensearch-project/security/pull/5678), [#5764](https://github.com/opensearch-project/security/pull/5764))

src/main/java/org/opensearch/security/filter/SecurityFilter.java

@@ -402,18 +402,14 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
             User finalUser = user;
             Consumer<PrivilegesEvaluatorResponse> handleUnauthorized = response -> {
                 auditLog.logMissingPrivileges(action, request, task);
-                String err;
-                if (!response.getMissingSecurityRoles().isEmpty()) {
-                    err = String.format("No mapping for %s on roles %s", finalUser, response.getMissingSecurityRoles());
-                } else {
-                    err = (injectedRoles != null)
-                        ? String.format(
-                            "no permissions for %s and associated roles %s",
-                            response.getMissingPrivileges(),
-                            context.getMappedRoles()
-                        )
-                        : String.format("no permissions for %s and %s", response.getMissingPrivileges(), finalUser);
-                }
+                String err = (injectedRoles != null)
+                    ? String.format(
+                        "no permissions for %s and associated roles %s",
+                        response.getMissingPrivileges(),
+                        context.getMappedRoles()
+                    )
+                    : String.format("no permissions for %s and %s", response.getMissingPrivileges(), finalUser);
+
                 log.debug(err);
                 listener.onFailure(new OpenSearchSecurityException(err, RestStatus.FORBIDDEN));
             };

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

@@ -282,7 +282,7 @@ void authorizeRequest(RestHandler original, SecurityRequestChannel request, User
             .findFirst();
         final boolean routeSupportsRestAuthorization = handler.isPresent() && handler.get() instanceof NamedRoute;
         if (routeSupportsRestAuthorization) {
-            PrivilegesEvaluatorResponse pres = new PrivilegesEvaluatorResponse();
+            PrivilegesEvaluatorResponse pres;
             NamedRoute route = ((NamedRoute) handler.get());
             // Check both route.actionNames() and route.name(). The presence of either is sufficient.
             Set<String> actionNames = ImmutableSet.<String>builder()
@@ -299,12 +299,7 @@ void authorizeRequest(RestHandler original, SecurityRequestChannel request, User
                 auditLog.logGrantedPrivileges(user.getName(), request);
             } else {
                 auditLog.logMissingPrivileges(route.name(), user.getName(), request);
-                String err;
-                if (!pres.getMissingSecurityRoles().isEmpty()) {
-                    err = String.format("No mapping for %s on roles %s", user, pres.getMissingSecurityRoles());
-                } else {
-                    err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user);
-                }
+                String err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user);
                 log.debug(err);
 
                 request.queueForSending(new SecurityResponse(HttpStatus.SC_UNAUTHORIZED, err));

src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java

@@ -40,12 +40,11 @@ public PrivilegesEvaluatorResponse evaluate(
         final PrivilegesEvaluationContext context,
         final ActionPrivileges actionPrivileges,
         final String action,
-        final PrivilegesEvaluatorResponse presponse,
         final IndexResolverReplacer irr
     ) {
 
         if (!(request instanceof DeletePitRequest || request instanceof PitSegmentsRequest)) {
-            return presponse;
+            return null;
         }
         List<String> pitIds = new ArrayList<>();
 
@@ -58,9 +57,9 @@ public PrivilegesEvaluatorResponse evaluate(
         }
         // if request is for all PIT IDs, skip custom pit ids evaluation
         if (pitIds.size() == 1 && "_all".equals(pitIds.get(0))) {
-            return presponse;
+            return null;
         } else {
-            return handlePitsAccess(pitIds, context, actionPrivileges, action, presponse, irr);
+            return handlePitsAccess(pitIds, context, actionPrivileges, action, irr);
         }
     }
 
@@ -72,7 +71,6 @@ private PrivilegesEvaluatorResponse handlePitsAccess(
         PrivilegesEvaluationContext context,
         ActionPrivileges actionPrivileges,
         final String action,
-        PrivilegesEvaluatorResponse presponse,
         final IndexResolverReplacer irr
     ) {
         Map<String, String[]> pitToIndicesMap = OpenSearchSecurityPlugin.GuiceHolder.getPitService().getIndicesForPits(pitIds);
@@ -87,10 +85,9 @@ private PrivilegesEvaluatorResponse handlePitsAccess(
         PrivilegesEvaluatorResponse subResponse = actionPrivileges.hasIndexPrivilege(context, ImmutableSet.of(action), pitResolved);
         // Only if user has access to all PIT's indices, allow operation, otherwise continue evaluation in PrivilegesEvaluator.
         if (subResponse.isAllowed()) {
-            presponse.allowed = true;
-            presponse.markComplete();
+            return PrivilegesEvaluatorResponse.ok();
         }
 
-        return presponse;
+        return null;
     }
 }

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java

@@ -307,7 +307,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
             action0 = PutMappingAction.NAME;
         }
 
-        PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse();
+        PrivilegesEvaluatorResponse presponse;
 
         final boolean isDebugEnabled = log.isDebugEnabled();
         if (isDebugEnabled) {
@@ -328,7 +328,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
 
             presponse = actionPrivileges.hasClusterPrivilege(context, action0);
 
-            if (!presponse.allowed) {
+            if (!presponse.isAllowed()) {
                 log.info(
                     "No cluster-level perm match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}",
                     user,
@@ -347,23 +347,26 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
         // check snapshot/restore requests
         // NOTE: Has to go first as restore request could be for protected and/or system indices and the request may
         // fail with 403 if system index or protected index evaluators are triggered first
-        if (snapshotRestoreEvaluator.evaluate(request, task, action0, presponse).isComplete()) {
+        presponse = snapshotRestoreEvaluator.evaluate(request, task, action0);
+        if (presponse != null) {
             return presponse;
         }
 
         // System index access
-        if (systemIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, context, actionPrivileges, user)
-            .isComplete()) {
+        presponse = systemIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, context, actionPrivileges, user);
+        if (presponse != null) {
             return presponse;
         }
 
         // Protected index access
-        if (protectedIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, mappedRoles).isComplete()) {
+        presponse = protectedIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, mappedRoles);
+        if (presponse != null) {
             return presponse;
         }
 
         // check access for point in time requests
-        if (pitPrivilegesEvaluator.evaluate(request, context, actionPrivileges, action0, presponse, irr).isComplete()) {
+        presponse = pitPrivilegesEvaluator.evaluate(request, context, actionPrivileges, action0, irr);
+        if (presponse != null) {
             return presponse;
         }
 
@@ -380,7 +383,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
 
             presponse = actionPrivileges.hasClusterPrivilege(context, action0);
 
-            if (!presponse.allowed) {
+            if (!presponse.isAllowed()) {
                 log.info(
                     "No cluster-level perm match for {} {} [Action [{}]] [RolesChecked {}]. No permissions for {}",
                     user,
@@ -412,28 +415,25 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
                             if (replaceResult.accessDenied) {
                                 auditLog.logMissingPrivileges(action0, request, task);
                             } else {
-                                presponse.allowed = true;
-                                presponse.createIndexRequestBuilder = replaceResult.createIndexRequestBuilder;
+                                return PrivilegesEvaluatorResponse.ok().with(replaceResult.createIndexRequestBuilder);
                             }
-                            return presponse;
                         }
                     }
 
                     log.debug("Allowed because we have cluster permissions for {}", action0);
 
-                    presponse.allowed = true;
-                    return presponse;
+                    return PrivilegesEvaluatorResponse.ok();
                 }
             }
         }
 
         if (checkDocAllowListHeader(user, action0, request)) {
-            presponse.allowed = true;
-            return presponse;
+            return PrivilegesEvaluatorResponse.ok();
         }
 
         // term aggregations
-        if (termsAggregationEvaluator.evaluate(requestedResolved, request, context, actionPrivileges, presponse).isComplete()) {
+        presponse = termsAggregationEvaluator.evaluate(requestedResolved, request, context, actionPrivileges);
+        if (presponse != null) {
             return presponse;
         }
 
@@ -462,9 +462,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
                     auditLog.logMissingPrivileges(action0, request, task);
                     return PrivilegesEvaluatorResponse.insufficient(action0);
                 } else {
-                    presponse.allowed = true;
-                    presponse.createIndexRequestBuilder = replaceResult.createIndexRequestBuilder;
-                    return presponse;
+                    return PrivilegesEvaluatorResponse.ok().with(replaceResult.createIndexRequestBuilder);
                 }
             }
         }
@@ -497,8 +495,7 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
 
         if (presponse.isAllowed()) {
             if (checkFilteredAliases(requestedResolved, action0, isDebugEnabled)) {
-                presponse.allowed = false;
-                return presponse;
+                return PrivilegesEvaluatorResponse.insufficient(action0);
             }
 
             log.debug("Allowed because we have all indices permissions for {}", action0);