Two-Factor Authentication

[amickan]

This adds Two-Factor Authentication through the django-allauth-2fa package.

I tested a few other packages, most notably django_two_factor_auth. It appears to be better maintained than django-allauth-2fa, but there are some issues with it. Forcing 2fa for existing users that don't have a totp device set-up yet, for example, does not work - you get stuck in an infinite loop. This is a known issue, there is a workaround for it and also a PR to fix it, but this PR has been around for while and it's not clear when it will be incorporated. This and some other issues with integrating it made me decide to not use this package.

I also checked out django-mfa-2, but that requires a lot of custom code to integrate.

With django-allauth-2fa, any user of GC has the option to enable 2FA through a link in the profile:

It works with the most common Authenticator apps and it also support backup tokens. Users can also remove 2FA again.

For staff users, 2FA is obligatory.

Getting this to work with social sign up required some custom code. The other packages also don't support 2FA with social sign up out of the box.

Back-up tokens:
At the moment, the backup tokens are always shown. That behaviour can be changed by setting ALLAUTH_2FA_ALWAYS_REVEAL_BACKUP_TOKENS to False in the settings.

[codecov]

Codecov Report

Merging #2497 (08eec0e) into main (4493a13) will increase coverage by 0.07%.
The diff coverage is 100.00%.

❗ Current head 08eec0e differs from pull request most recent head c454d4f. Consider uploading reports for the commit c454d4f to get more accurate results

@@            Coverage Diff             @@
##             main    #2497      +/-   ##
==========================================
+ Coverage   93.92%   94.00%   +0.07%     
==========================================
  Files         814      816       +2     
  Lines       30069    30193     +124     
==========================================
+ Hits        28243    28382     +139     
+ Misses       1826     1811      -15     

Impacted Files	Coverage Δ
app/config/settings.py	95.87% <100.00%> (+0.01%)	⬆️
app/config/urls/root.py	93.54% <100.00%> (+0.21%)	⬆️
app/grandchallenge/core/middleware.py	100.00% <100.00%> (ø)
app/grandchallenge/profiles/adapters.py	97.43% <100.00%> (+18.26%)	⬆️
...p/grandchallenge/profiles/templatetags/profiles.py	94.11% <100.00%> (+0.78%)	⬆️
app/grandchallenge/profiles/views.py	92.68% <100.00%> (+2.91%)	⬆️
app/tests/cases_tests/test_forms.py	100.00% <100.00%> (ø)
app/tests/conftest.py	100.00% <100.00%> (ø)
app/tests/pages_tests/test_pages.py	100.00% <100.00%> (ø)
app/tests/profiles_tests/test_2fa.py	100.00% <100.00%> (ø)
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4493a13...c454d4f. Read the comment docs.

[pkcakeout]

Wow... this looks pretty fan-ceee!

[jmsmkn]

This doesn't need to be a re_path.

Suggested change

	re_path(
	r"accounts/two_factor/setup/?$",
	TwoFactorSetup.as_view(),
	name="two-factor-setup",
	),
	path(
	"accounts/two_factor/setup/",
	TwoFactorSetup.as_view(),
	name="two-factor-setup",
	),

[amickan]

Somehow the partial path turns out to be necessary. I'm not sure why, yet, but if I just use path it goes to the original setup view and not the custom one, even thought this url is defined before I include the allauth-2fa urls.

[jmsmkn]

This is cool! Some manual testing:

• Create the dev server, use ./scripts/create_superuser.sh, cannot log in with username and password superuser
  
• Backup tokens should only be displayed once, the user should be instructed on to download them and keep them safe
• It's unclear how to use a backup token, I worked out you can put them in the authenticate field, but probably the user should have instructions about how to use them if they no longer have access to their TOTP device
• Disabling 2FA should require the user to enter their password again first
• Would be nice if this could be on one line, fine by me to call it 2FA Settings and expand the acronym in the elements title attribute on hover
  
• I've not tested the redirects and flatpages

[amickan]

• Added some explanations for the back-up tokens. Is this clear?
  
  
• Changed "Two-Factor Authentication Settings" to "2FA settings"
  
• Disabling 2FA now requires entering a 2FA token first. I took inspiration from how this is handled in other 2fa packages. If you prefer to require the user's password instead of a 2FA token, fine as well.
• The superuser issue is not specific to this PR. I also have it on main. I think this started for me when we moved away from docker compose. The scripts no longer creates a superuser, hence the password error you got. I will take a look and fix this in a seperate PR.

[jmsmkn]

LGTM, nice work!