Addresses most of the PR feedback

Signed-off-by: Darshit Chanpura <[email protected]>
samuelcostae · Sep 6, 2023 · c2ab7e6 · c2ab7e6
1 parent ffcf0b5
commit c2ab7e6
Show file tree

Hide file tree

Showing 2 changed files with 9 additions and 7 deletions.
diff --git a/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
@@ -82,7 +82,7 @@ public SecurityIndexAccessEvaluator(final Settings settings, AuditLog auditLog,
         this.systemIndexMatcher = WildcardMatcher.from(
             settings.getAsList(ConfigConstants.SECURITY_SYSTEM_INDICES_KEY, ConfigConstants.SECURITY_SYSTEM_INDICES_DEFAULT)
         );
-        this.protectedSystemIndexMatcher = WildcardMatcher.from(ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX);
+        this.protectedSystemIndexMatcher = WildcardMatcher.from(this.securityIndex);
         this.isSystemIndexEnabled = settings.getAsBoolean(
             ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_KEY,
             ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_DEFAULT
@@ -215,8 +215,10 @@ private boolean isActionAllowed(String action) {
      * @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 isDebugEnabled flag to indicate whether debug logging is enabled
      * @param securityRoles user's roles which will be used for access evaluation
+     * @param user this user's permissions will be looked up
+     * @param resolver the index expression resolver
+     * @param clusterService required to fetch cluster state metadata
      */
     private void evaluateSystemIndicesAccess(
         final String action,
@@ -289,10 +291,10 @@ private void evaluateSystemIndicesAccess(
                     presponse.allowed = false;
                     presponse.markComplete();
                 }
-            } else if (containsSystemIndex && !isSystemIndexPermissionEnabled) {
-                // if system index is enabled and system index permissions are enabled we don't need to perform any further
-                // checks as it has already been performed via isSystemIndexAccessProhibitedForUser
-
+            }
+            // if system index is enabled and system index permissions are enabled we don't need to perform any further
+            // checks as it has already been performed via hasExplicitIndexPermission
+            else if (containsSystemIndex && !isSystemIndexPermissionEnabled) {
                 if (filterSecurityIndex) {
                     Set<String> allWithoutSecurity = new HashSet<>(requestedResolved.getAllIndices());
                     allWithoutSecurity.remove(securityIndex);

diff --git a/src/test/java/org/opensearch/security/IntegrationTests.java b/src/test/java/org/opensearch/security/IntegrationTests.java
@@ -1047,7 +1047,7 @@ public void testSecurityIndexSecurity() throws Exception {
         );
         Assert.assertEquals(HttpStatus.SC_FORBIDDEN, res.getStatusCode());
         res = rh.executePostRequest(".opendistro_security/_freeze", "", encodeBasicHeader("nagilum", "nagilum"));
-        Assert.assertTrue(res.getStatusCode() >= 400);
+        Assert.assertEquals(400, res.getStatusCode());
 
         String bulkBody = "{ \"index\" : { \"_index\" : \".opendistro_security\", \"_id\" : \"1\" } }\n"
             + "{ \"field1\" : \"value1\" }\n"