Made PrivilegesEvaluationContext.mappedRoles non-final, moved OPENDIS…

…TRO_SECURITY_INJECTED_ROLES_VALIDATION handling back to evaluate()

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

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

@@ -379,15 +379,8 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
                 log.trace("Evaluate permissions for user: {}", user.getName());
             }
 
-            PrivilegesEvaluationContext context = null;
-            PrivilegesEvaluatorResponse pres;
-
-            try {
-                context = eval.createContext(user, action, request, task, injectedRoles);
-                pres = eval.evaluate(context);
-            } catch (PrivilegesEvaluatorResponse.NotAllowedException e) {
-                pres = e.getResponse();
-            }
+            PrivilegesEvaluationContext context = eval.createContext(user, action, request, task, injectedRoles);
+            PrivilegesEvaluatorResponse pres = eval.evaluate(context);
 
             if (log.isDebugEnabled()) {
                 log.debug(pres.toString());

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

@@ -32,7 +32,7 @@ public class PrivilegesEvaluationContext {
     private final ActionRequest request;
     private IndexResolverReplacer.Resolved resolvedRequest;
     private final Task task;
-    private final ImmutableSet<String> mappedRoles;
+    private ImmutableSet<String> mappedRoles;
     private final IndexResolverReplacer indexResolverReplacer;
 
     public PrivilegesEvaluationContext(
@@ -82,4 +82,16 @@ public ImmutableSet<String> getMappedRoles() {
         return mappedRoles;
     }
 
+    /**
+     * Note: Ideally, mappedRoles would be an unmodifiable attribute. PrivilegesEvaluator however contains logic
+     * related to OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION which first validates roles and afterwards modifies
+     * them again. Thus, we need to be able to set this attribute.
+     *
+     * However, this method should be only used for this one particular phase. Normally, all roles should be determined
+     * upfront and stay constant during the whole privilege evaluation process.
+     */
+    void setMappedRoles(ImmutableSet<String> mappedRoles) {
+        this.mappedRoles = mappedRoles;
+    }
+
 }

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

@@ -221,29 +221,19 @@ private void setUserInfoInThreadContext(User user) {
         }
     }
 
-    public PrivilegesEvaluationContext createContext(User user, String action0, ActionRequest request, Task task, Set<String> injectedRoles)
-        throws PrivilegesEvaluatorResponse.NotAllowedException {
+    public PrivilegesEvaluationContext createContext(
+        User user,
+        String action0,
+        ActionRequest request,
+        Task task,
+        Set<String> injectedRoles
+    ) {
         if (!isInitialized()) {
             throw new OpenSearchSecurityException("OpenSearch Security is not initialized.");
         }
 
         TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS);
         ImmutableSet<String> mappedRoles = ImmutableSet.copyOf((injectedRoles == null) ? mapRoles(user, caller) : injectedRoles);
-        final String injectedRolesValidationString = threadContext.getTransient(
-            ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION
-        );
-        if (injectedRolesValidationString != null) {
-            HashSet<String> injectedRolesValidationSet = new HashSet<>(Arrays.asList(injectedRolesValidationString.split(",")));
-            if (!mappedRoles.containsAll(injectedRolesValidationSet)) {
-                PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse();
-                presponse.allowed = false;
-                presponse.missingSecurityRoles.addAll(injectedRolesValidationSet);
-                presponse.reason("Injected roles validation failed");
-                log.info("Roles {} are not mapped to the user {}", injectedRolesValidationSet, user);
-                throw new PrivilegesEvaluatorResponse.NotAllowedException(presponse);
-            }
-            mappedRoles = ImmutableSet.copyOf(injectedRolesValidationSet);
-        }
 
         return new PrivilegesEvaluationContext(user, mappedRoles, action0, request, task, irr);
     }
@@ -272,8 +262,22 @@ public PrivilegesEvaluatorResponse evaluate(PrivilegesEvaluationContext context)
             action0 = PutMappingAction.NAME;
         }
 
-        PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse();
+        final PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse();
 
+        final String injectedRolesValidationString = threadContext.getTransient(
+            ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES_VALIDATION
+        );
+        if (injectedRolesValidationString != null) {
+            HashSet<String> injectedRolesValidationSet = new HashSet<>(Arrays.asList(injectedRolesValidationString.split(",")));
+            if (!mappedRoles.containsAll(injectedRolesValidationSet)) {
+                presponse.allowed = false;
+                presponse.missingSecurityRoles.addAll(injectedRolesValidationSet);
+                log.info("Roles {} are not mapped to the user {}", injectedRolesValidationSet, user);
+                return presponse;
+            }
+            mappedRoles = ImmutableSet.copyOf(injectedRolesValidationSet);
+            context.setMappedRoles(mappedRoles);
+        }
         presponse.resolvedSecurityRoles.addAll(mappedRoles);
         final SecurityRoles securityRoles = getSecurityRoles(mappedRoles);
 

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

@@ -96,27 +96,4 @@ public static enum PrivilegesEvaluatorResponseState {
         PENDING,
         COMPLETE;
     }
-
-    /**
-     * This exception can be used to indicate that a method denies a user access to an OpenSearch action.
-     *
-     * Note: As exceptions take their performance toll, please use this exception only when there is
-     * no other way. Prefer to use PrivilegesEvaluatorResponse directly as a return value.
-     */
-    public static class NotAllowedException extends Exception {
-        private final PrivilegesEvaluatorResponse response;
-
-        public NotAllowedException(PrivilegesEvaluatorResponse response) {
-            super(response.reason);
-            this.response = response;
-            if (response.allowed) {
-                throw new IllegalArgumentException("Only possible for PrivilegesEvaluatorResponse with allowed=false");
-            }
-        }
-
-        public PrivilegesEvaluatorResponse getResponse() {
-            return response;
-        }
-    }
-
 }