Allow preparation of providers to be optional

[Tjeerd]

Description

To pave the way for stateless 2nd factor authentication the bundle can be made aware of providers that do no need to be prepared.

These providers can be used for stateless auth because there is no need to store the state of the preparation.

This MR skips calling the preparation, storing the state of the preparation and checking this state if the provider does not need to be prepared. It does this by adding a bool method needsPreparation to the ProviderInterface and checking this method where appropriate.

Unfortunately this is a backwards incompatible change and would require other Provider implementations to add the method.

Also see #265

[scheb]

I have been thinking about this myself when I started the 8.x todo list :)

As you mentioned, this will be a BC break, so I'd consider this for the version 8 major release.

Though my approach would be a bit different. I'd set all the authentication providers that do not need preparation to "prepared" when the security token is created (in Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInitiator::beginTwoFactorAuthentication). Then all the other code can just stay as it is. Should overall have less complexity, because you don't need to spread "does this provider need preparation" logic at multiple places.

[Tjeerd]

Though my approach would be a bit different. I'd set all the authentication providers that do not need preparation to "prepared" when the security token is created (in Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInitiator::beginTwoFactorAuthentication). Then all the other code can just stay as it is. Should overall have less complexity, because you don't need to spread "does this provider need preparation" logic at multiple places.

I considered the same approach :) But choose to do it this way, because it feels a bit "cleaner" and represents the actual behavior instead of "faking" the preparation state. But I agree as well: it would probably require less changes and therefore probably introduce less bugs 😅

[scheb]

Yea, I feel the same issue with the naming/semantics. Would be better if it's called "is ready" or something like that, instead of calling it "prepared", when there was actually no preparation work done. Though, you could argue something that doesn't need preparation could be considered "automatically prepared" 🤷

We could rename things (and introduce more BC breaks), but that's probably not worth bothering too much with it. I'd definitely prefer a less complex implementation over the cleaner naming.

[scheb]

Looks much cleaner, definitely that approach 👍

Things that I'd change before merging:

• I'd evaluate the "needs preparation" when the active 2fa providers are collected, so we don't need to iterate the providers twice. The return value of the private getActiveTwoFactorProviders method would need to change for this. Or the code is reorganized as a whole. I believe the getActiveTwoFactorProviders is only there for historic reasons, as this used to be a public method.
• Unit test coverage :)

What would you suggest how to move forward? I can't merge this into 7.x and a version 8 is not going to appear that soon. I could create a 8.x development branch and we could merge it there. What do you think?

[Tjeerd]

Agreed, I'll add some tests and I've updated the loop to be a little more efficient.

I noticed this MR now also include the prio change of the listener. To be honest this was not necessarily the intention, but maybe we can use this MR to "Add support for stateless second factor authentication". In this case we should probably also add some documentation about how to setup such authentication. :)

[Tjeerd]

About that branching, I'll leave that up to you :)

I've integrated my MR branch into an actual project to test it "in production". If this branch/MR lands somewhere in this repo, I'll switch my project to this branch as well.

[scheb]

I've updated the MR to go into the 8.x branch. The 8.x is already containing the change for authenticator priority. So please do a rebase on your side.

[scheb]

Perfect 👍. Integration tests have also gone green.

Unit tests need to be added, then we're ready to merge.

[scheb]

Perfect, thank you! 🙇