refactor and test changes

Signed-off-by: Sam <[email protected]>
samuelcostae · Aug 30, 2023 · be44715 · be44715
1 parent 71b83f1
commit be44715
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 23 deletions.
diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
@@ -45,6 +45,7 @@
 import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
 import org.opensearch.security.ssl.transport.PrincipalExtractor;
 import org.opensearch.security.support.SecurityJsonNode;
+import org.opensearch.security.user.AccountType;
 import org.opensearch.security.user.UserService;
 import org.opensearch.security.user.UserServiceException;
 import org.opensearch.threadpool.ThreadPool;
@@ -132,11 +133,11 @@ protected void handleGet(final RestChannel channel, RestRequest request, Client
         SecurityDynamicConfiguration<?> configuration = load(getConfigName(), true);
         filter(configuration);
 
-        String filterBy = request.param("filterBy", "all");
+        String includeIfType = request.param("filterBy", "any");
 
-        if (filterBy != "all") {
+        if (includeIfType != "any") {
             try {
-                userService.filterAccountsByType(configuration, filterBy);
+                userService.includeAccountsIfType(configuration, AccountType.valueOf(includeIfType.toUpperCase()));
             } catch (UserServiceException ex) {
                 badRequestResponse(channel, ex.getMessage());
             }
@@ -160,7 +161,7 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
         throws IOException {
 
         final String username = request.param("name");
-
+        final boolean service = content.get("service").asBoolean();
         SecurityDynamicConfiguration<?> internalUsersConfiguration = load(getConfigName(), false);
 
         if (!isWriteable(channel, internalUsersConfiguration, username)) {
@@ -185,8 +186,9 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
         // when updating an existing user password hash can be blank, which means no
         // changes
 
+
         try {
-            if (request.hasParam("service")) {
+            if (service) {
                 ((ObjectNode) content).put("service", request.param("service"));
             }
             if (request.hasParam("enabled")) {
@@ -365,7 +367,8 @@ public Map<String, RequestContentValidator.DataType> allowedKeys() {
                     .put("opendistro_security_roles", DataType.ARRAY)
                     .put("hash", DataType.STRING)
                     .put("password", DataType.STRING)
-                    .build();
+                    .put("service", DataType.STRING)
+                     .build();
             }
         });
     }

diff --git a/src/main/java/org/opensearch/security/user/AccountType.java b/src/main/java/org/opensearch/security/user/AccountType.java
@@ -1,6 +1,26 @@
 package org.opensearch.security.user;
 
 public enum AccountType {
-    INTERNAL,
-    SERVICE
+
+    INTERNAL("internal"),
+    SERVICE("service");
+
+    private String name;
+
+    AccountType(String name){
+        this.name = name;
+    }
+
+    public String getName() {
+        return this.name;
+    }
+    public static AccountType fromString(String name) {
+        for (AccountType b : AccountType.values()) {
+            if (b.name.equalsIgnoreCase(name)) {
+                return b;
+            }
+        }
+        return null;
+    }
+
 }
diff --git a/src/main/java/org/opensearch/security/user/UserService.java b/src/main/java/org/opensearch/security/user/UserService.java
@@ -328,26 +328,38 @@ public static void saveAndUpdateConfigs(
      * @param requestedAccountType The type of account to be kept. Should be "service" or "internal"
      *
      */
-    public void filterAccountsByType(SecurityDynamicConfiguration<?> configuration, String requestedAccountType)
+    public void includeAccountsIfType(SecurityDynamicConfiguration<?> configuration, AccountType requestedAccountType)
         throws UserServiceException {
-        if (!requestedAccountType.equalsIgnoreCase(AccountType.INTERNAL.name())
-            && !requestedAccountType.equalsIgnoreCase(AccountType.SERVICE.name())) {
+        if (requestedAccountType != AccountType.INTERNAL && requestedAccountType != AccountType.SERVICE) {
             throw new UserServiceException("Invalid user type {} " + requestedAccountType);
         }
         List<String> toBeRemoved = new ArrayList<>();
 
-        for (Map.Entry<String, ?> entry : configuration.getCEntries().entrySet()) {
-            final InternalUserV7 internalUserEntry = (InternalUserV7) entry.getValue();
-            final Map accountAttributes = internalUserEntry.getAttributes();
-            final String accountName = entry.getKey();
-            boolean isServiceAccount = Boolean.parseBoolean(accountAttributes.getOrDefault("service", "false").toString());
+        if (requestedAccountType == AccountType.SERVICE) {
+            for (Map.Entry<String, ?> entry : configuration.getCEntries().entrySet()) {
+                final InternalUserV7 internalUserEntry = (InternalUserV7) entry.getValue();
+                final Map accountAttributes = internalUserEntry.getAttributes();
+                final String accountName = entry.getKey();
+                final boolean isServiceAccount = Boolean.parseBoolean(accountAttributes.getOrDefault("service", "false").toString());
 
-            if (requestedAccountType.equalsIgnoreCase(AccountType.INTERNAL.name()) && isServiceAccount) {
-                toBeRemoved.add(accountName);
-            } else if (requestedAccountType.equalsIgnoreCase(AccountType.SERVICE.name()) && isServiceAccount == false) {
-                toBeRemoved.add(accountName);
+                if (isServiceAccount == false) {
+                    toBeRemoved.add(accountName);
+                }
+            }
+        }
+        else if (requestedAccountType == AccountType.INTERNAL) {
+
+            for (Map.Entry<String, ?> entry : configuration.getCEntries().entrySet()) {
+                final InternalUserV7 internalUserEntry = (InternalUserV7) entry.getValue();
+                final Map accountAttributes = internalUserEntry.getAttributes();
+                final String accountName = entry.getKey();
+                final boolean isServiceAccount = Boolean.parseBoolean(accountAttributes.getOrDefault("service", "false").toString());
+
+                if (isServiceAccount == true) {
+                    toBeRemoved.add(accountName);
+                }
             }
         }
         configuration.remove(toBeRemoved);
-    }
+        }
 }
diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java
@@ -58,7 +58,8 @@ protected String getEndpointPrefix() {
 
     private static final String ENABLED_SERVICE_ACCOUNT_BODY = "{"
         + " \"attributes\": { \"service\": \"true\", "
-        + "\"enabled\": \"true\"}"
+        + " \"enabled   \": \"true\"},"
+        + " \"service\": \"true\" "
         + " }\n";
 
     private static final String DISABLED_SERVICE_ACCOUNT_BODY = "{"
@@ -157,6 +158,8 @@ public void testUserFilters() throws Exception {
         final String serviceAccountName = "JohnDoeService";
         HttpResponse response;
 
+
+
         response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=internal");
 
         Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode());
@@ -170,6 +173,7 @@ public void testUserFilters() throws Exception {
 
         response = rh.executePutRequest(ENDPOINT + "/internalusers/" + serviceAccountName, ENABLED_SERVICE_ACCOUNT_BODY);
 
+
         // repeat assertions after adding the service account
 
         response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=internal");
@@ -182,7 +186,7 @@ public void testUserFilters() throws Exception {
         Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode());
         list = DefaultObjectMapper.readTree(response.getBody());
         Assert.assertEquals(SERVICE_ACCOUNTS_IN_SETTINGS, list.size());
-        assertThat(response.findObjectInJson(serviceAccountName), Matchers.notNullValue());
+        assertThat(response.findValueInJson(serviceAccountName), Matchers.notNullValue());
         assertThat(response.findValueInJson(serviceAccountName + ".attributes.service"), containsString("true"));
 
         response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=ssas");