From 57fca2a34f6feb61292538c4fa3ee0bb69b51263 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1clav=20Muzik=C3=A1=C5=99?= Date: Fri, 30 Apr 2021 16:25:28 +0200 Subject: [PATCH] KEYCLOAK-15170 Reset password link is not invalidated if email address is changed --- ...r.java => AbstractActionTokenHandler.java} | 14 +++++++-- .../actiontoken/DefaultActionToken.java | 30 ++++++++++++++----- .../ExecuteActionsActionToken.java | 5 ++++ .../ExecuteActionsActionTokenHandler.java | 6 ++-- .../IdpVerifyAccountLinkActionToken.java | 3 +- ...dpVerifyAccountLinkActionTokenHandler.java | 5 ++-- .../ResetCredentialsActionToken.java | 3 +- .../ResetCredentialsActionTokenHandler.java | 10 ++++--- .../verifyemail/VerifyEmailActionToken.java | 14 +-------- .../VerifyEmailActionTokenHandler.java | 4 +-- .../IdpEmailVerificationAuthenticator.java | 2 +- .../resetcred/ResetCredentialEmail.java | 2 +- .../resources/admin/UserResource.java | 2 +- .../RequiredActionEmailVerificationTest.java | 24 +++++++++++++++ .../testsuite/forms/ResetPasswordTest.java | 20 +++++++++++++ 15 files changed, 107 insertions(+), 37 deletions(-) rename services/src/main/java/org/keycloak/authentication/actiontoken/{AbstractActionTokenHander.java => AbstractActionTokenHandler.java} (79%) diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/AbstractActionTokenHander.java b/services/src/main/java/org/keycloak/authentication/actiontoken/AbstractActionTokenHandler.java similarity index 79% rename from services/src/main/java/org/keycloak/authentication/actiontoken/AbstractActionTokenHander.java rename to services/src/main/java/org/keycloak/authentication/actiontoken/AbstractActionTokenHandler.java index 0fb41d082e..ffa3aa8c06 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/AbstractActionTokenHander.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/AbstractActionTokenHandler.java @@ -17,18 +17,21 @@ package org.keycloak.authentication.actiontoken; import org.keycloak.Config.Scope; +import org.keycloak.TokenVerifier; +import org.keycloak.events.Errors; import org.keycloak.events.EventType; import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSessionFactory; import org.keycloak.representations.JsonWebToken; import org.keycloak.services.managers.AuthenticationManager; +import org.keycloak.services.messages.Messages; import org.keycloak.sessions.AuthenticationSessionModel; /** * * @author hmlnarik */ -public abstract class AbstractActionTokenHander implements ActionTokenHandler, ActionTokenHandlerFactory { +public abstract class AbstractActionTokenHandler implements ActionTokenHandler, ActionTokenHandlerFactory { private final String id; private final Class tokenClass; @@ -36,7 +39,7 @@ public abstract class AbstractActionTokenHander implemen private final EventType defaultEventType; private final String defaultEventError; - public AbstractActionTokenHander(String id, Class tokenClass, String defaultErrorMessage, EventType defaultEventType, String defaultEventError) { + public AbstractActionTokenHandler(String id, Class tokenClass, String defaultErrorMessage, EventType defaultEventType, String defaultEventError) { this.id = id; this.tokenClass = tokenClass; this.defaultErrorMessage = defaultErrorMessage; @@ -102,4 +105,11 @@ public abstract class AbstractActionTokenHander implemen public boolean canUseTokenRepeatedly(T token, ActionTokenContext tokenContext) { return true; } + + protected TokenVerifier.Predicate verifyEmail(ActionTokenContext context) { + return TokenUtils.checkThat( + t -> t.getEmail() == null || t.getEmail().equals(context.getAuthenticationSession().getAuthenticatedUser().getEmail()), + Errors.INVALID_EMAIL, Messages.INVALID_EMAIL + ); + } } diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/DefaultActionToken.java b/services/src/main/java/org/keycloak/authentication/actiontoken/DefaultActionToken.java index fb4df465ff..17a01ee34d 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/DefaultActionToken.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/DefaultActionToken.java @@ -17,16 +17,20 @@ package org.keycloak.authentication.actiontoken; -import org.keycloak.TokenVerifier.Predicate; -import org.keycloak.common.VerificationException; - -import org.keycloak.common.util.Time; -import org.keycloak.models.*; -import org.keycloak.services.Urls; import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonProperty; -import java.util.*; +import org.keycloak.TokenVerifier.Predicate; +import org.keycloak.common.VerificationException; +import org.keycloak.common.util.Time; +import org.keycloak.models.ActionTokenValueModel; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.RealmModel; +import org.keycloak.services.Urls; + import javax.ws.rs.core.UriInfo; +import java.util.HashMap; +import java.util.Map; +import java.util.UUID; /** * Part of action token that is intended to be used e.g. in link sent in password-reset email. @@ -37,6 +41,10 @@ import javax.ws.rs.core.UriInfo; public class DefaultActionToken extends DefaultActionTokenKey implements ActionTokenValueModel { public static final String JSON_FIELD_AUTHENTICATION_SESSION_ID = "asid"; + public static final String JSON_FIELD_EMAIL = "eml"; + + @JsonProperty(value = JSON_FIELD_EMAIL) + private String email; public static final Predicate ACTION_TOKEN_BASIC_CHECKS = t -> { if (t.getActionVerificationNonce() == null) { @@ -122,6 +130,14 @@ public class DefaultActionToken extends DefaultActionTokenKey implements ActionT return res instanceof String ? (String) res : null; } + public void setEmail(String email) { + this.email = email; + } + + public String getEmail() { + return email; + } + /** * Updates the following fields and serializes this token into a signed JWT. The list of updated fields follows: *
    diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionToken.java b/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionToken.java index 7c32e2dce5..e6acd8c57d 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionToken.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionToken.java @@ -38,6 +38,11 @@ public class ExecuteActionsActionToken extends DefaultActionToken { this.issuedFor = clientId; } + public ExecuteActionsActionToken(String userId, String email, int absoluteExpirationInSecs, List requiredActions, String redirectUri, String clientId) { + this(userId, absoluteExpirationInSecs, requiredActions, redirectUri, clientId); + setEmail(email); + } + private ExecuteActionsActionToken() { } diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java b/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java index 8d4738cea6..d146a5682a 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/execactions/ExecuteActionsActionTokenHandler.java @@ -40,7 +40,7 @@ import javax.ws.rs.core.UriInfo; * * @author hmlnarik */ -public class ExecuteActionsActionTokenHandler extends AbstractActionTokenHander { +public class ExecuteActionsActionTokenHandler extends AbstractActionTokenHandler { public ExecuteActionsActionTokenHandler() { super( @@ -62,7 +62,9 @@ public class ExecuteActionsActionTokenHandler extends AbstractActionTokenHander< tokenContext.getAuthenticationSession().getClient()) != null, Errors.INVALID_REDIRECT_URI, Messages.INVALID_REDIRECT_URI - ) + ), + + verifyEmail(tokenContext) ); } diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionToken.java b/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionToken.java index 004f88b4e0..d81d21f252 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionToken.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionToken.java @@ -42,12 +42,13 @@ public class IdpVerifyAccountLinkActionToken extends DefaultActionToken { private String originalAuthenticationSessionId; - public IdpVerifyAccountLinkActionToken(String userId, int absoluteExpirationInSecs, String compoundAuthenticationSessionId, + public IdpVerifyAccountLinkActionToken(String userId, String email, int absoluteExpirationInSecs, String compoundAuthenticationSessionId, String identityProviderUsername, String identityProviderAlias, String clientId) { super(userId, TOKEN_TYPE, absoluteExpirationInSecs, null, compoundAuthenticationSessionId); this.identityProviderUsername = identityProviderUsername; this.identityProviderAlias = identityProviderAlias; this.issuedFor = clientId; + setEmail(email); } private IdpVerifyAccountLinkActionToken() { diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionTokenHandler.java b/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionTokenHandler.java index 306dd05f8e..092e939ac3 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionTokenHandler.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/idpverifyemail/IdpVerifyAccountLinkActionTokenHandler.java @@ -16,7 +16,7 @@ */ package org.keycloak.authentication.actiontoken.idpverifyemail; -import org.keycloak.authentication.actiontoken.AbstractActionTokenHander; +import org.keycloak.authentication.actiontoken.AbstractActionTokenHandler; import org.keycloak.TokenVerifier.Predicate; import org.keycloak.authentication.AuthenticationProcessor; import org.keycloak.authentication.actiontoken.*; @@ -43,7 +43,7 @@ import javax.ws.rs.core.UriInfo; * Action token handler for verification of e-mail address. * @author hmlnarik */ -public class IdpVerifyAccountLinkActionTokenHandler extends AbstractActionTokenHander { +public class IdpVerifyAccountLinkActionTokenHandler extends AbstractActionTokenHandler { public IdpVerifyAccountLinkActionTokenHandler() { super( @@ -58,6 +58,7 @@ public class IdpVerifyAccountLinkActionTokenHandler extends AbstractActionTokenH @Override public Predicate[] getVerifiers(ActionTokenContext tokenContext) { return TokenUtils.predicates( + verifyEmail(tokenContext) ); } diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionToken.java b/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionToken.java index bb7969c07a..f04f13e776 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionToken.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionToken.java @@ -27,9 +27,10 @@ public class ResetCredentialsActionToken extends DefaultActionToken { public static final String TOKEN_TYPE = "reset-credentials"; - public ResetCredentialsActionToken(String userId, int absoluteExpirationInSecs, String compoundAuthenticationSessionId, String clientId) { + public ResetCredentialsActionToken(String userId, String email, int absoluteExpirationInSecs, String compoundAuthenticationSessionId, String clientId) { super(userId, TOKEN_TYPE, absoluteExpirationInSecs, null, compoundAuthenticationSessionId); this.issuedFor = clientId; + setEmail(email); } private ResetCredentialsActionToken() { diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionTokenHandler.java b/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionTokenHandler.java index bc5773577e..567b5c0ea7 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionTokenHandler.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/resetcred/ResetCredentialsActionTokenHandler.java @@ -24,19 +24,19 @@ import org.keycloak.authentication.authenticators.broker.util.SerializedBrokered import org.keycloak.events.Errors; import org.keycloak.events.EventType; import org.keycloak.models.UserModel; -import org.keycloak.services.ErrorPage; import org.keycloak.services.messages.Messages; import org.keycloak.services.resources.LoginActionsService; import org.keycloak.services.resources.LoginActionsServiceChecks.IsActionRequired; import org.keycloak.sessions.CommonClientSessionModel.Action; import javax.ws.rs.core.Response; + import static org.keycloak.services.resources.LoginActionsService.RESET_CREDENTIALS_PATH; /** * * @author hmlnarik */ -public class ResetCredentialsActionTokenHandler extends AbstractActionTokenHander { +public class ResetCredentialsActionTokenHandler extends AbstractActionTokenHandler { public ResetCredentialsActionTokenHandler() { super( @@ -51,11 +51,13 @@ public class ResetCredentialsActionTokenHandler extends AbstractActionTokenHande @Override public Predicate[] getVerifiers(ActionTokenContext tokenContext) { - return new Predicate[] { + return TokenUtils.predicates( TokenUtils.checkThat(tokenContext.getRealm()::isResetPasswordAllowed, Errors.NOT_ALLOWED, Messages.RESET_CREDENTIAL_NOT_ALLOWED), + verifyEmail(tokenContext), + new IsActionRequired(tokenContext, Action.AUTHENTICATE) - }; + ); } @Override diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionToken.java b/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionToken.java index 4d0d5a5041..576536f9cf 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionToken.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionToken.java @@ -28,32 +28,20 @@ public class VerifyEmailActionToken extends DefaultActionToken { public static final String TOKEN_TYPE = "verify-email"; - private static final String JSON_FIELD_EMAIL = "eml"; private static final String JSON_FIELD_ORIGINAL_AUTHENTICATION_SESSION_ID = "oasid"; - @JsonProperty(value = JSON_FIELD_EMAIL) - private String email; - @JsonProperty(value = JSON_FIELD_ORIGINAL_AUTHENTICATION_SESSION_ID) private String originalAuthenticationSessionId; public VerifyEmailActionToken(String userId, int absoluteExpirationInSecs, String compoundAuthenticationSessionId, String email, String clientId) { super(userId, TOKEN_TYPE, absoluteExpirationInSecs, null, compoundAuthenticationSessionId); - this.email = email; + setEmail(email); this.issuedFor = clientId; } private VerifyEmailActionToken() { } - public String getEmail() { - return email; - } - - public void setEmail(String email) { - this.email = email; - } - public String getCompoundOriginalAuthenticationSessionId() { return originalAuthenticationSessionId; } diff --git a/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionTokenHandler.java b/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionTokenHandler.java index ad27e95fca..18ac7a8d7c 100644 --- a/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionTokenHandler.java +++ b/services/src/main/java/org/keycloak/authentication/actiontoken/verifyemail/VerifyEmailActionTokenHandler.java @@ -16,7 +16,7 @@ */ package org.keycloak.authentication.actiontoken.verifyemail; -import org.keycloak.authentication.actiontoken.AbstractActionTokenHander; +import org.keycloak.authentication.actiontoken.AbstractActionTokenHandler; import org.keycloak.TokenVerifier.Predicate; import org.keycloak.authentication.actiontoken.*; import org.keycloak.events.*; @@ -41,7 +41,7 @@ import javax.ws.rs.core.UriInfo; * Action token handler for verification of e-mail address. * @author hmlnarik */ -public class VerifyEmailActionTokenHandler extends AbstractActionTokenHander { +public class VerifyEmailActionTokenHandler extends AbstractActionTokenHandler { public VerifyEmailActionTokenHandler() { super( diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpEmailVerificationAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpEmailVerificationAuthenticator.java index c6a4967f46..7c104a07bd 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpEmailVerificationAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpEmailVerificationAuthenticator.java @@ -129,7 +129,7 @@ public class IdpEmailVerificationAuthenticator extends AbstractIdpAuthenticator String authSessionEncodedId = AuthenticationSessionCompoundId.fromAuthSession(authSession).getEncodedId(); IdpVerifyAccountLinkActionToken token = new IdpVerifyAccountLinkActionToken( - existingUser.getId(), absoluteExpirationInSecs, authSessionEncodedId, + existingUser.getId(), existingUser.getEmail(), absoluteExpirationInSecs, authSessionEncodedId, brokerContext.getUsername(), brokerContext.getIdpConfig().getAlias(), authSession.getClient().getClientId() ); UriBuilder builder = Urls.actionTokenBuilder(uriInfo.getBaseUri(), token.serialize(session, realm, uriInfo), diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/resetcred/ResetCredentialEmail.java b/services/src/main/java/org/keycloak/authentication/authenticators/resetcred/ResetCredentialEmail.java index 4fb8826c6f..abb2f0b627 100755 --- a/services/src/main/java/org/keycloak/authentication/authenticators/resetcred/ResetCredentialEmail.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/resetcred/ResetCredentialEmail.java @@ -91,7 +91,7 @@ public class ResetCredentialEmail implements Authenticator, AuthenticatorFactory // We send the secret in the email in a link as a query param. String authSessionEncodedId = AuthenticationSessionCompoundId.fromAuthSession(authenticationSession).getEncodedId(); - ResetCredentialsActionToken token = new ResetCredentialsActionToken(user.getId(), absoluteExpirationInSecs, authSessionEncodedId, authenticationSession.getClient().getClientId()); + ResetCredentialsActionToken token = new ResetCredentialsActionToken(user.getId(), user.getEmail(), absoluteExpirationInSecs, authSessionEncodedId, authenticationSession.getClient().getClientId()); String link = UriBuilder .fromUri(context.getActionTokenUrl(token.serialize(context.getSession(), context.getRealm(), context.getUriInfo()))) .build() diff --git a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java index 796b4241b6..dfbebd5145 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/UserResource.java @@ -781,7 +781,7 @@ public class UserResource { lifespan = realm.getActionTokenGeneratedByAdminLifespan(); } int expiration = Time.currentTime() + lifespan; - ExecuteActionsActionToken token = new ExecuteActionsActionToken(user.getId(), expiration, actions, redirectUri, clientId); + ExecuteActionsActionToken token = new ExecuteActionsActionToken(user.getId(), user.getEmail(), expiration, actions, redirectUri, clientId); try { UriBuilder builder = LoginActionsService.actionTokenProcessor(session.getContext().getUri()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java index 5cab3f2e22..5df50e435c 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/actions/RequiredActionEmailVerificationTest.java @@ -17,6 +17,7 @@ package org.keycloak.testsuite.actions; import org.jboss.arquillian.drone.api.annotation.Drone; +import org.keycloak.admin.client.resource.UserResource; import org.keycloak.authentication.actiontoken.verifyemail.VerifyEmailActionToken; import org.jboss.arquillian.graphene.page.Page; import org.junit.Assert; @@ -999,4 +1000,27 @@ public class RequiredActionEmailVerificationTest extends AbstractTestRealmKeyclo setTimeOffset(0); } } + + // KEYCLOAK-15170 + @Test + public void changeEmailAddressAfterSendingEmail() throws Exception { + loginPage.open(); + loginPage.login("test-user@localhost", "password"); + + verifyEmailPage.assertCurrent(); + + assertEquals(1, greenMail.getReceivedMessages().length); + + MimeMessage message = greenMail.getReceivedMessages()[0]; + String verificationUrl = getPasswordResetEmailLink(message); + + UserResource user = testRealm().users().get(testUserId); + UserRepresentation userRep = user.toRepresentation(); + userRep.setEmail("vmuzikar@redhat.com"); + user.update(userRep); + + driver.navigate().to(verificationUrl.trim()); + errorPage.assertCurrent(); + assertEquals("The link you clicked is an old stale link and is no longer valid. Maybe you have already verified your email.", errorPage.getError()); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java index 4d97dbd8ed..c7f56d5c07 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java @@ -1156,6 +1156,26 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { } } + // KEYCLOAK-15170 + @Test + public void changeEmailAddressAfterSendingEmail() throws IOException { + initiateResetPasswordFromResetPasswordPage(defaultUser.getUsername()); + + assertEquals(1, greenMail.getReceivedMessages().length); + + MimeMessage message = greenMail.getReceivedMessages()[0]; + String changePasswordUrl = MailUtils.getPasswordResetEmailLink(message); + + UserResource user = testRealm().users().get(defaultUser.getId()); + UserRepresentation userRep = user.toRepresentation(); + userRep.setEmail("vmuzikar@redhat.com"); + user.update(userRep); + + driver.navigate().to(changePasswordUrl.trim()); + errorPage.assertCurrent(); + assertEquals("Invalid email address.", errorPage.getError()); + } + private void changePasswordOnUpdatePage(WebDriver driver) { assertThat(driver.getPageSource(), Matchers.containsString("You need to change your password."));