laravel / passport

Laravel Passport provides OAuth2 server support to Laravel.
https://laravel.com/docs/passport
MIT License
3.25k stars 780 forks source link

[13.x] Make client RFC compatible #1744

Open hafezdivandari opened 1 month ago

hafezdivandari commented 1 month ago

This PR refactors OAuth2 Client implementation to make it more RFC compatible and removes some redundant codes from Passport.

Client Redirect URIs

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, RFC7591

Changes

Client Grant Types

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:

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

Changes

Client Scopes

The scopes property was added to Client model on #1682 to limit scopes of a client. This PR adds scopes column to the oauth_clients table as a JSON array and makes other changes to always check for the scopes that a client can handle.

Changes

Miscellaneous Changes

Upgrade Guide

Clients Table

The oauth_clients table now requires grant_types, scopes and redirect_uris columns as JSON array and personal_access_client and password_client columns are removed:

Schema::table('oauth_clients', function (Blueprint $table) {
    $table->after('name', function (Blueprint $table) {
        $table->text('grant_types');
        $table->text('scopes');
        $table->text('redirect_uris');
    });
});

foreach (Passport::client()->cursor() as $client) {
    Model::withoutTimestamps(fn () => $client->forceFill([
        'grant_types' => match (true) {
            (bool) $client->personal_access_client => ['personal_access'],
            (bool) $client->password_client => ['password', 'refresh_token'],
            empty($client->secret) && ! empty($client->redirect) => ['authorization_code', 'implicit', 'refresh_token'],
            ! empty($client->secret) && empty($client->redirect) => ['client_credentials'],
            ! empty($client->secret) && ! empty($client->redirect) => ['authorization_code', 'implicit', 'refresh_token', 'client_credentials'],
            default => [],
        },
        'scopes' => ['*'],
        'redirect_uris' => explode(',', $client->redirect),
    ])->save());
}

Schema::table('oauth_clients', function (Blueprint $table) {
    $table->dropColumn(['redirect', 'personal_access_client', 'password_client']);
});
taylorotwell commented 1 month ago

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

hafezdivandari commented 1 month ago

@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 commented 1 month ago

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:

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

hafezdivandari commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@stanliwise you may check this #1745