From ec9bf6206e3d937a974ac84b9618c00823edc884 Mon Sep 17 00:00:00 2001 From: Martin Bartos Date: Mon, 8 Jun 2020 11:18:20 +0200 Subject: [PATCH] [KEYCLOAK-13202] Reset password redirects to account client --- .../protocol/oidc/utils/RedirectUtils.java | 23 ++- .../resources/LoginActionsService.java | 22 ++- .../testsuite/util/BrowserTabUtil.java | 158 +++++++++++++++ .../testsuite/forms/ResetPasswordTest.java | 181 +++++++++++++++++- 4 files changed, 363 insertions(+), 21 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/BrowserTabUtil.java diff --git a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java index 0b108f1ace..dd8cebb2a1 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/utils/RedirectUtils.java @@ -28,7 +28,6 @@ import org.keycloak.services.Urls; import org.keycloak.services.util.ResolveRelative; import java.net.URI; -import java.net.URISyntaxException; import java.util.Collection; import java.util.HashSet; import java.util.Set; @@ -189,10 +188,26 @@ public class RedirectUtils { private static String getSingleValidRedirectUri(Collection validRedirects) { if (validRedirects.size() != 1) return null; String validRedirect = validRedirects.iterator().next(); - int idx = validRedirect.indexOf("/*"); + return validateRedirectUriWildcard(validRedirect); + } + + public static String validateRedirectUriWildcard(String redirectUri) { + if (redirectUri == null) + return null; + + int idx = redirectUri.indexOf("/*"); if (idx > -1) { - validRedirect = validRedirect.substring(0, idx); + redirectUri = redirectUri.substring(0, idx); } - return validRedirect; + return redirectUri; + } + + private static String getFirstValidRedirectUri(Collection validRedirects) { + final String redirectUri = validRedirects.stream().findFirst().orElse(null); + return (redirectUri != null) ? validateRedirectUriWildcard(redirectUri) : null; + } + + public static String getFirstValidRedirectUri(KeycloakSession session, String rootUrl, Set validRedirects) { + return getFirstValidRedirectUri(resolveValidRedirects(session, rootUrl, validRedirects)); } } diff --git a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java index 905f385fe1..955c648bf7 100755 --- a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java +++ b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java @@ -48,7 +48,6 @@ import org.keycloak.events.EventBuilder; import org.keycloak.events.EventType; import org.keycloak.exceptions.TokenNotActiveException; import org.keycloak.locale.LocaleSelectorProvider; -import org.keycloak.locale.LocaleSelectorSPI; import org.keycloak.locale.LocaleUpdaterProvider; import org.keycloak.models.ActionTokenKeyModel; import org.keycloak.models.AuthenticationFlowModel; @@ -70,6 +69,7 @@ import org.keycloak.protocol.LoginProtocol.Error; import org.keycloak.protocol.oidc.OIDCLoginProtocol; import org.keycloak.protocol.oidc.utils.OIDCResponseMode; 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.ServicesLogger; @@ -380,10 +380,10 @@ public class LoginActionsService { if (!realm.isResetPasswordAllowed()) { event.event(EventType.RESET_PASSWORD); event.error(Errors.NOT_ALLOWED); - return ErrorPage.error(session, authSession, Response.Status.BAD_REQUEST, Messages.RESET_CREDENTIAL_NOT_ALLOWED); + return ErrorPage.error(session, null, Response.Status.BAD_REQUEST, Messages.RESET_CREDENTIAL_NOT_ALLOWED); } - authSession = createAuthenticationSessionForClient(); + authSession = createAuthenticationSessionForClient(clientId); return processResetCredentials(false, null, authSession, null); } @@ -391,12 +391,19 @@ public class LoginActionsService { return resetCredentials(authSessionId, code, execution, clientId, tabId); } - AuthenticationSessionModel createAuthenticationSessionForClient() - throws UriBuilderException, IllegalArgumentException { + AuthenticationSessionModel createAuthenticationSessionForClient(String clientID) + throws UriBuilderException, IllegalArgumentException { AuthenticationSessionModel authSession; - // set up the account service as the endpoint to call. - ClientModel client = SystemClientUtil.getSystemClient(realm); + ClientModel client = session.realms().getClientByClientId(clientID, realm); + String redirectUri; + + if (client == null) { + 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()); + } RootAuthenticationSessionModel rootAuthSession = new AuthenticationSessionManager(session).createAuthenticationSession(realm, true); authSession = rootAuthSession.createAuthenticationSession(client); @@ -404,7 +411,6 @@ public class LoginActionsService { authSession.setAction(AuthenticationSessionModel.Action.AUTHENTICATE.name()); //authSession.setNote(AuthenticationManager.END_AFTER_REQUIRED_ACTIONS, "true"); authSession.setProtocol(OIDCLoginProtocol.LOGIN_PROTOCOL); - String redirectUri = Urls.accountBase(session.getContext().getUri().getBaseUri()).path("/").build(realm.getName()).toString(); authSession.setRedirectUri(redirectUri); authSession.setClientNote(OIDCLoginProtocol.RESPONSE_TYPE_PARAM, OAuth2Constants.CODE); authSession.setClientNote(OIDCLoginProtocol.REDIRECT_URI_PARAM, redirectUri); diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/BrowserTabUtil.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/BrowserTabUtil.java new file mode 100644 index 0000000000..7fc4144c2f --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/BrowserTabUtil.java @@ -0,0 +1,158 @@ +/* + * Copyright 2020 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. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.testsuite.util; + +import com.gargoylesoftware.htmlunit.WebClient; +import org.jboss.arquillian.drone.webdriver.htmlunit.DroneHtmlUnitDriver; +import org.openqa.selenium.JavascriptExecutor; +import org.openqa.selenium.WebDriver; +import org.openqa.selenium.htmlunit.HtmlUnitDriver; + +import java.util.ArrayList; +import java.util.List; +import java.util.Set; + +/** + * Helper class for managing tabs in browser. + * Tabs are indexed from 0. (f.e. first tab has index 0) + * + *

Note: For one particular WebDriver has to exist only one BrowserTabUtil instance. (Right order of tabs)

+ * + * @author Martin Bartos + */ +public class BrowserTabUtil implements AutoCloseable { + + private WebDriver driver; + private JavascriptExecutor jsExecutor; + private List tabs; + private static List instances; + + private BrowserTabUtil(WebDriver driver) { + this.driver = driver; + + if (driver instanceof JavascriptExecutor) { + this.jsExecutor = (JavascriptExecutor) driver; + } else { + throw new RuntimeException("WebDriver must be instance of JavascriptExecutor"); + } + + // HtmlUnit doesn't work very well with JS and it's recommended to use this settings. + // HtmlUnit validates all scripts and then fails. It turned off the validation. + if (driver instanceof HtmlUnitDriver) { + WebClient client = ((DroneHtmlUnitDriver) driver).getWebClient(); + client.getOptions().setThrowExceptionOnScriptError(false); + client.getOptions().setThrowExceptionOnFailingStatusCode(false); + } + + tabs = new ArrayList<>(driver.getWindowHandles()); + } + + public static BrowserTabUtil getInstanceAndSetEnv(WebDriver driver) { + if (instances == null) { + instances = new ArrayList<>(); + } + + BrowserTabUtil instance = instances.stream() + .filter(inst -> inst.getDriver().toString().equals(driver.toString())) + .findFirst() + .orElse(null); + + if (instance == null) { + instance = new BrowserTabUtil(driver); + instances.add(instance); + } + return instance; + } + + public WebDriver getDriver() { + return driver; + } + + public String getActualWindowHandle() { + return driver.getWindowHandle(); + } + + public void switchToTab(String windowHandle) { + driver.switchTo().window(windowHandle); + WaitUtils.waitForPageToLoad(); + } + + public void switchToTab(int index) { + assertValidIndex(index); + switchToTab(tabs.get(index)); + } + + public void newTab(String url) { + jsExecutor.executeScript("window.open(arguments[0]);", url); + + final Set handles = driver.getWindowHandles(); + final String tabHandle = handles.stream() + .filter(tab -> !tabs.contains(tab)) + .findFirst() + .orElse(null); + + if (handles.size() > tabs.size() + 1) { + throw new RuntimeException("Too many window handles. You can only create a new one by this method."); + } + + if (tabHandle == null) { + throw new RuntimeException("Creating the new tab failed."); + } + + tabs.add(tabHandle); + switchToTab(tabHandle); + } + + public void closeTab(int index) { + assertValidIndex(index); + + if (index == 0 || getCountOfTabs() == 1) + throw new RuntimeException("You must not close the original tab."); + + switchToTab(index); + driver.close(); + + tabs.remove(index); + switchToTab(index - 1); + } + + public int getCountOfTabs() { + return tabs.size(); + } + + public void destroy() { + for (int i = 1; i < getCountOfTabs(); i++) { + closeTab(i); + } + instances.removeIf(inst -> inst.getDriver().toString().equals(driver.toString())); + } + + private boolean validIndex(int index) { + return (index >= 0 && tabs != null && index < tabs.size()); + } + + private void assertValidIndex(int index) { + if (!validIndex(index)) + throw new IndexOutOfBoundsException("Invalid index of tab."); + } + + @Override + public void close() { + destroy(); + } +} \ No newline at end of file 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 b9f0afe895..f2975f817d 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 @@ -27,6 +27,8 @@ import org.keycloak.events.Errors; import org.keycloak.events.EventType; import org.keycloak.models.Constants; import org.keycloak.models.utils.SystemClientUtil; +import org.keycloak.protocol.oidc.utils.RedirectUtils; +import org.keycloak.representations.idm.ClientRepresentation; import org.keycloak.representations.idm.EventRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; @@ -43,6 +45,7 @@ import org.keycloak.testsuite.pages.LoginPasswordResetPage; import org.keycloak.testsuite.pages.LoginPasswordUpdatePage; import org.keycloak.testsuite.pages.VerifyEmailPage; import org.keycloak.testsuite.updaters.ClientAttributeUpdater; +import org.keycloak.testsuite.util.BrowserTabUtil; import org.keycloak.testsuite.util.GreenMailRule; import org.keycloak.testsuite.util.MailUtils; import org.keycloak.testsuite.util.OAuthClient; @@ -56,18 +59,23 @@ import javax.mail.internet.MimeMessage; import java.io.Closeable; import java.io.IOException; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; import org.junit.*; +import org.keycloak.testsuite.util.WaitUtils; import org.openqa.selenium.By; import org.openqa.selenium.WebDriver; import org.openqa.selenium.WebElement; import static org.hamcrest.MatcherAssert.assertThat; import static org.junit.Assert.*; + import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.AuthServer; /** @@ -78,6 +86,7 @@ import org.keycloak.testsuite.arquillian.annotation.AuthServerContainerExclude.A public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { private String userId; + private UserRepresentation defaultUser; @Drone @SecondBrowser @@ -92,13 +101,14 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { @Before public void setup() { log.info("Adding login-test user"); - UserRepresentation user = UserBuilder.create() + defaultUser = UserBuilder.create() .username("login-test") .email("login@test.com") .enabled(true) .build(); - userId = ApiUtil.createUserAndResetPasswordWithAdminClient(testRealm(), user, "password"); + userId = ApiUtil.createUserAndResetPasswordWithAdminClient(testRealm(), defaultUser, "password"); + defaultUser.setId(userId); expectedMessagesCount = 0; getCleanup().addUserId(userId); } @@ -1048,12 +1058,7 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { driver2.navigate().to(changePasswordUrl.trim()); - final WebElement newPassword = driver2.findElement(By.id("password-new")); - newPassword.sendKeys("resetPassword"); - final WebElement confirmPassword = driver2.findElement(By.id("password-confirm")); - confirmPassword.sendKeys("resetPassword"); - final WebElement submit = driver2.findElement(By.cssSelector("input[type=\"submit\"]")); - submit.click(); + changePasswordOnUpdatePage(driver2); assertThat(driver2.getCurrentUrl(), Matchers.containsString("client_id=test-app")); @@ -1073,7 +1078,7 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { serviceAccount = serviceAccount1.toRepresentation(); serviceAccount.setEmail("client-user@test.com"); serviceAccount1.update(serviceAccount); - + String resetUri = oauth.AUTH_SERVER_ROOT + "/realms/test/login-actions/reset-credentials"; driver.navigate().to(resetUri); @@ -1084,4 +1089,162 @@ public class ResetPasswordTest extends AbstractTestRealmKeycloakTest { loginPage.assertCurrent(); assertEquals("Invalid username or password.", errorPage.getError()); } + + @Test + public void resetPasswordLinkNewTabAndProperRedirectAccount() throws IOException { + final String REQUIRED_URI = OAuthClient.AUTH_SERVER_ROOT + "/realms/test/account/applications"; + final String REDIRECT_URI = getAccountRedirectUrl() + "?path=applications"; + final String CLIENT_ID = "account"; + + try (BrowserTabUtil tabUtil = BrowserTabUtil.getInstanceAndSetEnv(driver)) { + assertThat(tabUtil.getCountOfTabs(), Matchers.is(1)); + + driver.navigate().to(REQUIRED_URI); + resetPasswordTwiceInNewTab(defaultUser, CLIENT_ID, false, REDIRECT_URI, REQUIRED_URI); + assertThat(driver.getTitle(), Matchers.equalTo("Keycloak Account Management")); + + oauth.openLogout(); + + driver.navigate().to(REQUIRED_URI); + resetPasswordTwiceInNewTab(defaultUser, CLIENT_ID, true, REDIRECT_URI, REQUIRED_URI); + assertThat(driver.getTitle(), Matchers.equalTo("Keycloak Account Management")); + } + } + + @Test + public void resetPasswordLinkNewTabAndProperRedirectClient() throws IOException { + final String REDIRECT_URI = OAuthClient.AUTH_SERVER_ROOT + "/realms/master/app/auth"; + final String CLIENT_ID = "test-app"; + + try (BrowserTabUtil tabUtil = BrowserTabUtil.getInstanceAndSetEnv(driver)) { + assertThat(tabUtil.getCountOfTabs(), Matchers.is(1)); + + loginPage.open(); + resetPasswordTwiceInNewTab(defaultUser, CLIENT_ID, false, REDIRECT_URI); + assertThat(driver.getCurrentUrl(), Matchers.containsString(REDIRECT_URI)); + + oauth.openLogout(); + + loginPage.open(); + resetPasswordTwiceInNewTab(defaultUser, CLIENT_ID, true, REDIRECT_URI); + assertThat(driver.getCurrentUrl(), Matchers.containsString(REDIRECT_URI)); + + } + } + + private void changePasswordOnUpdatePage(WebDriver driver) { + assertThat(driver.getPageSource(), Matchers.containsString("You need to change your password.")); + + final WebElement newPassword = driver.findElement(By.id("password-new")); + newPassword.sendKeys("resetPassword"); + final WebElement confirmPassword = driver.findElement(By.id("password-confirm")); + confirmPassword.sendKeys("resetPassword"); + final WebElement submit = driver.findElement(By.cssSelector("input[type=\"submit\"]")); + + submit.click(); + } + + private void resetPasswordTwiceInNewTab(UserRepresentation user, String clientId, boolean shouldLogOut, String redirectUri) throws IOException { + resetPasswordTwiceInNewTab(user, clientId, shouldLogOut, redirectUri, redirectUri); + } + + private void resetPasswordTwiceInNewTab(UserRepresentation user, String clientId, boolean shouldLogOut, String redirectUri, String requiredUri) throws IOException { + events.clear(); + updateForgottenPassword(user, clientId, redirectUri, requiredUri); + + if (shouldLogOut) { + String sessionId = events.expectLogin().user(user.getId()).detail(Details.USERNAME, user.getUsername()) + .detail(Details.REDIRECT_URI, redirectUri) + .client(clientId) + .assertEvent().getSessionId(); + oauth.openLogout(); + events.expectLogout(sessionId).user(user.getId()).session(sessionId).assertEvent(); + } + + BrowserTabUtil util = BrowserTabUtil.getInstanceAndSetEnv(driver); + assertThat(util.getCountOfTabs(), Matchers.equalTo(2)); + util.closeTab(1); + assertThat(util.getCountOfTabs(), Matchers.equalTo(1)); + + if (shouldLogOut) { + final ClientRepresentation client = testRealm().clients() + .findByClientId(clientId) + .stream() + .findFirst() + .orElse(null); + + assertThat(client, Matchers.notNullValue()); + System.out.println("HEE"); + System.out.println(client.getRootUrl()); + updateForgottenPassword(user, clientId, getValidRedirectUriWithRootUrl(client.getRootUrl(), client.getRedirectUris())); + } else { + doForgotPassword(user.getUsername()); + } + } + + private void updateForgottenPassword(UserRepresentation user, String clientId, String redirectUri) throws IOException { + updateForgottenPassword(user, clientId, redirectUri, redirectUri); + } + + private void updateForgottenPassword(UserRepresentation user, String clientId, String redirectUri, String requiredUri) throws IOException { + final int emailCount = greenMail.getReceivedMessages().length; + + doForgotPassword(user.getUsername()); + assertEquals("You should receive an email shortly with further instructions.", loginPage.getSuccessMessage()); + + events.expectRequiredAction(EventType.SEND_RESET_PASSWORD) + .user(user.getId()) + .client(clientId) + .detail(Details.REDIRECT_URI, redirectUri) + .detail(Details.USERNAME, user.getUsername()) + .detail(Details.EMAIL, user.getEmail()) + .session((String) null) + .assertEvent(); + + assertEquals(emailCount + 1, greenMail.getReceivedMessages().length); + + final MimeMessage message = greenMail.getReceivedMessages()[emailCount]; + final String changePasswordUrl = MailUtils.getPasswordResetEmailLink(message); + + BrowserTabUtil util = BrowserTabUtil.getInstanceAndSetEnv(driver); + util.newTab(changePasswordUrl.trim()); + + changePasswordOnUpdatePage(driver); + + events.expectRequiredAction(EventType.UPDATE_PASSWORD) + .detail(Details.REDIRECT_URI, redirectUri) + .client(clientId) + .user(user.getId()).detail(Details.USERNAME, user.getUsername()).assertEvent(); + + assertThat(driver.getCurrentUrl(), Matchers.containsString(requiredUri)); + } + + private void doForgotPassword(String username) { + loginPage.assertCurrent(); + loginPage.resetPassword(); + + resetPasswordPage.assertCurrent(); + resetPasswordPage.changePassword(username); + WaitUtils.waitForPageToLoad(); + } + + private String getValidRedirectUriWithRootUrl(String rootUrl, Collection redirectUris) { + final boolean isRootUrlValid = isValidUrl(rootUrl); + + return redirectUris.stream() + .map(uri -> isRootUrlValid && uri.startsWith("/") ? rootUrl + uri : uri) + .map(uri -> uri.startsWith("/") ? OAuthClient.AUTH_SERVER_ROOT + uri : uri) + .map(RedirectUtils::validateRedirectUriWildcard) + .findFirst() + .orElse(null); + } + + private boolean isValidUrl(String url) { + try { + new URL(url); + return true; + } catch (MalformedURLException e) { + return false; + } + } }