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

New authenticator for the newer Symfony authenticator API #246

Merged
merged 1 commit into from
Jul 1, 2021

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Jun 10, 2021

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 existing Gesdinet\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:

security:
    enable_authenticator_manager: true

    firewalls:
        api_token_refresh:
            pattern: ^/api/token/refresh
            stateless: true
            user_checker: App\Security\UserChecker
            refresh_jwt: ~

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 the onAuthenticationSuccess() 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

  • Tests
  • Documentation
  • Deprecations


public function __construct(RefreshTokenInterface $refreshToken, PostAuthenticationGuardToken $preAuthenticatedToken)
public function __construct(RefreshTokenInterface $refreshToken, TokenInterface $token)
Copy link
Contributor Author

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()
Copy link
Contributor Author

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.

@mbabker mbabker force-pushed the new-authenticator branch 6 times, most recently from f375565 to 73efd36 Compare June 22, 2021 23:44
@mbabker mbabker changed the title [WIP] New authenticator for the newer Symfony authenticator API New authenticator for the newer Symfony authenticator API Jun 23, 2021
@mbabker mbabker marked this pull request as ready for review June 23, 2021 00:01
@markitosgv
Copy link
Owner

thanks @mbabker I need some time to review it

@markitosgv
Copy link
Owner

@mbabker could you please resolve conflicts? thanks!

@mbabker
Copy link
Contributor Author

mbabker commented Jun 29, 2021

Rebased

@mbabker
Copy link
Contributor Author

mbabker commented Jun 29, 2021

... 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.

@mbabker mbabker force-pushed the new-authenticator branch 3 times, most recently from 8e6e150 to 1010b69 Compare June 29, 2021 23:52
@mbabker
Copy link
Contributor Author

mbabker commented Jun 29, 2021

OK, everything's good here.

@markitosgv markitosgv merged commit 3a84970 into markitosgv:master Jul 1, 2021
@mbabker mbabker deleted the new-authenticator branch July 1, 2021 12:00
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.

2 participants