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

Add default scopes to issued token #1679

Closed
wants to merge 4 commits into from
Closed

Conversation

xdiegom
Copy link

@xdiegom xdiegom commented Jul 24, 2023

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
Copy link
Member

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
Copy link
Author

xdiegom commented Jul 24, 2023

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 🙌🏼

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.

2 participants