libre.sh/keycloak-scim
3
0
Fork 0
Code Issues 15 Pull requests Releases 1 Packages Activity Actions

KEYCLOAK-12858 Authenticator is sometimes required even when configured as alternative

Browse source
This commit is contained in:
mposolda 2020-02-10 21:38:46 +01:00 committed by Marek Posolda
parent 67ddd3b0eb
commit eeeaafb5e7
2 changed files with 115 additions and 89 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

116
services/src/main/java/org/keycloak/authentication/DefaultAuthenticationFlow.java
View file

@ -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<AuthenticationExecutionModel> 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<AuthenticationExecutionModel> 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<AuthenticationExecutionModel> requiredExecutions = new LinkedList<>();
List<AuthenticationExecutionModel> 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

88
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserFlowTest.java
View file

@ -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);