From bdba5ff8b10fa51829eed0d9e9170b7c1970300e Mon Sep 17 00:00:00 2001 From: Stian Thorgersen Date: Mon, 12 Oct 2015 09:38:13 +0200 Subject: [PATCH] KEYCLOAK-1947 Add tests without client secret --- .../ClientAuthenticationFlow.java | 21 +++++----- .../ClientIdAndSecretAuthenticator.java | 8 ++-- .../testsuite/oauth/AccessTokenTest.java | 15 +++++++ ...urceOwnerPasswordCredentialsGrantTest.java | 42 +++++++++++++++---- 4 files changed, 64 insertions(+), 22 deletions(-) diff --git a/services/src/main/java/org/keycloak/authentication/ClientAuthenticationFlow.java b/services/src/main/java/org/keycloak/authentication/ClientAuthenticationFlow.java index 2065de0f02..225491f314 100644 --- a/services/src/main/java/org/keycloak/authentication/ClientAuthenticationFlow.java +++ b/services/src/main/java/org/keycloak/authentication/ClientAuthenticationFlow.java @@ -48,12 +48,9 @@ public class ClientAuthenticationFlow implements AuthenticationFlow { AuthenticationProcessor.Result context = processor.createClientAuthenticatorContext(model, authenticator, executions); authenticator.authenticateClient(context); - Response response = processResult(context); - if (response != null) return response; ClientModel client = processor.getClient(); if (client != null) { - String expectedClientAuthType = client.getClientAuthenticatorType(); // Fallback to secret just in case (for backwards compatibility) @@ -64,12 +61,16 @@ public class ClientAuthenticationFlow implements AuthenticationFlow { // Check if client authentication matches if (factory.getId().equals(expectedClientAuthType)) { + Response response = processResult(context); + if (response != null) return response; + + if (!context.getStatus().equals(FlowStatus.SUCCESS)) { + throw new AuthenticationFlowException("Expected success, but for an unknown reason the status was " + context.getStatus(), AuthenticationFlowError.INTERNAL_ERROR); + } + AuthenticationProcessor.logger.debugv("Client {0} authenticated by {1}", client.getClientId(), factory.getId()); processor.getEvent().detail(Details.CLIENT_AUTH_METHOD, factory.getId()); return null; - } else { - throw new AuthenticationFlowException("Client " + client.getClientId() + " was authenticated by incorrect method " + factory.getId(), - AuthenticationFlowError.INVALID_CLIENT_CREDENTIALS); } } } @@ -116,7 +117,9 @@ public class ClientAuthenticationFlow implements AuthenticationFlow { if (status == FlowStatus.SUCCESS) { return null; - } else if (status == FlowStatus.FAILED) { + } + + if (status == FlowStatus.FAILED) { if (result.getChallenge() != null) { return sendChallenge(result, execution); } else { @@ -130,11 +133,9 @@ public class ClientAuthenticationFlow implements AuthenticationFlow { if (alternativeChallenge == null) { alternativeChallenge = result.getChallenge(); } - return null; + return sendChallenge(result, execution); } else if (status == FlowStatus.FAILURE_CHALLENGE) { return sendChallenge(result, execution); - } else if (status == FlowStatus.ATTEMPTED) { - return null; } else { AuthenticationProcessor.logger.error("Unknown result status"); throw new AuthenticationFlowException(AuthenticationFlowError.INTERNAL_ERROR); diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/client/ClientIdAndSecretAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/client/ClientIdAndSecretAuthenticator.java index e476280020..5f2a3681b8 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/client/ClientIdAndSecretAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/client/ClientIdAndSecretAuthenticator.java @@ -86,13 +86,13 @@ public class ClientIdAndSecretAuthenticator extends AbstractClientAuthenticator return; } + context.setClient(client); + if (!client.isEnabled()) { context.failure(AuthenticationFlowError.CLIENT_DISABLED, null); return; } - context.setClient(client); - // Skip client_secret validation for public client if (client.isPublicClient()) { context.success(); @@ -106,8 +106,8 @@ public class ClientIdAndSecretAuthenticator extends AbstractClientAuthenticator } if (client.getSecret() == null) { - Response challengeResponse = ClientAuthUtil.errorResponse(Response.Status.BAD_REQUEST.getStatusCode(), "unauthorized_client", "Client secret setup required for client " + client.getClientId()); - context.challenge(challengeResponse); + Response challengeResponse = ClientAuthUtil.errorResponse(Response.Status.BAD_REQUEST.getStatusCode(), "unauthorized_client", "Invalid client secret"); + context.failure(AuthenticationFlowError.INVALID_CLIENT_CREDENTIALS, challengeResponse); return; } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java index 24f491178e..86a1c25a9e 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/AccessTokenTest.java @@ -164,6 +164,21 @@ public class AccessTokenTest { expectedEvent.assertEvent(); } + @Test + public void accessTokenMissingClientCredentials() throws Exception { + oauth.doLogin("test-user@localhost", "password"); + + Event loginEvent = events.expectLogin().assertEvent(); + String codeId = loginEvent.getDetails().get(Details.CODE_ID); + + String code = oauth.getCurrentQuery().get(OAuth2Constants.CODE); + AccessTokenResponse response = oauth.doAccessTokenRequest(code, null); + Assert.assertEquals(400, response.getStatusCode()); + + AssertEvents.ExpectedEvent expectedEvent = events.expectCodeToToken(codeId, loginEvent.getSessionId()).error("invalid_client_credentials").clearDetails().user((String) null).session((String) null); + expectedEvent.assertEvent(); + } + @Test public void accessTokenInvalidRedirectUri() throws Exception { oauth.doLogin("test-user@localhost", "password"); diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java index 252909154f..23cb7f260b 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/oauth/ResourceOwnerPasswordCredentialsGrantTest.java @@ -40,6 +40,9 @@ public class ResourceOwnerPasswordCredentialsGrantTest { ClientModel app = new ClientManager(manager).createClient(appRealm, "resource-owner"); app.setSecret("secret"); + ClientModel app2 = new ClientManager(manager).createClient(appRealm, "resource-owner-public"); + app2.setPublicClient(true); + UserModel user = session.users().addUser(appRealm, "direct-login"); user.setEmail("direct-login@localhost"); user.setEnabled(true); @@ -66,16 +69,22 @@ public class ResourceOwnerPasswordCredentialsGrantTest { @Test public void grantAccessTokenUsername() throws Exception { - grantAccessToken("direct-login"); + grantAccessToken("direct-login", "resource-owner"); } @Test public void grantAccessTokenEmail() throws Exception { - grantAccessToken("direct-login@localhost"); + grantAccessToken("direct-login@localhost", "resource-owner"); } - private void grantAccessToken(String login) throws Exception { - oauth.clientId("resource-owner"); + @Test + public void grantAccessTokenPublic() throws Exception { + grantAccessToken("direct-login", "resource-owner-public"); + } + + + private void grantAccessToken(String login, String clientId) throws Exception { + oauth.clientId(clientId); OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest("secret", login, "password"); @@ -85,7 +94,7 @@ public class ResourceOwnerPasswordCredentialsGrantTest { RefreshToken refreshToken = oauth.verifyRefreshToken(response.getRefreshToken()); events.expectLogin() - .client("resource-owner") + .client(clientId) .user(userId) .session(accessToken.getSessionState()) .detail(Details.RESPONSE_TYPE, "token") @@ -107,7 +116,7 @@ public class ResourceOwnerPasswordCredentialsGrantTest { assertEquals(accessToken.getSessionState(), refreshedAccessToken.getSessionState()); assertEquals(accessToken.getSessionState(), refreshedRefreshToken.getSessionState()); - events.expectRefresh(refreshToken.getId(), refreshToken.getSessionState()).user(userId).client("resource-owner").assertEvent(); + events.expectRefresh(refreshToken.getId(), refreshToken.getSessionState()).user(userId).client(clientId).assertEvent(); } @Test @@ -147,8 +156,6 @@ public class ResourceOwnerPasswordCredentialsGrantTest { .error(Errors.INVALID_TOKEN).assertEvent(); } - - @Test public void grantAccessTokenInvalidClientCredentials() throws Exception { oauth.clientId("resource-owner"); @@ -168,6 +175,25 @@ public class ResourceOwnerPasswordCredentialsGrantTest { .assertEvent(); } + @Test + public void grantAccessTokenMissingClientCredentials() throws Exception { + oauth.clientId("resource-owner"); + + OAuthClient.AccessTokenResponse response = oauth.doGrantAccessTokenRequest(null, "test-user@localhost", "password"); + + assertEquals(400, response.getStatusCode()); + + assertEquals("unauthorized_client", response.getError()); + + events.expectLogin() + .client("resource-owner") + .session((String) null) + .clearDetails() + .error(Errors.INVALID_CLIENT_CREDENTIALS) + .user((String) null) + .assertEvent(); + } + @Test public void grantAccessTokenVerifyEmail() throws Exception {