From 87e8fbc7dc6b4c7652d94dd7796af9da0dfc1822 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Wed, 22 Jan 2025 13:16:23 -0300 Subject: [PATCH] Remember the organization once selected when reloading pages Closes #36629 Signed-off-by: Pedro Igor --- .../browser/OrganizationAuthenticator.java | 75 +++++++++++++++---- .../mappers/oidc/OrganizationScope.java | 38 ++++++++-- .../organization/utils/Organizations.java | 27 ++++++- .../OrganizationAuthenticationTest.java | 25 ++++++- .../AbstractBrokerSelfRegistrationTest.java | 61 ++++++++++++++- .../login/messages/messages_en.properties | 3 +- 6 files changed, 205 insertions(+), 24 deletions(-) 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 e3315b309585..2fa4ab03d92d 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 @@ -103,25 +103,27 @@ public void action(AuthenticationFlowContext context) { if (shouldUserSelectOrganization(context, user)) { return; } + + if (isMembershipRequired(context, null, user)) { + return; + } + + clearAuthenticationSession(context); // request does not map to any organization, go to the next step/sub-flow context.attempted(); return; } - if (user != null && isRequiresMembership(context) && !organization.isMember(user)) { - String errorMessage = "notMemberOfOrganization"; - // do not show try another way - context.setAuthenticationSelections(List.of()); - Response challenge = context.form() - .setError(errorMessage, organization.getName()) - .createErrorPage(Response.Status.FORBIDDEN); - context.failure(AuthenticationFlowError.GENERIC_AUTHENTICATION_ERROR, challenge, "User " + user.getUsername() + " not a member of organization " + organization.getAlias(), errorMessage); - return; - } - + // remember the organization during the lifetime of the authentication session + AuthenticationSessionModel authenticationSession = context.getAuthenticationSession(); + authenticationSession.setAuthNote(OrganizationModel.ORGANIZATION_ATTRIBUTE, organization.getId()); // make sure the organization is set to the session to make it available to templates session.getContext().setOrganization(organization); + if (isMembershipRequired(context, organization, user)) { + return; + } + if (tryRedirectBroker(context, organization, user, username, domain)) { return; } @@ -137,7 +139,7 @@ public void action(AuthenticationFlowContext context) { return; } - if (isSSOAuthentication(context.getAuthenticationSession())) { + if (isSSOAuthentication(authenticationSession)) { // if re-authenticating in the scope of an organization context.success(); } else { @@ -153,9 +155,10 @@ public boolean configuredFor(KeycloakSession session, RealmModel realm, UserMode private OrganizationModel resolveOrganization(UserModel user, String domain) { KeycloakContext context = session.getContext(); HttpRequest request = context.getHttpRequest(); + AuthenticationSessionModel authSession = context.getAuthenticationSession(); MultivaluedMap parameters = request.getDecodedFormParameters(); + // parameter from the organization selection page List alias = parameters.getOrDefault(OrganizationModel.ORGANIZATION_ATTRIBUTE, List.of()); - AuthenticationSessionModel authSession = context.getAuthenticationSession(); if (alias.isEmpty()) { OrganizationModel organization = Organizations.resolveOrganization(session, user, domain); @@ -210,6 +213,7 @@ public Map apply(Map attributes) { return attributes; } }); + clearAuthenticationSession(context); context.challenge(form.createForm("select-organization.ftl")); return true; } @@ -269,6 +273,8 @@ private UserModel resolveUser(AuthenticationFlowContext context, String username RealmModel realm = session.getContext().getRealm(); UserModel user = Optional.ofNullable(users.getUserByEmail(realm, username)).orElseGet(() -> users.getUserByUsername(realm, username)); + // make sure the organization will be resolved based on the username provided + clearAuthenticationSession(context); context.setUser(user); return user; @@ -359,4 +365,47 @@ private boolean isRequiresMembership(AuthenticationFlowContext context) { private Map getConfig(AuthenticationFlowContext context) { return Optional.ofNullable(context.getAuthenticatorConfig()).map(AuthenticatorConfigModel::getConfig).orElse(Map.of()); } + + private void clearAuthenticationSession(AuthenticationFlowContext context) { + AuthenticationSessionModel authenticationSession = context.getAuthenticationSession(); + authenticationSession.removeAuthNote(OrganizationModel.ORGANIZATION_ATTRIBUTE); + } + + private boolean isMembershipRequired(AuthenticationFlowContext context, OrganizationModel organization, UserModel user) { + if (user == null || !isRequiresMembership(context)) { + return false; + } + + if (organization == null) { + String rawScopes = context.getAuthenticationSession().getClientNote(OAuth2Constants.SCOPE); + OrganizationScope scope = OrganizationScope.valueOfScope(session, rawScopes); + + if (scope != null) { + organization = scope.resolveOrganizations(session, rawScopes).findAny().orElse(null); + } + } + + if (organization != null && organization.isMember(user)) { + return false; + } + + // do not show try another way + context.setAuthenticationSelections(List.of()); + + if (organization == null) { + String errorMessage = "notMemberOfAnyOrganization"; + Response challenge = context.form() + .setError(errorMessage) + .createErrorPage(Response.Status.FORBIDDEN); + context.failure(AuthenticationFlowError.GENERIC_AUTHENTICATION_ERROR, challenge, "User " + user.getUsername() + " not a member of any organization ", errorMessage); + } else { + String errorMessage = "notMemberOfOrganization"; + Response challenge = context.form() + .setError(errorMessage, organization.getName()) + .createErrorPage(Response.Status.FORBIDDEN); + context.failure(AuthenticationFlowError.GENERIC_AUTHENTICATION_ERROR, challenge, "User " + user.getUsername() + " not a member of organization " + organization.getAlias(), errorMessage); + } + + return true; + } } diff --git a/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java b/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java index dba85f8ef775..c892ab69d44c 100644 --- a/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java +++ b/services/src/main/java/org/keycloak/organization/protocol/mappers/oidc/OrganizationScope.java @@ -24,6 +24,7 @@ import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.function.BiFunction; import java.util.function.Predicate; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -63,7 +64,10 @@ public enum OrganizationScope { return getProvider(session).getByMember(user); }, (organizations) -> true, - (session, current, previous) -> valueOfScope(session, current) == null ? previous : current), + (session, current, previous) -> { + return valueOfScope(session, current) == null ? previous : current; + }, + (scopes, session) -> Stream.empty()), /** * Maps to a specific organization the user is a member. When this scope is requested by clients, only the @@ -99,7 +103,11 @@ public enum OrganizationScope { } return null; - }), + }, + (scopes, session) -> parseScopeParameter(session, scopes) + .map((String scope) -> parseScopeValue(session, scope)) + .map(alias -> getProvider(session).getByAlias(alias)) + .filter(Objects::nonNull)), /** * Maps to a single organization if the user is a member of a single organization. When this scope is requested by clients, @@ -144,7 +152,8 @@ public enum OrganizationScope { } return null; - }); + }, + ((scopes, session) -> Stream.empty())); private static final String ORGANIZATION_SCOPES_SESSION_ATTRIBUTE = "kc.org.client.scope"; private static final String UNSUPPORTED_ORGANIZATION_SCOPES_ATTRIBUTE = "kc.org.client.scope.unsupported"; @@ -159,7 +168,7 @@ public enum OrganizationScope { private final Predicate valueMatcher; /** - * Resolves the organizations based on the values of the scope. + * Resolves the organizations of the user based on the values of the scope. */ private final TriFunction> valueResolver; @@ -173,11 +182,17 @@ public enum OrganizationScope { */ private final TriFunction nameResolver; - OrganizationScope(Predicate valueMatcher, TriFunction> valueResolver, Predicate> valueValidator, TriFunction nameResolver) { + /** + * Resolves the organizations from the scope. + */ + private final BiFunction> rawValueResolver; + + OrganizationScope(Predicate valueMatcher, TriFunction> valueResolver, Predicate> valueValidator, TriFunction nameResolver, BiFunction> rawValueResolver) { this.valueMatcher = valueMatcher; this.valueResolver = valueResolver; this.valueValidator = valueValidator; this.nameResolver = nameResolver; + this.rawValueResolver = rawValueResolver; } /** @@ -186,7 +201,7 @@ public enum OrganizationScope { * @param user the user. Can be {@code null} depending on how the scope resolves its value. * @param scope the string referencing the scope * @param session the session - * @return the organizations mapped to the given {@code user}. Or an empty stream if no organizations were mapped from the {@code scope} parameter. + * @return the organizations mapped from the {@code scope} parameter. Or an empty stream if no organizations were mapped from the parameter. */ public Stream resolveOrganizations(UserModel user, String scope, KeycloakSession session) { if (isBlank(scope)) { @@ -195,6 +210,17 @@ public Stream resolveOrganizations(UserModel user, String sco return valueResolver.apply(user, scope, session).filter(OrganizationModel::isEnabled); } + /** + * Returns the organizations mapped from the {@code scope}. + * + * @param scope the string referencing the scope + * @param session the session + * @return the organizations mapped from the {@code scope} parameter. Or an empty stream if no organizations were mapped from the parameter. + */ + public Stream resolveOrganizations(KeycloakSession session, String scope) { + return rawValueResolver.apply(scope, session); + } + /** * Returns a {@link ClientScopeModel} with the given {@code name} for this scope. * diff --git a/services/src/main/java/org/keycloak/organization/utils/Organizations.java b/services/src/main/java/org/keycloak/organization/utils/Organizations.java index 434f42d28c9c..8ae845173216 100644 --- a/services/src/main/java/org/keycloak/organization/utils/Organizations.java +++ b/services/src/main/java/org/keycloak/organization/utils/Organizations.java @@ -27,6 +27,7 @@ import java.util.Objects; import java.util.Optional; import java.util.function.Consumer; +import java.util.stream.Stream; import org.keycloak.OAuth2Constants; import org.keycloak.TokenVerifier; @@ -41,6 +42,7 @@ import org.keycloak.models.GroupModel.Type; import org.keycloak.models.IdentityProviderModel; import org.keycloak.models.KeycloakSession; +import org.keycloak.models.OrganizationDomainModel; import org.keycloak.models.OrganizationModel; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; @@ -209,7 +211,18 @@ public static OrganizationModel resolveOrganization(KeycloakSession session, Use if (organizations.size() == 1) { // single organization mapped from authentication session - return organizations.get(0); + OrganizationModel resolved = organizations.get(0); + + if (user == null) { + return resolved; + } + + // make sure the user still maps to the organization from the authentication session + if (matchesOrganization(resolved, user)) { + return resolved; + } + + return null; } else if (scope != null && user != null) { // organization scope requested but no user and no single organization mapped from the scope return null; @@ -262,4 +275,16 @@ public static boolean isReadOnlyOrganizationMember(KeycloakSession session, User .anyMatch((org) -> (organizationProvider.isEnabled() && org.isManaged(delegate) && !org.isEnabled()) || (!organizationProvider.isEnabled() && org.isManaged(delegate))); } + + private static boolean matchesOrganization(OrganizationModel organization, UserModel user) { + if (organization == null || user == null) { + return false; + } + + String emailDomain = Optional.ofNullable(getEmailDomain(user.getEmail())).orElse(""); + Stream domains = organization.getDomains(); + Stream domainNames = domains.map(OrganizationDomainModel::getName); + + return organization.isMember(user) || domainNames.anyMatch(emailDomain::equals); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/authentication/OrganizationAuthenticationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/authentication/OrganizationAuthenticationTest.java index 6dfd8046ab5a..bd5dbdac3d0a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/authentication/OrganizationAuthenticationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/authentication/OrganizationAuthenticationTest.java @@ -151,14 +151,15 @@ public void testRequiresUserMembership() { runOnServer(setAuthenticatorConfig(OrganizationAuthenticatorFactory.REQUIRES_USER_MEMBERSHIP, Boolean.TRUE.toString())); try { - OrganizationResource organization = testRealm().organizations().get(createOrganization().getId()); + OrganizationRepresentation org = createOrganization(); + OrganizationResource organization = testRealm().organizations().get(org.getId()); UserRepresentation member = addMember(organization); organization.members().member(member.getId()).delete().close(); oauth.clientId("broker-app"); loginPage.open(bc.consumerRealmName()); loginPage.loginUsername(member.getEmail()); // user is not a member of any organization - assertThat(errorPage.getError(), Matchers.containsString("User is not a member of the organization " + organization.toRepresentation().getName())); + assertThat(errorPage.getError(), Matchers.containsString("User is not a member of the organization " + org.getName())); organization.members().addMember(member.getId()).close(); OrganizationRepresentation orgB = createOrganization("org-b"); @@ -169,6 +170,26 @@ public void testRequiresUserMembership() { // user is not a member of the organization selected by the client assertThat(errorPage.getError(), Matchers.containsString("User is not a member of the organization " + orgB.getName())); errorPage.assertTryAnotherWayLinkAvailability(false); + + organization.members().member(member.getId()).delete().close(); + oauth.clientId("broker-app"); + oauth.scope("organization:*"); + loginPage.open(bc.consumerRealmName()); + loginPage.loginUsername(member.getEmail()); + // user is not a member of any organization + assertThat(errorPage.getError(), Matchers.containsString("User is not a member of any organization")); + + organization.members().addMember(member.getId()).close(); + testRealm().organizations().get(orgB.getId()).members().addMember(member.getId()).close(); + oauth.clientId("broker-app"); + oauth.scope("organization"); + loginPage.open(bc.consumerRealmName()); + loginPage.loginUsername(member.getEmail()); + selectOrganizationPage.assertCurrent(); + organization.members().member(member.getId()).delete().close(); + selectOrganizationPage.selectOrganization(org.getAlias()); + // user is not a member of any organization + assertThat(errorPage.getError(), Matchers.containsString("User is not a member of the organization " + org.getName())); } finally { runOnServer(setAuthenticatorConfig(OrganizationAuthenticatorFactory.REQUIRES_USER_MEMBERSHIP, Boolean.FALSE.toString())); } 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 9c076f716386..1746bab81874 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 @@ -19,8 +19,10 @@ import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThat; +import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.keycloak.testsuite.broker.BrokerTestTools.waitForPage; @@ -208,7 +210,6 @@ public void testRealmLevelBrokersAvailableIfEmailDoesNotMatchOrganization() { IdentityProviderRepresentation idp = bc.setUpIdentityProvider(); idp.setAlias("realm-level-idp"); idp.setHideOnLogin(false); - Assert.assertFalse(loginPage.isSocialButtonPresent(idp.getAlias())); testRealm().identityProviders().create(idp).close(); driver.navigate().refresh(); @@ -216,6 +217,7 @@ public void testRealmLevelBrokersAvailableIfEmailDoesNotMatchOrganization() { Assert.assertTrue(loginPage.isUsernameInputPresent()); Assert.assertTrue(loginPage.isPasswordInputPresent()); Assert.assertTrue(loginPage.isSocialButtonPresent(idp.getAlias())); + Assert.assertFalse(loginPage.isSocialButtonPresent(bc.getIDPAlias())); } @Test @@ -713,6 +715,63 @@ public void testFailUpdateEmailWithDifferentDomainThanOrgIfBrokerHasDomainSet() } } + @Test + public void testRememberOrganizationWhenReloadingLoginPage() { + OrganizationResource organization = testRealm().organizations().get(createOrganization().getId()); + OrganizationRepresentation org1 = organization.toRepresentation(); + IdentityProviderRepresentation orgIdp = organization.identityProviders().getIdentityProviders().get(0); + orgIdp.setHideOnLogin(false); + orgIdp.getConfig().remove(OrganizationModel.ORGANIZATION_DOMAIN_ATTRIBUTE); + testRealm().identityProviders().get(orgIdp.getAlias()).update(orgIdp); + + IdentityProviderRepresentation realmIdp = bc.setUpIdentityProvider(); + realmIdp.setAlias("second-idp"); + realmIdp.setInternalId(null); + realmIdp.setHideOnLogin(false); + testRealm().identityProviders().create(realmIdp).close(); + getCleanup().addCleanup(testRealm().identityProviders().get("second-idp")::remove); + + oauth.clientId("broker-app"); + loginPage.open(bc.consumerRealmName()); + loginPage.loginUsername("test@" + org1.getDomains().iterator().next().getName()); + // only org idp + assertTrue(loginPage.isSocialButtonPresent(orgIdp.getAlias())); + assertFalse(loginPage.isSocialButtonPresent(realmIdp.getAlias())); + + driver.navigate().refresh(); + // still only org idp + assertTrue(loginPage.isSocialButtonPresent(orgIdp.getAlias())); + assertFalse(loginPage.isSocialButtonPresent(realmIdp.getAlias())); + + driver.navigate().back(); + // only org idp, back button won't reset the flow and the organization is the same + assertTrue(loginPage.isSocialButtonPresent(orgIdp.getAlias())); + assertFalse(loginPage.isSocialButtonPresent(realmIdp.getAlias())); + loginPage.loginUsername("test"); + // both realm and org idps because the user does not map to any organization + assertTrue(loginPage.isSocialButtonPresent(orgIdp.getAlias())); + assertTrue(loginPage.isSocialButtonPresent(realmIdp.getAlias())); + + loginPage.open(bc.consumerRealmName()); + loginPage.loginUsername("test@" + org1.getDomains().iterator().next().getName()); + // only org idp + assertTrue(loginPage.isSocialButtonPresent(orgIdp.getAlias())); + assertFalse(loginPage.isSocialButtonPresent(realmIdp.getAlias())); + + String org2Name = "org-2"; + OrganizationResource org2 = testRealm().organizations().get(createOrganization(org2Name).getId()); + IdentityProviderRepresentation org2Idp = org2.identityProviders().getIdentityProviders().get(0); + org2Idp.setHideOnLogin(false); + org2Idp.getConfig().remove(OrganizationModel.ORGANIZATION_DOMAIN_ATTRIBUTE); + testRealm().identityProviders().get(org2Idp.getAlias()).update(org2Idp); + driver.navigate().back(); + loginPage.loginUsername("test@" + org2.toRepresentation().getDomains().iterator().next().getName()); + // resolves to brokers from another organization + assertFalse(loginPage.isSocialButtonPresent(orgIdp.getAlias())); + assertFalse(loginPage.isSocialButtonPresent(realmIdp.getAlias())); + assertTrue(loginPage.isSocialButtonPresent(org2Idp.getAlias())); + } + private void assertIsNotMember(String userEmail, OrganizationResource organization) { UsersResource users = adminClient.realm(bc.consumerRealmName()).users(); List reps = users.searchByEmail(userEmail, true); diff --git a/themes/src/main/resources/theme/base/login/messages/messages_en.properties b/themes/src/main/resources/theme/base/login/messages/messages_en.properties index 9fc4addfd973..1422d822adde 100644 --- a/themes/src/main/resources/theme/base/login/messages/messages_en.properties +++ b/themes/src/main/resources/theme/base/login/messages/messages_en.properties @@ -530,4 +530,5 @@ organization.confirm-membership.title=You are about to join organization ${kc.or organization.confirm-membership=By clicking on the link below, you will become a member of the {0} organization: organization.member.register.title=Create an account to join the ${kc.org.name} organization organization.select=Select an organization to proceed: -notMemberOfOrganization=User is not a member of the organization {0} \ No newline at end of file +notMemberOfOrganization=User is not a member of the organization {0} +notMemberOfAnyOrganization=User is not a member of any organization \ No newline at end of file