#62 - add a few more tests and improve implementation. not finished yet.

src/main/java/no/cantara/cs/admin/ClientAdminResource.java

@@ -154,28 +154,24 @@ public Response putClient(@Context SecurityContext context, @PathParam("clientId
             return Response.status(Response.Status.FORBIDDEN).build();
         }
 
-        Client client;
+        Client updatedClient;
         try {
-            client = mapper.readValue(jsonRequest, Client.class);
+            updatedClient = mapper.readValue(jsonRequest, Client.class);
         } catch (IOException e) {
             log.error("Error parsing json. {}, json={}", e.getMessage(), jsonRequest);
             return Response.status(Response.Status.BAD_REQUEST).entity("Could not parse json.").build();
         }
 
-        if (!clientId.equals(client.clientId)) {
+        if (!clientId.equals(updatedClient.clientId)) {
             return Response.status(Response.Status.BAD_REQUEST).entity("Path parameter clientId '" + clientId +
-                    "' does not match body clientId: '" + client.clientId + "'").build();
+                    "' does not match body clientId: '" + updatedClient.clientId + "'").build();
         }
 
-        Client updatedClient = new Client(client.clientId, client.applicationConfigId, client.autoUpgrade);
-
         try {
             clientDao.saveClient(updatedClient);
-
             return mapResponseToJson(updatedClient);
         } catch (IllegalArgumentException e) {
-            return Response.status(Response.Status.BAD_REQUEST).entity(e.getMessage())
-                    .build();
+            return Response.status(Response.Status.BAD_REQUEST).entity(e.getMessage()).build();
         }
     }
 
@@ -188,7 +184,6 @@ private Response mapResponseToJson(Object response) {
             log.warn("Could not convert to Json {}", response.toString());
             return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
         }
-
         return Response.ok(jsonResult).build();
     }
 

src/main/java/no/cantara/cs/client/ClientResource.java

@@ -67,7 +67,7 @@ public Response registerClient(String json) {
             log.warn("Could not convert to Json {}", clientConfig.toString());
             return Response.status(Response.Status.INTERNAL_SERVER_ERROR).build();
         }
-        log.info("registered {}", clientConfig);
+        log.info("Registered new {}", clientConfig);
         return Response.ok(jsonResult).build();
     }
 
@@ -86,12 +86,6 @@ public Response sync(@PathParam("clientId") String clientId, String checkForUpda
             return Response.status(Response.Status.BAD_REQUEST).entity("Could not parse json.").build();
         }
 
-        if (clientSecretValidationEnabled && !clientService.validateClientSecret(checkForUpdateRequest)) {
-            log.warn("Invalid clientSecret={} for clientId={}. Returning {}",
-                    checkForUpdateRequest.clientSecret, checkForUpdateRequest.clientId, Response.Status.UNAUTHORIZED);
-            return Response.status(Response.Status.UNAUTHORIZED).entity("Invalid or mismatching client id and/or secret.").build();
-        }
-
         clientService.processEvents(clientId, checkForUpdateRequest.eventsStore);
 
 
@@ -102,6 +96,15 @@ public Response sync(@PathParam("clientId") String clientId, String checkForUpda
             return Response.status(Response.Status.PRECONDITION_FAILED).entity(msg).build();
         }
 
+        //note it might be sensible to move this check before checkForUpdatedClientConfig, but this will require a change to JAU CheckForUpdateHelper.
+        if (clientSecretValidationEnabled && !clientService.validateClientSecret(clientId, checkForUpdateRequest)) {
+                String invalidMsg = "Invalid or mismatching client id and/or secret.";
+                log.warn(invalidMsg + " clientId(pathparam)={}, clientId={}. Returning {}",
+                        clientId, checkForUpdateRequest.clientId, checkForUpdateRequest.clientSecret, Response.Status.UNAUTHORIZED);
+                return Response.status(Response.Status.UNAUTHORIZED).entity(invalidMsg).build();
+        }
+
+
         if (clientConfig.config == null) {
             log.trace("ClientConfig has not changed, return 204 No Content configLastChanged={}", checkForUpdateRequest.configLastChanged);
             return Response.status(Response.Status.NO_CONTENT).build();

src/main/java/no/cantara/cs/client/ClientService.java

@@ -115,11 +115,20 @@ public void processEvents(String clientId, ExtractedEventsStore eventsStore) {
         }
     }
 
-    boolean validateClientSecret(CheckForUpdateRequest checkForUpdateRequest) {
+    boolean validateClientSecret(String clientId, CheckForUpdateRequest checkForUpdateRequest) {
+        if (clientId == null || clientId.isEmpty()) {
+            return false;
+        }
+        if (!clientId.equals(checkForUpdateRequest.clientId)) {
+            return false;
+        }
+
         Client client = clientDao.getClient(checkForUpdateRequest.clientId);
-        if (client.clientSecret == null || client.clientSecret.isEmpty()) {
+        if (client == null || client.clientSecret == null || client.clientSecret.isEmpty()) {
             return false;
         }
+        log.debug("validateClientSecret:" + " clientId(pathparam)={}, clientId={}, clientSecret={}, clientSecret(db)={}.",
+                clientId, checkForUpdateRequest.clientId, checkForUpdateRequest.clientSecret, client.clientSecret);
         return Objects.equals(client.clientSecret, checkForUpdateRequest.clientSecret);
     }
 }

src/test/java/no/cantara/cs/admin/ApplicationResourceTest.java

@@ -87,7 +87,8 @@ public void testApplicationStatus() throws Exception {
 
         // Register and update client 2
         ClientConfig registerClientResponse = testServer.getConfigServiceClient().registerClient(new ClientRegistrationRequest(application.artifactId, "client-2-name"));
-        ClientConfig client2 = testServer.getConfigServiceClient().checkForUpdate(registerClientResponse.clientId, new CheckForUpdateRequest("force-updated-config"));
+        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest(registerClientResponse.clientId, "force-updated-config", registerClientResponse.clientSecret);
+        ClientConfig client2 = testServer.getConfigServiceClient().checkForUpdate(checkForUpdateRequest);
 
         ApplicationStatus applicationStatus = testServer.getAdminClient().getApplicationStatus(application.artifactId);
 

src/test/java/no/cantara/cs/admin/ClientAdminResourceEnvTest.java

@@ -3,11 +3,7 @@
 import com.jayway.restassured.http.ContentType;
 import no.cantara.cs.client.ClientResource;
 import no.cantara.cs.client.ConfigServiceAdminClient;
-import no.cantara.cs.dto.Application;
-import no.cantara.cs.dto.CheckForUpdateRequest;
-import no.cantara.cs.dto.ClientConfig;
-import no.cantara.cs.dto.ClientEnvironment;
-import no.cantara.cs.dto.ClientRegistrationRequest;
+import no.cantara.cs.dto.*;
 import no.cantara.cs.testsupport.ApplicationConfigBuilder;
 import no.cantara.cs.testsupport.TestServer;
 import org.testng.annotations.AfterClass;
@@ -66,8 +62,8 @@ public void testClientEnvAfterRegisterClient() throws Exception {
     public void testClientEnvAfterCheckForUpdate() throws IOException {
         HashMap<String, String> envInfo = new HashMap<>();
         envInfo.put("var2", "value2");
-        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest("force-new-config", envInfo);
-        testServer.getConfigServiceClient().checkForUpdate(this.clientConfig.clientId, checkForUpdateRequest);
+        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest(this.clientConfig.clientId, "force-new-config", this.clientConfig.clientSecret).withEnvInfo(envInfo);
+        testServer.getConfigServiceClient().checkForUpdate(checkForUpdateRequest);
 
         ClientEnvironment clientEnvironment = testServer.getAdminClient().getClientEnvironment(clientConfig.clientId);
         assertNotNull(clientEnvironment);

src/test/java/no/cantara/cs/admin/ClientAdminResourceStatusTest.java

@@ -3,12 +3,7 @@
 import com.jayway.restassured.http.ContentType;
 import no.cantara.cs.client.ClientResource;
 import no.cantara.cs.client.ConfigServiceAdminClient;
-import no.cantara.cs.dto.Application;
-import no.cantara.cs.dto.CheckForUpdateRequest;
-import no.cantara.cs.dto.ClientConfig;
-import no.cantara.cs.dto.ClientHeartbeatData;
-import no.cantara.cs.dto.ClientRegistrationRequest;
-import no.cantara.cs.dto.ClientStatus;
+import no.cantara.cs.dto.*;
 import no.cantara.cs.testsupport.ApplicationConfigBuilder;
 import no.cantara.cs.testsupport.TestServer;
 import org.testng.annotations.AfterClass;
@@ -70,8 +65,9 @@ public void testClientStatusAfterRegisterClient() throws Exception {
 
     @Test(dependsOnMethods = "testClientStatusAfterRegisterClient")
     public void testClientStatusAfterCheckForUpdate() throws IOException {
-        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest("force-new-config", new HashMap<>(), "checkForUpdateTags", "checkForUpdateName");
-        ClientConfig checkForUpdateResponse = testServer.getConfigServiceClient().checkForUpdate(this.clientConfig.clientId, checkForUpdateRequest);
+        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest(this.clientConfig.clientId, "force-new-config", this.clientConfig.clientSecret)
+                .withEnvInfo(new HashMap<>()).withTags("checkForUpdateTags").withClientName("checkForUpdateName");
+        ClientConfig checkForUpdateResponse = testServer.getConfigServiceClient().checkForUpdate(checkForUpdateRequest);
 
         ClientStatus clientStatus = testServer.getAdminClient().getClientStatus(clientConfig.clientId);
         assertEquals(clientStatus.client.clientId, checkForUpdateResponse.clientId);

src/test/java/no/cantara/cs/client/ChangeConfigForSpecificClientTest.java

@@ -1,11 +1,6 @@
 package no.cantara.cs.client;
 
-import no.cantara.cs.dto.Application;
-import no.cantara.cs.dto.ApplicationConfig;
-import no.cantara.cs.dto.CheckForUpdateRequest;
-import no.cantara.cs.dto.Client;
-import no.cantara.cs.dto.ClientConfig;
-import no.cantara.cs.dto.ClientRegistrationRequest;
+import no.cantara.cs.dto.*;
 import no.cantara.cs.testsupport.ApplicationConfigBuilder;
 import no.cantara.cs.testsupport.TestServer;
 import org.testng.annotations.AfterClass;
@@ -14,12 +9,9 @@
 
 import java.io.IOException;
 
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertNotEquals;
-import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.*;
 
 public class ChangeConfigForSpecificClientTest {
-
     private ConfigServiceClient configServiceClient;
     private Application application;
     private ConfigServiceAdminClient configServiceAdminClient;
@@ -32,13 +24,13 @@ public void setup() throws Exception {
         testServer = new TestServer(getClass());
         testServer.cleanAllData();
         testServer.start();
-        configServiceClient = testServer.getConfigServiceClient();
 
         configServiceAdminClient = testServer.getAdminClient();
         application = configServiceAdminClient.registerApplication(getClass().getSimpleName());
         configServiceAdminClient.createApplicationConfig(application, ApplicationConfigBuilder.createConfigDto("default-config", application));
 
         // Register client
+        configServiceClient = testServer.getConfigServiceClient();
         currentClientConfig = configServiceClient.registerClient(new ClientRegistrationRequest(application.artifactId));
         configServiceClient.saveApplicationState(currentClientConfig);
     }
@@ -65,16 +57,18 @@ public void testChangeConfigForSingleClient() throws IOException {
         assertNotNull(putClientResponse);
         assertEquals(putClientResponse.applicationConfigId, newConfig.getId());
         assertEquals(putClientResponse.clientId, client.clientId);
+        assertEquals(putClientResponse.clientSecret, client.clientSecret);
     }
 
     @Test(dependsOnMethods = "testChangeConfigForSingleClient")
     public void testCheckForUpdateWithNewClientSpecificConfig() throws Exception {
         String previousLastChanged = configServiceClient.getApplicationState().getProperty("lastChanged");
 
         // CheckForUpdate should return new config
-        ClientConfig checkForUpdateResponse = configServiceClient.checkForUpdate(this.currentClientConfig.clientId, new CheckForUpdateRequest(previousLastChanged));
+        CheckForUpdateRequest checkForUpdateRequest =
+                new CheckForUpdateRequest(this.currentClientConfig.clientId, previousLastChanged, this.currentClientConfig.clientSecret);
+        ClientConfig checkForUpdateResponse = configServiceClient.checkForUpdate(checkForUpdateRequest);
         assertNotNull(checkForUpdateResponse);
         assertNotEquals(checkForUpdateResponse.config.getId(), currentClientConfig.config.getId());
     }
-
 }

src/test/java/no/cantara/cs/client/CheckForUpdateTest.java

@@ -1,7 +1,12 @@
 package no.cantara.cs.client;
 
+import com.fasterxml.jackson.databind.ObjectMapper;
 import com.jayway.restassured.http.ContentType;
+import no.cantara.cs.dto.Application;
 import no.cantara.cs.dto.CheckForUpdateRequest;
+import no.cantara.cs.dto.ClientConfig;
+import no.cantara.cs.dto.ClientRegistrationRequest;
+import no.cantara.cs.testsupport.ApplicationConfigBuilder;
 import no.cantara.cs.testsupport.TestServer;
 import org.apache.http.HttpStatus;
 import org.testng.annotations.AfterClass;
@@ -15,9 +20,8 @@
 import static org.testng.Assert.fail;
 
 public class CheckForUpdateTest {
-
+    private static final ObjectMapper mapper = new ObjectMapper();
     private ConfigServiceClient configServiceClient;
-
     private TestServer testServer;
 
     @BeforeClass
@@ -50,16 +54,49 @@ public void testCheckForUpdateBrokenJson() {
                 .post(ClientResource.CLIENT_PATH + "/{clientId}/sync", 1);
     }
 
+    //HttpURLConnection.HTTP_PRECON_FAILED is used in JAU CheckForUpdateHelper.
     @Test
     public void testCheckForUpdateUnregisteredClient() throws IOException {
         try {
-            configServiceClient.checkForUpdate("non-existing-client-id", new CheckForUpdateRequest(""));
+            configServiceClient.checkForUpdate(new CheckForUpdateRequest("non-existing-client-id", "", null));
             fail();
         } catch (HttpException e) {
             assertEquals(e.getStatusCode(), HttpStatus.SC_PRECONDITION_FAILED);
         }
     }
 
+    @Test(enabled = false)  //TODO this test requires client.secret.validation.enabled=true, but default is false
+    public void testClientIdNotMatchingCredentialsReturnUnauthorized() throws Exception {
+        configServiceClient = testServer.getConfigServiceClient();
+        ConfigServiceAdminClient configServiceAdminClient = testServer.getAdminClient();
+        Application application = configServiceAdminClient.registerApplication(getClass().getSimpleName());
+        configServiceAdminClient.createApplicationConfig(application, ApplicationConfigBuilder.createConfigDto("default-config", application));
+
+        // Register client
+        ClientConfig currentClientConfig = configServiceClient.registerClient(new ClientRegistrationRequest(application.artifactId));
+        configServiceClient.saveApplicationState(currentClientConfig);
+
+        // Register another client
+        ClientConfig clientConfig2 = configServiceClient.registerClient(new ClientRegistrationRequest(application.artifactId));
+        configServiceClient.saveApplicationState(clientConfig2);
+
+
+        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest(currentClientConfig.clientId,
+                currentClientConfig.config.getLastChanged(), currentClientConfig.clientSecret);
+        String jsonRequest = mapper.writeValueAsString(checkForUpdateRequest);
+
+        given()
+                .auth().basic(TestServer.USERNAME, TestServer.PASSWORD)
+                .contentType(ContentType.JSON)
+                .body(jsonRequest)
+                .log().everything()
+                .expect()
+                .statusCode(HttpStatus.SC_UNAUTHORIZED)
+                .log().ifError()
+                .when()
+                .post(ClientResource.CLIENT_PATH + "/{clientId}/sync", clientConfig2.clientId); //Not the same clientId => should not be allowed
+    }
+
     @Test
     public void testCheckForUpdateRequestParsingBackwardCompatible() throws Exception {
         /*

src/test/java/no/cantara/cs/client/JauUseCaseTest.java

@@ -1,10 +1,6 @@
 package no.cantara.cs.client;
 
-import no.cantara.cs.dto.Application;
-import no.cantara.cs.dto.ApplicationConfig;
-import no.cantara.cs.dto.CheckForUpdateRequest;
-import no.cantara.cs.dto.ClientConfig;
-import no.cantara.cs.dto.ClientRegistrationRequest;
+import no.cantara.cs.dto.*;
 import no.cantara.cs.dto.event.EventExtractionConfig;
 import no.cantara.cs.testsupport.ApplicationConfigBuilder;
 import no.cantara.cs.testsupport.TestServer;
@@ -15,10 +11,7 @@
 import java.io.IOException;
 import java.util.List;
 
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertNotEquals;
-import static org.testng.Assert.assertNotNull;
-import static org.testng.Assert.assertNull;
+import static org.testng.Assert.*;
 
 /**
  * Verify endpoints used by JAU client.
@@ -68,7 +61,7 @@ public void testGetExtractionConfigs() throws IOException {
 
     @Test(dependsOnMethods = "startupAndRegisterClient")
     public void testCheckForUpdateWithUpToDateClientConfig() throws Exception {
-        ClientConfig clientConfig = configServiceClient.checkForUpdate(currentClientConfig.clientId, new CheckForUpdateRequest(currentClientConfig.config.getLastChanged()));
+        ClientConfig clientConfig = configServiceClient.checkForUpdate(new CheckForUpdateRequest(currentClientConfig.clientId, currentClientConfig.config.getLastChanged(), currentClientConfig.clientSecret));
         assertNull(clientConfig);
     }
 
@@ -85,7 +78,8 @@ public void testCheckForUpdateWhenCurrentConfigHasBeenChanged() throws Exception
         assertNotEquals(updateConfigResponse.getLastChanged(), currentClientConfig.config.getLastChanged());
 
         // CheckForUpdate should return updated config
-        ClientConfig checkForUpdateResponse = configServiceClient.checkForUpdate(this.currentClientConfig.clientId, new CheckForUpdateRequest(currentClientConfig.config.getLastChanged()));
+        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest(this.currentClientConfig.clientId, currentClientConfig.config.getLastChanged(), this.currentClientConfig.clientSecret);
+        ClientConfig checkForUpdateResponse = configServiceClient.checkForUpdate(checkForUpdateRequest);
         assertNotNull(checkForUpdateResponse);
         assertEquals(checkForUpdateResponse.config.getConfigurationStores().iterator().next().properties.get("new-property"), "new-value");
 
@@ -101,8 +95,8 @@ public void testCheckForUpdateWhenCurrentConfigHasBeenChanged() throws Exception
     public void testCheckForUpdateWithNewDefaultConfig() throws Exception {
         ApplicationConfig newDefaultConfig = configServiceAdminClient.createApplicationConfig(application, ApplicationConfigBuilder.createConfigDto("NewDefaultConfigTest", application));
 
-        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest(currentClientConfig.config.getLastChanged());
-        ClientConfig checkForUpdateResponse = configServiceClient.checkForUpdate(currentClientConfig.clientId, checkForUpdateRequest);
+        CheckForUpdateRequest checkForUpdateRequest = new CheckForUpdateRequest(currentClientConfig.clientId, currentClientConfig.config.getLastChanged(), currentClientConfig.clientSecret);
+        ClientConfig checkForUpdateResponse = configServiceClient.checkForUpdate(checkForUpdateRequest);
 
         assertNotNull(checkForUpdateResponse);
         assertEquals(checkForUpdateResponse.config.getId(), newDefaultConfig.getId());