From 3fca731395b4724816aee8edccffef09a1b7d219 Mon Sep 17 00:00:00 2001 From: mposolda Date: Tue, 11 Jul 2017 12:23:07 +0200 Subject: [PATCH] KEYCLOAK-5136 Improve browser refresh button after switch to different flow --- .../resources/IdentityBrokerService.java | 4 +-- .../resources/LoginActionsService.java | 8 +++--- .../services/resources/SessionCodeChecks.java | 14 ++++++++-- .../services/util/BrowserHistoryHelper.java | 27 ++++++++++++++----- .../testsuite/forms/BrowserButtonsTest.java | 25 ++++++++++++++++- .../broker/AbstractFirstBrokerLoginTest.java | 4 --- 6 files changed, 63 insertions(+), 19 deletions(-) diff --git a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java index eed2858e73..a3983a4ac6 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java @@ -979,7 +979,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal return ParsedCodeContext.response(staleCodeError); } - SessionCodeChecks checks = new SessionCodeChecks(realmModel, uriInfo, clientConnection, session, event, code, null, clientId, LoginActionsService.AUTHENTICATE_PATH); + SessionCodeChecks checks = new SessionCodeChecks(realmModel, uriInfo, request, clientConnection, session, event, code, null, clientId, LoginActionsService.AUTHENTICATE_PATH); checks.initialVerify(); if (!checks.verifyActiveAndValidAction(AuthenticationSessionModel.Action.AUTHENTICATE.name(), ClientSessionCode.ActionType.LOGIN)) { @@ -993,7 +993,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal Response errorResponse = checks.getResponse(); // Remove "code" from browser history - errorResponse = BrowserHistoryHelper.getInstance().saveResponseAndRedirect(session, authSession, errorResponse, true); + errorResponse = BrowserHistoryHelper.getInstance().saveResponseAndRedirect(session, authSession, errorResponse, true, request); return ParsedCodeContext.response(errorResponse); } } else { 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 f3d4769786..054cc6ef0a 100755 --- a/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java +++ b/services/src/main/java/org/keycloak/services/resources/LoginActionsService.java @@ -179,7 +179,7 @@ public class LoginActionsService { } private SessionCodeChecks checksForCode(String code, String execution, String clientId, String flowPath) { - SessionCodeChecks res = new SessionCodeChecks(realm, uriInfo, clientConnection, session, event, code, execution, clientId, flowPath); + SessionCodeChecks res = new SessionCodeChecks(realm, uriInfo, request, clientConnection, session, event, code, execution, clientId, flowPath); res.initialVerify(); return res; } @@ -200,7 +200,7 @@ public class LoginActionsService { @GET public Response restartSession(@QueryParam("client_id") String clientId) { event.event(EventType.RESTART_AUTHENTICATION); - SessionCodeChecks checks = new SessionCodeChecks(realm, uriInfo, clientConnection, session, event, null, null, clientId, null); + SessionCodeChecks checks = new SessionCodeChecks(realm, uriInfo, request, clientConnection, session, event, null, null, clientId, null); AuthenticationSessionModel authSession = checks.initialVerifyAuthSession(); if (authSession == null) { @@ -286,7 +286,7 @@ public class LoginActionsService { authSession = processor.getAuthenticationSession(); // Could be changed (eg. Forked flow) } - return BrowserHistoryHelper.getInstance().saveResponseAndRedirect(session, authSession, response, action); + return BrowserHistoryHelper.getInstance().saveResponseAndRedirect(session, authSession, response, action, request); } /** @@ -927,7 +927,7 @@ public class LoginActionsService { throw new RuntimeException("Unreachable"); } - return BrowserHistoryHelper.getInstance().saveResponseAndRedirect(session, authSession, response, true); + return BrowserHistoryHelper.getInstance().saveResponseAndRedirect(session, authSession, response, true, request); } } diff --git a/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java b/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java index 0f3ebbe158..b5011fb2cc 100644 --- a/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java +++ b/services/src/main/java/org/keycloak/services/resources/SessionCodeChecks.java @@ -24,6 +24,7 @@ import javax.ws.rs.core.UriBuilder; import javax.ws.rs.core.UriInfo; import org.jboss.logging.Logger; +import org.jboss.resteasy.spi.HttpRequest; import org.keycloak.authentication.AuthenticationProcessor; import org.keycloak.common.ClientConnection; import org.keycloak.common.util.ObjectUtil; @@ -59,6 +60,7 @@ public class SessionCodeChecks { private final RealmModel realm; private final UriInfo uriInfo; + private final HttpRequest request; private final ClientConnection clientConnection; private final KeycloakSession session; private final EventBuilder event; @@ -69,9 +71,10 @@ public class SessionCodeChecks { private final String flowPath; - public SessionCodeChecks(RealmModel realm, UriInfo uriInfo, ClientConnection clientConnection, KeycloakSession session, EventBuilder event, String code, String execution, String clientId, String flowPath) { + public SessionCodeChecks(RealmModel realm, UriInfo uriInfo, HttpRequest request, ClientConnection clientConnection, KeycloakSession session, EventBuilder event, String code, String execution, String clientId, String flowPath) { this.realm = realm; this.uriInfo = uriInfo; + this.request = request; this.clientConnection = clientConnection; this.session = session; this.event = event; @@ -220,10 +223,17 @@ public class SessionCodeChecks { } } - if (ObjectUtil.isEqualOrBothNull(execution, lastExecFromSession)) { + if (execution == null || execution.equals(lastExecFromSession)) { // Allow refresh of previous page clientCode = new ClientSessionCode<>(session, realm, authSession); actionRequest = false; + + // Allow refresh, but rewrite browser history + if (execution == null && lastExecFromSession != null) { + logger.debugf("Parameter 'execution' is not in the request, but flow wasn't changed. Will update browser history"); + request.setAttribute(BrowserHistoryHelper.SHOULD_UPDATE_BROWSER_HISTORY, true); + } + return true; } else { response = showPageExpired(authSession); diff --git a/services/src/main/java/org/keycloak/services/util/BrowserHistoryHelper.java b/services/src/main/java/org/keycloak/services/util/BrowserHistoryHelper.java index ef34b16116..6832f10985 100644 --- a/services/src/main/java/org/keycloak/services/util/BrowserHistoryHelper.java +++ b/services/src/main/java/org/keycloak/services/util/BrowserHistoryHelper.java @@ -24,6 +24,7 @@ import java.util.regex.Pattern; import javax.ws.rs.core.Response; import org.jboss.logging.Logger; +import org.jboss.resteasy.spi.HttpRequest; import org.keycloak.models.KeycloakSession; import org.keycloak.sessions.AuthenticationSessionModel; import org.keycloak.theme.BrowserSecurityHeaderSetup; @@ -44,13 +45,27 @@ import org.keycloak.utils.MediaType; */ public abstract class BrowserHistoryHelper { + // Request attribute, which specifies if flow was changed in this request (eg. click "register" from the login screen) + public static final String SHOULD_UPDATE_BROWSER_HISTORY = "SHOULD_UPDATE_BROWSER_HISTORY"; + protected static final Logger logger = Logger.getLogger(BrowserHistoryHelper.class); - public abstract Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest); + public abstract Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest, HttpRequest httpRequest); public abstract Response loadSavedResponse(KeycloakSession session, AuthenticationSessionModel authSession); + protected boolean shouldReplaceBrowserHistory(boolean actionRequest, HttpRequest httpRequest) { + if (actionRequest) { + return true; + } + + Boolean flowChanged = (Boolean) httpRequest.getAttribute(SHOULD_UPDATE_BROWSER_HISTORY); + return (flowChanged != null && flowChanged); + } + + + // Always rely on javascript for now public static BrowserHistoryHelper getInstance() { return new JavascriptHistoryReplace(); @@ -66,8 +81,8 @@ public abstract class BrowserHistoryHelper { private static final Pattern HEAD_END_PATTERN = Pattern.compile(""); @Override - public Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest) { - if (!actionRequest) { + public Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest, HttpRequest httpRequest) { + if (!shouldReplaceBrowserHistory(actionRequest, httpRequest)) { return response; } @@ -129,8 +144,8 @@ public abstract class BrowserHistoryHelper { private static final String CACHED_RESPONSE = "cached.response"; @Override - public Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest) { - if (!actionRequest) { + public Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest, HttpRequest httpRequest) { + if (!shouldReplaceBrowserHistory(actionRequest, httpRequest)) { return response; } @@ -179,7 +194,7 @@ public abstract class BrowserHistoryHelper { private static class NoOpHelper extends BrowserHistoryHelper { @Override - public Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest) { + public Response saveResponseAndRedirect(KeycloakSession session, AuthenticationSessionModel authSession, Response response, boolean actionRequest, HttpRequest httpRequest) { return response; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserButtonsTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserButtonsTest.java index 08d826522e..da54a72301 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserButtonsTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/BrowserButtonsTest.java @@ -263,11 +263,12 @@ public class BrowserButtonsTest extends AbstractTestRealmKeycloakTest { registerPage.clickBackToLogin(); loginPage.assertCurrent(); - // Click browser "back" button. Should be back on register page + // Click browser "back" button. driver.navigate().back(); registerPage.assertCurrent(); } + @Test public void clickBackButtonFromRegisterPage() { loginPage.open(); @@ -280,6 +281,28 @@ public class BrowserButtonsTest extends AbstractTestRealmKeycloakTest { } + // KEYCLOAK-5136 + @Test + public void clickRefreshButtonOnRegisterPage() { + loginPage.open(); + loginPage.clickRegister(); + registerPage.assertCurrent(); + + // Click browser "refresh" button. Should be still on register page + driver.navigate().refresh(); + registerPage.assertCurrent(); + + // Click 'back to login'. Should be on login page + registerPage.clickBackToLogin(); + loginPage.assertCurrent(); + + // Click browser 'refresh'. Should be still on login page + driver.navigate().refresh(); + loginPage.assertCurrent(); + + } + + @Test public void backButtonToAuthorizationEndpoint() { loginPage.open(); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java index acf775c313..ca5cb76ff9 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java @@ -540,16 +540,12 @@ public abstract class AbstractFirstBrokerLoginTest extends AbstractIdentityProvi driver.navigate().back(); Assert.assertTrue(driver.getPageSource().contains("You are already logged in.")); driver.navigate().forward(); - this.loginExpiredPage.assertCurrent(); - this.loginExpiredPage.clickLoginContinueLink(); this.idpConfirmLinkPage.assertCurrent(); // Click browser 'back' on review profile page this.idpConfirmLinkPage.clickReviewProfile(); this.updateProfilePage.assertCurrent(); driver.navigate().back(); - this.loginExpiredPage.assertCurrent(); - this.loginExpiredPage.clickLoginContinueLink(); this.updateProfilePage.assertCurrent(); this.updateProfilePage.update("Pedro", "Igor", "psilva@redhat.com");