Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Two-Factor Authentication #2497

Merged
merged 23 commits into from
Jun 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions app/config/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,8 @@
"django.middleware.common.CommonMiddleware",
"django.middleware.csrf.CsrfViewMiddleware",
"django.contrib.auth.middleware.AuthenticationMiddleware",
# Django-otp middleware must be after the AuthenticationMiddleware.
"django_otp.middleware.OTPMiddleware",
"django.contrib.messages.middleware.MessageMiddleware",
"django.contrib.sites.middleware.CurrentSiteMiddleware",
"django.middleware.clickjacking.XFrameOptionsMiddleware",
Expand All @@ -411,6 +413,14 @@
"grandchallenge.subdomains.middleware.subdomain_urlconf_middleware",
"grandchallenge.timezones.middleware.TimezoneMiddleware",
"machina.apps.forum_permission.middleware.ForumPermissionMiddleware",
# 2FA middleware, needs to be after subdomain middleware
# TwoFactorMiddleware resets the login flow if another page is loaded
# between login and successfully entering two-factor credentials. We're using
# a modified version of the original allauth_2fa middleware to pass the
# correct urlconf.
"grandchallenge.core.middleware.TwoFactorMiddleware",
# Force 2FA for staff users
"grandchallenge.core.middleware.RequireStaffAndSuperuser2FAMiddleware",
# Flatpage fallback almost last
"django.contrib.flatpages.middleware.FlatpageFallbackMiddleware",
# Redirects last as they're a last resort
Expand Down Expand Up @@ -480,6 +490,12 @@
# Overridden apps
"grandchallenge.forum_conversation",
"grandchallenge.forum_member",
# Configure the django-otp package
"django_otp",
"django_otp.plugins.otp_totp",
"django_otp.plugins.otp_static",
# Enable two-factor auth
"allauth_2fa",
]

LOCAL_APPS = [
Expand Down Expand Up @@ -577,6 +593,9 @@
LOGOUT_URL = "/accounts/logout/"
LOGIN_REDIRECT_URL = "/users/profile/"

# django-allauth-2fa
ALLAUTH_2FA_ALWAYS_REVEAL_BACKUP_TOKENS = False

##############################################################################
#
# stdimage
Expand Down
14 changes: 13 additions & 1 deletion app/config/urls/root.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from django.contrib.auth.decorators import login_required
from django.contrib.sitemaps.views import sitemap
from django.template.response import TemplateResponse
from django.urls import path
from django.urls import path, re_path
amickan marked this conversation as resolved.
Show resolved Hide resolved
from django.views.generic import TemplateView
from machina import urls as machina_urls

Expand All @@ -17,6 +17,7 @@
from grandchallenge.pages.sitemaps import PagesSitemap
from grandchallenge.policies.sitemaps import PoliciesSitemap
from grandchallenge.products.sitemaps import CompaniesSitemap, ProductsSitemap
from grandchallenge.profiles.views import TwoFactorRemove, TwoFactorSetup
from grandchallenge.reader_studies.sitemaps import ReaderStudiesSiteMap

admin.autodiscover()
Expand Down Expand Up @@ -58,6 +59,17 @@ def handler500(request):
name="django.contrib.sitemaps.views.sitemap",
),
path(settings.ADMIN_URL, admin.site.urls),
re_path(
r"accounts/two_factor/setup/?$",
TwoFactorSetup.as_view(),
name="two-factor-setup",
),
Comment on lines +62 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

re_path(
r"accounts/two_factor/remove/?$",
TwoFactorRemove.as_view(),
name="two-factor-remove",
),
path("accounts/", include("allauth_2fa.urls")),
path("accounts/", include("allauth.urls")),
path(
"stats/",
Expand Down
36 changes: 36 additions & 0 deletions app/grandchallenge/core/middleware.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
from allauth_2fa.middleware import BaseRequire2FAMiddleware
from django.urls import Resolver404, get_resolver
from django.utils.deprecation import MiddlewareMixin


class RequireStaffAndSuperuser2FAMiddleware(BaseRequire2FAMiddleware):
def require_2fa(self, request):
# Staff users and superusers are required to have 2FA.
return request.user.is_staff or request.user.is_superuser


class TwoFactorMiddleware(MiddlewareMixin):
"""
Reset the login flow if another page is loaded halfway through the login.
(I.e. if the user has logged in with a username/password, but not yet
entered their two-factor credentials.) This makes sure a user does not stay
half logged in by mistake.
"""

def __init__(self, get_response):
self.get_response = get_response

def process_request(self, request):
try:
match = get_resolver(request.urlconf).resolve(request.path)
if (
match
and not match.url_name
or not match.url_name.startswith("two-factor-authenticate")
):
try:
del request.session["allauth_2fa_user_id"]
except KeyError:
pass
except Resolver404:
return self.get_response(request)
28 changes: 26 additions & 2 deletions app/grandchallenge/profiles/adapters.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,18 @@
from allauth.account.adapter import DefaultAccountAdapter
from allauth.account.utils import user_email, user_username
from allauth.exceptions import ImmediateHttpResponse
from allauth.socialaccount.adapter import DefaultSocialAccountAdapter
from allauth_2fa.adapter import OTPAdapter
from allauth_2fa.utils import user_has_valid_totp_device
from django import forms
from django.conf import settings
from django.http import HttpResponseRedirect
from django.utils.http import url_has_allowed_host_and_scheme

from grandchallenge.challenges.models import Challenge
from grandchallenge.subdomains.utils import reverse


class AccountAdapter(DefaultAccountAdapter):
class AccountAdapter(OTPAdapter):
def is_safe_url(self, url):
challenge_domains = {
f"{c.short_name.lower()}{settings.SESSION_COOKIE_DOMAIN}"
Expand Down Expand Up @@ -50,3 +54,23 @@ def populate_user(self, *args, **kwargs):
user_username(user, user_email(user).split("@")[0])

return user

def pre_social_login(self, request, sociallogin):
if user_has_valid_totp_device(sociallogin.user):
# Cast to string for the case when this is not a JSON serializable
# object, e.g. a UUID.
request.session["allauth_2fa_user_id"] = str(sociallogin.user.id)
redirect_url = reverse("two-factor-authenticate")
redirect_url += "?next=" + request.get_full_path()
raise ImmediateHttpResponse(
response=HttpResponseRedirect(redirect_url)
)
elif sociallogin.user.is_staff and not user_has_valid_totp_device(
sociallogin.user
):
redirect_url = reverse("two-factor-setup")
redirect_url += "?next=" + request.get_full_path()
raise ImmediateHttpResponse(
response=HttpResponseRedirect(redirect_url)
)
return super().pre_social_login(request, sociallogin)
29 changes: 19 additions & 10 deletions app/grandchallenge/profiles/templates/account/login.html
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ <h1>{% trans "Sign In" %}</h1>
{% include "socialaccount/snippets/provider_list.html" with process="login" %}
</ul>

{% include "profiles/partials/or.html" %}
<div class="row">
<div class="col-lg-6 col-md-8 mx-auto">
{% include "profiles/partials/or.html" %}
</div>
</div>

</div>

Expand All @@ -36,14 +40,19 @@ <h1>{% trans "Sign In" %}</h1>
<a href="{{ signup_url }}">sign up</a> first.{% endblocktrans %}</p>
{% endif %}

<form class="login" method="POST" action="{% url 'account_login' %}">
{% csrf_token %}
{{ form|crispy }}
{% if redirect_field_value %}
<input type="hidden" name="{{ redirect_field_name }}" value="{{ redirect_field_value }}"/>
{% endif %}
<button class="btn btn-primary" type="submit">{% trans "Sign In" %}</button>
<a class="btn btn-secondary" href="{% url 'account_reset_password' %}">{% trans "Forgot Password?" %}</a>
</form>
<div class="row">
<div class="col-lg-6 col-md-8 mx-auto">
<form class="login" method="POST" action="{% url 'account_login' %}">
{% csrf_token %}
{{ form|crispy }}
{% if redirect_field_value %}
<input type="hidden" name="{{ redirect_field_name }}" value="{{ redirect_field_value }}"/>
{% endif %}
<button class="btn btn-primary" type="submit">{% trans "Sign In" %}</button>
<a class="btn btn-secondary"
href="{% url 'account_reset_password' %}">{% trans "Forgot Password?" %}</a>
</form>
</div>
</div>

{% endblock %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{% extends "account/base.html" %}
{% load i18n %}

{% block content %}
<h1>
{% trans "Two-Factor Authentication" %}
</h1>
<p>{% trans "Enter the token from your authenticator app below." %}</p>
<form method="post" class="mt-3">
{% csrf_token %}
{{ form.non_field_errors }}
{{ form.otp_token.label }}:
{{ form.otp_token }}
<br>
<button class="btn btn-primary mt-3" type="submit">
{% trans 'Authenticate' %}
</button>
</form>
{% endblock %}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
{% extends "base.html" %}
{% load i18n %}

{% block title %}Two-Factor Authentication{% endblock %}

{% block breadcrumbs %}
<ol class="breadcrumb">
<li class="breadcrumb-item text-light">Users</li>
<li class="breadcrumb-item"><a
href="{% url 'profile-detail' username=request.user.username %}">{{ request.user.username }}</a></li>
<li class="breadcrumb-item active" aria-current="page">Two Factor Authentication Settings</li>
</ol>
{% endblock %}

{% block content %}
<h1 class="mb-3">
{% trans "Two-Factor Authentication" %}
</h1>
<p>
<i class="fas fa-check-circle text-success mr-1"></i>{% trans 'Two-Factor Authentication is enabled for your account.' %}
</p>

<h3 class="mt-4">
{% trans "Back-Up Tokens" %}
</h3>
<p>{% trans "If you have lost access to your authentication device, you can use back-up tokens for authentication instead. Back-up tokens can be used in the same way as the tokens generated by your authentication device. <b>Make sure to keep your back-up tokens secret and store them in a secure place</b>. Should you run out of tokens, you can generate new ones on this page." %}</p>

{% if backup_tokens %}
{% if reveal_tokens %}
<p>{% trans "We have generated the following back-up tokens. These will only be displayed once. <b>Please keep them secret and store them securely</b>." %}</p>
<ul>
{% for token in backup_tokens %}
<li>{{ token.token }}</li>
{% endfor %}
</ul>
{% else %}
<p> {% trans 'Backup tokens have been generated, but are not revealed here for security reasons. Press the button below to generate new ones.' %} </p>
{% endif %}
{% else %}
<p> {% trans 'No tokens. Press the button below to generate some.' %}</p>
{% endif %}

<form method="post">
{% csrf_token %}
<button class="btn btn-primary" type="submit">
{% trans 'Generate backup tokens' %}
</button>
</form>

<h3 class="mt-4">
{% trans "Disable Two-Factor Authentication" %}
</h3>
<p>{% trans 'You can disable two-factor authentication for your account at any time.' %}</p>
<a class="btn btn-primary" href="{% url 'two-factor-remove' %}">Disable Two Factor Authentication</a>

{% endblock %}
32 changes: 32 additions & 0 deletions app/grandchallenge/profiles/templates/allauth_2fa/remove.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
{% extends "base.html" %}
{% load i18n %}

{% block title %}Disable Two-Factor Authentication{% endblock %}

{% block breadcrumbs %}
<ol class="breadcrumb">
<li class="breadcrumb-item text-light">Users</li>
<li class="breadcrumb-item"><a
href="{% url 'profile-detail' username=request.user.username %}">{{ request.user.username }}</a></li>
<li class="breadcrumb-item active" aria-current="page">Disable Two Factor Authentication</li>
</ol>
{% endblock %}

{% block content %}
<h1>
{% trans "Disable Two-Factor Authentication" %}
</h1>

<p>{% trans "Are you sure you want to disable Two-Factor Authentication from your account?" %}</p>
<p>{% trans "Confirm by entering the token from your authenticator app." %}</p>
<form method="post" class="mt-3">
{% csrf_token %}
{{ form.non_field_errors }}
{{ form.otp_token.label }}:
{{ form.otp_token }}
<br>
<button class="btn btn-primary mt-3" type="submit">
{% trans 'Yes, disable 2FA' %}
</button>
</form>
{% endblock %}
67 changes: 67 additions & 0 deletions app/grandchallenge/profiles/templates/allauth_2fa/setup.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
{% extends "base.html" %}
{% load i18n %}

{% block title %}Enable Two-Factor Authentication{% endblock %}

{% block breadcrumbs %}
<ol class="breadcrumb">
<li class="breadcrumb-item text-light">Users</li>
<li class="breadcrumb-item"><a
href="{% url 'profile-detail' username=request.user.username %}">{{ request.user.username }}</a></li>
<li class="breadcrumb-item active" aria-current="page">Set-up Two Factor Authentication</li>
</ol>
{% endblock %}

{% block content %}
<h1>
{% trans "Setup Two-Factor Authentication" %}
</h1>

<h4>
{% trans 'Step 1' %}:
</h4>

<p>
{% trans "Scan the QR code below with a token generator of your choice (e.g., Google Authenticator, Microsoft Authenticator)." %}
</p>

<img src="{{ qr_code_url }}"/>
<p>
{% trans "If you can't use the QR code, enter " %}
<a href="#secret-modal" data-toggle="modal" data-target="#secret-modal">{% trans "this code instead." %}</a>
</p>
<h4>
{% trans 'Step 2' %}:
</h4>

<p>
{% trans 'Input the token generated by the app:' %}
</p>

<form method="post">
{% csrf_token %}
{{ form.non_field_errors }}
{{ form.token.label }}: {{ form.token }}

<button class="btn btn-primary btn-sm" type="submit">
{% trans 'Verify' %}
</button>
</form>

{# modal #}
<div id="secret-modal" class="modal fade" tabindex="-1" role="dialog">
<div class="modal-dialog modal-dialog-centered" role="document">
<div class="modal-content">
<div class="modal-header">
<h5 class="modal-title">Your two-factor secret</h5>
<button type="button" class="close" data-dismiss="modal" aria-label="Close">
<span aria-hidden="true">&times;</span>
</button>
</div>
<div class="modal-body">
<p>{{ secret_key }}</p>
</div>
</div>
</div>
</div>
{% endblock %}
Loading