Merge pull request #4397 from mposolda/master

KEYCLOAK-5260 kc_idp_hint was only working first time

services/src/main/java/org/keycloak/protocol/AuthorizationEndpointBase.java

@@ -180,9 +180,10 @@ public abstract class AuthorizationEndpointBase {
                 return new AuthorizationEndpointChecks(authSession);
 
             } else if (isNewRequest(authSession, client, requestState)) {
-                // Check if we have lastProcessedExecution and restart the session just if yes. Otherwise update just client information from the AuthorizationEndpoint request.
+                // Check if we have lastProcessedExecution note or if some request parameter beside state (eg. prompt, kc_idp_hint) changed. Restart the session just if yes.
+                // Otherwise update just client information from the AuthorizationEndpoint request.
                 // This difference is needed, because of logout from JS applications in multiple browser tabs.
-                if (hasProcessedExecution(authSession)) {
+                if (shouldRestartAuthSession(authSession)) {
                     logger.debug("New request from application received, but authentication session already exists. Restart existing authentication session");
                     authSession.restartSession(realm, client);
                 } else {
@@ -223,11 +224,18 @@ public abstract class AuthorizationEndpointBase {
 
     }
 
+
+    protected boolean shouldRestartAuthSession(AuthenticationSessionModel authSession) {
+        return hasProcessedExecution(authSession);
+    }
+
+
     private boolean hasProcessedExecution(AuthenticationSessionModel authSession) {
         String lastProcessedExecution = authSession.getAuthNote(AuthenticationProcessor.LAST_PROCESSED_EXECUTION);
         return (lastProcessedExecution != null);
     }
 
+
     // See if we have lastProcessedExecution note. If yes, we are expired. Also if we are in different flow than initial one. Otherwise it is browser refresh of initial username/password form
     private boolean shouldShowExpirePage(AuthenticationSessionModel authSession) {
         if (hasProcessedExecution(authSession)) {

services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java

@@ -49,6 +49,8 @@ import org.keycloak.util.TokenUtil;
 import javax.ws.rs.GET;
 import javax.ws.rs.core.MultivaluedMap;
 import javax.ws.rs.core.Response;
+
+import java.util.Objects;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -370,7 +372,48 @@ public class AuthorizationEndpoint extends AuthorizationEndpointBase {
 
         // If state is same, we likely have the refresh of some previous request
         String stateFromSession = authSession.getClientNote(OIDCLoginProtocol.STATE_PARAM);
-        return !stateFromRequest.equals(stateFromSession);
+        boolean stateChanged =!stateFromRequest.equals(stateFromSession);
+        if (stateChanged) {
+            return true;
+        }
+
+        return isOIDCAuthenticationRelatedParamsChanged(authSession);
+    }
+
+
+    @Override
+    protected boolean shouldRestartAuthSession(AuthenticationSessionModel authSession) {
+        return super.shouldRestartAuthSession(authSession) || isOIDCAuthenticationRelatedParamsChanged(authSession);
+    }
+
+
+    // Check if some important OIDC parameters, which have impact on authentication, changed. If yes, we need to restart auth session
+    private boolean isOIDCAuthenticationRelatedParamsChanged(AuthenticationSessionModel authSession) {
+        if (isRequestParamChanged(authSession, OIDCLoginProtocol.LOGIN_HINT_PARAM, request.getLoginHint())) {
+            return true;
+        }
+        if (isRequestParamChanged(authSession, OIDCLoginProtocol.PROMPT_PARAM, request.getPrompt())) {
+            return true;
+        }
+        if (isRequestParamChanged(authSession, AdapterConstants.KC_IDP_HINT, request.getIdpHint())) {
+            return true;
+        }
+
+        String maxAgeValue = authSession.getClientNote(OIDCLoginProtocol.MAX_AGE_PARAM);
+        if (maxAgeValue == null && request.getMaxAge() == null) {
+            return false;
+        }
+        if (maxAgeValue != null && Integer.parseInt(maxAgeValue) == request.getMaxAge()) {
+            return false;
+        }
+
+        return true;
+    }
+
+
+    private boolean isRequestParamChanged(AuthenticationSessionModel authSession, String noteName, String requestParamValue) {
+        String authSessionNoteValue = authSession.getClientNote(noteName);
+        return !Objects.equals(authSessionNoteValue, requestParamValue);
     }
 
 

testsuite/integration/src/test/java/org/keycloak/testsuite/broker/IdentityProviderHintTest.java

@@ -95,6 +95,29 @@ public class IdentityProviderHintTest {
         assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8082/auth/"));
     }
 
+
+    // KEYCLOAK-5260
+    @Test
+    public void testSuccessfulRedirectToProviderAfterLoginPageShown() {
+        this.driver.navigate().to("http://localhost:8081/test-app");
+        String loginPageUrl = driver.getCurrentUrl();
+        assertTrue(loginPageUrl.startsWith("http://localhost:8081/auth/"));
+
+        // Manually add "kc_idp_hint" to URL . Should redirect to provider
+        loginPageUrl = loginPageUrl + "&kc_idp_hint=kc-oidc-idp-hidden";
+        this.driver.navigate().to(loginPageUrl);
+        assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8082/auth/"));
+
+        // Redirect from the app with the "kc_idp_hint". Should redirect to provider
+        this.driver.navigate().to("http://localhost:8081/test-app?kc_idp_hint=kc-oidc-idp-hidden");
+        assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8082/auth/"));
+
+        // Now redirect should't happen
+        this.driver.navigate().to("http://localhost:8081/test-app");
+        assertTrue(this.driver.getCurrentUrl().startsWith("http://localhost:8081/auth/"));
+    }
+
+
     @Test
     public void testInvalidIdentityProviderHint() {
         this.driver.navigate().to("http://localhost:8081/test-app?kc_idp_hint=invalid-idp-id");