From 422eadecf41f96e844b935ffc3a2c95f4ce6ab40 Mon Sep 17 00:00:00 2001 From: Steven Hawkins Date: Thu, 23 Oct 2025 09:46:35 -0400 Subject: [PATCH] fix: adding type validation and lazily adding cli options (#43467) * fix: adding type validation and lazily adding cli options closes: #43466 Signed-off-by: Steve Hawkins * consolidating empty value checking Signed-off-by: Steve Hawkins * stripping the smallrye code if possible Signed-off-by: Steve Hawkins --------- Signed-off-by: Steve Hawkins --- .../main/java/org/keycloak/config/Option.java | 8 +- .../org/keycloak/config/OptionBuilder.java | 2 +- .../keycloak/quarkus/runtime/cli/Picocli.java | 164 +++++++++--------- .../configuration/mappers/PropertyMapper.java | 21 +++ .../quarkus/runtime/cli/PicocliTest.java | 23 ++- .../cli/dist/CacheEmbeddedMtlsDistTest.java | 2 +- 6 files changed, 129 insertions(+), 91 deletions(-) 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 71eb99433fc..6d2122bb703 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 @@ -9,6 +9,7 @@ import java.util.stream.Collectors; public class Option { private final Class type; + private final Class componentType; private final String key; private final OptionCategory category; private final boolean hidden; @@ -24,7 +25,7 @@ public class Option { public Option(Class type, String key, OptionCategory category, boolean hidden, boolean buildTime, String description, Optional defaultValue, List expectedValues, boolean strictExpectedValues, boolean caseInsensitiveExpectedValues, - DeprecatedMetadata deprecatedMetadata, Set connectedOptions, String wildcardKey) { + DeprecatedMetadata deprecatedMetadata, Set connectedOptions, String wildcardKey, Class componentType) { this.type = type; this.key = key; this.category = category; @@ -38,6 +39,7 @@ public class Option { this.deprecatedMetadata = deprecatedMetadata; this.connectedOptions = connectedOptions; this.wildcardKey = wildcardKey; + this.componentType = componentType; } public Class getType() { @@ -168,4 +170,8 @@ public class Option { public static String transformEnumValue(String value) { return CaseFormat.UPPER_UNDERSCORE.to(CaseFormat.LOWER_HYPHEN, value); } + + public Class getComponentType() { + return componentType; + } } 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 a96d903218d..5d696c68b83 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 @@ -194,7 +194,7 @@ public class OptionBuilder { } } - return new Option(type, key, category, hidden, build, description, defaultValue, expectedValues, strictExpectedValues, caseInsensitiveExpectedValues, deprecatedMetadata, connectedOptions, wildcardKey); + return new Option(type, key, category, hidden, build, description, defaultValue, expectedValues, strictExpectedValues, caseInsensitiveExpectedValues, deprecatedMetadata, connectedOptions, wildcardKey, expected); } } 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 01389ea6c52..b4582c0bd6b 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 @@ -39,12 +39,9 @@ import java.util.Map; import java.util.Optional; import java.util.Properties; import java.util.Set; -import java.util.function.Consumer; import java.util.function.Function; -import java.util.stream.Collectors; import org.keycloak.common.profile.ProfileException; -import org.keycloak.common.util.CollectionUtil; import org.keycloak.config.DeprecatedMetadata; import org.keycloak.config.Option; import org.keycloak.config.OptionCategory; @@ -77,6 +74,7 @@ import picocli.CommandLine.Help.Ansi; import picocli.CommandLine.Help.Ansi.Style; import picocli.CommandLine.Help.ColorScheme; import picocli.CommandLine.IFactory; +import picocli.CommandLine.MissingParameterException; import picocli.CommandLine.Model.ArgGroupSpec; import picocli.CommandLine.Model.CommandSpec; import picocli.CommandLine.Model.ISetter; @@ -99,8 +97,6 @@ public class Picocli { } private final ExecutionExceptionHandler errorHandler = new ExecutionExceptionHandler(); - private Set> allowedMappers; - private final List unrecognizedArgs = new ArrayList<>(); private Optional parsedCommand = Optional.empty(); private boolean warnedTimestampChanged; @@ -114,17 +110,65 @@ public class Picocli { return colorMode; } + private boolean isHelpRequested(ParseResult result) { + if (result.isUsageHelpRequested()) { + return true; + } + + return result.subcommands().stream().anyMatch(this::isHelpRequested); + } + public void parseAndRun(List cliArgs) { - // perform two passes over the cli args. First without option validation to determine the current command, then with option validation enabled - CommandLine cmd = createCommandLine(spec -> {}).setUnmatchedArgumentsAllowed(true); + List unrecognizedArgs = new ArrayList<>(); + CommandLine cmd = createCommandLine(unrecognizedArgs); + String[] argArray = cliArgs.toArray(new String[0]); try { - ParseResult result = cmd.parseArgs(argArray); // process the cli args first to init the config file and perform validation + ParseResult result = cmd.parseArgs(argArray); + var commandLineList = result.asCommandLineList(); - // recreate the command specifically for the current - cmd = createCommandLineForCommand(cliArgs, commandLineList); + CommandLine cl = commandLineList.get(commandLineList.size() - 1); + + AbstractCommand currentCommand = null; + if (cl.getCommand() instanceof AbstractCommand ac) { + currentCommand = ac; + } + initConfig(cliArgs, currentCommand); + + if (!unrecognizedArgs.isEmpty()) { + IncludeOptions options = Optional.ofNullable(currentCommand).map(c -> getIncludeOptions(cliArgs, c, c.getName())).orElse(new IncludeOptions()); + Set allowedCategories = Set.copyOf(Optional.ofNullable(currentCommand).map(AbstractCommand::getOptionCategories).orElse(List.of())); + // TODO: further refactor this as these args should be the source for ConfigArgsConfigSource + unrecognizedArgs.removeIf(arg -> { + boolean hasArg = false; + if (arg.contains("=")) { + arg = arg.substring(0, arg.indexOf("=")); + hasArg = true; + } + PropertyMapper mapper = PropertyMappers.getMapperByCliKey(arg); + if (mapper != null) { + if (!allowedCategories.contains(mapper.getCategory()) || (mapper.isBuildTime() && !options.includeBuildTime) || (mapper.isRunTime() && !options.includeRuntime)) { + return false; + } + if (!hasArg) { + addCommandOptions(cliArgs, cl); + throw new MissingParameterException(cl, cl.getCommandSpec().optionsMap().get(arg), null); + } + return true; + } + return false; + }); + if (!unrecognizedArgs.isEmpty()) { + addCommandOptions(cliArgs, cl); + throw new KcUnmatchedArgumentException(cl, unrecognizedArgs); + } + } + + if (isHelpRequested(result)) { + addCommandOptions(cliArgs, cl); + } int exitCode = cmd.execute(argArray); @@ -136,53 +180,6 @@ public class Picocli { } } - private CommandLine createCommandLineForCommand(List cliArgs, List commandLineList) { - return createCommandLine(spec -> { - // use the incoming commandLineList from the initial parsing to determine the current command - CommandSpec currentSpec = spec; - - // add help to the root and all commands as it is not inherited - addHelp(currentSpec); - - for (CommandLine commandLine : commandLineList.subList(1, commandLineList.size())) { - CommandLine subCommand = currentSpec.subcommands().get(commandLine.getCommandName()); - if (subCommand == null) { - currentSpec = null; - break; - } - - currentSpec = subCommand.getCommandSpec(); - - currentSpec.addUnmatchedArgsBinding(CommandLine.Model.UnmatchedArgsBinding.forStringArrayConsumer(new ISetter() { - @Override - public T set(T value) { - if (value != null) { - unrecognizedArgs.addAll(Arrays.asList((String[]) value)); - } - return null; // doesn't matter - } - })); - - addHelp(currentSpec); - } - - AbstractCommand currentCommand = null; - CommandLine commandLine = null; - if (currentSpec != null) { - commandLine = currentSpec.commandLine(); - - if (commandLine != null && commandLine.getCommand() instanceof AbstractCommand ac) { - currentCommand = ac; - } - } - // init the config before adding options to properly sanitize mappers - initConfig(cliArgs, currentCommand); - if (commandLine != null) { - addCommandOptions(cliArgs, commandLine); - } - }); - } - public Optional getParsedCommand() { return parsedCommand; } @@ -218,17 +215,6 @@ public class Picocli { * @param abstractCommand */ public void validateConfig(List cliArgs, AbstractCommand abstractCommand) { - unrecognizedArgs.removeIf(arg -> { - if (arg.contains("=")) { - arg = arg.substring(0, arg.indexOf("=")); - } - PropertyMapper mapper = PropertyMappers.getMapperByCliKey(arg); - return mapper != null && mapper.hasWildcard() && allowedMappers.contains(mapper); - }); - if (!unrecognizedArgs.isEmpty()) { - throw new KcUnmatchedArgumentException(abstractCommand.getCommandLine().orElseThrow(), unrecognizedArgs); - } - if (cliArgs.contains(OPTIMIZED_BUILD_OPTION_LONG) && !wasBuildEverRun()) { throw new PropertyException(Messages.optimizedUsedForFirstStartup()); } @@ -601,7 +587,30 @@ public class Picocli { return properties; } - public CommandLine createCommandLine(Consumer consumer) { + private void updateSpecHelpAndUnmatched(CommandSpec spec, List unrecognizedArgs) { + try { + spec.addOption(OptionSpec.builder(Help.OPTION_NAMES) + .usageHelp(true) + .description("This help message.") + .build()); + } catch (DuplicateOptionAnnotationsException e) { + // Completion is inheriting mixinStandardHelpOptions = true + } + + spec.addUnmatchedArgsBinding(CommandLine.Model.UnmatchedArgsBinding.forStringArrayConsumer(new ISetter() { + @Override + public T set(T value) { + if (value != null) { + unrecognizedArgs.addAll(Arrays.asList((String[]) value)); + } + return null; // doesn't matter + } + })); + + spec.subcommands().values().forEach(c -> updateSpecHelpAndUnmatched(c.getCommandSpec(), unrecognizedArgs)); + } + + CommandLine createCommandLine(List unrecognizedArgs) { CommandSpec spec = CommandSpec.forAnnotatedObject(new Main(), new IFactory() { @Override public K create(Class cls) throws Exception { @@ -612,7 +621,7 @@ public class Picocli { return result; } }).name(Environment.getCommand()); - consumer.accept(spec); + updateSpecHelpAndUnmatched(spec, unrecognizedArgs); CommandLine cmd = new CommandLine(spec); cmd.setExpandAtFiles(false); @@ -634,17 +643,6 @@ public class Picocli { return new PrintWriter(System.out, true); } - private static void addHelp(CommandSpec currentSpec) { - try { - currentSpec.addOption(OptionSpec.builder(Help.OPTION_NAMES) - .usageHelp(true) - .description("This help message.") - .build()); - } catch (DuplicateOptionAnnotationsException e) { - // Completion is inheriting mixinStandardHelpOptions = true - } - } - private IncludeOptions getIncludeOptions(List cliArgs, AbstractCommand abstractCommand, String commandName) { IncludeOptions result = new IncludeOptions(); if (abstractCommand == null) { @@ -689,10 +687,6 @@ public class Picocli { } addMappedOptionsToArgGroups(commandLine, mappers); - - if (CollectionUtil.isEmpty(allowedMappers)) { - allowedMappers = mappers.values().stream().flatMap(List::stream).collect(Collectors.toUnmodifiableSet()); - } } private static >>> void combinePropertyMappers(T origMappers, T additionalMappers) { 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 50c5653b77e..032f7d1e1af 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 @@ -31,6 +31,8 @@ import java.util.function.BiFunction; import java.util.function.BooleanSupplier; import java.util.function.Consumer; import java.util.function.Function; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import java.util.stream.Stream; import org.keycloak.common.Profile; @@ -42,6 +44,7 @@ import org.keycloak.quarkus.runtime.cli.PropertyException; import org.keycloak.quarkus.runtime.cli.ShortErrorMessageHandler; import org.keycloak.quarkus.runtime.cli.command.AbstractCommand; import org.keycloak.quarkus.runtime.configuration.ConfigArgsConfigSource; +import org.keycloak.quarkus.runtime.configuration.Configuration; import org.keycloak.quarkus.runtime.configuration.KcEnvConfigSource; import org.keycloak.quarkus.runtime.configuration.KeycloakConfigSourceProvider; import org.keycloak.quarkus.runtime.configuration.NestedPropertyMappingInterceptor; @@ -595,6 +598,24 @@ public class PropertyMapper { continue; } try { + if (option.getComponentType() != String.class && option.getExpectedValues().isEmpty()) { + if (v.isEmpty()) { + throw new PropertyException("Invalid empty value for option %s".formatted(getOptionAndSourceMessage(configValue))); + } + try { + Configuration.getConfig().convert(v, option.getComponentType()); + } catch (Exception e) { + // strip the smallrye code if possible + String message = e.getMessage(); + Pattern p = Pattern.compile("SRCFG\\d+: (.*)$"); + Matcher m = p.matcher(message); + if (m.find()) { + message = m.group(1); + } + throw new PropertyException("Invalid value for option %s: %s".formatted(getOptionAndSourceMessage(configValue), message)); + } + } + singleValidator.accept(configValue, v); } catch (PropertyException e) { if (!result.isEmpty()) { diff --git a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java index 5a2e32f5eb8..fc78fcbce26 100644 --- a/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java +++ b/quarkus/runtime/src/test/java/org/keycloak/quarkus/runtime/cli/PicocliTest.java @@ -187,7 +187,24 @@ public class PicocliTest extends AbstractConfigurationTest { NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--http-port=a"); assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode); assertThat(nonRunningPicocli.getErrString(), - containsString("Invalid value for option '--http-port': 'a' is not an int")); + containsString("Invalid value for option '--http-port': Expected an integer value, got \"a\"")); + } + + @Test + public void testInvalidArgumentTypeEnv() { + putEnvVar("KC_HTTP_PORT", "a"); + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev"); + assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getErrString(), + containsString("Invalid value for option 'KC_HTTP_PORT': Expected an integer value, got \"a\"")); + } + + @Test + public void testEmptyValue() { + NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--http-port="); + assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode); + assertThat(nonRunningPicocli.getErrString(), + containsString("Invalid empty value for option '--http-port'")); } @Test @@ -804,7 +821,7 @@ public class PicocliTest extends AbstractConfigurationTest { NonRunningPicocli nonRunningPicocli = pseudoLaunch("start-dev", "--log=%s".formatted(logHandlerOptionsName), "--log-%s-async=true".formatted(logHandlerOptionsName), "--log-%s-async-queue-length=invalid".formatted(logHandlerOptionsName)); assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode); - assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': 'invalid' is not an int".formatted(logHandlerOptionsName))); + assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': Expected an integer value, got \"invalid\"".formatted(logHandlerOptionsName))); onAfter(); nonRunningPicocli = pseudoLaunch("start-dev", "--log=%s".formatted(logHandlerName), "--log-%s-async-queue-length=768".formatted(logHandlerOptionsName)); @@ -858,7 +875,7 @@ public class PicocliTest extends AbstractConfigurationTest { var nonRunningPicocli = pseudoLaunch("start-dev", "--log=%s".formatted(handlerName), "--log-%s-async=true".formatted(handlerName), "--log-%s-async-queue-length=invalid".formatted(handlerName)); assertEquals(CommandLine.ExitCode.USAGE, nonRunningPicocli.exitCode); - assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': 'invalid' is not an int".formatted(handlerName))); + assertThat(nonRunningPicocli.getErrString(), containsString("Invalid value for option '--log-%s-async-queue-length': Expected an integer value, got \"invalid\"".formatted(handlerName))); } @Test diff --git a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/CacheEmbeddedMtlsDistTest.java b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/CacheEmbeddedMtlsDistTest.java index 137d3cc23f9..9dc4a93e47e 100644 --- a/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/CacheEmbeddedMtlsDistTest.java +++ b/quarkus/tests/integration/src/test/java/org/keycloak/it/cli/dist/CacheEmbeddedMtlsDistTest.java @@ -99,7 +99,7 @@ public class CacheEmbeddedMtlsDistTest { // test blank result = dist.run("start-dev", "--cache=ispn", "--cache-embedded-mtls-enabled=true", "--%s=".formatted(key)); - result.assertError("Invalid value for option '--%s': '' is not an int".formatted(key)); + result.assertError("Invalid empty value for option '--%s'".formatted(key)); } @Test