From 320f8eb1b403fb6e970a497b4da75e4963b0daa1 Mon Sep 17 00:00:00 2001 From: Pedro Igor Date: Tue, 28 May 2024 18:00:41 -0300 Subject: [PATCH] Improve invitation messages and flow Closes #29945 Signed-off-by: Pedro Igor --- .../InviteOrgActionTokenHandler.java | 24 ++++++++--- .../forms/RegistrationPage.java | 31 ++++++++++++++ .../forms/RegistrationUserCreation.java | 18 +++----- .../OrganizationInvitationResource.java | 5 +++ .../organization/utils/Organizations.java | 18 ++++++++ .../keycloak/services/messages/Messages.java | 4 +- .../testsuite/pages/AbstractPage.java | 4 ++ .../testsuite/pages/RegisterPage.java | 8 +++- .../admin/OrganizationInvitationLinkTest.java | 41 +++++++++++++++---- .../email/messages/messages_en.properties | 4 +- .../main/resources/theme/base/login/info.ftl | 2 +- .../login/messages/messages_en.properties | 4 +- .../resources/theme/base/login/register.ftl | 6 ++- .../theme/keycloak.v2/login/register.ftl | 6 ++- 14 files changed, 141 insertions(+), 34 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/inviteorg/InviteOrgActionTokenHandler.java b/services/src/main/java/org/keycloak/authentication/actiontoken/inviteorg/InviteOrgActionTokenHandler.java index 1d9607698a..0713e75806 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/inviteorg/InviteOrgActionTokenHandler.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/inviteorg/InviteOrgActionTokenHandler.java @@ -1,13 +1,13 @@ /* * Copyright 2024 Red Hat, Inc. and/or its affiliates * and other contributors as indicated by the @author tags. - * + * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at - * + * * http://www.apache.org/licenses/LICENSE-2.0 - * + * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. @@ -17,6 +17,7 @@ package org.keycloak.authentication.actiontoken.inviteorg; import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.Response.Status; import jakarta.ws.rs.core.UriBuilder; import jakarta.ws.rs.core.UriInfo; import org.keycloak.TokenVerifier.Predicate; @@ -43,7 +44,7 @@ import org.keycloak.services.messages.Messages; import org.keycloak.sessions.AuthenticationSessionCompoundId; import org.keycloak.sessions.AuthenticationSessionModel; -import java.util.List; +import java.net.URI; import java.util.Objects; /** @@ -112,9 +113,9 @@ public class InviteOrgActionTokenHandler extends AbstractActionTokenHandler formData, String email) { if (Profile.isFeatureEnabled(Feature.ORGANIZATION)) { - MultivaluedMap queryParameters = context.getHttpRequest().getUri().getQueryParameters(); - String tokenFromQuery = queryParameters.getFirst(Constants.TOKEN); - - if (tokenFromQuery == null) { - return true; - } - Consumer> error = messages -> { - context.getEvent().detail(Messages.INVALID_ORG_INVITE, tokenFromQuery); context.error(Errors.INVALID_TOKEN); context.validationError(formData, messages); }; - TokenVerifier tokenVerifier = TokenVerifier.create(tokenFromQuery, InviteOrgActionToken.class); InviteOrgActionToken token; try { - token = tokenVerifier.getToken(); + token = Organizations.parseInvitationToken(context.getHttpRequest()); } catch (VerificationException e) { error.accept(List.of(new FormMessage("Unexpected error parsing the invitation token"))); return false; } + if (token == null) { + return true; + } + KeycloakSession session = context.getSession(); OrganizationProvider provider = session.getProvider(OrganizationProvider.class); OrganizationModel organization = provider.getById(token.getOrgId()); diff --git a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationInvitationResource.java b/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationInvitationResource.java index 1f670b18eb..a7d6f0ce04 100644 --- a/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationInvitationResource.java +++ b/services/src/main/java/org/keycloak/organization/admin/resource/OrganizationInvitationResource.java @@ -83,8 +83,13 @@ public class OrganizationInvitationResource { return sendInvitation(user); } + if (!realm.isRegistrationAllowed()) { + throw ErrorResponse.error("Realm does not allow self-registration", Status.BAD_REQUEST); + } + user = new InMemoryUserAdapter(session, realm, null); user.setEmail(email); + if (firstName != null && lastName != null) { user.setFirstName(firstName); user.setLastName(lastName); 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 512af7d5c3..efe7b9bdf0 100644 --- a/services/src/main/java/org/keycloak/organization/utils/Organizations.java +++ b/services/src/main/java/org/keycloak/organization/utils/Organizations.java @@ -19,14 +19,21 @@ package org.keycloak.organization.utils; import static java.util.Optional.ofNullable; +import jakarta.ws.rs.core.MultivaluedMap; import jakarta.ws.rs.core.Response; import java.util.List; import java.util.Objects; import java.util.Set; import java.util.function.Consumer; import java.util.stream.Collectors; + +import org.keycloak.TokenVerifier; +import org.keycloak.authentication.actiontoken.inviteorg.InviteOrgActionToken; import org.keycloak.common.Profile; import org.keycloak.common.Profile.Feature; +import org.keycloak.common.VerificationException; +import org.keycloak.http.HttpRequest; +import org.keycloak.models.Constants; import org.keycloak.models.FederatedIdentityModel; import org.keycloak.models.GroupModel; import org.keycloak.models.IdentityProviderModel; @@ -180,4 +187,15 @@ public class Organizations { public static OrganizationDomainModel toModel(OrganizationDomainRepresentation domainRepresentation) { return new OrganizationDomainModel(domainRepresentation.getName(), domainRepresentation.isVerified()); } + + public static InviteOrgActionToken parseInvitationToken(HttpRequest request) throws VerificationException { + MultivaluedMap queryParameters = request.getUri().getQueryParameters(); + String tokenFromQuery = queryParameters.getFirst(Constants.TOKEN); + + if (tokenFromQuery == null) { + return null; + } + + return TokenVerifier.create(tokenFromQuery, InviteOrgActionToken.class).getToken(); + } } diff --git a/services/src/main/java/org/keycloak/services/messages/Messages.java b/services/src/main/java/org/keycloak/services/messages/Messages.java index 0d477cf557..62ecc4558d 100755 --- a/services/src/main/java/org/keycloak/services/messages/Messages.java +++ b/services/src/main/java/org/keycloak/services/messages/Messages.java @@ -192,7 +192,7 @@ public class Messages { public static final String STALE_INVITE_ORG_LINK = "staleInviteOrgLink"; public static final String IDENTITY_PROVIDER_UNEXPECTED_ERROR = "identityProviderUnexpectedErrorMessage"; - + public static final String IDENTITY_PROVIDER_UNMATCHED_ESSENTIAL_CLAIM_ERROR = "federatedIdentityUnmatchedEssentialClaimMessage"; public static final String IDENTITY_PROVIDER_MISSING_STATE_ERROR = "identityProviderMissingStateMessage"; @@ -321,4 +321,6 @@ public class Messages { public static final String OAUTH2_DEVICE_CONSENT_DENIED = "oauth2DeviceConsentDeniedMessage"; public static final String CONFIRM_ORGANIZATION_MEMBERSHIP = "organization.confirm-membership"; + public static final String CONFIRM_ORGANIZATION_MEMBERSHIP_TITLE = "organization.confirm-membership.title"; + public static final String REGISTER_ORGANIZATION_MEMBER = "organization.member.register.title"; } diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AbstractPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AbstractPage.java index 5a18c851c0..8b8f5eb786 100755 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AbstractPage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/AbstractPage.java @@ -58,6 +58,10 @@ public abstract class AbstractPage { abstract public boolean isCurrent(); + public boolean isCurrent(String expectedTitle) { + return PageUtils.getPageTitle(driver).equals(expectedTitle); + } + abstract public void open() throws Exception; public WebDriver getDriver() { diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/RegisterPage.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/RegisterPage.java index ddab36fe98..c53b1ac445 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/RegisterPage.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/pages/RegisterPage.java @@ -26,6 +26,7 @@ import org.keycloak.models.Constants; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.testsuite.auth.page.AccountFields; import org.keycloak.testsuite.auth.page.PasswordFields; +import org.keycloak.testsuite.util.DroneUtils; import org.keycloak.testsuite.util.UIUtils; import org.openqa.selenium.By; import org.openqa.selenium.NoSuchElementException; @@ -245,7 +246,7 @@ public class RegisterPage extends AbstractPage { public boolean isCurrent() { - return PageUtils.getPageTitle(driver).equals("Register"); + return isCurrent("Register"); } public AccountFields.AccountErrors getInputAccountErrors(){ @@ -267,4 +268,9 @@ public class RegisterPage extends AbstractPage { assertCurrent(); } + public void assertCurrent(String orgName) { + String name = getClass().getSimpleName(); + Assert.assertTrue("Expected " + name + " but was " + DroneUtils.getCurrentDriver().getTitle() + " (" + DroneUtils.getCurrentDriver().getCurrentUrl() + ")", + isCurrent("Create an account to join the " + orgName + " organization")); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationInvitationLinkTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationInvitationLinkTest.java index 153c6a602f..39035675a5 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationInvitationLinkTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/organization/admin/OrganizationInvitationLinkTest.java @@ -19,6 +19,7 @@ package org.keycloak.testsuite.organization.admin; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; import java.io.IOException; @@ -29,6 +30,7 @@ import java.util.concurrent.TimeUnit; import jakarta.mail.MessagingException; import jakarta.mail.internet.MimeMessage; import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.Response.Status; import org.hamcrest.Matchers; import org.jboss.arquillian.graphene.page.Page; import org.junit.Rule; @@ -37,6 +39,7 @@ import org.keycloak.admin.client.resource.OrganizationResource; import org.keycloak.common.Profile.Feature; import org.keycloak.common.util.UriUtils; import org.keycloak.cookie.CookieType; +import org.keycloak.representations.idm.ErrorRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.testsuite.Assert; @@ -135,7 +138,7 @@ public class OrganizationInvitationLinkTest extends AbstractOrganizationTest { Assert.assertNotNull(orgToken); driver.navigate().to(link.trim()); Assert.assertFalse(organization.members().getAll().stream().anyMatch(actual -> user.getId().equals(actual.getId()))); - registerPage.assertCurrent(); + registerPage.assertCurrent(organizationName); registerPage.register("firstName", "lastName", user.getEmail(), user.getUsername(), "password", "password", null, false, null); List users = testRealm().users().searchByEmail(user.getEmail(), true); @@ -149,6 +152,27 @@ public class OrganizationInvitationLinkTest extends AbstractOrganizationTest { Assert.assertNotNull(driver.manage().getCookieNamed(CookieType.IDENTITY.getName())); } + @Test + public void testFailRegistrationNotEnabledWhenInvitingNewUser() throws IOException, MessagingException { + UserRepresentation user = UserBuilder.create() + .username("invitedUser") + .email("inviteduser@email") + .enabled(true) + .build(); + // User isn't created when we send the invite + OrganizationResource organization = testRealm().organizations().get(createOrganization().getId()); + RealmRepresentation realm = testRealm().toRepresentation(); + realm.setRegistrationAllowed(false); + testRealm().update(realm); + try (Response response = organization.members().inviteUser(user.getEmail(), null, null)) { + assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus()); + assertEquals("Realm does not allow self-registration", response.readEntity(ErrorRepresentation.class).getErrorMessage()); + } finally { + realm.setRegistrationAllowed(true); + testRealm().update(realm); + } + } + @Test public void testEmailDoesNotChangeOnRegistration() throws IOException { UserRepresentation user = UserBuilder.create() @@ -168,8 +192,7 @@ public class OrganizationInvitationLinkTest extends AbstractOrganizationTest { Assert.assertNotNull(orgToken); driver.navigate().to(link.trim()); Assert.assertFalse(organization.members().getAll().stream().anyMatch(actual -> user.getId().equals(actual.getId()))); - registerPage.assertCurrent(); - driver.manage().timeouts().pageLoadTimeout(1, TimeUnit.DAYS); + registerPage.assertCurrent(organizationName); registerPage.register("firstName", "lastName", "invalid@email.com", user.getUsername(), "password", "password", null, false, null); Assert.assertTrue(driver.getPageSource().contains("Email does not match the invitation")); @@ -198,7 +221,7 @@ public class OrganizationInvitationLinkTest extends AbstractOrganizationTest { Assert.assertNotNull(orgToken); driver.navigate().to(link.trim()); Assert.assertFalse(organization.members().getAll().stream().anyMatch(actual -> user.getId().equals(actual.getId()))); - registerPage.assertCurrent(); + registerPage.assertCurrent(organizationName); driver.manage().timeouts().pageLoadTimeout(1, TimeUnit.DAYS); registerPage.register("firstName", "lastName", "invalid@email.com", user.getUsername(), "password", "password", null, false, null); @@ -216,17 +239,19 @@ public class OrganizationInvitationLinkTest extends AbstractOrganizationTest { Assert.assertEquals("Invitation to join the " + organizationName + " organization", message.getSubject()); EmailBody body = MailUtils.getBody(message); if (user.getFirstName() != null && user.getLastName() != null) { - assertThat(body.getText(), Matchers.containsString(user.getFirstName() + " " + user.getLastName())); + assertThat(body.getText(), Matchers.containsString("Hi, " + user.getFirstName() + " " + user.getLastName() + ".")); } String link = MailUtils.getLink(body.getHtml()); driver.navigate().to(link.trim()); // not yet a member Assert.assertFalse(organization.members().getAll().stream().anyMatch(actual -> user.getId().equals(actual.getId()))); // confirm the intent of membership - assertThat(infoPage.getInfo(), containsString("You are about to join organization " + organizationName)); + assertThat(driver.getPageSource(), containsString("You are about to join organization " + organizationName)); + assertThat(infoPage.getInfo(), containsString("By clicking on the link below, you will become a member of the " + organizationName + " organization:")); infoPage.clickToContinue(); - assertThat(infoPage.getInfo(), containsString("Your account has been updated.")); + // redirect to the account console and eventually force the user to authenticate if not already + assertThat(driver.getTitle(), containsString("Account Management")); // now a member Assert.assertNotNull(organization.members().member(user.getId()).toRepresentation()); } -} \ No newline at end of file +} diff --git a/themes/src/main/resources/theme/base/email/messages/messages_en.properties b/themes/src/main/resources/theme/base/email/messages/messages_en.properties index 2e89968a79..4f41282efa 100755 --- a/themes/src/main/resources/theme/base/email/messages/messages_en.properties +++ b/themes/src/main/resources/theme/base/email/messages/messages_en.properties @@ -4,8 +4,8 @@ emailVerificationBodyHtml=

Someone has created a {2} account with this email a orgInviteSubject=Invitation to join the {0} organization orgInviteBody=You were invited to join the "{3}" organization. Click the link below to join.\n\n{0}\n\nThis link will expire within {4}.\n\nIf you don't want to join the organization, just ignore this message. orgInviteBodyHtml=

You were invited to join the {3} organization. Click the link below to join.

Link to join the organization

This link will expire within {4}.

If you don't want to join the organization, just ignore this message.

-orgInviteBodyPersonalized="{5}" "{6}"\n\n You were invited to join the "{3}" organization. Click the link below to join.\n\n{0}\n\nThis link will expire within {4}.\n\nIf you don't want to join the organization, just ignore this message. -orgInviteBodyPersonalizedHtml=

{5} {6}

You were invited to join the {3} organization. Click the link below to join.

Link to join the organization

This link will expire within {4}.

If you don't want to join the organization, just ignore this message.

+orgInviteBodyPersonalized=Hi, "{5}" "{6}".\n\n You were invited to join the "{3}" organization. Click the link below to join.\n\n{0}\n\nThis link will expire within {4}.\n\nIf you don't want to join the organization, just ignore this message. +orgInviteBodyPersonalizedHtml=

Hi, {5} {6}.

You were invited to join the {3} organization. Click the link below to join.

Link to join the organization

This link will expire within {4}.

If you don't want to join the organization, just ignore this message.

emailUpdateConfirmationSubject=Verify new email emailUpdateConfirmationBody=To update your {2} account with email address {1}, click the link below\n\n{0}\n\nThis link will expire within {3}.\n\nIf you don''t want to proceed with this modification, just ignore this message. emailUpdateConfirmationBodyHtml=

To update your {2} account with email address {1}, click the link below

{0}

This link will expire within {3}.

If you don''t want to proceed with this modification, just ignore this message.

diff --git a/themes/src/main/resources/theme/base/login/info.ftl b/themes/src/main/resources/theme/base/login/info.ftl index 400e8bfdff..d8cb648802 100755 --- a/themes/src/main/resources/theme/base/login/info.ftl +++ b/themes/src/main/resources/theme/base/login/info.ftl @@ -2,7 +2,7 @@ <@layout.registrationLayout displayMessage=false; section> <#if section = "header"> <#if messageHeader??> - ${messageHeader} + ${kcSanitize(msg("${messageHeader}"))?no_esc} <#else> ${message.summary} 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 fe49ca08e2..c023a6ed4d 100755 --- a/themes/src/main/resources/theme/base/login/messages/messages_en.properties +++ b/themes/src/main/resources/theme/base/login/messages/messages_en.properties @@ -518,4 +518,6 @@ doLogout=Logout readOnlyUsernameMessage=You can''t update your username as it is read-only. error-invalid-multivalued-size=Attribute {0} must have at least {1} and at most {2} value(s). -requiredAction.organization.confirm-membership=You are about to join organization ${kc.org.name} \ No newline at end of file +organization.confirm-membership.title=You are about to join organization ${kc.org.name} +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 diff --git a/themes/src/main/resources/theme/base/login/register.ftl b/themes/src/main/resources/theme/base/login/register.ftl index 662c488bef..f640848683 100755 --- a/themes/src/main/resources/theme/base/login/register.ftl +++ b/themes/src/main/resources/theme/base/login/register.ftl @@ -3,7 +3,11 @@ <#import "register-commons.ftl" as registerCommons> <@layout.registrationLayout displayMessage=messagesPerField.exists('global') displayRequiredFields=true; section> <#if section = "header"> - ${msg("registerTitle")} + <#if messageHeader??> + ${kcSanitize(msg("${messageHeader}"))?no_esc} + <#else> + ${msg("registerTitle")} + <#elseif section = "form">
diff --git a/themes/src/main/resources/theme/keycloak.v2/login/register.ftl b/themes/src/main/resources/theme/keycloak.v2/login/register.ftl index 9d778e2648..3fe455499e 100755 --- a/themes/src/main/resources/theme/keycloak.v2/login/register.ftl +++ b/themes/src/main/resources/theme/keycloak.v2/login/register.ftl @@ -3,7 +3,11 @@ <#import "register-commons.ftl" as registerCommons> <@layout.registrationLayout displayMessage=messagesPerField.exists('global') displayRequiredFields=true; section> <#if section = "header"> - ${msg("registerTitle")} + <#if messageHeader??> + ${kcSanitize(msg("${messageHeader}"))?no_esc} + <#else> + ${msg("registerTitle")} + <#elseif section = "form">