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

Added new Oauth2Authenticator using the new Symony 5.2 Authenticator … #292

Merged
merged 1 commit into from
Mar 23, 2021

Conversation

Anthodev
Copy link
Contributor

…interface

First draft for the implementation of the new authenticator interface, not sure that's really usable at the moment.

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there!

thanks for getting this started! I haven’t tested with it yet, but here’s a quick review to get things started. :).

Cheers!

composer.json Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Security/Authenticator/OAuth2Authenticator.php Outdated Show resolved Hide resolved
src/Security/Authenticator/OAuth2Authenticator.php Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments!

README.md Show resolved Hide resolved
src/Security/Authenticator/OAuth2Authenticator.php Outdated Show resolved Hide resolved
src/Security/Authenticator/OAuth2Authenticator.php Outdated Show resolved Hide resolved
@Anthodev
Copy link
Contributor Author

@weaverryan done :)

Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're close - check out the test failures - I just commented about one. Another is related to needing to run vendor/bin/php-cs-fixer fix locally to fix some phpcs stuff :)

tests/Security/Authenticator/Oauth2AuthenticatorTest.php Outdated Show resolved Hide resolved
@Anthodev
Copy link
Contributor Author

Finally done :D

@weaverryan weaverryan merged commit b9e474b into knpuniversity:master Mar 23, 2021
@weaverryan
Copy link
Member

Thank you for your hard work @Anthodev!

@Anthodev
Copy link
Contributor Author

Anthodev commented Mar 23, 2021

It was too sloppy for my taste, i'll do better next time :p

Thanks anyway @weaverryan ^^

@fcaraballo
Copy link

Thank you for this @Anthodev !

@Anthodev
Copy link
Contributor Author

Thank you @fcaraballo :)

@fliespl
Copy link

fliespl commented Apr 22, 2021

It's cool example, but how about moving a little further?

Currently it assumes you just create account based only on access_token. Obviously most oauth2 servers will also return refresh token which is worth saving on user entity/document to be able to refresh access token in seperate logic.

With this implementation only access token is avaiable in Badge context.

Also I had to make change to:
$facebookUser = $this->clientRegistry->getClient('facebook_main')->fetchUserFromToken($credentials);

and replace it with:

$facebookUser = $this->clientRegistry->getClient('facebook_main')->fetchUserFromToken(new AccessToken(['access_token' => $credentials]));

That's because first argument of UserBadge expects string (which is taken from AccessToken toString) and fetchUserFromToken method expects and AccessToken object.

@weaverryan
Copy link
Member

Good points @fliespl - WDYT about #302?

@fliespl
Copy link

fliespl commented Apr 26, 2021

@weaverryan left comment to PR.

weaverryan added a commit that referenced this pull request Apr 26, 2021
This PR was squashed before being merged into the master branch.

Discussion
----------

Fixing access token, which becomes a string

Renamed $credentials -> $accessToken for clarity.

Also, use $accessToken from the original authenticate() method, instead of the argument that's passed to the callback, because that will have been cast into just the access token string.

See: #292 (comment)

Commits
-------

388f39f Fixing access token, which becomes a string
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.

4 participants