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

Reviewed authenticator and made refreshToken method public. #1831

Merged
merged 1 commit into from
Dec 10, 2021

Conversation

gassan
Copy link
Contributor

@gassan gassan commented Dec 4, 2021

With the new authentication manager the access token could not be refreshed by a request listener without copying part of the code from the OAuthAuthenticator. I had reviewed authenticator and:

  1. made a public method refreshToken. Using DI I can now pass authenticator in every function/listener, get new token and replace them in token storage.
  2. Because we already have in authenticate() function a ready to use OAuthToken, I have extended SelfValidatedPassport and pass token to them instead of a UserBadge.
  3. It is also possible to implement a kind of RememberMe function but with an OAuthRememberMeToken. You can store resource owner name and refresh_token with unlimited lifetime (usually you can get it with scope offline) in the db and get a new access token every time without asking credentials. OAuthRememberMeToken is the same like OAuthToken (will expired, refreshed etc), but has an string $series property, which must be always copied from the expired token to a new one, that's why I have permitted myself to add copyPersistentDataTo(self $token): void to OAuthToken.

rememberme

Feel free to ask or suggest something.

@stloyd
Copy link
Collaborator

stloyd commented Dec 4, 2021

Tests fail due to Symfony 6, most of it is covered with #1800, but still cannot find a way to re-enabled sessions in test env in that version.

@gassan
Copy link
Contributor Author

gassan commented Dec 5, 2021

@session was removed from container in symfony6. You have to handle it like in KernelBrowser::loginUser()

@gassan gassan force-pushed the reviewed-authenticator branch 2 times, most recently from b95896d to 865aedf Compare December 7, 2021 10:10
@stloyd
Copy link
Collaborator

stloyd commented Dec 7, 2021

@gassan Requires one more rebase (sorry ;)).

Security/Http/Authenticator/OAuthAuthenticator.php Outdated Show resolved Hide resolved
Security/Http/Authenticator/OAuthAuthenticator.php Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
HWIOAuthBundle.php Outdated Show resolved Hide resolved
Security/Core/Authentication/Token/AbstractOAuthToken.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@stloyd stloyd left a comment

Choose a reason for hiding this comment

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

@gassan Just please squash your commits into one :)

…hToken function public, so we can refresh access token from outside (Listener), login to oauth instance with a LoginForm - 'password' grand, etc
@gassan
Copy link
Contributor Author

gassan commented Dec 10, 2021

its done with rebasing on your latest master commit. thx

@stloyd stloyd merged commit ab989d9 into hwi:master Dec 10, 2021
@stloyd
Copy link
Collaborator

stloyd commented Dec 10, 2021

Great! Thank you @gassan for this changes & for support in testing other mine changes!

@gassan gassan deleted the reviewed-authenticator branch December 10, 2021 18:15
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.

Please check whether that four values should not be stored in passports attributes?
2 participants