Remove random redirect after password reset (#27076)

closes #20867

Signed-off-by: mposolda <mposolda@gmail.com>


Co-authored-by: Ricardo Martin <rmartinc@redhat.com>

services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java

@@ -287,13 +287,4 @@ public class RedirectUtils {
         }
         return redirectUri;
     }
-
-    private static String getFirstValidRedirectUri(Collection<String> validRedirects) {
-        final String redirectUri = validRedirects.stream().findFirst().orElse(null);
-        return (redirectUri != null) ? validateRedirectUriWildcard(redirectUri) : null;
-    }
-
-    public static String getFirstValidRedirectUri(KeycloakSession session, String rootUrl, Set<String> validRedirects) {
-        return getFirstValidRedirectUri(resolveValidRedirects(session, rootUrl, validRedirects));
-    }
 }

services/src/main/java/org/keycloak/services/resources/LoginActionsService.java

@@ -76,6 +76,8 @@ import org.keycloak.protocol.oidc.utils.OIDCResponseType;
 import org.keycloak.protocol.oidc.utils.RedirectUtils;
 import org.keycloak.representations.JsonWebToken;
 import org.keycloak.services.ErrorPage;
+import org.keycloak.services.ErrorPageException;
+import org.keycloak.services.ErrorResponseException;
 import org.keycloak.services.ServicesLogger;
 import org.keycloak.services.Urls;
 import org.keycloak.services.managers.AuthenticationManager;
@@ -87,7 +89,6 @@ import org.keycloak.services.util.AuthenticationFlowURLHelper;
 import org.keycloak.services.util.BrowserHistoryHelper;
 import org.keycloak.services.util.CacheControlUtil;
 import org.keycloak.services.util.LocaleUtil;
-import org.keycloak.sessions.AuthenticationSessionCompoundId;
 import org.keycloak.sessions.AuthenticationSessionModel;
 import org.keycloak.sessions.RootAuthenticationSessionModel;
 
@@ -422,6 +423,7 @@ public class LoginActionsService {
                                         @QueryParam(SESSION_CODE) String code,
                                         @QueryParam(Constants.EXECUTION) String execution,
                                         @QueryParam(Constants.CLIENT_ID) String clientId,
+                                        @QueryParam(OIDCLoginProtocol.REDIRECT_URI_PARAM) String redirectUri,
                                         @QueryParam(Constants.TAB_ID) String tabId) {
         ClientModel client = realm.getClientByClientId(clientId);
         AuthenticationSessionModel authSession = new AuthenticationSessionManager(session).getCurrentAuthenticationSession(realm, client, tabId);
@@ -435,7 +437,7 @@ public class LoginActionsService {
                 return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.RESET_CREDENTIAL_NOT_ALLOWED);
 
             }
-            authSession = createAuthenticationSessionForClient(clientId);
+            authSession = createAuthenticationSessionForClient(clientId, redirectUri);
             return processResetCredentials(false, null, authSession, null);
         }
 
@@ -443,31 +445,50 @@ public class LoginActionsService {
         return resetCredentials(authSessionId, code, execution, clientId, tabId);
     }
 
-    AuthenticationSessionModel createAuthenticationSessionForClient(String clientID)
+    AuthenticationSessionModel createAuthenticationSessionForClient(String clientID, String redirectUriParam)
             throws UriBuilderException, IllegalArgumentException {
         AuthenticationSessionModel authSession;
 
-        ClientModel client = session.clients().getClientByClientId(realm, clientID);
+        ClientModel client;
-        String redirectUri;
+        String redirectUri = null;
 
-        if (client == null) {
+        if (clientID == null) {
+            if (redirectUriParam != null) {
+                logger.warn("Unsupported to send 'redirect_uri' parameter without providing 'client_id' parameter.");
+                throw new ErrorPageException(session, null, Response.Status.BAD_REQUEST, Messages.MISSING_PARAMETER, OIDCLoginProtocol.CLIENT_ID_PARAM);
+            }
             client = SystemClientUtil.getSystemClient(realm);
             redirectUri = Urls.accountBase(session.getContext().getUri().getBaseUri()).path("/").build(realm.getName()).toString();
         } else {
-            redirectUri = RedirectUtils.getFirstValidRedirectUri(session, client.getRootUrl(), client.getRedirectUris());
+            client = session.clients().getClientByClientId(realm, clientID);
+            if (client == null) {
+                throw new ErrorPageException(session, null, Response.Status.BAD_REQUEST, Messages.CLIENT_NOT_FOUND);
+            }
+            if (!client.isEnabled()) {
+                throw new ErrorPageException(session, null, Response.Status.BAD_REQUEST, Messages.CLIENT_DISABLED);
+            }
+            if (redirectUriParam != null) {
+                redirectUri = RedirectUtils.verifyRedirectUri(session, redirectUriParam, client);
+                if (redirectUri == null) {
+                    throw new ErrorPageException(session, null, Response.Status.BAD_REQUEST, Messages.INVALID_PARAMETER, OIDCLoginProtocol.REDIRECT_URI_PARAM);
+                }
+            }
         }
 
         RootAuthenticationSessionModel rootAuthSession = new AuthenticationSessionManager(session).createAuthenticationSession(realm, true);
         authSession = rootAuthSession.createAuthenticationSession(client);
 
         authSession.setAction(AuthenticationSessionModel.Action.AUTHENTICATE.name());
-        //authSession.setNote(AuthenticationManager.END_AFTER_REQUIRED_ACTIONS, "true");
         authSession.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL);
-        authSession.setRedirectUri(redirectUri);
         authSession.setClientNote(OIDCLoginProtocol.RESPONSE_TYPE_PARAM, OAuth2Constants.CODE);
-        authSession.setClientNote(OIDCLoginProtocol.REDIRECT_URI_PARAM, redirectUri);
         authSession.setClientNote(OIDCLoginProtocol.ISSUER, Urls.realmIssuer(session.getContext().getUri().getBaseUri(), realm.getName()));
 
+        if (redirectUri != null) {
+            authSession.setRedirectUri(redirectUri);
+            authSession.setClientNote(OIDCLoginProtocol.REDIRECT_URI_PARAM, redirectUri);
+        } else {
+            authSession.setAuthNote(AuthenticationManager.END_AFTER_REQUIRED_ACTIONS, "true");
+        }
         return authSession;
     }
 

testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/ResetPasswordTest.java

@@ -16,8 +16,10 @@
  */
 package org.keycloak.testsuite.forms;
 
+import org.hamcrest.MatcherAssert;
 import org.hamcrest.Matchers;
 import org.jboss.arquillian.drone.api.annotation.Drone;
+import org.keycloak.admin.client.resource.ClientResource;
 import org.keycloak.admin.client.resource.UserResource;
 import org.keycloak.authentication.actiontoken.resetcred.ResetCredentialsActionToken;
 import org.jboss.arquillian.graphene.page.Page;
@@ -29,6 +31,7 @@ import org.keycloak.events.EventType;
 import org.keycloak.models.AuthenticationExecutionModel;
 import org.keycloak.models.Constants;
 import org.keycloak.models.utils.SystemClientUtil;
+import org.keycloak.representations.idm.ClientRepresentation;
 import org.keycloak.representations.idm.EventRepresentation;
 import org.keycloak.representations.idm.RealmRepresentation;
 import org.keycloak.representations.idm.UserRepresentation;
@@ -80,6 +83,7 @@ import org.openqa.selenium.firefox.FirefoxDriver;
 import org.openqa.selenium.htmlunit.HtmlUnitDriver;
 
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.endsWith;
 import static org.junit.Assert.*;
 
 /**
@@ -155,6 +159,22 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest {
     public void resetPasswordLink() throws IOException, MessagingException {
         String username = "login-test";
         String resetUri = oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials";
+
+        openResetPasswordUrlAndDoFlow(resetUri, "account", oauth.AUTH_SERVER_ROOT + "/realms/test/account/");
+
+        AccountHelper.logout(testRealm(), username);
+
+        TestAppHelper testAppHelper = new TestAppHelper(oauth, loginPage, appPage);
+        testAppHelper.login(username, "resetPassword");
+
+        appPage.assertCurrent();
+
+        assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType());
+    }
+
+    // Starts by opening "reset-password-url". Then go through the successful reset-password flow for the particular user. After user confirms new password, this method ends.
+    private void openResetPasswordUrlAndDoFlow(String resetUri, String expectedClientId, String expectedRedirectUri) throws IOException {
+        String username = "login-test";
         driver.navigate().to(resetUri);
 
         resetPasswordPage.assertCurrent();
@@ -164,14 +184,18 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest {
         loginPage.assertCurrent();
         assertEquals("You should receive an email shortly with further instructions.", loginPage.getSuccessMessage());
 
-        events.expectRequiredAction(EventType.SEND_RESET_PASSWORD)
+        AssertEvents.ExpectedEvent event = events.expectRequiredAction(EventType.SEND_RESET_PASSWORD)
                 .user(userId)
-                .detail(Details.REDIRECT_URI,  oauth.AUTH_SERVER_ROOT + "/realms/test/account/")
+                .client(expectedClientId)
-                .client("account")
                 .detail(Details.USERNAME, username)
                 .detail(Details.EMAIL, "login@test.com")
-                .session((String)null)
+                .session((String)null);
-                .assertEvent();
+        if (expectedRedirectUri != null) {
+            event.detail(Details.REDIRECT_URI,  expectedRedirectUri);
+        } else {
+            event.removeDetail(Details.REDIRECT_URI);
+        }
+        event.assertEvent();
 
         assertEquals(1, greenMail.getReceivedMessages().length);
 
@@ -185,19 +209,71 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest {
 
         updatePasswordPage.changePassword("resetPassword", "resetPassword");
 
-        events.expectRequiredAction(EventType.UPDATE_PASSWORD)
+        event = events.expectRequiredAction(EventType.UPDATE_PASSWORD)
-                .detail(Details.REDIRECT_URI, oauth.AUTH_SERVER_ROOT + "/realms/test/account/")
+                .client(expectedClientId)
-                .client("account")
+                .user(userId).detail(Details.USERNAME, username);
-                .user(userId).detail(Details.USERNAME, username).assertEvent();
+        if (expectedRedirectUri != null) {
+            event.detail(Details.REDIRECT_URI,  expectedRedirectUri);
+        } else {
+            event.removeDetail(Details.REDIRECT_URI);
+        }
+        event.assertEvent();
+    }
 
-        AccountHelper.logout(testRealm(), username);
+    @Test
+    public void resetPasswordLinkTestAppWithoutRedirectUriParam() throws IOException {
+        String resetUri = oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials?client_id=test-app";
 
-        TestAppHelper testAppHelper = new TestAppHelper(oauth, loginPage, appPage);
+        openResetPasswordUrlAndDoFlow(resetUri, "test-app", null);
-        testAppHelper.login("login-test", "resetPassword");
 
+        // Link "Back to application" with the baseUrl of client "test-app"
+        infoPage.assertCurrent();
+        assertEquals("Your account has been updated.", infoPage.getInfo());
+
+        infoPage.clickBackToApplicationLink();
+        WaitUtils.waitForPageToLoad();
+        MatcherAssert.assertThat(driver.getCurrentUrl(), endsWith("/app/auth"));
+    }
+
+    @Test
+    public void resetPasswordLinkTestAppWithRedirectUriParam() throws IOException {
+        String resetUri = oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials?client_id=test-app&redirect_uri=" + oauth.getRedirectUri();
+
+        openResetPasswordUrlAndDoFlow(resetUri, "test-app", oauth.getRedirectUri());
+
+        // Should be directly redirected to "application because of "redirect_uri" parameter
         appPage.assertCurrent();
+    }
 
-        assertEquals(RequestType.AUTH_RESPONSE, appPage.getRequestType());
+    @Test
+    public void resetPasswordLinkErrorFlows() throws IOException {
+        // Client not found
+        driver.navigate().to(oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials?client_id=not_found");
+        errorPage.assertCurrent();
+        Assert.assertEquals("Client not found.", errorPage.getError());
+
+        // Redirect_uri without client
+        driver.navigate().to(oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials?redirect_uri=https://foo/bar/");
+        errorPage.assertCurrent();
+        Assert.assertEquals("Missing parameters: client_id", errorPage.getError());
+
+        // Incorrect redirect_uri
+        driver.navigate().to(oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials?client_id=test-app&redirect_uri=https://foo/bar/");
+        errorPage.assertCurrent();
+        Assert.assertEquals("Invalid parameter: redirect_uri", errorPage.getError());
+
+        // Client disabled
+        ClientResource client = ApiUtil.findClientByClientId(testRealm(), "test-app");
+        ClientRepresentation clientRep = client.toRepresentation();
+        clientRep.setEnabled(false);
+        client.update(clientRep);
+        try {
+            driver.navigate().to(oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials?client_id=test-app");
+            Assert.assertEquals("Client disabled.", errorPage.getError());
+        } finally {
+            clientRep.setEnabled(true);
+            client.update(clientRep);
+        }
     }