[13.x] Make client RFC compatible

[hafezdivandari]

This PR refactors OAuth2 Client implementation to make it more RFC compatible and more secure.

Client Grant Types

The problem: Currently there is no consistent way to determine what grants each client can handle, this is a security concern! We have personal_access_client and password_client boolean properties but it's not a good idea to add a boolean attribute for each grant, e.g. client_credentials.

The solution: The grant_types property was added to the Client model long time ago (first appearance was on #729 then #731). This PR adds grant_types column to the oauth_clients table as a JSON array and makes other changes to always check the allowed grant types the client can handle, RFC7591. Here is the list of grant types:

• "authorization_code"
• "personal_access"
• "implicit"
• "password"
• "client_credentials"
• "refresh_token"
• "urn:ietf:params:oauth:grant-type:device_code" (TBD [13.x] Device Authorization Grant RFC8628 #1750)

For example, a client with 'grant_types' => ['authorization_code', 'refresh_token'] can handle "Authorization Code" and "Refresh Token" grant types.

Client Redirect URIs

The problem: Auth clients ("Implicit" and "Auth Code" grants) may have multiple redirect URIs, we currently use comma separated list of values which is not RFC compatible.

The Solution: The redirect property of the Client model has been renamed to redirect_uris and is going to be stored as an array of strings instead of a comma-separated list of values, to be compatible with RFC7591

Fixed

• Client::hasGrantType() method has been fixed to determine what grants the client handles.
• ClientRepository::create() method and passport:client artisan command has been fixed:
  • "Personal access" grant: doesn't have redirect URI and now asks for user provider.
  • "Password" grant: default user provider now respects defined configs.
  • `Client credentials" grant: doesn't have redirect URI.
  • "Implicit" grant: --implicit option has been added for backward compatibility.

Changes

• grant_types and redirect_uris columns has been added on oauth_clients table as text (JSON array).
• redirect, personal_access_client, and password_client columns of oauth_clients table has been removed.

Upgrade Guide

No upgrade needed. When oauth_clients table doesn't have grant_types and redirect_uris columns, it fallbacks to old redirect, personal_access_client, and password_client columns.

[taylorotwell]

A lot of breaking changes here and I'm not sure the ROI is there to support them.

[hafezdivandari]

@taylorotwell This PR actually make this package easier to maintain and support by removing some deprecations and unnecessary configurations.

Please let us know what the community can do to make Passport profitable in the way you prefer, e.g. I wanted to make Passport compatible with Laravel Jetstream / Fortify. We can also change its logo just like other Laravel first-party packages, etc.

Many people are using Laravel to develop APIs and OAuth2 is a must in most scenarios. Sanctum is great, but you know the difference better than me.

We have already added support for v9 of the OAuth2 Server (#1734). The latest version adds support for "Device Authorization Flow" RFC8628; I've already prepared a PR to support that on Passport that I will send after this one.

cc @driesvints

[driesvints]

Hi @hafezdivandari. We really appreciate all of this work! But like Taylor said, I also feel this is a bit too much... The changes in this PR all seem sound to me but impose a hefty upgrade path on users, something we at Laravel try to avoid at all cost. There for, I feel we should cut on some of these changes.

I would:

• At the very least send in separate PR's for all of these changes as doing it in a single PR is just too difficult to review for Taylor.
• Not do the following changes: Client ID, Client Redirect URIs, Client Grant Types, Client Scopes & Personal Access Token Model and Table. These take up the bulk of the upgrade steps and aren't 100% needed imo. Also passport:client --implicit isn't needed since that grant type is no longer recommended.
• Still do: Client Secret, Laravel v10 bump, remove "Password Grant" from install command and also the --password flag, do the UriRule rewrite (and also same changes for RedirectRule).

I realise this will cut a lot of the work you made but this will make the transition for users much more feasible.

[hafezdivandari]

Hi @driesvints, thanks for your reply. I should have sent these changes as separate PRs, but I thought it would be hard to guess why each one is needed without knowing the whole picture. I'll resend separately, but please keep this one open as draft for a while.

[stanliwise]

Finally, We are going to have a compatible OAuth Framework. The optional hash part of Passport is a security concern. User should instead need to specify they don't want it hashed rather than specify they wanted it hashed.

[hafezdivandari]

@stanliwise you may check this #1745

[Jubeki]

From a security and maintainability perspective, I also agree, that aligning with the RFC is better!

But of course at the same time aligning with the RFC, also makes it sometimes harder, because you need to know all of the RFCs related to OAuth 2.0, which is a lot. (Something which should be addressed with OAuth 2.1, which reduces the number of RFC to I think a single one. OAuth 2.1 is still WIP).

[hafezdivandari]

@Jubeki thanks for your comment. This PR just fixes 2 problems that we already have with client registration: redirect_uris and grant_types. Even the OAuth 2.1 (draft) supports these changes. According to these RFCs, What we know for sure that the client doesn't have personal_access_client, password_client and comma separated redirect properties.

• We know that a client may have multiple redirect URIs as array of strings. ref
• We also know that a client can handle combination of several grant types. ref

[taylorotwell]

I personally still remain unconvinced the ROI is there on this PR. Every Passport app in the world will have to perform a pretty significant database change that will incur downtime for very marginal benefit as far as I can tell. People don't seem to be begging for this by the hundreds.

[hafezdivandari]

@taylorotwell I've fixed this to be backward compatible, new installations will have grant_types and redirect_uris . We fallback to old redirect, personal_access_client and password_client columns for old versions.

People don't seem to be begging for this by the hundreds.

PR #729 merged on 2018 explains why grant_types is needed. The current PR just makes it the default solution.

Some people may don't know the risk here, e.g. every confidential client can be used as client_credentials grant to generate an access token. Some fixed / changed tests show the problem here. Currently there is no way to check what grant types each client does handle.

[Jubeki]

How about using a config option, instead of querying the column details?

[hafezdivandari]

Also thought about a new config option, but it seems to be redundant when we can easily check for existing columns?

[Jubeki]

It would save one query. But I will leave that decision to you / the Laravel Team.

[hafezdivandari]

Yeah but client creation / updating doesn't happen frequently. Lets see what maintainers decide.

[Jubeki]

I would also like it if there are test cases with the old columns, so that you can continue checking if everything works as expected.

[hafezdivandari]

@Jubeki thanks. I've added some tests at bf7ce0d