From ff6fcd30d9d4c8ed99dcee4667d485ffaa3ae4e0 Mon Sep 17 00:00:00 2001 From: mposolda Date: Mon, 4 Dec 2017 12:26:46 +0100 Subject: [PATCH] KEYCLOAK-4478 OIDC auth response lacks session_state in some cases --- .../adapters/OAuthRequestAuthenticator.java | 3 ++- .../src/main/java/org/keycloak/OAuth2Constants.java | 3 +++ .../keycloak/protocol/oidc/OIDCLoginProtocol.java | 3 ++- .../protocol/oidc/endpoints/TokenEndpoint.java | 13 +++++++++++-- .../org/keycloak/testsuite/util/OAuthClient.java | 7 +++++++ .../oidc/flows/AbstractOIDCResponseTypeTest.java | 13 ++++++++++--- .../oidc/flows/OIDCBasicResponseTypeCodeTest.java | 12 +++++++++--- .../OIDCHybridResponseTypeCodeIDTokenTest.java | 9 +++++++-- .../OIDCHybridResponseTypeCodeIDTokenTokenTest.java | 9 +++++++-- .../flows/OIDCHybridResponseTypeCodeTokenTest.java | 9 +++++++-- .../flows/OIDCImplicitResponseTypeIDTokenTest.java | 9 +++++++-- .../OIDCImplicitResponseTypeIDTokenTokenTest.java | 9 +++++++-- 12 files changed, 79 insertions(+), 20 deletions(-) diff --git a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java index e7c9071e8fa..2538edb7cb5 100755 --- a/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java +++ b/adapters/oidc/adapter-core/src/main/java/org/keycloak/adapters/OAuthRequestAuthenticator.java @@ -390,7 +390,8 @@ public class OAuthRequestAuthenticator { protected String stripOauthParametersFromRedirect() { KeycloakUriBuilder builder = KeycloakUriBuilder.fromUri(facade.getRequest().getURI()) .replaceQueryParam(OAuth2Constants.CODE, null) - .replaceQueryParam(OAuth2Constants.STATE, null); + .replaceQueryParam(OAuth2Constants.STATE, null) + .replaceQueryParam(OAuth2Constants.SESSION_STATE, null); return builder.build().toString(); } diff --git a/core/src/main/java/org/keycloak/OAuth2Constants.java b/core/src/main/java/org/keycloak/OAuth2Constants.java index d0b445cf3b2..59e0eeed4ea 100644 --- a/core/src/main/java/org/keycloak/OAuth2Constants.java +++ b/core/src/main/java/org/keycloak/OAuth2Constants.java @@ -83,6 +83,9 @@ public interface OAuth2Constants { String MAX_AGE = "max_age"; + // OIDC Session Management + String SESSION_STATE = "session_state"; + String JWT = "JWT"; // https://tools.ietf.org/html/rfc7636#section-6.1 diff --git a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java index 22d5313d21d..eb1aede8324 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/OIDCLoginProtocol.java @@ -183,6 +183,8 @@ public class OIDCLoginProtocol implements LoginProtocol { if (state != null) redirectUri.addParam(OAuth2Constants.STATE, state); + redirectUri.addParam(OAuth2Constants.SESSION_STATE, userSession.getId()); + // Standard or hybrid flow String code = null; if (responseType.hasResponseType(OIDCResponseType.CODE)) { @@ -219,7 +221,6 @@ public class OIDCLoginProtocol implements LoginProtocol { if (responseType.hasResponseType(OIDCResponseType.TOKEN)) { redirectUri.addParam(OAuth2Constants.ACCESS_TOKEN, res.getToken()); redirectUri.addParam("token_type", res.getTokenType()); - redirectUri.addParam("session_state", res.getSessionState()); redirectUri.addParam("expires_in", String.valueOf(res.getExpiresIn())); } diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java index 0e8d36e8815..1826edf6102 100644 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/TokenEndpoint.java @@ -35,6 +35,7 @@ import org.keycloak.common.ClientConnection; import org.keycloak.common.Profile; import org.keycloak.common.constants.ServiceAccountConstants; import org.keycloak.common.util.Base64Url; +import org.keycloak.common.util.KeycloakUriBuilder; import org.keycloak.constants.AdapterConstants; import org.keycloak.events.Details; import org.keycloak.events.Errors; @@ -299,8 +300,16 @@ public class TokenEndpoint { } String redirectUri = clientSession.getNote(OIDCLoginProtocol.REDIRECT_URI_PARAM); - String formParam = formParams.getFirst(OAuth2Constants.REDIRECT_URI); - if (redirectUri != null && !redirectUri.equals(formParam)) { + String redirectUriParam = formParams.getFirst(OAuth2Constants.REDIRECT_URI); + + // KEYCLOAK-4478 Backwards compatibility with the adapters earlier than KC 3.4.2 + if (redirectUriParam.contains("session_state=")) { + redirectUriParam = KeycloakUriBuilder.fromUri(redirectUriParam) + .replaceQueryParam(OAuth2Constants.SESSION_STATE, null) + .build().toString(); + } + + if (redirectUri != null && !redirectUri.equals(redirectUriParam)) { event.error(Errors.INVALID_CODE); throw new CorsErrorResponseException(cors, OAuthErrorException.INVALID_GRANT, "Incorrect redirect_uri", Response.Status.BAD_REQUEST); } diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java index 8794b1e83f0..29fb765c565 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/OAuthClient.java @@ -894,6 +894,8 @@ public class OAuthClient { private String error; private String errorDescription; + private String sessionState; + // Just during OIDC implicit or hybrid flow private String accessToken; private String idToken; @@ -920,6 +922,7 @@ public class OAuthClient { state = params.get(OAuth2Constants.STATE); error = params.get(OAuth2Constants.ERROR); errorDescription = params.get(OAuth2Constants.ERROR_DESCRIPTION); + sessionState = params.get(OAuth2Constants.SESSION_STATE); accessToken = params.get(OAuth2Constants.ACCESS_TOKEN); idToken = params.get(OAuth2Constants.ID_TOKEN); } @@ -944,6 +947,10 @@ public class OAuthClient { return errorDescription; } + public String getSessionState() { + return sessionState; + } + public String getAccessToken() { return accessToken; } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/AbstractOIDCResponseTypeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/AbstractOIDCResponseTypeTest.java index 103f8c94026..1794359f5ad 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/AbstractOIDCResponseTypeTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/AbstractOIDCResponseTypeTest.java @@ -75,12 +75,17 @@ public abstract class AbstractOIDCResponseTypeTest extends AbstractTestRealmKeyc @Test - public void nonceMatches() { + public void nonceAndSessionStateMatches() { EventRepresentation loginEvent = loginUser("abcdef123456"); - List idTokens = retrieveIDTokens(loginEvent); + + OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, isFragment()); + Assert.assertNotNull(authzResponse.getSessionState()); + + List idTokens = testAuthzResponseAndRetrieveIDTokens(authzResponse, loginEvent); for (IDToken idToken : idTokens) { Assert.assertEquals("abcdef123456", idToken.getNonce()); + Assert.assertEquals(authzResponse.getSessionState(), idToken.getSessionState()); } } @@ -169,7 +174,9 @@ public abstract class AbstractOIDCResponseTypeTest extends AbstractTestRealmKeyc return events.expectLogin().detail(Details.USERNAME, "test-user@localhost").assertEvent(); } - protected abstract List retrieveIDTokens(EventRepresentation loginEvent); + protected abstract boolean isFragment(); + + protected abstract List testAuthzResponseAndRetrieveIDTokens(OAuthClient.AuthorizationEndpointResponse authzResponse, EventRepresentation loginEvent); protected ClientManager.ClientManagerBuilder clientManagerBuilder() { return ClientManager.realm(adminClient.realm("test")).clientId("test-app"); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCBasicResponseTypeCodeTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCBasicResponseTypeCodeTest.java index 73e9cb847d0..d4b4f9e3a1a 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCBasicResponseTypeCodeTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCBasicResponseTypeCodeTest.java @@ -45,10 +45,15 @@ public class OIDCBasicResponseTypeCodeTest extends AbstractOIDCResponseTypeTest } - protected List retrieveIDTokens(EventRepresentation loginEvent) { + @Override + protected boolean isFragment() { + return false; + } + + @Override + protected List testAuthzResponseAndRetrieveIDTokens(OAuthClient.AuthorizationEndpointResponse authzResponse, EventRepresentation loginEvent) { Assert.assertEquals(OIDCResponseType.CODE, loginEvent.getDetails().get(Details.RESPONSE_TYPE)); - OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, false); Assert.assertNull(authzResponse.getAccessToken()); Assert.assertNull(authzResponse.getIdToken()); @@ -62,7 +67,8 @@ public class OIDCBasicResponseTypeCodeTest extends AbstractOIDCResponseTypeTest public void nonceNotUsed() { EventRepresentation loginEvent = loginUser(null); - List idTokens = retrieveIDTokens(loginEvent); + OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, false); + List idTokens = testAuthzResponseAndRetrieveIDTokens(authzResponse, loginEvent); for (IDToken idToken : idTokens) { Assert.assertNull(idToken.getNonce()); } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTest.java index 3fca3b25dcd..3dfa580360d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTest.java @@ -46,11 +46,16 @@ public class OIDCHybridResponseTypeCodeIDTokenTest extends AbstractOIDCResponseT } - protected List retrieveIDTokens(EventRepresentation loginEvent) { + @Override + protected boolean isFragment() { + return true; + } + + + protected List testAuthzResponseAndRetrieveIDTokens(OAuthClient.AuthorizationEndpointResponse authzResponse, EventRepresentation loginEvent) { Assert.assertEquals(OIDCResponseType.CODE + " " + OIDCResponseType.ID_TOKEN, loginEvent.getDetails().get(Details.RESPONSE_TYPE)); // IDToken from the authorization response - OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, true); Assert.assertNull(authzResponse.getAccessToken()); String idTokenStr = authzResponse.getIdToken(); IDToken idToken = oauth.verifyIDToken(idTokenStr); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTokenTest.java index 63a9f7885bc..132195d5a6d 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeIDTokenTokenTest.java @@ -46,11 +46,16 @@ public class OIDCHybridResponseTypeCodeIDTokenTokenTest extends AbstractOIDCResp } - protected List retrieveIDTokens(EventRepresentation loginEvent) { + @Override + protected boolean isFragment() { + return true; + } + + + protected List testAuthzResponseAndRetrieveIDTokens(OAuthClient.AuthorizationEndpointResponse authzResponse, EventRepresentation loginEvent) { Assert.assertEquals(OIDCResponseType.CODE + " " + OIDCResponseType.ID_TOKEN + " " + OIDCResponseType.TOKEN, loginEvent.getDetails().get(Details.RESPONSE_TYPE)); // IDToken from the authorization response - OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, true); Assert.assertNotNull(authzResponse.getAccessToken()); String idTokenStr = authzResponse.getIdToken(); IDToken idToken = oauth.verifyIDToken(idTokenStr); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeTokenTest.java index 4b4ba7e336e..13e43d265ae 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCHybridResponseTypeCodeTokenTest.java @@ -45,10 +45,15 @@ public class OIDCHybridResponseTypeCodeTokenTest extends AbstractOIDCResponseTyp } - protected List retrieveIDTokens(EventRepresentation loginEvent) { + @Override + protected boolean isFragment() { + return true; + } + + + protected List testAuthzResponseAndRetrieveIDTokens(OAuthClient.AuthorizationEndpointResponse authzResponse, EventRepresentation loginEvent) { Assert.assertEquals(OIDCResponseType.CODE + " " + OIDCResponseType.TOKEN, loginEvent.getDetails().get(Details.RESPONSE_TYPE)); - OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, true); Assert.assertNotNull(authzResponse.getAccessToken()); Assert.assertNull(authzResponse.getIdToken()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTest.java index 103c91f3880..d81ff5614d1 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTest.java @@ -45,10 +45,15 @@ public class OIDCImplicitResponseTypeIDTokenTest extends AbstractOIDCResponseTyp } - protected List retrieveIDTokens(EventRepresentation loginEvent) { + @Override + protected boolean isFragment() { + return true; + } + + + protected List testAuthzResponseAndRetrieveIDTokens(OAuthClient.AuthorizationEndpointResponse authzResponse, EventRepresentation loginEvent) { Assert.assertEquals(OIDCResponseType.ID_TOKEN, loginEvent.getDetails().get(Details.RESPONSE_TYPE)); - OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, true); Assert.assertNull(authzResponse.getAccessToken()); String idTokenStr = authzResponse.getIdToken(); IDToken idToken = oauth.verifyIDToken(idTokenStr); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTokenTest.java index 578b1a7e074..cd45908a803 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/flows/OIDCImplicitResponseTypeIDTokenTokenTest.java @@ -46,10 +46,15 @@ public class OIDCImplicitResponseTypeIDTokenTokenTest extends AbstractOIDCRespon } - protected List retrieveIDTokens(EventRepresentation loginEvent) { + @Override + protected boolean isFragment() { + return true; + } + + + protected List testAuthzResponseAndRetrieveIDTokens(OAuthClient.AuthorizationEndpointResponse authzResponse, EventRepresentation loginEvent) { Assert.assertEquals(OIDCResponseType.ID_TOKEN + " " + OIDCResponseType.TOKEN, loginEvent.getDetails().get(Details.RESPONSE_TYPE)); - OAuthClient.AuthorizationEndpointResponse authzResponse = new OAuthClient.AuthorizationEndpointResponse(oauth, true); Assert.assertNotNull(authzResponse.getAccessToken()); String idTokenStr = authzResponse.getIdToken(); IDToken idToken = oauth.verifyIDToken(idTokenStr);