keycloak-aplcache

Merge pull request #4314 from mposolda/KEYCLOAK-5136-browserRefreshImprove KEYCLOAK-5136

7/11/2017 9:36:03 AM

Details

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 eed2858..a3983a4 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 f3d4769..054cc6e 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 0f3ebbe..b5011fb 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 ef34b16..6832f10 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("</[hH][eE][aA][dD]>");
 
         @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/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/AbstractFirstBrokerLoginTest.java
index acf775c..ca5cb76 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");
 
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 08d8265..da54a72 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();