Resovle review comments

Signed-off-by: cliu123 <[email protected]>
opensearch-project · Jun 2, 2022 · 5e1ff85 · 5e1ff85
1 parent 4fba30e
commit 5e1ff85
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 23 deletions.
diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java
@@ -18,6 +18,7 @@
 import java.io.IOException;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.Set;
 
 import com.fasterxml.jackson.databind.JsonNode;
 import com.google.common.collect.ImmutableList;
@@ -113,23 +114,20 @@ protected void handlePut(RestChannel channel, RestRequest request, Client client
 		}
 
 		// Prevent the case where action group and role share a same name.
-		SecurityDynamicConfiguration<?> existingRoles = load(CType.ROLES, false);
-		for (String role : existingRoles.getCEntries().keySet()) {
-			if (role.equals(name)) {
-				badRequestResponse(channel, name + " is an existing role. A action group cannot be named with an existing role name.");
-				return;
-			}
+		SecurityDynamicConfiguration<?> existingRolesConfig = load(CType.ROLES, false);
+		Set<String> existingRoles = existingRolesConfig.getCEntries().keySet();
+		if (existingRoles.contains(name)) {
+			badRequestResponse(channel, name + " is an existing role. A action group cannot be named with an existing role name.");
+			return;
 		}
 
 		// Prevent the case where action group references to itself in the allowed_actions.
-		final SecurityDynamicConfiguration<?> existingActionGroups = load(getConfigName(), false);
-		existingActionGroups.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroups.getImplementingClass()));
-
-		for (String allowed_action : ((ActionGroupsV7) existingActionGroups.getCEntry(name)).getAllowed_actions()) {
-			if (allowed_action.equals(name)) {
-				badRequestResponse(channel, name + " cannot be an allowed_action of itself");
-				return;
-			}
+		final SecurityDynamicConfiguration<?> existingActionGroupsConfig = load(getConfigName(), false);
+		existingActionGroupsConfig.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass()));
+		List<String> allowedActions = ((ActionGroupsV7) existingActionGroupsConfig.getCEntry(name)).getAllowed_actions();
+		if (allowedActions.contains(name)) {
+			badRequestResponse(channel, name + " cannot be an allowed_action of itself");
+			return;
 		}
 
 		super.handlePut(channel, request, client, content);

diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java
@@ -19,6 +19,7 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
 import java.util.Iterator;
+import java.util.List;
 
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.JsonNode;
@@ -159,7 +160,7 @@ private void handleSinglePatch(RestChannel channel, RestRequest request, Client
                                    , existingConfiguration.getVersion(), existingConfiguration.getSeqNo(), existingConfiguration.getPrimaryTerm());
 
         if (existingConfiguration.getCType().equals(CType.ACTIONGROUPS)) {
-            if(selfReferencingActionGroup(mdc, name)) {
+            if(hasActionGroupSelfReference(mdc, name)) {
                 badRequestResponse(channel, name + " cannot be an allowed_action of itself");
                 return;
             }
@@ -235,7 +236,7 @@ private void handleBulkPatch(RestChannel channel, RestRequest request, Client cl
 
         if (existingConfiguration.getCType().equals(CType.ACTIONGROUPS)) {
             for (String actiongroup : mdc.getCEntries().keySet()) {
-                if(selfReferencingActionGroup(mdc, actiongroup)) {
+                if(hasActionGroupSelfReference(mdc, actiongroup)) {
                     badRequestResponse(channel, actiongroup + " cannot be an allowed_action of itself");
                     return;
                 }
@@ -280,12 +281,8 @@ private AbstractConfigurationValidator getValidator(RestRequest request, JsonNod
     }
 
     // Prevent the case where action group references to itself in the allowed_actions.
-    private Boolean selfReferencingActionGroup(SecurityDynamicConfiguration<?> mdc, String name) {
-        for (String allowed_action : ((ActionGroupsV7) mdc.getCEntry(name)).getAllowed_actions()) {
-            if (allowed_action.equals(name)) {
-                return true;
-            }
-        }
-        return false;
+    private Boolean hasActionGroupSelfReference(SecurityDynamicConfiguration<?> mdc, String name) {
+        List<String> allowedActions = ((ActionGroupsV7) mdc.getCEntry(name)).getAllowed_actions();
+        return allowedActions.contains(name);
     }
 }