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

Only provide scopes when set in options #1053

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

barryvdh
Copy link
Member

@barryvdh barryvdh commented Dec 21, 2024

Partially reverts #1030
This will still allow to set a scope on the access token as array and format it properly, but it will not add the default scopes by default.

Setting the scope in the access token request is optional according to https://www.rfc-editor.org/rfc/rfc6749#section-3.3
In practice it seems to limit the scopes that are set in the authorization flow to a subset of the original scopes. But this is depending on the implementation.

Hopefully fixes #1052, #1051, RiskioFr/oauth2-auth0#28 Weble/ZohoClient#34

cc @sandervanhooft @liayn

For libraries needing to add default scopes to the access request, I would suggest something like this in your own provider:

public function getAccessToken($grant, array $options = [])
{
    if (empty($options['scope'])) {
        $options['scope'] = $this->getDefaultScopes();
    }
    
    return parent::getAccessToken($grant, $options);
}

Copy link

codecov bot commented Dec 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7a4e44d) to head (167763d).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##              master     #1053   +/-   ##
===========================================
  Coverage     100.00%   100.00%           
  Complexity       193       193           
===========================================
  Files             20        20           
  Lines            521       519    -2     
===========================================
- Hits             521       519    -2     
Files with missing lines Coverage Δ
src/Provider/AbstractProvider.php 100.00% <100.00%> (ø)

@matweew
Copy link

matweew commented Dec 23, 2024

I've tried your branch and it works good with Auth0.

@tm1000
Copy link

tm1000 commented Dec 23, 2024

Anyway to prioritize this getting merged?

@barryvdh
Copy link
Member Author

Anyway to prioritize this getting merged?

Not sure. I think only @ramsey can merge this. I'm not really sure about the impact but it seems the previous PR does break some cases.

@jamesmacwhite
Copy link

Not sure. I think only @ramsey can merge this. I'm not really sure about the impact but it seems the previous PR does break some cases.

It unfortunately breaks Google OAuth refresh tokens quite significantly. Any custom scope aside from the default provider ones that was originally requested on the initial token (which works) is lost on a refresh making the token essentially invalid for the API context it was originally requested for.

I can see this PR is kind of the happy medium between the original purpose of the original PR and keeping existing clients working. If it is not accepted, everyone who uses Google APIs through the oauth2-client provider would need to amend their provider class to handle the token side of things specifically. I'm not sure that's the best move.

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.8.0 breaks exiting scope handling
4 participants