Security issue: Loadable users are always authenticated

[sandermarechal]

The current implementation to refresh JWT tokens is not safe. When a user presents a refresh token, the only check that occurs is whether the user provider can load the user. If a user is loadable, it is granted a new access token.

But the authenticator should also check if the loaded user is still allowed to access the application. For example, users that implement the AdvancedUserInterface should be checked whether their account is enabled, is not locked and is not expired. Other means of authentication may have other requirements (e.g. for OAuth accounts, check whether the correct permissions are still present).

Currently, this bundle will happily give out new access tokens to users who have been disabled, expired, etc.

I think there should be some way to hook into the authenticator to provide this logic. Or perhaps the authenticator in this bundle should delegate authentication to some other authentication provider.

Currently I am using a compiler pass to overwrite the authenticator provided in this bundle with my own. That's a bit of a hackish solution.

[YaniceSC]

having the same issue.
also, we should consider that this security point has to be handled by LexikJWTAuthenticationBundle.
See the issue lexik/LexikJWTAuthenticationBundle#193

[markitosgv]

I have no time to check this now, but you can do a PR solving this, thanks!

[sandermarechal]

@YaniceSC It should not be handled by lexik/LexikJWTAuthenticationBundle#193. JWT access tokens are short-lived and are assumed to be valid until they expire, much like a session for example. A refresh token is long lived. It's credentials should be checked every time it is used, much like a "Remember me" cookie or a password.

Using a JWT refresh token is like logging in. Using a JWT access token is like continuing with a logged-in session. Symfony itself does not trigger a full re-authentication when you have a logged-in session, so neither should LexikJWTAuthenticationBundle.

[sandermarechal]

@markitosgv It's not an easy fix. You'll need to think how refresh tokens should interact with the rest of the Symfony authentication system. I think that will require redesigning the entire bundle. The core of the problem is that currenty, a refresh token is the authentication, but a refresh token should just be a way to tell you which user to authenticate, and then have some other part of the stack handle the authentication.

[YaniceSC]

@sandermarechal , ok I got it.
also, what do you suggest to fix it at the moment ? I doubt the bundle will be redesigned easily...
On my end, I used the solution given in lexik/LexikJWTAuthenticationBundle#193 but I understand that need to be a temporary fix.

[sandermarechal]

In my application I use a DI compiler pass to overwrite this bundle's RefreshTokenAuthenticator with my own implementation. It's dirty, and only works my my specific auth setup. Something like this:

class RefreshTokenAuthenticatorPass implements CompilerPassInterface
{
    public function process(ContainerBuilder $container)
    {
        if (!$container->has('gesdinet.jwtrefreshtoken.authenticator')) {
            return;
        }

        $definition = $container->findDefinition('gesdinet.jwtrefreshtoken.authenticator');
        $definition->setClass(MyCustomRefreshTokenAuthenticator::class);
        // Add your dependencies here
    }
}

and

class MyCustomRefreshTokenAuthenticator extends RefreshTokenAuthenticator
{
    public function authenticateToken(TokenInterface $token, UserProviderInterface $userProvider, $providerKey)
    {
        $token = parent::authenticateToken($token, $userProvider, $providerKey);
        $user = $token->getUser();
        // Perform custom auth with the user
    }
}

I'd have to think about a proper way to do this generically.

[sandermarechal]

I think I found a proper way to handle this: Use Symfony's UserChecker. That seems to be the right entry point where Symfony checks for non-enabled or locked accounts. It's also a good point where people can inject their own weird authentication verification (e.g. communicate with an OAuth provider or other SAAS backend).

I will prepare a PR.

[sandermarechal]

@markitosgv this security issue is still valid for the latest release of this bundle. Why was it (and the associated pull request) closed? Was it fixed some other way?