search

laravel / passport

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

Add default scopes to issued token #1679

Closed xdiegom closed 1 year ago

xdiegom commented 1 year ago

Hi there,

This PR is related to passport default scopes.

I was working on a project using Laravel Passport and after I register the default scopes in AuthServiceProvider.php with Passport::setDefaultScope() the returned token didn't have the scopes defined in the payload. Thus, I decided to make some changes which are:

  1. Default scopes are now part of the token if they are defined with Passport::setDefaultScope()`.
  2. Given the premise of the RFC about Access Token Scope, specially in this part

    "The authorization server MAY fully or partially ignore the scope requested by the client, based on the authorization server policy or the resource owner's instructions. If the issued access token scope is different from the one requested by the client, the authorization server MUST include the "scope" response parameter to inform the client of the actual scope granted"

when validating scopes, Passport should add the defaultScope as part of the validation hasScope() and methods like scopeIds() and scopes(). For that matter, I added a new Passport static method called useDefaultScopes() that sets a boolean static attribute to true in case default scopes MAY want to be included in issued token.

For this, I am not quite sure if this method is useful and always add default scopes if defaultScope is not empty 🤔 trying to adhere to the RFC fragment I addressed in this numeral.

  1. I believe that Passport::$defaultScope should follow the same data type as Passport::$scopes regarding of one of the reasons of what scopes are for: "displaying the description on the authorization approval screen"

Hope this helps 😃, if this PR is approved, I think that the only part where Laravel Passport docs may change is here Default Scope and also within an upgrade docs.

taylorotwell commented 1 year ago

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

xdiegom commented 1 year ago

Hi @taylorotwell,

Thanks for your feedback.

Just realized that I didn't understand the documentation about Default Scopes until now and either Passport::tokensCan() and Passport::defaultScope() must co-exists in the AuthServiceProvider in order to work, in other words Passport::setDefaultScope() can't be defined in AuthServiceProvider by itself unless Passport::tokensCan() is declared/defined before, which is something that in the Default Scopes documentation section does not say directly, it has to be inferred in the example and with knowledge of OAuth2, I guess.

I wanted to make it clear in order to definitely close this issue in case someone find this useful and maybe my solution could help to make it as a package or in future references for Laravel 🙌🏼