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

Remember-Me Implementation with Authenticators in Symfony 5.3 #61

Closed
wants to merge 20 commits into from

Conversation

scheb
Copy link
Owner

@scheb scheb commented Feb 24, 2021

symfony/symfony#40145 is refactoring the implementation of the remember-me feature in Symfony security, with the goal to have a cleaner and more extensible implementation for the authenticator-based security system. The changes are targeting the upcoming Symfony 5.3 release. Changes allow to better integrate the bundle with the remember-me feature from Symfony security, effectively removing the weird "DIC hack" that I'm currently doing to manage remember-me during 2fa.

  • Bump up the minimum required version to use authenticator-based security with the bundle to Symfony 5.3
  • Not use the DIC hack when authenticator-based security is used
  • Implement new solution to suppress the remember-me cookie to be set
  • Implement new solution to carry the remember-me flag through the 2fa process
  • Implement new solution to set the remember-me cookie after successful 2fa
  • Symfony 5.3 compatibility changes
  • Test coverage and cleanup

@scheb
Copy link
Owner Author

scheb commented Mar 6, 2021

The new approach is confirmed to be working. Also, the changes from symfony/symfony#40145 aren't breaking 2fa in any of the other configurations (i.e. old security system).

The implementation is way cleaner now. Just needs a bit a cleanup and test coverage for the new code, then it would be ready to be merged and released as soon as Symfony 5.3 is out.

@Seldaek
Copy link
Contributor

Seldaek commented Apr 20, 2021

With symfony 5.3-beta1 out, I'd be happy to try out a beta of this feature too to ensure all is well, but the branches here are not available on the 2fa-bundle subtree split which is making things harder :) Just to say, if you find time to release a beta with this PR merged I'll happily give it a shot. Would rather nail down any issue in the new remember me system before Symfony 5.3.0 hits.

@scheb
Copy link
Owner Author

scheb commented Apr 20, 2021

@Seldaek For the purpose of testing you can reference the scheb/2fa package, which is is the all-in-one package. I wouldn't recommend using that package for production, as it's potentially pulling in extra dependencies, but for testing it's fine.

I've already tested the changes from symfony/symfony#40145 quite extensively while it was still in development and version that got ultimately merged was working fine. The last CI run was already going against Symfony 5.x and integrations tests (which cover the entire 2fa process including remember-me) have been successful. So I'm very sure we're not running into issues with Symfony 5.3.

@Seldaek
Copy link
Contributor

Seldaek commented Apr 28, 2021

Ok just FYI - can confirm our test suite passes with 5.3-dev and this PR 👍🏻 Looking forward to a release!

@scheb scheb force-pushed the sf5.3-remember-me branch 4 times, most recently from 3015498 to eff5b8a Compare May 2, 2021 11:59
@mmarton
Copy link

mmarton commented May 31, 2021

FYI symfony 5.3 is released and it isn't working with 2fabundle

Argument 1 passed to Symfony\Component\Security\Http\Authenticator\RememberMeAuthenticator::__construct() must implement interface Symfony\Component\Security\Http\RememberMe\RememberMeHandlerInterface, instance of Scheb\TwoFactorBundle\Security\Authentication\RememberMe\RememberMeServicesDecorator given.

https://symfony.com/blog/symfony-5-3-0-released

@scheb
Copy link
Owner Author

scheb commented May 31, 2021

That's expected, that's what this PR is for ;)

I'm still at work, so please be patient. If you have to upgrade to Symfony 5.3 on day 0, well, then feel free to refer this branch here until a proper version is assembled and released.

@mmarton
Copy link

mmarton commented May 31, 2021

That's expected, that's what this PR is for ;)

I'm still at work, so please be patient. If you have to upgrade to Symfony 5.3 on day 0, well, then feel free to refer this branch here until a proper version is assembled and released.

I'm just playing with the new release, and thought leaving a comment. I didn't mean to rush things at all. :)

@scheb
Copy link
Owner Author

scheb commented Jun 1, 2021

Merged (manually)

@scheb scheb closed this Jun 1, 2021
@scheb scheb deleted the sf5.3-remember-me branch June 2, 2021 20:37
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.

None yet

3 participants