From e1b735fc41832df7962a7bb31488e6b517d09e55 Mon Sep 17 00:00:00 2001 From: Martin Kanis Date: Mon, 1 Jul 2024 16:30:58 +0200 Subject: [PATCH] Identity-first login flow should be followed by asking for the user credentials Closes #30339 Signed-off-by: Martin Kanis --- .../keycloak/forms/login/freemarker/LoginFormsUtil.java | 8 +++++--- .../authenticators/browser/OrganizationAuthenticator.java | 1 + .../organization/admin/AbstractOrganizationTest.java | 2 +- .../organization/admin/OrganizationThemeTest.java | 3 --- .../broker/AbstractBrokerSelfRegistrationTest.java | 2 -- .../member/OrganizationMemberAuthenticationTest.java | 3 +-- 6 files changed, 8 insertions(+), 11 deletions(-) diff --git a/services/src/main/java/org/keycloak/forms/login/freemarker/LoginFormsUtil.java b/services/src/main/java/org/keycloak/forms/login/freemarker/LoginFormsUtil.java index f083fc7ab9..26d59ed343 100755 --- a/services/src/main/java/org/keycloak/forms/login/freemarker/LoginFormsUtil.java +++ b/services/src/main/java/org/keycloak/forms/login/freemarker/LoginFormsUtil.java @@ -23,6 +23,7 @@ import org.keycloak.authentication.authenticators.broker.AbstractIdpAuthenticato import org.keycloak.authentication.authenticators.broker.util.SerializedBrokeredIdentityContext; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.OrganizationModel; import org.keycloak.models.UserModel; import org.keycloak.services.resources.LoginActionsService; import org.keycloak.sessions.AuthenticationSessionModel; @@ -45,23 +46,24 @@ public class LoginFormsUtil { AuthenticationSessionModel authSession = context.getAuthenticationSession(); String currentFlowPath = authSession.getAuthNote(AuthenticationProcessor.CURRENT_FLOW_PATH); UserModel currentUser = context.getUser(); + Object organization = session.getAttribute(OrganizationModel.class.getName()); // Fixing #14173 // If the current user is not null, then it's a re-auth, and we should filter the possible options with the pre-14173 logic // If the current user is null, then it's one of the following cases: - // - either connecting a new IdP to the user's account. + // - either connecting a new IdP to the user's account. // - in this case the currentUser is null AND the current flow is the FIRST_BROKER_LOGIN_PATH // - so we should filter out the one they just used for login, as they need to re-auth themself with an already linked IdP account // - or we're on the Login page // - in this case the current user is null AND the current flow is NOT the FIRST_BROKER_LOGIN_PATH // - so we should show all the possible IdPs to the user trying to log in (this is the bug in #14173) // - so we're skipping this branch, and retunring everything at the end of the method - if (currentUser != null || Objects.equals(LoginActionsService.FIRST_BROKER_LOGIN_PATH, currentFlowPath)) { + if (organization == null && (currentUser != null || Objects.equals(LoginActionsService.FIRST_BROKER_LOGIN_PATH, currentFlowPath))) { return filterIdentityProviders(providers, session, context); } } return providers.collect(Collectors.toList()); } - + public static List filterIdentityProviders(Stream providers, KeycloakSession session, AuthenticationFlowContext context) { if (context != null) { diff --git a/services/src/main/java/org/keycloak/organization/authentication/authenticators/browser/OrganizationAuthenticator.java b/services/src/main/java/org/keycloak/organization/authentication/authenticators/browser/OrganizationAuthenticator.java index e0d10c6ece..fb6e604380 100644 --- a/services/src/main/java/org/keycloak/organization/authentication/authenticators/browser/OrganizationAuthenticator.java +++ b/services/src/main/java/org/keycloak/organization/authentication/authenticators/browser/OrganizationAuthenticator.java @@ -107,6 +107,7 @@ public class OrganizationAuthenticator extends IdentityProviderAuthenticator { return attributes; }); } + context.setUser(user); context.attempted(); } else if (broker.size() == 1) { // user is a managed member and associated with a broker, redirect automatically diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java index 35bfb572f4..973015f28a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/AbstractOrganizationTest.java @@ -268,7 +268,7 @@ public abstract class AbstractOrganizationTest extends AbstractAdminTest { if (clickIdp) { assertTrue(loginPage.isPasswordInputPresent()); - assertTrue(loginPage.isUsernameInputPresent()); + assertTrue(loginPage.isSocialButtonPresent(idp.getAlias())); loginPage.clickSocial(idp.getAlias()); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationThemeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationThemeTest.java index a2962ac43e..44aace92f6 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationThemeTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationThemeTest.java @@ -87,7 +87,6 @@ public class OrganizationThemeTest extends AbstractOrganizationTest { loginPage.open(bc.consumerRealmName()); Assert.assertTrue(driver.getPageSource().contains("Sign-in to the realm")); loginPage.loginUsername("tom@myorg.com"); - Assert.assertTrue(driver.getPageSource().contains("Sign-in to myorg organization")); Assert.assertTrue(loginPage.isPasswordInputPresent()); } @@ -205,8 +204,6 @@ public class OrganizationThemeTest extends AbstractOrganizationTest { // organization available to identity-first login page loginPage.open(bc.consumerRealmName()); loginPage.loginUsername(user.getEmail()); - Assert.assertTrue(driver.getPageSource().contains("Sign-in to myorg organization")); - Assert.assertTrue(driver.getPageSource().contains("User is member of " + orgRep.getName())); Assert.assertTrue(loginPage.isPasswordInputPresent()); } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/broker/AbstractBrokerSelfRegistrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/broker/AbstractBrokerSelfRegistrationTest.java index 067285d492..6a3b8d816d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/broker/AbstractBrokerSelfRegistrationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/broker/AbstractBrokerSelfRegistrationTest.java @@ -184,9 +184,7 @@ public abstract class AbstractBrokerSelfRegistrationTest extends AbstractOrganiz openIdentityFirstLoginPage("user@neworg.org", false, null, false, false); - Assert.assertTrue(loginPage.isUsernameInputPresent()); Assert.assertTrue(loginPage.isPasswordInputPresent()); - Assert.assertTrue(loginPage.isRegisterLinkPresent()); } @Test diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/member/OrganizationMemberAuthenticationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/member/OrganizationMemberAuthenticationTest.java index cf33ed04a9..5608328513 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/member/OrganizationMemberAuthenticationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/member/OrganizationMemberAuthenticationTest.java @@ -43,12 +43,11 @@ public class OrganizationMemberAuthenticationTest extends AbstractOrganizationTe openIdentityFirstLoginPage(member.getEmail(), false, null, false, false); Assert.assertTrue(loginPage.isPasswordInputPresent()); - Assert.assertEquals(member.getEmail(), loginPage.getUsername()); // no idp should be shown because there is only a single idp that is bound to an organization Assert.assertFalse(loginPage.isSocialButtonPresent(bc.getIDPAlias())); // the member should be able to log in using the credentials - loginPage.login(member.getEmail(), memberPassword); + loginPage.login(memberPassword); appPage.assertCurrent(); }