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

Add new jwt authenticator for Symfony 5.3+ Security system #872

Merged
merged 2 commits into from
Jun 23, 2021

Conversation

TristanPouliquen
Copy link
Contributor

@TristanPouliquen TristanPouliquen commented Jun 8, 2021

Working on implementing the new Authenticator system with Passports & Badges in LexikJWT.
Fixes #874
Fixes #866
Fixes #810

@TristanPouliquen
Copy link
Contributor Author

Added unit tests to my new authenticator.

The only point that frustrates me is being forced to inject the UserProvider & store the payload as a property of the class to be able to pass them down to the loadUser method that calls the loadXXXAndPayload methods.

I didn't find a way to work around this issue and rely simply on the UserProvider structure & pass only the identifier to the UserBadge to let the magic work it all!

If anyone has an idea, I'm open to it :)

@chalasr
Copy link
Collaborator

chalasr commented Jun 8, 2021

@TristanPouliquen Thank you very much for this PR! You just made my day :)
I can't review it right now but I'll be back at the very end of the week to hopefully help moving forward as quickly as possible.

@TristanPouliquen
Copy link
Contributor Author

@chalasr Np! Still fighting with getting it to pass all the different CIs & make it backwards compatible though

@mbabker
Copy link
Contributor

mbabker commented Jun 10, 2021

The only point that frustrates me is being forced to inject the UserProvider & store the payload as a property of the class to be able to pass them down to the loadUser method that calls the loadXXXAndPayload methods.

I didn't find a way to work around this issue and rely simply on the UserProvider structure & pass only the identifier to the UserBadge to let the magic work it all!

If anyone has an idea, I'm open to it :)

I bounced this PR around in one of my apps yesterday to see if there was a way to just use Symfony's authenticator manager without the authenticator needing to be aware of the user provider, and I couldn't come up with anything. It's basically stuck like this thanks to PayloadAwareUserProviderInterface and the authenticator manager not passing any information from the passport into the user loader.

@mbabker
Copy link
Contributor

mbabker commented Jun 10, 2021

make it backwards compatible though

One of the biggest things that'll help will be to check if loadUserByIdentifier exists on the provider and fall back to the legacy loadUserByUsername. Similar to this check in the UserProviderListener.

@TristanPouliquen TristanPouliquen force-pushed the feature_new_authenticators branch 2 times, most recently from 68c2176 to d1cfc68 Compare June 11, 2021 07:57
@TristanPouliquen
Copy link
Contributor Author

Tests are passing on all scenarii except for PHP8 & SF5.x-dev => composer does not seem to be able to resolve a correct set of dependencies.

Someone has any idea of why?

@TristanPouliquen
Copy link
Contributor Author

Tests are passing on all scenarii except for PHP8 & SF5.x-dev => composer does not seem to be able to resolve a correct set of dependencies.

Someone has any idea of why?

In the meantime, I've switched it to a more reasonable 5.3.x-dev to test against the latest SF5.3

@chalasr
Copy link
Collaborator

chalasr commented Jun 11, 2021

Sounds good 👍

@mbabker
Copy link
Contributor

mbabker commented Jun 11, 2021

Someone has any idea of why?

There isn’t a 5.x branch anymore, so it can always use a version string and work right.

@TristanPouliquen TristanPouliquen changed the title WIP - Security Authenticators SF5.X Security Authenticators SF5.X Jun 11, 2021
@TristanPouliquen
Copy link
Contributor Author

What's the next step on this PR now? :)

Copy link
Collaborator

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Nice start! This misses registering the authenticator as a service (via a factory). I'm going to address my own comments and add what's missing if you don't mind.

Tests/Functional/CompleteTokenAuthenticationTest.php Outdated Show resolved Hide resolved
* @return array|false The JWT token payload or false if an error occurs
* @throws JWTDecodeFailureException
*/
public function decodeFromJsonWebToken(string $jwtToken);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding a new method to an interface is a BC break.
To avoid that, we need to introduce that method as "virtual" first using the @method phpdoc annotation. Then, in 3.0, we will remove the phpdoc and add the real method.
Here is an example from Symfony core for inspiration: https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/PasswordHasher/Hasher/UserPasswordHasherInterface.php#L21

Security/User/PayloadAwareUserProviderInterface.php Outdated Show resolved Hide resolved
@chalasr chalasr force-pushed the feature_new_authenticators branch 2 times, most recently from 81c2f28 to f27d469 Compare June 19, 2021 17:42
@chalasr chalasr force-pushed the feature_new_authenticators branch 5 times, most recently from 1e86bb9 to 0f395a5 Compare June 20, 2021 09:13
@chalasr chalasr force-pushed the feature_new_authenticators branch from 0f395a5 to c503ce9 Compare June 20, 2021 09:26
@chalasr
Copy link
Collaborator

chalasr commented Jun 20, 2021

Now green and deprecation-free! (well, one remaining notice but purely testing/internal, should not impact developers).
Here is how this PR should be used.

Before:

# config/packages/security.yaml
security:
   # ...
    firewalls:
        api:
            pattern:  ^/api
            stateless: true
            guard:
                authenticators: 
                    - lexik_jwt_authentication.jwt_token_authenticator

After:

# config/packages/security.yaml
security:
    enable_authenticator_manager: true
    # ...
    firewalls:
        api:
            pattern:  ^/api
            stateless: true
            jwt: ~ 

We might add more configuration under the jwt key, for now there is not.
Btw, the documentation needs an update , PR welcome and much appreciated!

I'm going to merge this and draft a new release later today.

@chalasr chalasr changed the title Security Authenticators SF5.X Add new jwt authenticator for Symfony 5.3+ Security system Jun 20, 2021
@chalasr
Copy link
Collaborator

chalasr commented Jun 23, 2021

Thank you very much @TristanPouliquen, this is much appreciated.

@chalasr chalasr merged commit 319364d into lexik:2.x Jun 23, 2021
@chalasr
Copy link
Collaborator

chalasr commented Jun 23, 2021

Released in v2.12.0.

@mnocon
Copy link
Contributor

mnocon commented Jun 23, 2021

Hi!

Please correct me if I'm wrong, but looks like the idea from this comment: #872 (comment) was never executed?

There's a new method in the interface in 2.12:
https://github.com/lexik/LexikJWTAuthenticationBundle/blob/v2.12.0/Services/JWTTokenManagerInterface.php#L31-L35

Our CI setup started failing with:

!!  PHP Fatal error:  Class XYZ\TokenManager contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (Lexik\Bundle\JWTAuthenticationBundle\Services\JWTTokenManagerInterface::decodeFromJsonWebToken) in /var/www/vendor/XYZ/src/lib/Security/EditorialMode/TokenManager.php on line 24

which pointed me to this PR.

@chalasr
Copy link
Collaborator

chalasr commented Jun 23, 2021

@mnocon You're right, I'm going to revert that breaking change this weekend. Thanks for noticing!

@mnocon
Copy link
Contributor

mnocon commented Jun 24, 2021

@chalasr thank you for confirming this, for now we will stick with 2.11.x and update when the next release is available.

Also thank you for maintaining this bundle! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants