Client credentials middleware should allow any sort of valid/existing client

[JuanDMeGon]

• Passport Version: 8.0.1
• Laravel Version: 6.5.2
• PHP Version: 7.3.11
• Database Driver & Version: Any driver and version

Description:

Currently, it is not possible to get access to any action protected by the ClientCredentials middleware (CheckClientCredentials) if using a password or a personal client. BUT it should be possible, as those clients still being valid clients.

Now, even when it was discussed and seems to be a lot of confusion about this. The only job for the CheckClientCredentials middleware is to validate if the client is a valid client; does not matter the type of client it is.

Based on the theory and official standards of OAuth2: "The Client Credentials grant is used when applications request an access token to access their own resources, not on behalf of a user." (REF1, REF2). So, basically you use the CheckClientCredentials to protect the route and validate if the request comes from a valid client (independently of the type of client). In fact, a token obtained using the client credentials grant does not have a user associated with it, so any user-related action is going to fail.

It was introduced in #1040 with the call to $token->client->firstParty() (source).

In conclusion: If ANY client obtains an access token using the client credentials grant it SHOULD be authorized when making a request to an endpoint protected with the CheckClientCredentials middleware and currently it is not working that way.

Steps To Reproduce:

• Create a route protected with the CheckClientCredentials middleware
• Obtain a valid access token with a password or personal client (any of those) & using the grant_type: client_credentials
• Send a request using that access token to the route created at the first step and it will fail with "unauthorized" response. BUT, again, it should be allowed as it is a valid client AND successfully obtained an access token.

Thanks in advance. I hope it helps to resolve the current issues with this.

[alvirbismonte]

Just to add up to this issue, it should be either a Personal Access Client or Password Grant Client to replicate the Unauthorized response and not simply a Client Credentials Grant.

[JuanDMeGon]

Just to add up to this issue, it should be either a Personal Access Client or Password Grant Client to replicate the Unauthorized response and not simply a Client Credentials Grant.

Thanks. I fixed that now to get it more clear.

[driesvints]

I'll need @Sephster & @alshf89 to both weigh in here. Even @matt-allan if you're willing to.

We need to get a consensus about this once and for all so that we don't go back and forth changing things all the time.

[driesvints]

@billriess @soundsgoodsofar would be cool if you both could also give your insights here given your involvement on the original issue here: #691

[billriess]

In our apis we use both password and client_credentials grants using the same oauth client. We require the consumer apps to use client credentials for non-user apis like pulling app configuration or registering new users then password grants for authenticating. All of our apps are first party though so we can trust this use case.

While I agree that the middleware should work for all grant types, this would be an unexpected flow for our use case. If this is changed it would likely need a major version bump to prevent breaking existing use cases.

[T0miii]

Has some one got a workaround going for this, while it will or not will be changed?

[driesvints]

@T0miii don't upgrade to 8.0 for now

[JuanDMeGon]

In our apis we use both password and client_credentials grants using the same oauth client. We require the consumer apps to use client credentials for non-user apis like pulling app configuration or registering new users then password grants for authenticating. All of our apps are first party though so we can trust this use case.

While I agree that the middleware should work for all grant types, this would be an unexpected flow for our use case. If this is changed it would likely need a major version bump to prevent breaking existing use cases.

I am not so sure to understand your issue here. If a token is obtained using the client credentials grant, it will be allowed to pass ONLY through the routes protected by the CheckClientCredentials middleware. A token issued using client credentials grant does not pass through a route protected by auth:api. Only the tokens that have a user associated with it can pass the auth:api middleware (those are the ones using password, or authorization code, or even personal, grants).

Now, finally, I think this is not a matter of a consensus but more of following the OAuth2 directives. Here is the official RFC for the client credentials grant in the OAuth2 specification: 4.4. Client Credentials Grant.

As in the previous RFC there is no distinction of the "client type" at all, it only matters if the credentials are correct or not to get an access token. Once again, as the standard says, this grant is intended to provide access to ANY route which is not related to any user recourse (which is the current way to work of Laravel Passport).

It seems like we are accommodating Passport to the use cases of some, instead of stick to the specification itself.

[billriess]

In our apis we use both password and client_credentials grants using the same oauth client. We require the consumer apps to use client credentials for non-user apis like pulling app configuration or registering new users then password grants for authenticating. All of our apps are first party though so we can trust this use case.
While I agree that the middleware should work for all grant types, this would be an unexpected flow for our use case. If this is changed it would likely need a major version bump to prevent breaking existing use cases.

I am not so sure to understand your issue here. If a token is obtained using the client credentials grant, it will be allowed to pass ONLY through the routes protected by the CheckClientCredentials middleware. A token issued using client credentials grant does not pass through a route protected by auth:api. Only the tokens that have a user associated with it can pass the auth:api middleware (those are the ones using password, or authorization code, or even personal, grants).

Now, finally, I think this is not a matter of a consensus but more of following the OAuth2 directives. Here is the official RFC for the client credentials grant in the OAuth2 specification: 4.4. Client Credentials Grant.

As in the previous RFC there is no distinction of the "client type" at all, it only matters if the credentials are correct or not to get an access token. Once again, as the standard says, this grant is intended to provide access to ANY route which is not related to any user recourse (which is the current way to work of Laravel Passport).

It seems like we are accommodating Passport to the use cases of some, instead of stick to the specification itself.

I agree that the middleware should only care about authenticating the client, for any grant type. The issue for our implementation is the opposite of what you described. In our case, we don't want the password grant to access non-user specific APIs which this change would allow. So we're leveraging the fact that it does not work as it should according to the specification. We would just need to update the way we're handling that flow on our backend but changing this without a major version change could be a breaking change for people currently using it as is. That was my only concern.

Again, just to be clear, I agree with your original post. The middleware should only care about validating the client, not the grant type.

[JuanDMeGon]

I agree that the middleware should only care about authenticating the client, for any grant type. The issue for our implementation is the opposite of what you described. In our case, we don't want the password grant to access non-user specific APIs which this change would allow. So we're leveraging the fact that it does not work as it should according to the specification. We would just need to update the way we're handling that flow on our backend but changing this without a major version change could be a breaking change for people currently using it as is. That was my only concern.

Again, just to be clear, I agree with your original post. The middleware should only care about validating the client, not the grant type.

OK, I think I understand now. So, I think this should be resolved, of course, at app level and not in the package as currently seems to be.

Just to provide a possible solution to your case, you can add one middleware or condition checking the client type and blocking access the case it is not the client type you want (basically the same segment is currently causing the issues here).

I think we agree on the issue about the current way it is working, so if @driesvints is OK with it, I would send a PR to remove the current behavior. Additionally, I would like to know if it will require a major release again or just a fix on the current one?

[alshf89]

as long as I remember, I faced with this issue when I had some API routes that didn't belong to User scope API but CheckClientCredentials middleware didn't care about it... so when someone issued an access token via Password Credential they could access these Client Credential API Routes as well.

[JuanDMeGon]

as long as I remember, I faced with this issue when I had some API routes that didn't belong to User scope API but CheckClientCredentials middleware didn't care about it... so when someone issued an access token via Password Credential they could access these Client Credential API Routes as well.

OK, I get your point, but the implementation is not correct, so. As it is restricting the client and not the token itself.

ANY client can issue a token using client_credentials grant type, so you should not restrict based on the client type but in the grant used to obtain that specific token (following your idea).

Now, in the RFC, there is not any detail about this, so even when your point is valid (about restricting this only to access tokens obtained using the client_credentials grant) there is not a specification that says if any other access token still valid or not.

From the logic, even when a client used the password grant to obtain an access token, its credentials still being valid, so that client (using that token) can definitively pass through the client.credentials middleware as well.

The main purpose of the client.credentials middleware is to validate the credentials of the clients, which, by definition, are correct if the access token is valid.

Finally, I am not basing this in my thinking of how should it work, it is based on the current implementations and RFCs. Even when the client_credentials grant is suitable to issue tokens for machine-to-machine communication is not the only case of use. Those access tokens are valid for any case where "the client making the requests don’t require a user’s permission" (REF1, REF2).

I hope it helps to resolve the current issue.

Again, if @driesvints or anyone behind this is OK I would make a PR to remove that small segment in the condition verifying the client type, which is completely wrong (even for the use case mentioned by @alshf89).

Please let me know, I would like to resolve this as soon as possible as it is probably affecting a lot of projects.

[Sephster]

Hey @driesvints sorry for the late response. I don't think the check is required. Apologies for the wall of text as you will no doubt be aware of all that I'm writing. I've mainly laid it out to get my thoughts down.

The middleware is called CheckClientCredentials so I would assume that it is checking if the client's credentials provided are valid. The only method that is doing this is the handle() method which is calling the validateAuthenticatedRequest() method in the OAuth2 server. There are other methods that don't appear to be checking the client credentials so I'm not sure they should be in this middleware but there names make this a little confusing. I will lay out my thoughts below.

The validate() function makes two calls to validateCredentials() and validateScopes() and upon reflection, I don't think either of these are required.

validateCredentials() checks if a token has been found in the DB based on OAuth Access Token ID. It also checks if the client is a first party client. Neither of these activities are associated with the checking of a client's credentials so the name of this method could cause confusion. The description for the method is Validate token credentials but it is probably better to say this method is checking that a token exists in the DB. I don't think that the first party check should exist because as @JuanDMeGon has mentioned, this isn't a requirement in the OAuth spec and I can't think of a good reason for it being added.

Similarly, I don't think we need to check if there is a token in the DB. The only reason I can think to do this is to find out if the token is revoked but we already have a method that does this in the token repository so there is no reason to keep this method. It doesn't serve a purpose as far as I can see.

The next method in the validate() call is validateScopes() which we had discussed briefly in PR #1112. After much thought, I think this method should be removed as well. The docblock for this method has the same description as validateCredentials(), Validate token credentials. Like validateCredentials(), I think this description doesn't match what the method is doing. If my understanding is correct, It is checking that the scopes in the request's token match the ones specified in the DB.

Upon initial review we had come to the conclusion that it was best to keep this method for personal access tokens where you can edit the scopes of the token without reissuing the personal access token. I looked into this and I think the way that the personal access token is created at the moment won't allow for this. It is a signed JWT like all the other access tokens meaning it can't be tampered with.

There is no need to check these against the DB as we can be sure that if the access token passes validation in the OAuth2-Server, the scopes are the ones that were approved in the original access request. Unfortunately the only way to edit these is to issue a new token as any modification will result in an auth fail.

In summary, I think the validate() method and its two child methods, validateScopes() and validateCredentials() are both redundant and can be removed. The only concern is that as @billriess mentioned, removing the first party check would be a change in behaviour so could be considered breaking for some. Sorry in advance for the wall of text.

[JuanDMeGon]

Hey @driesvints sorry for the late response. I don't think the check is required. Apologies for the wall of text as you will no doubt be aware of all that I'm writing. I've mainly laid it out to get my thoughts down.
...

I very agree with everything. Anyway, our main purpose (at least for this issue) is to remove the client type verification (the call to firstParty()).

Now, even when it seems to be a breaking change, it is basically a fix to return Passport to the regular and accurate behavior. Of course, it is a decision from the maintainers if put it as a fix or bump the version and consider this a breaking change.

So, once again, if @driesvints can let me know, I will gladly make the PR but need to know if do this to the current version or the upcoming one. Anyway, I think, even when it would be for a future version, that version should be released almost immediately. Let me know guys, I am just waiting for the green light :)

Thanks :)

[driesvints]

Can you list the exact changes you'd like to make? Then we can determine if it can go to 6.x or master.

[JuanDMeGon]

Can you list the exact changes you'd like to make? Then we can determine if it can go to 6.x or master.

This is literally a small segment. In this line the condition would change from if (! $token || $token->client->firstParty()) just to if (! $token)

I would need to check locally if there is any test involved on this which could need any change as well. So even when it is a minor fix, it changes the current behavior, but still being a fix introduced after release or version 8.0.

So, that's all. Let me know how it sounds to you.

[alshf89]

In our apis we use both password and client_credentials grants using the same oauth client. We require the consumer apps to use client credentials for non-user apis like pulling app configuration or registering new users then password grants for authenticating. All of our apps are first party though so we can trust this use case.

While I agree that the middleware should work for all grant types, this would be an unexpected flow for our use case. If this is changed it would likely need a major version bump to prevent breaking existing use cases.

We have same structure...

[JuanDMeGon]

We have same structure...

Yes, following those possibilities, even when that was not the correct way to go, that would be a breaking change for some projects so probably need to be in a major release.

[driesvints]

@billriess @alshf89 do you both agree that the change mentioned here is the correct one and that we should implement it?

#1125 (comment)

[alshf89]

@billriess @alshf89 do you both agree that the change mentioned here is the correct one and that we should implement it?

#1125 (comment)

In my opinion I’m sort of agree with @JuanDMeGon, but this issue (#691) was really important for lots of people and one of them was me and talked about it a lot so we have decided about it and have created a PR and fix it on that moment.

Everyone have different use cases for this scenario so we should find the best practices and best approach for this section, @JuanDMeGon did some research about it and he said its not correct and we should not filter token base on client type but like he said :

in the RFC, there is not any detail about this, so even when your point is valid (about restricting this only to access tokens obtained using the client_credentials grant) there is not a specification that says if any other access token still valid or not.

And beside @Sephster’s explanation which is very great in detail, he said, we don’t need them (those changes) at all...

But I think we should talk about it more to find out how many developers or projects will imapct these changes or why does issue #691 created in first place? or why does PRs accept on it ?

We used laravel/passport in lots of project like @billriess mentioned, in some of them we faced this issue and we talked about it ... but on that moment these changes didn’t exist in laravel/passport package so we extended those middlewares and override the validate methods based on what we needed in our projects.

If we can issue an access token via client credential grant type for password credential client then I think it is fine like @JuanDMeGon said
But what if we have some api routes that only use machine to machine request and we really don’t want password credential client issue an access token and reach those endpoints!

So @driesvints, changes on this section is up to laravel/passport community and what u guys decide and what is the best approach...

[JuanDMeGon]

I understand the use-cases exposed. I think the solution proposed was functional BUT not correct, especially at the package level.
The package (Passport) should care about following OAuth standards and practices and restricting the usage of an access token based on the client type is not correct.
If someone needs to restrict the usage of a token ONLY to some specific clients, can fix that by its own but not the package itself. Or at least no changing the client.credentials middleware (maybe adding another middleware with that different job may help).
The logic for this is simple: the client.credentials middleware should check if the credentials of the client are valid which is easily assumed once the access_token is valid. It does not matter the grant used to obtain that access token, the credentials of the client must be valid for that.
The referenced issue #691 seems to have some misunderstanding, as protecting an endpoint with client.credentials does not allow any user-based actions, as this is only possible when protecting the route with auth:api. Basically, if protecting the route with client.credentials, when you try to get the user (for example $request->user()) will return null.
Finally, the use-case exposed and the solution given is not correct. You should not restrict a token based on the client type BUT based on the grant used for the token. BUT once again, it is not a job for Passport (at least not in the client.credentials middleware).
I hope it helps to clarify this, as it is getting long.
PS: We can additionally create a new middleware called, for example, OnlyClientCredentialsTokens which not only check credentials (validate the token) but additionally verify if the token was obtained using a client_credentials grant (which is anyway different from what Passport is doing now).

[alshf89]

I understand the use-cases exposed. I think the solution proposed was functional BUT not correct, especially at the package level.
The package (Passport) should care about following OAuth standards and practices and restricting the usage of an access token based on the client type is not correct.
If someone needs to restrict the usage of a token ONLY to some specific clients, can fix that by its own but not the package itself. Or at least no changing the client.credentials middleware (maybe adding another middleware with that different job may help).
The logic for this is simple: the client.credentials middleware should check if the credentials of the client are valid which is easily assumed once the access_token is valid. It does not matter the grant used to obtain that access token, the credentials of the client must be valid for that.
The referenced issue #691 seems to have some misunderstanding, as protecting an endpoint with client.credentials does not allow any user-based actions, as this is only possible when protecting the route with auth:api. Basically, if protecting the route with client.credentials, when you try to get the user (for example $request->user()) will return null.
Finally, the use-case exposed and the solution given is not correct. You should not restrict a token based on the client type BUT based on the grant used for the token. BUT once again, it is not a job for Passport (at least not in the client.credentials middleware).
I hope it helps to clarify this, as it is getting long.
PS: We can additionally create a new middleware called, for example, OnlyClientCredentialsTokens which not only check credentials (validate the token) but additionally verify if the token was obtained using a client_credentials grant (which is anyway different from what Passport is doing now).

I like the idea of OnlyClientCredentialsTokens middleware ...

[Sephster]

I think @JuanDMeGon has hit the nail on the head. There was a misunderstanding of the middleware in #691. If people want to restrict certain grants this should be a userland issue and not a Passport implementation. This is not something the OAuth spec deals with (nor should it). Effectively, it is the scopes that typically restrict what actions you can do with an access token, not the client type. Using the client type in this manner is something custom and niche.

I fell that both the verify functions should be stripped out in their current form. It probably is a major change though as some people have already pointed out they rely on this functionality.

[JuanDMeGon]

I think @JuanDMeGon has hit the nail on the head. There was a misunderstanding of the middleware in #691. If people want to restrict certain grants this should be a userland issue and not a Passport implementation. This is not something the OAuth spec deals with (nor should it). Effectively, it is the scopes that typically restrict what actions you can do with an access token, not the client type. Using the client type in this manner is something custom and niche.

I fell that both the verify functions should be stripped out in their current form. It probably is a major change though as some people have already pointed out they rely on this functionality.

Very agree. So, I think I would make the PR to the next major release. Anyway, I think this release should be released as soon as possible to return Passport to its regular/correct behavior.

I will wait a few, in case @driesvints or someone else has something to add, and then will send a small PR with the fix.

Thank you all for the comments. I am glad we get into a solution.

[driesvints]

Anyway, I think this release should be released as soon as possible to return Passport to its regular/correct behavior.

We only just released a new major version so we probably won't be releasing a new one anytime soon. If this gets merged on master you could use dev-master for the time being if you really need it.

Feel free to send in a PR to master.

[JuanDMeGon]

Anyway, I think this release should be released as soon as possible to return Passport to its regular/correct behavior.

We only just released a new major version so we probably won't be releasing a new one anytime soon. If this gets merged on master you could use dev-master for the time being if you really need it.

Feel free to send in a PR to master.

Ok, I understand that. I hope there does not pass so much time on it.
I already sent the PR (#1132). Please let me know if something else.

[driesvints]

We merged this into master. Thanks for everyone's participation in figuring this out!

[sylouuu]

@driesvints I'm a bit stuck here, why the merge is in the master branch since December and not inside the latest release 8.4.1?

Bests

[JuanDMeGon]

@sylouuu this will be released in the next major version (Passport 9), will be probably around the release of Laravel 7.
For now, you can use the dev-master or version 7 of Passport to avoid this issue.