Install allauth-mfa and add data migration

[amickan]

First step for #3215

[jmsmkn]

Looks good, don't the allauth_2fa.urls also need to be removed from the main router? After doing that, have you tested that everything gets set correctly when adding a 2fa device, logging in with that device or recovery code also works? What about testing the flow with social login?

[amickan]

don't the allauth_2fa.urls also need to be removed from the main router?

I thought that could wait until the next PR when I remove the dependency?

[jmsmkn]

don't the allauth_2fa.urls also need to be removed from the main router?

I thought that could wait until the next PR when I remove the dependency?

I think you'll need to remove them now - if you don't people could add a TOTP device etc to the old tables after the migration has been run.

[amickan]

don't the allauth_2fa.urls also need to be removed from the main router?

I thought that could wait until the next PR when I remove the dependency?

I think you'll need to remove them now - if you don't people could add a TOTP device etc to the old tables after the migration has been run.

Ah right, good point. But then I can also remove it from the list of installed apps at the same time, right?

[amickan]

What about testing the flow with social login?

I remember those tests being very difficult to get to work and I see that they were removed in #2735. What was the reason for removing them there?

[jmsmkn]

What about testing the flow with social login?

I remember those tests being very difficult to get to work and I see that they were removed in #2735. What was the reason for removing them there?

They were broken with Django All Auth 0.52 and I couldn't work out how to fix them. Those were covering functionalty that should be tested in Django All Auth. With my comment today I just meant a sanity check test in your dev environment. I think they have a dummy provider now that should help with development tests https://github.com/pennersr/django-allauth/blob/main/allauth/socialaccount/providers/dummy/provider.py

[jmsmkn]

don't the allauth_2fa.urls also need to be removed from the main router?

I thought that could wait until the next PR when I remove the dependency?

I think you'll need to remove them now - if you don't people could add a TOTP device etc to the old tables after the migration has been run.

Ah right, good point. But then I can also remove it from the list of installed apps at the same time, right?

You need to keep it in the installed apps so that the migration will run and the data still appears in the admin for validation.

[amickan]

Those were covering functionalty that should be tested in Django All Auth.

There was one test that checked the 2fa required workflow for staff users with a social login. I think that's the test we would want now as well. Just tried adding that back, but can't get it to work. I'm stuck at mocking the login... the DummyProvider doesn't help with that I think. I must admit that the social login stuff is rather opaque to me, so I'm afraid it will take some time to understand what happens and what needs testing exactly.

[jmsmkn]

Those were covering functionalty that should be tested in Django All Auth.

There was one test that checked the 2fa required workflow for staff users with a social login. I think that's the test we would want now as well. Just tried adding that back, but can't get it to work. I'm stuck at mocking the login... the DummyProvider doesn't help with that I think. I must admit that the social login stuff is rather opaque to me, so I'm afraid it will take some time to understand what happens and what needs testing exactly.

If I understand it correctly it should, then you wouldn't need any mocks at all, this is taken from the allauth tests:

def test_login(client, db):
     resp = client.post(reverse("dummy_login"))
     assert resp.status_code == 302
     assert resp["location"] == reverse("dummy_authenticate")
     resp = client.post(
         reverse("dummy_authenticate"),
         {"id": "123", "email": "[email protected]", "email_verified": True},
     )
     assert resp.status_code == 302
     assert resp["location"] == settings.LOGIN_REDIRECT_URL
     get_user_model().objects.filter(email="[email protected]").exists()

[amickan]

taken from the allauth tests

If I understand it correctly it should, then you wouldn't need any mocks at all, this is taken from the allauth tests:

def test_login(client, db):
     resp = client.post(reverse("dummy_login"))
     assert resp.status_code == 302
     assert resp["location"] == reverse("dummy_authenticate")
     resp = client.post(
         reverse("dummy_authenticate"),
         {"id": "123", "email": "[email protected]", "email_verified": True},
     )
     assert resp.status_code == 302
     assert resp["location"] == settings.LOGIN_REDIRECT_URL
     get_user_model().objects.filter(email="[email protected]").exists()

Cool. Hadn't seen that yet. Somehow it can't resolve the dummy_login url though. Adding the dummy provider to the test settings under SOCIALACCOUNT_PROVIDERS doesn't fix it. I will take a look tomorrow.

[jmsmkn]

I think you need to add it to INSTALLED_APPS in tests/settings.py.

[amickan]

I think you need to add it to INSTALLED_APPS in tests/settings.py.

This plus adding it to SOCIALACOUNT_PROVIDERS did the trick.

[jmsmkn]

Great work!