From 4dd28c0adf322f6a42e87e9e490b902832324b76 Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 8 Jul 2016 11:04:08 +0200 Subject: [PATCH 1/2] KEYCLOAK-3221 Tokens should be invalidated if an attempt to reuse code is made --- .../keycloak/protocol/oidc/TokenManager.java | 5 ++ .../oidc/endpoints/TokenEndpoint.java | 14 ++++ .../org/keycloak/services/ServicesLogger.java | 4 ++ .../testsuite/util/UserInfoClientUtil.java | 65 +++++++++++++++++++ .../testsuite/oauth/AccessTokenTest.java | 63 +++++++++++++----- .../keycloak/testsuite/oidc/UserInfoTest.java | 45 ++++--------- 6 files changed, 147 insertions(+), 49 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java diff --git a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java index 0950d4d5b9..0cb4bbfc7f 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java @@ -199,6 +199,11 @@ public class TokenManager { return false; } + ClientSessionModel clientSession = session.sessions().getClientSession(realm, token.getClientSession()); + if (clientSession == null) { + return false; + } + return true; } 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 f7d68dc0d5..d84cf57932 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 @@ -68,6 +68,9 @@ import java.util.Map; */ public class TokenEndpoint { + // Flag if code was already exchanged for token + private static final String CODE_EXCHANGED = "CODE_EXCHANGED"; + private static final ServicesLogger logger = ServicesLogger.ROOT_LOGGER; private MultivaluedMap formParams; private ClientModel client; @@ -215,12 +218,23 @@ public class TokenEndpoint { ClientSessionModel clientSession = accessCode.getClientSession(); event.detail(Details.CODE_ID, clientSession.getId()); + + String codeExchanged = clientSession.getNote(CODE_EXCHANGED); + if (codeExchanged != null && Boolean.parseBoolean(codeExchanged)) { + logger.codeUsedAlready(code); + session.sessions().removeClientSession(realm, clientSession); + + event.error(Errors.INVALID_CODE); + throw new ErrorResponseException("invalid_grant", "Code used already", Response.Status.BAD_REQUEST); + } + if (!accessCode.isValid(ClientSessionModel.Action.CODE_TO_TOKEN.name(), ClientSessionCode.ActionType.CLIENT)) { event.error(Errors.INVALID_CODE); throw new ErrorResponseException("invalid_grant", "Code is expired", Response.Status.BAD_REQUEST); } accessCode.setAction(null); + clientSession.setNote(CODE_EXCHANGED, "true"); UserSessionModel userSession = clientSession.getUserSession(); if (userSession == null) { diff --git a/services/src/main/java/org/keycloak/services/ServicesLogger.java b/services/src/main/java/org/keycloak/services/ServicesLogger.java index fb71a2e2ad..4536eefe0f 100644 --- a/services/src/main/java/org/keycloak/services/ServicesLogger.java +++ b/services/src/main/java/org/keycloak/services/ServicesLogger.java @@ -402,4 +402,8 @@ public interface ServicesLogger extends BasicLogger { @LogMessage(level = ERROR) @Message(id=90, value="Failed to close ProviderSession") void failedToCloseProviderSession(@Cause Throwable t); + + @LogMessage(level = WARN) + @Message(id=91, value="Attempt to re-use code '%s'") + void codeUsedAlready(String code); } diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java new file mode 100644 index 0000000000..3d6033d10e --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/UserInfoClientUtil.java @@ -0,0 +1,65 @@ +/* + * Copyright 2016 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.testsuite.util; + +import java.net.URI; + +import javax.ws.rs.client.Client; +import javax.ws.rs.client.WebTarget; +import javax.ws.rs.core.HttpHeaders; +import javax.ws.rs.core.Response; +import javax.ws.rs.core.UriBuilder; + +import org.junit.Assert; +import org.keycloak.protocol.oidc.OIDCLoginProtocolService; +import org.keycloak.representations.UserInfo; + +/** + * @author Marek Posolda + */ +public class UserInfoClientUtil { + + public static Response executeUserInfoRequest_getMethod(Client client, String accessToken) { + WebTarget userInfoTarget = getUserInfoWebTarget(client); + + return userInfoTarget.request() + .header(HttpHeaders.AUTHORIZATION, "bearer " + accessToken) + .get(); + } + + public static WebTarget getUserInfoWebTarget(Client client) { + UriBuilder builder = UriBuilder.fromUri(OAuthClient.AUTH_SERVER_ROOT); + UriBuilder uriBuilder = OIDCLoginProtocolService.tokenServiceBaseUrl(builder); + URI userInfoUri = uriBuilder.path(OIDCLoginProtocolService.class, "issueUserInfo").build("test"); + return client.target(userInfoUri); + } + + public static void testSuccessfulUserInfoResponse(Response response, String expectedUsername, String expectedEmail) { + Assert.assertEquals(Response.Status.OK.getStatusCode(), response.getStatus()); + + UserInfo userInfo = response.readEntity(UserInfo.class); + + response.close(); + + Assert.assertNotNull(userInfo); + Assert.assertNotNull(userInfo.getSubject()); + Assert.assertEquals(expectedEmail, userInfo.getEmail()); + Assert.assertEquals(expectedUsername, userInfo.getPreferredUsername()); + } + +} diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java index effeeadbd4..88d7f3f2fb 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java @@ -16,6 +16,8 @@ */ package org.keycloak.testsuite.oauth; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import org.apache.http.NameValuePair; import org.apache.http.client.entity.UrlEncodedFormEntity; import org.apache.http.client.methods.HttpPost; @@ -32,10 +34,8 @@ import org.keycloak.admin.client.resource.ClientTemplateResource; import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.admin.client.resource.UserResource; import org.keycloak.common.enums.SslRequired; -import org.keycloak.common.util.PemUtils; import org.keycloak.events.Details; import org.keycloak.events.Errors; -import org.keycloak.jose.jwk.JWKBuilder; import org.keycloak.jose.jws.JWSHeader; import org.keycloak.jose.jws.JWSInput; import org.keycloak.jose.jws.JWSInputException; @@ -62,6 +62,7 @@ import org.keycloak.testsuite.util.OAuthClient; import org.keycloak.testsuite.util.RealmManager; import org.keycloak.testsuite.util.RoleBuilder; import org.keycloak.testsuite.util.UserBuilder; +import org.keycloak.testsuite.util.UserInfoClientUtil; import org.keycloak.testsuite.util.UserManager; import org.keycloak.util.BasicAuthHelper; @@ -72,6 +73,8 @@ import javax.ws.rs.core.Form; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.Response; import javax.ws.rs.core.UriBuilder; + +import java.io.IOException; import java.net.URI; import java.util.Arrays; import java.util.LinkedList; @@ -320,7 +323,7 @@ public class AccessTokenTest extends AbstractKeycloakTest { } @Test - public void accessTokenCodeUsed() { + public void accessTokenCodeUsed() throws IOException { oauth.doLogin("test-user@localhost", "password"); EventRepresentation loginEvent = events.expectLogin().assertEvent(); @@ -331,23 +334,53 @@ public class AccessTokenTest extends AbstractKeycloakTest { String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); OAuthClient.AccessTokenResponse response = oauth.doAccessTokenRequest(code, "password"); Assert.assertEquals(200, response.getStatusCode()); + String accessToken = response.getAccessToken(); - events.clear(); + Client jaxrsClient = javax.ws.rs.client.ClientBuilder.newClient(); + try { + // Check that userInfo can be invoked + Response userInfoResponse = UserInfoClientUtil.executeUserInfoRequest_getMethod(jaxrsClient, accessToken); + UserInfoClientUtil.testSuccessfulUserInfoResponse(userInfoResponse, "test-user@localhost", "test-user@localhost"); - response = oauth.doAccessTokenRequest(code, "password"); - Assert.assertEquals(400, response.getStatusCode()); + // Check that tokenIntrospection can be invoked + String introspectionResponse = oauth.introspectAccessTokenWithClientCredential("test-app", "password", accessToken); + ObjectMapper objectMapper = new ObjectMapper(); + JsonNode jsonNode = objectMapper.readTree(introspectionResponse); + Assert.assertEquals(true, jsonNode.get("active").asBoolean()); + Assert.assertEquals("test-user@localhost", jsonNode.get("email").asText()); - AssertEvents.ExpectedEvent expectedEvent = events.expectCodeToToken(codeId, null); - expectedEvent.error("invalid_code") - .removeDetail(Details.TOKEN_ID) - .removeDetail(Details.REFRESH_TOKEN_ID) - .removeDetail(Details.REFRESH_TOKEN_TYPE) - .user((String) null); - expectedEvent.assertEvent(); + events.clear(); - events.clear(); + // Repeating attempt to exchange code should be refused and invalidate previous clientSession + response = oauth.doAccessTokenRequest(code, "password"); + Assert.assertEquals(400, response.getStatusCode()); - RealmManager.realm(adminClient.realm("test")).accessCodeLifeSpan(60); + AssertEvents.ExpectedEvent expectedEvent = events.expectCodeToToken(codeId, null); + expectedEvent.error("invalid_code") + .removeDetail(Details.TOKEN_ID) + .removeDetail(Details.REFRESH_TOKEN_ID) + .removeDetail(Details.REFRESH_TOKEN_TYPE) + .user((String) null); + expectedEvent.assertEvent(); + + // Check that userInfo can't be invoked with invalidated accessToken + userInfoResponse = UserInfoClientUtil.executeUserInfoRequest_getMethod(jaxrsClient, accessToken); + assertEquals(Response.Status.FORBIDDEN.getStatusCode(), userInfoResponse.getStatus()); + userInfoResponse.close(); + + // Check that tokenIntrospection can't be invoked with invalidated accessToken + introspectionResponse = oauth.introspectAccessTokenWithClientCredential("test-app", "password", accessToken); + objectMapper = new ObjectMapper(); + jsonNode = objectMapper.readTree(introspectionResponse); + Assert.assertEquals(false, jsonNode.get("active").asBoolean()); + Assert.assertNull(jsonNode.get("email")); + + events.clear(); + + RealmManager.realm(adminClient.realm("test")).accessCodeLifeSpan(60); + } finally { + jaxrsClient.close(); + } } @Test diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java index 7e91d96b56..994ce3ae6b 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java @@ -28,6 +28,7 @@ import org.keycloak.testsuite.AbstractKeycloakTest; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.util.ClientManager; import org.keycloak.testsuite.util.RealmBuilder; +import org.keycloak.testsuite.util.UserInfoClientUtil; import org.keycloak.util.BasicAuthHelper; import javax.ws.rs.client.Client; @@ -75,12 +76,12 @@ public class UserInfoTest extends AbstractKeycloakTest { } @Test - public void testSuccess_getMethod_bearer() throws Exception { + public void testSuccess_getMethod_header() throws Exception { Client client = ClientBuilder.newClient(); try { AccessTokenResponse accessTokenResponse = executeGrantAccessTokenRequest(client); - Response response = executeUserInfoRequest_getMethod(client, accessTokenResponse.getToken()); + Response response = UserInfoClientUtil.executeUserInfoRequest_getMethod(client, accessTokenResponse.getToken()); testSuccessfulUserInfoResponse(response); @@ -90,13 +91,13 @@ public class UserInfoTest extends AbstractKeycloakTest { } @Test - public void testSuccess_postMethod_bearer() throws Exception { + public void testSuccess_postMethod_header() throws Exception { Client client = ClientBuilder.newClient(); try { AccessTokenResponse accessTokenResponse = executeGrantAccessTokenRequest(client); - WebTarget userInfoTarget = getUserInfoWebTarget(client); + WebTarget userInfoTarget = UserInfoClientUtil.getUserInfoWebTarget(client); Response response = userInfoTarget.request() .header(HttpHeaders.AUTHORIZATION, "bearer " + accessTokenResponse.getToken()) .post(Entity.form(new Form())); @@ -118,7 +119,7 @@ public class UserInfoTest extends AbstractKeycloakTest { Form form = new Form(); form.param("access_token", accessTokenResponse.getToken()); - WebTarget userInfoTarget = getUserInfoWebTarget(client); + WebTarget userInfoTarget = UserInfoClientUtil.getUserInfoWebTarget(client); Response response = userInfoTarget.request() .post(Entity.form(form)); @@ -130,13 +131,13 @@ public class UserInfoTest extends AbstractKeycloakTest { } @Test - public void testSuccess_postMethod_bearer_textEntity() throws Exception { + public void testSuccess_postMethod_header_textEntity() throws Exception { Client client = ClientBuilder.newClient(); try { AccessTokenResponse accessTokenResponse = executeGrantAccessTokenRequest(client); - WebTarget userInfoTarget = getUserInfoWebTarget(client); + WebTarget userInfoTarget = UserInfoClientUtil.getUserInfoWebTarget(client); Response response = userInfoTarget.request() .header(HttpHeaders.AUTHORIZATION, "bearer " + accessTokenResponse.getToken()) .post(Entity.text("")); @@ -157,7 +158,7 @@ public class UserInfoTest extends AbstractKeycloakTest { testingClient.testing().removeUserSessions("test"); - Response response = executeUserInfoRequest_getMethod(client, accessTokenResponse.getToken()); + Response response = UserInfoClientUtil.executeUserInfoRequest_getMethod(client, accessTokenResponse.getToken()); assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); @@ -173,7 +174,7 @@ public class UserInfoTest extends AbstractKeycloakTest { Client client = ClientBuilder.newClient(); try { - Response response = executeUserInfoRequest_getMethod(client, "bad"); + Response response = UserInfoClientUtil.executeUserInfoRequest_getMethod(client, "bad"); response.close(); @@ -208,31 +209,7 @@ public class UserInfoTest extends AbstractKeycloakTest { return accessTokenResponse; } - private Response executeUserInfoRequest_getMethod(Client client, String accessToken) { - WebTarget userInfoTarget = getUserInfoWebTarget(client); - - return userInfoTarget.request() - .header(HttpHeaders.AUTHORIZATION, "bearer " + accessToken) - .get(); - } - - private WebTarget getUserInfoWebTarget(Client client) { - UriBuilder builder = UriBuilder.fromUri(AUTH_SERVER_ROOT); - UriBuilder uriBuilder = OIDCLoginProtocolService.tokenServiceBaseUrl(builder); - URI userInfoUri = uriBuilder.path(OIDCLoginProtocolService.class, "issueUserInfo").build("test"); - return client.target(userInfoUri); - } - private void testSuccessfulUserInfoResponse(Response response) { - assertEquals(Status.OK.getStatusCode(), response.getStatus()); - - UserInfo userInfo = response.readEntity(UserInfo.class); - - response.close(); - - assertNotNull(userInfo); - assertNotNull(userInfo.getSubject()); - assertEquals("test-user@localhost", userInfo.getEmail()); - assertEquals("test-user@localhost", userInfo.getPreferredUsername()); + UserInfoClientUtil.testSuccessfulUserInfoResponse(response, "test-user@localhost", "test-user@localhost"); } } From c10a005997b4c6d975ee6f0c2af60ed42bae9c83 Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 8 Jul 2016 12:15:07 +0200 Subject: [PATCH 2/2] KEYCLOAK-3290 UserInfoEndpoint error responses don't have correct statuses --- .../org/keycloak/OAuthErrorException.java | 2 + .../main/java/org/keycloak/events/Errors.java | 1 + .../oidc/endpoints/TokenEndpoint.java | 48 ++++++++--------- .../oidc/endpoints/UserInfoEndpoint.java | 53 +++++++++++++++---- .../org/keycloak/services/ServicesLogger.java | 4 -- .../managers/AuthenticationManager.java | 2 +- .../testsuite/oauth/AccessTokenTest.java | 2 +- .../keycloak/testsuite/oidc/UserInfoTest.java | 32 +++++++++-- 8 files changed, 98 insertions(+), 46 deletions(-) diff --git a/core/src/main/java/org/keycloak/OAuthErrorException.java b/core/src/main/java/org/keycloak/OAuthErrorException.java index c5f0173fd4..710dbf1181 100755 --- a/core/src/main/java/org/keycloak/OAuthErrorException.java +++ b/core/src/main/java/org/keycloak/OAuthErrorException.java @@ -28,6 +28,8 @@ public class OAuthErrorException extends Exception { public static final String INVALID_SCOPE = "invalid_grant"; public static final String UNAUTHORIZED_CLIENT = "unauthorized_client"; public static final String UNSUPPORTED_GRANT_TYPE = "unsupported_grant_type"; + public static final String INVALID_TOKEN = "invalid_token"; + public static final String INSUFFICIENT_SCOPE = "insufficient_scope"; public OAuthErrorException(String error, String description, String message, Throwable cause) { super(message, cause); diff --git a/server-spi/src/main/java/org/keycloak/events/Errors.java b/server-spi/src/main/java/org/keycloak/events/Errors.java index 2d6438fd2d..0981806a83 100755 --- a/server-spi/src/main/java/org/keycloak/events/Errors.java +++ b/server-spi/src/main/java/org/keycloak/events/Errors.java @@ -65,6 +65,7 @@ public interface Errors { String SSL_REQUIRED = "ssl_required"; String USER_SESSION_NOT_FOUND = "user_session_not_found"; + String SESSION_EXPIRED = "session_expired"; String EMAIL_SEND_FAILED = "email_send_failed"; String INVALID_EMAIL = "invalid_email"; 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 d84cf57932..a79e67980e 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 @@ -152,7 +152,7 @@ public class TokenEndpoint { private void checkSsl() { if (!uriInfo.getBaseUri().getScheme().equals("https") && realm.getSslRequired().isRequired(clientConnection)) { - throw new ErrorResponseException("invalid_request", "HTTPS required", Response.Status.FORBIDDEN); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "HTTPS required", Response.Status.FORBIDDEN); } } @@ -168,13 +168,13 @@ public class TokenEndpoint { clientAuthAttributes = clientAuth.getClientAuthAttributes(); if (client.isBearerOnly()) { - throw new ErrorResponseException("invalid_client", "Bearer-only not allowed", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_CLIENT, "Bearer-only not allowed", Response.Status.BAD_REQUEST); } } private void checkGrantType() { if (grantType == null) { - throw new ErrorResponseException("invalid_request", "Missing form parameter: " + OIDCLoginProtocol.GRANT_TYPE_PARAM, Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "Missing form parameter: " + OIDCLoginProtocol.GRANT_TYPE_PARAM, Response.Status.BAD_REQUEST); } if (grantType.equals(OAuth2Constants.AUTHORIZATION_CODE)) { @@ -200,20 +200,17 @@ public class TokenEndpoint { String code = formParams.getFirst(OAuth2Constants.CODE); if (code == null) { event.error(Errors.INVALID_CODE); - throw new ErrorResponseException("invalid_request", "Missing parameter: " + OAuth2Constants.CODE, Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "Missing parameter: " + OAuth2Constants.CODE, Response.Status.BAD_REQUEST); } ClientSessionCode accessCode = ClientSessionCode.parse(code, session, realm); if (accessCode == null) { String[] parts = code.split("\\."); if (parts.length == 2) { - try { - event.detail(Details.CODE_ID, new String(parts[1])); - } catch (Throwable t) { - } + event.detail(Details.CODE_ID, parts[1]); } event.error(Errors.INVALID_CODE); - throw new ErrorResponseException("invalid_grant", "Code not found", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Code not found", Response.Status.BAD_REQUEST); } ClientSessionModel clientSession = accessCode.getClientSession(); @@ -221,16 +218,15 @@ public class TokenEndpoint { String codeExchanged = clientSession.getNote(CODE_EXCHANGED); if (codeExchanged != null && Boolean.parseBoolean(codeExchanged)) { - logger.codeUsedAlready(code); session.sessions().removeClientSession(realm, clientSession); event.error(Errors.INVALID_CODE); - throw new ErrorResponseException("invalid_grant", "Code used already", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Code used already", Response.Status.BAD_REQUEST); } if (!accessCode.isValid(ClientSessionModel.Action.CODE_TO_TOKEN.name(), ClientSessionCode.ActionType.CLIENT)) { event.error(Errors.INVALID_CODE); - throw new ErrorResponseException("invalid_grant", "Code is expired", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Code is expired", Response.Status.BAD_REQUEST); } accessCode.setAction(null); @@ -239,17 +235,17 @@ public class TokenEndpoint { if (userSession == null) { event.error(Errors.USER_SESSION_NOT_FOUND); - throw new ErrorResponseException("invalid_grant", "User session not found", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "User session not found", Response.Status.BAD_REQUEST); } UserModel user = userSession.getUser(); if (user == null) { event.error(Errors.USER_NOT_FOUND); - throw new ErrorResponseException("invalid_grant", "User not found", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "User not found", Response.Status.BAD_REQUEST); } if (!user.isEnabled()) { event.error(Errors.USER_DISABLED); - throw new ErrorResponseException("invalid_grant", "User disabled", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "User disabled", Response.Status.BAD_REQUEST); } event.user(userSession.getUser()); @@ -258,22 +254,22 @@ public class TokenEndpoint { String redirectUri = clientSession.getNote(OIDCLoginProtocol.REDIRECT_URI_PARAM); if (redirectUri != null && !redirectUri.equals(formParams.getFirst(OAuth2Constants.REDIRECT_URI))) { event.error(Errors.INVALID_CODE); - throw new ErrorResponseException("invalid_grant", "Incorrect redirect_uri", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Incorrect redirect_uri", Response.Status.BAD_REQUEST); } if (!client.getClientId().equals(clientSession.getClient().getClientId())) { event.error(Errors.INVALID_CODE); - throw new ErrorResponseException("invalid_grant", "Auth error", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Auth error", Response.Status.BAD_REQUEST); } if (!client.isStandardFlowEnabled()) { event.error(Errors.NOT_ALLOWED); - throw new ErrorResponseException("invalid_grant", "Client not allowed to exchange code", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Client not allowed to exchange code", Response.Status.BAD_REQUEST); } if (!AuthenticationManager.isSessionValid(realm, userSession)) { event.error(Errors.USER_SESSION_NOT_FOUND); - throw new ErrorResponseException("invalid_grant", "Session not active", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Session not active", Response.Status.BAD_REQUEST); } updateClientSession(clientSession); @@ -368,12 +364,12 @@ public class TokenEndpoint { if (!client.isDirectAccessGrantsEnabled()) { event.error(Errors.NOT_ALLOWED); - throw new ErrorResponseException("invalid_grant", "Client not allowed for direct access grants", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Client not allowed for direct access grants", Response.Status.BAD_REQUEST); } if (client.isConsentRequired()) { event.error(Errors.CONSENT_DENIED); - throw new ErrorResponseException("invalid_client", "Client requires user consent", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_CLIENT, "Client requires user consent", Response.Status.BAD_REQUEST); } String scope = formParams.getFirst(OAuth2Constants.SCOPE); @@ -401,7 +397,7 @@ public class TokenEndpoint { UserModel user = clientSession.getAuthenticatedUser(); if (user.getRequiredActions() != null && user.getRequiredActions().size() > 0) { event.error(Errors.RESOLVE_REQUIRED_ACTIONS); - throw new ErrorResponseException("invalid_grant", "Account is not fully set up", Response.Status.BAD_REQUEST); + throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Account is not fully set up", Response.Status.BAD_REQUEST); } processor.attachSession(); @@ -423,15 +419,15 @@ public class TokenEndpoint { public Response buildClientCredentialsGrant() { if (client.isBearerOnly()) { event.error(Errors.INVALID_CLIENT); - throw new ErrorResponseException("unauthorized_client", "Bearer-only client not allowed to retrieve service account", Response.Status.UNAUTHORIZED); + throw new ErrorResponseException(OAuthErrorException.UNAUTHORIZED_CLIENT, "Bearer-only client not allowed to retrieve service account", Response.Status.UNAUTHORIZED); } if (client.isPublicClient()) { event.error(Errors.INVALID_CLIENT); - throw new ErrorResponseException("unauthorized_client", "Public client not allowed to retrieve service account", Response.Status.UNAUTHORIZED); + throw new ErrorResponseException(OAuthErrorException.UNAUTHORIZED_CLIENT, "Public client not allowed to retrieve service account", Response.Status.UNAUTHORIZED); } if (!client.isServiceAccountsEnabled()) { event.error(Errors.INVALID_CLIENT); - throw new ErrorResponseException("unauthorized_client", "Client not enabled to retrieve service account", Response.Status.UNAUTHORIZED); + throw new ErrorResponseException(OAuthErrorException.UNAUTHORIZED_CLIENT, "Client not enabled to retrieve service account", Response.Status.UNAUTHORIZED); } UserModel clientUser = session.users().getServiceAccount(client); @@ -449,7 +445,7 @@ public class TokenEndpoint { if (!clientUser.isEnabled()) { event.error(Errors.USER_DISABLED); - throw new ErrorResponseException("invalid_request", "User '" + clientUsername + "' disabled", Response.Status.UNAUTHORIZED); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "User '" + clientUsername + "' disabled", Response.Status.UNAUTHORIZED); } String scope = formParams.getFirst(OAuth2Constants.SCOPE); diff --git a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/UserInfoEndpoint.java b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/UserInfoEndpoint.java index ab60db1a16..06989cdf5a 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/endpoints/UserInfoEndpoint.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/endpoints/UserInfoEndpoint.java @@ -24,6 +24,7 @@ import org.keycloak.OAuthErrorException; import org.keycloak.RSATokenVerifier; import org.keycloak.common.VerificationException; import org.keycloak.events.Details; +import org.keycloak.events.Errors; import org.keycloak.events.EventBuilder; import org.keycloak.events.EventType; import org.keycloak.models.ClientModel; @@ -49,7 +50,6 @@ import javax.ws.rs.core.Context; import javax.ws.rs.core.HttpHeaders; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; -import javax.ws.rs.core.Response.Status; import javax.ws.rs.core.UriInfo; import java.util.HashMap; import java.util.Map; @@ -122,30 +122,61 @@ public class UserInfoEndpoint { .event(EventType.USER_INFO_REQUEST) .detail(Details.AUTH_METHOD, Details.VALIDATE_ACCESS_TOKEN); + if (tokenString == null) { + event.error(Errors.INVALID_TOKEN); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "Token not provided", Response.Status.BAD_REQUEST); + } + AccessToken token = null; try { token = RSATokenVerifier.verifyToken(tokenString, realm.getPublicKey(), Urls.realmIssuer(uriInfo.getBaseUri(), realm.getName()), true, true); } catch (VerificationException e) { - throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Token invalid: " + e.getMessage(), Status.FORBIDDEN); + event.error(Errors.INVALID_TOKEN); + throw new ErrorResponseException(OAuthErrorException.INVALID_TOKEN, "Token invalid: " + e.getMessage(), Response.Status.UNAUTHORIZED); } UserSessionModel userSession = session.sessions().getUserSession(realm, token.getSessionState()); ClientSessionModel clientSession = session.sessions().getClientSession(token.getClientSession()); - if (userSession == null || clientSession == null || !AuthenticationManager.isSessionValid(realm, userSession)) { - throw new ErrorResponseException(OAuthErrorException.INVALID_GRANT, "Token invalid", Status.FORBIDDEN); + + if (userSession == null) { + event.error(Errors.USER_SESSION_NOT_FOUND); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "User session not found", Response.Status.BAD_REQUEST); + } + + event.session(userSession); + + UserModel userModel = userSession.getUser(); + if (userModel == null) { + event.error(Errors.USER_NOT_FOUND); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "User not found", Response.Status.BAD_REQUEST); + } + + event.user(userModel) + .detail(Details.USERNAME, userModel.getUsername()); + + + if (clientSession == null || !AuthenticationManager.isSessionValid(realm, userSession)) { + event.error(Errors.SESSION_EXPIRED); + throw new ErrorResponseException(OAuthErrorException.INVALID_TOKEN, "Session expired", Response.Status.UNAUTHORIZED); } ClientModel clientModel = realm.getClientByClientId(token.getIssuedFor()); - UserModel userModel = userSession.getUser(); + if (clientModel == null) { + event.error(Errors.CLIENT_NOT_FOUND); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "Client not found", Response.Status.BAD_REQUEST); + } + + event.client(clientModel); + + if (!clientModel.isEnabled()) { + event.error(Errors.CLIENT_DISABLED); + throw new ErrorResponseException(OAuthErrorException.INVALID_REQUEST, "Client disabled", Response.Status.BAD_REQUEST); + } + AccessToken userInfo = new AccessToken(); tokenManager.transformAccessToken(session, userInfo, realm, clientModel, userModel, userSession, clientSession); - event - .detail(Details.USERNAME, userModel.getUsername()) - .client(clientModel) - .session(userSession) - .user(userModel) - .success(); + event.success(); Map claims = new HashMap(); claims.putAll(userInfo.getOtherClaims()); diff --git a/services/src/main/java/org/keycloak/services/ServicesLogger.java b/services/src/main/java/org/keycloak/services/ServicesLogger.java index 4536eefe0f..fb71a2e2ad 100644 --- a/services/src/main/java/org/keycloak/services/ServicesLogger.java +++ b/services/src/main/java/org/keycloak/services/ServicesLogger.java @@ -402,8 +402,4 @@ public interface ServicesLogger extends BasicLogger { @LogMessage(level = ERROR) @Message(id=90, value="Failed to close ProviderSession") void failedToCloseProviderSession(@Cause Throwable t); - - @LogMessage(level = WARN) - @Message(id=91, value="Attempt to re-use code '%s'") - void codeUsedAlready(String code); } diff --git a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java index e21062609d..4e6901f313 100755 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java @@ -84,7 +84,7 @@ public class AuthenticationManager { } int currentTime = Time.currentTime(); int max = userSession.getStarted() + realm.getSsoSessionMaxLifespan(); - return userSession != null && userSession.getLastSessionRefresh() + realm.getSsoSessionIdleTimeout() > currentTime && max > currentTime; + return userSession.getLastSessionRefresh() + realm.getSsoSessionIdleTimeout() > currentTime && max > currentTime; } public static void expireUserSessionCookie(KeycloakSession session, UserSessionModel userSession, RealmModel realm, UriInfo uriInfo, HttpHeaders headers, ClientConnection connection) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java index 88d7f3f2fb..dfade959ed 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java @@ -365,7 +365,7 @@ public class AccessTokenTest extends AbstractKeycloakTest { // Check that userInfo can't be invoked with invalidated accessToken userInfoResponse = UserInfoClientUtil.executeUserInfoRequest_getMethod(jaxrsClient, accessToken); - assertEquals(Response.Status.FORBIDDEN.getStatusCode(), userInfoResponse.getStatus()); + assertEquals(Response.Status.UNAUTHORIZED.getStatusCode(), userInfoResponse.getStatus()); userInfoResponse.close(); // Check that tokenIntrospection can't be invoked with invalidated accessToken diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java index 994ce3ae6b..268333f0e5 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oidc/UserInfoTest.java @@ -16,10 +16,14 @@ */ package org.keycloak.testsuite.oidc; +import org.hamcrest.Matchers; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.keycloak.OAuth2Constants; +import org.keycloak.events.Details; +import org.keycloak.events.Errors; +import org.keycloak.events.EventType; import org.keycloak.protocol.oidc.OIDCLoginProtocolService; import org.keycloak.representations.AccessTokenResponse; import org.keycloak.representations.UserInfo; @@ -44,7 +48,6 @@ import java.net.URI; import java.util.List; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertNotNull; import static org.keycloak.testsuite.admin.AbstractAdminTest.loadJson; import static org.keycloak.testsuite.util.OAuthClient.AUTH_SERVER_ROOT; @@ -160,10 +163,18 @@ public class UserInfoTest extends AbstractKeycloakTest { Response response = UserInfoClientUtil.executeUserInfoRequest_getMethod(client, accessTokenResponse.getToken()); - assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); + assertEquals(Status.BAD_REQUEST.getStatusCode(), response.getStatus()); response.close(); + events.expect(EventType.USER_INFO_REQUEST_ERROR) + .error(Errors.USER_SESSION_NOT_FOUND) + .client((String) null) + .user(Matchers.nullValue(String.class)) + .session(Matchers.nullValue(String.class)) + .detail(Details.AUTH_METHOD, Details.VALIDATE_ACCESS_TOKEN) + .assertEvent(); + } finally { client.close(); } @@ -178,7 +189,15 @@ public class UserInfoTest extends AbstractKeycloakTest { response.close(); - assertEquals(Status.FORBIDDEN.getStatusCode(), response.getStatus()); + assertEquals(Status.UNAUTHORIZED.getStatusCode(), response.getStatus()); + + events.expect(EventType.USER_INFO_REQUEST_ERROR) + .error(Errors.INVALID_TOKEN) + .client((String) null) + .user(Matchers.nullValue(String.class)) + .session(Matchers.nullValue(String.class)) + .detail(Details.AUTH_METHOD, Details.VALIDATE_ACCESS_TOKEN) + .assertEvent(); } finally { client.close(); @@ -206,10 +225,17 @@ public class UserInfoTest extends AbstractKeycloakTest { response.close(); + events.clear(); + return accessTokenResponse; } private void testSuccessfulUserInfoResponse(Response response) { + events.expect(EventType.USER_INFO_REQUEST) + .session(Matchers.notNullValue(String.class)) + .detail(Details.AUTH_METHOD, Details.VALIDATE_ACCESS_TOKEN) + .detail(Details.USERNAME, "test-user@localhost") + .assertEvent(); UserInfoClientUtil.testSuccessfulUserInfoResponse(response, "test-user@localhost", "test-user@localhost"); } }