Send scope when requesting access tokens

[liayn]

Resolves: #1029

[DaveWoodCom]

Thanks @liayn, this fixed the issue I was having generating OAuth tokens using an identity provider from PingIdentity.

[ramsey]

Thank you for contributing! 🎉

[barryvdh]

Is this a breaking change?
#1052
#1051
RiskioFr/oauth2-auth0#28
https://github.com/mollie/oauth2-mollie-php/releases/tag/v2.8.2
Cc @sandervanhooft

[barryvdh]

I think the problem is that the scope is usually only set in getAuthorizationUrl(). So with custom scopes:

Only default scope:

• getAuthorizationUrl sends default scope
• getAccessToken sends default scope
  Result: Scope is either not used in access token or still the same

Custom scope:

• getAuthorizationUrl sends custom scope
• getAccessToken sends default scope
  Result: Scope is either not used in access token or the integration resets the scope to the new default scope level, breaking the access

So either the integration needs to remember the tokens, or not send them. The best solution would probably be to only send the token if explicitly set, so not the default?

[barryvdh]

Something like this (untested)

    public function getAccessToken($grant, array $options = [])
    {
        $grant = $this->verifyGrant($grant);

        if (isset($options['scope']) && is_array($options['scope'])) {
            $separator = $this->getScopeSeparator();
            $options['scope'] = implode($separator, $options['scope']);
        }

[liayn]

It's always hard to define what is breaking and what not.
My initial report was made, because it was an existing bug, that no scopes were sent and there was no possible workaround.

If this now breaks for others this basically means that this worked by accident, and their default scopes where incorrect.
At least this is what I understood from reading the referenced issues.

To my (maybe limited) knowledge a lot of IdPs require the scopes to be sent together with the access token request.
And the library usually falls back to the default scopes, if none where specified explicitly. (that's what defaults are there for, aren't they?)

[sandervanhooft]

Thanks for looking into this. I don't have availability this weekend for a deep dive, perhaps a bit on Monday. 🤞

Given the potential severity and the time of the year 🧑‍🎄 I'd suggest to revert (part of) last release if details are unclear at the moment. On mollie we've shipped a temporary patch but I can imagine the impact is broader seeing other issues being reported here.

[liayn]

Given the potential severity

I don't see the criticality yet. Until now I only see that people need to get their scopes right, which should have been correct ever since. For us this allowed to remove useless code: xperseguers/t3ext-oidc@38d7dfb

As noted here it is only possible to get the default scopes from the GenericProvider, because it "violates" the visibility of the AbstractProvider.
This is of course not the case for other providers, like AzureProvider.

But of course, I'm curious if this caused other issues besides wrong default scopes.

[barryvdh]

If this now breaks for others this basically means that this worked by accident, and their default scopes where incorrect. At least this is what I understood from reading the referenced issues.

As far is I can tell, it's not about the default scopes. People using the default scopes should be fine, but when using custom scopes, they will be lost, depending on the implementation of the server.

As far as I can tell, the Authorization URL defines the scopes and informs the user. The token request optionally allows the scope.
https://www.rfc-editor.org/rfc/rfc6749#section-3.3

The authorization and token endpoints allow the client to specify the
scope of the access request using the "scope" request parameter. In
turn, the authorization server uses the "scope" response parameter to
inform the client of the scope of the access token issued.

But a token request cannot inform the user, so it can cannot exceed the original scope tied to the authorization/refresh token.

See the official examples, that are only mentioning the custom scopes for the getAuthorizationUrl

https://github.com/thephpleague/oauth2-github?tab=readme-ov-file#managing-scopes
https://github.com/thephpleague/oauth2-google?tab=readme-ov-file#scopes
https://github.com/thephpleague/oauth2-linkedin?tab=readme-ov-file#managing-scopes
https://github.com/thephpleague/oauth2-instagram?tab=readme-ov-file#managing-scopes

I think the most common use of the token in the accessToken request is to limit the access to a subset. If not received it should fall back to the scopes granted in the Authorization URL.

To my (maybe limited) knowledge a lot of IdPs require the scopes to be sent together with the access token request. And the library usually falls back to the default scopes, if none where specified explicitly. (that's what defaults are there for, aren't they?)

I find this hard to believe, as this library has worked for hundreds of libraries before? I do see your linked documentation in #1029 where https://learn.microsoft.com/en-us/entra/identity-platform/v2-oauth-ropc#authorization-request describes this as Recommended.

In my opininion, the Entra library should provide the default scopes if these are required, but it should not be done for all libraries and revert this.

[barryvdh]

Also see the note for the Authorization Flow where it explicitly states that the usage op de scope parameter here is Microsoft extension to the authorization code flow (so not according to spec)

https://learn.microsoft.com/en-us/entra/identity-platform/v2-oauth2-auth-code-flow#request-an-access-token-with-a-client_secret

scope | optional | A space-separated list of scopes. The scopes must all be from a single resource, along with OIDC scopes (profile, openid, email). For more information, see permissions, consent, and scopes. This parameter is a Microsoft extension to the authorization code flow. This extension allows apps to declare the resource they want the token for during token redemption.

[liayn]

Thanks for the detailed explanation, I had a deeper look now. The patch was created a long time ago, so I had to dig into things again.

See the official examples, that are only mentioning the custom scopes for the getAuthorizationUrl

Indeed an interesting pointer I wasn't aware of. But honestly, this reads like a workaround/not intended usage of API to me, since the idea of the default scopes is exactly to contain the scopes used for the authorization URL. See 1 and 2

So I summarize for me:

• My fix made authentication work with at least Microsoft and PingIdentity, using default scopes obviously.
• My fix, accidentally, broke those "custom scope" usages, which did not initialize the default scopes.
  (And possibly cases, where default scopes were set, but others were actually used for the authorization URL)
• Official implementations do not use the intended API (default scopes) at all.
• GenericProvider exposes the default scopes through a public getter, overriding the visibility as defined in the abstract class. Reason: unknown to me

Conclusions for me, if #1053 removes the default scopes again:

• I need to patch the providers for Microsoft again, in order to retrieve the default scopes, such that I can provide the scopes to the accessToken function again.
• I can't keep things DRY, as I need to specify scopes in more than one place.
• Every other provider, requiring scopes to be passed for token acquisition, suffers from the same issue.

Probably the most optimal solution would be to:

• use default scopes only for authorization url retrieval, unless custom scopes are defined
• remember the scopes used to generate the authorization url and use them by default to retrieve tokens, unless a more specific (narrowed down) set of scopes is passed via options
• clarify documentation in regards to scopes, both, for users of the library and for developers of providers

Please let me know if my assessment is correct, or if I missed something. Thanks.

[barryvdh]

Not sure what you mean by this:

Official implementations do not use the intended API (default scopes) at all.

I do agree that the usage of the scope option is not mentioned in the default docs, but my linked examples are official integrations. And the example from 1 that you use, seem to imply that you definitely should be able to set your own scopes.

I've looked more closely at your example and I see you use an abstraction there, but your workaround seemed okay to me.
We can disagree about the optimal default case, but your suggested solution is a breaking solution to existing users and should require a SemVer bump because the accessToken should be requested differently. So that should be done in a new major version with a heavy disclaimer imho,

[liayn]

tl;dr I never intended a breaking change! Yes, lets fix this in the best way possible.

Long version:

Not sure what you mean by this:

Official implementations do not use the intended API (default scopes) at all.

I mean that the examples do not make use of the "default scopes" feature. Upon init of the providers the "scopes" are not set via the constructor options.

but your suggested solution is a breaking solution to existing users and should require a SemVer bump because the accessToken should be requested differently. So that should be done in a new major version with a heavy disclaimer imho,

Of course! As I said, this was really not meant as a breaking change, by any means!
The change must be modified in a way that this is not breaking. I did not realize this back in May and obviously the reviewers did not either. Well, sh** happens.

My goal is to fix the breaking issue and to still preserve my intended functionality as well, in a modified form, since I learned a lot about different usages throughout this conversation.

From a dev's POV: Using a provider should be as simple as to set the intended scopes with the constructor options (aka "default scopes") and not care about them any further.
For devs of the providers themselves: Most of the basic functionality should be handled in the AbstractProvider, in order to avoid mistakes and useless code repetition. In the concrete case of Microsoft (and PingIdentity? and others?): I don't see it necessarily an obligation of these specific providers to send the scopes again with the access token request.

I've looked more closely at your example and I see you use an abstraction there

I presume you refer to https://github.com/xperseguers/t3ext-oidc/. Correct, this is the integration of OpenID Connect in the authentication system of TYPO3. The concrete provider actually used is not controlled by our integration. We offer the GenericProvider by default for ease of use, but TYPO3 site developers may load any other provider. The developer does (of course) not interact directly with this library here, otherwise t3ext-oidc would be pointless. => This library and the providers need to work seamless, obey to standards and to requirements made by the the actual IdP.

[barryvdh]

So ZoHo Weble/ZohoClient#34 and google #1052 (comment) also seem to be affected, and Mollie was already reported