feat(mfa): Add RequireMFAMiddleware

[amickan]

This implementation was worked on independently, but it does bear a lot of resemblance to https://github.com/valohai/django-allauth-2fa/blob/main/allauth_2fa/middleware.py simply because there isn't really a different way of implementing middleware like this. Up to you whether and how you would want to reference the other implementation.

Submitting Pull Requests

General

• Make sure you use semantic commit messages.
  Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
• All Python code must formatted using Black, and clean from pep8 and isort issues.
• JavaScript code should adhere to StandardJS.
• If your changes are significant, please update ChangeLog.rst.
• If your change is substantial, feel free to add yourself to AUTHORS.

Provider Specifics

In case you add a new provider:

• Make sure unit tests are available.
• Add an entry of your provider in test_settings.py::INSTALLED_APPS and docs/installation.rst::INSTALLED_APPS.
• Add documentation to docs/providers/<provider name>.rst and docs/providers/index.rst Provider Specifics toctree.
• Add an entry to the list of supported providers over at docs/overview.rst.

[amickan]

Has anyone had a chance to look at this? Or am I missing a step to get this into review?

[pennersr]

I must admit, I am not sure about this approach -- whether or not a middleware is the best way forward. For example, another way of nudging users into turning on MFA would be to add a login stage that must be completed before the user can proceed. Also, the scope of such functionality is more about policies than authentication, and without a clear de facto way of handling things I am not sure it is wise to bring that into allauth. As for implementation, the allowed_urls approach feels a bit tricky. So, given all of this, and the fact that projects can easily add this themselves, I am leaning towards not bringing this into scope.

[amickan]

Fair enough. I hadn't thought of adding this as a login stage. Trying that out just now, however, it seems that you would end up in an infinite loop since MFA set-up requires reauthentication?

[pennersr]

Yeah, that won't be trivial either. For now, let's leave this all out of scope...

[pennersr]

@jmsmkn I should have done a better job at explaining the reasoning. Here goes, hopefully.

First, the implementation over at #3710 is not without issues. It uses a middleware, with an allowed_urls list, which may be a bit brittle to maintain and get right. For example, changing your email address is not allowed, yet, activating MFA only works if you have zero verified email addresses. UX wise it may also be regarded as a bit intrusive, you cannot even visit "/" without being promptly redirected to the MFA activation page.

Another way of implementing this would be to gently nudge the user to turn on MFA when the user signs in. Perhaps even allowing for a time window of a few days before this kicks in as mandatory.

And, in case you are solely doing this for staff users, perhaps another way of handling this would be to override user.has_perm() and always return False unless MFA is turned on.

Which leads me to the "Clear de facto way of handling things" remark -- given that there may be multiple approaches to the problem, and given that the proposed solution is not trivial, I prefer to not take that on board. It is really rather trivial for any project to add this middleware themselves, it's just a 60 lines or so middleware.

On the short term, I would be open to adding a more simple solution, e.g. a @mfa_required decorator that you can slap on your own views where needed. Such a decorator does not come at a maintenance cost, although it may not serve your needs if you are looking for a more site wide approach.

[bguggs]

On the short term, I would be open to adding a more simple solution, e.g. a @mfa_required decorator that you can slap on your own views where needed. Such a decorator does not come at a maintenance cost, although it may not serve your needs if you are looking for a more site wide approach.

Just to add some color to this issue, the @mfa_required solution unfortunately wouldn't work for our use case. As a SaaS product, we allow users or organizations to optionally require MFA for their accounts or for members of their organization. Rather than having MFA required based on which view we're hitting, we need to require MFA based on which user is hitting the view. That's where having a middleware solution built-in (where we could tailor how to set is_required to our use case) would be great.

The maintenence complexity makes sense, but I don't think this kind of middleware needs to be out-of-scope as a feature and seems like it may be a relatively common pattern and as you note it's not super complex, so including it as an option out-of-the box like in this issue would let allauth be a more complete MFA solution.