Merge pull request #3476 from abstractj/KEYCLOAK-3875

[KEYCLOAK-3875] - Conditional OTP Forms not working as expected

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<String, String> 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<String, String> config) {
+    private OtpDecision voteForUserOtpControlAttribute(UserModel user, Map<String, String> config) {
 
         if (!config.containsKey(OTP_CONTROL_USER_ATTRIBUTE)) {
             return ABSTAIN;
@@ -173,7 +191,7 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator {
             return ABSTAIN;
         }
 
-        List<String> values = context.getUser().getAttribute(attributeName);
+        List<String> values = user.getAttribute(attributeName);
 
         if (values.isEmpty()) {
             return ABSTAIN;
@@ -191,14 +209,12 @@ public class ConditionalOtpFormAuthenticator extends OTPFormAuthenticator {
         }
     }
 
-    private OtpDecision voteForHttpHeaderMatchesPattern(AuthenticationFlowContext context, Map<String, String> config) {
+    private OtpDecision voteForHttpHeaderMatchesPattern(MultivaluedMap<String, String> requestHeaders, Map<String, String> config) {
 
         if (!config.containsKey(FORCE_OTP_FOR_HTTP_HEADER) && !config.containsKey(SKIP_OTP_FOR_HTTP_HEADER)) {
             return ABSTAIN;
         }
 
-        MultivaluedMap<String, String> 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<String, String> config) {
+    private OtpDecision voteForUserRole(RealmModel realm, UserModel user, Map<String, String> 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);
+        RoleModel role = getRoleFromString(realm, roleName);
-        UserModel user = context.getUser();
 
         return RoleUtils.hasRole(user.getRoleMappings(), role);
     }
+
+    private boolean isOTPRequired(KeycloakSession session, RealmModel realm, UserModel user) {
+        MultivaluedMap<String, String> 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());
+        }
+    }
 }

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() {
 

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;
@@ -59,6 +60,9 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
     @Page
     private OneTimeCode testLoginOneTimeCodePage;
 
+    @Page
+    private LoginConfigTotpPage loginConfigTotpPage;
+    
     @Override
     public void setDefaultPageUriParameters() {
         super.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<String> 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);
@@ -94,9 +103,12 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
     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
@@ -105,6 +117,8 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
 
     @Test
     public void conditionalOTPNoDefault() {
+        configureRequiredActions();
+        configureOTP();
         //prepare config - no configuration specified
         Map<String, String> config = new HashMap<>();
         setConditionalOTPForm(config);
@@ -134,6 +148,7 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
     
     @Test
     public void conditionalOTPDefaultForce() {
+
         //prepare config - default force
         Map<String, String> config = new HashMap<>();
         config.put(DEFAULT_OTP_OUTCOME, FORCE);
@@ -143,6 +158,10 @@ 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
@@ -165,11 +184,13 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
         //test OTP is skipped
         testRealmAccountManagementPage.navigateTo();
         testRealmLoginPage.form().login(testUser);
+
         assertCurrentUrlStartsWith(testRealmAccountManagementPage);
     }
 
     @Test
     public void conditionalOTPUserAttributeForce() {
+
         //prepare config - user attribute, default to skip
         Map<String, String> config = new HashMap<>();
         config.put(OTP_CONTROL_USER_ATTRIBUTE, "userSkipAttribute");
@@ -184,6 +205,10 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
         //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
@@ -237,6 +262,11 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
         //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
@@ -272,6 +302,10 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
         //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
@@ -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
@@ -325,7 +359,7 @@ public class CustomAuthFlowOTPTest extends AbstractCustomAccountManagementTest {
 
         //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();
     }
 }