Merge pull request #1717 from stianst/master

KEYCLOAK-1947 Add tests without client secret

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);

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());
+            Response challengeResponse = ClientAuthUtil.errorResponse(Response.Status.BAD_REQUEST.getStatusCode(), "unauthorized_client", "Invalid client secret");
-            context.challenge(challengeResponse);
+            context.failure(AuthenticationFlowError.INVALID_CLIENT_CREDENTIALS, challengeResponse);
             return;
         }
 

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");

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 {
+    @Test
-        oauth.clientId("resource-owner");
+    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 {