From a6a6a62dc03cb07e08b28d72dcfbad83f866e614 Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 18 Aug 2017 11:08:51 +0200 Subject: [PATCH] KEYCLOAK-5260 kc_idp_hint was only working first time --- .../protocol/AuthorizationEndpointBase.java | 12 ++++- .../oidc/endpoints/AuthorizationEndpoint.java | 45 ++++++++++++++++++- .../broker/IdentityProviderHintTest.java | 23 ++++++++++ 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/services/src/main/java/org/keycloak/protocol/AuthorizationEndpointBase.java b/services/src/main/java/org/keycloak/protocol/AuthorizationEndpointBase.java index c06caa0737..b854e52cd9 100755 --- a/services/src/main/java/org/keycloak/protocol/AuthorizationEndpointBase.java +++ b/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)) { diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java index 38dbc8f5e4..f00c89237d 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/AuthorizationEndpoint.java +++ b/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); } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/IdentityProviderHintTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/IdentityProviderHintTest.java index 05d1afa116..a58b82f257 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/broker/IdentityProviderHintTest.java +++ b/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");