From 8f91c217fc7c6ef8f7b454dd40e4656737617859 Mon Sep 17 00:00:00 2001 From: Eduard Heimbuch Date: Wed, 28 Apr 2021 08:47:29 +0200 Subject: [PATCH] Add patch endpoint for global config (#1629) Co-authored-by: Sebastian Sdorra --- gradle/changelog/config_patch_endpoint.yaml | 2 + gradle/dependencies.gradle | 3 +- .../main/java/sonia/scm/util/JsonMerger.java | 214 ++++++++++++++++ .../java/sonia/scm/util/JsonMergerTest.java | 236 ++++++++++++++++++ scm-test/build.gradle | 1 + .../scm/api/v2/resources/ConfigResource.java | 88 +++++-- .../java/sonia/scm/web/i18n/I18nServlet.java | 49 +--- .../api/v2/resources/ConfigResourceTest.java | 151 ++++++----- .../sonia/scm/web/i18n/I18nServletTest.java | 9 +- 9 files changed, 633 insertions(+), 120 deletions(-) create mode 100644 gradle/changelog/config_patch_endpoint.yaml create mode 100644 scm-core/src/main/java/sonia/scm/util/JsonMerger.java create mode 100644 scm-core/src/test/java/sonia/scm/util/JsonMergerTest.java diff --git a/gradle/changelog/config_patch_endpoint.yaml b/gradle/changelog/config_patch_endpoint.yaml new file mode 100644 index 0000000000..a59b95606e --- /dev/null +++ b/gradle/changelog/config_patch_endpoint.yaml @@ -0,0 +1,2 @@ +- type: added + description: Patch endpoint for global configuration ([#1629](https://github.com/scm-manager/scm-manager/pull/1629)) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index c07a78fb50..b110a04789 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -1,7 +1,7 @@ ext { slf4jVersion = '1.7.30' guiceVersion = '4.2.3' - resteasyVersion = '4.5.8.Final' + resteasyVersion = '4.6.0.Final' // TODO upgrade to 2.12.0, but this breaks openapi spec generation jacksonVersion = '2.11.3' @@ -101,6 +101,7 @@ ext { ssp: "com.github.sdorra:ssp-lib:${sspVersion}", sspProcessor: "com.github.sdorra:ssp-processor:${sspVersion}", shiroUnit: 'com.github.sdorra:shiro-unit:1.0.1', + shiroExtension: 'com.github.sdorra:junit-shiro-extension:1.0.1', // jwt jjwtApi: "io.jsonwebtoken:jjwt-api:${jjwtVersion}", diff --git a/scm-core/src/main/java/sonia/scm/util/JsonMerger.java b/scm-core/src/main/java/sonia/scm/util/JsonMerger.java new file mode 100644 index 0000000000..9bde2379e9 --- /dev/null +++ b/scm-core/src/main/java/sonia/scm/util/JsonMerger.java @@ -0,0 +1,214 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.util; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.ObjectNode; +import sonia.scm.web.api.DtoValidator; + +import javax.inject.Inject; +import java.util.Iterator; + +/** + * This json merger can be used to apply various new fields to an existing object without overwriting + * the whole data or simply merge some json nodes into one. + * + * @since 2.18.0 + */ +public class JsonMerger { + + private final ObjectMapper objectMapper; + + @Inject + public JsonMerger(ObjectMapper objectMapper) { + this.objectMapper = objectMapper; + } + + /** + * Creates a MergerBuilder for the object. + * + * @param object object which should be updated + * @return merger builder + */ + public MergeStage fromObject(Object object) { + JsonNode mainNode = objectMapper.valueToTree(object); + return new MergeStage(objectMapper, mainNode); + } + + /** + * Creates a MergerBuilder for the json node. + * + * @param mainNode json node which should be updated + * @return merger builder + */ + public MergeStage fromJson(JsonNode mainNode) { + return new MergeStage(objectMapper, mainNode); + } + + public static class MergeStage { + private final ObjectMapper objectMapper; + private final JsonNode mainNode; + + private MergeStage(ObjectMapper objectMapper, JsonNode node) { + this.objectMapper = objectMapper; + this.mainNode = node; + } + + /** + * Merge object with main node + * + * @param object object which will be transformed to a json node in order to be merged with the main node + * @return this merge builder + */ + public MergeStage mergeWithObject(Object object) { + JsonNode updateNode = objectMapper.valueToTree(object); + merge(mainNode, updateNode); + return this; + } + + /** + * Merge json node with main node + * + * @param updateNode json node with data which should be applied to main node + * @return this merge builder + */ + public MergeStage mergeWithJson(JsonNode updateNode) { + merge(mainNode, updateNode); + return this; + } + + /** + * Returns the merged json node + * + * @return merged json node + */ + public JsonNode toJsonNode() { + return mainNode; + } + + /** + * Creates a specific object merger which can apply logic like validation after the actual merge. + * + * @param clazz class to which the output should be transformed + * @return object merger + */ + public ToObjectStage toObject(Class clazz) { + return new ToObjectStage<>(objectMapper, mainNode, clazz); + } + + /** + * Updates the {@param mainNode} with the provided fields from the {@param updateNode}. + * + * @param mainNode base nodes which will be updated + * @param updateNode updateNode that contains the new data + * @return updated json node + */ + private JsonNode merge(JsonNode mainNode, JsonNode updateNode) { + Iterator fieldNames = updateNode.fieldNames(); + while (fieldNames.hasNext()) { + String fieldName = fieldNames.next(); + if (mainNode.has(fieldName)) { + mergeNodes(mainNode, updateNode, fieldName); + } else { + mergeField(mainNode, updateNode, fieldName); + } + } + return mainNode; + } + + private void mergeField(JsonNode mainNode, JsonNode updateNode, String fieldName) { + if (mainNode instanceof ObjectNode) { + JsonNode value = updateNode.get(fieldName); + if (value.isNull()) { + return; + } + if (value.isIntegralNumber() && value.toString().equals("0")) { + return; + } + if (value.isFloatingPointNumber() && value.toString().equals("0.0")) { + return; + } + ((ObjectNode) mainNode).set(fieldName, value); + } + } + + private void mergeNodes(JsonNode mainNode, JsonNode updateNode, String fieldName) { + JsonNode jsonNode = updateNode.get(fieldName); + if (jsonNode.isObject()) { + ((ObjectNode) mainNode).set(fieldName, merge(mainNode.get(fieldName), updateNode.get(fieldName))); + } else if (jsonNode.isArray()) { + mergeArray((ObjectNode) mainNode, updateNode, fieldName, jsonNode); + } else { + ((ObjectNode) mainNode).set(fieldName, jsonNode); + } + } + + private void mergeArray(ObjectNode mainNode, JsonNode updateNode, String fieldName, JsonNode jsonNode) { + ArrayNode arrayNode = objectMapper.createArrayNode(); + for (int i = 0; i < jsonNode.size(); i++) { + arrayNode.add(merge(jsonNode.get(i), updateNode.get(fieldName).get(i))); + } + mainNode.set(fieldName, arrayNode); + } + } + + public static class ToObjectStage { + private final ObjectMapper objectMapper; + private final JsonNode node; + private final Class clazz; + private boolean shouldValidate = false; + + private ToObjectStage(ObjectMapper objectMapper, JsonNode node, Class clazz) { + this.objectMapper = objectMapper; + this.node = node; + this.clazz = clazz; + } + + /** + * Enables validation for dto object + * + * @return this builder instance + */ + public ToObjectStage withValidation() { + this.shouldValidate = true; + return this; + } + + /** + * Returns the merged object as the provided class + * + * @return merged object + */ + public T build() { + T updatedObject = objectMapper.convertValue(node, clazz); + if (shouldValidate) { + DtoValidator.validate(updatedObject); + } + return updatedObject; + } + } +} diff --git a/scm-core/src/test/java/sonia/scm/util/JsonMergerTest.java b/scm-core/src/test/java/sonia/scm/util/JsonMergerTest.java new file mode 100644 index 0000000000..1f51033ffd --- /dev/null +++ b/scm-core/src/test/java/sonia/scm/util/JsonMergerTest.java @@ -0,0 +1,236 @@ +/* + * MIT License + * + * Copyright (c) 2020-present Cloudogu GmbH and Contributors + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + */ + +package sonia.scm.util; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.common.collect.ImmutableList; +import org.junit.jupiter.api.Test; + +import javax.validation.ConstraintViolationException; +import javax.validation.constraints.NotNull; +import java.util.List; + +import static java.util.Collections.emptyList; +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class JsonMergerTest { + + private final ObjectMapper objectMapper = new ObjectMapper(); + private final JsonMerger jsonMerger = new JsonMerger(objectMapper); + + @Test + void shouldMergeMultipleFieldsWithDto() throws JsonProcessingException { + JsonNode updateNode = objectMapper.readValue("{ \"name\":\"zorbot\", \"value\":\"42\"}", JsonNode.class); + TestDto testDto = new TestDto("trillian", "blue", 21, new TestUser(), emptyList()); + + TestDto updatedDto = jsonMerger.fromObject(testDto).mergeWithJson(updateNode).toObject(TestDto.class).withValidation().build(); + + assertThat(updatedDto.name).isEqualTo("zorbot"); + assertThat(updatedDto.color).isEqualTo("blue"); + assertThat(updatedDto.value).isEqualTo(42); + } + + @Test + void shouldMergeNestedObjects() throws JsonProcessingException { + JsonNode updateNode = objectMapper.readValue("{ \"user\": { \"username\":\"zorbot\" } }", JsonNode.class); + TestDto testDto = + new TestDto("trillian", "blue", 21, new TestUser("trillian", "trillian@hitchhiker.org"), emptyList()); + + TestDto updatedDto = jsonMerger.fromObject(testDto).mergeWithJson(updateNode).toObject(TestDto.class).withValidation().build(); + + assertThat(updatedDto.user.username).isEqualTo("zorbot"); + assertThat(updatedDto.user.mail).isEqualTo("trillian@hitchhiker.org"); + assertThat(updatedDto.color).isEqualTo("blue"); + assertThat(updatedDto.value).isEqualTo(21); + } + + @Test + void shouldMergeArrays() throws JsonProcessingException { + JsonNode updateNode = objectMapper.readValue("{ \"fruits\": [\"banana\",\"apple\",\"orange\"] }", JsonNode.class); + TestDto testDto = + new TestDto("trillian", "blue", 21, new TestUser(), ImmutableList.of("strawberries")); + + TestDto updatedDto = jsonMerger.fromObject(testDto).mergeWithJson(updateNode).toObject(TestDto.class).withValidation().build(); + + assertThat(updatedDto.fruits).contains("banana", "apple", "orange"); + assertThat(updatedDto.color).isEqualTo("blue"); + assertThat(updatedDto.value).isEqualTo(21); + } + + @Test + void shouldRemoveFieldOnNull() throws JsonProcessingException { + JsonNode updateNode = objectMapper.readValue("{ \"fruits\": null, \"color\":null }", JsonNode.class); + + TestDto testDto = + new TestDto("trillian", "blue", 21, new TestUser(), ImmutableList.of("strawberries")); + TestDto updatedDto = jsonMerger.fromObject(testDto).mergeWithJson(updateNode).toObject(TestDto.class).withValidation().build(); + + assertThat(updatedDto.fruits).isNull(); + assertThat(updatedDto.color).isNull(); + assertThat(updatedDto.value).isEqualTo(21); + assertThat(updatedDto.name).isEqualTo("trillian"); + } + + @Test + void shouldMergeJsonNodes() throws JsonProcessingException { + JsonNode main = objectMapper.readValue( + "{ \"fruits\": [\"banana\", \"pineapple\"], \"name\":\"main\" }", + JsonNode.class); + JsonNode update = objectMapper.readValue("{ \"fruits\": [\"apple\"], \"color\":\"blue\"}", JsonNode.class); + + JsonNode jsonNode = jsonMerger.fromJson(main).mergeWithJson(update).toJsonNode(); + + assertThat(jsonNode.get("fruits").toPrettyString()).isEqualTo("[ \"apple\" ]"); + assertThat(jsonNode.get("color").asText()).isEqualTo("blue"); + assertThat(jsonNode.get("name").asText()).isEqualTo("main"); + } + + @Test + void shouldMergeMultipleTimes() throws JsonProcessingException { + TestDto mainDto = new TestDto("main", null, 0, new TestUser("trillian", ""), emptyList()); + TestDto firstUpdate = new TestDto("main", "blue", 42, new TestUser("trillian", "trillian@hitchhiker.org"), emptyList()); + JsonNode secondUpdate = objectMapper.readValue("{ \"fruits\": [\"banana\", \"pineapple\"], \"color\":\"red\" }", JsonNode.class); + + TestDto resultDto = jsonMerger + .fromObject(mainDto) + .mergeWithObject(firstUpdate) + .mergeWithJson(secondUpdate) + .toObject(TestDto.class) + .withValidation() + .build(); + + assertThat(resultDto.name).isEqualTo("main"); + assertThat(resultDto.color).isEqualTo("red"); + assertThat(resultDto.value).isEqualTo(42); + assertThat(resultDto.user.username).isEqualTo("trillian"); + assertThat(resultDto.user.mail).isEqualTo("trillian@hitchhiker.org"); + assertThat(resultDto.fruits).contains("banana", "pineapple"); + } + + @Test + void shouldFailForInvalidDto() { + TestDto mainDto = new TestDto("main", null, 0, new TestUser("trillian", ""), emptyList()); + TestDto firstUpdate = new TestDto(null, "blue", 42, new TestUser("trillian", "trillian@hitchhiker.org"), emptyList()); + + JsonMerger.ToObjectStage objectStage = jsonMerger.fromObject(mainDto) + .mergeWithObject(firstUpdate) + .toObject(TestDto.class) + .withValidation(); + + assertThrows(ConstraintViolationException.class, objectStage::build); + } + + private static class TestDto { + @NotNull + String name; + String color; + int value; + TestUser user; + List fruits; + + public TestDto() { + } + + public TestDto(String name, String color, int value, TestUser user, List fruits) { + this.name = name; + this.color = color; + this.value = value; + this.user = user; + this.fruits = fruits; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public String getColor() { + return color; + } + + public void setColor(String color) { + this.color = color; + } + + public int getValue() { + return value; + } + + public void setValue(int value) { + this.value = value; + } + + public TestUser getUser() { + return user; + } + + public void setUser(TestUser user) { + this.user = user; + } + + public List getFruits() { + return fruits; + } + + public void setFruits(List fruits) { + this.fruits = fruits; + } + } + + private static class TestUser { + private String username; + private String mail; + + public TestUser() { + } + + public TestUser(String username, String mail) { + this.username = username; + this.mail = mail; + } + + public String getUsername() { + return username; + } + + public void setUsername(String username) { + this.username = username; + } + + public String getMail() { + return mail; + } + + public void setMail(String mail) { + this.mail = mail; + } + } +} diff --git a/scm-test/build.gradle b/scm-test/build.gradle index f6ba683de9..ec5119431c 100644 --- a/scm-test/build.gradle +++ b/scm-test/build.gradle @@ -36,6 +36,7 @@ dependencies { api libraries.junitJupiterParams api libraries.junitJupiterEngine api libraries.shiroUnit + api libraries.shiroExtension // junit 4 support api libraries.junitVintageEngine diff --git a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ConfigResource.java b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ConfigResource.java index 13dec7d44e..9c6dedcec3 100644 --- a/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ConfigResource.java +++ b/scm-webapp/src/main/java/sonia/scm/api/v2/resources/ConfigResource.java @@ -24,6 +24,7 @@ package sonia.scm.api.v2.resources; +import com.fasterxml.jackson.databind.JsonNode; import com.google.common.annotations.VisibleForTesting; import io.swagger.v3.oas.annotations.OpenAPIDefinition; import io.swagger.v3.oas.annotations.Operation; @@ -36,6 +37,7 @@ import io.swagger.v3.oas.annotations.tags.Tag; import sonia.scm.config.ConfigurationPermissions; import sonia.scm.config.ScmConfiguration; import sonia.scm.repository.NamespaceStrategyValidator; +import sonia.scm.util.JsonMerger; import sonia.scm.util.ScmConfigurationUtil; import sonia.scm.web.VndMediaType; @@ -43,6 +45,7 @@ import javax.inject.Inject; import javax.validation.Valid; import javax.ws.rs.Consumes; import javax.ws.rs.GET; +import javax.ws.rs.PATCH; import javax.ws.rs.PUT; import javax.ws.rs.Path; import javax.ws.rs.Produces; @@ -63,17 +66,21 @@ public class ConfigResource { private final ScmConfigurationToConfigDtoMapper configToDtoMapper; private final ScmConfiguration configuration; private final NamespaceStrategyValidator namespaceStrategyValidator; + private final JsonMerger jsonMerger; - private Consumer store = (config) -> ScmConfigurationUtil.getInstance().store(config); + private Consumer store = config -> ScmConfigurationUtil.getInstance().store(config); @Inject public ConfigResource(ConfigDtoToScmConfigurationMapper dtoToConfigMapper, ScmConfigurationToConfigDtoMapper configToDtoMapper, - ScmConfiguration configuration, NamespaceStrategyValidator namespaceStrategyValidator) { + ScmConfiguration configuration, + NamespaceStrategyValidator namespaceStrategyValidator, + JsonMerger jsonMerger) { this.dtoToConfigMapper = dtoToConfigMapper; this.configToDtoMapper = configToDtoMapper; this.configuration = configuration; this.namespaceStrategyValidator = namespaceStrategyValidator; + this.jsonMerger = jsonMerger; } @VisibleForTesting @@ -107,7 +114,6 @@ public class ConfigResource { ) ) public Response get() { - // We do this permission check in Resource and not in ScmConfiguration, because it must be available for reading // from within the code (plugins, etc.), but not for the whole anonymous world outside. ConfigurationPermissions.read(configuration).check(); @@ -151,21 +157,73 @@ public class ConfigResource { ) ) public Response update(@Valid ConfigDto configDto) { - - // This *could* be moved to ScmConfiguration or ScmConfigurationUtil classes. - // But to where to check? load() or store()? Leave it for now, SCMv1 legacy that can be cleaned up later. ConfigurationPermissions.write(configuration).check(); - - // ensure the namespace strategy is valid - namespaceStrategyValidator.check(configDto.getNamespaceStrategy()); - - ScmConfiguration config = dtoToConfigMapper.map(configDto); - synchronized (ScmConfiguration.class) { - configuration.load(config); - store.accept(configuration); - } + updateConfig(configDto); return Response.noContent().build(); } + /** + * Modifies the global scm config partially. + * + * @param updateNode json object which contains changed fields + */ + @PATCH + @Path("") + @Consumes(VndMediaType.CONFIG) + @Operation( + summary = "Update instance configuration partially", + description = "Modifies the instance configuration partially.", + tags = "Instance configuration", + requestBody = @RequestBody( + content = @Content( + mediaType = VndMediaType.CONFIG, + schema = @Schema(implementation = UpdateConfigDto.class), + examples = @ExampleObject( + name = "Overwrites the provided fields of the current configuration.", + value = "{\n \"realmDescription\":\"SCM-Manager Realm\" \n}", + summary = "Update configuration partially" + ) + ) + ) + ) + @ApiResponse(responseCode = "204", description = "update success") + @ApiResponse(responseCode = "401", description = "not authenticated / invalid credentials") + @ApiResponse(responseCode = "403", description = "not authorized, the current user does not have the \"configuration:write\" privilege") + @ApiResponse( + responseCode = "500", + description = "internal server error", + content = @Content( + mediaType = VndMediaType.ERROR_TYPE, + schema = @Schema(implementation = ErrorDto.class) + ) + ) + public Response updatePartially(JsonNode updateNode) { + ConfigurationPermissions.write(configuration).check(); + + ConfigDto updatedConfigDto = jsonMerger + .fromObject(configToDtoMapper.map(configuration)) + .mergeWithJson(updateNode) + .toObject(ConfigDto.class) + .withValidation() + .build(); + updateConfig(updatedConfigDto); + + return Response.noContent().build(); + } + + private void updateConfig(ConfigDto updatedConfigDto) { + // This *could* be moved to ScmConfiguration or ScmConfigurationUtil classes. + // But to where to check? load() or store()? Leave it for now, SCMv1 legacy that can be cleaned up later. + + // ensure the namespace strategy is valid + namespaceStrategyValidator.check(updatedConfigDto.getNamespaceStrategy()); + + ScmConfiguration config = dtoToConfigMapper.map(updatedConfigDto); + synchronized (ScmConfiguration.class) { + configuration.load(config); + store.accept(configuration); + } + } + } diff --git a/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java b/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java index 3c7f1cfef5..2d68dc53b9 100644 --- a/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java +++ b/scm-webapp/src/main/java/sonia/scm/web/i18n/I18nServlet.java @@ -27,7 +27,6 @@ package sonia.scm.web.i18n; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.node.ObjectNode; import com.github.legman.Subscribe; import com.google.common.annotations.VisibleForTesting; import com.google.inject.Singleton; @@ -40,6 +39,7 @@ import sonia.scm.cache.CacheManager; import sonia.scm.filter.WebElement; import sonia.scm.lifecycle.RestartEvent; import sonia.scm.plugin.PluginLoader; +import sonia.scm.util.JsonMerger; import javax.inject.Inject; import javax.servlet.http.HttpServlet; @@ -49,7 +49,6 @@ import java.io.IOException; import java.io.PrintWriter; import java.net.URL; import java.util.Enumeration; -import java.util.Iterator; import java.util.Optional; @@ -70,13 +69,15 @@ public class I18nServlet extends HttpServlet { private final ClassLoader classLoader; private final Cache cache; private final ObjectMapper objectMapper = new ObjectMapper(); + private final JsonMerger jsonMerger; @Inject - public I18nServlet(SCMContextProvider context, PluginLoader pluginLoader, CacheManager cacheManager) { + public I18nServlet(SCMContextProvider context, PluginLoader pluginLoader, CacheManager cacheManager, JsonMerger jsonMerger) { this.context = context; this.classLoader = pluginLoader.getUberClassLoader(); this.cache = cacheManager.getCache(CACHE_NAME); + this.jsonMerger = jsonMerger; } @Subscribe(async = false) @@ -146,7 +147,7 @@ public class I18nServlet extends HttpServlet { URL url = resources.nextElement(); JsonNode jsonNode = objectMapper.readTree(url); if (mergedJsonNode != null) { - merge(mergedJsonNode, jsonNode); + mergedJsonNode = jsonMerger.fromJson(mergedJsonNode).mergeWithJson(jsonNode).toJsonNode(); } else { mergedJsonNode = jsonNode; } @@ -155,44 +156,4 @@ public class I18nServlet extends HttpServlet { return Optional.ofNullable(mergedJsonNode); } - private JsonNode merge(JsonNode mainNode, JsonNode updateNode) { - Iterator fieldNames = updateNode.fieldNames(); - while (fieldNames.hasNext()) { - String fieldName = fieldNames.next(); - JsonNode jsonNode = mainNode.get(fieldName); - if (jsonNode != null) { - mergeNode(updateNode, fieldName, jsonNode); - } else { - mergeField(mainNode, updateNode, fieldName); - } - } - return mainNode; - } - - private void mergeField(JsonNode mainNode, JsonNode updateNode, String fieldName) { - if (mainNode instanceof ObjectNode) { - JsonNode value = updateNode.get(fieldName); - if (value.isNull()) { - return; - } - if (value.isIntegralNumber() && value.toString().equals("0")) { - return; - } - if (value.isFloatingPointNumber() && value.toString().equals("0.0")) { - return; - } - ((ObjectNode) mainNode).set(fieldName, value); - } - } - - private void mergeNode(JsonNode updateNode, String fieldName, JsonNode jsonNode) { - if (jsonNode.isObject()) { - merge(jsonNode, updateNode.get(fieldName)); - } else if (jsonNode.isArray()) { - for (int i = 0; i < jsonNode.size(); i++) { - merge(jsonNode.get(i), updateNode.get(fieldName).get(i)); - } - } - } - } diff --git a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/ConfigResourceTest.java b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/ConfigResourceTest.java index 6f3803bbfd..306c61eb4e 100644 --- a/scm-webapp/src/test/java/sonia/scm/api/v2/resources/ConfigResourceTest.java +++ b/scm-webapp/src/test/java/sonia/scm/api/v2/resources/ConfigResourceTest.java @@ -24,19 +24,21 @@ package sonia.scm.api.v2.resources; -import com.github.sdorra.shiro.ShiroRule; -import com.github.sdorra.shiro.SubjectAware; +import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.io.Resources; -import org.apache.shiro.util.ThreadContext; +import org.github.sdorra.jse.ShiroExtension; +import org.github.sdorra.jse.SubjectAware; import org.jboss.resteasy.mock.MockHttpRequest; import org.jboss.resteasy.mock.MockHttpResponse; -import org.junit.Before; -import org.junit.Rule; -import org.junit.Test; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; import sonia.scm.config.ScmConfiguration; import sonia.scm.repository.NamespaceStrategyValidator; +import sonia.scm.util.JsonMerger; import sonia.scm.web.RestDispatcher; import sonia.scm.web.VndMediaType; @@ -48,110 +50,104 @@ import java.net.URISyntaxException; import java.net.URL; import java.nio.charset.StandardCharsets; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.verify; -import static org.mockito.MockitoAnnotations.initMocks; +@ExtendWith(ShiroExtension.class) +@ExtendWith(MockitoExtension.class) @SubjectAware( - configuration = "classpath:sonia/scm/configuration/shiro.ini", - password = "secret" + value = "trillian" ) -public class ConfigResourceTest { +class ConfigResourceTest { - @Rule - public ShiroRule shiro = new ShiroRule(); - - private RestDispatcher dispatcher = new RestDispatcher(); + private final RestDispatcher dispatcher = new RestDispatcher(); private final URI baseUri = URI.create("/"); @SuppressWarnings("unused") // Is injected - private ResourceLinks resourceLinks = ResourceLinksMock.createMock(baseUri); + private final ResourceLinks resourceLinks = ResourceLinksMock.createMock(baseUri); @Mock private NamespaceStrategyValidator namespaceStrategyValidator; + private final JsonMerger jsonMerger = new JsonMerger(new ObjectMapper()); + @InjectMocks private ConfigDtoToScmConfigurationMapperImpl dtoToConfigMapper; @InjectMocks private ScmConfigurationToConfigDtoMapperImpl configToDtoMapper; - public ConfigResourceTest() { - // cleanup state that might have been left by other tests - ThreadContext.unbindSecurityManager(); - ThreadContext.unbindSubject(); - ThreadContext.remove(); - } - - @Before - public void prepareEnvironment() { - initMocks(this); - - ConfigResource configResource = new ConfigResource(dtoToConfigMapper, configToDtoMapper, createConfiguration(), namespaceStrategyValidator); - configResource.setStore(config -> {}); + @BeforeEach + void prepareEnvironment() { + ConfigResource configResource = new ConfigResource(dtoToConfigMapper, configToDtoMapper, createConfiguration(), namespaceStrategyValidator, jsonMerger); + configResource.setStore(config -> { + }); dispatcher.addSingletonResource(configResource); } @Test - @SubjectAware(username = "readOnly") - public void shouldGetGlobalConfig() throws URISyntaxException, UnsupportedEncodingException { + @SubjectAware( + permissions = "configuration:read:global" + ) + void shouldGetGlobalConfig() throws URISyntaxException, UnsupportedEncodingException { MockHttpRequest request = MockHttpRequest.get("/" + ConfigResource.CONFIG_PATH_V2); MockHttpResponse response = new MockHttpResponse(); dispatcher.invoke(request, response); - assertEquals(HttpServletResponse.SC_OK, response.getStatus()); - assertTrue(response.getContentAsString().contains("\"proxyPassword\":\"heartOfGold\"")); - assertTrue(response.getContentAsString().contains("\"self\":{\"href\":\"/v2/config")); - assertFalse("Update link present", response.getContentAsString().contains("\"update\":{\"href\":\"/v2/config")); + + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + assertThat(response.getContentAsString()).contains("\"proxyPassword\":\"heartOfGold\""); + assertThat(response.getContentAsString()).contains("\"self\":{\"href\":\"/v2/config"); + assertThat(response.getContentAsString()).doesNotContain("\"update\":{\"href\":\"/v2/config"); } @Test - @SubjectAware(username = "writeOnly") - public void shouldNotGetConfigWhenNotAuthorized() throws URISyntaxException, UnsupportedEncodingException { + void shouldNotGetConfigWhenNotAuthorized() throws URISyntaxException { MockHttpRequest request = MockHttpRequest.get("/" + ConfigResource.CONFIG_PATH_V2); MockHttpResponse response = new MockHttpResponse(); dispatcher.invoke(request, response); - assertEquals("Subject does not have permission [configuration:read:global]", response.getContentAsString()); - assertEquals(HttpServletResponse.SC_FORBIDDEN, response.getStatus()); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); } @Test - @SubjectAware(username = "readWrite") - public void shouldUpdateConfig() throws URISyntaxException, IOException { - MockHttpRequest request = post("sonia/scm/api/v2/config-test-update.json"); + @SubjectAware( + permissions = "configuration:read,write:global" + ) + void shouldUpdateConfig() throws URISyntaxException, IOException { + MockHttpRequest request = put("sonia/scm/api/v2/config-test-update.json"); MockHttpResponse response = new MockHttpResponse(); dispatcher.invoke(request, response); - assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_NO_CONTENT); request = MockHttpRequest.get("/" + ConfigResource.CONFIG_PATH_V2); response = new MockHttpResponse(); dispatcher.invoke(request, response); - assertEquals(HttpServletResponse.SC_OK, response.getStatus()); - assertTrue(response.getContentAsString().contains("\"proxyPassword\":\"newPassword\"")); - assertTrue(response.getContentAsString().contains("\"self\":{\"href\":\"/v2/config")); - assertTrue("link not found", response.getContentAsString().contains("\"update\":{\"href\":\"/v2/config")); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + // Should overwrite old realm description with null + assertThat(response.getContentAsString()).contains("\"realmDescription\":null"); + assertThat(response.getContentAsString()).contains("\"proxyPassword\":\"newPassword\""); + assertThat(response.getContentAsString()).contains("\"self\":{\"href\":\"/v2/config"); + assertThat(response.getContentAsString()).contains("\"update\":{\"href\":\"/v2/config"); } @Test - @SubjectAware(username = "readOnly") - public void shouldNotUpdateConfigWhenNotAuthorized() throws URISyntaxException, IOException { - MockHttpRequest request = post("sonia/scm/api/v2/config-test-update.json"); + void shouldNotUpdateConfigWhenNotAuthorized() throws URISyntaxException, IOException { + MockHttpRequest request = put("sonia/scm/api/v2/config-test-update.json"); MockHttpResponse response = new MockHttpResponse(); dispatcher.invoke(request, response); - assertEquals("Subject does not have permission [configuration:write:global]", response.getContentAsString()); - assertEquals(HttpServletResponse.SC_FORBIDDEN, response.getStatus()); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); } @Test - @SubjectAware(username = "readWrite") - public void shouldValidateNamespaceStrategy() throws URISyntaxException { + @SubjectAware( + permissions = "configuration:write:global" + ) + void shouldValidateNamespaceStrategy() throws URISyntaxException { MockHttpRequest request = MockHttpRequest.put("/" + ConfigResource.CONFIG_PATH_V2) .contentType(VndMediaType.CONFIG) .content("{ \"namespaceStrategy\": \"AwesomeStrategy\" }".getBytes(StandardCharsets.UTF_8)); @@ -159,11 +155,44 @@ public class ConfigResourceTest { MockHttpResponse response = new MockHttpResponse(); dispatcher.invoke(request, response); - assertEquals(HttpServletResponse.SC_NO_CONTENT, response.getStatus()); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_NO_CONTENT); verify(namespaceStrategyValidator).check("AwesomeStrategy"); } - private MockHttpRequest post(String resourceName) throws IOException, URISyntaxException { + @Test + @SubjectAware( + permissions = "configuration:read,write:global" + ) + void shouldUpdateConfigPartially() throws URISyntaxException, IOException { + MockHttpRequest request = patch("{ \"proxyPort\":\"1337\", \"proxyPassword\":null }"); + + MockHttpResponse response = new MockHttpResponse(); + dispatcher.invoke(request, response); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_NO_CONTENT); + + request = MockHttpRequest.get("/" + ConfigResource.CONFIG_PATH_V2); + response = new MockHttpResponse(); + dispatcher.invoke(request, response); + + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_OK); + // Should not change old realm description + assertThat(response.getContentAsString()).contains("\"realmDescription\":\"SONIA :: SCM Manager\""); + assertThat(response.getContentAsString()).contains("\"proxyPassword\":null"); + assertThat(response.getContentAsString()).contains("\"proxyPort\":1337"); + assertThat(response.getContentAsString()).contains("\"self\":{\"href\":\"/v2/config"); + assertThat(response.getContentAsString()).contains("\"update\":{\"href\":\"/v2/config"); + } + + @Test + void shouldNotUpdateConfigPartiallyIfNotAuthorized() throws URISyntaxException { + MockHttpRequest request = patch("{ \"proxyPort\":\"1337\", \"proxyPassword\":\"hitchhiker\" }"); + + MockHttpResponse response = new MockHttpResponse(); + dispatcher.invoke(request, response); + assertThat(response.getStatus()).isEqualTo(HttpServletResponse.SC_FORBIDDEN); + } + + private MockHttpRequest put(String resourceName) throws IOException, URISyntaxException { URL url = Resources.getResource(resourceName); byte[] configJson = Resources.toByteArray(url); return MockHttpRequest.put("/" + ConfigResource.CONFIG_PATH_V2) @@ -171,6 +200,12 @@ public class ConfigResourceTest { .content(configJson); } + private MockHttpRequest patch(String json) throws URISyntaxException { + return MockHttpRequest.patch("/" + ConfigResource.CONFIG_PATH_V2) + .contentType(VndMediaType.CONFIG) + .content(json.getBytes()); + } + private static ScmConfiguration createConfiguration() { ScmConfiguration scmConfiguration = new ScmConfiguration(); scmConfiguration.setProxyPassword("heartOfGold"); diff --git a/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java b/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java index 978d45fbd9..2d7cb5297b 100644 --- a/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java +++ b/scm-webapp/src/test/java/sonia/scm/web/i18n/I18nServletTest.java @@ -41,6 +41,7 @@ import sonia.scm.cache.Cache; import sonia.scm.cache.CacheManager; import sonia.scm.lifecycle.RestartEventFactory; import sonia.scm.plugin.PluginLoader; +import sonia.scm.util.JsonMerger; import javax.annotation.Nonnull; import javax.servlet.http.HttpServletRequest; @@ -62,7 +63,11 @@ import java.util.List; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.fail; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; @ExtendWith(MockitoExtension.class) class I18nServletTest { @@ -223,7 +228,7 @@ class I18nServletTest { @Nonnull private I18nServlet createServlet() { - return new I18nServlet(context, pluginLoader, cacheManager); + return new I18nServlet(context, pluginLoader, cacheManager, new JsonMerger(new ObjectMapper())); } private String doGetString(I18nServlet servlet, HttpServletRequest request, HttpServletResponse response) throws IOException {