Merge pull request from GHSA-vf78-3q9f-92g3

MODEXPS-226 - Invalid system user credentials usage
folio-org · Jul 20, 2023 · 93aff45 · 93aff45
2 parents f5fe775 + 1ddebf6
commit 93aff45
Show file tree

Hide file tree

Showing 12 changed files with 130 additions and 11 deletions.
diff --git a/README.md b/README.md
@@ -20,6 +20,8 @@ API for Data Export Spring module.
 | KAFKA_HOST                    | kafka                     | Kafka broker hostname                                             |
 | KAFKA_PORT                    | 9092                      | Kafka broker port                                                 |
 | OKAPI_URL                     | http://okapi:9130         | Okapi url                                                         |
+| SYSTEM\_USER\_NAME            | data-export-system-user   | Username of the system user                                       |
+| SYSTEM\_USER\_PASSWORD        | -                         | Password of the system user                                       |
 | ENV                           | folio                     | Logical name of the deployment, must be set if Kafka/Elasticsearch are shared for environments, `a-z (any case)`, `0-9`, `-`, `_` symbols only allowed|
 
 
@@ -65,7 +67,7 @@ Before running scheduled task(job) there is check, that module is registered for
 
 Tenant information need to define DB schema for storing information about Job and etc.
 
-In the post tenant API controller specific user (`data-export-system-user`) is created for running scheduled export tasks. Permissions are defined in `src/main/resources/permissions/system-user-permissions.csv`.
+The `data-export-system-user` system user for running scheduled export tasks is created in the post tenant API controller. The password must be set using the `SYSTEM_USER_PASSWORD` environment variable. Permissions are defined in `src/main/resources/permissions/system-user-permissions.csv`.
 
 Also Okapi headers, system user, tenant information are s-tored in memory in a FolioExecutionContext.
 

diff --git a/descriptors/ModuleDescriptor-template.json b/descriptors/ModuleDescriptor-template.json
@@ -167,6 +167,7 @@
             "users.item.post",
             "users.item.put",
             "login.item.post",
+            "login.item.delete",
             "perms.users.item.post",
             "perms.users.get",
             "configuration.entries.collection.get",

diff --git a/src/main/java/org/folio/des/ModDataExportSpringApplication.java b/src/main/java/org/folio/des/ModDataExportSpringApplication.java
@@ -1,5 +1,6 @@
 package org.folio.des;
 
+import org.apache.commons.lang3.StringUtils;
 import org.folio.de.entity.JobCommand;
 import org.springframework.boot.SpringApplication;
 import org.springframework.boot.autoconfigure.SpringBootApplication;
@@ -10,8 +11,13 @@
 @EnableFeignClients
 @EntityScan(basePackageClasses = JobCommand.class)
 public class ModDataExportSpringApplication {
+  public static final String SYSTEM_USER_PASSWORD = "SYSTEM_USER_PASSWORD";
 
   public static void main(String[] args) {
+    if (StringUtils.isEmpty(System.getenv(SYSTEM_USER_PASSWORD))) {
+      throw new IllegalArgumentException("Required environment variable is missing: " + SYSTEM_USER_PASSWORD);
+    }
+
     SpringApplication.run(ModDataExportSpringApplication.class, args);
   }
 

diff --git a/src/main/java/org/folio/des/client/AuthClient.java b/src/main/java/org/folio/des/client/AuthClient.java
@@ -4,8 +4,10 @@
 import org.springframework.cloud.openfeign.FeignClient;
 import org.springframework.http.MediaType;
 import org.springframework.http.ResponseEntity;
+import org.springframework.web.bind.annotation.DeleteMapping;
 import org.springframework.web.bind.annotation.PostMapping;
 import org.springframework.web.bind.annotation.RequestBody;
+import org.springframework.web.bind.annotation.RequestParam;
 
 @FeignClient("authn")
 public interface AuthClient {
@@ -15,4 +17,7 @@ public interface AuthClient {
 
   @PostMapping(value = "/credentials", consumes = MediaType.APPLICATION_JSON_VALUE)
   void saveCredentials(@RequestBody SystemUserParameters systemUserParameters);
+
+  @DeleteMapping(value = "/credentials", consumes = MediaType.APPLICATION_JSON_VALUE)
+  void deleteCredentials(@RequestParam("userId") String userId);
 }
diff --git a/src/main/java/org/folio/des/security/AuthService.java b/src/main/java/org/folio/des/security/AuthService.java
@@ -25,14 +25,16 @@ public class AuthService {
 
   @Value("${folio.system.username}")
   private String username;
+  @Value("${folio.system.password}")
+  private String password;
 
   public String getTokenForSystemUser(String tenant, String url) {
     SystemUserParameters userParameters =
         SystemUserParameters.builder()
             .okapiUrl(url)
             .tenantId(tenant)
             .username(username)
-            .password(username)
+            .password(password)
             .build();
 
     log.info("Attempt login with url={} tenant={} username={}.", url, tenant, username);
@@ -63,6 +65,12 @@ private boolean isNotEmpty(java.util.List<String> token) {
     return CollectionUtils.isNotEmpty(token) && StringUtils.isNotBlank(token.get(0));
   }
 
+  public void deleteCredentials(String userId) {
+    authClient.deleteCredentials(userId);
+
+    log.info("Removed credentials for user {}.", userId);
+  }
+
   public void saveCredentials(SystemUserParameters systemUserParameters) {
     authClient.saveCredentials(systemUserParameters);
 

diff --git a/src/main/java/org/folio/des/security/SecurityManagerService.java b/src/main/java/org/folio/des/security/SecurityManagerService.java
@@ -35,6 +35,8 @@ public class SecurityManagerService {
 
   @Value("${folio.system.username}")
   private String username;
+  @Value("${folio.system.password}")
+  private String password;
 
   public void prepareSystemUser(String okapiUrl, String tenantId) {
     Optional<User> userOptional = getUser(username);
@@ -45,15 +47,21 @@ public void prepareSystemUser(String okapiUrl, String tenantId) {
       updateUser(user);
     } else {
       user = createUser(username);
-      authService.saveCredentials(SystemUserParameters.builder()
-          .id(UUID.randomUUID())
-          .username(username)
-          .password(username)
-          .okapiUrl(okapiUrl)
-          .tenantId(tenantId)
-          .build());
     }
 
+    try {
+      authService.deleteCredentials(user.getId());
+    } catch (feign.FeignException.NotFound e) {
+      // ignore if not exist
+    }
+    authService.saveCredentials(SystemUserParameters.builder()
+        .id(UUID.randomUUID())
+        .username(username)
+        .password(password)
+        .okapiUrl(okapiUrl)
+        .tenantId(tenantId)
+        .build());
+
     Optional<PermissionUser> permissionUserOptional = permissionsClient.get("userId==" + user.getId())
         .getPermissionUsers()
         .stream()

diff --git a/src/main/resources/application.yml b/src/main/resources/application.yml
@@ -1,6 +1,7 @@
 folio:
   system:
-    username: data-export-system-user
+    username: ${SYSTEM_USER_NAME:data-export-system-user}
+    password: ${SYSTEM_USER_PASSWORD}
   okapi:
     url: ${OKAPI_URL:http://okapi:9130}
   tenant:

diff --git a/src/test/java/org/folio/des/InstallUpgradeIT.java b/src/test/java/org/folio/des/InstallUpgradeIT.java
@@ -94,7 +94,8 @@ class InstallUpgradeIT {
     .withEnv("DB_PASSWORD", "password")
     .withEnv("DB_DATABASE", "postgres")
     .withEnv("KAFKA_HOST", "mykafka")
-    .withEnv("KAFKA_PORT", "9092");
+    .withEnv("KAFKA_PORT", "9092")
+    .withEnv("SYSTEM_USER_PASSWORD", "password");
 
   private static void mockPath(MockServerClient mockServerClient, String path, String jsonBody) {
     mockServerClient.when(request(path))

diff --git a/src/test/java/org/folio/des/ModDataExportSpringApplicationTest.java b/src/test/java/org/folio/des/ModDataExportSpringApplicationTest.java
@@ -0,0 +1,17 @@
+package org.folio.des;
+
+import static org.hamcrest.CoreMatchers.containsString;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertThrows;
+
+import org.junit.jupiter.api.Test;
+
+class ModDataExportSpringApplicationTest {
+
+  @Test
+  void exceptionOnMissingSystemUserPassword() {
+    var e = assertThrows(IllegalArgumentException.class, () -> ModDataExportSpringApplication.main(null));
+    assertThat(e.getMessage(), containsString(ModDataExportSpringApplication.SYSTEM_USER_PASSWORD));
+  }
+
+}
diff --git a/src/test/java/org/folio/des/security/SecurityManagerServiceTest.java b/src/test/java/org/folio/des/security/SecurityManagerServiceTest.java
@@ -1,8 +1,11 @@
 package org.folio.des.security;
 
 import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
+import static com.github.tomakehurst.wiremock.client.WireMock.delete;
+import static com.github.tomakehurst.wiremock.client.WireMock.deleteRequestedFor;
 import static com.github.tomakehurst.wiremock.client.WireMock.get;
 import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor;
+import static com.github.tomakehurst.wiremock.client.WireMock.postRequestedFor;
 import static com.github.tomakehurst.wiremock.client.WireMock.putRequestedFor;
 import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
 
@@ -14,6 +17,7 @@
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.http.HttpStatus;
 import org.springframework.http.MediaType;
 
 import java.util.Collection;
@@ -75,6 +79,56 @@ void prepareSystemUser() {
                     .withHeader("Content-Type", MediaType.APPLICATION_JSON_VALUE)
                     .withBody(USER_PERMS_RESPONSE)));
 
+    wireMockServer.stubFor(
+      delete(urlEqualTo("/authn/credentials?userId=a85c45b7-d427-4122-8532-5570219c5e59"))
+        .willReturn(
+          aResponse()
+            .withStatus(HttpStatus.NO_CONTENT.value())));
+
+    Map<String, Collection<String>> tenantOkapiHeaders = new HashMap<>() {{
+      put(XOkapiHeaders.TENANT, List.of(TENANT));
+      put(XOkapiHeaders.URL, List.of(wireMockServer.baseUrl()));
+      put(XOkapiHeaders.TOKEN, List.of(TOKEN));
+    }};
+
+    try (var context = new FolioExecutionContextSetter(new DefaultFolioExecutionContext(folioModuleMetadata, tenantOkapiHeaders))) {
+      securityManagerService.prepareSystemUser(wireMockServer.baseUrl(), TENANT);
+    }
+
+    wireMockServer.verify(
+        getRequestedFor(urlEqualTo("/users?query=username%3D%3Ddata-export-system-user")));
+    wireMockServer.verify(
+        putRequestedFor(urlEqualTo("/users/a85c45b7-d427-4122-8532-5570219c5e59")));
+    wireMockServer.verify(
+        deleteRequestedFor(urlEqualTo("/authn/credentials?userId=a85c45b7-d427-4122-8532-5570219c5e59")));
+    wireMockServer.verify(
+        postRequestedFor(urlEqualTo("/authn/credentials")));
+  }
+
+  @Test
+  @DisplayName("Update user without previous password")
+  void prepareSystemUserWithoutPreviousPassword() {
+
+    wireMockServer.stubFor(
+        get(urlEqualTo("/users?query=username%3D%3Ddata-export-system-user"))
+            .willReturn(
+                aResponse()
+                    .withHeader("Content-Type", MediaType.APPLICATION_JSON_VALUE)
+                    .withBody(SYS_USER_EXIST_RESPONSE)));
+
+    wireMockServer.stubFor(
+        get(urlEqualTo("/perms/users?query=userId%3D%3Da85c45b7-d427-4122-8532-5570219c5e59"))
+            .willReturn(
+                aResponse()
+                    .withHeader("Content-Type", MediaType.APPLICATION_JSON_VALUE)
+                    .withBody(USER_PERMS_RESPONSE)));
+
+    wireMockServer.stubFor(
+      delete(urlEqualTo("/authn/credentials?userId=a85c45b7-d427-4122-8532-5570219c5e59"))
+        .willReturn(
+          aResponse()
+            .withStatus(HttpStatus.NOT_FOUND.value())));
+
     Map<String, Collection<String>> tenantOkapiHeaders = new HashMap<>() {{
       put(XOkapiHeaders.TENANT, List.of(TENANT));
       put(XOkapiHeaders.URL, List.of(wireMockServer.baseUrl()));
@@ -89,5 +143,9 @@ void prepareSystemUser() {
         getRequestedFor(urlEqualTo("/users?query=username%3D%3Ddata-export-system-user")));
     wireMockServer.verify(
         putRequestedFor(urlEqualTo("/users/a85c45b7-d427-4122-8532-5570219c5e59")));
+    wireMockServer.verify(
+        deleteRequestedFor(urlEqualTo("/authn/credentials?userId=a85c45b7-d427-4122-8532-5570219c5e59")));
+    wireMockServer.verify(
+        postRequestedFor(urlEqualTo("/authn/credentials")));
   }
 }
diff --git a/src/test/resources/config/application.yml b/src/test/resources/config/application.yml
@@ -0,0 +1,3 @@
+folio:
+  system:
+    password: testpassword
diff --git a/src/test/resources/mappings/authn.json b/src/test/resources/mappings/authn.json
@@ -25,6 +25,15 @@
         }
       }
     },
+    {
+      "request": {
+        "method": "DELETE",
+        "url": "/authn/credentials"
+      },
+      "response": {
+        "status": 204
+      }
+    },
     {
       "request": {
         "method": "POST",