Revert BC break by only providing scopes in access token when set in options

[barryvdh]

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);
}

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (7a4e44d) to head (167763d).
Report is 4 commits behind head on master.

Additional details and impacted files

@@             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]

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

[tm1000]

Anyway to prioritize this getting merged?

[barryvdh]

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]

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.

[tm1000]

@ramsey can this be merged into a 2.8.1 release please?

[ramsey]

Thanks for pinging me, and sorry for the delay. I didn't see this! 😳

I'll try to merge and get out a release tonight (my time).

[sandervanhooft]

Thanks @ramsey, looking forward to it.

[tm1000]

@ramsey I feel like I'm paying for my sins of ignored PRs because of how busy I know you are! Just reminding you.

[redwardh]

Thank you for providing this fix to 2.8.0. Saved a world of hurt with Google OAuth for two projects I maintain.

[it-can]

Any news on this?

[tm1000]

@ramsey i realize we all get busy but this just hit me again and for the sake of not having to pin every repo I work on an update would be appreciated

[Ugoku]

@ramsey can you please merge this? This is holding back mollie/oauth2-mollie-php#35, a major payment provider in the Netherlands, which in turn is holding us back from upgrading to PHP 8.4

[nathanmay]

is @ramsey the only one who can merge PR's? Has anyone been able to contact him?

[tm1000]

@nathanmay I emailed him. Havent heard back.

If I recall correctly @frankdejonge is the owner of @thephpleague itself

[frankdejonge]

I'll check if I can contact somebody for this.

[barryvdh]

Just to clarify; this is not a new feature. This is reverting a BC break back to as it was in 2.7.

But it's and edge-case because it only happens with custom scopes, and depends on the provider how they handle it. So not really clear how often issues occur, only that multiple clients have already reported issues.
And the longer we wait with merging this (or reverting #1030 entirely), more issues will occur with people relying on the new behavior.

[liayn]

This should be merged ASAP. I can only second @barryvdh here.

Sorry, but I need to express this:
I'm actually a bit disappointed that one of the most important authentication libraries in the PHP field gets so little love by the maintainers. At least from our perspective, OAuth-based authentication is currently the one and only authentication protocol in the B2B area, replacing anything else over time. It's the foundation of state-level authentication (ID Austria, for instance). So seemingly this package slowly can be found "everywhere".
I'd really wish for an active person with merge and release permissions here.
😐

[frankdejonge]

@liayn and @tm1000 while I understand where you're coming from, members of this organisation have spent countless unpaid hours for the (financial or other) benefit of others. Guilt-tripping volunteers into action isn't going to get you very far and I advice you to stop doing it.

That all said, I've reached out to Ben and put in a request (not demand) to look into this. I'd like to ask everybody to remain respectful and cognisant of this project is not funded and only exists because people have graciously donated their free time to you.

[frankdejonge]

A direct response to @tm1000 on your comment I'll just keep trying people till it gets merged. Doing so will result in a ban from this organisation, this behaviour is not accepted.

[liayn]

@frankdejonge Since you tagged me directly: Please do not assume anything about me. You can be sure I spent my fair share of (unpaid) hours too for open source. So please don't blame/shame in the other direction here, thanks. Let's declare this discussion finished here.

Besides that:

I've reached out to Ben and put in a request ...

Thank you!

[tm1000]

All good this will be my last response. I'll fork this into packagist.org and just maintain it that way. Didn't mean to bother anyone from an unintentionally released bug. Just want the bug fix merged since it was fixed weeks ago and the last statement from a maintainer was "I'll try to look at it tonight"

Great work though (sincere I do really mean that) on this project and all the php league has done and continues to do. Y'all are really dedicated and hard workers

See ya around!

[frankdejonge]

@liayn what did I assume about you?

[barryvdh]

Thanks for merging!

[sandervanhooft]

Big thanks!

[liayn]

Thanks a lot, also from my side!

@frankdejonge I actually don't want to trigger a lengthy public discussion here. I understood your words as a direct blame on me
"... Guilt-tripping ...", hence my reaction. My words literally were written in "I"-perspective and simply were an assessment of the situation in my perception. Fact is that there hasn't been a single commit/merge between Apr and Dec 2024. While that might be totally okay for some side-level library, it seems to me that this very library is not that kind of library. It's a (the?) foundation for a lot of authentication systems out there, so I feel it deserves some love. (It has some log4j character, if you know what I mean.)
I fully understand that time capacity is very limited on all ends, I would propose to turn the stick around and simply ask for new people to help out. (Disclaimer: I have absolutely no idea how "thephpleague"-circle is treating stuff like that and what the "rules" are.) 🤝

So far, I'm happy we got this bug sorted out and I'm sorry for all those, who had to invest support time in their integrations for finding out that this was the issue. The details are described extensively here and in the associated issue.

[barryvdh]

I see a discussion was already started in #1041

I would suggest that if anyone feels they want to take on more responsibility, they let that know in that issue, so that Ramsey/PHPLeague leadership can take that into account.

[ramsey]

I’m sorry for all the trouble I’ve caused everyone who uses this library. I will be stepping down as a maintainer so that someone else can take it over.

[barryvdh]

I’m sorry for all the trouble I’ve caused everyone who uses this library. I will be stepping down as a maintainer so that someone else can take it over.

Don't worry about it @ramsey , we're all just trying to do our best.

[liayn]

@ramsey This is sad to hear. As I wrote above: I wish for additional manpower, not less.

How does the process at PHPLeague work to add new maintainers to a project?

[sandervanhooft]

Sad to hear this Ramsey, but I applaud you for all the effort you've put in here. This package adoption is wild, also making it vital to many daily interactions. Must have been stressful at times to maintain this.

I am curious where Ramsey stepping down is going to lead us. I certainly don't feel equipped to step in here as a maintainer.

Yet I was thinking on a more practical note:

Would it have helped if by default minor releases (major.minor.patch) are by default preceded by a release candidate and let's say a 4 week time window until tagging the production ready release?

Some driver maintainers would have to adopt the same practice in order to get some feedback from actual usage but it may reduce the risk and stress involved.

[sandervanhooft]

Some driver maintainers would have to adopt the same practice in order to get some feedback from actual usage but it may reduce the risk and stress involved.

For as long as I am/my team is maintaining the Mollie driver I am willing to do this.

[barryvdh]

There probably aren’t that many changes so I suggest that drivers enable watch notifications on issues/pull requests here and keep track of merged items, maybe enable testing with a scheduler on the dev-master branch also.

But it’s difficult because obviously there are never meant breaking issues in minor releases and probably no existing tests would have caught it on the drivers side, so maybe just mark this as unlucky.

Also, the PR causing the issue had been open for over 6 months and only had 1 review, which suggest it fixed an issue. So definitely not something @ramsey could have suspected.

[sandervanhooft]

Thanks @barryvdh !

I can work with daily testing the driver against dev-master, good thinking.

And yes, this definitely is the "unlucky" category. 😅

[frankdejonge]

Just an FYI, I'm currently trying to get one of our existing maintainers to adopt the package. They have experience with OAuth2, but that doesn't mean they are willing to take this on of course.