diff --git a/src/main/java/no/cantara/cs/admin/ClientAdminResource.java b/src/main/java/no/cantara/cs/admin/ClientAdminResource.java index a7c8a13b..898b0ddf 100644 --- a/src/main/java/no/cantara/cs/admin/ClientAdminResource.java +++ b/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(); } diff --git a/src/main/java/no/cantara/cs/client/ClientResource.java b/src/main/java/no/cantara/cs/client/ClientResource.java index 8a30733d..6f4318f1 100644 --- a/src/main/java/no/cantara/cs/client/ClientResource.java +++ b/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(); diff --git a/src/main/java/no/cantara/cs/client/ClientService.java b/src/main/java/no/cantara/cs/client/ClientService.java index a1650f48..a1500e81 100644 --- a/src/main/java/no/cantara/cs/client/ClientService.java +++ b/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); } } diff --git a/src/test/java/no/cantara/cs/admin/ApplicationResourceTest.java b/src/test/java/no/cantara/cs/admin/ApplicationResourceTest.java index 1ddd5264..e0f4cd82 100644 --- a/src/test/java/no/cantara/cs/admin/ApplicationResourceTest.java +++ b/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); diff --git a/src/test/java/no/cantara/cs/admin/ClientAdminResourceEnvTest.java b/src/test/java/no/cantara/cs/admin/ClientAdminResourceEnvTest.java index 58674c9f..a7a7ca9a 100644 --- a/src/test/java/no/cantara/cs/admin/ClientAdminResourceEnvTest.java +++ b/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 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); diff --git a/src/test/java/no/cantara/cs/admin/ClientAdminResourceStatusTest.java b/src/test/java/no/cantara/cs/admin/ClientAdminResourceStatusTest.java index 5ce18f1d..a2f2b6ec 100644 --- a/src/test/java/no/cantara/cs/admin/ClientAdminResourceStatusTest.java +++ b/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); diff --git a/src/test/java/no/cantara/cs/client/ChangeConfigForSpecificClientTest.java b/src/test/java/no/cantara/cs/client/ChangeConfigForSpecificClientTest.java index 636acaf1..82861b03 100644 --- a/src/test/java/no/cantara/cs/client/ChangeConfigForSpecificClientTest.java +++ b/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,6 +57,7 @@ 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") @@ -72,9 +65,10 @@ 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()); } - } diff --git a/src/test/java/no/cantara/cs/client/CheckForUpdateTest.java b/src/test/java/no/cantara/cs/client/CheckForUpdateTest.java index 04f92f45..37774a38 100644 --- a/src/test/java/no/cantara/cs/client/CheckForUpdateTest.java +++ b/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 { /* diff --git a/src/test/java/no/cantara/cs/client/JauUseCaseTest.java b/src/test/java/no/cantara/cs/client/JauUseCaseTest.java index cab8a93d..baf0eca8 100644 --- a/src/test/java/no/cantara/cs/client/JauUseCaseTest.java +++ b/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());