From 799201f406bdc8c1475ac913cbf8232302dc044f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1clav=20Muzik=C3=A1=C5=99?= Date: Fri, 16 Aug 2024 11:25:46 +0200 Subject: [PATCH] Fix duplicate options in `show-config` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #32182 Signed-off-by: Václav Muzikář --- .../configuration/KcEnvConfigSource.java | 21 +++-- .../configuration/test/ConfigurationTest.java | 2 +- .../it/cli/ShowConfigCommandTest.java | 76 ------------------- .../cli/dist/ShowConfigCommandDistTest.java | 63 +++++++++++++++ .../keycloak-keystore.conf | 2 +- 5 files changed, 79 insertions(+), 85 deletions(-) delete mode 100644 quarkus/tests/integration/src/test/java/org/keycloak/it/cli/ShowConfigCommandTest.java diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java index 4d97a6a076..5167e52cf6 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/KcEnvConfigSource.java @@ -18,34 +18,35 @@ package org.keycloak.quarkus.runtime.configuration; import static io.smallrye.config.common.utils.StringUtil.replaceNonAlphanumericByUnderscores; +import static org.keycloak.quarkus.runtime.configuration.MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX; import java.util.HashMap; import java.util.Map; +import io.smallrye.config.PropertiesConfigSource; import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMapper; import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers; -import io.smallrye.config.EnvConfigSource; - -public class KcEnvConfigSource extends EnvConfigSource { +// Not extending EnvConfigSource as it's too smart for our own good. It does unnecessary mapping of provided keys +// leading to e.g. duplicate entries (like kc.db-password and kc.db.password), or incorrectly handling getters due to +// how equals() is implemented. We don't need that here as we do our own mapping. +public class KcEnvConfigSource extends PropertiesConfigSource { public static final String NAME = "KcEnvVarConfigSource"; public KcEnvConfigSource() { - super(buildProperties(), 500); + super(buildProperties(), NAME, 500); } private static Map buildProperties() { Map properties = new HashMap<>(); - String kcPrefix = replaceNonAlphanumericByUnderscores(MicroProfileConfigProvider.NS_KEYCLOAK_PREFIX.toUpperCase()); + String kcPrefix = replaceNonAlphanumericByUnderscores(NS_KEYCLOAK_PREFIX.toUpperCase()); for (Map.Entry entry : System.getenv().entrySet()) { String key = entry.getKey(); String value = entry.getValue(); if (key.startsWith(kcPrefix)) { - properties.put(key, value); - PropertyMapper mapper = PropertyMappers.getMapper(key); if (mapper != null) { @@ -57,6 +58,11 @@ public class KcEnvConfigSource extends EnvConfigSource { properties.put(mapper.getFrom(), value); } + else { + // most probably an SPI but could be also something else + String transformedKey = NS_KEYCLOAK_PREFIX + key.substring(kcPrefix.length()).toLowerCase().replace("_", "-"); + properties.put(transformedKey, value); + } } } @@ -64,6 +70,7 @@ public class KcEnvConfigSource extends EnvConfigSource { } @Override + // a workaround for https://github.com/smallrye/smallrye-config/issues/1207 public String getName() { return NAME; } diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java index f6d03a692f..8f06ede9dc 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/ConfigurationTest.java @@ -183,7 +183,7 @@ public class ConfigurationTest extends AbstractConfigurationTest { System.getProperties().remove("kc.spi-client-registration-openid-connect-static-jwk-url"); putEnvVar("KC_SPI_CLIENT_REGISTRATION_OPENID_CONNECT_STATIC_JWK_URL", "http://c.jwk.url/from-env"); config = initConfig("client-registration", "openid-connect"); - assertEquals(1, config.getPropertyNames().size()); + assertEquals(2, config.getPropertyNames().size()); // transformed name is coming from KcEnvVarConfigSource, raw env var name is coming from EnvVarConfigSource assertEquals("http://c.jwk.url/from-env", config.get("static-jwk-url")); } diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/ShowConfigCommandTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/ShowConfigCommandTest.java deleted file mode 100644 index 5d126e65e3..0000000000 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/ShowConfigCommandTest.java +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright 2021 Red Hat, Inc. and/or its affiliates - * and other contributors as indicated by the @author tags. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package org.keycloak.it.cli; - -import io.quarkus.test.common.QuarkusTestResource; -import io.quarkus.test.junit.main.Launch; -import io.quarkus.test.junit.main.LaunchResult; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; -import org.keycloak.it.junit5.extension.CLITest; -import org.keycloak.it.junit5.extension.ConfigurationTestResource; -import org.keycloak.quarkus.runtime.cli.command.ShowConfig; -import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.not; -import static org.keycloak.quarkus.runtime.cli.command.Main.CONFIG_FILE_LONG_NAME; - -@QuarkusTestResource(value = ConfigurationTestResource.class, restrictToAnnotatedClass = true) -@CLITest -public class ShowConfigCommandTest { - - @Test - @Launch({ ShowConfig.NAME }) - void testShowConfigCommandShowsRuntimeConfig(LaunchResult result) { - Assertions.assertTrue(result.getOutput() - .contains("Current Configuration")); - } - - @Test - @Launch({ ShowConfig.NAME, "all" }) - void testShowConfigCommandWithAllShowsAllProfiles(LaunchResult result) { - Assertions.assertTrue(result.getOutput() - .contains("Current Configuration")); - Assertions.assertTrue(result.getOutput() - .contains("Quarkus Configuration")); - } - - @Test - @Launch({ CONFIG_FILE_LONG_NAME+"=src/test/resources/ShowConfigCommandTest/keycloak.conf", ShowConfig.NAME, "all" }) - void testShowConfigCommandHidesCredentialsInProfiles(LaunchResult result) { - String output = result.getOutput(); - Assertions.assertFalse(output.contains("testpw1")); - Assertions.assertFalse(output.contains("testpw2")); - Assertions.assertFalse(output.contains("testpw3")); - Assertions.assertTrue(output.contains("kc.db-password = " + PropertyMappers.VALUE_MASK)); - } - - @Test - @Launch({ CONFIG_FILE_LONG_NAME+"=src/test/resources/ShowConfigCommandTest/keycloak-keystore.conf", ShowConfig.NAME, "all" }) - void testSmallRyeKeyStoreConfigSource(LaunchResult result) { - // keystore is shared with QuarkusPropertiesDistTest#testSmallRyeKeyStoreConfigSource - String output = result.getOutput(); - assertThat(output, containsString("kc.config-keystore-password = " + PropertyMappers.VALUE_MASK)); - assertThat(output, containsString("kc.log-level = " + PropertyMappers.VALUE_MASK)); - - assertThat(output, not(containsString("secret"))); - assertThat(output, not(containsString("debug"))); - } -} diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ShowConfigCommandDistTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ShowConfigCommandDistTest.java index 7d2694f213..07605c2196 100644 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ShowConfigCommandDistTest.java +++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/ShowConfigCommandDistTest.java @@ -1,10 +1,23 @@ package org.keycloak.it.cli.dist; +import io.quarkus.test.junit.main.Launch; +import io.quarkus.test.junit.main.LaunchResult; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.keycloak.it.junit5.extension.CLIResult; import org.keycloak.it.junit5.extension.DistributionTest; import org.keycloak.it.junit5.extension.RawDistOnly; +import org.keycloak.it.junit5.extension.WithEnvVars; import org.keycloak.it.utils.KeycloakDistribution; +import org.keycloak.quarkus.runtime.cli.command.ShowConfig; +import org.keycloak.quarkus.runtime.configuration.mappers.PropertyMappers; + +import java.nio.file.Paths; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.keycloak.quarkus.runtime.cli.command.Main.CONFIG_FILE_LONG_NAME; @DistributionTest public class ShowConfigCommandDistTest { @@ -28,4 +41,54 @@ public class ShowConfigCommandDistTest { resetResult.assertMessage("Current Mode: production"); resetResult.assertMessage("kc.db = dev-file"); } + + @Test + @Launch({ ShowConfig.NAME }) + void testShowConfigCommandShowsRuntimeConfig(LaunchResult result) { + Assertions.assertTrue(result.getOutput() + .contains("Current Configuration")); + } + + @Test + @Launch({ ShowConfig.NAME, "all" }) + void testShowConfigCommandWithAllShowsAllProfiles(LaunchResult result) { + Assertions.assertTrue(result.getOutput() + .contains("Current Configuration")); + Assertions.assertTrue(result.getOutput() + .contains("Quarkus Configuration")); + } + + @Test + @RawDistOnly(reason = "Containers are immutable") + void testShowConfigCommandHidesCredentialsInProfiles(KeycloakDistribution distribution) { + CLIResult result = distribution.run(String.format("%s=%s", CONFIG_FILE_LONG_NAME, Paths.get("src/test/resources/ShowConfigCommandTest/keycloak.conf").toAbsolutePath().normalize()), ShowConfig.NAME, "all"); + String output = result.getOutput(); + Assertions.assertFalse(output.contains("testpw1")); + Assertions.assertFalse(output.contains("testpw2")); + Assertions.assertFalse(output.contains("testpw3")); + Assertions.assertTrue(output.contains("kc.db-password = " + PropertyMappers.VALUE_MASK)); + } + + @Test + @RawDistOnly(reason = "Containers are immutable") + void testSmallRyeKeyStoreConfigSource(KeycloakDistribution distribution) { + // keystore is shared with QuarkusPropertiesDistTest#testSmallRyeKeyStoreConfigSource + CLIResult result = distribution.run(String.format("%s=%s", CONFIG_FILE_LONG_NAME, Paths.get("src/test/resources/ShowConfigCommandTest/keycloak-keystore.conf").toAbsolutePath().normalize()), ShowConfig.NAME, "all"); + String output = result.getOutput(); + assertThat(output, containsString("kc.config-keystore-password = " + PropertyMappers.VALUE_MASK)); + assertThat(output, containsString("kc.log-level = " + PropertyMappers.VALUE_MASK)); + + assertThat(output, not(containsString("secret"))); + assertThat(output, not(containsString("debug"))); + } + + @Test + @Launch({ ShowConfig.NAME }) + @WithEnvVars({"KC_DB_PASSWORD", "secret-pass"}) + void testNoDuplicitEnvVarEntries(LaunchResult result) { + String output = result.getOutput(); + assertThat(output, containsString("kc.db-password = " + PropertyMappers.VALUE_MASK)); + assertThat(output, not(containsString("kc.db.password"))); + assertThat(output, not(containsString("secret-pass"))); + } } diff --git a/quarkus/tests/integration/src/test/resources/ShowConfigCommandTest/keycloak-keystore.conf b/quarkus/tests/integration/src/test/resources/ShowConfigCommandTest/keycloak-keystore.conf index 6a9edddf94..5f72985d50 100644 --- a/quarkus/tests/integration/src/test/resources/ShowConfigCommandTest/keycloak-keystore.conf +++ b/quarkus/tests/integration/src/test/resources/ShowConfigCommandTest/keycloak-keystore.conf @@ -1,2 +1,2 @@ -config-keystore=src/test/resources/keystore +config-keystore=../../../../src/test/resources/keystore config-keystore-password=secret \ No newline at end of file