diff --git a/CHANGELOG.md b/CHANGELOG.md index 825c0dc203..5a76e6e9e4 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/src/main/java/org/opensearch/security/filter/SecurityFilter.java b/src/main/java/org/opensearch/security/filter/SecurityFilter.java index f5424fd2ad..f4e005d6a2 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityFilter.java @@ -402,18 +402,14 @@ private void ap User finalUser = user; Consumer 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)); }; diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 53cfdc03e0..d96e704e83 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/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 actionNames = ImmutableSet.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)); diff --git a/src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java index 4fd4141b08..93cdb1d973 100644 --- a/src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java +++ b/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 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 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; } } diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java index 3880f16dfe..558fb6e6ad 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorImpl.java +++ b/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); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java index d072ec301c..574200b3d7 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java @@ -26,13 +26,11 @@ package org.opensearch.security.privileges; -import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.HashSet; -import java.util.List; import java.util.Set; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; import org.opensearch.action.admin.indices.create.CreateIndexRequestBuilder; @@ -40,19 +38,49 @@ import com.selectivem.collections.CheckTable; public class PrivilegesEvaluatorResponse { - boolean allowed = false; - Set missingSecurityRoles = new HashSet<>(); - PrivilegesEvaluatorResponseState state = PrivilegesEvaluatorResponseState.PENDING; - CreateIndexRequestBuilder createIndexRequestBuilder; - private Set onlyAllowedForIndices = ImmutableSet.of(); - private CheckTable indexToActionCheckTable; + private final boolean allowed; + private final CreateIndexRequestBuilder createIndexRequestBuilder; + private final ImmutableSet onlyAllowedForIndices; + private final CheckTable indexToActionCheckTable; private String privilegeMatrix; - private String reason; + private final String reason; /** * Contains issues that were encountered during privilege evaluation. Can be used for logging. */ - private List evaluationExceptions = new ArrayList<>(); + private ImmutableList evaluationExceptions; + + public PrivilegesEvaluatorResponse( + boolean allowed, + ImmutableSet onlyAllowedForIndices, + CheckTable indexToActionCheckTable, + String privilegeMatrix, + String reason, + ImmutableList evaluationExceptions, + CreateIndexRequestBuilder createIndexRequestBuilder + ) { + this.allowed = allowed; + this.createIndexRequestBuilder = createIndexRequestBuilder; + this.onlyAllowedForIndices = onlyAllowedForIndices; + this.indexToActionCheckTable = indexToActionCheckTable; + this.privilegeMatrix = privilegeMatrix; + this.reason = reason; + this.evaluationExceptions = evaluationExceptions; + } + + public PrivilegesEvaluatorResponse( + boolean allowed, + ImmutableSet onlyAllowedForIndices, + CheckTable indexToActionCheckTable + ) { + this.allowed = allowed; + this.createIndexRequestBuilder = null; + this.onlyAllowedForIndices = onlyAllowedForIndices; + this.indexToActionCheckTable = indexToActionCheckTable; + this.privilegeMatrix = null; + this.reason = null; + this.evaluationExceptions = ImmutableList.of(); + } /** * Returns true if the request can be fully allowed. See also isAllowedForSpecificIndices(). @@ -93,8 +121,15 @@ public String getReason() { } public PrivilegesEvaluatorResponse reason(String reason) { - this.reason = reason; - return this; + return new PrivilegesEvaluatorResponse( + this.allowed, + this.onlyAllowedForIndices, + this.indexToActionCheckTable, + this.privilegeMatrix, + reason, + this.evaluationExceptions, + this.createIndexRequestBuilder + ); } /** @@ -115,8 +150,18 @@ public boolean hasEvaluationExceptions() { } public PrivilegesEvaluatorResponse evaluationExceptions(Collection evaluationExceptions) { - this.evaluationExceptions.addAll(evaluationExceptions); - return this; + if (evaluationExceptions.isEmpty()) { + return this; + } + return new PrivilegesEvaluatorResponse( + this.allowed, + this.onlyAllowedForIndices, + this.indexToActionCheckTable, + this.privilegeMatrix, + this.reason, + ImmutableList.builder().addAll(this.evaluationExceptions).addAll(evaluationExceptions).build(), + this.createIndexRequestBuilder + ); } /** @@ -133,30 +178,24 @@ public String getPrivilegeMatrix() { return result; } - public Set getMissingSecurityRoles() { - return new HashSet<>(missingSecurityRoles); - } - public CreateIndexRequestBuilder getCreateIndexRequestBuilder() { return createIndexRequestBuilder; } - public PrivilegesEvaluatorResponse markComplete() { - this.state = PrivilegesEvaluatorResponseState.COMPLETE; - return this; - } - - public PrivilegesEvaluatorResponse markPending() { - this.state = PrivilegesEvaluatorResponseState.PENDING; - return this; - } - - public boolean isComplete() { - return this.state == PrivilegesEvaluatorResponseState.COMPLETE; - } + public PrivilegesEvaluatorResponse with(CreateIndexRequestBuilder createIndexRequestBuilder) { + if (createIndexRequestBuilder == this.createIndexRequestBuilder) { + return this; + } - public boolean isPending() { - return this.state == PrivilegesEvaluatorResponseState.PENDING; + return new PrivilegesEvaluatorResponse( + this.allowed, + this.onlyAllowedForIndices, + this.indexToActionCheckTable, + this.privilegeMatrix, + this.reason, + this.evaluationExceptions, + createIndexRequestBuilder + ); } @Override @@ -171,36 +210,25 @@ public String toString() { } public static PrivilegesEvaluatorResponse ok() { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.allowed = true; - return response; + return new PrivilegesEvaluatorResponse(true, ImmutableSet.of(), null); } public static PrivilegesEvaluatorResponse partiallyOk( Set availableIndices, CheckTable indexToActionCheckTable ) { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.onlyAllowedForIndices = ImmutableSet.copyOf(availableIndices); - response.indexToActionCheckTable = indexToActionCheckTable; - return response; + return new PrivilegesEvaluatorResponse(false, ImmutableSet.copyOf(availableIndices), indexToActionCheckTable); } public static PrivilegesEvaluatorResponse insufficient(String missingPrivilege) { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.indexToActionCheckTable = CheckTable.create(ImmutableSet.of("_"), ImmutableSet.of(missingPrivilege)); - return response; + return new PrivilegesEvaluatorResponse( + false, + ImmutableSet.of(), + CheckTable.create(ImmutableSet.of("_"), ImmutableSet.of(missingPrivilege)) + ); } public static PrivilegesEvaluatorResponse insufficient(CheckTable indexToActionCheckTable) { - PrivilegesEvaluatorResponse response = new PrivilegesEvaluatorResponse(); - response.indexToActionCheckTable = indexToActionCheckTable; - return response; + return new PrivilegesEvaluatorResponse(false, ImmutableSet.of(), indexToActionCheckTable); } - - public static enum PrivilegesEvaluatorResponseState { - PENDING, - COMPLETE; - } - } diff --git a/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java index 877e6fd787..05d431ccc8 100644 --- a/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/ProtectedIndexAccessEvaluator.java @@ -67,16 +67,18 @@ public ProtectedIndexAccessEvaluator(final Settings settings, AuditLog auditLog) this.deniedActionMatcher = WildcardMatcher.from(indexDeniedActionPatterns); } + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ public PrivilegesEvaluatorResponse evaluate( final ActionRequest request, final Task task, final String action, final IndexResolverReplacer.Resolved requestedResolved, - final PrivilegesEvaluatorResponse presponse, final Set mappedRoles ) { if (!protectedIndexEnabled) { - return presponse; + return null; } if (!requestedResolved.isLocalAll() && indexMatcher.matchAny(requestedResolved.getAllIndices()) @@ -84,15 +86,13 @@ public PrivilegesEvaluatorResponse evaluate( && !allowedRolesMatcher.matchAny(mappedRoles)) { auditLog.logMissingPrivileges(action, request, task); log.warn("{} for '{}' index/indices is not allowed for a regular user", action, indexMatcher); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } if (requestedResolved.isLocalAll() && deniedActionMatcher.test(action) && !allowedRolesMatcher.matchAny(mappedRoles)) { auditLog.logMissingPrivileges(action, request, task); log.warn("{} for '_all' indices is not allowed for a regular user", action); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } if ((requestedResolved.isLocalAll() || indexMatcher.matchAny(requestedResolved.getAllIndices())) && !allowedRolesMatcher.matchAny(mappedRoles)) { @@ -112,6 +112,6 @@ public PrivilegesEvaluatorResponse evaluate( } } } - return presponse; + return null; } } diff --git a/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java index b83e174600..95ff4d6482 100644 --- a/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/ResourceAccessEvaluator.java @@ -78,8 +78,6 @@ public void evaluateAsync( final String action, final ActionListener pResponseListener ) { - PrivilegesEvaluatorResponse pResponse = new PrivilegesEvaluatorResponse(); - log.debug("Evaluating resource access"); // if it reached this evaluator, it is safe to assume that the request if of DocRequest type @@ -87,12 +85,11 @@ public void evaluateAsync( resourceAccessHandler.hasPermission(req.id(), req.type(), action, ActionListener.wrap(hasAccess -> { if (hasAccess) { - pResponse.allowed = true; - pResponseListener.onResponse(pResponse.markComplete()); - return; + pResponseListener.onResponse(PrivilegesEvaluatorResponse.ok()); + } else { + pResponseListener.onResponse(PrivilegesEvaluatorResponse.insufficient(action)); } - pResponseListener.onResponse(PrivilegesEvaluatorResponse.insufficient(action).markComplete()); - }, e -> { pResponseListener.onResponse(pResponse.markComplete()); })); + }, e -> { pResponseListener.onResponse(PrivilegesEvaluatorResponse.insufficient(action)); })); } /** diff --git a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java index 4890dd5a89..36e32d650a 100644 --- a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java @@ -38,7 +38,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, final String routeN PrivilegesEvaluatorResponse result = context.getActionPrivileges().hasAnyClusterPrivilege(context, actions); - if (!result.allowed) { + if (!result.isAllowed()) { log.info( "No permission match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}", user, diff --git a/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java b/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java index ca5cf985d3..ca56f81df4 100644 --- a/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SnapshotRestoreEvaluator.java @@ -68,34 +68,29 @@ public SnapshotRestoreEvaluator( this.isLocalNodeElectedClusterManagerSupplier = isLocalNodeElectedClusterManagerSupplier; } - public PrivilegesEvaluatorResponse evaluate( - final ActionRequest request, - final Task task, - final String action, - final PrivilegesEvaluatorResponse presponse - ) { + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ + public PrivilegesEvaluatorResponse evaluate(final ActionRequest request, final Task task, final String action) { if (!(request instanceof RestoreSnapshotRequest)) { - return presponse; + return null; } // snapshot restore for regular users not enabled if (!enableSnapshotRestorePrivilege) { log.warn("{} is not allowed for a regular user", action); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(action); } // if this feature is enabled, users can also snapshot and restore // the Security index and the global state if (restoreSecurityIndexEnabled) { - presponse.allowed = true; - return presponse; + return null; } if (!isLocalNodeElectedClusterManagerSupplier.get()) { - presponse.allowed = true; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.ok(); } final RestoreSnapshotRequest restoreRequest = (RestoreSnapshotRequest) request; @@ -104,8 +99,7 @@ public PrivilegesEvaluatorResponse evaluate( if (restoreRequest.includeGlobalState()) { auditLog.logSecurityIndexAttempt(request, action, task); log.warn("{} with 'include_global_state' enabled is not allowed", action); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(action).reason("'include_global_state' is not allowed"); } final List rs = SnapshotRestoreHelper.resolveOriginalIndices(restoreRequest); @@ -113,9 +107,8 @@ public PrivilegesEvaluatorResponse evaluate( if (rs != null && (rs.contains(securityIndex) || rs.contains("_all") || rs.contains("*"))) { auditLog.logSecurityIndexAttempt(request, action, task); log.warn("{} for '{}' as source index is not allowed", action, securityIndex); - presponse.allowed = false; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient("").reason(securityIndex + " as source index is not allowed"); } - return presponse; + return null; } } diff --git a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java index fb28eab972..a2c87b6367 100644 --- a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java @@ -123,17 +123,30 @@ private static List deniedActionPatterns() { return securityIndexDeniedActionPatternsList; } + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ public PrivilegesEvaluatorResponse evaluate( final ActionRequest request, final Task task, final String action, final Resolved requestedResolved, - final PrivilegesEvaluatorResponse presponse, final PrivilegesEvaluationContext context, final ActionPrivileges actionPrivileges, final User user ) { - evaluateSystemIndicesAccess(action, requestedResolved, request, task, presponse, context, actionPrivileges, user); + PrivilegesEvaluatorResponse presponse = evaluateSystemIndicesAccess( + action, + requestedResolved, + request, + task, + context, + actionPrivileges, + user + ); + if (presponse != null && !presponse.isAllowed()) { + return presponse; + } if (requestedResolved.isLocalAll() || requestedResolved.getAllIndices().contains(securityIndex) @@ -234,17 +247,15 @@ private boolean isActionAllowed(String action) { * @param requestedResolved this object contains all indices this request is resolved to * @param request the action request to be used for audit logging * @param task task in which this access check will be performed - * @param presponse the pre-response object that will eventually become a response and returned to the requester * @param context conveys information about user and mapped roles, etc. * @param actionPrivileges the up-to-date ActionPrivileges instance * @param user this user's permissions will be looked up */ - private void evaluateSystemIndicesAccess( + private PrivilegesEvaluatorResponse evaluateSystemIndicesAccess( final String action, final Resolved requestedResolved, final ActionRequest request, final Task task, - final PrivilegesEvaluatorResponse presponse, final PrivilegesEvaluationContext context, final ActionPrivileges actionPrivileges, final User user @@ -269,9 +280,7 @@ private void evaluateSystemIndicesAccess( .collect(Collectors.toList()); log.debug("Service account cannot access regular indices: {}", regularIndices); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient("").reason("Service account cannot access regular indices"); } boolean containsProtectedIndex = requestContainsAnyProtectedSystemIndices(requestedResolved); if (containsProtectedIndex) { @@ -284,9 +293,7 @@ private void evaluateSystemIndicesAccess( String.join(", ", getAllProtectedSystemIndices(requestedResolved)) ); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } else if (containsSystemIndex && !actionPrivileges.hasExplicitIndexPrivilege(context, SYSTEM_INDEX_PERMISSION_SET, requestedResolved).isAllowed()) { auditLog.logSecurityIndexAttempt(request, action, task); @@ -298,9 +305,7 @@ private void evaluateSystemIndicesAccess( String.join(", ", getAllSystemIndices(requestedResolved)) ); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } } @@ -313,9 +318,7 @@ private void evaluateSystemIndicesAccess( ); if (requestedResolved.getAllIndices().equals(matchingPluginIndices)) { // plugin is authorized to perform any actions on its own registered system indices - presponse.allowed = true; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.ok(); } else { Set matchingSystemIndices = SystemIndexRegistry.matchesSystemIndexPattern(requestedResolved.getAllIndices()); matchingSystemIndices.removeAll(matchingPluginIndices); @@ -329,17 +332,12 @@ private void evaluateSystemIndicesAccess( matchingPluginIndices ); } - presponse.allowed = false; - presponse.getMissingPrivileges(); - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } } } else { // no system index protection and request originating from plugin, allow - presponse.allowed = true; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.ok(); } } @@ -359,8 +357,7 @@ private void evaluateSystemIndicesAccess( } else { auditLog.logSecurityIndexAttempt(request, action, task); log.warn("{} for '_all' indices is not allowed for a regular user", action); - presponse.allowed = false; - presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } } // if system index is enabled and system index permissions are enabled we don't need to perform any further @@ -373,9 +370,7 @@ else if (containsSystemIndex && !isSystemIndexPermissionEnabled) { if (log.isDebugEnabled()) { log.debug("Filtered '{}' but resulting list is empty", securityIndex); } - presponse.allowed = false; - presponse.markComplete(); - return; + return PrivilegesEvaluatorResponse.insufficient(""); } irr.replace(request, false, allWithoutSecurity.toArray(new String[0])); if (log.isDebugEnabled()) { @@ -385,10 +380,10 @@ else if (containsSystemIndex && !isSystemIndexPermissionEnabled) { auditLog.logSecurityIndexAttempt(request, action, task); final String foundSystemIndexes = String.join(", ", getAllSystemIndices(requestedResolved)); log.warn("{} for '{}' index is not allowed for a regular user", action, foundSystemIndexes); - presponse.allowed = false; - presponse.markComplete(); + return PrivilegesEvaluatorResponse.insufficient(""); } } } + return null; } } diff --git a/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java b/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java index a2cd1c16a7..f9de82f798 100644 --- a/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/TermsAggregationEvaluator.java @@ -61,12 +61,14 @@ public class TermsAggregationEvaluator { public TermsAggregationEvaluator() {} + /** + * @return a PrivilegesEvaluatorResponse if the evaluation process is completed here, null otherwise + */ public PrivilegesEvaluatorResponse evaluate( final Resolved resolved, final ActionRequest request, PrivilegesEvaluationContext context, - ActionPrivileges actionPrivileges, - PrivilegesEvaluatorResponse presponse + ActionPrivileges actionPrivileges ) { try { if (request instanceof SearchRequest) { @@ -102,17 +104,16 @@ public PrivilegesEvaluatorResponse evaluate( sr.source().query(NONE_QUERY); } - presponse.allowed = true; - return presponse.markComplete(); + return PrivilegesEvaluatorResponse.ok(); } } } } } catch (Exception e) { log.warn("Unable to evaluate terms aggregation", e); - return presponse; + return null; } - return presponse; + return null; } } diff --git a/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java index ff70fe8d9f..b6c75ac435 100644 --- a/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/ResourceAccessEvaluatorTest.java @@ -87,8 +87,7 @@ private void assertEvaluateAsync(boolean hasPermission, boolean expectedAllowed) verify(callback).onResponse(captor.capture()); PrivilegesEvaluatorResponse out = captor.getValue(); - assertThat(out.allowed, equalTo(expectedAllowed)); - assertThat(out.isComplete(), equalTo(true)); + assertThat(out.isAllowed(), equalTo(expectedAllowed)); } @Test diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java index bfb5719be7..311176c9a0 100644 --- a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java @@ -101,7 +101,7 @@ public void testEvaluate_Successful_NewPermission() throws Exception { PrivilegesConfiguration privilegesConfiguration = createPrivilegesConfiguration(roles); RestLayerPrivilegesEvaluator restPrivilegesEvaluator = new RestLayerPrivilegesEvaluator(privilegesConfiguration); PrivilegesEvaluatorResponse response = restPrivilegesEvaluator.evaluate(TEST_USER, "route_name", Set.of(action)); - assertThat(response.allowed, equalTo(true)); + assertThat(response.isAllowed(), equalTo(true)); } @Test @@ -113,7 +113,7 @@ public void testEvaluate_Successful_LegacyPermission() throws Exception { PrivilegesConfiguration privilegesConfiguration = createPrivilegesConfiguration(roles); RestLayerPrivilegesEvaluator restPrivilegesEvaluator = new RestLayerPrivilegesEvaluator(privilegesConfiguration); PrivilegesEvaluatorResponse response = restPrivilegesEvaluator.evaluate(TEST_USER, "route_name", Set.of(action)); - assertThat(response.allowed, equalTo(true)); + assertThat(response.isAllowed(), equalTo(true)); } @Test @@ -125,7 +125,7 @@ public void testEvaluate_Unsuccessful() throws Exception { PrivilegesConfiguration privilegesConfiguration = createPrivilegesConfiguration(roles); RestLayerPrivilegesEvaluator restPrivilegesEvaluator = new RestLayerPrivilegesEvaluator(privilegesConfiguration); PrivilegesEvaluatorResponse response = restPrivilegesEvaluator.evaluate(TEST_USER, "route_name", Set.of(action)); - assertThat(response.allowed, equalTo(false)); + assertThat(response.isAllowed(), equalTo(false)); } PrivilegesConfiguration createPrivilegesConfiguration(SecurityDynamicConfiguration roles) { diff --git a/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java index 17a78501c9..69e94cccca 100644 --- a/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/SystemIndexAccessEvaluatorTest.java @@ -54,11 +54,11 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; import static org.opensearch.security.support.ConfigConstants.SYSTEM_INDEX_PERMISSION; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.when; @@ -74,8 +74,6 @@ public class SystemIndexAccessEvaluatorTest { @Mock private Task task; @Mock - private PrivilegesEvaluatorResponse presponse; - @Mock private Logger log; @Mock ClusterService cs; @@ -181,7 +179,7 @@ PrivilegesEvaluationContext ctx(String action) { @After public void after() { - verifyNoMoreInteractions(auditLog, irr, request, task, presponse, log); + verifyNoMoreInteractions(auditLog, irr, request, task); } @Test @@ -195,13 +193,11 @@ public void testUnprotectedActionOnRegularIndex_systemIndexDisabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @@ -216,13 +212,11 @@ public void testUnprotectedActionOnRegularIndex_systemIndexPermissionDisabled() null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -236,13 +230,11 @@ public void testUnprotectedActionOnRegularIndex_systemIndexPermissionEnabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -256,13 +248,11 @@ public void testUnprotectedActionOnSystemIndex_systemIndexDisabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -276,13 +266,11 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionDisabled() { null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verifyNoInteractions(presponse); - assertThat(response, is(presponse)); + assertThat(response, is(nullValue())); } @Test @@ -296,13 +284,11 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionEnabled_With null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - verify(presponse).markComplete(); - assertThat(response, is(presponse)); + assertThat(response.isAllowed(), is(false)); verify(auditLog).logSecurityIndexAttempt(request, UNPROTECTED_ACTION, null); verify(log).isInfoEnabled(); @@ -325,14 +311,12 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionEnabled_With null, UNPROTECTED_ACTION, resolved, - presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user ); - assertThat(response, is(presponse)); // unprotected action is not allowed on a system index - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -344,11 +328,20 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexDisabled() { final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - - verifyNoInteractions(presponse); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); } @Test @@ -360,9 +353,20 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexPermissionDisable final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); verify(searchRequest).requestCache(Boolean.FALSE); verify(realtimeRequest).realtime(Boolean.FALSE); @@ -381,30 +385,30 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexPermissionEnabled final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - - verify(searchRequest).requestCache(Boolean.FALSE); - verify(realtimeRequest).realtime(Boolean.FALSE); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response.isAllowed(), is(false)); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response.isAllowed(), is(false)); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response.isAllowed(), is(false)); - verify(log, times(2)).isDebugEnabled(); - verify(log).debug("Disable search request cache for this request"); - verify(log).debug("Disable realtime for this request"); verify(auditLog).logSecurityIndexAttempt(request, UNPROTECTED_ACTION, null); verify(auditLog).logSecurityIndexAttempt(searchRequest, UNPROTECTED_ACTION, null); verify(auditLog).logSecurityIndexAttempt(realtimeRequest, UNPROTECTED_ACTION, null); - verify(presponse, times(3)).markComplete(); - verify(log, times(2)).isDebugEnabled(); - verify(log, times(3)).isInfoEnabled(); verify(log, times(3)).info( "No {} permission for user roles {} to System Indices {}", UNPROTECTED_ACTION, user.getSecurityRoles(), TEST_SYSTEM_INDEX ); - verify(log).debug("Disable search request cache for this request"); - verify(log).debug("Disable realtime for this request"); } @Test @@ -416,14 +420,24 @@ public void testDisableCacheOrRealtimeOnSystemIndex_systemIndexPermissionEnabled final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); - evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + null, + UNPROTECTED_ACTION, + resolved, + ctx(UNPROTECTED_ACTION), + actionPrivileges, + user + ); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); + response = evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, ctx(UNPROTECTED_ACTION), actionPrivileges, user); + assertThat(response, is(nullValue())); verify(searchRequest).requestCache(Boolean.FALSE); verify(realtimeRequest).realtime(Boolean.FALSE); - verify(log, times(2)).isDebugEnabled(); verify(log).debug("Disable search request cache for this request"); verify(log).debug("Disable realtime for this request"); } @@ -434,11 +448,18 @@ public void testProtectedActionLocalAll_systemIndexDisabled() { final Resolved resolved = Resolved._LOCAL_ALL; // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '_all' indices is not allowed for a regular user", "indices:data/write"); } @@ -448,11 +469,18 @@ public void testProtectedActionLocalAll_systemIndexPermissionDisabled() { final Resolved resolved = Resolved._LOCAL_ALL; // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '_all' indices is not allowed for a regular user", PROTECTED_ACTION); } @@ -462,11 +490,18 @@ public void testProtectedActionLocalAll_systemIndexPermissionEnabled() { final Resolved resolved = Resolved._LOCAL_ALL; // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '_all' indices is not allowed for a regular user", PROTECTED_ACTION); } @@ -476,9 +511,17 @@ public void testProtectedActionOnRegularIndex_systemIndexDisabled() { final Resolved resolved = createResolved(TEST_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -487,9 +530,17 @@ public void testProtectedActionOnRegularIndex_systemIndexPermissionDisabled() { final Resolved resolved = createResolved(TEST_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -498,9 +549,17 @@ public void testProtectedActionOnRegularIndex_systemIndexPermissionEnabled() { final Resolved resolved = createResolved(TEST_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -509,9 +568,17 @@ public void testProtectedActionOnSystemIndex_systemIndexDisabled() { final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -520,11 +587,18 @@ public void testProtectedActionOnSystemIndex_systemIndexPermissionDisabled() { final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '{}' index is not allowed for a regular user", PROTECTED_ACTION, TEST_SYSTEM_INDEX); } @@ -534,11 +608,18 @@ public void testProtectedActionOnSystemIndex_systemIndexPermissionEnabled_withou final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).isInfoEnabled(); verify(log).info( "No {} permission for user roles {} to System Indices {}", @@ -555,9 +636,17 @@ public void testProtectedActionOnSystemIndex_systemIndexPermissionEnabled_withSy final Resolved resolved = createResolved(TEST_SYSTEM_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); - assertThat(presponse.allowed, is(false)); + assertThat(response, is(nullValue())); } @Test @@ -566,11 +655,18 @@ public void testProtectedActionOnProtectedSystemIndex_systemIndexDisabled() { final Resolved resolved = createResolved(SECURITY_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '{}' index is not allowed for a regular user", PROTECTED_ACTION, SECURITY_INDEX); } @@ -581,11 +677,18 @@ public void testProtectedActionOnProtectedSystemIndex_systemIndexPermissionDisab final Resolved resolved = createResolved(SECURITY_INDEX); // Action - evaluator.evaluate(request, task, PROTECTED_ACTION, resolved, presponse, ctx(PROTECTED_ACTION), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate( + request, + task, + PROTECTED_ACTION, + resolved, + ctx(PROTECTED_ACTION), + actionPrivileges, + user + ); verify(auditLog).logSecurityIndexAttempt(request, PROTECTED_ACTION, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).warn("{} for '{}' index is not allowed for a regular user", PROTECTED_ACTION, SECURITY_INDEX); } @@ -616,11 +719,10 @@ private void testSecurityIndexAccess(String action) { final Resolved resolved = createResolved(SECURITY_INDEX); // Action - evaluator.evaluate(request, task, action, resolved, presponse, ctx(action), actionPrivileges, user); + PrivilegesEvaluatorResponse response = evaluator.evaluate(request, task, action, resolved, ctx(action), actionPrivileges, user); verify(auditLog).logSecurityIndexAttempt(request, action, task); - assertThat(presponse.allowed, is(false)); - verify(presponse).markComplete(); + assertThat(response.isAllowed(), is(false)); verify(log).isInfoEnabled(); verify(log).info(