From 39f40bc0054bd5a63d23b80c879a2bdc804faeb0 Mon Sep 17 00:00:00 2001 From: Bruno Oliveira Date: Mon, 7 Nov 2016 14:37:11 -0200 Subject: [PATCH] [KEYCLOAK-3875] - Conditional OTP Forms not working as expected --- .../ConditionalOtpFormAuthenticator.java | 74 ++++++++++--- .../browser/OTPFormAuthenticator.java | 4 - .../account/custom/CustomAuthFlowOTPTest.java | 104 ++++++++++++------ 3 files changed, 129 insertions(+), 53 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 ef17476e33..1696a1d920 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,6 +18,9 @@ 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 org.keycloak.models.utils.RoleUtils; @@ -106,15 +109,15 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator { Map config = context.getAuthenticatorConfig().getConfig(); - if (tryConcludeBasedOn(voteForUserOtpControlAttribute(context, config), context)) { + if (tryConcludeBasedOn(voteForUserOtpControlAttribute(context.getUser(), config), context)) { return; } - if (tryConcludeBasedOn(voteForUserRole(context, config), context)) { + if (tryConcludeBasedOn(voteForUserRole(context.getRealm(), context.getUser(), config), context)) { return; } - if (tryConcludeBasedOn(voteForHttpHeaderMatchesPattern(context, config), context)) { + if (tryConcludeBasedOn(voteForHttpHeaderMatchesPattern(context.getHttpRequest().getHttpHeaders().getRequestHeaders(), config), context)) { return; } @@ -158,11 +161,26 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator { } } + private boolean tryConcludeBasedOn(OtpDecision state) { + + switch (state) { + + case SHOW_OTP: + return true; + + case SKIP_OTP: + return false; + + default: + return false; + } + } + private void showOtpForm(AuthenticationFlowContext context) { super.authenticate(context); } - private OtpDecision voteForUserOtpControlAttribute(AuthenticationFlowContext context, Map config) { + private OtpDecision voteForUserOtpControlAttribute(UserModel user, Map config) { if (!config.containsKey(OTP_CONTROL_USER_ATTRIBUTE)) { return ABSTAIN; @@ -173,7 +191,7 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator { return ABSTAIN; } - List values = context.getUser().getAttribute(attributeName); + List values = user.getAttribute(attributeName); if (values.isEmpty()) { return ABSTAIN; @@ -191,14 +209,12 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator { } } - private OtpDecision voteForHttpHeaderMatchesPattern(AuthenticationFlowContext context, Map config) { + private OtpDecision voteForHttpHeaderMatchesPattern(MultivaluedMap requestHeaders, Map config) { if (!config.containsKey(FORCE_OTP_FOR_HTTP_HEADER) && !config.containsKey(SKIP_OTP_FOR_HTTP_HEADER)) { return ABSTAIN; } - MultivaluedMap requestHeaders = context.getHttpRequest().getHttpHeaders().getRequestHeaders(); - //Inverted to allow white-lists, e.g. for specifying trusted remote hosts: X-Forwarded-Host: (1.2.3.4|1.2.3.5) if (containsMatchingRequestHeader(requestHeaders, config.get(SKIP_OTP_FOR_HTTP_HEADER))) { return SKIP_OTP; @@ -238,32 +254,62 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator { return false; } - private OtpDecision voteForUserRole(AuthenticationFlowContext context, Map config) { + private OtpDecision voteForUserRole(RealmModel realm, UserModel user, Map config) { if (!config.containsKey(SKIP_OTP_ROLE) && !config.containsKey(FORCE_OTP_ROLE)) { return ABSTAIN; } - if (userHasRole(context, config.get(SKIP_OTP_ROLE))) { + if (userHasRole(realm, user, config.get(SKIP_OTP_ROLE))) { return SKIP_OTP; } - if (userHasRole(context, config.get(FORCE_OTP_ROLE))) { + if (userHasRole(realm, user, config.get(FORCE_OTP_ROLE))) { return SHOW_OTP; } return ABSTAIN; } - private boolean userHasRole(AuthenticationFlowContext context, String roleName) { + private boolean userHasRole(RealmModel realm, UserModel user, String roleName) { if (roleName == null) { return false; } - RoleModel role = getRoleFromString(context.getRealm(), roleName); - UserModel user = context.getUser(); + RoleModel role = getRoleFromString(realm, roleName); return RoleUtils.hasRole(user.getRoleMappings(), role); } + + private boolean isOTPRequired(KeycloakSession session, RealmModel realm, UserModel user) { + MultivaluedMap requestHeaders = session.getContext().getRequestHeaders().getRequestHeaders(); + for (AuthenticatorConfigModel configModel : realm.getAuthenticatorConfigs()) { + + if (tryConcludeBasedOn(voteForUserOtpControlAttribute(user, configModel.getConfig()))) { + return true; + } + if (tryConcludeBasedOn(voteForUserRole(realm, user, configModel.getConfig()))) { + return true; + } + if (tryConcludeBasedOn(voteForHttpHeaderMatchesPattern(requestHeaders, configModel.getConfig()))) { + return true; + } + if (configModel.getConfig().get(DEFAULT_OTP_OUTCOME) != null + && configModel.getConfig().get(DEFAULT_OTP_OUTCOME).equals(FORCE) + && configModel.getConfig().size() <= 1) { + return true; + } + } + return false; + } + + @Override + public void setRequiredActions(KeycloakSession session, RealmModel realm, UserModel user) { + if (!isOTPRequired(session, realm, user)) { + user.removeRequiredAction(UserModel.RequiredAction.CONFIGURE_TOTP); + } else if (!user.getRequiredActions().contains(UserModel.RequiredAction.CONFIGURE_TOTP.name())) { + user.addRequiredAction(UserModel.RequiredAction.CONFIGURE_TOTP.name()); + } + } } diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java index 9df33fca68..91266895db 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/browser/OTPFormAuthenticator.java @@ -37,8 +37,6 @@ import javax.ws.rs.core.Response; * @version $Revision: 1 $ */ public class OTPFormAuthenticator extends AbstractUsernameFormAuthenticator implements Authenticator { - public static final String TOTP_FORM_ACTION = "totp"; - @Override public void action(AuthenticationFlowContext context) { validateOTP(context); @@ -99,8 +97,6 @@ public class OTPFormAuthenticator extends AbstractUsernameFormAuthenticator impl } - - @Override public void close() { 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 02308420ff..e492672b18 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 @@ -17,7 +17,6 @@ package org.keycloak.testsuite.account.custom; import org.jboss.arquillian.graphene.page.Page; -import org.junit.Assert; import org.junit.Before; import org.junit.Test; import org.keycloak.models.AuthenticationExecutionModel.Requirement; @@ -28,6 +27,7 @@ import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.RoleRepresentation; import org.keycloak.testsuite.admin.Users; import org.keycloak.testsuite.auth.page.login.OneTimeCode; +import org.keycloak.testsuite.pages.LoginConfigTotpPage; import javax.ws.rs.core.Response; import java.util.ArrayList; @@ -35,6 +35,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import static org.keycloak.authentication.authenticators.browser.ConditionalOtpFormAuthenticator.DEFAULT_OTP_OUTCOME; import static org.keycloak.authentication.authenticators.browser.ConditionalOtpFormAuthenticator.FORCE; @@ -58,6 +59,9 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { @Page private OneTimeCode testLoginOneTimeCodePage; + + @Page + private LoginConfigTotpPage loginConfigTotpPage; @Override public void setDefaultPageUriParameters() { @@ -69,12 +73,17 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { @Override public void beforeTest() { super.beforeTest(); + } + + private void configureRequiredActions() { //set configure TOTP as required action to test user List requiredActions = new ArrayList<>(); requiredActions.add(CONFIGURE_TOTP.name()); testUser.setRequiredActions(requiredActions); testRealmResource().users().get(testUser.getId()).update(testUser); - + } + + private void configureOTP() { //configure OTP for test user testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); @@ -83,7 +92,7 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { testRealmLoginPage.form().totpForm().setTotp(totp.generateTOTP(totpSecret)); testRealmLoginPage.form().totpForm().submit(); testRealmAccountManagementPage.signOut(); - + //verify that user has OTP configured testUser = testRealmResource().users().get(testUser.getId()).toRepresentation(); Users.setPasswordFor(testUser, PASSWORD); @@ -92,40 +101,45 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { @Test public void requireOTPTest() { - + updateRequirement("browser", "auth-otp-form", Requirement.REQUIRED); - testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); + assertTrue(loginConfigTotpPage.isCurrent()); + + configureOTP(); + testRealmLoginPage.form().login(testUser); testRealmLoginPage.form().totpForm().waitForTotpInputFieldPresent(); - + //verify that the page is login page, not totp setup assertCurrentUrlStartsWith(testLoginOneTimeCodePage); } - + @Test public void conditionalOTPNoDefault() { + configureRequiredActions(); + configureOTP(); //prepare config - no configuration specified Map config = new HashMap<>(); setConditionalOTPForm(config); - + //test OTP is required testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); testRealmLoginPage.form().totpForm().waitForTotpInputFieldPresent(); - + //verify that the page is login page, not totp setup assertCurrentUrlStartsWith(testLoginOneTimeCodePage); } - + @Test public void conditionalOTPDefaultSkip() { //prepare config - default skip Map config = new HashMap<>(); config.put(DEFAULT_OTP_OUTCOME, SKIP); - + setConditionalOTPForm(config); - + //test OTP is skipped testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); @@ -134,6 +148,7 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { @Test public void conditionalOTPDefaultForce() { + //prepare config - default force Map config = new HashMap<>(); config.put(DEFAULT_OTP_OUTCOME, FORCE); @@ -143,8 +158,12 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { //test OTP is forced testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); + assertTrue(loginConfigTotpPage.isCurrent()); + + configureOTP(); + testRealmLoginPage.form().login(testUser); testRealmLoginPage.form().totpForm().waitForTotpInputFieldPresent(); - + //verify that the page is login page, not totp setup assertCurrentUrlStartsWith(testLoginOneTimeCodePage); } @@ -155,48 +174,54 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { Map config = new HashMap<>(); config.put(OTP_CONTROL_USER_ATTRIBUTE, "userSkipAttribute"); config.put(DEFAULT_OTP_OUTCOME, FORCE); - + setConditionalOTPForm(config); //add skip user attribute to user testUser.singleAttribute("userSkipAttribute", "skip"); testRealmResource().users().get(testUser.getId()).update(testUser); - + //test OTP is skipped testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); + assertCurrentUrlStartsWith(testRealmAccountManagementPage); } - + @Test public void conditionalOTPUserAttributeForce() { + //prepare config - user attribute, default to skip Map config = new HashMap<>(); config.put(OTP_CONTROL_USER_ATTRIBUTE, "userSkipAttribute"); config.put(DEFAULT_OTP_OUTCOME, SKIP); - + setConditionalOTPForm(config); //add force user attribute to user testUser.singleAttribute("userSkipAttribute", "force"); testRealmResource().users().get(testUser.getId()).update(testUser); - + //test OTP is required testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); + assertTrue(loginConfigTotpPage.isCurrent()); + + configureOTP(); + testRealmLoginPage.form().login(testUser); testRealmLoginPage.form().totpForm().waitForTotpInputFieldPresent(); - + //verify that the page is login page, not totp setup assertCurrentUrlStartsWith(testLoginOneTimeCodePage); } - + @Test public void conditionalOTPRoleSkip() { //prepare config - role, default to force Map config = new HashMap<>(); config.put(SKIP_OTP_ROLE, "otp_role"); config.put(DEFAULT_OTP_OUTCOME, FORCE); - + setConditionalOTPForm(config); //create role @@ -208,20 +233,20 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { List realmRoles = new ArrayList<>(); realmRoles.add(role); testRealmResource().users().get(testUser.getId()).roles().realmLevel().add(realmRoles); - + //test OTP is skipped testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); assertCurrentUrlStartsWith(testRealmAccountManagementPage); } - + @Test public void conditionalOTPRoleForce() { //prepare config - role, default to skip Map config = new HashMap<>(); config.put(FORCE_OTP_ROLE, "otp_role"); config.put(DEFAULT_OTP_OUTCOME, SKIP); - + setConditionalOTPForm(config); //create role @@ -233,16 +258,21 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { List realmRoles = new ArrayList<>(); realmRoles.add(role); testRealmResource().users().get(testUser.getId()).roles().realmLevel().add(realmRoles); - + //test OTP is required testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); + + assertTrue(loginConfigTotpPage.isCurrent()); + + configureOTP(); + testRealmLoginPage.form().login(testUser); testRealmLoginPage.form().totpForm().waitForTotpInputFieldPresent(); - + //verify that the page is login page, not totp setup assertCurrentUrlStartsWith(testLoginOneTimeCodePage); } - + @Test public void conditionalOTPRequestHeaderSkip() { //prepare config - request header skip, default to force @@ -250,7 +280,7 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { String port = System.getProperty("auth.server.http.port", "8180"); config.put(SKIP_OTP_FOR_HTTP_HEADER, "Host: localhost:" + port); config.put(DEFAULT_OTP_OUTCOME, FORCE); - + setConditionalOTPForm(config); //test OTP is skipped @@ -258,7 +288,7 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { testRealmLoginPage.form().login(testUser); assertCurrentUrlStartsWith(testRealmAccountManagementPage); } - + @Test public void conditionalOTPRequestHeaderForce() { //prepare config - equest header force, default to skip @@ -266,18 +296,22 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { String port = System.getProperty("auth.server.http.port", "8180"); config.put(FORCE_OTP_FOR_HTTP_HEADER, "Host: localhost:" + port); config.put(DEFAULT_OTP_OUTCOME, SKIP); - + setConditionalOTPForm(config); //test OTP is required testRealmAccountManagementPage.navigateTo(); testRealmLoginPage.form().login(testUser); + assertEquals(driver.getTitle(), "Mobile Authenticator Setup"); + + configureOTP(); + testRealmLoginPage.form().login(testUser); testRealmLoginPage.form().totpForm().waitForTotpInputFieldPresent(); - + //verify that the page is login page, not totp setup assertCurrentUrlStartsWith(testLoginOneTimeCodePage); } - + private void setConditionalOTPForm(Map config) { String flowAlias = "ConditionalOTPFlow"; String provider = "auth-conditional-otp-form"; @@ -291,7 +325,7 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { flow.setBuiltIn(false); Response response = getAuthMgmtResource().createFlow(flow); - Assert.assertEquals(flowAlias + " create success", 201, response.getStatus()); + assertEquals(flowAlias + " create success", 201, response.getStatus()); response.close(); //add execution - username-password form @@ -322,10 +356,10 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest { AuthenticatorConfigRepresentation authConfig = new AuthenticatorConfigRepresentation(); authConfig.setAlias("Config alias"); authConfig.setConfig(config); - + //add auth config to the execution response = getAuthMgmtResource().newExecutionConfig(executionId, authConfig); - Assert.assertEquals("new execution success", 201, response.getStatus()); + assertEquals("new execution success", 201, response.getStatus()); response.close(); } }