Migrate to built-in 2FA

[jmsmkn]

https://grand-challenge.sentry.io/issues/4606002971/?project=303639&query=%21logger%3Acsp+is%3Aunresolved&referrer=issue-stream&statsPeriod=30d&stream_index=0

[jmsmkn]

This is caused by an incompatibility between django-allauth-2fa and the latest version of django-allauth. The 2FA is still usable but users currently cannot remove their 2FA devices.

Given 2FA is now in the core of all auth we should migrate to it, see https://docs.allauth.org/en/latest/mfa/django-allauth-2fa.html

[amickan]

There is not a lot of documentation yet, but having taken a look at the codebase, it should all work out of the box, except for requiring 2FA for staff users. There is an issue for this feature request already. Should we wait until this is added, or should we migrate anyway and write our own middleware to enforce 2FA for staff users (we can use BaseRequire2FAMiddleware from the django-allauth-2fa as a basis)?

[jmsmkn]

django-allauth is open source software and this is something that we want, so maybe it would be a nice contribution? You cannot take the code from django-allauth-2fa and just add it to django-allauth as they are licensed differently, but you could make your own implementation.

If you want to go down this route make a fork of django-allauth, add a new branch, then get the tests up and running first. Then start implementing, making sure that the tests you write follow the existing structure of that project. Then make a PR upstream and see if it gets accepted!

[amickan]

That sound nice. I'll give it a try.

[amickan]

I made a PR to allauth a while ago to add middleware to force MFA for certain users, but it sounds as though it will not make it into allauth. Should I go ahead and migrate and add our own version of this middleware?

[jmsmkn]

Okay, we tried at least, shame as it seems a common use case. From the comments:

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.

Is is clear to you how that would be done?

[amickan]

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.

Is is clear to you how that would be done?

No, I have not had a look at how the login stages work exactly. If we have to implement our own version, I'd stick to the middleware to be honest.

[jmsmkn]

I think that we should at least investigate it given they know the framework the best. It looks like we would need to adapt this in our account adapters:

https://github.com/pennersr/django-allauth/blob/2e37568ccf51658d466a173b8695d690ecda0161/allauth/account/adapter.py#L697-L702

And then add an authentication stage:

https://github.com/pennersr/django-allauth/blob/2e37568ccf51658d466a173b8695d690ecda0161/allauth/mfa/stages.py#L8

or

https://github.com/pennersr/django-allauth/blob/2e37568ccf51658d466a173b8695d690ecda0161/allauth/account/stages.py#L89

I don't know if there would be any complications from ordering of the different stages. I definitely think that it is worth trying out this solution first and then selecting the most promising.

[amickan]

Ok, I will investigate

[amickan]

Ok, I think this works well. The only downside is that it is only triggered when someone logs in, not when they are already logged in like with the middleware, but I think this is fine, certainly for our use case.

[jmsmkn]

Cool! Yes I think that it is okay, people must log in every two weeks anyway.

[amickan]

Ah testing this when integrated into our app (and not just in a test), it turns out that it doesn't work how I thought it would. If I add the MFA-Setup stage in the same way as the authenticate stage (no matter whether before or after since only one of the two gets hit anyway), you end up in an infinite loop because the MFA set-up requires reauthentication.

[jmsmkn]

Okay! Good feedback for the PR review.

[amickan]

Looks like it won't be easy to get this to work with login stages. Thinking about other alternatives to middleware, we could add this to the post_login method on our custom AccountAdapter. I just tried that out and it seems to work as expected. Need to do some more testing. Again, the check is only done when someone logs in, but that is ok.

[jmsmkn]

If I understand this correctly at that point I think that they would be authenticated, so could navigate away from the configuration page and remain logged in?

[amickan]

Ah yes, you're absolutely right. Sorry, Friday afternoon brain.

[amickan]

Taking a closer look at the stages code, it's actually not clear to me why we end up in this infinite loop. We pass the reauthentication check and should get redirected to the activate view. I will continue looking into this on Monday.

[amickan]

Ok, looking at this again with a fresh mind, we do indeed end up in an infinite loop because the user is not authenticated yet when hitting the ActivateTOTPView and that view requires that the user is authenticated or has recently reauthenticated.

I don't think that we want to change that prerequisite for the activate view, and I don't think there is a way to finish authentication first if we're using the stages approach.

I can continue looking for an alternative implementation, or we go with he middleware approach, which we know works since we've been using it for a while. If we implement middleware, I will take another close look at which urls to allow.

[jmsmkn]

To summarise the discussion in the RSE meeting: we will go for middleware in GC for now, then see if a way of doing this appears in allauth afterall.