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

[7.0] Allow first party clients to skip the authorization prompt #1022

Merged
merged 2 commits into from
May 23, 2019
Merged

[7.0] Allow first party clients to skip the authorization prompt #1022

merged 2 commits into from
May 23, 2019

Conversation

matt-allan
Copy link
Contributor

@matt-allan matt-allan commented May 21, 2019

Overview

This PR adds a new method Client::skipsAuthorization(). If the method returns true the client is allowed to skip the authorization prompt.

Rationale

It's considered best practice to use the Authorization code grant for first party mobile apps and SPAs but Passport currently requires you to show an authorization prompt to the user ("{client} is requesting permission...") with this grant type.

It doesn't really make sense to show the authorization prompt for first party clients. If Google asked you to grant permission to Google every time you logged in to gmail it would be pretty confusing.

Allowing first party clients to skip the authorization prompt is supported by the majority of OAuth servers: Auth0, Otka, Doorkeeper, Django OAuth Toolkit, IdentityServer, Keycloak, Ory Hydra, and others. It's also explicitly allowed by the OAuth specification.

It's difficult to add this feature yourself. You have to override the Authorization controller which requires copying ~30 lines of code. You then need to register the route override in the AuthServiceProvider after the Passport::routes() call and make sure to add the same middleware.

There is some more background info in #1010.

Usage

To use this feature you need to extend the Client and override the skipsAuthorization method. You also need to tell Passport to use your extended model in the AuthServiceProvider.

How you implement skipsAuthorization is up to you. For example, you may add a first_party boolean column to the database and check the value. Alternatively you might want to check the redirect URL and allow any client for a given domain.

use App\Models\Passport\Client;
use App\Models\Passport\Token;
use App\Models\Passport\AuthCode;
use App\Models\Passport\PersonalAccessClient;

/**
 * Register any authentication / authorization services.
 *
 * @return void
 */
public function boot()
{
    $this->registerPolicies();

    Passport::routes();

    Passport::useClientModel(Client::class);
}
namespace App\Passport;

use Laravel\Passport\Client as BaseClient;

class Client extends BaseClient
{
    /**
     * Determine if the client should skip the authorization prompt.
     *
     * @return bool
     */
    public function skipsAuthorization()
    {
        return $this->first_party;
    }
}

Backwards Compatibility

This feature is opt-in and does not do anything unless you override the skipsAuthorization method to return true.

Use the expected user and client objects instead of strings
@driesvints driesvints changed the title Allow first party clients to skip the authorization prompt [7.0] Allow first party clients to skip the authorization prompt May 21, 2019
@driesvints
Copy link
Member

Instead of doing this in the settings which might be a little more overhead, why don't we do this in the model itself?

public function skipsAuthorization()
{
    return false;
}

And then:

if (($token && $token->scopes === collect($scopes)->pluck('id')->all()) ||
    $client-> skipsAuthorization()) {
    return $this->approveRequest($authRequest, $user);
}

This way it's done on a model base without an additional setting. If people want to override they can override the model and method and still build in custom support for anything specific if they want.

@matt-allan
Copy link
Contributor Author

👍 sounds good to me, I will push an update shortly.

@matt-allan
Copy link
Contributor Author

@driesvints I implemented the change, updated the PR description, and rebased.

@taylorotwell taylorotwell merged commit f57d130 into laravel:7.0 May 23, 2019
@RomainLanz
Copy link

Hey all, 👋

Are you going to provide a PR for the documentation too @matt-allan?

@matt-allan
Copy link
Contributor Author

Thanks for the reminder @RomainLanz, I forgot. I just opened a PR: laravel/docs#5226

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.

None yet

4 participants