From eeeaafb5e7c7ae253bd2481c4837d1eb59bdcfcf Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 10 Feb 2020 21:38:46 +0100 Subject: [PATCH] KEYCLOAK-12858 Authenticator is sometimes required even when configured as alternative --- .../DefaultAuthenticationFlow.java | 116 ++++++------------ .../testsuite/forms/BrowserFlowTest.java | 88 +++++++++++-- 2 files changed, 115 insertions(+), 89 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java b/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java index 3004e6ee19..06c14b7af7 100755 --- a/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java +++ b/services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java @@ -32,6 +32,7 @@ import javax.ws.rs.core.MultivaluedMap; import javax.ws.rs.core.Response; import java.util.ArrayList; import java.util.Iterator; +import java.util.LinkedList; import java.util.List; import java.util.stream.Collectors; @@ -187,76 +188,6 @@ public class DefaultAuthenticationFlow implements AuthenticationFlow { } - /** - * Clear execution status of targetExecution and also clear execution status of all the executions, which were triggered after this execution. - * This covers also "flow" executions and executions, which were set automatically - * - * @param targetExecution - */ - private void recursiveClearExecutionStatusOfAllExecutionsAfterOurExecutionInclusive(AuthenticationExecutionModel targetExecution) { - RealmModel realm = processor.getRealm(); - AuthenticationSessionModel authSession = processor.getAuthenticationSession(); - - // Clear execution status of our execution - authSession.getExecutionStatus().remove(targetExecution.getId()); - - // Find all the "sibling" executions after target execution including target execution. For those, we can recursively remove execution status - recursiveClearExecutionStatusOfAllSiblings(targetExecution); - - // Find the parent flow. If corresponding execution of this parent flow already has "executionStatus" set, we should clear it and also clear - // the status for all the siblings after that execution - while (true) { - AuthenticationFlowModel parentFlow = realm.getAuthenticationFlowById(targetExecution.getParentFlow()); - if (parentFlow.isTopLevel()) { - return; - } - - AuthenticationExecutionModel flowExecution = realm.getAuthenticationExecutionByFlowId(parentFlow.getId()); - if (authSession.getExecutionStatus().containsKey(flowExecution.getId())) { - authSession.getExecutionStatus().remove(flowExecution.getId()); - recursiveClearExecutionStatusOfAllSiblings(flowExecution); - targetExecution = flowExecution; - } else { - return; - } - - } - } - - - /** - * Recursively removes the execution status of all "sibling" executions after targetExecution. - * - * @param targetExecution - */ - private void recursiveClearExecutionStatusOfAllSiblings(AuthenticationExecutionModel targetExecution) { - RealmModel realm = processor.getRealm(); - AuthenticationFlowModel parentFlow = realm.getAuthenticationFlowById(targetExecution.getParentFlow()); - - logger.debugf("Recursively clearing executions in flow '%s', which are after execution '%s'", parentFlow.getAlias(), targetExecution.getId()); - - List siblingExecutions = realm.getAuthenticationExecutions(parentFlow.getId()); - int index = siblingExecutions.indexOf(targetExecution); - siblingExecutions = siblingExecutions.subList(index + 1, siblingExecutions.size()); - - for (AuthenticationExecutionModel authExec : siblingExecutions) { - recursiveClearExecutionStatus(authExec); - } - } - - - /** - * Removes the execution status for an execution. If it is a flow, do the same for all sub-executions. - * - * @param execution the execution for which the status must be cleared - */ - private void recursiveClearExecutionStatus(AuthenticationExecutionModel execution) { - processor.getAuthenticationSession().getExecutionStatus().remove(execution.getId()); - if (execution.isAuthenticatorFlow()) { - processor.getRealm().getAuthenticationExecutions(execution.getFlowId()).forEach(this::recursiveClearExecutionStatus); - } - } - /** * This method makes sure that the parent flow's corresponding execution is considered successful if its contained * executions are successful. @@ -270,13 +201,23 @@ public class DefaultAuthenticationFlow implements AuthenticationFlow { while (true) { List localExecutions = processor.getRealm().getAuthenticationExecutions(model.getParentFlow()); AuthenticationExecutionModel parentFlowExecutionModel = processor.getRealm().getAuthenticationExecutionByFlowId(model.getParentFlow()); - if (parentFlowExecutionModel != null && - ((model.isRequired() && localExecutions.stream().allMatch(processor::isSuccessful)) || - (model.isAlternative() && localExecutions.stream().anyMatch(processor::isSuccessful)))) { - processor.getAuthenticationSession().setExecutionStatus(parentFlowExecutionModel.getId(), AuthenticationSessionModel.ExecutionStatus.SUCCESS); - // Flow is successfully finished. Recursively check whether it's parent flow is now successful as well - model = parentFlowExecutionModel; + if (parentFlowExecutionModel != null) { + List requiredExecutions = new LinkedList<>(); + List alternativeExecutions = new LinkedList<>(); + fillListsOfExecutions(localExecutions, requiredExecutions, alternativeExecutions); + + // Note: If we evaluate alternative execution, we will also doublecheck that there are not required elements in same subflow + if ((model.isRequired() && requiredExecutions.stream().allMatch(processor::isSuccessful)) || + (model.isAlternative() && alternativeExecutions.stream().anyMatch(processor::isSuccessful) && requiredExecutions.isEmpty())) { + logger.debugf("Flow '%s' successfully finished after children executions success", logExecutionAlias(parentFlowExecutionModel)); + processor.getAuthenticationSession().setExecutionStatus(parentFlowExecutionModel.getId(), AuthenticationSessionModel.ExecutionStatus.SUCCESS); + + // Flow is successfully finished. Recursively check whether it's parent flow is now successful as well + model = parentFlowExecutionModel; + } else { + return model.getParentFlow(); + } } else { return model.getParentFlow(); } @@ -428,21 +369,22 @@ public class DefaultAuthenticationFlow implements AuthenticationFlow { private Response processSingleFlowExecutionModel(AuthenticationExecutionModel model, boolean calledFromFlow) { - logger.debugv("check execution: {0} requirement: {1}", model.getAuthenticator(), model.getRequirement()); + logger.debugf("check execution: '%s', requirement: '%s'", logExecutionAlias(model), model.getRequirement()); if (isProcessed(model)) { - logger.debug("execution is processed"); + logger.debugf("execution '%s' is processed", logExecutionAlias(model)); return null; } //handle case where execution is a flow if (model.isAuthenticatorFlow()) { - logger.debug("execution is flow"); AuthenticationFlow authenticationFlow = processor.createFlowExecution(model.getFlowId(), model); Response flowChallenge = authenticationFlow.processFlow(); if (flowChallenge == null) { if (authenticationFlow.isSuccessful()) { + logger.debugf("Flow '%s' successfully finished", logExecutionAlias(model)); processor.getAuthenticationSession().setExecutionStatus(model.getId(), AuthenticationSessionModel.ExecutionStatus.SUCCESS); } else { + logger.debugf("Flow '%s' failed", logExecutionAlias(model)); processor.getAuthenticationSession().setExecutionStatus(model.getId(), AuthenticationSessionModel.ExecutionStatus.FAILED); } return null; @@ -498,6 +440,22 @@ public class DefaultAuthenticationFlow implements AuthenticationFlow { return processResult(context, false); } + // Used for debugging purpose only. Log alias of authenticator (for non-flow executions) or alias of authenticationFlow (for flow executions) + private String logExecutionAlias(AuthenticationExecutionModel executionModel) { + if (executionModel.isAuthenticatorFlow()) { + // Resolve authenticationFlow model in case of debug logging. Otherwise don't lookup flowModel just because of logging and return only flowId + if (logger.isDebugEnabled()) { + AuthenticationFlowModel flowModel = processor.getRealm().getAuthenticationFlowById(executionModel.getFlowId()); + if (flowModel != null) { + return flowModel.getAlias() + " flow"; + } + } + return executionModel.getFlowId() + " flow"; + } else { + return executionModel.getAuthenticator(); + } + } + /** * This method creates the list of authenticators that is presented to the user. For a required execution, this is * only the credentials associated to the authenticator, and for an alternative execution, this is all other alternative diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserFlowTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserFlowTest.java index bc6fc98963..d588f5badc 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserFlowTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserFlowTest.java @@ -45,6 +45,7 @@ import org.keycloak.testsuite.util.OAuthClient; import org.keycloak.testsuite.authentication.ConditionalUserAttributeValueFactory; import org.keycloak.testsuite.authentication.SetUserAttributeAuthenticatorFactory; import org.keycloak.testsuite.util.URLUtils; +import org.keycloak.testsuite.util.WaitUtils; import org.openqa.selenium.By; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; @@ -63,6 +64,10 @@ import static org.keycloak.testsuite.broker.SocialLoginTest.Provider.GOOGLE; public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { private static final String INVALID_AUTH_CODE = "Invalid authenticator code."; + private static final String USER_WITH_ONE_OTP_OTP_SECRET = "DJmQfC73VGFhw7D4QJ8A"; + private static final String USER_WITH_TWO_OTPS_OTP1_SECRET = "DJmQfC73VGFhw7D4QJ8A"; + private static final String USER_WITH_TWO_OTPS_OTP2_SECRET = "ABCQfC73VGFhw7D4QJ8A"; + @ArquillianResource protected OAuthClient oauth; @@ -160,16 +165,13 @@ public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { loginTotpPage.assertCurrent(); loginTotpPage.assertOtpCredentialSelectorAvailability(false); - oneTimeCodePage.sendCode(getOtpCode("DJmQfC73VGFhw7D4QJ8A")); + oneTimeCodePage.sendCode(getOtpCode(USER_WITH_ONE_OTP_OTP_SECRET)); Assert.assertFalse(loginPage.isCurrent()); Assert.assertFalse(oneTimeCodePage.isOtpLabelPresent()); } @Test public void testUserWithTwoAdditionalFactors() { - final String firstKey = "DJmQfC73VGFhw7D4QJ8A"; - final String secondKey = "ABCQfC73VGFhw7D4QJ8A"; - // Provide username and password provideUsernamePassword("user-with-two-configured-otp"); Assert.assertTrue(oneTimeCodePage.isOtpLabelPresent()); @@ -181,17 +183,17 @@ public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { // Select "second" factor (which is unnamed as it doesn't have userLabel) but try to connect with the OTP code from the "first" one loginTotpPage.selectOtpCredential(OTPFormAuthenticator.UNNAMED); - loginTotpPage.login(getOtpCode(firstKey)); + loginTotpPage.login(getOtpCode(USER_WITH_TWO_OTPS_OTP1_SECRET)); Assert.assertEquals(INVALID_AUTH_CODE, oneTimeCodePage.getError()); // Select "first" factor but try to connect with the OTP code from the "second" one loginTotpPage.selectOtpCredential("first"); - loginTotpPage.login(getOtpCode(secondKey)); + loginTotpPage.login(getOtpCode(USER_WITH_TWO_OTPS_OTP2_SECRET)); Assert.assertEquals(INVALID_AUTH_CODE, oneTimeCodePage.getError()); // Select "second" factor and try to connect with its OTP code loginTotpPage.selectOtpCredential(OTPFormAuthenticator.UNNAMED); - loginTotpPage.login(getOtpCode(secondKey)); + loginTotpPage.login(getOtpCode(USER_WITH_TWO_OTPS_OTP2_SECRET)); Assert.assertFalse(oneTimeCodePage.isOtpLabelPresent()); } @@ -617,7 +619,7 @@ public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { loginTotpPage.assertCurrent(); loginTotpPage.assertOtpCredentialSelectorAvailability(false); - loginTotpPage.login(getOtpCode("DJmQfC73VGFhw7D4QJ8A")); + loginTotpPage.login(getOtpCode(USER_WITH_ONE_OTP_OTP_SECRET)); Assert.assertFalse(loginTotpPage.isCurrent()); events.expectLogin().user(testRealm().users().search("user-with-one-configured-otp").get(0).getId()) .detail(Details.USERNAME, "user-with-one-configured-otp").assertEvent(); @@ -683,7 +685,7 @@ public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { loginTotpPage.assertCurrent(); loginTotpPage.assertOtpCredentialSelectorAvailability(true); - loginTotpPage.login(getOtpCode("DJmQfC73VGFhw7D4QJ8A")); + loginTotpPage.login(getOtpCode(USER_WITH_TWO_OTPS_OTP1_SECRET)); Assert.assertFalse(loginTotpPage.isCurrent()); events.expectLogin().user(userId).detail(Details.USERNAME, "user-with-two-configured-otp").assertEvent(); } finally { @@ -1100,6 +1102,40 @@ public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { } } + /** + * Test for KEYCLOAK-12858 + * + * Flow is configured, so that once user provides username, there are 2 alternatives: + * - OTP + * - Subflow1, which contains another conditional subflow2, which requires user to authenticate with Password if he has password configured + * + * After login with password and fulfill the conditional subflow2, the subflow1 should be considered successful as well and the OTP authentication should not be needed + */ + @Test + @AuthServerContainerExclude(REMOTE) + public void testLoginWithAlternativeOTPAndConditionalPassword(){ + String newFlowAlias = "browser - copy 2"; + configureBrowserFlowWithAlternativeOTPAndConditionalPassword(newFlowAlias); + try { + + loginUsernameOnlyPage.open(); + loginUsernameOnlyPage.assertCurrent(); + loginUsernameOnlyPage.login("user-with-one-configured-otp"); + + // Assert that the login skipped the OTP authenticator and moved to the password + passwordPage.assertCurrent(); + passwordPage.assertTryAnotherWayLinkAvailability(true); + passwordPage.login("password"); + + Assert.assertFalse(loginPage.isCurrent()); + Assert.assertFalse(oneTimeCodePage.isOtpLabelPresent()); + events.expectLogin().user(testRealm().users().search("user-with-one-configured-otp").get(0).getId()) + .detail(Details.USERNAME, "user-with-one-configured-otp").assertEvent(); + } finally { + revertFlows(newFlowAlias); + } + } + /** * This flow contains: * UsernameForm REQUIRED @@ -1110,7 +1146,7 @@ public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { * * The password form is in a sub-subflow, because otherwise credential preference mechanisms would take over and any * way go into the password form. Note that this flow only works for the test because WebAuthn is a isUserSetupAllowed - * flow that is not a CredentialValidator. When this changes, this flow will have to be modified to use another appropriate + * flow . When this changes, this flow will have to be modified to use another appropriate * authenticator. * * @param newFlowAlias @@ -1131,6 +1167,38 @@ public class BrowserFlowTest extends AbstractTestRealmKeycloakTest { ); } + /** + * This flow contains: + * UsernameForm REQUIRED + * Subflow REQUIRED + * ** OTP ALTERNATIVE + * ** password-subflow ALTERNATIVE + * **** PasswordConditional subflow CONDITIONAL + * ****** ConditionalUserConfiguredOTP REQUIRED + * ****** Password REQUIRED + * + * @param newFlowAlias + */ + private void configureBrowserFlowWithAlternativeOTPAndConditionalPassword(String newFlowAlias) { + testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session).copyBrowserFlow(newFlowAlias)); + testingClient.server("test").run(session -> FlowUtil.inCurrentRealm(session) + .selectFlow(newFlowAlias) + .inForms(forms -> forms + .clear() + .addAuthenticatorExecution(Requirement.REQUIRED, UsernameFormFactory.PROVIDER_ID) + .addSubFlowExecution(Requirement.REQUIRED, subflow -> subflow + .addAuthenticatorExecution(Requirement.ALTERNATIVE, OTPFormAuthenticatorFactory.PROVIDER_ID) + .addSubFlowExecution(Requirement.ALTERNATIVE, sf -> sf + .addSubFlowExecution(Requirement.CONDITIONAL, sf2 -> sf2 + .addAuthenticatorExecution(Requirement.REQUIRED, ConditionalUserConfiguredAuthenticatorFactory.PROVIDER_ID) + .addAuthenticatorExecution(Requirement.REQUIRED, PasswordFormFactory.PROVIDER_ID) + ) + ) + ) + ).defineAsBrowserFlow() // Activate this new flow + ); + } + private void revertFlows(String flowToDeleteAlias) { revertFlows(testRealm(), flowToDeleteAlias);