Validate confidential clients and determine if the client handles the grant type #1420
Conversation
hafezdivandari
changed the title
hafezdivandari
changed the title
| @@ -38,4 +38,9 @@ public function getRedirectUri(): string|array; | |||
| * Returns true if the client is confidential. | |||
| */ | |||
| public function isConfidential(): bool; | |||
|
|
|||
| /** | |||
| * Returns true if the client handles the given grant type. | |||
There was a problem hiding this comment.
This should be a grant type settings. Otherwise the client must know about all grant types.
There was a problem hiding this comment.
Yes, the client may know the grant types it can handle, for example by having grant_types according to RFC7591
There was a problem hiding this comment.
RFC7591 describes the proccess of filling the client repository. It responses with a client information record. ClientRepositoryInterface has validateClient($clientIdentifier, $clientSecret, $grantType) method. The client's support for the grant type should be done in this method. Do you agree with it?
There was a problem hiding this comment.
RFC7591 describes the proccess of filling the client repository. It responses with a client information record
It was just an example of how client may define its supported grant types.
ClientRepositoryInterfacehasvalidateClient($clientIdentifier, $clientSecret, $grantType)method. The client's support for the grant type should be done in this method. Do you agree with it?
That's exactly what I don't agree with in the current implementation, and this PR tries to fix.
First of all, this method returns bool, and causes invalid_client error when it returns false, but we need unauthorized_client error, when the client doesn't support the given grant type.
Second this method (validateClient) main intention is to validate the client secret, you may check its description.
Third, this method is being called too late, in the last step of the flow, on respondToAccessTokenRequest method of all grants. But we want to check if the client actually handles the grant type in the very beginning of the flow.
Lets assume we have a client that only supports client_credentials grant, currently you can use this client to get an authorization code with no problem, and then in the last step when you want to use that code to get an access_token the server calls validateClient method, and you'll get invalid_client error. After this PR, you will get unauthorized_client error, in the first step, when you want to use that client to get an authorization code, because this client doesn't actually handle "authorization code" grant but only "client credentials" grant.
Fixes #1174
Fixes #1369
Fixes #1073
Closes #1036
This PR can be considered as a security enhancement and does 2 main changes:
AbstractGrant::validateClient()only if the client is confidential (RFC6749 section 4.1.3). But "device authorization", "refresh token" and "password" grants always validate the client secret. This PR allows public "device authorization" (RFC8628 section 3.4), "refresh token" (RFC6749 section 6) and "password" (RFC6749 section 4.3.2) clients by calling theClientRepositoryInterface::validateClient()method only if the client is confidential.AbstractGrant::getClientEntityOrFail()twice instead of just using theAbstractGrant::validateClient()return value, this PR fixes that.ClientEntityInterface::hasGrantType()function:ClientTraitthat returnstrueby default to avoid BC breaking changes.