Example client repository allows public clients when it shouldn't

[matt-allan]

The client credentials grant should only be used by confidential clients:

The client MUST authenticate with the authorization server as described in Section 3.2.1.

https://tools.ietf.org/html/rfc6749#section-4.4.2

The ClientCredentialsGrant class does not validate that the client is confidential, but instead calls ClientRepository::validateClient.

To prevent a public client from using the client credentials grant type you would need to check the $grantType and fail validation if it's client credentials and the client is public.

However, the example ClientRepository does not do this and if you follow the example public clients would be allowed to use the client credentials grant type.

You can illustrate the problem by adding a public client to the example and making a curl request:

diff --git a/examples/src/Repositories/ClientRepository.php b/examples/src/Repositories/ClientRepository.php
index 47db508c..9339a971 100644
--- a/examples/src/Repositories/ClientRepository.php
+++ b/examples/src/Repositories/ClientRepository.php
@@ -43,6 +43,12 @@ class ClientRepository implements ClientRepositoryInterface
                 'redirect_uri'    => self::REDIRECT_URI,
                 'is_confidential' => true,
             ],
+            'mypublicapp' => [
+                'secret'          => null,
+                'name'            => 'My Other app',
+                'redirect_uri'    => self::REDIRECT_URI,
+                'is_confidential' => false,
+            ],
         ];
 
         // Check if client is registered

curl -X "POST" "http://localhost:4444/client_credentials.php/access_token" \
        -H "Content-Type: application/x-www-form-urlencoded" \
        -H "Accept: 1.0" \
        --data-urlencode "grant_type=client_credentials" \
        --data-urlencode "client_id=myothereapp" \ 
        --data-urlencode "scope=basic email"
{"token_type":"Bearer","expires_in":3600,"access_token":"..."}

Should the example be updated, or should we update the ClientCredentialsGrant to check Client::isConfidential so it's not necessary to check in validateClient?

[matt-allan]

Something that was confusing to me about this is the AuthCodeGrant only calls validateCredentials for private clients which made it seem like you no longer needed to check if the client was confidential before attempting to verify the secret.

However the refresh token grant and password grant both are supposed to support public clients according to the OAuth specification and do not check Client::isConfidential before calling validateClient.

Wouldn't that mean the isConfidential check in the AuthCodeGrant is unnecessary as you will need to check yourself in validateClient in order to support the other grant types?

[Sephster]

Hi Matt. Thanks for spotting this. Personally I think that we should only allow confidential clients so update the grant. Previously we didn't do this as we had no way of knowing if a client was confidential. We left those details up to the implementer. I think it would be wise we update the grant now to restrict it to confidential clients only.

For the other grants, we should probably follow suit. We should only validate a client's credentials if it is confidential. Part of the problem with this is at that point, the credentials have already been transmitted but I suppose it is enforcing good practice within the library.

[Sephster]

Closing this as the original issue has been resolved. I've opened up issue #1073 to deal with the public client support issue. Thanks @matt-allan