diff --git a/services/src/main/java/org/keycloak/authorization/util/Tokens.java b/services/src/main/java/org/keycloak/authorization/util/Tokens.java index b17a01f710..9b04457210 100644 --- a/services/src/main/java/org/keycloak/authorization/util/Tokens.java +++ b/services/src/main/java/org/keycloak/authorization/util/Tokens.java @@ -18,7 +18,6 @@ package org.keycloak.authorization.util; -import org.keycloak.models.KeycloakContext; import org.keycloak.models.KeycloakSession; import org.keycloak.representations.AccessToken; import org.keycloak.services.managers.AppAuthManager; 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 84c05327ae..d719054b72 100755 --- a/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java +++ b/services/src/main/java/org/keycloak/protocol/oidc/TokenManager.java @@ -240,18 +240,13 @@ public class TokenManager { try { TokenVerifier.createWithoutSignature(token) - .withChecks(NotBeforeCheck.forModel(client), TokenVerifier.IS_ACTIVE) + .withChecks(NotBeforeCheck.forModel(client), TokenVerifier.IS_ACTIVE, new TokenRevocationCheck(session)) .verify(); } catch (VerificationException e) { logger.debugf("JWT check failed: %s", e.getMessage()); return false; } - TokenRevocationStoreProvider revocationStore = session.getProvider(TokenRevocationStoreProvider.class); - if (revocationStore.isRevoked(token.getId())) { - return false; - } - boolean valid = false; // Tokens without sessions are considered valid. Signature check and revocation check are sufficient checks for them @@ -1346,6 +1341,24 @@ public class TokenManager { } } + /** + * Check if access token was revoked with OAuth revocation endpoint + */ + public static class TokenRevocationCheck implements TokenVerifier.Predicate { + + private final KeycloakSession session; + + public TokenRevocationCheck(KeycloakSession session) { + this.session = session; + } + + @Override + public boolean test(AccessToken token) { + TokenRevocationStoreProvider revocationStore = session.getProvider(TokenRevocationStoreProvider.class); + return !revocationStore.isRevoked(token.getId()); + } + } + public LogoutTokenValidationCode verifyLogoutToken(KeycloakSession session, RealmModel realm, String encodedLogoutToken) { Optional logoutTokenOptional = toLogoutToken(encodedLogoutToken); if (!logoutTokenOptional.isPresent()) { 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 19705bba70..53efd32cb1 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 @@ -174,7 +174,7 @@ public class UserInfoEndpoint { cors.allowedOrigins(session, clientModel); TokenVerifier.createWithoutSignature(token) - .withChecks(NotBeforeCheck.forModel(clientModel)) + .withChecks(NotBeforeCheck.forModel(clientModel), new TokenManager.TokenRevocationCheck(session)) .verify(); } catch (VerificationException e) { if (clientModel == null) { 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 63ebeb5e94..556d1715c0 100755 --- a/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java +++ b/services/src/main/java/org/keycloak/services/managers/AuthenticationManager.java @@ -1395,6 +1395,11 @@ public class AuthenticationManager { verifier.audience(checkAudience); } + // Check token revocation in case of access token + if (checkTokenType) { + verifier.withChecks(new TokenManager.TokenRevocationCheck(session)); + } + String kid = verifier.getHeader().getKeyId(); String algorithm = verifier.getHeader().getAlgorithm().name(); @@ -1432,7 +1437,11 @@ public class AuthenticationManager { UserSessionModel offlineUserSession = session.sessions().getOfflineUserSession(realm, token.getSessionState()); if (isOfflineSessionValid(realm, offlineUserSession)) { user = offlineUserSession.getUser(); - return new AuthResult(user, offlineUserSession, token); + ClientModel client = realm.getClientByClientId(token.getIssuedFor()); + if (!isClientValid(offlineUserSession, client, token)) { + return null; + } + return new AuthResult(user, offlineUserSession, token, client); } } @@ -1443,13 +1452,45 @@ public class AuthenticationManager { session.setAttribute("state_checker", token.getOtherClaims().get("state_checker")); - return new AuthResult(user, userSession, token); + ClientModel client; + if (isCookie) { + client = null; + } else { + client = realm.getClientByClientId(token.getIssuedFor()); + if (!isClientValid(userSession, client, token)) { + return null; + } + } + return new AuthResult(user, userSession, token, client); } catch (VerificationException e) { logger.debugf("Failed to verify identity token: %s", e.getMessage()); } return null; } + // Verify client and whether clientSession exists + private static boolean isClientValid(UserSessionModel userSession, ClientModel client, AccessToken token) { + if (client == null || !client.isEnabled()) { + logger.debugf("Identity token issued for unknown or disabled client '%s'", token.getIssuedFor()); + return false; + } + + if (token.getIssuedAt() < client.getNotBefore()) { + logger.debug("Client notBefore newer than token"); + return false; + } + + // User session may not exists for example during client credentials auth + if (userSession == null) return true; + + AuthenticatedClientSessionModel clientSession = userSession.getAuthenticatedClientSessionByClient(client.getId()); + if (clientSession == null) { + logger.debugf("Client session for client '%s' not present in user session '%s'", client.getClientId(), userSession.getId()); + return false; + } + return true; + } + private static boolean isUserValid(KeycloakSession session, RealmModel realm, UserModel user, AccessToken token) { if (user == null || !user.isEnabled()) { logger.debug("Unknown user in identity token"); @@ -1473,11 +1514,13 @@ public class AuthenticationManager { private final UserModel user; private final UserSessionModel session; private final AccessToken token; + private final ClientModel client; - public AuthResult(UserModel user, UserSessionModel session, AccessToken token) { + public AuthResult(UserModel user, UserSessionModel session, AccessToken token, ClientModel client) { this.user = user; this.session = session; this.token = token; + this.client = client; } public UserSessionModel getSession() { @@ -1491,6 +1534,10 @@ public class AuthenticationManager { public AccessToken getToken() { return token; } + + public ClientModel getClient() { + return client; + } } public static void setKcActionStatus(String executedProviderId, RequiredActionContext.KcActionStatus status, AuthenticationSessionModel authSession) { 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 8915103a51..27e81cefde 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java @@ -451,15 +451,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal if (authResult != null) { AccessToken token = authResult.getToken(); - String issuedFor = token.getIssuedFor(); - ClientModel clientModel = this.realmModel.getClientByClientId(issuedFor); - - if (clientModel == null) { - return badRequest("Invalid client."); - } - if (!clientModel.isEnabled()) { - return badRequest("Client is disabled"); - } + ClientModel clientModel = authResult.getClient(); session.getContext().setClient(clientModel); diff --git a/services/src/main/java/org/keycloak/services/resources/admin/AdminRoot.java b/services/src/main/java/org/keycloak/services/resources/admin/AdminRoot.java index 412f346e4f..dcb88bc086 100755 --- a/services/src/main/java/org/keycloak/services/resources/admin/AdminRoot.java +++ b/services/src/main/java/org/keycloak/services/resources/admin/AdminRoot.java @@ -179,13 +179,7 @@ public class AdminRoot { throw new NotAuthorizedException("Bearer"); } - ClientModel client = realm.getClientByClientId(token.getIssuedFor()); - if (client == null) { - throw new NotFoundException("Could not find client for authorization"); - - } - - return new AdminAuth(realm, authResult.getToken(), authResult.getUser(), client); + return new AdminAuth(realm, authResult.getToken(), authResult.getUser(), authResult.getClient()); } public static UriBuilder realmsUrl(UriInfo uriInfo) { diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AdminClientTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AdminClientTest.java index 5889bd5bb4..6c3ba92bb1 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AdminClientTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AdminClientTest.java @@ -20,11 +20,14 @@ package org.keycloak.testsuite.admin; import java.util.List; +import javax.ws.rs.NotAuthorizedException; + import org.junit.Assert; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.keycloak.admin.client.Keycloak; +import org.keycloak.admin.client.resource.ClientResource; import org.keycloak.common.constants.ServiceAccountConstants; import org.keycloak.models.AdminRoles; import org.keycloak.models.Constants; @@ -88,7 +91,9 @@ public class AdminClientTest extends AbstractKeycloakTest { UserBuilder defaultUser = UserBuilder.create() .id(KeycloakModelUtils.generateId()) - .username("test-user@localhost"); + .username("test-user@localhost") + .password("password") + .role(Constants.REALM_MANAGEMENT_CLIENT_ID, AdminRoles.REALM_ADMIN); realm.user(defaultUser); testRealms.add(realm.build()); @@ -103,9 +108,60 @@ public class AdminClientTest extends AbstractKeycloakTest { setTimeOffset(1000); - // Check still possible to load the realm after token expired + // Check still possible to load the realm after original token expired (admin client should automatically re-authenticate) realm = adminClient.realm("test").toRepresentation(); Assert.assertEquals("test", realm.getRealm()); } } + + @Test + public void clientCredentialsClientDisabled() throws Exception { + try (Keycloak adminClient = AdminClientUtil.createAdminClientWithClientCredentials("test", "service-account-cl", "secret1")) { + // Check possible to load the realm + RealmRepresentation realm = adminClient.realm("test").toRepresentation(); + Assert.assertEquals("test", realm.getRealm()); + + // Disable client and check it should not be possible to load the realms anymore + setClientEnabled("service-account-cl", false); + + // Check not possible to invoke anymore + try { + realm = adminClient.realm("test").toRepresentation(); + Assert.fail("Not expected to successfully get realm"); + } catch (NotAuthorizedException nae) { + // Expected + } + } finally { + setClientEnabled("service-account-cl", true); + } + } + + @Test + public void adminAuthClientDisabled() throws Exception { + try (Keycloak adminClient = AdminClientUtil.createAdminClient(false, "test", "test-user@localhost", "password", Constants.ADMIN_CLI_CLIENT_ID, null)) { + // Check possible to load the realm + RealmRepresentation realm = adminClient.realm("test").toRepresentation(); + Assert.assertEquals("test", realm.getRealm()); + + // Disable client and check it should not be possible to load the realms anymore + setClientEnabled(Constants.ADMIN_CLI_CLIENT_ID, false); + + // Check not possible to invoke anymore + try { + realm = adminClient.realm("test").toRepresentation(); + Assert.fail("Not expected to successfully get realm"); + } catch (NotAuthorizedException nae) { + // Expected + } + } finally { + setClientEnabled(Constants.ADMIN_CLI_CLIENT_ID, true); + } + } + + private void setClientEnabled(String clientId, boolean enabled) { + ClientResource client = ApiUtil.findClientByClientId(adminClient.realms().realm("test"), clientId); + ClientRepresentation clientRep = client.toRepresentation(); + clientRep.setEnabled(enabled); + client.update(clientRep); + } } diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/TokenRevocationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/TokenRevocationTest.java index fe7ad3308f..e07130ac21 100644 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/TokenRevocationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/oauth/TokenRevocationTest.java @@ -30,10 +30,14 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import javax.ws.rs.NotAuthorizedException; +import javax.ws.rs.client.Client; +import javax.ws.rs.core.Response; import javax.ws.rs.core.Response.Status; import org.apache.commons.io.output.ByteArrayOutputStream; import org.apache.http.NameValuePair; +import org.apache.http.client.HttpClient; import org.apache.http.client.entity.UrlEncodedFormEntity; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpPost; @@ -41,25 +45,32 @@ import org.apache.http.impl.client.CloseableHttpClient; import org.apache.http.impl.client.HttpClientBuilder; import org.apache.http.message.BasicNameValuePair; import org.jboss.arquillian.graphene.page.Page; +import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.keycloak.OAuth2Constants; import org.keycloak.OAuthErrorException; +import org.keycloak.admin.client.Keycloak; import org.keycloak.admin.client.resource.RealmResource; import org.keycloak.admin.client.resource.UserResource; +import org.keycloak.broker.provider.util.SimpleHttp; +import org.keycloak.representations.account.UserRepresentation; import org.keycloak.representations.idm.OAuth2ErrorRepresentation; import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserSessionRepresentation; import org.keycloak.representations.oidc.TokenMetadataRepresentation; import org.keycloak.testsuite.AbstractKeycloakTest; +import org.keycloak.testsuite.Assert; import org.keycloak.testsuite.AssertEvents; import org.keycloak.testsuite.pages.LoginPage; +import org.keycloak.testsuite.util.AdminClientUtil; import org.keycloak.testsuite.util.ClientManager; import org.keycloak.testsuite.util.Matchers; import org.keycloak.testsuite.util.OAuthClient; import org.keycloak.testsuite.util.OAuthClient.AccessTokenResponse; import org.keycloak.testsuite.util.RealmBuilder; +import org.keycloak.testsuite.util.UserInfoClientUtil; import org.keycloak.util.JsonSerialization; /** @@ -69,6 +80,9 @@ public class TokenRevocationTest extends AbstractKeycloakTest { private RealmResource realm; + private Client userInfoClient; + private CloseableHttpClient restHttpClient; + @Rule public AssertEvents events = new AssertEvents(this); @@ -87,10 +101,21 @@ public class TokenRevocationTest extends AbstractKeycloakTest { } @Before - public void clientConfiguration() { + public void beforeTest() { + // Create client configuration realm = adminClient.realm("test"); ClientManager.realm(realm).clientId("test-app").directAccessGrant(true); ClientManager.realm(realm).clientId("test-app-scope").directAccessGrant(true); + + // Create clients + userInfoClient = AdminClientUtil.createResteasyClient(); + restHttpClient = HttpClientBuilder.create().build(); + } + + @After + public void afterTest() throws IOException { + userInfoClient.close(); + restHttpClient.close(); } @Page @@ -270,10 +295,32 @@ public class TokenRevocationTest extends AbstractKeycloakTest { } private void isAccessTokenDisabled(String accessTokenString, String clientId) throws IOException { + // Test introspection endpoint not possible String introspectionResponse = oauth.introspectAccessTokenWithClientCredential(clientId, "password", accessTokenString); TokenMetadataRepresentation rep = JsonSerialization.readValue(introspectionResponse, TokenMetadataRepresentation.class); assertFalse(rep.isActive()); + + // Test userInfo endpoint not possible + Response response = UserInfoClientUtil.executeUserInfoRequest_getMethod(userInfoClient, accessTokenString); + assertEquals(Status.UNAUTHORIZED.getStatusCode(), response.getStatus()); + + // Test account REST not possible + String accountUrl = OAuthClient.AUTH_SERVER_ROOT + "/realms/test/account"; + SimpleHttp accountRequest = SimpleHttp.doGet(accountUrl, restHttpClient) + .auth(accessTokenString) + .acceptJson(); + assertEquals(Status.UNAUTHORIZED.getStatusCode(), accountRequest.asStatus()); + + // Test admin REST not possible + try (Keycloak adminClient = Keycloak.getInstance(OAuthClient.AUTH_SERVER_ROOT, "test", "test-app", accessTokenString)) { + try { + adminClient.realms().realm("test").toRepresentation(); + Assert.fail("Not expected to obtain realm"); + } catch (NotAuthorizedException nae) { + // Expected + } + } } private String doTokenRevokeWithDuplicateParams(String token, String tokenTypeHint, String clientSecret)