From 10f46d2f265e3a14049c58022d075cb78b4a2b65 Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Mon, 26 Oct 2020 15:43:01 +0100 Subject: [PATCH 1/8] Add integration test annotation for Spring Boot apps QUES-36 --- spring-infrastructure/pom.xml | 7 +++++ .../pkb/common/testing/IntegrationTest.java | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 spring-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java diff --git a/spring-infrastructure/pom.xml b/spring-infrastructure/pom.xml index 3003c2a7..0014f30c 100644 --- a/spring-infrastructure/pom.xml +++ b/spring-infrastructure/pom.xml @@ -27,6 +27,12 @@ spring-context ${spring.version} + + org.springframework + spring-test + ${spring.version} + compile + com.pkb.pkbcommon infrastructure @@ -45,6 +51,7 @@ org.junit.jupiter junit-jupiter-api + compile org.junit.jupiter diff --git a/spring-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java b/spring-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java new file mode 100644 index 00000000..fa015a92 --- /dev/null +++ b/spring-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java @@ -0,0 +1,28 @@ +package com.pkb.common.testing; + +import java.lang.annotation.Documented; +import java.lang.annotation.ElementType; +import java.lang.annotation.Inherited; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +import org.junit.jupiter.api.Tag; +import org.springframework.test.context.ActiveProfiles; + +/** + * This tag helps organise integration tests for spring boot applications. + * When applied to a junit 5 test, it enables it to be filtered in maven + * using "groups" in the surefire/failsafe configuration. + * + * In addition, it applies the spring profile "integration-test" allowing + * selective configuration overrides. + */ +@Target(ElementType.TYPE) +@Retention(RetentionPolicy.RUNTIME) +@Inherited +@ActiveProfiles("integration-test") +@Documented +@Tag("integration-test") +public @interface IntegrationTest { +} From 379748db1bfaa781f03325215a03b5fd7777caa4 Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Fri, 6 Nov 2020 16:10:18 +0100 Subject: [PATCH 2/8] Add SpringBootConfigStorage to allow mutable properties in Spring Boot apps QUES-36 Also move Spring boot tools into their own module to avoid pulling in unnecessary dependencies to non-spring-boot apps. --- pom.xml | 40 +++++++ spring-boot-infrastructure/pom.xml | 43 ++++++++ ...urationPropertiesScopingPostProcessor.java | 58 ++++++++++ .../config/SpringBootConfigStorage.java | 102 ++++++++++++++++++ .../pkb/common/testing/IntegrationTest.java | 0 spring-infrastructure/pom.xml | 20 ---- .../common/config/SpringConfigStorage.java | 26 ----- 7 files changed, 243 insertions(+), 46 deletions(-) create mode 100644 spring-boot-infrastructure/pom.xml create mode 100644 spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java create mode 100644 spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java rename {spring-infrastructure => spring-boot-infrastructure}/src/main/java/com/pkb/common/testing/IntegrationTest.java (100%) delete mode 100644 spring-infrastructure/src/main/java/com/pkb/common/config/SpringConfigStorage.java diff --git a/pom.xml b/pom.xml index 6fe57f0a..1fec326f 100644 --- a/pom.xml +++ b/pom.xml @@ -52,6 +52,9 @@ 12-a115018-86851 3.0.7-PKB + 5.2.3.RELEASE + 20-789c720-63866 + Hoxton.SR8 5.3.15.Final @@ -64,10 +67,23 @@ spring-infrastructure testlogging testsupport + spring-boot-infrastructure + + + com.pkb.pkbcommon + infrastructure + ${revision}${changelist} + + + com.pkb + commons-testing-base + ${commons.testing.version} + test + org.apache.commons commons-text @@ -210,6 +226,13 @@ + + org.springframework.cloud + spring-cloud-dependencies + ${spring-cloud.version} + pom + import + org.hibernate hibernate-core @@ -226,6 +249,23 @@ + + org.springframework + spring-context + ${spring.version} + + + + org.springframework + spring-test + ${spring.version} + compile + + + org.springframework + spring-beans + ${spring.version} + diff --git a/spring-boot-infrastructure/pom.xml b/spring-boot-infrastructure/pom.xml new file mode 100644 index 00000000..8af5ea7f --- /dev/null +++ b/spring-boot-infrastructure/pom.xml @@ -0,0 +1,43 @@ + + + + pkb-common + com.pkb.pkbcommon + ${revision}${changelist} + + 4.0.0 + + spring-boot-infrastructure + + + + org.springframework + spring-test + compile + + + org.springframework + spring-beans + + + org.springframework.cloud + spring-cloud-context + + + org.junit.jupiter + junit-jupiter-api + compile + + + com.pkb.pkbcommon + config + + + org.springframework + spring-context + + + + \ No newline at end of file diff --git a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java new file mode 100644 index 00000000..46a8c1af --- /dev/null +++ b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java @@ -0,0 +1,58 @@ +package com.pkb.common.config; + +import org.springframework.aop.scope.ScopedProxyUtils; +import org.springframework.beans.BeansException; +import org.springframework.beans.factory.config.BeanDefinition; +import org.springframework.beans.factory.config.BeanDefinitionHolder; +import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; +import org.springframework.beans.factory.config.Scope; +import org.springframework.beans.factory.support.BeanDefinitionRegistry; +import org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor; +import org.springframework.beans.factory.support.GenericBeanDefinition; +import org.springframework.context.annotation.ScopedProxyMode; +import org.springframework.core.annotation.AnnotationAttributes; +import org.springframework.core.type.AnnotationMetadata; + +import javax.annotation.ParametersAreNonnullByDefault; +import java.util.Optional; + +/** + * This post-processor works around a bug/limitation of Spring Boot/Spring Cloud whereby + * beans discovered by the @EnableConfigurationProperties or @ConfigurationPropertiesScan annotations + * aren't ever put into custom scopes like the RefreshScope. This post-processor mimics what the + * standard Spring bean component scanning does by reading the scope attributes and then creating + * a scoped proxy if the requested scope requires it. It also ensure that the bean definition + * has the appropriate scope marked on it. + * + * To use this, define a static method on Configuration class that returns an instance of this as a Bean. + * You should probably give it an @Order attribute as well to ensure that this executes as early as possible + * so that other standard scope processing steps can operate on the corrected bean definition. + */ +@ParametersAreNonnullByDefault +public class ConfigurationPropertiesScopingPostProcessor implements BeanDefinitionRegistryPostProcessor { + + @Override + public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) throws BeansException { + for (String name : registry.getBeanDefinitionNames()) { + BeanDefinition beanDefinition = registry.getBeanDefinition(name); + if (beanDefinition.getClass().getName().equals("org.springframework.boot.context.properties.ConfigurationPropertiesValueObjectBeanDefinition")) { + GenericBeanDefinition gbd = (GenericBeanDefinition) beanDefinition; + Optional annAttrs = Optional.ofNullable(AnnotationAttributes.fromMap(AnnotationMetadata.introspect(gbd.getBeanClass()).getAnnotationAttributes(Scope.class.getName(), false))); + ScopedProxyMode scopedProxyMode = annAttrs.map(attrs -> attrs.getEnum("proxyMode")).orElse(ScopedProxyMode.NO); + gbd.setScope(annAttrs.map(attrs -> attrs.getString("value")).orElse(gbd.getScope())); + if (scopedProxyMode == ScopedProxyMode.NO) { + continue; + } + boolean proxyTargetClass = scopedProxyMode == ScopedProxyMode.TARGET_CLASS; + registry.removeBeanDefinition(name); + BeanDefinitionHolder scopedProxy = ScopedProxyUtils.createScopedProxy(new BeanDefinitionHolder(gbd, name), registry, proxyTargetClass); + registry.registerBeanDefinition(scopedProxy.getBeanName(), scopedProxy.getBeanDefinition()); + } + } + } + + @Override + public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) throws BeansException { + //No-op + } +} diff --git a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java new file mode 100644 index 00000000..37a650c6 --- /dev/null +++ b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java @@ -0,0 +1,102 @@ +package com.pkb.common.config; + +import org.springframework.cloud.context.scope.refresh.RefreshScope; +import org.springframework.core.env.ConfigurableEnvironment; +import org.springframework.core.env.MapPropertySource; + +import javax.annotation.ParametersAreNonnullByDefault; +import java.util.LinkedHashMap; +import java.util.Map; + +/** + * This class provides an implementation of the PKB ConfigStorage interface that interacts + * helpfully with the Spring environment. This class can be passed to both the TestSupportAgent + * and any PKB config objects that require a ConfigStorage. It will read properties from the + * Spring environment, allowing Spring Boot applications to be be configured using all the standard + * Spring properties mechanisms such as application.properties and the OS environment - so you don't + * need to provide an additional config.properties file just to control shared PKB components that + * use PKB Common configuration classes + * + * Additionally, this class maintains an override map that is loaded into the Spring environment + * as a high-priority property source. If mutations to properties are requested (e.g. from the + * TestSupportAgent) these overrides will be visible in the spring environment. + * + * In a typical Spring Boot application, properties are not accessed directly from the environment but + * are bound to type-safe classes annotated with @ConfigurationProperties. Since this binding happens when + * the Spring context is loading, in order for later changes to the Spring environment to be visible + * to these classes and their clients, it is necessary to do a selective refresh of the spring context. + * ConfigurationProperties beans that contain mutable properties that may need to be updated at runtime + * should be annotated with @RefreshScope. Additionally, you should include the {@link ConfigurationPropertiesScopingPostProcessor} + * bean in your context to ensure that all ConfigurationProperties classes get included in the RefreshScope, + * irrespective of how they are constructed. Once you do this, this class will ensure that configuration + * beans are dynamically re-bound at runtime when properties change. + * + * It is recommended to use @ConstructorBinding for ConfigurationProperties classes; this can be used with + * kotlin data classes, java value objects or at least a POJO with final fields and no setters. This ensures + * that properties present an immutable interface to clients and can only be changed by the framework. + * + * + * NB This class and the above-described mechanism only ensure that the relevant properties classes will + * return the updated values on the next method call to one of the property getters. As yet there is no + * mechanism for notifying clients that an update has occured (although in principle this would not be difficult) + * nor does it make any attempt to ensure that any dependencies of these properties beans - including spring + * infrastructure that may have been wired together according to their values - are dynamically updated. + */ +@ParametersAreNonnullByDefault +public class SpringBootConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage { + private final ConfigurableEnvironment environment; + private final RefreshScope refreshScope; + private final Map overrides = new LinkedHashMap<>(); + + public SpringBootConfigStorage(ConfigurableEnvironment environment, RefreshScope refreshScope) { + this.environment = environment; + this.refreshScope = refreshScope; + environment.getPropertySources().addFirst(new MapPropertySource("TestSupportOverrides", overrides)); + } + + + @Override + public String getString(String key) { + return environment.getProperty(key); + } + + @Override + public String getString(String key, String defaultValue) { + return environment.getProperty(key, defaultValue); + } + + @Override + public boolean isMutableConfigEnabled() { + return getBoolean("mutableConfig.enabled", false); + } + + @Override + public void setValue(String key, String value) { + if (isMutableConfigEnabled()) { + overrides.put(key, value); + refreshScope.refreshAll(); + } + } + + @Override + public OverrideRemovalResult removeOverrideAtKey(String key) { + if (!isMutableConfigEnabled()) { + return OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE; + } + if (overrides.remove(key) == null) { + return OverrideRemovalResult.KEY_NOT_FOUND; + } + refreshScope.refreshAll(); + return OverrideRemovalResult.REMOVED; + } + + @Override + public void reset() { + if (isMutableConfigEnabled()) { + overrides.clear(); + refreshScope.refreshAll(); + } + } + +} + diff --git a/spring-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java b/spring-boot-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java similarity index 100% rename from spring-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java rename to spring-boot-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java diff --git a/spring-infrastructure/pom.xml b/spring-infrastructure/pom.xml index 0014f30c..09d4d5cf 100644 --- a/spring-infrastructure/pom.xml +++ b/spring-infrastructure/pom.xml @@ -12,11 +12,6 @@ spring-infrastructure pkb-common/spring-infrastructure - - 5.2.3.RELEASE - 20-789c720-63866 - - com.google.guava @@ -25,18 +20,10 @@ org.springframework spring-context - ${spring.version} - - - org.springframework - spring-test - ${spring.version} - compile com.pkb.pkbcommon infrastructure - ${revision}${changelist} com.github.karsaig @@ -45,17 +32,10 @@ com.pkb commons-testing-base - ${commons.testing.version} - test org.junit.jupiter junit-jupiter-api - compile - - - org.junit.jupiter - junit-jupiter-params com.pkb diff --git a/spring-infrastructure/src/main/java/com/pkb/common/config/SpringConfigStorage.java b/spring-infrastructure/src/main/java/com/pkb/common/config/SpringConfigStorage.java deleted file mode 100644 index 84f82dd1..00000000 --- a/spring-infrastructure/src/main/java/com/pkb/common/config/SpringConfigStorage.java +++ /dev/null @@ -1,26 +0,0 @@ -package com.pkb.common.config; - -import org.jetbrains.annotations.NotNull; -import org.springframework.core.env.Environment; - -public class SpringConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage { - - - private final Environment environment; - - public SpringConfigStorage(@NotNull Environment environment) { - this.environment = environment; - } - - @Override - public String getString(String key) { - return environment.getProperty(key); - } - - @Override - public String getString(String key, String defaultValue) { - return environment.getProperty(key, defaultValue); - } - - -} From 7d3eedb5fd3145bf8d3de830bc718cdee08041c8 Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Tue, 3 Nov 2020 19:55:01 +0100 Subject: [PATCH 3/8] Fix Owasp QUES-38 --- spring-boot-infrastructure/pom.xml | 6 ++++++ suppression.xml | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/spring-boot-infrastructure/pom.xml b/spring-boot-infrastructure/pom.xml index 8af5ea7f..6c964cfe 100644 --- a/spring-boot-infrastructure/pom.xml +++ b/spring-boot-infrastructure/pom.xml @@ -24,6 +24,12 @@ org.springframework.cloud spring-cloud-context + + + org.springframework.security + spring-security-crypto + + org.junit.jupiter diff --git a/suppression.xml b/suppression.xml index 97a89c29..d45e2fc1 100644 --- a/suppression.xml +++ b/suppression.xml @@ -16,6 +16,13 @@ + ^pkg:maven/org\.eclipse\.jetty/jetty\-util@.*$ + CVE-2020-27216 + + + d25481cf27aab12be6e7ba11eca5eea3f0bc4be2 From 6a952a7b793653eed43bac0d21f9860ff170fa12 Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Tue, 10 Nov 2020 12:17:38 +0100 Subject: [PATCH 4/8] PR review changes QUES-36 Moved test utilities + deps to commons-testing project Got rid of unhelpful marker interfaces Add some testing (unit + integration) for SpringBootConfigStorage --- .../config/AbstractMutableConfigStorage.java | 14 +++ .../com/pkb/common/config/ConfigStorage.java | 5 +- .../common/config/ConfigStorageFactory.java | 2 +- .../common/config/ImmutableConfigStorage.java | 30 ------ .../config/ImmutableRawConfigStorage.java | 23 ++++- .../common/config/MutableConfigStorage.java | 10 -- .../config/MutableRawConfigStorage.java | 22 ++--- .../config/ImmutableRawConfigStorageTest.java | 6 -- .../config/MutableRawConfigStorageTest.java | 6 -- pom.xml | 14 ++- spring-boot-infrastructure/pom.xml | 47 +++++++--- ...urationPropertiesScopingPostProcessor.java | 15 ++- .../config/SpringBootConfigStorage.java | 18 ++-- .../pkb/common/testing/IntegrationTest.java | 28 ------ ...pringBootConfigStorageIntegrationTest.java | 80 ++++++++++++++++ .../config/SpringBootConfigStorageTest.java | 94 +++++++++++++++++++ .../src/test/resources/application.properties | 2 + 17 files changed, 283 insertions(+), 133 deletions(-) create mode 100644 config/src/main/java/com/pkb/common/config/AbstractMutableConfigStorage.java delete mode 100644 config/src/main/java/com/pkb/common/config/ImmutableConfigStorage.java delete mode 100644 config/src/main/java/com/pkb/common/config/MutableConfigStorage.java delete mode 100644 spring-boot-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java create mode 100644 spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageIntegrationTest.java create mode 100644 spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java create mode 100644 spring-boot-infrastructure/src/test/resources/application.properties diff --git a/config/src/main/java/com/pkb/common/config/AbstractMutableConfigStorage.java b/config/src/main/java/com/pkb/common/config/AbstractMutableConfigStorage.java new file mode 100644 index 00000000..7f4458ec --- /dev/null +++ b/config/src/main/java/com/pkb/common/config/AbstractMutableConfigStorage.java @@ -0,0 +1,14 @@ +package com.pkb.common.config; + +public abstract class AbstractMutableConfigStorage extends AbstractBaseConfigStorage { + + @Override + public void setValue(String key, String value) { + if (!MUTABLE_CONFIG_KEY.equals(key)) { + setValueInternal(key, value); + } + } + + abstract void setValueInternal(String key, String value); + +} diff --git a/config/src/main/java/com/pkb/common/config/ConfigStorage.java b/config/src/main/java/com/pkb/common/config/ConfigStorage.java index e8cd73ac..c9519e59 100644 --- a/config/src/main/java/com/pkb/common/config/ConfigStorage.java +++ b/config/src/main/java/com/pkb/common/config/ConfigStorage.java @@ -20,7 +20,9 @@ public interface ConfigStorage { Long getLong(String key, long defaultValue); - boolean isMutableConfigEnabled(); + default boolean isMutableConfigEnabled() { + return getBoolean(MUTABLE_CONFIG_KEY, false); + } void setValue(String key, String value); @@ -28,5 +30,4 @@ public interface ConfigStorage { void reset(); - ImmutableConfigStorage getImmutableConfig(); } diff --git a/config/src/main/java/com/pkb/common/config/ConfigStorageFactory.java b/config/src/main/java/com/pkb/common/config/ConfigStorageFactory.java index 9832703b..5304f647 100644 --- a/config/src/main/java/com/pkb/common/config/ConfigStorageFactory.java +++ b/config/src/main/java/com/pkb/common/config/ConfigStorageFactory.java @@ -10,7 +10,7 @@ public static ConfigStorage getConfigStorage() { } - public static ConfigStorage getConfigStorage(ImmutableConfigStorage baseStorage) { + public static ConfigStorage getConfigStorage(ConfigStorage baseStorage) { if (baseStorage.isMutableConfigEnabled()) { return new MutableRawConfigStorage(baseStorage); } diff --git a/config/src/main/java/com/pkb/common/config/ImmutableConfigStorage.java b/config/src/main/java/com/pkb/common/config/ImmutableConfigStorage.java deleted file mode 100644 index 637e046e..00000000 --- a/config/src/main/java/com/pkb/common/config/ImmutableConfigStorage.java +++ /dev/null @@ -1,30 +0,0 @@ -package com.pkb.common.config; - -public interface ImmutableConfigStorage extends ConfigStorage { - - @Override - default ImmutableConfigStorage getImmutableConfig() { - return this; - } - - @Override - default boolean isMutableConfigEnabled() { - return getBoolean(MUTABLE_CONFIG_KEY, false); - } - - @Override - default void setValue(String key, String value) { - /* no-op */ - } - - @Override - default OverrideRemovalResult removeOverrideAtKey(String key) { - return OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE; - } - - @Override - default void reset() { - /* no-op */ - } - -} diff --git a/config/src/main/java/com/pkb/common/config/ImmutableRawConfigStorage.java b/config/src/main/java/com/pkb/common/config/ImmutableRawConfigStorage.java index bed247c7..a3ccb9e5 100644 --- a/config/src/main/java/com/pkb/common/config/ImmutableRawConfigStorage.java +++ b/config/src/main/java/com/pkb/common/config/ImmutableRawConfigStorage.java @@ -1,12 +1,12 @@ package com.pkb.common.config; -import java.util.HashMap; -import java.util.Map; - import static java.util.Collections.emptyMap; import static java.util.Collections.unmodifiableMap; -public final class ImmutableRawConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage { +import java.util.HashMap; +import java.util.Map; + +public final class ImmutableRawConfigStorage extends AbstractBaseConfigStorage { public static final ImmutableRawConfigStorage EMPTY = new ImmutableRawConfigStorage(emptyMap()); @@ -36,4 +36,19 @@ public String getString(String key, String defaultValue) { return storage.getOrDefault(key, defaultValue); } + @Override + public void setValue(String key, String value) { + //No-op + } + + @Override + public OverrideRemovalResult removeOverrideAtKey(String key) { + return OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE; + } + + @Override + public void reset() { + //No-op + } + } diff --git a/config/src/main/java/com/pkb/common/config/MutableConfigStorage.java b/config/src/main/java/com/pkb/common/config/MutableConfigStorage.java deleted file mode 100644 index c8030d79..00000000 --- a/config/src/main/java/com/pkb/common/config/MutableConfigStorage.java +++ /dev/null @@ -1,10 +0,0 @@ -package com.pkb.common.config; - -public interface MutableConfigStorage extends ConfigStorage { - - @Override - default boolean isMutableConfigEnabled() { - return true; - } - -} diff --git a/config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java b/config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java index 07d7a9cd..21271117 100644 --- a/config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java +++ b/config/src/main/java/com/pkb/common/config/MutableRawConfigStorage.java @@ -4,12 +4,12 @@ import java.util.Map; import java.util.function.Supplier; -public final class MutableRawConfigStorage extends AbstractBaseConfigStorage implements MutableConfigStorage { +public final class MutableRawConfigStorage extends AbstractMutableConfigStorage { private final Map overrideMap = new HashMap<>(); - private final ImmutableConfigStorage configStorage; + private final ConfigStorage configStorage; - MutableRawConfigStorage(ImmutableConfigStorage configStorage) { + MutableRawConfigStorage(ConfigStorage configStorage) { this.configStorage = configStorage; } @@ -24,10 +24,8 @@ public String getString(String key, String defaultValue) { } @Override - public void setValue(String key, String value) { - if (!MUTABLE_CONFIG_KEY.equals(key)) { - overrideMap.put(key, value); - } + void setValueInternal(String key, String value) { + overrideMap.put(key, value); } @Override @@ -45,15 +43,15 @@ public void reset() { overrideMap.clear(); } - @Override - public ImmutableConfigStorage getImmutableConfig() { - return configStorage; - } - private String getOverriddenOrOriginalValue(String key, Supplier originalSupplier) { if (overrideMap.containsKey(key)) { return overrideMap.get(key); } return originalSupplier.get(); } + + @Override + public boolean isMutableConfigEnabled() { + return true; + } } diff --git a/config/src/test/java/com/pkb/common/config/ImmutableRawConfigStorageTest.java b/config/src/test/java/com/pkb/common/config/ImmutableRawConfigStorageTest.java index d909f723..c270cba5 100644 --- a/config/src/test/java/com/pkb/common/config/ImmutableRawConfigStorageTest.java +++ b/config/src/test/java/com/pkb/common/config/ImmutableRawConfigStorageTest.java @@ -4,7 +4,6 @@ import static com.github.karsaig.approvalcrest.jupiter.MatcherAssert.assertThrows; import static com.github.karsaig.approvalcrest.jupiter.matcher.Matchers.sameJsonAsApproved; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.sameInstance; import static org.junit.jupiter.api.Assertions.assertEquals; import java.util.HashMap; @@ -197,9 +196,4 @@ public void removingNonExistentKey() { assertEquals(OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE, removalResult); } - @Test - void getImmutableConfigReturnsSelf() { - ConfigStorage configStorage = underTest.getImmutableConfig(); - assertThat(configStorage, sameInstance(underTest)); - } } diff --git a/config/src/test/java/com/pkb/common/config/MutableRawConfigStorageTest.java b/config/src/test/java/com/pkb/common/config/MutableRawConfigStorageTest.java index 094ff1ab..087d2f71 100644 --- a/config/src/test/java/com/pkb/common/config/MutableRawConfigStorageTest.java +++ b/config/src/test/java/com/pkb/common/config/MutableRawConfigStorageTest.java @@ -4,7 +4,6 @@ import static com.github.karsaig.approvalcrest.jupiter.MatcherAssert.assertThrows; import static com.github.karsaig.approvalcrest.jupiter.matcher.Matchers.sameJsonAsApproved; import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.sameInstance; import static org.junit.jupiter.api.Assertions.assertEquals; import java.util.HashMap; @@ -195,11 +194,6 @@ void mutableConfigCannotBeOverridden() { assertThat(underTest.isMutableConfigEnabled(), is(true)); } - @Test - void getImmutableConfigReturnsOriginalImmutableConfig() { - ConfigStorage configStorage = underTest.getImmutableConfig(); - assertThat(configStorage, sameInstance(immutableRawConfigStorage)); - } @DisplayName("getInt returns original value without override") @Test diff --git a/pom.xml b/pom.xml index 1fec326f..ac36d7a7 100644 --- a/pom.xml +++ b/pom.xml @@ -56,6 +56,7 @@ 20-789c720-63866 Hoxton.SR8 5.3.15.Final + 2.3.5.RELEASE @@ -254,17 +255,22 @@ spring-context ${spring.version} - org.springframework - spring-test + spring-beans ${spring.version} - compile + + + org.springframework.boot + spring-boot-test + ${spring.boot.version} + test org.springframework - spring-beans + spring-test ${spring.version} + test diff --git a/spring-boot-infrastructure/pom.xml b/spring-boot-infrastructure/pom.xml index 6c964cfe..0041334c 100644 --- a/spring-boot-infrastructure/pom.xml +++ b/spring-boot-infrastructure/pom.xml @@ -14,27 +14,22 @@ org.springframework - spring-test - compile + spring-beans - org.springframework - spring-beans + org.springframework.boot + spring-boot-starter + ${spring.boot.version} + test org.springframework.cloud - spring-cloud-context - - - org.springframework.security - spring-security-crypto - - + spring-cloud-starter + test - org.junit.jupiter - junit-jupiter-api - compile + org.springframework.cloud + spring-cloud-context com.pkb.pkbcommon @@ -44,6 +39,30 @@ org.springframework spring-context + + com.github.karsaig + approvalcrest-junit-jupiter + + + org.hamcrest + hamcrest + + + org.mockito + mockito-core + + + org.mockito + mockito-junit-jupiter + + + org.springframework.boot + spring-boot-test + + + org.springframework + spring-test + \ No newline at end of file diff --git a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java index 46a8c1af..a7ed3159 100644 --- a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java +++ b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/ConfigurationPropertiesScopingPostProcessor.java @@ -1,21 +1,22 @@ package com.pkb.common.config; +import java.util.Optional; + +import javax.annotation.ParametersAreNonnullByDefault; + import org.springframework.aop.scope.ScopedProxyUtils; import org.springframework.beans.BeansException; import org.springframework.beans.factory.config.BeanDefinition; import org.springframework.beans.factory.config.BeanDefinitionHolder; import org.springframework.beans.factory.config.ConfigurableListableBeanFactory; -import org.springframework.beans.factory.config.Scope; import org.springframework.beans.factory.support.BeanDefinitionRegistry; import org.springframework.beans.factory.support.BeanDefinitionRegistryPostProcessor; import org.springframework.beans.factory.support.GenericBeanDefinition; +import org.springframework.context.annotation.Scope; import org.springframework.context.annotation.ScopedProxyMode; import org.springframework.core.annotation.AnnotationAttributes; import org.springframework.core.type.AnnotationMetadata; -import javax.annotation.ParametersAreNonnullByDefault; -import java.util.Optional; - /** * This post-processor works around a bug/limitation of Spring Boot/Spring Cloud whereby * beans discovered by the @EnableConfigurationProperties or @ConfigurationPropertiesScan annotations @@ -31,13 +32,17 @@ @ParametersAreNonnullByDefault public class ConfigurationPropertiesScopingPostProcessor implements BeanDefinitionRegistryPostProcessor { + private static final String SCOPE_ANNOTATION = Scope.class.getName(); + @Override public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) throws BeansException { for (String name : registry.getBeanDefinitionNames()) { BeanDefinition beanDefinition = registry.getBeanDefinition(name); if (beanDefinition.getClass().getName().equals("org.springframework.boot.context.properties.ConfigurationPropertiesValueObjectBeanDefinition")) { GenericBeanDefinition gbd = (GenericBeanDefinition) beanDefinition; - Optional annAttrs = Optional.ofNullable(AnnotationAttributes.fromMap(AnnotationMetadata.introspect(gbd.getBeanClass()).getAnnotationAttributes(Scope.class.getName(), false))); + Optional annAttrs = Optional + .ofNullable(AnnotationAttributes.fromMap(AnnotationMetadata.introspect(gbd.getBeanClass()).getAnnotationAttributes( + SCOPE_ANNOTATION, false))); ScopedProxyMode scopedProxyMode = annAttrs.map(attrs -> attrs.getEnum("proxyMode")).orElse(ScopedProxyMode.NO); gbd.setScope(annAttrs.map(attrs -> attrs.getString("value")).orElse(gbd.getScope())); if (scopedProxyMode == ScopedProxyMode.NO) { diff --git a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java index 37a650c6..59a2ec3a 100644 --- a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java +++ b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java @@ -1,12 +1,14 @@ package com.pkb.common.config; +import java.util.LinkedHashMap; +import java.util.Map; + +import javax.annotation.ParametersAreNonnullByDefault; + import org.springframework.cloud.context.scope.refresh.RefreshScope; import org.springframework.core.env.ConfigurableEnvironment; import org.springframework.core.env.MapPropertySource; -import javax.annotation.ParametersAreNonnullByDefault; -import java.util.LinkedHashMap; -import java.util.Map; /** * This class provides an implementation of the PKB ConfigStorage interface that interacts @@ -43,7 +45,7 @@ * infrastructure that may have been wired together according to their values - are dynamically updated. */ @ParametersAreNonnullByDefault -public class SpringBootConfigStorage extends AbstractBaseConfigStorage implements ImmutableConfigStorage { +public class SpringBootConfigStorage extends AbstractMutableConfigStorage { private final ConfigurableEnvironment environment; private final RefreshScope refreshScope; private final Map overrides = new LinkedHashMap<>(); @@ -66,12 +68,7 @@ public String getString(String key, String defaultValue) { } @Override - public boolean isMutableConfigEnabled() { - return getBoolean("mutableConfig.enabled", false); - } - - @Override - public void setValue(String key, String value) { + public void setValueInternal(String key, String value) { if (isMutableConfigEnabled()) { overrides.put(key, value); refreshScope.refreshAll(); @@ -99,4 +96,3 @@ public void reset() { } } - diff --git a/spring-boot-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java b/spring-boot-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java deleted file mode 100644 index fa015a92..00000000 --- a/spring-boot-infrastructure/src/main/java/com/pkb/common/testing/IntegrationTest.java +++ /dev/null @@ -1,28 +0,0 @@ -package com.pkb.common.testing; - -import java.lang.annotation.Documented; -import java.lang.annotation.ElementType; -import java.lang.annotation.Inherited; -import java.lang.annotation.Retention; -import java.lang.annotation.RetentionPolicy; -import java.lang.annotation.Target; - -import org.junit.jupiter.api.Tag; -import org.springframework.test.context.ActiveProfiles; - -/** - * This tag helps organise integration tests for spring boot applications. - * When applied to a junit 5 test, it enables it to be filtered in maven - * using "groups" in the surefire/failsafe configuration. - * - * In addition, it applies the spring profile "integration-test" allowing - * selective configuration overrides. - */ -@Target(ElementType.TYPE) -@Retention(RetentionPolicy.RUNTIME) -@Inherited -@ActiveProfiles("integration-test") -@Documented -@Tag("integration-test") -public @interface IntegrationTest { -} diff --git a/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageIntegrationTest.java b/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageIntegrationTest.java new file mode 100644 index 00000000..0296b2ad --- /dev/null +++ b/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageIntegrationTest.java @@ -0,0 +1,80 @@ +package com.pkb.common.config; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.autoconfigure.EnableAutoConfiguration; +import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.boot.context.properties.ConstructorBinding; +import org.springframework.boot.context.properties.EnableConfigurationProperties; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.cloud.context.scope.refresh.RefreshScope; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.annotation.Order; +import org.springframework.core.env.ConfigurableEnvironment; + +import com.pkb.common.config.SpringBootConfigStorageIntegrationTest.TestConfiguration.TestConfigProperties; + +@SpringBootTest +public class SpringBootConfigStorageIntegrationTest { + + @Configuration + @EnableConfigurationProperties(TestConfigProperties.class) + @EnableAutoConfiguration + public static class TestConfiguration { + @Bean + @Order + public static ConfigurationPropertiesScopingPostProcessor configurationPropertiesScopingPostProcessor() { + return new ConfigurationPropertiesScopingPostProcessor(); + } + + @Bean + public SpringBootConfigStorage configStorage(ConfigurableEnvironment environment, RefreshScope refreshScope) { + return new SpringBootConfigStorage(environment, refreshScope); + } + + @ConfigurationProperties(prefix = "com.pkb.common.config") + @ConstructorBinding + @org.springframework.cloud.context.config.annotation.RefreshScope + public static class TestConfigProperties { + private final int testVal; + + public TestConfigProperties(int testVal) { + this.testVal = testVal; + } + + public int getTestVal() { + return testVal; + } + } + } + + @Autowired + private SpringBootConfigStorage configStorage; + + @Autowired + TestConfigProperties props; + + @Test + void returnsValueFromAppProperties() { + assertThat(props.getTestVal(), equalTo(10)); + } + + @Test + void overridesCorrectly() { + configStorage.setValue("com.pkb.common.config.test-val", "50"); + assertThat(props.getTestVal(), equalTo(50)); + } + + @Test + void resetsCorrectly() { + configStorage.setValue("com.pkb.common.config.test-val", "50"); + configStorage.reset(); + assertThat(props.getTestVal(), equalTo(10)); + + } + +} diff --git a/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java b/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java new file mode 100644 index 00000000..ecee2fdd --- /dev/null +++ b/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java @@ -0,0 +1,94 @@ +package com.pkb.common.config; + +import static com.github.karsaig.approvalcrest.jupiter.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + +import java.util.HashMap; +import java.util.Map; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.cloud.context.scope.refresh.RefreshScope; +import org.springframework.core.env.MapPropertySource; +import org.springframework.core.env.StandardEnvironment; + +@ExtendWith(MockitoExtension.class) +class SpringBootConfigStorageTest { + + private final Map lowPriorityProperties = new HashMap<>(); + + private final StandardEnvironment environment = new StandardEnvironment(); + + @Mock + private RefreshScope refreshScope; + + private SpringBootConfigStorage underTest; + + @BeforeEach + void setup() { + underTest = new SpringBootConfigStorage(environment, refreshScope); + environment.getPropertySources().addLast(new MapPropertySource("standardProperties", lowPriorityProperties)); + } + + @Test + void cantOverrideMutableConfigProperty() { + assertThat(underTest.isMutableConfigEnabled(), equalTo(false)); + underTest.setValue(ConfigStorage.MUTABLE_CONFIG_KEY, "true"); + assertThat(underTest.getBoolean(ConfigStorage.MUTABLE_CONFIG_KEY, false), equalTo(false)); + assertThat(underTest.isMutableConfigEnabled(), equalTo(false)); + } + + @Test + void returnsOverrideForNormalProperty() { + lowPriorityProperties.put(ConfigStorage.MUTABLE_CONFIG_KEY, "true"); + lowPriorityProperties.put("myProp", "myVal"); + assertThat(underTest.getString("myProp"), equalTo("myVal")); + + underTest.setValue("myProp", "myOverrideValue"); + assertThat(underTest.getString("myProp"), equalTo("myOverrideValue")); + } + + @Test + void fallsBackToDefaultForNonExistentProperty() { + assertThat(underTest.getString("myProp", "myVal"), equalTo("myVal")); + } + + @Test + void resetsOverrideForPropertyCorrectly() { + lowPriorityProperties.put(ConfigStorage.MUTABLE_CONFIG_KEY, "true"); + lowPriorityProperties.put("myProp", "myVal"); + underTest.setValue("myProp", "myOverrideValue"); + underTest.reset(); + assertThat(underTest.getString("myProp"), equalTo("myVal")); + } + + @Test + void removesOverrideForPropertyCorrectly() { + lowPriorityProperties.put(ConfigStorage.MUTABLE_CONFIG_KEY, "true"); + lowPriorityProperties.put("myProp", "myVal"); + underTest.setValue("myProp", "myOverrideValue"); + assertThat(underTest.removeOverrideAtKey("myProp"), equalTo(OverrideRemovalResult.REMOVED)); + assertThat(underTest.getString("myProp"), equalTo("myVal")); + } + + @Test + void failsToRemoveNonExistentOverride() { + lowPriorityProperties.put(ConfigStorage.MUTABLE_CONFIG_KEY, "true"); + lowPriorityProperties.put("myProp", "myVal"); + underTest.setValue("myProp", "myOverrideValue"); + assertThat(underTest.removeOverrideAtKey("myProp2"), equalTo(OverrideRemovalResult.KEY_NOT_FOUND)); + assertThat(underTest.getString("myProp"), equalTo("myOverrideValue")); + } + + @Test + void skipsOverrideRemovalForNonMutableConfig() { + lowPriorityProperties.put("myProp", "myVal"); + underTest.setValue("myProp", "myOverrideValue"); + assertThat(underTest.removeOverrideAtKey("myProp2"), equalTo(OverrideRemovalResult.NO_OP_AS_CONFIG_IS_IMMUTABLE)); + assertThat(underTest.getString("myProp"), equalTo("myVal")); + } + +} diff --git a/spring-boot-infrastructure/src/test/resources/application.properties b/spring-boot-infrastructure/src/test/resources/application.properties new file mode 100644 index 00000000..68d5c440 --- /dev/null +++ b/spring-boot-infrastructure/src/test/resources/application.properties @@ -0,0 +1,2 @@ +com.pkb.common.config.test-val=10 +mutableConfig.enabled=true \ No newline at end of file From 0168455e4e721d78d62cf8168e7e381a6ad081af Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Tue, 10 Nov 2020 12:31:28 +0100 Subject: [PATCH 5/8] Refix OWASP QUES-36 --- suppression.xml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/suppression.xml b/suppression.xml index d45e2fc1..a6152f82 100644 --- a/suppression.xml +++ b/suppression.xml @@ -259,4 +259,11 @@ ]]> CVE-2019-14900 + + + ^pkg:maven/org\.springframework\.security/spring\-security\-crypto@.*$ + CVE-2018-1258 + From 451246e74efa64133755b5f5ee626e18c16e050f Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Tue, 10 Nov 2020 13:32:15 +0100 Subject: [PATCH 6/8] Set Spring deps to provided to avoid version conflicts QUES-36 --- pom.xml | 2 ++ spring-boot-infrastructure/pom.xml | 1 + 2 files changed, 3 insertions(+) diff --git a/pom.xml b/pom.xml index ac36d7a7..f26f76c3 100644 --- a/pom.xml +++ b/pom.xml @@ -254,11 +254,13 @@ org.springframework spring-context ${spring.version} + provided org.springframework spring-beans ${spring.version} + provided org.springframework.boot diff --git a/spring-boot-infrastructure/pom.xml b/spring-boot-infrastructure/pom.xml index 0041334c..10ea1491 100644 --- a/spring-boot-infrastructure/pom.xml +++ b/spring-boot-infrastructure/pom.xml @@ -30,6 +30,7 @@ org.springframework.cloud spring-cloud-context + provided com.pkb.pkbcommon From cbaa928f1e4afe433f0421835ff2994545fae4d0 Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Tue, 10 Nov 2020 17:05:27 +0100 Subject: [PATCH 7/8] Fix incorrect method visibility of internal method. QUES-36 --- .../java/com/pkb/common/config/SpringBootConfigStorage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java index 59a2ec3a..dffce6e2 100644 --- a/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java +++ b/spring-boot-infrastructure/src/main/java/com/pkb/common/config/SpringBootConfigStorage.java @@ -68,7 +68,7 @@ public String getString(String key, String defaultValue) { } @Override - public void setValueInternal(String key, String value) { + void setValueInternal(String key, String value) { if (isMutableConfigEnabled()) { overrides.put(key, value); refreshScope.refreshAll(); From cf475a743ff244b3290dbcbf0728aacba4cdf921 Mon Sep 17 00:00:00 2001 From: Lucian Holland Date: Tue, 10 Nov 2020 17:59:43 +0100 Subject: [PATCH 8/8] Add another test for immutability of mutableConfig.enabled property. QUES-36 --- .../common/config/SpringBootConfigStorageTest.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java b/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java index ecee2fdd..7e993573 100644 --- a/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java +++ b/spring-boot-infrastructure/src/test/java/com/pkb/common/config/SpringBootConfigStorageTest.java @@ -34,13 +34,22 @@ void setup() { } @Test - void cantOverrideMutableConfigProperty() { + void cantOverrideMutableConfigPropertyWhenSetToFalse() { assertThat(underTest.isMutableConfigEnabled(), equalTo(false)); underTest.setValue(ConfigStorage.MUTABLE_CONFIG_KEY, "true"); assertThat(underTest.getBoolean(ConfigStorage.MUTABLE_CONFIG_KEY, false), equalTo(false)); assertThat(underTest.isMutableConfigEnabled(), equalTo(false)); } + @Test + void cantOverrideMutableConfigPropertyWhenSetToTrue() { + lowPriorityProperties.put(ConfigStorage.MUTABLE_CONFIG_KEY, "true"); + assertThat(underTest.isMutableConfigEnabled(), equalTo(true)); + underTest.setValue(ConfigStorage.MUTABLE_CONFIG_KEY, "false"); + assertThat(underTest.getBoolean(ConfigStorage.MUTABLE_CONFIG_KEY, false), equalTo(true)); + assertThat(underTest.isMutableConfigEnabled(), equalTo(true)); + } + @Test void returnsOverrideForNormalProperty() { lowPriorityProperties.put(ConfigStorage.MUTABLE_CONFIG_KEY, "true");