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

AbstractProvider - Replace 'AccessToken' dependency with 'AccessTokenInterface' to support cases where AccessToken class is being extended. #897

Open
cloudcogsio opened this issue Aug 2, 2021 · 3 comments
Milestone

Comments

@cloudcogsio
Copy link

cloudcogsio commented Aug 2, 2021

A fatal error is thrown for the following case:

  1. The concrete class League\OAuth2\Client\Token\AccessToken is extended by a custom provider. Lets call this 'CustomAccessToken'

  2. The custom provider overrides the 'createAccessToken' method to return the 'CustomAccessToken' class instead of the League\OAuth2\Client\Token\AccessToken. (There should be no issue since base functionality is extended and implements 'AccessTokenInterface'

  3. Methods such as 'getResourceOwnerDetailsUrl' will throw a fatal exception since it's declaration, although abstract, uses the concrete 'League\OAuth2\Client\Token\AccessToken' instead of the better suited 'AccessTokenInterface'

Fix:
Replace all occurrences of 'League\OAuth2\Client\Token\AccessToken' with 'League\OAuth2\Client\Token\AccessTokenInterface' in the abstract methods of AbstractProvider.php

Changed in:
cloudcogsio@07dd41c

cloudcogsio added a commit to cloudcogsio/oauth2-keycloak that referenced this issue Aug 3, 2021
- Extend the League AccessToken class
- NOTE: Requires a change in the League's AbstractProvider class.
- see thephpleague/oauth2-client#897
@cloudcogsio
Copy link
Author

I've added a new Keycloak OAuth2 client with a branch that implements a use case for extending the base AccessToken.

See https://github.com/cloudcogsio/oauth2-keycloak/blob/master/README.md#custom-access-token-class

@ramsey ramsey added this to the v3 milestone Dec 22, 2021
@ramsey
Copy link
Contributor

ramsey commented Dec 22, 2021

Adding this for consideration to our v3 milestone. Thanks!

@pkly
Copy link

pkly commented Feb 22, 2022

I would also like to see this happen, after updating some stuff and increasing the phpstan level to 7 suddenly I started getting a bunch of errors related to the facebook provider which takes an AccessToken object for getResourceOwner even though it's a perfect use-case for an interface.

What I would suggest instead is making the declaration for the abstract provider one that's based on the interface, but have the providers themselves tighten the type via @method annotations or overriding methods (safer).

That way one can't pass a twitter access token to facebook, for example, but the specific providers would still return some sort of token. One could create a dummy class, that'd simply extend AccessToken (from abstract) to create more specific return type, and then simply use said class in the provider itself.

hemberger added a commit to hemberger/smr that referenced this issue Jun 9, 2022
The base classes for League\OAuth2 loosen the return types in the
abstract interfaces used by the providers (Google, Facebook, etc.),
but we need the more specific types.

See thephpleague/oauth2-client#897.
hemberger added a commit to hemberger/smr that referenced this issue Jul 5, 2022
Providers are expected to get an AccessToken, but are only given an
AccessTokenInterface.

Since the providers we use don't extend AccessToken, this is only a
theoretical problem, but it does trigger a static analysis warning.

See thephpleague/oauth2-client#897.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants