-
Notifications
You must be signed in to change notification settings - Fork 159
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
New authenticator for the newer Symfony authenticator API #246
Conversation
Event/RefreshEvent.php
Outdated
|
||
public function __construct(RefreshTokenInterface $refreshToken, PostAuthenticationGuardToken $preAuthenticatedToken) | ||
public function __construct(RefreshTokenInterface $refreshToken, TokenInterface $token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a type widening change, only the Symfony\Component\Security\Guard\Token\PostAuthenticationGuardToken
token was accepted but with this PR the interface is allowed.
} | ||
|
||
public function getRefreshToken() | ||
{ | ||
return $this->refreshToken; | ||
} | ||
|
||
/** | ||
* @deprecated use getToken() instead | ||
*/ | ||
public function getPreAuthenticatedToken() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deprecation since the token being retrieved here isn't guaranteed to be a pre-authentication, or post-authentication, token with the interface typehinted.
0c55fce
to
caf4832
Compare
f375565
to
73efd36
Compare
73efd36
to
4d67b5f
Compare
thanks @mbabker I need some time to review it |
4d67b5f
to
0e9235f
Compare
@mbabker could you please resolve conflicts? thanks! |
0e9235f
to
23f4fa6
Compare
Rebased |
... actually, looking at the diff, something fell out of sync between computers doing the rebase on this. I'll have to fix it when I get home later. |
8e6e150
to
1010b69
Compare
1010b69
to
cf34c9d
Compare
OK, everything's good here. |
This is an implementation of an authenticator supporting the newer Symfony API. It's very loosely based on a similar authenticator I wrote for one of our applications. In theory, this should make setting up the firewall config a bit simpler, and allows deprecating the existing authentication path (namely
Gesdinet\JWTRefreshTokenBundle\Service\RefreshToken
and the existingGesdinet\JWTRefreshTokenBundle\Security\Authenticator\RefreshTokenAuthenticator
).The intent is that a user can enable the authenticator for their refresh endpoint in a single spot, the firewall configuration:
Which means the user doesn't have to define a controller for the refresh endpoint anymore (hence, the potential to deprecate
Gesdinet\JWTRefreshTokenBundle\Service\RefreshToken
). This also means this bundle no longer has to manage the authentication flow on its own in a controller well after the Symfony authentication system has processed, it runs natively as a part of the core authenticator manager.The authenticator is self-contained and goes through all the steps that the existing service and authenticator go through now regarding decoding the token, loading the token from storage and validating it, and refreshing the TTL when configured; all of this is now in a single class. Because there isn't a "nice" way to pass along the decoded refresh token from the
authenticate()
method to theonAuthenticationSuccess()
method, a custom authentication token is used as a middle layer for passing the refresh token through the authentication system without having to constantly re-query it.This also decouples from the
LexikJWTAuthenticationBundle
failed authentication flow in favor of similar events and response handling native to this bundle, but better scoped to this bundle (so a downstream app can catch the new fail events from this bundle and handle that separately from a failure in the main JWT auth workflow). The success handler is still used.With the current implementation, this also helps in addressing #242 by identifying configuration that might need to be adjusted on a per-firewall basis. Unfortunately, there are still some code paths (especially with the
LexikJWTAuthenticationBundle
authentication success handler) that need a more in-depth review/refactor to make this one work out well.TODO