-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate confidential clients and determine if the client handles the grant type #1420
base: master
Are you sure you want to change the base?
Validate confidential clients and determine if the client handles the grant type #1420
Conversation
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a grant type settings. Otherwise the client must know about all grant types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ClientRepositoryInterface
hasvalidateClient($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:ClientTrait
that returnstrue
by default to avoid BC breaking changes.