Add expiring email codes

[danielburger1337]

See #191

First off, thanks for taking the time to review ;)

Description

Expiring the email authentication code is generally a good idea and could easily be implemented in a custom provider (which I previously did including rate limiting & re-sending in case the email got lost in transit). But having it provided by the bundle with minimal effort for the maintainer is (imo at least) even better as it provides a little bit of extra security straight out of the box.

I think I figured out a way to contribute the expiration feature without breaking BC. This however uses a trick, which depending on how you feel, is a little bit nasty (but Symfony has used it in the past for BC reasons). Using @method declarations at the interface level we can check at runtime if the users has implemented the newly added methods and otherwise, to preserve BC, ignore/disable the expiration feature.

The only "BC break", if you can even call it that, is that when someone upgrades to, lets say 7.2 where this is included, their static analysis will bark at them for not implementing the @methods from the TwoFactorInterface and, in case they have a custom code generator, the CodeGeneratorInterface. This however wont break the code and it will still run without any errors at runtime.

---

This feature is currently disabled by default, but could and imo should be auto enabled with version 8.0 of the bundle:

scheb_two_factor:
    email:
        expires_after: PT15M # default is NULL which disables this feature

I also added the resend_expired option to automatically invoke CodeGeneratorInterface::generateAndSend when the user submitted the TFA form and their code has expired. This is enabled by default but only has an effect when expires_after is configured.

---

Maintenance in the future

Whenever BC is allowed to be broken again, lets assume 8.0, there are some changes that will have to be made to the code base:

• Add the @method annotations to the interface properly
• Remove the method_exists checks in CodeGenerator and EmailTwoFactorProvider
• Change TestableTwoFactorInterface and TestableCodeGeneratorInterface usages to the real interfaces in tests and delete them

What are TestableTwoFactorInterface and TestableCodeGeneratorInterface?

AFAIK and tried to search the docs for, PHPUnit mocks completely ignore @method annotations. That's why I added these helper interfaces in tests that do nothing but extend the real interface and add the @method methods to the interface properly.

[scheb]

First off, thanks for going the effort and suggesting an implementation.

My view on this solution:

1. It adds this messy kind-of-an-interface-but-actually-not with the @method annotation, which is against the point of having a clearly defined interface.
2. It puts the burden on the bundle, to handle existence and non-existence of the extra method. As a result, it's adding extra complexity that needs to be covered in tests and needs to be maintained, once introduced. Just from the viewpoint of code quality, things become worse.
3. Although it doesn't break code, it may break people's code linting, because suddenly a new method is declared on the interface. From my own experience, I know how annoying it is when an author of a dependency modifies the annotation of some public interface and suddenly Psalm is starting to complain about "incompatible" implementation. And then you have to put effort into adjusting your code just to satisfy the linting.

As mention on the original issue, if you want specific behavior for your 2fa authentication method, that the ouf-of-the-box implementation doesn't deliver, then implement your own 2fa provider. It would be different, if people had no options.

Since this was brought up in June 2023, no one else has really bothered about it. No comments, no reactions. So the demand in this feature seems to be quite low.

In summary: I agree, it would be definitely "nice" to have it, but the disadvantages of the implementation outweigh its value. Looking at the demand and considering what it would mean for the bundle and its users, I believe from the economical standpoint it's not sensible to accept it into the codebase.

[danielburger1337]

Mhm...I see it differently but it is your decision.

The maintenance burden is very minimal in my opinion because the email provider basically never gets touched anyway. With version 8.0 there would be a ~5 minute code change required to remove the ugliness. That's it.

Yes, as mentioned in my description as well, static analysis will be broken BUT that is okay in my opinion because you (the user) decided to update a dependency. Hear me out, I know in reality it is done quite differently, it is the users responsibility to look through the changes before upgrading. BC is never fully guaranteed. It is always their responsibility! Just think of necessary security fixes.

In my opinion your argument implicitly calls deprecations a bad thing because a lot of them will also trigger static analysis errors. I don't think BC should apply to static analysis and most libraries/frameworks think that as well. It eases the migration burden to newer versions, e.g. Symfonys UserInterface::getUserIdentifier, because the user can slowly migrate their code base over time instead of all at once when the new BC incompatible version drops.

---

For the users that use Psalm/PHPStan, I hope its close to 100% but we both know that is sadly not the reality, it requires a very small code change to not have the tool bark at them:

class User implements TwoFactorInterface
{
    public function getEmailAuthCodeCreatedAt(): DateTimeImmutable|null
    {
        return null;
    }

    public function setEmailAuthCodeCreatedAt(DateTimeImmutable|null $createdAt): void
    {
        // noop
    }
}

And for the couple of people that have a custom code generator:

class CodeGenerator implements CodeGeneratorInterface
{
    public function isCodeExpired(TwoFactorInterface $user): bool
    {
        return false;
    }
}

And of course, they can always add a rule in their config to ignore these errors.

---

In the end it is your decision and I support it either way!

You can gladly ping me when you are about to release version 8 and I will make these changes myself. I know that you can't "rely" on me and in the end you might be stuck with it, I can just give you my word!

[scheb]

Well, I'm not going to merge this into the bundle. I have a suggestion for you: Why not make this your own package, that provides like extended 2fa email functionality? There are a few other packages out there, which implement additional 2fa providers. I'd be happy to link yours so it's visible for anyone who's looking for a more sophisticated 2fa email provider.

[danielburger1337]

I will look into that. Is there a preferred naming convention for custom providers?

Are the package name "2fa-email" and the bundle name "EmailTwoFactorBundle" ok?

[scheb]

There is no convention for naming providers. Pick something that is likely unique. email as a provider name is used by the 2fa-email package.

On the package name, yourvendorname/2fa-email would be fine I guess. Those keywords will help people finding the package. In the Symfony ecosystem bundles are typically named after their package name, including the vendor. So foo/bar-bundle as a package name would be FooBarBundle as a bundle name.

You can check out these two packages for reference:
https://github.com/erkens/2fa-text
https://github.com/jbtronics/2fa-webauthn

[danielburger1337]

Okay. Thank you for feedback.

I released the package under the danielburger1337/2fa-email name. I named the provider "db1337email" to be as unique as possible.

It should be fully tested and has some nice little extra features compared to the basic 2fa-email provider:

• Expiring authentication codes
• Easier customization of the email message

I also added an example in the README how the developer can easily achieve rate limiting for the "resending" of the auth code email message.

As you mentioned, feel free to link it and to close the PR when you are ready.

Much thanks
~ Daniel

[scheb]

It's linked now (Symfony bundle docs usually updated over night). Thanks for contributing!