Adds more tests around system index permissions and cleans up code

Signed-off-by: Darshit Chanpura <[email protected]>
samuelcostae · Sep 5, 2023 · e595953 · e595953
1 parent 526a824
commit e595953
Show file tree

Hide file tree

Showing 13 changed files with 402 additions and 173 deletions.
diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -314,7 +314,17 @@ public PrivilegesEvaluatorResponse evaluate(
         }
 
         // Security index access
-        if (securityIndexAccessEvaluator.evaluate(request, task, action0, requestedResolved, presponse, securityRoles, user, resolver, clusterService).isComplete()) {
+        if (securityIndexAccessEvaluator.evaluate(
+            request,
+            task,
+            action0,
+            requestedResolved,
+            presponse,
+            securityRoles,
+            user,
+            resolver,
+            clusterService
+        ).isComplete()) {
             return presponse;
         }
 

diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
@@ -247,20 +247,27 @@ private void evaluateSystemIndicesAccess(
                 presponse.allowed = false;
                 presponse.markComplete();
                 return;
-            } else if (containsSystemIndex && !securityRoles.hasExplicitIndexPermission(requestedResolved, user, new String[] {ConfigConstants.SYSTEM_INDEX_PERMISSION}, resolver, clusterService)) {
-                auditLog.logSecurityIndexAttempt(request, action, task);
-                if (log.isInfoEnabled()) {
-                    log.info(
-                        "No {} permission for user roles {} to System Indices {}",
-                        action,
-                        securityRoles,
-                        String.join(", ", getAllSystemIndices(requestedResolved))
-                    );
+            } else if (containsSystemIndex
+                && !securityRoles.hasExplicitIndexPermission(
+                    requestedResolved,
+                    user,
+                    new String[] { ConfigConstants.SYSTEM_INDEX_PERMISSION },
+                    resolver,
+                    clusterService
+                )) {
+                    auditLog.logSecurityIndexAttempt(request, action, task);
+                    if (log.isInfoEnabled()) {
+                        log.info(
+                            "No {} permission for user roles {} to System Indices {}",
+                            action,
+                            securityRoles,
+                            String.join(", ", getAllSystemIndices(requestedResolved))
+                        );
+                    }
+                    presponse.allowed = false;
+                    presponse.markComplete();
+                    return;
                 }
-                presponse.allowed = false;
-                presponse.markComplete();
-                return;
-            }
         }
 
         if (isActionAllowed(action)) {

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV6.java
@@ -448,7 +448,13 @@ public EvaluatedDlsFlsConfig getDlsFls(
             return new EvaluatedDlsFlsConfig(dlsQueries, flsFields, maskedFieldsMap);
         }
 
-        public boolean hasExplicitIndexPermission(Resolved resolved, User user, String[] actions, IndexNameExpressionResolver resolver, ClusterService cs) {
+        public boolean hasExplicitIndexPermission(
+            Resolved resolved,
+            User user,
+            String[] actions,
+            IndexNameExpressionResolver resolver,
+            ClusterService cs
+        ) {
             // TODO: Handle this scenario in V6 config
             return false;
         }

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
@@ -499,18 +499,21 @@ public boolean impliesClusterPermissionPermission(String action) {
 
         @Override
         public boolean hasExplicitClusterPermissionPermission(String action) {
-            return roles.stream()
-                .map(r -> matchExplicitly(r.clusterPerms))
-                .filter(m -> m.test(action))
-                .count() > 0;
+            return roles.stream().map(r -> matchExplicitly(r.clusterPerms)).filter(m -> m.test(action)).count() > 0;
         }
 
         private static WildcardMatcher matchExplicitly(final WildcardMatcher matcher) {
             return matcher == WildcardMatcher.ANY ? WildcardMatcher.NONE : matcher;
         }
 
         @Override
-        public boolean hasExplicitIndexPermission(final Resolved resolved, final User user, final String[] actions, final IndexNameExpressionResolver resolver, final ClusterService cs) {
+        public boolean hasExplicitIndexPermission(
+            final Resolved resolved,
+            final User user,
+            final String[] actions,
+            final IndexNameExpressionResolver resolver,
+            final ClusterService cs
+        ) {
 
             final Set<String> indicesForRequest = new HashSet<>(resolved.getAllIndicesResolved(cs, resolver));
             if (indicesForRequest.isEmpty()) {
@@ -520,11 +523,15 @@ public boolean hasExplicitIndexPermission(final Resolved resolved, final User us
 
             final Set<String> explicitlyAllowedIndices = roles.stream()
                 .map(role -> role.getAllResolvedPermittedIndices(resolved, user, actions, resolver, cs, SecurityRoles::matchExplicitly))
-                .flatMap(s -> s.stream())
+                .flatMap(Collection::stream)
                 .collect(Collectors.toSet());
 
             if (log.isDebugEnabled()) {
-                log.debug("ExplicitIndexPermission check indices for request {}, explicitly allowed indices {}", indicesForRequest.toString(), explicitlyAllowedIndices.toString());
+                log.debug(
+                    "ExplicitIndexPermission check indices for request {}, explicitly allowed indices {}",
+                    indicesForRequest.toString(),
+                    explicitlyAllowedIndices.toString()
+                );
             }
 
             indicesForRequest.removeAll(explicitlyAllowedIndices);

diff --git a/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java b/src/main/java/org/opensearch/security/securityconf/SecurityRoles.java
@@ -45,7 +45,13 @@ public interface SecurityRoles {
      * Determines if the actions are explicitly granted for indices
      * @return if all indices in the request have an explicit grant for all actions
      */
-    boolean hasExplicitIndexPermission(Resolved resolved, User user, String[] actions, IndexNameExpressionResolver resolver, ClusterService cs);
+    boolean hasExplicitIndexPermission(
+        Resolved resolved,
+        User user,
+        String[] actions,
+        IndexNameExpressionResolver resolver,
+        ClusterService cs
+    );
 
     Set<String> getRoleNames();
 

diff --git a/src/test/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluatorTest.java
@@ -27,23 +27,17 @@
 import org.opensearch.security.auditlog.AuditLog;
 import org.opensearch.security.resolver.IndexResolverReplacer;
 import org.opensearch.security.resolver.IndexResolverReplacer.Resolved;
-import org.opensearch.security.securityconf.ConfigModelV7;
 import org.opensearch.security.securityconf.SecurityRoles;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.tasks.Task;
 
-import java.util.List;
-import java.util.Set;
-
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.is;
 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;
-import static org.opensearch.security.support.ConfigConstants.SYSTEM_INDEX_PERMISSION;
 
 @RunWith(MockitoJUnitRunner.class)
 public class SecurityIndexAccessEvaluatorTest {
@@ -107,12 +101,12 @@ public void testUnprotectedActionOnRegularIndex_systemIndexDisabled() {
 
         // Action
         // final PrivilegesEvaluatorResponse response = evaluator.evaluate(
-        //     request,
-        //     null,
-        //     UNPROTECTED_ACTION,
-        //     resolved,
-        //     presponse,
-        //     securityRoles
+        // request,
+        // null,
+        // UNPROTECTED_ACTION,
+        // resolved,
+        // presponse,
+        // securityRoles
         // );
         // verifyNoInteractions(presponse);
         // assertThat(response, is(presponse));
@@ -127,12 +121,12 @@ public void testUnprotectedActionOnRegularIndex_systemIndexPermissionDisabled()
 
         // Action
         // final PrivilegesEvaluatorResponse response = evaluator.evaluate(
-        //     request,
-        //     null,
-        //     UNPROTECTED_ACTION,
-        //     resolved,
-        //     presponse,
-        //     securityRoles
+        // request,
+        // null,
+        // UNPROTECTED_ACTION,
+        // resolved,
+        // presponse,
+        // securityRoles
         // );
         // verifyNoInteractions(presponse);
         // assertThat(response, is(presponse));
@@ -147,12 +141,12 @@ public void testUnprotectedActionOnRegularIndex_systemIndexPermissionEnabled() {
 
         // Action
         // final PrivilegesEvaluatorResponse response = evaluator.evaluate(
-        //     request,
-        //     null,
-        //     UNPROTECTED_ACTION,
-        //     resolved,
-        //     presponse,
-        //     securityRoles
+        // request,
+        // null,
+        // UNPROTECTED_ACTION,
+        // resolved,
+        // presponse,
+        // securityRoles
         // );
         // verifyNoInteractions(presponse);
         // assertThat(response, is(presponse));
@@ -167,12 +161,12 @@ public void testUnprotectedActionOnSystemIndex_systemIndexDisabled() {
 
         // Action
         // final PrivilegesEvaluatorResponse response = evaluator.evaluate(
-        //     request,
-        //     null,
-        //     UNPROTECTED_ACTION,
-        //     resolved,
-        //     presponse,
-        //     securityRoles
+        // request,
+        // null,
+        // UNPROTECTED_ACTION,
+        // resolved,
+        // presponse,
+        // securityRoles
         // );
         // verifyNoInteractions(presponse);
         // assertThat(response, is(presponse));
@@ -187,12 +181,12 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionDisabled() {
 
         // Action
         // final PrivilegesEvaluatorResponse response = evaluator.evaluate(
-        //     request,
-        //     null,
-        //     UNPROTECTED_ACTION,
-        //     resolved,
-        //     presponse,
-        //     securityRoles
+        // request,
+        // null,
+        // UNPROTECTED_ACTION,
+        // resolved,
+        // presponse,
+        // securityRoles
         // );
         // verifyNoInteractions(presponse);
         // assertThat(response, is(presponse));
@@ -207,12 +201,12 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionEnabled_With
 
         // Action
         // final PrivilegesEvaluatorResponse response = evaluator.evaluate(
-        //     request,
-        //     null,
-        //     UNPROTECTED_ACTION,
-        //     resolved,
-        //     presponse,
-        //     securityRoles
+        // request,
+        // null,
+        // UNPROTECTED_ACTION,
+        // resolved,
+        // presponse,
+        // securityRoles
         // );
         // verify(presponse).markComplete();
         // assertThat(response, is(presponse));
@@ -230,12 +224,12 @@ public void testUnprotectedActionOnSystemIndex_systemIndexPermissionEnabled_With
 
         // Action
         // final PrivilegesEvaluatorResponse response = evaluator.evaluate(
-        //     request,
-        //     null,
-        //     UNPROTECTED_ACTION,
-        //     resolved,
-        //     presponse,
-        //     securityRoles
+        // request,
+        // null,
+        // UNPROTECTED_ACTION,
+        // resolved,
+        // presponse,
+        // securityRoles
         // );
         // assertThat(response, is(presponse));
         // unprotected action is not allowed on a system index

diff --git a/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java b/src/test/java/org/opensearch/security/securityconf/SecurityRolesPermissionsTest.java
@@ -30,7 +30,6 @@
 import java.util.AbstractMap.SimpleEntry;
 import java.util.Arrays;
 import java.util.Collection;
-import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.stream.Collectors;