Skip to content
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

Always validate auth code clients, even when public #1036

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Always validate auth code clients, even when public #1036

wants to merge 1 commit into from

Conversation

matt-allan
Copy link
Contributor

The validateClient method is called for public clients when using the refresh_token and password grant type and the interface allows passing a null secret, so it's not necessary to skip calling the method for the authorization_code grant type.

When I saw that we skipped calling validateSecret for public clients in the auth code grant and that the $mustValidateSecret parameter was removed for 8.0 I assumed that meant validateClient would no longer be called for public clients. I later realizedvalidateClient is called for public clients when using the password or refresh token grant.

If validateClient is called for public clients for the other grant types you will need to do something like return !$client->isConfidential() || hash_equals($client->getSecret(), $clientSecret) anyway, so it doesn't seem necessary to skip calling it here.

Hopefully by removing the unnecessary check it will be less misleading to other developers and increase the likelihood that they handle public clients appropriately in validateClient.

The validateClient method also calls validateRedirectUri which emits a CLIENT_AUTHENTICATION_FAILED event if validation fails. The validateAuthorizationCode also validates the redirect URI but does not emit an event. Calling validateClient regardless will ensure the event is consistently fired for invalid redirect URIs just in case someone is relying on it.

The validateClient method is called for public clients when using the
refresh_token and password grant type and the interface allows passing
a null secret, so it's not necessary to skip calling the method for the
authorization_code grant type.
@matt-allan
Copy link
Contributor Author

Alternatively this could go the other way and we could only call validateClient for confidential clients for all grant types?

@Sephster
Copy link
Member

Good catch. I didn't notice that the function doesn't require a secret at the time this change was made. I think I need to look into this in a bit more detail. There is still some validation that is occurring by checking the client ID against the redirect URI which is useful. It might well be the case that we just validate regardless.

I will have a deeper think about this tonight. Thanks for spotting this!

@Sephster
Copy link
Member

Hi @matt-allan - sorry for my delay in looking at this. I've had a big think and I think ultimately, we should probably be only validate a client for confidential clients in all grants.

I did think about approving this change, but I ultimately feel that calling validateClient would be incorrect in this scenario. We don't have any credentials to validate for a pubic client and we don't need to validate the redirect URL in the context provided by the validateClient method as this was already done when the auth code was requested.

The only check we need to perform is to make sure that the redirect URI matches the one provided when getting the auth code and we do this later in the code.

There is a bit of tidying up that needs to be done by the look of things. ValidateClient needs to be changed so that it only validates client credentials (no point in passing null) I think we also need to move the redirect uri checks into its own method to make things a bit more explicit.

For now though, I think just making sure we only call validateClient if the client is confidential in the password grant and refresh grant would suffice. I would appreciate your thoughts on this and thanks very much for highlighting the inconsistency here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants