From 78aa08941ac32be765a8348f6e3ca9cfb325c196 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Mon, 28 Oct 2024 18:26:17 +0100 Subject: [PATCH] Fix NPE in ConditionalOtpFormAuthenticator if no configuration Closes #34298 Signed-off-by: rmartinc --- .../ConditionalOtpFormAuthenticator.java | 41 ++++++++------- .../account/custom/CustomAuthFlowOTPTest.java | 51 +++++++++++++------ 2 files changed, 59 insertions(+), 33 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/browser/ConditionalOtpFormAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/ConditionalOtpFormAuthenticator.java index 2675968cd0..b34a04a0e8 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/ConditionalOtpFormAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/browser/ConditionalOtpFormAuthenticator.java @@ -18,16 +18,19 @@ package org.keycloak.authentication.authenticators.browser; import org.keycloak.authentication.AuthenticationFlowContext; +import org.keycloak.models.AuthenticatorConfigModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; import jakarta.ws.rs.core.MultivaluedMap; +import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.regex.Pattern; +import java.util.stream.Collectors; import static org.keycloak.authentication.authenticators.browser.ConditionalOtpFormAuthenticator.OtpDecision.ABSTAIN; import static org.keycloak.authentication.authenticators.browser.ConditionalOtpFormAuthenticator.OtpDecision.SHOW_OTP; @@ -105,8 +108,8 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator { @Override public void authenticate(AuthenticationFlowContext context) { - - Map config = context.getAuthenticatorConfig().getConfig(); + AuthenticatorConfigModel model = context.getAuthenticatorConfig(); + Map config = model != null? model.getConfig() : Collections.emptyMap(); if (tryConcludeBasedOn(voteForUserOtpControlAttribute(context.getUser(), config), context)) { return; @@ -284,30 +287,32 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator { private boolean isOTPRequired(KeycloakSession session, RealmModel realm, UserModel user) { MultivaluedMap requestHeaders = session.getContext().getRequestHeaders().getRequestHeaders(); - return realm.getAuthenticatorConfigsStream().anyMatch(configModel -> { - if (tryConcludeBasedOn(voteForUserOtpControlAttribute(user, configModel.getConfig()))) { + List> configs = realm.getAuthenticatorConfigsStream().map(AuthenticatorConfigModel::getConfig) + .filter(this::containsConditionalOtpConfig) + .collect(Collectors.toList()); + if (configs.isEmpty()) { + // no configuration at all means it is configured + return true; + } + return configs.stream().anyMatch(config -> { + if (tryConcludeBasedOn(voteForUserOtpControlAttribute(user, config))) { return true; } - if (tryConcludeBasedOn(voteForUserRole(realm, user, configModel.getConfig()))) { + if (tryConcludeBasedOn(voteForUserRole(realm, user, config))) { return true; } - if (tryConcludeBasedOn(voteForHttpHeaderMatchesPattern(requestHeaders, configModel.getConfig()))) { + if (tryConcludeBasedOn(voteForHttpHeaderMatchesPattern(requestHeaders, config))) { return true; } - if (configModel.getConfig().get(DEFAULT_OTP_OUTCOME) != null - && configModel.getConfig().get(DEFAULT_OTP_OUTCOME).equals(FORCE) - && configModel.getConfig().size() <= 1) { + if (config.get(DEFAULT_OTP_OUTCOME) != null + && config.get(DEFAULT_OTP_OUTCOME).equals(FORCE) + && config.size() <= 1) { return true; } - if (containsConditionalOtpConfig(configModel.getConfig()) - && voteForUserOtpControlAttribute(user, configModel.getConfig()) == ABSTAIN - && voteForUserRole(realm, user, configModel.getConfig()) == ABSTAIN - && voteForHttpHeaderMatchesPattern(requestHeaders, configModel.getConfig()) == ABSTAIN - && (voteForDefaultFallback(configModel.getConfig()) == SHOW_OTP - || voteForDefaultFallback(configModel.getConfig()) == ABSTAIN)) { - return true; - } - return false; + return voteForUserOtpControlAttribute(user, config) == ABSTAIN + && voteForUserRole(realm, user, config) == ABSTAIN + && voteForHttpHeaderMatchesPattern(requestHeaders, config) == ABSTAIN + && (voteForDefaultFallback(config) == SHOW_OTP || voteForDefaultFallback(config) == ABSTAIN); }); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/custom/CustomAuthFlowOTPTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/custom/CustomAuthFlowOTPTest.java index 106c7505eb..c8f02222fb 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/custom/CustomAuthFlowOTPTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/custom/CustomAuthFlowOTPTest.java @@ -439,6 +439,25 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { assertCurrentUrlStartsWith(testLoginOneTimeCodePage); } + @Test + public void conditionalOTPEmptyConfiguration() { + // prepare config empty + setConditionalOTPForm(null); + + // test OTP is required + driver.navigate().to(oauth.getLoginFormUrl()); + testRealmLoginPage.form().login(testUser); + + assertTrue(loginConfigTotpPage.isCurrent()); + + configureOTP(); + driver.navigate().to(oauth.getLoginFormUrl()); + testRealmLoginPage.form().login(testUser); + + // verify that the page is login page, not totp setup + assertCurrentUrlStartsWith(testLoginOneTimeCodePage); + } + private RoleRepresentation getOrCreateOTPRole() { try { return testRealmResource().roles().get("otp_role").toRepresentation(); @@ -526,9 +545,9 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { flow.setTopLevel(true); flow.setBuiltIn(false); - Response response = getAuthMgmtResource().createFlow(flow); - assertEquals(flowAlias + " create success", 201, response.getStatus()); - response.close(); + try (Response response = getAuthMgmtResource().createFlow(flow)) { + assertEquals(flowAlias + " create success", 201, response.getStatus()); + } //add execution - username-password form Map data = new HashMap<>(); @@ -550,20 +569,22 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { RealmRepresentation realm = testRealmResource().toRepresentation(); realm.setBrowserFlow(flowAlias); testRealmResource().update(realm); - - //get executionId - String executionId = getExecution(flowAlias, provider).getId(); - //prepare auth config - AuthenticatorConfigRepresentation authConfig = new AuthenticatorConfigRepresentation(); - authConfig.setAlias("Config alias"); - authConfig.setConfig(config); + if (config != null) { + //get executionId + String executionId = getExecution(flowAlias, provider).getId(); - //add auth config to the execution - response = getAuthMgmtResource().newExecutionConfig(executionId, authConfig); - assertEquals("new execution success", 201, response.getStatus()); - getCleanup().addAuthenticationConfigId(ApiUtil.getCreatedId(response)); - response.close(); + //prepare auth config + AuthenticatorConfigRepresentation authConfig = new AuthenticatorConfigRepresentation(); + authConfig.setAlias("Config alias"); + authConfig.setConfig(config); + + //add auth config to the execution + try (Response response = getAuthMgmtResource().newExecutionConfig(executionId, authConfig)) { + assertEquals("new execution success", 201, response.getStatus()); + getCleanup().addAuthenticationConfigId(ApiUtil.getCreatedId(response)); + } + } } }