refactor and test changes

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

Hide file tree

Showing 5 changed files with 42 additions and 14 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
@@ -134,8 +134,12 @@ protected void handleGet(final RestChannel channel, RestRequest request, Client
 
         String filterBy = request.param("filterBy", "all");
 
-        if (filterBy.equalsIgnoreCase("internal") || filterBy.equalsIgnoreCase("service")) {
-            userService.filterAccountsByType(configuration, filterBy);
+        if (filterBy != "all") {
+            try {
+                userService.filterAccountsByType(configuration, filterBy);
+            } catch (UserServiceException ex) {
+                badRequestResponse(channel, ex.getMessage());
+            }
         }
 
         // no specific resource requested, return complete config

diff --git a/src/main/java/org/opensearch/security/user/AccountType.java b/src/main/java/org/opensearch/security/user/AccountType.java
@@ -0,0 +1,6 @@
+package org.opensearch.security.user;
+
+public enum AccountType {
+    INTERNAL,
+    SERVICE
+}
diff --git a/src/main/java/org/opensearch/security/user/UserService.java b/src/main/java/org/opensearch/security/user/UserService.java
@@ -328,7 +328,12 @@ 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 filterAccountsByType(SecurityDynamicConfiguration<?> configuration, String requestedAccountType)
+        throws UserServiceException {
+        if (!requestedAccountType.equalsIgnoreCase(AccountType.INTERNAL.name())
+            && !requestedAccountType.equalsIgnoreCase(AccountType.SERVICE.name())) {
+            throw new UserServiceException("Invalid user type {} " + requestedAccountType);
+        }
         List<String> toBeRemoved = new ArrayList<>();
 
         for (Map.Entry<String, ?> entry : configuration.getCEntries().entrySet()) {
@@ -337,9 +342,9 @@ public void filterAccountsByType(SecurityDynamicConfiguration<?> configuration,
             final String accountName = entry.getKey();
             boolean isServiceAccount = Boolean.parseBoolean(accountAttributes.getOrDefault("service", "false").toString());
 
-            if (requestedAccountType.equalsIgnoreCase("internal") && isServiceAccount) {
+            if (requestedAccountType.equalsIgnoreCase(AccountType.INTERNAL.name()) && isServiceAccount) {
                 toBeRemoved.add(accountName);
-            } else if (requestedAccountType.equalsIgnoreCase("service") && isServiceAccount == false) {
+            } else if (requestedAccountType.equalsIgnoreCase(AccountType.SERVICE.name()) && isServiceAccount == false) {
                 toBeRemoved.add(accountName);
             }
         }

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
@@ -19,6 +19,7 @@
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.http.message.BasicHeader;
+import org.hamcrest.Matchers;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -151,13 +152,10 @@ public void testUserFilters() throws Exception {
         setup();
         rh.keystore = "restapi/kirk-keystore.jks";
         rh.sendAdminCertificate = true;
-
-        HttpResponse response;
-
-        response = rh.executePutRequest(ENDPOINT + "/internalusers/happyServiceDead", ENABLED_SERVICE_ACCOUNT_BODY);
-
         final int SERVICE_ACCOUNTS_IN_SETTINGS = 1;
         final int INTERNAL_ACCOUNTS_IN_SETTINGS = 19;
+        final String serviceAccountName = "JohnDoeService";
+        HttpResponse response;
 
         response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=internal");
 
@@ -168,12 +166,27 @@ public void testUserFilters() throws Exception {
         response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=service");
         Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode());
         list = DefaultObjectMapper.readTree(response.getBody());
-        Assert.assertEquals(SERVICE_ACCOUNTS_IN_SETTINGS, list.size());
+        assertThat(list, Matchers.emptyIterable());
 
-        response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=ssas");
+        response = rh.executePutRequest(ENDPOINT + "/internalusers/" + serviceAccountName, ENABLED_SERVICE_ACCOUNT_BODY);
+
+        // repeat assertions after adding the service account
+
+        response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=internal");
+
+        Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode());
+        list = DefaultObjectMapper.readTree(response.getBody());
+        Assert.assertEquals(INTERNAL_ACCOUNTS_IN_SETTINGS, list.size());
+
+        response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=service");
         Assert.assertEquals(response.getBody(), HttpStatus.SC_OK, response.getStatusCode());
         list = DefaultObjectMapper.readTree(response.getBody());
-        Assert.assertEquals(SERVICE_ACCOUNTS_IN_SETTINGS + INTERNAL_ACCOUNTS_IN_SETTINGS, list.size());
+        Assert.assertEquals(SERVICE_ACCOUNTS_IN_SETTINGS, list.size());
+        assertThat(response.findObjectInJson(serviceAccountName), Matchers.notNullValue());
+        assertThat(response.findValueInJson(serviceAccountName + ".attributes.service"), containsString("true"));
+
+        response = rh.executeGetRequest(ENDPOINT + "/internalusers?filterBy=ssas");
+        Assert.assertEquals(response.getBody(), HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
 
         response = rh.executeGetRequest(ENDPOINT + "/internalusers?wrongparameter=jhondoe");
         Assert.assertEquals(response.getBody(), HttpStatus.SC_BAD_REQUEST, response.getStatusCode());

diff --git a/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java b/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java
@@ -527,7 +527,7 @@ private void throwIfNotValueNode(final JsonNode node) {
             }
         }
 
-        private JsonNode findObjectInJson(final String jsonDotPath) {
+        public JsonNode findObjectInJson(final String jsonDotPath) {
             // Make sure its json / then parse it
             if (!isJsonContentType()) {
                 throw new RuntimeException("Response was expected to be JSON, body was: \n" + body);