8000 Identity provider should not be hidden from login page when the a user goes back from the reset credentials page by vilmosnagy · Pull Request #33254 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

Identity provider should not be hidden from login page when the a user goes back from the reset credentials page #33254

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
vilmosnagy wants to merge 3 commits into from
Closed

Identity provider should not be hidden from login page when the a user goes back from the reset credentials page #33254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a7bdcc1
#33204 add the failing test
vilmosnagy Sep 23, 2024
a49f2d8
#33204 possible fix (?)
vilmosnagy Sep 24, 2024
c8fd722
#33204 refactor the previous tests to work with the changed behaviour
vilmosnagy Oct 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ...rc/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
import jakarta.ws.rs.core.MultivaluedMap;
import jakarta.ws.rs.core.Response;

import static org.keycloak.authentication.authenticators.resetcred.ResetCredentialChooseUser.RESET_CREDENTIAL_USER_CHOSEN;

/**
* @author <a href="mailto:bill@burkecentral.com">Bill Burke</a>
* @version $Revision: 1 $
Expand Down Expand Up @@ -58,6 +60,7 @@ public void authenticate(AuthenticationFlowContext context) {
MultivaluedMap<String, String> formData = new MultivaluedHashMap<>();
String loginHint = context.getAuthenticationSession().getClientNote(OIDCLoginProtocol.LOGIN_HINT_PARAM);

clearUserIfComingFromResetPassword(context);
String rememberMeUsername = AuthenticationManager.getRememberMeUsername(context.getSession());

if (context.getUser() != null) {
Expand All @@ -80,6 +83,12 @@ public void authenticate(AuthenticationFlowContext context) {
context.challenge(challengeResponse);
}

private void clearUserIfComingFromResetPassword(AuthenticationFlowContext context) {
if ("true".equals(context.getAuthenticationSession().getAuthNote(RESET_CREDENTIAL_USER_CHOSEN))) {
context.clearUser();
@mposolda mposolda Oct 10, 2024
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it will be good to clear the RESET_CREDENTIAL_USER_CHOSEN note as well. Otherwise, during next invocation of this method, the user will be cleared again, which is not feasible.

An alternative and more generic approach might be to move this from UsernamePasswordForm somewhere to the upper layer (AuthenticationProcessor or DefaultAuthenticationFlow) and reset whole authenticationSession (something like AuthenticationPRocessor.resetFlow) when it is detected that flow was changed from reset-credentials back to the browser . However I am not sure if it is easily and reliably possible to detect this?

}
}

@Override
public boolean requiresUser() {
return false;
Expand Down
4 changes: 3 additions & 1 deletion .../java/org/keycloak/authentication/authenticators/resetcred/ResetCredentialChooseUser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public class ResetCredentialChooseUser implements Authenticator, AuthenticatorFa
private static final Logger logger = Logger.getLogger(ResetCredentialChooseUser.class);

public static final String PROVIDER_ID = "reset-credentials-choose-user";
public static final String RESET_CREDENTIAL_USER_CHOSEN = "RESET_CREDENTIAL_USER_CHOSEN";

@Override
public void authenticate(AuthenticationFlowContext context) {
Expand Down Expand Up @@ -104,7 +105,7 @@ public void action(AuthenticationFlowContext context) {
}

username = username.trim();

RealmModel realm = context.getRealm();
UserModel user = context.getSession().users().getUserByUsername(realm, username);
if (user == null && realm.isLoginWithEmailAllowed() && username.contains("@")) {
Expand All @@ -126,6 +127,7 @@ public void action(AuthenticationFlowContext context) {
.user(user).error(Errors.USER_DISABLED);
context.clearUser();
} else {
context.getAuthenticationSession().setAuthNote(RESET_CREDENTIAL_USER_CHOSEN, "true");
context.setUser(user);
}

Expand Down
4 changes: 2 additions & 2 deletions services/src/main/java/org/keycloak/forms/login/freemarker/model/IdentityProviderBean.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,10 +188,10 @@ protected Set<String> getLinkedBrokerAliases(KeycloakSession session, RealmModel
.map(FederatedIdentityModel::getIdentityProvider)
.collect(Collectors.toSet());

if (!federatedIdentities.isEmpty() || organizationsDisabled(realm))
if (!federatedIdentities.isEmpty() || organizationsDisabled(realm)) {
// if orgs are enabled, we don't want to return an empty set - we want the organization IDPs to be shown if those are available.
result = new HashSet<>(federatedIdentities);

}
}
}
return result;
Expand Down
39 changes: 38 additions & 1 deletion ...an/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcFirstBrokerLoginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package org.keycloak.testsuite.broker;

import org.apache.commons.lang3.StringUtils;
import org.hamcrest.CoreMatchers;
import org.jboss.arquillian.graphene.page.Page;
import org.junit.Test;
import org.keycloak.admin.client.resource.IdentityProviderResource;
Expand Down Expand Up @@ -443,6 +442,44 @@ public void shouldOfferOidcOptionOnLoginPageAfterUserTriedToLogInButDecidedNotTo
final var socialButton = this.loginPage.findSocialButton(bc.getIDPAlias());
}

@Test
public void shouldOfferAllOidcOptionOnLoginPageUserTriesToResetTheirPasswordAndGoesBack() {
final var realmRepresentation = adminClient.realm(bc.consumerRealmName()).toRepresentation();
realmRepresentation.setResetPasswordAllowed(true);
adminClient.realm(bc.consumerRealmName()).update(realmRepresentation);

createUser(bc.consumerRealmName(), "user-without-idp", "password", "Vilmos", "Szabó-Nagy", "spam@vnagy.eu");

oauth.clientId("broker-app");
loginPage.open(bc.consumerRealmName());
loginPage.assertCurrent(bc.consumerRealmName());

System.out.println(driver.getCurrentUrl());

// loginPage.clickResetLogin(); does not work somehow?
driver.findElement(By.linkText("Forgot Password?")).click();
loginPasswordResetPage.assertCurrent();

loginPasswordResetPage.backToLogin();

String urlWhenBackFromRegistrationPage = driver.getCurrentUrl();
log.debug("Social button should be visible");
final var socialButton = this.loginPage.findSocialButton(bc.getIDPAlias());

// loginPage.clickResetLogin(); does not work somehow?
driver.findElement(By.linkText("Forgot Password?")).click();
loginPasswordResetPage.assertCurrent();

loginPasswordResetPage.changePassword("user-without-idp");
driver.navigate().back();
driver.navigate().back();
String urlWhenWentBackFromResetPassword = driver.getCurrentUrl();
assertEquals(urlWhenBackFromRegistrationPage, urlWhenWentBackFromResetPassword);

log.debug("Should not fail here... We're still not logged in, so the IDP should be shown on the login page.");
assertTrue("We should be on the login page.", driver.getPageSource().contains("Sign in to your account"));
final var socialButtonAfterPasswordReset = this.loginPage.findSocialButton(bc.getIDPAlias());
}

@Test
public void testDisplayName() {
Expand Down
30 changes: 25 additions & 5 deletions ...ration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ public void registerExistingUsernameForbidden() {
.removeDetail(Details.EMAIL)
.user((String) null).error("username_in_use").assertEvent();
}

@Test
public void registerExistingEmailForbidden() {
loginPage.open();
Expand All @@ -154,7 +154,7 @@ public void registerExistingEmailForbidden() {
.removeDetail(Details.EMAIL)
.user((String) null).error("email_in_use").assertEvent();
}

@Test
public void registerExistingEmailAllowed() throws IOException {
try (RealmAttributeUpdater rau = setDuplicateEmailsAllowed(true).update()) {
Expand Down Expand Up @@ -849,17 +849,18 @@ public void registerUserSuccessTermsAcceptanceWithRequiredActionEnabled() {
public void testRegisterShouldFailBeforeUserCreationWhenUserIsInContext() {
loginPage.open();
loginPage.clickRegister();
String registrationUrl = driver.getCurrentUrl();

registerPage.clickBackToLogin();
loginPage.assertCurrent(testRealm().toRepresentation().getRealm());

loginPage.resetPassword();
resetPasswordPage.assertCurrent();
resetPasswordPage.changePassword("test-user@localhost");

driver.navigate().back();
driver.navigate().back();
events.clear();
driver.navigate().back();

driver.navigate().to(registrationUrl);

< 8 /td> 8973 errorPage.assertCurrent();
Assert.assertEquals("Action expired. Please continue with login now.", errorPage.getError());
Expand All @@ -873,6 +874,25 @@ public void testRegisterShouldFailBeforeUserCreationWhenUserIsInContext() {
.error(Errors.GENERIC_AUTHENTICATION_ERROR).assertEvent();
}

@Test
public void testLoginPageClearsUserFromContextIfUserNavigatesBackFromResetPassword() {
loginPage.open();
loginPage.clickRegister();
registerPage.clickBackToLogin();
loginPage.assertCurrent(testRealm().toRepresentation().getRealm());

loginPage.resetPassword();
resetPasswordPage.assertCurrent();
resetPasswordPage.changePassword("test-user@localhost");

driver.navigate().back();
driver.navigate().back();
events.clear();
driver.navigate().back();

registerPage.assertCurrent();
}

protected RealmAttributeUpdater configureRealmRegistrationEmailAsUsername(final boolean value) {
return getRealmAttributeUpdater().setRegistrationEmailAsUsername(value);
}
Expand Down
0