From fe39d1b5ebba9d489c330f61d7e2d4d95a695d5f Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Mon, 15 Jan 2024 12:33:49 -0500 Subject: [PATCH] fix: provide better error messages for list options (#25918) closes: #25235 Signed-off-by: Steve Hawkins --- .../org/keycloak/config/DatabaseOptions.java | 2 +- .../org/keycloak/config/FeatureOptions.java | 8 +-- .../java/org/keycloak/config/HttpOptions.java | 7 +- .../org/keycloak/config/LoggingOptions.java | 10 +-- .../java/org/keycloak/config/MultiOption.java | 19 ------ .../main/java/org/keycloak/config/Option.java | 18 +++-- .../org/keycloak/config/OptionBuilder.java | 68 +++++++------------ .../org/keycloak/config/SecurityOptions.java | 2 +- .../keycloak/config/TruststoreOptions.java | 4 +- .../keycloak/quarkus/runtime/cli/Picocli.java | 11 ++- .../cli/PropertyMapperParameterConsumer.java | 39 ++++------- .../mappers/FeaturePropertyMappers.java | 62 ++++++++--------- .../configuration/mappers/PropertyMapper.java | 50 ++++++++++---- .../test/FeaturePropertyMappersTest.java | 12 ++-- .../keycloak/it/cli/OptionValidationTest.java | 18 ++++- .../keycloak/it/cli/dist/LoggingDistTest.java | 4 +- .../src/main/java/org/keycloak/Keycloak.java | 30 ++------ .../it/junit5/extension/CLITestExtension.java | 34 +++++----- 18 files changed, 184 insertions(+), 214 deletions(-) delete mode 100644 quarkus/config-api/src/main/java/org/keycloak/config/MultiOption.java diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/DatabaseOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/DatabaseOptions.java index 20fc1adda8..4cc9e71c05 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/DatabaseOptions.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/DatabaseOptions.java @@ -19,7 +19,7 @@ public class DatabaseOptions { .category(OptionCategory.DATABASE) .description("The database vendor.") .defaultValue("dev-file") - .expectedValues(Database::getDatabaseAliases) + .expectedValues(Database.getDatabaseAliases()) .buildTime(true) .build(); diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/FeatureOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/FeatureOptions.java index def3a450e1..c9e4c0e91a 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/FeatureOptions.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/FeatureOptions.java @@ -10,18 +10,18 @@ import java.util.stream.Collectors; public class FeatureOptions { - public static final Option FEATURES = new OptionBuilder("features", List.class, Profile.Feature.class) + public static final Option> FEATURES = OptionBuilder.listOptionBuilder("features", String.class) .category(OptionCategory.FEATURE) .description("Enables a set of one or more features.") .defaultValue(Optional.empty()) - .expectedValues(() -> getFeatureValues(true)) + .expectedValues(getFeatureValues(true)) .buildTime(true) .build(); - public static final Option FEATURES_DISABLED = new OptionBuilder("features-disabled", List.class, Profile.Feature.class) + public static final Option> FEATURES_DISABLED = OptionBuilder.listOptionBuilder("features-disabled", String.class) .category(OptionCategory.FEATURE) .description("Disables a set of one or more features.") - .expectedValues(() -> getFeatureValues(false)) + .expectedValues(getFeatureValues(false)) .buildTime(true) .build(); diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java index 137f74a36c..3bc778ab68 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/HttpOptions.java @@ -1,6 +1,9 @@ package org.keycloak.config; import java.io.File; +import java.util.Arrays; +import java.util.List; + import org.keycloak.common.crypto.FipsMode; public class HttpOptions { @@ -54,10 +57,10 @@ public class HttpOptions { .description("The cipher suites to use. If none is given, a reasonable default is selected.") .build(); - public static final Option HTTPS_PROTOCOLS = new OptionBuilder<>("https-protocols", String.class) + public static final Option> HTTPS_PROTOCOLS = OptionBuilder.listOptionBuilder("https-protocols", String.class) .category(OptionCategory.HTTP) .description("The list of protocols to explicitly enable.") - .defaultValue("TLSv1.3,TLSv1.2") + .defaultValue(Arrays.asList("TLSv1.3,TLSv1.2")) .build(); public static final Option HTTPS_CERTIFICATE_FILE = new OptionBuilder<>("https-certificate-file", File.class) diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/LoggingOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/LoggingOptions.java index 8a95729148..5a00eb07b9 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/LoggingOptions.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/LoggingOptions.java @@ -30,11 +30,11 @@ public class LoggingOptions { .toList(); } - public static final Option LOG = new OptionBuilder("log", List.class, Handler.class) + public static final Option> LOG = OptionBuilder.listOptionBuilder("log", Handler.class) .category(OptionCategory.LOGGING) .description("Enable one or more log handlers in a comma-separated list.") - .expectedValues(() -> getAvailableHandlerNames()) - .defaultValue(DEFAULT_LOG_HANDLER) + .expectedValues(getAvailableHandlerNames()) + .defaultValue(Arrays.asList(DEFAULT_LOG_HANDLER)) .build(); public enum Level { @@ -53,9 +53,9 @@ public class LoggingOptions { } } - public static final Option LOG_LEVEL = new OptionBuilder<>("log-level", String.class) + public static final Option> LOG_LEVEL = OptionBuilder.listOptionBuilder("log-level", String.class) .category(OptionCategory.LOGGING) - .defaultValue(DEFAULT_LOG_LEVEL.toString()) + .defaultValue(Arrays.asList(DEFAULT_LOG_LEVEL.toString())) .description("The log level of the root category or a comma-separated list of individual categories and their levels. For the root category, you don't need to specify a category.") .build(); diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/MultiOption.java b/quarkus/config-api/src/main/java/org/keycloak/config/MultiOption.java deleted file mode 100644 index 9d8261cc27..0000000000 --- a/quarkus/config-api/src/main/java/org/keycloak/config/MultiOption.java +++ /dev/null @@ -1,19 +0,0 @@ -package org.keycloak.config; - -import java.util.List; -import java.util.Optional; -import java.util.function.Supplier; - -public class MultiOption extends Option { - - private final Class auxiliaryType; - - public MultiOption(Class type, Class auxiliaryType, String key, OptionCategory category, boolean hidden, boolean buildTime, String description, Optional defaultValue, Supplier> expectedValues, DeprecatedMetadata deprecatedMetadata) { - super(type, key, category, hidden, buildTime, description, defaultValue, expectedValues, deprecatedMetadata); - this.auxiliaryType = auxiliaryType; - } - - public Class getAuxiliaryType() { - return auxiliaryType; - } -} diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/Option.java b/quarkus/config-api/src/main/java/org/keycloak/config/Option.java index ebf70a3335..cc15b9b702 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/Option.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/Option.java @@ -2,7 +2,7 @@ package org.keycloak.config; import java.util.List; import java.util.Optional; -import java.util.function.Supplier; +import java.util.stream.Collectors; public class Option { @@ -13,10 +13,10 @@ public class Option { private final boolean buildTime; private final String description; private final Optional defaultValue; - private final Supplier> expectedValues; + private final List expectedValues; private final DeprecatedMetadata deprecatedMetadata; - public Option(Class type, String key, OptionCategory category, boolean hidden, boolean buildTime, String description, Optional defaultValue, Supplier> expectedValues, DeprecatedMetadata deprecatedMetadata) { + public Option(Class type, String key, OptionCategory category, boolean hidden, boolean buildTime, String description, Optional defaultValue, List expectedValues, DeprecatedMetadata deprecatedMetadata) { this.type = type; this.key = key; this.category = category; @@ -53,7 +53,7 @@ public class Option { } public List getExpectedValues() { - return expectedValues.get(); + return expectedValues; } public Optional getDeprecatedMetadata() { @@ -90,4 +90,14 @@ public class Option { return description; } + + public static String getDefaultValueString(Object value) { + if (value == null) { + return null; + } + if (value instanceof List) { + return ((List) value).stream().map(String::valueOf).collect(Collectors.joining(",")); + } + return String.valueOf(value); + } } diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java b/quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java index 61270293ce..beee0e391b 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/OptionBuilder.java @@ -1,56 +1,54 @@ package org.keycloak.config; +import java.util.Collection; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.Set; -import java.util.function.Supplier; import java.util.stream.Collectors; public class OptionBuilder { - private static final Supplier> EMPTY_VALUES_SUPPLIER = List::of; - private static final Supplier> BOOLEAN_TYPE_VALUES = new Supplier>() { - List values = List.of(Boolean.TRUE.toString(), Boolean.FALSE.toString()); - - @Override - public List get() { - return values; - } - }; + private static final List BOOLEAN_TYPE_VALUES = List.of(Boolean.TRUE.toString(), Boolean.FALSE.toString()); private final Class type; - private final Class auxiliaryType; private final String key; private OptionCategory category; private boolean hidden; private boolean build; private String description; private Optional defaultValue; - private Supplier> expectedValues; + private List expectedValues = List.of(); private DeprecatedMetadata deprecatedMetadata; + public static OptionBuilder> listOptionBuilder(String key, Class type) { + return new OptionBuilder(key, List.class, type); + } + public OptionBuilder(String key, Class type) { this(key, type, null); } - public OptionBuilder(String key, Class type, Class auxiliaryType) { + private OptionBuilder(String key, Class type, Class auxiliaryType) { this.type = type; - this.auxiliaryType = auxiliaryType; + if (type.isArray() || ((Collection.class.isAssignableFrom(type) || Map.class.isAssignableFrom(type)) && type != java.util.List.class)) { + throw new IllegalArgumentException("Non-List multi-valued options are not yet supported"); + } this.key = key; category = OptionCategory.GENERAL; hidden = false; build = false; description = null; - defaultValue = Boolean.class.equals(type) ? Optional.of((T) Boolean.FALSE) : Optional.empty(); - expectedValues = EMPTY_VALUES_SUPPLIER; - if (Boolean.class.equals(type)) { + Class expected = type; + if (auxiliaryType != null) { + expected = auxiliaryType; + } + defaultValue = Boolean.class.equals(expected) ? Optional.of((T) Boolean.FALSE) : Optional.empty(); + if (Boolean.class.equals(expected)) { expectedValues(BOOLEAN_TYPE_VALUES); } - if (Enum.class.isAssignableFrom(type)) { - expectedValues((Class) type); - } - if (auxiliaryType != null && Enum.class.isAssignableFrom(auxiliaryType)) { - expectedValues((Class) auxiliaryType); + if (Enum.class.isAssignableFrom(expected)) { + expectedValues((Class) expected); } } @@ -84,32 +82,18 @@ public class OptionBuilder { return this; } - public OptionBuilder expectedValues(Supplier> expected) { + public OptionBuilder expectedValues(List expected) { this.expectedValues = expected; return this; } public OptionBuilder expectedValues(Class expected) { - this.expectedValues = new Supplier<>() { - List values = List.of(expected.getEnumConstants()).stream().map(Object::toString).collect(Collectors.toList()); - - @Override - public List get() { - return values; - } - }; + this.expectedValues = List.of(expected.getEnumConstants()).stream().map(Object::toString).collect(Collectors.toList()); return this; } public OptionBuilder expectedValues(T ... expected) { - this.expectedValues = new Supplier<>() { - List values = List.of(expected).stream().map(v -> v.toString()).collect(Collectors.toList()); - - @Override - public List get() { - return values; - } - }; + this.expectedValues = List.of(expected).stream().map(v -> v.toString()).collect(Collectors.toList()); return this; } @@ -135,11 +119,7 @@ public class OptionBuilder { public Option build() { - if (auxiliaryType != null) { - return new MultiOption(type, auxiliaryType, key, category, hidden, build, description, defaultValue, expectedValues, deprecatedMetadata); - } else { - return new Option(type, key, category, hidden, build, description, defaultValue, expectedValues, deprecatedMetadata); - } + return new Option(type, key, category, hidden, build, description, defaultValue, expectedValues, deprecatedMetadata); } } diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/SecurityOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/SecurityOptions.java index 6434162403..236dfd74b6 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/SecurityOptions.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/SecurityOptions.java @@ -10,7 +10,7 @@ public class SecurityOptions { public static final Option FIPS_MODE = new OptionBuilder<>("fips-mode", FipsMode.class) .category(OptionCategory.SECURITY) - .expectedValues(SecurityOptions::getFipsModeValues) + .expectedValues(getFipsModeValues()) .buildTime(true) .description("Sets the FIPS mode. If '" + FipsMode.NON_STRICT + "' is set, FIPS is enabled but on non-approved mode. For full FIPS compliance, set '" + FipsMode.STRICT + "' to run on approved mode. " + "This option defaults to '" + FipsMode.DISABLED + "' when '" + Profile.Feature.FIPS.getKey() + "' feature is disabled, which is by default. " diff --git a/quarkus/config-api/src/main/java/org/keycloak/config/TruststoreOptions.java b/quarkus/config-api/src/main/java/org/keycloak/config/TruststoreOptions.java index 3d0240d8e0..d5e395f5a2 100644 --- a/quarkus/config-api/src/main/java/org/keycloak/config/TruststoreOptions.java +++ b/quarkus/config-api/src/main/java/org/keycloak/config/TruststoreOptions.java @@ -2,9 +2,11 @@ package org.keycloak.config; import org.keycloak.common.enums.HostnameVerificationPolicy; +import java.util.List; + public class TruststoreOptions { - public static final Option TRUSTSTORE_PATHS = new OptionBuilder<>("truststore-paths", String.class) + public static final Option> TRUSTSTORE_PATHS = OptionBuilder.listOptionBuilder("truststore-paths", String.class) .category(OptionCategory.TRUSTSTORE) .description("List of pkcs12 (p12 or pfx file extensions), PEM files, or directories containing those files that will be used as a system truststore.") .build(); diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java index f1f72bdb39..68b5b0cd0f 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/Picocli.java @@ -56,7 +56,7 @@ import java.util.stream.Collectors; import org.eclipse.microprofile.config.spi.ConfigSource; import org.jboss.logging.Logger; import org.keycloak.config.DeprecatedMetadata; -import org.keycloak.config.MultiOption; +import org.keycloak.config.Option; import org.keycloak.config.OptionCategory; import org.keycloak.quarkus.runtime.cli.command.AbstractCommand; import org.keycloak.quarkus.runtime.cli.command.Build; @@ -399,7 +399,7 @@ public final class Picocli { if (runtimeValue == null && isNotBlank(persistedValue)) { PropertyMapper mapper = PropertyMappers.getMapper(propertyName); - if (mapper != null && persistedValue.equals(mapper.getDefaultValue().map(Object::toString).orElse(null))) { + if (mapper != null && persistedValue.equals(Option.getDefaultValueString(mapper.getDefaultValue().orElse(null)))) { // same as default continue; } @@ -608,14 +608,11 @@ public final class Picocli { .hidden(mapper.isHidden()); if (mapper.getDefaultValue().isPresent()) { - optBuilder.defaultValue(mapper.getDefaultValue().get().toString()); + optBuilder.defaultValue(Option.getDefaultValueString(mapper.getDefaultValue().get())); } if (mapper.getType() != null) { optBuilder.type(mapper.getType()); - if (mapper.getOption() instanceof MultiOption) { - optBuilder.auxiliaryTypes(((MultiOption) mapper.getOption()).getAuxiliaryType()); - } } else { optBuilder.type(String.class); } @@ -639,7 +636,7 @@ public final class Picocli { } mapper.getDefaultValue() - .map(d -> d.toString().replaceAll("%", "%%")) // escape formats + .map(d -> Option.getDefaultValueString(d).replaceAll("%", "%%")) // escape formats .map(d -> " Default: " + d + ".") .ifPresent(transformedDesc::append); diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/PropertyMapperParameterConsumer.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/PropertyMapperParameterConsumer.java index a6be2a8527..93332381f8 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/PropertyMapperParameterConsumer.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/cli/PropertyMapperParameterConsumer.java @@ -19,11 +19,8 @@ package org.keycloak.quarkus.runtime.cli; import static org.keycloak.quarkus.runtime.cli.Picocli.ARG_PREFIX; -import java.util.Collection; import java.util.Stack; -import org.keycloak.utils.StringUtil; - import picocli.CommandLine; import picocli.CommandLine.Model.ArgSpec; import picocli.CommandLine.Model.CommandSpec; @@ -52,21 +49,27 @@ public final class PropertyMapperParameterConsumer implements CommandLine.IParam CommandLine commandLine = commandSpec.commandLine(); if (args.isEmpty() || !isOptionValue(args.peek())) { - throw new ParameterException( - commandLine, "Missing required value for option '" + name + "' (" + argSpec.paramLabel() + ")." + getExpectedValuesMessage(argSpec.completionCandidates(), option.completionCandidates())); + throw new ParameterException(commandLine, + "Missing required value. " + getExpectedMessage(argSpec, option, name)); } // consumes the value, actual value validation will be performed later args.pop(); if (!args.isEmpty() && isOptionValue(args.peek())) { - throw new ParameterException( - commandLine, "Option '" + name + "' expects a single value (" + argSpec.paramLabel() + ")" + getExpectedValuesMessage(argSpec.completionCandidates(), option.completionCandidates())); + throw new ParameterException(commandLine, getExpectedMessage(argSpec, option, name)); } } - public static String getErrorMessage(String name, String value, Iterable specCandidates, Iterable optionCandidates) { - return "Invalid value for option '" + name + "': " + value + "." + getExpectedValuesMessage(specCandidates, optionCandidates); + private String getExpectedMessage(ArgSpec argSpec, OptionSpec option, String name) { + return String.format("Option '%s' (%s) expects %s.%s", name, argSpec.paramLabel(), + option.typeInfo().isMultiValue() ? "one or more comma separated values without whitespace": "a single value", + getExpectedValuesMessage(argSpec.completionCandidates(), option.completionCandidates())); + } + + public static String getErrorMessage(String name, String value, Iterable expected) { + return String.format("Invalid value for option '%s': %s.%s", name, value, + getExpectedValuesMessage(expected, expected)); } private boolean isOptionValue(String arg) { @@ -77,22 +80,4 @@ public final class PropertyMapperParameterConsumer implements CommandLine.IParam return optionCandidates.iterator().hasNext() ? " Expected values are: " + String.join(", ", specCandidates) : ""; } - public static boolean isExpectedValue(Collection expectedValues, String value) { - if (expectedValues.isEmpty()) { - // accept any - return true; - } - - if (StringUtil.isBlank(value)) { - return false; - } - - for (String v : value.split(",")) { - if (!expectedValues.contains(v)) { - return false; - } - } - - return true; - } } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/FeaturePropertyMappers.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/FeaturePropertyMappers.java index 4829bc4d3c..73c457b50b 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/FeaturePropertyMappers.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/FeaturePropertyMappers.java @@ -9,7 +9,6 @@ import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; -import java.util.stream.Stream; import static org.keycloak.quarkus.runtime.configuration.mappers.PropertyMapper.fromOption; @@ -24,7 +23,8 @@ public final class FeaturePropertyMappers { return new PropertyMapper[] { fromOption(FeatureOptions.FEATURES) .paramLabel("feature") - .validator((mapper, value) -> validateEnabledFeatures(value.getValue())) + .validator((mapper, value) -> mapper.validateExpectedValues(value, + (c, v) -> validateEnabledFeature(v))) .build(), fromOption(FeatureOptions.FEATURES_DISABLED) .paramLabel("feature") @@ -32,36 +32,34 @@ public final class FeaturePropertyMappers { }; } - public static void validateEnabledFeatures(String s) { - Stream.of(s.split(",")).forEach(feature -> { - if (!Profile.getFeatureVersions(feature).isEmpty()) { - return; + public static void validateEnabledFeature(String feature) { + if (!Profile.getFeatureVersions(feature).isEmpty()) { + return; + } + if (feature.equals(Profile.Feature.Type.PREVIEW.name().toLowerCase())) { + return; + } + Matcher matcher = VERSIONED_PATTERN.matcher(feature); + if (!matcher.matches()) { + if (feature.contains(":")) { + throw new PropertyException(String.format( + "%s has an invalid format for enabling a feature, expected format is feature:v{version}, e.g. docker:v1", + feature)); } - if (feature.equals(Profile.Feature.Type.PREVIEW.name().toLowerCase())) { - return; - } - Matcher matcher = VERSIONED_PATTERN.matcher(feature); - if (!matcher.matches()) { - if (feature.contains(":")) { - throw new PropertyException(String.format( - "%s has an invalid format for enabling a feature, expected format is feature:v{version}, e.g. docker:v1", - feature)); - } - throw new PropertyException(String.format("%s is an unrecognized feature, it should be one of %s", feature, - FeatureOptions.getFeatureValues(false))); - } - String unversionedFeature = matcher.group(1); - Set featureVersions = Profile.getFeatureVersions(unversionedFeature); - if (featureVersions.isEmpty()) { - throw new PropertyException(String.format("%s has an unrecognized feature, it should be one of %s", - feature, FeatureOptions.getFeatureValues(false))); - } - int version = Integer.parseInt(matcher.group(2)); - if (featureVersions.stream().noneMatch(f -> f.getVersion() == version)) { - throw new PropertyException( - String.format("%s has an unrecognized feature version, it should be one of %s", feature, - featureVersions.stream().map(Feature::getVersion).map(String::valueOf).collect(Collectors.toList()))); - } - }); + throw new PropertyException(String.format("%s is an unrecognized feature, it should be one of %s", feature, + FeatureOptions.getFeatureValues(false))); + } + String unversionedFeature = matcher.group(1); + Set featureVersions = Profile.getFeatureVersions(unversionedFeature); + if (featureVersions.isEmpty()) { + throw new PropertyException(String.format("%s has an unrecognized feature, it should be one of %s", + feature, FeatureOptions.getFeatureValues(false))); + } + int version = Integer.parseInt(matcher.group(2)); + if (!featureVersions.stream().anyMatch(f -> f.getVersion() == version)) { + throw new PropertyException( + String.format("%s has an unrecognized feature version, it should be one of %s", feature, + featureVersions.stream().map(Feature::getVersion).map(String::valueOf).collect(Collectors.toList()))); + } } } diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java index bf99bf5feb..cbd7093e37 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/configuration/mappers/PropertyMapper.java @@ -24,7 +24,6 @@ import static org.keycloak.quarkus.runtime.configuration.Configuration.toCliForm import static org.keycloak.quarkus.runtime.configuration.Configuration.toEnvVarFormat; import java.util.List; -import java.util.Objects; import java.util.Optional; import java.util.function.BiConsumer; import java.util.function.BiFunction; @@ -32,7 +31,6 @@ import java.util.function.BiFunction; import io.smallrye.config.ConfigSourceInterceptorContext; import io.smallrye.config.ConfigValue; -import org.jboss.logging.Logger; import org.keycloak.config.DeprecatedMetadata; import org.keycloak.config.Option; import org.keycloak.config.OptionBuilder; @@ -69,8 +67,6 @@ public class PropertyMapper { private String cliFormat; private BiConsumer, ConfigValue> validator; - private static final Logger logger = Logger.getLogger(PropertyMapper.class); - PropertyMapper(Option option, String to, BiFunction, ConfigSourceInterceptorContext, Optional> mapper, String mapFrom, String paramLabel, boolean mask, BiConsumer, ConfigValue> validator) { this.option = option; @@ -119,14 +115,14 @@ public class PropertyMapper { PropertyMapper parentMapper = PropertyMappers.getMapper(parentKey); if (parentMapper != null && parentMapper.getDefaultValue().isPresent()) { - parentValue = ConfigValue.builder().withValue(parentMapper.getDefaultValue().get().toString()).build(); + parentValue = ConfigValue.builder().withValue(Option.getDefaultValueString(parentMapper.getDefaultValue().get())).build(); } } return transformValue(name, ofNullable(parentValue == null ? null : parentValue.getValue()), context, null); } - ConfigValue defaultValue = transformValue(name, this.option.getDefaultValue().map(Objects::toString), context, null); + ConfigValue defaultValue = transformValue(name, this.option.getDefaultValue().map(Option::getDefaultValueString), context, null); if (defaultValue != null) { return defaultValue; @@ -242,7 +238,7 @@ public class PropertyMapper { private String mapFrom = null; private boolean isMasked = false; private String paramLabel; - private BiConsumer, ConfigValue> validator = (mapper, value) -> mapper.validateExpectedValues(value); + private BiConsumer, ConfigValue> validator = (mapper, value) -> mapper.validateExpectedValues(value, (c, v) -> mapper.validateSingleValue(c, v)); public Builder(Option option) { this.option = option; @@ -296,15 +292,39 @@ public class PropertyMapper { } } - public void validateExpectedValues(ConfigValue value) { - if (PropertyMapperParameterConsumer.isExpectedValue(getExpectedValues(), value.getValue())) { - return; + public void validateExpectedValues(ConfigValue configValue, BiConsumer singleValidator) { + String value = configValue.getValue(); + + boolean multiValued = getOption().getType() == java.util.List.class; + + String[] values = multiValued ? value.split(",") : new String[] { value }; + for (String v : values) { + boolean cli = isCliOption(configValue); + if (multiValued && !v.trim().equals(v)) { + throw new PropertyException("Invalid value for multivalued option '" + (cli ? this.getCliFormat() : getFrom()) + + "': list value '" + v + "' should not have leading nor trailing whitespace" + + getConfigSourceMessage(configValue, cli)); + } + singleValidator.accept(configValue, v); } - boolean cli = Optional.ofNullable(value.getConfigSourceName()).filter(name -> name.contains(ConfigArgsConfigSource.NAME)).isPresent(); - throw new PropertyException( - PropertyMapperParameterConsumer.getErrorMessage(cli ? this.getCliFormat() : getFrom(), - value.getValue(), getExpectedValues(), getExpectedValues()) - + (cli ? "" : ". From ConfigSource " + value.getConfigSourceName())); + } + + private boolean isCliOption(ConfigValue configValue) { + return Optional.ofNullable(configValue.getConfigSourceName()).filter(name -> name.contains(ConfigArgsConfigSource.NAME)).isPresent(); + } + + void validateSingleValue(ConfigValue configValue, String v) { + List expectedValues = getExpectedValues(); + if (!expectedValues.isEmpty() && !expectedValues.contains(v)) { + boolean cli = isCliOption(configValue); + throw new PropertyException( + PropertyMapperParameterConsumer.getErrorMessage(cli ? this.getCliFormat() : getFrom(), v, + expectedValues) + getConfigSourceMessage(configValue, cli)); + } + } + + String getConfigSourceMessage(ConfigValue configValue, boolean cli) { + return cli ? "" : ". From ConfigSource " + configValue.getConfigSourceName(); } } diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/FeaturePropertyMappersTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/FeaturePropertyMappersTest.java index 27f32709e5..17e5b3f055 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/FeaturePropertyMappersTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/configuration/test/FeaturePropertyMappersTest.java @@ -28,28 +28,28 @@ public class FeaturePropertyMappersTest { @Test public void testInvalidFeatureFormat() { - assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeatures("invalid:")); + assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeature("invalid:")); } @Test public void testInvalidFeature() { - assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeatures("invalid")); + assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeature("invalid")); } @Test public void testInvalidVersionedFeature() { - assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeatures("invalid:v1")); + assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeature("invalid:v1")); } @Test public void testInvalidFeatureVersion() { - assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeatures(Feature.DOCKER.getUnversionedKey() + ":v0")); + assertThrows(PropertyException.class, () -> FeaturePropertyMappers.validateEnabledFeature(Feature.DOCKER.getUnversionedKey() + ":v0")); } @Test public void testValidFeatures() { - FeaturePropertyMappers.validateEnabledFeatures( - Feature.DOCKER.getUnversionedKey() + "," + "preview" + "," + Feature.ACCOUNT2.getVersionedKey()); + FeaturePropertyMappers.validateEnabledFeature("preview"); + FeaturePropertyMappers.validateEnabledFeature(Feature.ACCOUNT2.getVersionedKey()); } } diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/OptionValidationTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/OptionValidationTest.java index 98727b187f..62ecafc91a 100644 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/OptionValidationTest.java +++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/OptionValidationTest.java @@ -36,14 +36,28 @@ public class OptionValidationTest { @Launch({"build", "--db"}) public void failMissingOptionValue(LaunchResult result) { CLIResult cliResult = (CLIResult) result; - assertThat(cliResult.getErrorOutput(), containsString("Missing required value for option '--db' (vendor). Expected values are: dev-file, dev-mem, mariadb, mssql, mysql, oracle, postgres")); + assertThat(cliResult.getErrorOutput(), containsString("Missing required value. Option '--db' (vendor) expects a single value. Expected values are: dev-file, dev-mem, mariadb, mssql, mysql, oracle, postgres")); } @Test @Launch({"build", "--db", "foo", "bar"}) public void failMultipleOptionValue(LaunchResult result) { CLIResult cliResult = (CLIResult) result; - assertThat(cliResult.getErrorOutput(), containsString("Option '--db' expects a single value (vendor) Expected values are: dev-file, dev-mem, mariadb, mssql, mysql, oracle, postgres")); + assertThat(cliResult.getErrorOutput(), containsString("Option '--db' (vendor) expects a single value. Expected values are: dev-file, dev-mem, mariadb, mssql, mysql, oracle, postgres")); + } + + @Test + @Launch({"build", "--features", "account2", "account3"}) + public void failMultipleMultiOptionValue(LaunchResult result) { + CLIResult cliResult = (CLIResult) result; + assertThat(cliResult.getErrorOutput(), containsString("Option '--features' (feature) expects one or more comma separated values without whitespace. Expected values are: ")); + } + + @Test + @Launch({"build", "--features", "xyz,account2"}) + public void failInvalidMultiOptionValue(LaunchResult result) { + CLIResult cliResult = (CLIResult) result; + assertThat(cliResult.getErrorOutput(), containsString("xyz is an unrecognized feature, it should be one of")); } @Test diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/LoggingDistTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/LoggingDistTest.java index c69f246cc1..7bf2253de1 100644 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/LoggingDistTest.java +++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/LoggingDistTest.java @@ -145,7 +145,7 @@ public class LoggingDistTest { void failUnknownHandlersInConfFile(KeycloakDistribution dist) { dist.copyOrReplaceFileFromClasspath("/logging/keycloak.conf", Paths.get("conf", "keycloak.conf")); CLIResult cliResult = dist.run("start-dev"); - cliResult.assertError("Invalid value for option 'kc.log': foo,console. Expected values are: console, file, gelf."); + cliResult.assertError("Invalid value for option 'kc.log': foo. Expected values are: console, file, gelf."); } @Test @@ -159,7 +159,7 @@ public class LoggingDistTest { @Launch({ "start-dev","--log=foo,bar" }) void failUnknownHandlersInCliCommand(LaunchResult result) { CLIResult cliResult = (CLIResult) result; - cliResult.assertError("Invalid value for option '--log': foo,bar"); + cliResult.assertError("Invalid value for option '--log': foo"); } @Test diff --git a/quarkus/tests/junit5/src/main/java/org/keycloak/Keycloak.java b/quarkus/tests/junit5/src/main/java/org/keycloak/Keycloak.java index 749e264b9f..fbdf47eafb 100644 --- a/quarkus/tests/junit5/src/main/java/org/keycloak/Keycloak.java +++ b/quarkus/tests/junit5/src/main/java/org/keycloak/Keycloak.java @@ -28,7 +28,6 @@ import java.util.concurrent.TimeoutException; import org.eclipse.microprofile.config.spi.ConfigProviderResolver; import org.keycloak.common.Version; import org.keycloak.common.crypto.FipsMode; -import org.keycloak.config.DatabaseOptions; import org.keycloak.config.HttpOptions; import org.keycloak.config.LoggingOptions; import org.keycloak.config.Option; @@ -117,7 +116,7 @@ public class Keycloak { addOptionIfNotSet(args, HttpOptions.HTTP_PORT); addOptionIfNotSet(args, HttpOptions.HTTPS_PORT); - boolean isFipsEnabled = ofNullable(getOptionValue(args, SecurityOptions.FIPS_MODE)).orElse(FipsMode.DISABLED).isFipsEnabled(); + boolean isFipsEnabled = ofNullable(getOptionValue(args, SecurityOptions.FIPS_MODE)).map(FipsMode::valueOf).orElse(FipsMode.DISABLED).isFipsEnabled(); if (isFipsEnabled) { String logLevel = getOptionValue(args, LoggingOptions.LOG_LEVEL); @@ -135,41 +134,22 @@ public class Keycloak { } private void addOptionIfNotSet(List args, Option option, T defaultValue) { - T value = getOptionValue(args, option); + String value = getOptionValue(args, option); if (value == null) { defaultValue = ofNullable(defaultValue).orElseGet(option.getDefaultValue()::get); - args.add(Configuration.toCliFormat(option.getKey()) + "=" + defaultValue); + args.add(Configuration.toCliFormat(option.getKey()) + "=" + Option.getDefaultValueString(defaultValue)); } } - private T getOptionValue(List args, Option option) { + private String getOptionValue(List args, Option option) { for (String arg : args) { if (arg.contains(option.getKey())) { if (arg.endsWith(option.getKey())) { throw new IllegalArgumentException("Option '" + arg + "' value must be set using '=' as a separator"); } - String value = arg.substring(Picocli.ARG_PREFIX.length() + option.getKey().length() + 1); - Class type = option.getType(); - - if (type.equals(String.class)) { - return (T) value; - } - - if (type.isEnum()) { - return (T) Enum.valueOf((Class) type, value); - } - - if (Integer.class.isAssignableFrom(type)) { - return (T) Integer.valueOf(value); - } - - if (Boolean.class.isAssignableFrom(type)) { - return (T) Boolean.valueOf(value); - } - - throw new RuntimeException("Unsupported option type '" + type + "'"); + return arg.substring(Picocli.ARG_PREFIX.length() + option.getKey().length() + 1); } } diff --git a/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java b/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java index 97301d807b..4937a930fa 100644 --- a/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java +++ b/quarkus/tests/junit5/src/main/java/org/keycloak/it/junit5/extension/CLITestExtension.java @@ -22,7 +22,10 @@ import io.quarkus.runtime.configuration.QuarkusConfigFactory; import io.quarkus.test.junit.QuarkusMainTestExtension; import io.quarkus.test.junit.main.Launch; import io.quarkus.test.junit.main.LaunchResult; + import org.junit.jupiter.api.extension.ExtensionContext; +import org.junit.jupiter.api.extension.ExtensionContext.Namespace; +import org.junit.jupiter.api.extension.ExtensionContext.Store; import org.junit.jupiter.api.extension.ParameterContext; import org.junit.jupiter.api.extension.ParameterResolutionException; import org.junit.jupiter.api.extension.ReflectiveInvocationContext; @@ -35,20 +38,20 @@ import org.keycloak.quarkus.runtime.cli.command.StartDev; import org.keycloak.quarkus.runtime.configuration.KeycloakPropertiesConfigSource; import org.keycloak.quarkus.runtime.configuration.test.TestConfigArgsConfigSource; import org.keycloak.quarkus.runtime.integration.QuarkusPlatform; -import org.testcontainers.containers.GenericContainer; import java.io.IOException; import java.lang.annotation.Annotation; import java.lang.reflect.Method; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; +import java.util.HashMap; import java.util.List; -import java.util.Set; +import java.util.Properties; import java.util.regex.Pattern; import java.util.stream.Collectors; import java.util.stream.Stream; +import static java.lang.System.setProperty; import static org.keycloak.it.junit5.extension.DistributionTest.ReInstall.BEFORE_ALL; import static org.keycloak.it.junit5.extension.DistributionType.RAW; import static org.keycloak.quarkus.runtime.Environment.forceTestLaunchMode; @@ -57,9 +60,9 @@ import static org.keycloak.quarkus.runtime.cli.command.Main.CONFIG_FILE_SHORT_NA public class CLITestExtension extends QuarkusMainTestExtension { + private static final String SYS_PROPS = "sys-props"; private static final String KEY_VALUE_SEPARATOR = "[= ]"; private KeycloakDistribution dist; - private final Set testSysProps = new HashSet<>(); private DatabaseContainer databaseContainer; private InfinispanContainer infinispanContainer; private CLIResult result; @@ -68,6 +71,7 @@ public class CLITestExtension extends QuarkusMainTestExtension { public void beforeEach(ExtensionContext context) throws Exception { DistributionTest distConfig = getDistributionConfig(context); Launch launch = context.getRequiredTestMethod().getAnnotation(Launch.class); + getStore(context).put(SYS_PROPS, new HashMap<>(System.getProperties())); if (launch != null) { for (String arg : launch.value()) { @@ -117,6 +121,10 @@ public class CLITestExtension extends QuarkusMainTestExtension { } } + private Store getStore(ExtensionContext context) { + return context.getStore(Namespace.create(context.getRequiredTestClass(), context.getRequiredTestMethod())); + } + private static LegacyStore getLegacyStoreConfig(ExtensionContext context) { return context.getTestClass().get().getDeclaredAnnotation(LegacyStore.class); } @@ -180,18 +188,15 @@ public class CLITestExtension extends QuarkusMainTestExtension { } super.afterEach(context); - reset(distConfig); + reset(distConfig, context); } - private void reset(DistributionTest distConfig) { + private void reset(DistributionTest distConfig, ExtensionContext context) { QuarkusConfigFactory.setConfig(null); - //remove the config file property if set, and also the profile, to not have side effects in other tests. - System.getProperties().remove(Environment.PROFILE); - System.getProperties().remove("quarkus.profile"); + HashMap props = getStore(context).remove(SYS_PROPS, HashMap.class); + System.getProperties().clear(); + System.getProperties().putAll(props); TestConfigArgsConfigSource.setCliArgs(new String[0]); - for (String property : testSysProps) { - System.getProperties().remove(property); - } if (databaseContainer != null && databaseContainer.isRunning()) { databaseContainer.stop(); databaseContainer = null; @@ -367,11 +372,6 @@ public class CLITestExtension extends QuarkusMainTestExtension { } } - private void setProperty(String name, String value) { - System.setProperty(name, value); - testSysProps.add(name); - } - private List getCliArgs(ExtensionContext context) { Launch annotation = context.getRequiredTestMethod().getAnnotation(Launch.class);