From 0913f60deb52bf913e043409a1caed8af36923b2 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 6 Aug 2024 19:29:06 +0200 Subject: [PATCH 01/44] wip --- .../bid_main/components/mfa_form.html | 28 +++++++++++++ bid_main/templates/bid_main/index.html | 3 ++ bid_main/templates/bid_main/login.html | 4 ++ bid_main/templates/bid_main/mfa_setup.html | 9 +++++ bid_main/urls.py | 10 +++++ bid_main/views/normal_pages.py | 39 +++++++++---------- blenderid/settings.py | 12 ++++++ requirements.txt | 9 +++-- 8 files changed, 90 insertions(+), 24 deletions(-) create mode 100644 bid_main/templates/bid_main/components/mfa_form.html create mode 100644 bid_main/templates/bid_main/mfa_setup.html diff --git a/bid_main/templates/bid_main/components/mfa_form.html b/bid_main/templates/bid_main/components/mfa_form.html new file mode 100644 index 0000000..85c0953 --- /dev/null +++ b/bid_main/templates/bid_main/components/mfa_form.html @@ -0,0 +1,28 @@ +{% load add_form_classes from forms %} +{% load static %} + +
+
+

Multi-factor Authentication

+
+ {% with form=form|add_form_classes %} +
{% csrf_token %} +
+ {% with field=form.otp_device %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.otp_token %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.otp_trust_agent %} + {% include "components/forms/field.html" %} + {% endwith %} + + {% if form.errors %} +

{{ form.errors }}

+ {% endif %} +
+ +
+ {% endwith %} +
diff --git a/bid_main/templates/bid_main/index.html b/bid_main/templates/bid_main/index.html index 87055ec..3dbdded 100644 --- a/bid_main/templates/bid_main/index.html +++ b/bid_main/templates/bid_main/index.html @@ -139,6 +139,9 @@ Profile Active Sessions + + Multi-factor Authentication +
diff --git a/bid_main/templates/bid_main/login.html b/bid_main/templates/bid_main/login.html index 8665e5c..49c3dd8 100644 --- a/bid_main/templates/bid_main/login.html +++ b/bid_main/templates/bid_main/login.html @@ -3,5 +3,9 @@ {% block page_title %}Sign in{% endblock %} {% block form %} +{% if is_authentication_form %} {% include 'bid_main/components/login_form.html' %} +{% elif is_mfa_form %} + {% include 'bid_main/components/mfa_form.html' %} +{% endif %} {% endblock form %} diff --git a/bid_main/templates/bid_main/mfa_setup.html b/bid_main/templates/bid_main/mfa_setup.html new file mode 100644 index 0000000..0b858b8 --- /dev/null +++ b/bid_main/templates/bid_main/mfa_setup.html @@ -0,0 +1,9 @@ +{% extends 'layout.html' %} +{% load pipeline static %} +{% block page_title %} +MFA Setup +{% endblock %} + +{% block body %} +{% for d in devices %}{{d}}{% endfor %} +{% endblock %} diff --git a/bid_main/urls.py b/bid_main/urls.py index 3fdee9d..bc9a982 100644 --- a/bid_main/urls.py +++ b/bid_main/urls.py @@ -1,6 +1,7 @@ from django.conf import settings from django.urls import reverse_lazy, path, re_path from django.contrib.auth import views as auth_views +from otp_agents.decorators import otp_required from . import forms from .views import normal_pages, registration_email, json_api, developer_applications @@ -146,6 +147,15 @@ urlpatterns = [ normal_pages.TerminateSessionView.as_view(), name='terminate_session', ), + path( + 'mfa/', + otp_required( + accept_trusted_agent=True, + if_configured=True, + view=normal_pages.MfaView.as_view(), + ), + name='mfa', + ), ] # Only enable this on a dev server: diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index acef774..ad72db8 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -20,13 +20,14 @@ from django.urls import reverse_lazy from django.utils import timezone from django.utils.decorators import method_decorator from django.views.decorators.cache import never_cache -from django.views.decorators.csrf import csrf_exempt -from django.views.decorators.debug import sensitive_post_parameters from django.views.generic import TemplateView, FormView from django.views.generic.base import View from django.views.generic.edit import UpdateView +from django_otp import devices_for_user +from otp_agents.forms import OTPTokenForm import loginas.utils import oauth2_provider.models as oauth2_models +import otp_agents.views from .. import forms, email from . import mixins @@ -72,35 +73,22 @@ class IndexView(LoginRequiredMixin, mixins.PageIdMixin, TemplateView): } -class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, auth_views.LoginView): +class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agents.views.LoginView): """Shows the login view.""" + otp_authentication_form = forms.AuthenticationForm page_id = "login" - template_name = "bid_main/login.html" - authentication_form = forms.AuthenticationForm - redirect_authenticated_user = True + redirect_authenticated_user = False success_url_allowed_hosts = settings.NEXT_REDIR_AFTER_LOGIN_ALLOWED_HOSTS + template_name = "bid_main/login.html" authorize_url = reverse_lazy("oauth2_provider:authorize") - @method_decorator(sensitive_post_parameters()) - @method_decorator(csrf_exempt) - @method_decorator(never_cache) - def dispatch(self, request, *args, **kwargs): - """Don't check CSRF token when already authenticated.""" - if self.redirect_authenticated_user and self.request.user.is_authenticated: - redirect_to = self.get_success_url() - if redirect_to == self.request.path: - raise ValueError( - "Redirection loop for authenticated user detected. Check that " - "your LOGIN_REDIRECT_URL doesn't point to a login page." - ) - return HttpResponseRedirect(redirect_to) - return super().dispatch(request, *args, **kwargs) - def get_context_data(self, **kwargs) -> dict: ctx = super().get_context_data(**kwargs) self.find_oauth_flow(ctx) + ctx["is_authentication_form"] = isinstance(self.get_form(), forms.AuthenticationForm) + ctx["is_mfa_form"] = isinstance(self.get_form(), OTPTokenForm) return ctx def find_oauth_flow(self, ctx: dict): @@ -453,3 +441,12 @@ class TerminateSessionView(LoginRequiredMixin, View): user_session.terminate() return redirect('bid_main:active_sessions') return HttpResponseNotFound("session not found") + + +class MfaView(LoginRequiredMixin, TemplateView): + template_name = "bid_main/mfa_setup.html" + + def get_context_data(self, **kwargs): + return { + 'devices': list(devices_for_user(self.request.user)), + } diff --git a/blenderid/settings.py b/blenderid/settings.py index b9ae27e..479f134 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -56,6 +56,11 @@ INSTALLED_APPS = [ "django.contrib.staticfiles", "django.contrib.sites", "django.contrib.flatpages", + "django_otp", + "django_otp.plugins.otp_totp", + "django_otp.plugins.otp_hotp", + "django_otp.plugins.otp_static", + "django_agent_trust", "oauth2_provider", "pipeline", "sorl.thumbnail", @@ -73,6 +78,8 @@ MIDDLEWARE = [ "django.middleware.common.CommonMiddleware", "django.middleware.csrf.CsrfViewMiddleware", "django.contrib.auth.middleware.AuthenticationMiddleware", + "django_agent_trust.middleware.AgentMiddleware", + "django_otp.middleware.OTPMiddleware", "bid_main.middleware.user_session_middleware", "django.contrib.messages.middleware.MessageMiddleware", "django.middleware.clickjacking.XFrameOptionsMiddleware", @@ -264,6 +271,10 @@ NEXT_REDIR_AFTER_LOGIN_ALLOWED_HOSTS = { "blender.community", } +AGENT_COOKIE_SECURE = True +AGENT_TRUST_DAYS = 30 +AGENT_INACTIVITY_DAYS = 7 + CSRF_COOKIE_SECURE = True CSRF_FAILURE_VIEW = "bid_main.views.errors.csrf_failure" CSRF_TRUSTED_ORIGINS = ['https://*.blender.org'] @@ -349,6 +360,7 @@ if TESTING: # For Debug Toolbar, extend with whatever address you use to connect # to your dev server. if DEBUG: + AGENT_COOKIE_SECURE = False CSRF_COOKIE_SECURE = False INSTALLED_APPS += ['debug_toolbar'] INTERNAL_IPS = ["127.0.0.1"] diff --git a/requirements.txt b/requirements.txt index c972bb7..0c33d54 100644 --- a/requirements.txt +++ b/requirements.txt @@ -8,15 +8,18 @@ colorama==0.4.6 ; python_version >= "3.8" and python_version < "4" and platform_ cryptography==41.0.0 ; python_version >= "3.8" and python_version < "4" csscompressor==0.9.5 ; python_version >= "3.8" and python_version < "4" deprecated==1.2.14 ; python_version >= "3.8" and python_version < "4" -dj-database-url==2.2.0 +django==4.2.13 ; python_version >= "3.8" and python_version < "4" django-admin-select2==1.0.1 ; python_version >= "3.8" and python_version < "4" +django-agent-trust==1.1.0 django-background-tasks-updated @ git+https://projects.blender.org/infrastructure/django-background-tasks.git@1.2.10 +django[bcrypt]==4.2.13 ; python_version >= "3.8" and python_version < "4" django-compat==1.0.15 ; python_version >= "3.8" and python_version < "4" django-loginas==0.3.11 ; python_version >= "3.8" and python_version < "4" django-oauth-toolkit @ git+https://projects.blender.org/Oleg-Komarov/django-oauth-toolkit.git@0b056a99ca943771615b859f48aaff0e12357f22 ; python_version >= "3.8" and python_version < "4" +django-otp==1.5.1 +django-otp-agents==1.0.1 django-pipeline==3.1.0 ; python_version >= "3.8" and python_version < "4" -django==4.2.13 ; python_version >= "3.8" and python_version < "4" -django[bcrypt]==4.2.13 ; python_version >= "3.8" and python_version < "4" +dj-database-url==2.2.0 docutils==0.14 ; python_version >= "3.8" and python_version < "4" htmlmin==0.1.12 ; python_version >= "3.8" and python_version < "4" idna==2.8 ; python_version >= "3.8" and python_version < "4" -- 2.30.2 From 935c19f5027bfa6ba1f30f91223329e893539cd3 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 8 Aug 2024 13:01:29 +0200 Subject: [PATCH 02/44] use MfaRequiredIfConfiguredMixin in place of LoginRequiredMixin --- bid_main/urls.py | 7 +----- bid_main/views/developer_applications.py | 15 +++++++++---- bid_main/views/json_api.py | 4 ++-- bid_main/views/mixins.py | 12 ++++++++++ bid_main/views/normal_pages.py | 28 +++++++++++++++--------- bid_main/views/registration_email.py | 12 +++++----- 6 files changed, 50 insertions(+), 28 deletions(-) diff --git a/bid_main/urls.py b/bid_main/urls.py index bc9a982..cfbda5a 100644 --- a/bid_main/urls.py +++ b/bid_main/urls.py @@ -1,7 +1,6 @@ from django.conf import settings from django.urls import reverse_lazy, path, re_path from django.contrib.auth import views as auth_views -from otp_agents.decorators import otp_required from . import forms from .views import normal_pages, registration_email, json_api, developer_applications @@ -149,11 +148,7 @@ urlpatterns = [ ), path( 'mfa/', - otp_required( - accept_trusted_agent=True, - if_configured=True, - view=normal_pages.MfaView.as_view(), - ), + normal_pages.MfaView.as_view(), name='mfa', ), ] diff --git a/bid_main/views/developer_applications.py b/bid_main/views/developer_applications.py index 80ce134..f33f5cb 100644 --- a/bid_main/views/developer_applications.py +++ b/bid_main/views/developer_applications.py @@ -1,14 +1,14 @@ """Pages for displaying and editing OAuth applications.""" -from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.messages.views import SuccessMessageMixin from django.forms import inlineformset_factory from django.urls import reverse from django.views.generic import ListView from django.views.generic.edit import UpdateView -import bid_main.forms +from bid_main.views import mixins import bid_api.forms import bid_api.models +import bid_main.forms import bid_main.models WebhookFormSet = inlineformset_factory( @@ -28,14 +28,21 @@ class _OwnedOAuth2ApplicationsMixin: return self.request.user.bid_main_oauth2application -class ListApplicationsView(LoginRequiredMixin, _OwnedOAuth2ApplicationsMixin, ListView): +class ListApplicationsView( + mixins.MfaRequiredIfConfiguredMixin, + _OwnedOAuth2ApplicationsMixin, + ListView, +): """List all OAuth 2 applications that currently logged in user is allowed to manage.""" template_name = 'bid_main/developer_applications.html' class EditApplicationView( - LoginRequiredMixin, SuccessMessageMixin, _OwnedOAuth2ApplicationsMixin, UpdateView + mixins.MfaRequiredIfConfiguredMixin, + SuccessMessageMixin, + _OwnedOAuth2ApplicationsMixin, + UpdateView, ): """Edit an OAuth 2 application, if allowed.""" diff --git a/bid_main/views/json_api.py b/bid_main/views/json_api.py index bde6a8c..d67db52 100644 --- a/bid_main/views/json_api.py +++ b/bid_main/views/json_api.py @@ -7,17 +7,17 @@ than via bearer tokens. import logging -from django.contrib.auth.mixins import LoginRequiredMixin from django.http import JsonResponse from django.shortcuts import get_object_or_404 from django.views.generic import View from .. import models +from bid_main.views import mixins log = logging.getLogger(__name__) -class BadgeTogglePrivateView(LoginRequiredMixin, View): +class BadgeTogglePrivateView(mixins.MfaRequiredIfConfiguredMixin, View): """JSON endpoint that toggles 'is_private' flag for badges.""" def post(self, request, *args, **kwargs) -> JsonResponse: diff --git a/bid_main/views/mixins.py b/bid_main/views/mixins.py index 0ec2986..cd8a081 100644 --- a/bid_main/views/mixins.py +++ b/bid_main/views/mixins.py @@ -2,7 +2,9 @@ import logging import urllib.parse +from django.contrib.auth.mixins import AccessMixin from django.urls import reverse_lazy +from otp_agents.decorators import otp_required log = logging.getLogger(__name__) @@ -35,3 +37,13 @@ class RedirectToPrivacyAgreeMixin: redirect_to = f"{self.privacy_policy_agree_url}?{next_url_qs}" log.debug("Directing user to %s", redirect_to) return redirect_to + + +class MfaRequiredIfConfiguredMixin(AccessMixin): + """Use this mixin instead of LoginRequiredMixin for bid_main.views.""" + + def dispatch(self, request, *args, **kwargs): + if not request.user.is_authenticated: + return self.handle_no_permission() + decorator = otp_required(accept_trusted_agent=True, if_configured=True) + return decorator(super().dispatch)(request, *args, **kwargs) diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index ad72db8..76e5fe1 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -9,7 +9,6 @@ import urllib.parse from django.conf import settings from django.contrib.auth import views as auth_views, logout, get_user_model -from django.contrib.auth.mixins import LoginRequiredMixin from django.core.exceptions import ValidationError from django.utils.translation import gettext as _ from django.db import transaction, IntegrityError @@ -38,7 +37,7 @@ User = get_user_model() log = logging.getLogger(__name__) -class IndexView(LoginRequiredMixin, mixins.PageIdMixin, TemplateView): +class IndexView(mixins.MfaRequiredIfConfiguredMixin, mixins.PageIdMixin, TemplateView): page_id = "index" template_name = "bid_main/index.html" login_url = reverse_lazy("bid_main:login") @@ -230,7 +229,7 @@ class LogoutView(auth_views.LogoutView): return next_url -class ProfileView(LoginRequiredMixin, UpdateView): +class ProfileView(mixins.MfaRequiredIfConfiguredMixin, UpdateView): form_class = forms.UserProfileForm model = User template_name = "bid_main/profile.html" @@ -271,7 +270,11 @@ class ProfileView(LoginRequiredMixin, UpdateView): return success_resp -class SwitchUserView(mixins.RedirectToPrivacyAgreeMixin, LoginRequiredMixin, auth_views.LoginView): +class SwitchUserView( + mixins.RedirectToPrivacyAgreeMixin, + mixins.MfaRequiredIfConfiguredMixin, + auth_views.LoginView, +): template_name = "bid_main/switch_user.html" form_class = forms.AuthenticationForm success_url_allowed_hosts = settings.NEXT_REDIR_AFTER_LOGIN_ALLOWED_HOSTS @@ -303,7 +306,12 @@ class GetAppsMixin: return app_model.objects.filter(id__in=app_ids).order_by("name") -class ApplicationTokenView(mixins.PageIdMixin, LoginRequiredMixin, GetAppsMixin, FormView): +class ApplicationTokenView( + mixins.PageIdMixin, + mixins.MfaRequiredIfConfiguredMixin, + GetAppsMixin, + FormView, +): page_id = "auth_tokens" template_name = "bid_main/auth_tokens.html" form_class = forms.AppRevokeTokensForm @@ -333,7 +341,7 @@ class ApplicationTokenView(mixins.PageIdMixin, LoginRequiredMixin, GetAppsMixin, return super().form_valid(form) -class PrivacyPolicyAgreeView(mixins.PageIdMixin, LoginRequiredMixin, FormView): +class PrivacyPolicyAgreeView(mixins.PageIdMixin, mixins.MfaRequiredIfConfiguredMixin, FormView): page_id = "privacy_policy_agree" template_name = "bid_main/privacy_policy_agree.html" form_class = forms.PrivacyPolicyAgreeForm @@ -374,7 +382,7 @@ class PrivacyPolicyAgreeView(mixins.PageIdMixin, LoginRequiredMixin, FormView): class DeleteUserView( - mixins.RedirectToPrivacyAgreeMixin, LoginRequiredMixin, GetAppsMixin, FormView + mixins.RedirectToPrivacyAgreeMixin, mixins.MfaRequiredIfConfiguredMixin, GetAppsMixin, FormView ): template_name = "bid_main/delete_user.html" form_class = forms.DeleteForm @@ -420,7 +428,7 @@ class DeleteUserView( return render(self.request, "bid_main/delete_user/confirm.html", context=ctx) -class ActiveSessionsView(LoginRequiredMixin, TemplateView): +class ActiveSessionsView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): template_name = "bid_main/active_sessions.html" def get_context_data(self, **kwargs): @@ -434,7 +442,7 @@ class ActiveSessionsView(LoginRequiredMixin, TemplateView): } -class TerminateSessionView(LoginRequiredMixin, View): +class TerminateSessionView(mixins.MfaRequiredIfConfiguredMixin, View): def post(self, request, *args, **kwargs): user_session_pk = kwargs.get('pk') if user_session := self.request.user.sessions.filter(pk=user_session_pk).first(): @@ -443,7 +451,7 @@ class TerminateSessionView(LoginRequiredMixin, View): return HttpResponseNotFound("session not found") -class MfaView(LoginRequiredMixin, TemplateView): +class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): template_name = "bid_main/mfa_setup.html" def get_context_data(self, **kwargs): diff --git a/bid_main/views/registration_email.py b/bid_main/views/registration_email.py index 4c46dde..bf02692 100644 --- a/bid_main/views/registration_email.py +++ b/bid_main/views/registration_email.py @@ -3,7 +3,6 @@ import logging from django.contrib.auth import views as auth_views from django.contrib.auth.forms import PasswordResetForm -from django.contrib.auth.mixins import LoginRequiredMixin from django.core.exceptions import ValidationError from django.db import transaction, IntegrityError from django.http import HttpResponseBadRequest, JsonResponse @@ -14,6 +13,7 @@ from django.utils import timezone from django.views.generic import CreateView, TemplateView, FormView, View from .. import forms, email +from . import mixins from ..models import User log = logging.getLogger(__name__) @@ -91,7 +91,7 @@ class InitialSetPasswordView(auth_views.PasswordResetConfirmView): form_class = forms.SetInitialPasswordForm -class ConfirmEmailView(LoginRequiredMixin, FormView): +class ConfirmEmailView(mixins.MfaRequiredIfConfiguredMixin, FormView): template_name = "bid_main/confirm_email/start.html" form_class = forms.ConfirmEmailStartForm log = logging.getLogger(f"{__name__}.ConfirmEmailView") @@ -127,7 +127,7 @@ class ConfirmEmailView(LoginRequiredMixin, FormView): return redirect("bid_main:confirm-email-sent") -class CancelEmailChangeView(LoginRequiredMixin, View): +class CancelEmailChangeView(mixins.MfaRequiredIfConfiguredMixin, View): """Cancel the user's email change and redirect to the profile page.""" log = logging.getLogger(f"{__name__}.CancelEmailChangeView") @@ -142,11 +142,11 @@ class CancelEmailChangeView(LoginRequiredMixin, View): return redirect("bid_main:index") -class ConfirmEmailSentView(LoginRequiredMixin, TemplateView): +class ConfirmEmailSentView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): template_name = "bid_main/confirm_email/sent.html" -class ConfirmEmailPollView(LoginRequiredMixin, View): +class ConfirmEmailPollView(mixins.MfaRequiredIfConfiguredMixin, View): """Returns JSON indicating when the email address has last been confirmed. The timestamp is returned as ISO 8601 to allow future periodic checks @@ -167,7 +167,7 @@ class ConfirmEmailPollView(LoginRequiredMixin, View): return JsonResponse({"confirmed": timestamp}) -class ConfirmEmailVerifiedView(LoginRequiredMixin, TemplateView): +class ConfirmEmailVerifiedView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): """Render explanation on GET, handle confirmation on POST. We only perform the actual database change on a POST, since that protects -- 2.30.2 From 1277d939f89bbec3118538c9a7a7f9a2737e3f0c Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 8 Aug 2024 14:22:45 +0200 Subject: [PATCH 03/44] mixin to ensure a recent mfa --- bid_main/views/mixins.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/bid_main/views/mixins.py b/bid_main/views/mixins.py index cd8a081..da4be64 100644 --- a/bid_main/views/mixins.py +++ b/bid_main/views/mixins.py @@ -47,3 +47,13 @@ class MfaRequiredIfConfiguredMixin(AccessMixin): return self.handle_no_permission() decorator = otp_required(accept_trusted_agent=True, if_configured=True) return decorator(super().dispatch)(request, *args, **kwargs) + + +class MfaRequiredMixin(AccessMixin): + """This mixin ensures an mfa check within the session.""" + + def dispatch(self, request, *args, **kwargs): + if not request.user.is_authenticated: + return self.handle_no_permission() + decorator = otp_required(accept_trusted_agent=False, if_configured=False) + return decorator(super().dispatch)(request, *args, **kwargs) -- 2.30.2 From 6d094a1a91493d8409da6541268a00020c88fb91 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 8 Aug 2024 19:15:30 +0200 Subject: [PATCH 04/44] draft template --- bid_main/templates/bid_main/mfa_setup.html | 53 +++++++++++++++++++++- bid_main/views/normal_pages.py | 13 +++++- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/bid_main/templates/bid_main/mfa_setup.html b/bid_main/templates/bid_main/mfa_setup.html index 0b858b8..6ead4c0 100644 --- a/bid_main/templates/bid_main/mfa_setup.html +++ b/bid_main/templates/bid_main/mfa_setup.html @@ -1,9 +1,58 @@ {% extends 'layout.html' %} {% load pipeline static %} {% block page_title %} -MFA Setup +Multi-factor Authentication Setup {% endblock %} {% block body %} -{% for d in devices %}{{d}}{% endfor %} +
+

Multi-factor Authentication (MFA) Setup

+ {% if user_has_mfa_configured %} +

+ You have configured MFA for your account. + You can disable MFA at any time, but you have to sign-in using your authentication device or a recovery code. +

+
+
+ {% else %} +

TODO Explain why and how

+ {% endif %} +
+ +
+

Time-based one-time password (TOTP)

+

TODO Explain how to use TOTP, TOTP applications

+ {% with devices=devices_per_category.totp %} + {% if devices %} +
    + {% for d in devices %} +
  • {{ d.name }}
    • + {% endfor %} +
+ {% endif %} + {% endwith %} + +
+ +
+

Recovery codes

+

+ Store your recovery codes safely (e.g. in a password manager) and don't share them. + Each code can be used only once. + You can generate a new set of recovery codes at any time, any remaining old codes will become invalidated. +

+ {% with devices=devices_per_category.recovery %} + {% for d in devices %} +
+ {% with code_count=d.token_set.count %} + {{ code_count }} recovery code{{ code_count|pluralize }} remaining + + + {% endwith %} +
+ {% endfor %} + + {% endwith %} +
{% endblock %} diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index 76e5fe1..7f71027 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -4,6 +4,7 @@ No error handlers, no usually-one-off things like registration and email confirmation. """ +from collections import defaultdict import logging import urllib.parse @@ -23,6 +24,8 @@ from django.views.generic import TemplateView, FormView from django.views.generic.base import View from django.views.generic.edit import UpdateView from django_otp import devices_for_user +from django_otp.plugins.otp_static.models import StaticDevice +from django_otp.plugins.otp_totp.models import TOTPDevice from otp_agents.forms import OTPTokenForm import loginas.utils import oauth2_provider.models as oauth2_models @@ -455,6 +458,14 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): template_name = "bid_main/mfa_setup.html" def get_context_data(self, **kwargs): + devices = list(devices_for_user(self.request.user)) + devices_per_category = defaultdict(list) + for device in devices: + if isinstance(device, StaticDevice): + devices_per_category['recovery'].append(device) + if isinstance(device, TOTPDevice): + devices_per_category['totp'].append(device) return { - 'devices': list(devices_for_user(self.request.user)), + 'devices_per_category': devices_per_category, + 'user_has_mfa_configured': len(devices) > 0, } -- 2.30.2 From 389d6f52c9e9349d43ca6fbe8b74e0c41a3902d9 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 8 Aug 2024 19:33:24 +0200 Subject: [PATCH 05/44] placeholder explanation --- bid_main/templates/bid_main/mfa_setup.html | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bid_main/templates/bid_main/mfa_setup.html b/bid_main/templates/bid_main/mfa_setup.html index 6ead4c0..365d0ed 100644 --- a/bid_main/templates/bid_main/mfa_setup.html +++ b/bid_main/templates/bid_main/mfa_setup.html @@ -16,7 +16,12 @@ Multi-factor Authentication Setup
{% else %} -

TODO Explain why and how

+

+ MFA makes your account more secure against account takeover attacks. +

+

+ If you have privileged access (admin or moderator) on any of Blender websites, you should setup MFA to avoid potential harm done to other community members through misuse of your account. +

{% endif %} -- 2.30.2 From 8e419948f16c218fef0574e1d07280dba744fe20 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 8 Aug 2024 21:03:30 +0200 Subject: [PATCH 06/44] form to disable mfa --- bid_main/forms.py | 14 +++++++ bid_main/templates/bid_main/mfa/disable.html | 22 ++++++++++ .../{mfa_setup.html => mfa/setup.html} | 2 +- bid_main/urls.py | 9 +++- bid_main/views/mfa.py | 42 +++++++++++++++++++ bid_main/views/normal_pages.py | 21 ---------- blenderid/settings.py | 1 - 7 files changed, 86 insertions(+), 25 deletions(-) create mode 100644 bid_main/templates/bid_main/mfa/disable.html rename bid_main/templates/bid_main/{mfa_setup.html => mfa/setup.html} (96%) create mode 100644 bid_main/views/mfa.py diff --git a/bid_main/forms.py b/bid_main/forms.py index fc04797..fd59b4d 100644 --- a/bid_main/forms.py +++ b/bid_main/forms.py @@ -409,3 +409,17 @@ class OAuth2ApplicationForm(forms.ModelForm): super().__init__(*args, **kwargs) for key in readonly_fields: self.fields[key].widget.attrs['readonly'] = True + + +class DisableMfaForm(forms.Form): + disable_mfa_confirm = forms.BooleanField( + help_text="Confirming disabling of multi-factor authentication", + initial=False, + label=_('Confirm'), + required=True, + widget=forms.CheckboxInput( + attrs={ + "autocomplete": "off", + }, + ), + ) diff --git a/bid_main/templates/bid_main/mfa/disable.html b/bid_main/templates/bid_main/mfa/disable.html new file mode 100644 index 0000000..6752a74 --- /dev/null +++ b/bid_main/templates/bid_main/mfa/disable.html @@ -0,0 +1,22 @@ +{% extends 'layout.html' %} +{% load pipeline static %} +{% load add_form_classes from forms %} +{% block page_title %} +Disable Multi-factor Authentication +{% endblock %} + +{% block body %} +
+

+ You are going to disable multi-factor authentication (MFA). + You can always configure MFA again. +

+
{% csrf_token %} + {% with form=form|add_form_classes %} + {% with field=form.disable_mfa_confirm %} + {% include "components/forms/field.html" %} + {% endwith %} + {% endwith %} + +
+{% endblock %} diff --git a/bid_main/templates/bid_main/mfa_setup.html b/bid_main/templates/bid_main/mfa/setup.html similarity index 96% rename from bid_main/templates/bid_main/mfa_setup.html rename to bid_main/templates/bid_main/mfa/setup.html index 365d0ed..7fdbce5 100644 --- a/bid_main/templates/bid_main/mfa_setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -13,7 +13,7 @@ Multi-factor Authentication Setup You can disable MFA at any time, but you have to sign-in using your authentication device or a recovery code.

-
{% else %}

diff --git a/bid_main/urls.py b/bid_main/urls.py index cfbda5a..c6134ef 100644 --- a/bid_main/urls.py +++ b/bid_main/urls.py @@ -3,7 +3,7 @@ from django.urls import reverse_lazy, path, re_path from django.contrib.auth import views as auth_views from . import forms -from .views import normal_pages, registration_email, json_api, developer_applications +from .views import mfa, normal_pages, registration_email, json_api, developer_applications app_name = "bid_main" urlpatterns = [ @@ -148,9 +148,14 @@ urlpatterns = [ ), path( 'mfa/', - normal_pages.MfaView.as_view(), + mfa.MfaView.as_view(), name='mfa', ), + path( + 'mfa/disable/', + mfa.DisableMfaView.as_view(), + name='disable_mfa', + ), ] # Only enable this on a dev server: diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py new file mode 100644 index 0000000..466f6b3 --- /dev/null +++ b/bid_main/views/mfa.py @@ -0,0 +1,42 @@ + +from collections import defaultdict + +from django_otp import devices_for_user +from django_otp.plugins.otp_static.models import StaticDevice +from django_otp.plugins.otp_totp.models import TOTPDevice +from django.db import transaction +from django.urls import reverse_lazy +from django.views.generic import TemplateView +from django.views.generic.edit import FormView + +from . import mixins +from bid_main.forms import DisableMfaForm + + +class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): + template_name = "bid_main/mfa/setup.html" + + def get_context_data(self, **kwargs): + devices = list(devices_for_user(self.request.user)) + devices_per_category = defaultdict(list) + for device in devices: + if isinstance(device, StaticDevice): + devices_per_category['recovery'].append(device) + if isinstance(device, TOTPDevice): + devices_per_category['totp'].append(device) + return { + 'devices_per_category': devices_per_category, + 'user_has_mfa_configured': len(devices) > 0, + } + + +class DisableMfaView(mixins.MfaRequiredMixin, FormView): + form_class = DisableMfaForm + template_name = "bid_main/mfa/disable.html" + success_url = reverse_lazy('bid_main:mfa') + + def form_valid(self, form): + with transaction.atomic(): + for device in devices_for_user(self.request.user, confirmed=None): + device.delete() + return super().form_valid(form) diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index 7f71027..ca9f774 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -4,7 +4,6 @@ No error handlers, no usually-one-off things like registration and email confirmation. """ -from collections import defaultdict import logging import urllib.parse @@ -23,9 +22,6 @@ from django.views.decorators.cache import never_cache from django.views.generic import TemplateView, FormView from django.views.generic.base import View from django.views.generic.edit import UpdateView -from django_otp import devices_for_user -from django_otp.plugins.otp_static.models import StaticDevice -from django_otp.plugins.otp_totp.models import TOTPDevice from otp_agents.forms import OTPTokenForm import loginas.utils import oauth2_provider.models as oauth2_models @@ -452,20 +448,3 @@ class TerminateSessionView(mixins.MfaRequiredIfConfiguredMixin, View): user_session.terminate() return redirect('bid_main:active_sessions') return HttpResponseNotFound("session not found") - - -class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): - template_name = "bid_main/mfa_setup.html" - - def get_context_data(self, **kwargs): - devices = list(devices_for_user(self.request.user)) - devices_per_category = defaultdict(list) - for device in devices: - if isinstance(device, StaticDevice): - devices_per_category['recovery'].append(device) - if isinstance(device, TOTPDevice): - devices_per_category['totp'].append(device) - return { - 'devices_per_category': devices_per_category, - 'user_has_mfa_configured': len(devices) > 0, - } diff --git a/blenderid/settings.py b/blenderid/settings.py index 479f134..c9137ac 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -58,7 +58,6 @@ INSTALLED_APPS = [ "django.contrib.flatpages", "django_otp", "django_otp.plugins.otp_totp", - "django_otp.plugins.otp_hotp", "django_otp.plugins.otp_static", "django_agent_trust", "oauth2_provider", -- 2.30.2 From ad0cfe9bee0ab672a02d1539cd0ad2e59820c083 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 9 Aug 2024 14:28:23 +0200 Subject: [PATCH 07/44] initial recovery codes support --- bid_main/templates/bid_main/mfa/setup.html | 32 ++++++++++---- bid_main/urls.py | 10 +++++ bid_main/views/mfa.py | 49 +++++++++++++++++++--- 3 files changed, 78 insertions(+), 13 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 7fdbce5..5aa1d05 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -40,6 +40,7 @@ Multi-factor Authentication Setup

+{% if user_can_setup_recovery %}

Recovery codes

@@ -47,17 +48,34 @@ Multi-factor Authentication Setup Each code can be used only once. You can generate a new set of recovery codes at any time, any remaining old codes will become invalidated.

- {% with devices=devices_per_category.recovery %} - {% for d in devices %} + {% with recovery=devices_per_category.recovery.0 %} + {% if recovery %}
- {% with code_count=d.token_set.count %} + {% with code_count=recovery.token_set.count %} {{ code_count }} recovery code{{ code_count|pluralize }} remaining - - + {% if recovery_codes %} + Hide + {% else %} + Display + {% endif %} +
{% csrf_token %} + +
+ {# populated on display_recovery_codes=1 #} + {% if recovery_codes %} +
    + {% for code in recovery_codes %} +
  • {{ code }}
    • + {% endfor %} +
+ {% endif %} {% endwith %}
- {% endfor %} - + {% endif %} +
{% csrf_token %} + +
{% endwith %}
+{% endif %} {% endblock %} diff --git a/bid_main/urls.py b/bid_main/urls.py index c6134ef..fc2ba35 100644 --- a/bid_main/urls.py +++ b/bid_main/urls.py @@ -156,6 +156,16 @@ urlpatterns = [ mfa.DisableMfaView.as_view(), name='disable_mfa', ), + path( + 'mfa/generate-recovery/', + mfa.GenerateRecoveryMfaView.as_view(), + name='generate_recovery_mfa', + ), + path( + 'mfa/invalidate-recovery/', + mfa.InvalidateRecoveryMfaView.as_view(), + name='invalidate_recovery_mfa', + ), ] # Only enable this on a dev server: diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 466f6b3..59889e0 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -1,12 +1,15 @@ - from collections import defaultdict +from os import urandom from django_otp import devices_for_user from django_otp.plugins.otp_static.models import StaticDevice from django_otp.plugins.otp_totp.models import TOTPDevice from django.db import transaction -from django.urls import reverse_lazy +from django.http import HttpResponseBadRequest +from django.shortcuts import redirect +from django.urls import reverse, reverse_lazy from django.views.generic import TemplateView +from django.views.generic.base import View from django.views.generic.edit import FormView from . import mixins @@ -14,18 +17,31 @@ from bid_main.forms import DisableMfaForm class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): + """Don't allow to setup recovery codes unless the user has already configured some other method. + + Otherwise MfaRequiredIfConfiguredMixin locks the user out immediately, not giving a chance + to copy the recovery codes. + """ template_name = "bid_main/mfa/setup.html" def get_context_data(self, **kwargs): - devices = list(devices_for_user(self.request.user)) + user = self.request.user + devices = list(devices_for_user(user)) devices_per_category = defaultdict(list) + recovery_codes = [] + user_can_setup_recovery = False for device in devices: if isinstance(device, StaticDevice): devices_per_category['recovery'].append(device) + if self.request.GET.get('display_recovery_codes', None): + recovery_codes = [t.token for t in device.token_set.all()] if isinstance(device, TOTPDevice): devices_per_category['totp'].append(device) + user_can_setup_recovery = True return { 'devices_per_category': devices_per_category, + 'recovery_codes': recovery_codes, + 'user_can_setup_recovery': user_can_setup_recovery, 'user_has_mfa_configured': len(devices) > 0, } @@ -35,8 +51,29 @@ class DisableMfaView(mixins.MfaRequiredMixin, FormView): template_name = "bid_main/mfa/disable.html" success_url = reverse_lazy('bid_main:mfa') + @transaction.atomic def form_valid(self, form): - with transaction.atomic(): - for device in devices_for_user(self.request.user, confirmed=None): - device.delete() + for device in devices_for_user(self.request.user, confirmed=None): + device.delete() return super().form_valid(form) + + +class GenerateRecoveryMfaView(mixins.MfaRequiredIfConfiguredMixin, View): + @transaction.atomic + def post(self, request, *args, **kwargs): + user = self.request.user + if not list(filter(lambda d: not isinstance(d, StaticDevice), devices_for_user(user))): + # Forbid setting up recovery codes unless the user already has some other method + return HttpResponseBadRequest("can't setup recovery codes before other methods") + user.staticdevice_set.all().delete() + device = StaticDevice.objects.create(name='recovery', user=user) + for _ in range(10): + device.token_set.create(token=urandom(8).hex().upper()) + return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1') + + +class InvalidateRecoveryMfaView(mixins.MfaRequiredIfConfiguredMixin, View): + def post(self, request, *args, **kwargs): + user = self.request.user + user.staticdevice_set.all().delete() + return redirect('bid_main:mfa') -- 2.30.2 From 13b856b7ccf1d27f26854252385b1dbcd08d4a42 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 12 Aug 2024 12:59:56 +0200 Subject: [PATCH 08/44] initial totp support without key encryption at rest, reimplement with a custom Device class --- bid_main/forms.py | 62 ++++++++++++++++++++++ bid_main/templates/bid_main/mfa/setup.html | 2 +- bid_main/templates/bid_main/mfa/totp.html | 45 ++++++++++++++++ bid_main/urls.py | 5 ++ bid_main/views/mfa.py | 53 +++++++++++++++--- blenderid/settings.py | 2 + requirements.txt | 2 + 7 files changed, 164 insertions(+), 7 deletions(-) create mode 100644 bid_main/templates/bid_main/mfa/totp.html diff --git a/bid_main/forms.py b/bid_main/forms.py index fd59b4d..804512f 100644 --- a/bid_main/forms.py +++ b/bid_main/forms.py @@ -1,14 +1,19 @@ +from binascii import unhexlify import logging import pathlib from django import forms from django.conf import settings from django.contrib.auth import forms as auth_forms, password_validation, authenticate +from django.core.signing import BadSignature, TimestampSigner +from django.core.validators import RegexValidator from django.db import transaction from django.forms import ValidationError from django.template import defaultfilters from django.utils import timezone from django.utils.translation import gettext_lazy as _ +from django_otp.oath import TOTP +from django_otp.plugins.otp_totp.models import TOTPDevice from .models import User, OAuth2Application from .recaptcha import check_recaptcha @@ -423,3 +428,60 @@ class DisableMfaForm(forms.Form): }, ), ) + + +class TotpMfaForm(forms.Form): + code = forms.CharField( + validators=[RegexValidator(r'^[0-9]{6}$')], + widget=forms.TextInput( + attrs={ + "autocomplete": "one-time-code", + "inputmode": "numeric", + "maxlength": 6, + "pattern": "[0-9]{6}", + "placeholder": "123456", + }, + ), + ) + key = forms.CharField(widget=forms.HiddenInput) + name = forms.CharField( + max_length=TOTPDevice._meta.get_field('name').max_length, + widget=forms.TextInput( + attrs={"placeholder": "device name (for your convenience)"}, + ), + ) + signature = forms.CharField(widget=forms.HiddenInput) + + def __init__(self, *args, **kwargs): + self.user = kwargs.pop('user', None) + super().__init__(*args, **kwargs) + + def clean(self): + super().clean() + code = self.data.get('code') + key = self.data.get('key') + signature = self.cleaned_data.get('signature') + if not self.verify_signature(self.user, key, signature): + raise forms.ValidationError(_('Invalid signature')) + self.cleaned_data['key'] = key + if not TOTP(unhexlify(key)).verify(int(code), tolerance=1): + self.add_error('code', _('Invalid code')) + return self.cleaned_data + + def save(self): + key = self.cleaned_data.get('key') + name = self.cleaned_data.get('name') + TOTPDevice.objects.create(key=key, name=name, user=self.user) + + @classmethod + def sign(cls, user, key): + signer = TimestampSigner() + return signer.sign(f'{user.email}_{key}') + + @classmethod + def verify_signature(cls, user, key, signature, max_age=3600): + signer = TimestampSigner() + try: + return f'{user.email}_{key}' == signer.unsign(signature, max_age=max_age) + except BadSignature: + return False diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 5aa1d05..24e529a 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -37,7 +37,7 @@ Multi-factor Authentication Setup {% endif %} {% endwith %} - + Configure a new TOTP device {% if user_can_setup_recovery %} diff --git a/bid_main/templates/bid_main/mfa/totp.html b/bid_main/templates/bid_main/mfa/totp.html new file mode 100644 index 0000000..4cb4489 --- /dev/null +++ b/bid_main/templates/bid_main/mfa/totp.html @@ -0,0 +1,45 @@ +{% extends 'layout.html' %} +{% load pipeline static %} +{% load add_form_classes from forms %} +{% block page_title %} +Multi-factor Authentication Setup +{% endblock %} + +{% block body %} +
+

New TOTP device

+
+
+
+ QR code +
show code for manual entry{{ manual_secret_key }}
+
+
+
+

TODO explain what's happening

+ {% with form=form|add_form_classes %} +
{% csrf_token %} + {% with field=form.name %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.code %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.key %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.signature %} + {% include "components/forms/field.html" %} + {% endwith %} + + {% if form.non_field_errors %} +
+ something went wrong +
+ {% endif %} +
+ {% endwith %} +
+
+
+{% endblock %} diff --git a/bid_main/urls.py b/bid_main/urls.py index fc2ba35..ee7fcd8 100644 --- a/bid_main/urls.py +++ b/bid_main/urls.py @@ -166,6 +166,11 @@ urlpatterns = [ mfa.InvalidateRecoveryMfaView.as_view(), name='invalidate_recovery_mfa', ), + path( + 'mfa/totp/', + mfa.TotpMfaView.as_view(), + name='totp_mfa', + ), ] # Only enable this on a dev server: diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 59889e0..048d0bf 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -1,9 +1,8 @@ +from base64 import b32encode, b64encode +from binascii import unhexlify from collections import defaultdict -from os import urandom +from io import BytesIO -from django_otp import devices_for_user -from django_otp.plugins.otp_static.models import StaticDevice -from django_otp.plugins.otp_totp.models import TOTPDevice from django.db import transaction from django.http import HttpResponseBadRequest from django.shortcuts import redirect @@ -11,9 +10,14 @@ from django.urls import reverse, reverse_lazy from django.views.generic import TemplateView from django.views.generic.base import View from django.views.generic.edit import FormView +from django_otp import devices_for_user +from django_otp.plugins.otp_static.models import StaticDevice +from django_otp.plugins.otp_totp.models import TOTPDevice, default_key +from django_otp.util import random_hex +import qrcode from . import mixins -from bid_main.forms import DisableMfaForm +from bid_main.forms import DisableMfaForm, TotpMfaForm class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): @@ -68,7 +72,7 @@ class GenerateRecoveryMfaView(mixins.MfaRequiredIfConfiguredMixin, View): user.staticdevice_set.all().delete() device = StaticDevice.objects.create(name='recovery', user=user) for _ in range(10): - device.token_set.create(token=urandom(8).hex().upper()) + device.token_set.create(token=random_hex(8).upper()) return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1') @@ -77,3 +81,40 @@ class InvalidateRecoveryMfaView(mixins.MfaRequiredIfConfiguredMixin, View): user = self.request.user user.staticdevice_set.all().delete() return redirect('bid_main:mfa') + + +class TotpMfaView(mixins.MfaRequiredIfConfiguredMixin, FormView): + template_name = "bid_main/mfa/totp.html" + success_url = reverse_lazy('bid_main:mfa') + + def get_form(self, *args, **kwargs): + kwargs = self.get_form_kwargs() + key = self.request.POST.get('key', default_key()) + kwargs['initial']['key'] = key + kwargs['initial']['signature'] = TotpMfaForm.sign(kwargs['user'], key) + return TotpMfaForm(**kwargs) + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + kwargs['user'] = self.request.user + return kwargs + + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + key = context['form'].initial['key'] + b32key = b32encode(unhexlify(key)).decode('utf-8') + context['manual_secret_key'] = b32key + device = TOTPDevice(key=key, user=self.request.user) + context['config_url'] = device.config_url + image = qrcode.make( + device.config_url, + error_correction=qrcode.constants.ERROR_CORRECT_H, + ) + buf = BytesIO() + image.save(buf) + context['qrcode'] = b64encode(buf.getvalue()).decode('utf-8') + return context + + def form_valid(self, form): + form.save() + return super().form_valid(form) diff --git a/blenderid/settings.py b/blenderid/settings.py index c9137ac..e832b27 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -90,6 +90,8 @@ AUTHENTICATION_BACKENDS = [ "django.contrib.auth.backends.ModelBackend", ] +OTP_TOTP_ISSUER = "id.blender.org" + ROOT_URLCONF = "blenderid.urls" TEMPLATES = [ diff --git a/requirements.txt b/requirements.txt index 0c33d54..2cc0767 100644 --- a/requirements.txt +++ b/requirements.txt @@ -41,11 +41,13 @@ pycparser==2.19 ; python_version >= "3.8" and python_version < "4" pygments==2.17.2 ; python_version >= "3.8" and python_version < "4" pyinstrument==4.6.0 ; python_version >= "3.8" and python_version < "4" pymdown-extensions==10.7 ; python_version >= "3.8" and python_version < "4" +pypng==0.20220715.0 pypugjs==5.9.12 ; python_version >= "3.8" and python_version < "4" python-dateutil==2.8.1 ; python_version >= "3.8" and python_version < "4" python-dotenv==0.21.0 pytz==2019.3 ; python_version >= "3.8" and python_version < "4" pyyaml==5.1.2 ; python_version >= "3.8" and python_version < "4" +qrcode==7.4.2 requests==2.30.0 ; python_version >= "3.8" and python_version < "4" sentry-sdk==1.4.3 ; python_version >= "3.8" and python_version < "4" six==1.12.0 ; python_version >= "3.8" and python_version < "4" -- 2.30.2 From d57b7a651f4bd16fcc5b2b1b48ae9460451084cc Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 12 Aug 2024 15:10:48 +0200 Subject: [PATCH 09/44] delete device button --- .../templates/bid_main/mfa/delete_device.html | 18 ++++++++++ bid_main/templates/bid_main/mfa/disable.html | 1 + bid_main/templates/bid_main/mfa/setup.html | 10 +++--- bid_main/urls.py | 22 +++++++----- bid_main/views/mfa.py | 36 +++++++++++++++---- 5 files changed, 68 insertions(+), 19 deletions(-) create mode 100644 bid_main/templates/bid_main/mfa/delete_device.html diff --git a/bid_main/templates/bid_main/mfa/delete_device.html b/bid_main/templates/bid_main/mfa/delete_device.html new file mode 100644 index 0000000..13d6af4 --- /dev/null +++ b/bid_main/templates/bid_main/mfa/delete_device.html @@ -0,0 +1,18 @@ +{% extends 'layout.html' %} +{% load pipeline static %} +{% load add_form_classes from forms %} +{% block page_title %} +Delete {{ object.name }} +{% endblock %} + +{% block body %} +
+

Delete {{ object.name }}?

+
{% csrf_token %} + {% with form=form|add_form_classes %} + {{ form }} + {% endwith %} + + Cancel +
+{% endblock %} diff --git a/bid_main/templates/bid_main/mfa/disable.html b/bid_main/templates/bid_main/mfa/disable.html index 6752a74..136a2c3 100644 --- a/bid_main/templates/bid_main/mfa/disable.html +++ b/bid_main/templates/bid_main/mfa/disable.html @@ -18,5 +18,6 @@ Disable Multi-factor Authentication {% endwith %} {% endwith %} + Cancel {% endblock %} diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 24e529a..dfc9750 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -13,7 +13,7 @@ Multi-factor Authentication Setup You can disable MFA at any time, but you have to sign-in using your authentication device or a recovery code.

{% else %}

@@ -32,12 +32,12 @@ Multi-factor Authentication Setup {% if devices %}

    {% for d in devices %} -
  • {{ d.name }}
    • +
  • {{ d.name }}
    • {% endfor %}
{% endif %} {% endwith %} - Configure a new TOTP device + Configure a new TOTP device
{% if user_can_setup_recovery %} @@ -58,7 +58,7 @@ Multi-factor Authentication Setup {% else %} Display {% endif %} -
{% csrf_token %} + {% csrf_token %}
{# populated on display_recovery_codes=1 #} @@ -72,7 +72,7 @@ Multi-factor Authentication Setup {% endwith %} {% endif %} -
{% csrf_token %} + {% csrf_token %}
{% endwith %} diff --git a/bid_main/urls.py b/bid_main/urls.py index ee7fcd8..5f6bda3 100644 --- a/bid_main/urls.py +++ b/bid_main/urls.py @@ -153,23 +153,29 @@ urlpatterns = [ ), path( 'mfa/disable/', - mfa.DisableMfaView.as_view(), - name='disable_mfa', + mfa.DisableView.as_view(), + name='mfa_disable', ), path( 'mfa/generate-recovery/', - mfa.GenerateRecoveryMfaView.as_view(), - name='generate_recovery_mfa', + mfa.GenerateRecoveryView.as_view(), + name='mfa_generate_recovery', ), path( 'mfa/invalidate-recovery/', - mfa.InvalidateRecoveryMfaView.as_view(), - name='invalidate_recovery_mfa', + mfa.InvalidateRecoveryView.as_view(), + name='mfa_invalidate_recovery', ), path( 'mfa/totp/', - mfa.TotpMfaView.as_view(), - name='totp_mfa', + mfa.TotpView.as_view(), + name='mfa_totp', + ), + path( + # using `path` converter because persistent_id contains a slash + 'mfa/delete-device//', + mfa.DeleteDeviceView.as_view(), + name='mfa_delete_device', ), ] diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 048d0bf..3c19bd6 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -4,13 +4,14 @@ from collections import defaultdict from io import BytesIO from django.db import transaction -from django.http import HttpResponseBadRequest +from django.http import Http404, HttpResponseBadRequest from django.shortcuts import redirect from django.urls import reverse, reverse_lazy from django.views.generic import TemplateView from django.views.generic.base import View -from django.views.generic.edit import FormView +from django.views.generic.edit import DeleteView, FormView from django_otp import devices_for_user +from django_otp.models import Device from django_otp.plugins.otp_static.models import StaticDevice from django_otp.plugins.otp_totp.models import TOTPDevice, default_key from django_otp.util import random_hex @@ -50,7 +51,7 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): } -class DisableMfaView(mixins.MfaRequiredMixin, FormView): +class DisableView(mixins.MfaRequiredMixin, FormView): form_class = DisableMfaForm template_name = "bid_main/mfa/disable.html" success_url = reverse_lazy('bid_main:mfa') @@ -62,7 +63,7 @@ class DisableMfaView(mixins.MfaRequiredMixin, FormView): return super().form_valid(form) -class GenerateRecoveryMfaView(mixins.MfaRequiredIfConfiguredMixin, View): +class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): @transaction.atomic def post(self, request, *args, **kwargs): user = self.request.user @@ -76,14 +77,14 @@ class GenerateRecoveryMfaView(mixins.MfaRequiredIfConfiguredMixin, View): return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1') -class InvalidateRecoveryMfaView(mixins.MfaRequiredIfConfiguredMixin, View): +class InvalidateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): def post(self, request, *args, **kwargs): user = self.request.user user.staticdevice_set.all().delete() return redirect('bid_main:mfa') -class TotpMfaView(mixins.MfaRequiredIfConfiguredMixin, FormView): +class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): template_name = "bid_main/mfa/totp.html" success_url = reverse_lazy('bid_main:mfa') @@ -118,3 +119,26 @@ class TotpMfaView(mixins.MfaRequiredIfConfiguredMixin, FormView): def form_valid(self, form): form.save() return super().form_valid(form) + + +class DeleteDeviceView(mixins.MfaRequiredMixin, DeleteView): + model = Device + template_name = "bid_main/mfa/delete_device.html" + success_url = reverse_lazy('bid_main:mfa') + + def get(self, request, *args, **kwargs): + for device in devices_for_user(self.request.user): + if ( + device.persistent_id != kwargs['persistent_id'] + and not isinstance(device, StaticDevice) + ): + # there are other non-recovery devices, it's fine to delete this one + return super().get(request, *args, **kwargs) + # this seems to be the last device, we are effectively disabling mfa + return redirect('bid_main:mfa_disable') + + def get_object(self, queryset=None): + device = Device.from_persistent_id(self.kwargs['persistent_id']) + if not device or self.request.user != device.user: + raise Http404() + return device -- 2.30.2 From 144d26eba0fa723ce1a8fc9aee38d84515885fa2 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 12 Aug 2024 20:50:42 +0200 Subject: [PATCH 10/44] forms --- README.md | 3 ++ bid_main/forms.py | 47 ++++++++++++++++ bid_main/models.py | 19 +++++-- .../bid_main/components/mfa_form.html | 54 +++++++++++++------ bid_main/templates/bid_main/mfa/setup.html | 4 +- .../templates/components/forms/field.html | 4 +- bid_main/templatetags/common.py | 12 +++++ bid_main/views/mfa.py | 26 ++++----- bid_main/views/normal_pages.py | 17 +++++- docs/mfa.md | 20 +++++++ 10 files changed, 165 insertions(+), 41 deletions(-) create mode 100644 bid_main/templatetags/common.py create mode 100644 docs/mfa.md diff --git a/README.md b/README.md index 5a4749e..432db5c 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,9 @@ See [OAuth.md](docs/OAuth.md). See [user_deletion.md](docs/user_deletion.md). +# Multi-factor authentication + +See [mfa.md](docs/mfa.md). # Troubleshooting diff --git a/bid_main/forms.py b/bid_main/forms.py index 804512f..7947ad7 100644 --- a/bid_main/forms.py +++ b/bid_main/forms.py @@ -14,6 +14,7 @@ from django.utils import timezone from django.utils.translation import gettext_lazy as _ from django_otp.oath import TOTP from django_otp.plugins.otp_totp.models import TOTPDevice +from otp_agents.views import OTPTokenForm from .models import User, OAuth2Application from .recaptcha import check_recaptcha @@ -213,6 +214,52 @@ class AuthenticationForm(auth_forms.AuthenticationForm): self.fields["password"].widget.attrs["placeholder"] = "Enter your password" +class MfaForm(BootstrapModelFormMixin, OTPTokenForm): + """Restyle the form widgets to do less work in the template.""" + + def __init__(self, *args, **kwargs): + use_recovery = kwargs.pop('use_recovery', False) + super().__init__(*args, **kwargs) + + otp_token = self.fields["otp_token"] + otp_token.label = _('Code') + otp_token.required = True + if use_recovery: + otp_token.validators = [RegexValidator(r'^[A-F0-9]{16}$')] + otp_token.widget = forms.TextInput( + attrs={ + "autocomplete": "one-time-code", + "maxlength": 16, + "pattern": "[A-F0-9]{16}", + "placeholder": "123456890ABCDEF", + }, + ) + else: + otp_token.validators = [RegexValidator(r'^[0-9]{6}$')] + otp_token.widget = forms.TextInput( + attrs={ + "autocomplete": "one-time-code", + "inputmode": "numeric", + "maxlength": 6, + "pattern": "[0-9]{6}", + "placeholder": "123456", + }, + ) + + otp_trust_agent = self.fields["otp_trust_agent"] + otp_trust_agent.help_text = _( + "We won't ask for MFA next time you sign-in on this device. " + "Use only on your private device." + ) + otp_trust_agent.initial = False + otp_trust_agent.label = _("Remember this device") + otp_trust_agent.widget = forms.CheckboxInput( + attrs={ + "autocomplete": "off", + }, + ) + + class UserProfileForm(BootstrapModelFormMixin, forms.ModelForm): """Edits full name and email address. diff --git a/bid_main/models.py b/bid_main/models.py index f2f1d04..3d93d77 100644 --- a/bid_main/models.py +++ b/bid_main/models.py @@ -1,22 +1,26 @@ +from collections import defaultdict from typing import Optional, Set import itertools import logging import os.path import re -from django import urls from django.conf import settings from django.contrib.auth.base_user import AbstractBaseUser, BaseUserManager from django.contrib.auth.models import PermissionsMixin from django.contrib.sessions.models import Session -from django.core import validators from django.core.exceptions import ValidationError +from django.core import validators from django.core.mail import send_mail from django.db import models, transaction from django.db.models import Q +from django import urls +from django_otp import devices_for_user +from django_otp.plugins.otp_static.models import StaticDevice +from django_otp.plugins.otp_totp.models import TOTPDevice from django.templatetags.static import static -from django.utils import timezone from django.utils.deconstruct import deconstructible +from django.utils import timezone from django.utils.translation import gettext_lazy as _ import oauth2_provider.models as oa2_models import user_agents @@ -541,6 +545,15 @@ class User(AbstractBaseUser, PermissionsMixin): return bid_main.file_utils.get_absolute_url(static(settings.AVATAR_DEFAULT_FILENAME)) return bid_main.file_utils.get_absolute_url(self.avatar.storage.url(default_thumbnail_path)) + def mfa_devices_per_category(self): + devices_per_category = defaultdict(list) + for device in devices_for_user(self): + if isinstance(device, StaticDevice): + devices_per_category['recovery'].append(device) + if isinstance(device, TOTPDevice): + devices_per_category['totp'].append(device) + return devices_per_category + class SettingValueField(models.CharField): def __init__(self, *args, **kwargs): # noqa: D107 diff --git a/bid_main/templates/bid_main/components/mfa_form.html b/bid_main/templates/bid_main/components/mfa_form.html index 85c0953..11d2987 100644 --- a/bid_main/templates/bid_main/components/mfa_form.html +++ b/bid_main/templates/bid_main/components/mfa_form.html @@ -1,5 +1,5 @@ {% load add_form_classes from forms %} -{% load static %} +{% load common static %}
@@ -7,22 +7,42 @@
{% with form=form|add_form_classes %}
{% csrf_token %} -
- {% with field=form.otp_device %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.otp_token %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.otp_trust_agent %} - {% include "components/forms/field.html" %} - {% endwith %} - - {% if form.errors %} -

{{ form.errors }}

- {% endif %} -
- +
+ {% if use_recovery %} +

Use a recovery code

+ + {% else %} + {% if devices.totp|length == 1 %} + + {% else %} +
+ {% for device in devices.totp %} + + {% endfor %} +
+ {% endif %} + {% endif %} + {% with field=form.otp_token %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.otp_trust_agent %} + {% include "components/forms/field.html" with with_help_text=True %} + {% endwith %} +
+ {{ form.non_field_errors }} +
{% endwith %} + {% if devices.recovery %} +
+ {% if use_recovery %} + Use an authenticator + {% else %} + Lost your authenticator? Use a recovery code + {% endif %} +
+ {% endif %}
diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index dfc9750..b9820e0 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -42,7 +42,7 @@ Multi-factor Authentication Setup {% if user_can_setup_recovery %}
-

Recovery codes

+

Recovery codes

Store your recovery codes safely (e.g. in a password manager) and don't share them. Each code can be used only once. @@ -56,7 +56,7 @@ Multi-factor Authentication Setup {% if recovery_codes %} Hide {% else %} - Display + Display {% endif %}

{% csrf_token %} diff --git a/bid_main/templates/components/forms/field.html b/bid_main/templates/components/forms/field.html index 5d05c08..0670a9c 100644 --- a/bid_main/templates/components/forms/field.html +++ b/bid_main/templates/components/forms/field.html @@ -5,7 +5,7 @@ {% if not field.is_hidden %} {% include 'components/forms/label.html' with label_class="form-check-label" %} {% if with_help_text and field.help_text %} - {{ form.new_password1.help_text|safe }} + {{ field.help_text|safe }} {% endif %} {% endif %}
{{ field.errors }}
@@ -18,7 +18,7 @@ {{ field }}
{{ field.errors }}
{% if with_help_text and field.help_text %} - {{ form.new_password1.help_text|safe }} + {{ field.help_text|safe }} {% endif %}
{% endif %} diff --git a/bid_main/templatetags/common.py b/bid_main/templatetags/common.py new file mode 100644 index 0000000..b779b89 --- /dev/null +++ b/bid_main/templatetags/common.py @@ -0,0 +1,12 @@ +from django import template + +register = template.Library() + + +# Credit: https://stackoverflow.com/questions/46026268/ +@register.simple_tag(takes_context=True) +def query_transform(context, **kwargs): + query = context['request'].GET.copy() + for k, v in kwargs.items(): + query[k] = v + return query.urlencode() diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 3c19bd6..19803f4 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -1,6 +1,5 @@ from base64 import b32encode, b64encode from binascii import unhexlify -from collections import defaultdict from io import BytesIO from django.db import transaction @@ -31,30 +30,25 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): def get_context_data(self, **kwargs): user = self.request.user - devices = list(devices_for_user(user)) - devices_per_category = defaultdict(list) recovery_codes = [] user_can_setup_recovery = False - for device in devices: - if isinstance(device, StaticDevice): - devices_per_category['recovery'].append(device) - if self.request.GET.get('display_recovery_codes', None): - recovery_codes = [t.token for t in device.token_set.all()] - if isinstance(device, TOTPDevice): - devices_per_category['totp'].append(device) - user_can_setup_recovery = True + devices_per_category = user.mfa_devices_per_category() + if self.request.GET.get('display_recovery_codes') and 'recovery' in devices_per_category: + recovery_codes = [t.token for t in devices_per_category['recovery'][0].token_set.all()] + if devices_per_category.keys() - {'recovery'}: + user_can_setup_recovery = True return { 'devices_per_category': devices_per_category, 'recovery_codes': recovery_codes, 'user_can_setup_recovery': user_can_setup_recovery, - 'user_has_mfa_configured': len(devices) > 0, + 'user_has_mfa_configured': bool(devices_per_category), } class DisableView(mixins.MfaRequiredMixin, FormView): form_class = DisableMfaForm - template_name = "bid_main/mfa/disable.html" success_url = reverse_lazy('bid_main:mfa') + template_name = "bid_main/mfa/disable.html" @transaction.atomic def form_valid(self, form): @@ -73,6 +67,8 @@ class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): user.staticdevice_set.all().delete() device = StaticDevice.objects.create(name='recovery', user=user) for _ in range(10): + # https://pages.nist.gov/800-63-3/sp800-63b.html#5122-look-up-secret-verifiers + # don't use less than 64 bits device.token_set.create(token=random_hex(8).upper()) return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1') @@ -85,8 +81,8 @@ class InvalidateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): - template_name = "bid_main/mfa/totp.html" success_url = reverse_lazy('bid_main:mfa') + template_name = "bid_main/mfa/totp.html" def get_form(self, *args, **kwargs): kwargs = self.get_form_kwargs() @@ -134,7 +130,7 @@ class DeleteDeviceView(mixins.MfaRequiredMixin, DeleteView): ): # there are other non-recovery devices, it's fine to delete this one return super().get(request, *args, **kwargs) - # this seems to be the last device, we are effectively disabling mfa + # this is the last non-recovery device, we are effectively disabling mfa return redirect('bid_main:mfa_disable') def get_object(self, queryset=None): diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index ca9f774..e802708 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -22,6 +22,7 @@ from django.views.decorators.cache import never_cache from django.views.generic import TemplateView, FormView from django.views.generic.base import View from django.views.generic.edit import UpdateView +from django_otp import devices_for_user from otp_agents.forms import OTPTokenForm import loginas.utils import oauth2_provider.models as oauth2_models @@ -75,6 +76,7 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agen """Shows the login view.""" otp_authentication_form = forms.AuthenticationForm + otp_token_form = forms.MfaForm page_id = "login" redirect_authenticated_user = False success_url_allowed_hosts = settings.NEXT_REDIR_AFTER_LOGIN_ALLOWED_HOSTS @@ -85,10 +87,21 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agen def get_context_data(self, **kwargs) -> dict: ctx = super().get_context_data(**kwargs) self.find_oauth_flow(ctx) - ctx["is_authentication_form"] = isinstance(self.get_form(), forms.AuthenticationForm) - ctx["is_mfa_form"] = isinstance(self.get_form(), OTPTokenForm) + form = self.get_form() + if isinstance(form, forms.MfaForm): + ctx["is_mfa_form"] = True + ctx["devices"] = self.request.user.mfa_devices_per_category() + ctx["use_recovery"] = self.request.GET.get("use_recovery", False) + else: + ctx["is_authentication_form"] = isinstance(form, forms.AuthenticationForm) return ctx + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + if self.request.GET.get("use_recovery", False): + kwargs["use_recovery"] = True + return kwargs + def find_oauth_flow(self, ctx: dict): """Figure out if this is an OAuth flow, and for which OAuth Client.""" diff --git a/docs/mfa.md b/docs/mfa.md new file mode 100644 index 0000000..b29bf55 --- /dev/null +++ b/docs/mfa.md @@ -0,0 +1,20 @@ +--- +gitea: none +include_toc: true +--- + +# Multi-factor authentication (MFA) + +## Supported configurations + +- TOTP authenticators +- TOTP + recovery codes + +TODO: yubikeys, one-time code via email + +## Implementation details + +Using a combination of +- django-otp and its device plugins +- django-trusted-agents +- django-otp-agents -- 2.30.2 From 7d0aa08001e336ca9815018ebf822a76b479b0a6 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 13 Aug 2024 10:22:30 +0200 Subject: [PATCH 11/44] small markup tweak for recovery codes --- bid_main/templates/bid_main/mfa/setup.html | 2 +- bid_main/views/mfa.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index b9820e0..906b27d 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -65,7 +65,7 @@ Multi-factor Authentication Setup {% if recovery_codes %}
    {% for code in recovery_codes %} -
  • {{ code }}
    • +
  • {{ code }}
    • {% endfor %}
{% endif %} diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 19803f4..fcf7a83 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -70,7 +70,7 @@ class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): # https://pages.nist.gov/800-63-3/sp800-63b.html#5122-look-up-secret-verifiers # don't use less than 64 bits device.token_set.create(token=random_hex(8).upper()) - return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1') + return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1#recovery-codes') class InvalidateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): -- 2.30.2 From 734de7e49c501edd15290e0eb1b1fab7d8a52b2d Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 13 Aug 2024 14:11:17 +0200 Subject: [PATCH 12/44] use encrypted storage for mfa secrets --- bid_main/forms.py | 6 +- bid_main/models.py | 8 +-- bid_main/templates/bid_main/mfa/setup.html | 2 +- bid_main/views/mfa.py | 23 +++---- bid_main/views/normal_pages.py | 2 - blenderid/settings.py | 18 ++++++ mfa/__init__.py | 0 mfa/apps.py | 6 ++ mfa/migrations/0001_initial.py | 70 ++++++++++++++++++++++ mfa/migrations/__init__.py | 0 mfa/models.py | 50 ++++++++++++++++ requirements.txt | 2 + 12 files changed, 165 insertions(+), 22 deletions(-) create mode 100644 mfa/__init__.py create mode 100644 mfa/apps.py create mode 100644 mfa/migrations/0001_initial.py create mode 100644 mfa/migrations/__init__.py create mode 100644 mfa/models.py diff --git a/bid_main/forms.py b/bid_main/forms.py index 7947ad7..2443653 100644 --- a/bid_main/forms.py +++ b/bid_main/forms.py @@ -13,11 +13,11 @@ from django.template import defaultfilters from django.utils import timezone from django.utils.translation import gettext_lazy as _ from django_otp.oath import TOTP -from django_otp.plugins.otp_totp.models import TOTPDevice from otp_agents.views import OTPTokenForm from .models import User, OAuth2Application from .recaptcha import check_recaptcha +from mfa.models import EncryptedTOTPDevice import bid_main.tasks log = logging.getLogger(__name__) @@ -492,7 +492,7 @@ class TotpMfaForm(forms.Form): ) key = forms.CharField(widget=forms.HiddenInput) name = forms.CharField( - max_length=TOTPDevice._meta.get_field('name').max_length, + max_length=EncryptedTOTPDevice._meta.get_field('name').max_length, widget=forms.TextInput( attrs={"placeholder": "device name (for your convenience)"}, ), @@ -518,7 +518,7 @@ class TotpMfaForm(forms.Form): def save(self): key = self.cleaned_data.get('key') name = self.cleaned_data.get('name') - TOTPDevice.objects.create(key=key, name=name, user=self.user) + EncryptedTOTPDevice.objects.create(encrypted_key=key, key='', name=name, user=self.user) @classmethod def sign(cls, user, key): diff --git a/bid_main/models.py b/bid_main/models.py index 3d93d77..1ef136e 100644 --- a/bid_main/models.py +++ b/bid_main/models.py @@ -15,9 +15,6 @@ from django.core.mail import send_mail from django.db import models, transaction from django.db.models import Q from django import urls -from django_otp import devices_for_user -from django_otp.plugins.otp_static.models import StaticDevice -from django_otp.plugins.otp_totp.models import TOTPDevice from django.templatetags.static import static from django.utils.deconstruct import deconstructible from django.utils import timezone @@ -27,6 +24,7 @@ import user_agents from . import fields from . import hashers +from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, devices_for_user import bid_main.file_utils import bid_main.utils @@ -548,9 +546,9 @@ class User(AbstractBaseUser, PermissionsMixin): def mfa_devices_per_category(self): devices_per_category = defaultdict(list) for device in devices_for_user(self): - if isinstance(device, StaticDevice): + if isinstance(device, EncryptedRecoveryDevice): devices_per_category['recovery'].append(device) - if isinstance(device, TOTPDevice): + if isinstance(device, EncryptedTOTPDevice): devices_per_category['totp'].append(device) return devices_per_category diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 906b27d..fca12db 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -51,7 +51,7 @@ Multi-factor Authentication Setup {% with recovery=devices_per_category.recovery.0 %} {% if recovery %}
- {% with code_count=recovery.token_set.count %} + {% with code_count=recovery_codes|length %} {{ code_count }} recovery code{{ code_count|pluralize }} remaining {% if recovery_codes %} Hide diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index fcf7a83..9b00dee 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -9,15 +9,13 @@ from django.urls import reverse, reverse_lazy from django.views.generic import TemplateView from django.views.generic.base import View from django.views.generic.edit import DeleteView, FormView -from django_otp import devices_for_user from django_otp.models import Device -from django_otp.plugins.otp_static.models import StaticDevice -from django_otp.plugins.otp_totp.models import TOTPDevice, default_key from django_otp.util import random_hex import qrcode from . import mixins from bid_main.forms import DisableMfaForm, TotpMfaForm +from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, devices_for_user class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): @@ -34,7 +32,8 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): user_can_setup_recovery = False devices_per_category = user.mfa_devices_per_category() if self.request.GET.get('display_recovery_codes') and 'recovery' in devices_per_category: - recovery_codes = [t.token for t in devices_per_category['recovery'][0].token_set.all()] + recovery_device = devices_per_category['recovery'][0] + recovery_codes = [t.encrypted_token for t in recovery_device.encryptedtoken_set.all()] if devices_per_category.keys() - {'recovery'}: user_can_setup_recovery = True return { @@ -52,7 +51,7 @@ class DisableView(mixins.MfaRequiredMixin, FormView): @transaction.atomic def form_valid(self, form): - for device in devices_for_user(self.request.user, confirmed=None): + for device in devices_for_user(self.request.user): device.delete() return super().form_valid(form) @@ -61,15 +60,17 @@ class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): @transaction.atomic def post(self, request, *args, **kwargs): user = self.request.user - if not list(filter(lambda d: not isinstance(d, StaticDevice), devices_for_user(user))): + if not list( + filter(lambda d: not isinstance(d, EncryptedRecoveryDevice), devices_for_user(user)) + ): # Forbid setting up recovery codes unless the user already has some other method return HttpResponseBadRequest("can't setup recovery codes before other methods") user.staticdevice_set.all().delete() - device = StaticDevice.objects.create(name='recovery', user=user) + device = EncryptedRecoveryDevice.objects.create(name='recovery', user=user) for _ in range(10): # https://pages.nist.gov/800-63-3/sp800-63b.html#5122-look-up-secret-verifiers # don't use less than 64 bits - device.token_set.create(token=random_hex(8).upper()) + device.encryptedtoken_set.create(encrypted_token=random_hex(8).upper()) return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1#recovery-codes') @@ -86,7 +87,7 @@ class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): def get_form(self, *args, **kwargs): kwargs = self.get_form_kwargs() - key = self.request.POST.get('key', default_key()) + key = self.request.POST.get('key', random_hex(20)) kwargs['initial']['key'] = key kwargs['initial']['signature'] = TotpMfaForm.sign(kwargs['user'], key) return TotpMfaForm(**kwargs) @@ -101,7 +102,7 @@ class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): key = context['form'].initial['key'] b32key = b32encode(unhexlify(key)).decode('utf-8') context['manual_secret_key'] = b32key - device = TOTPDevice(key=key, user=self.request.user) + device = EncryptedTOTPDevice(encrypted_key=key, user=self.request.user) context['config_url'] = device.config_url image = qrcode.make( device.config_url, @@ -126,7 +127,7 @@ class DeleteDeviceView(mixins.MfaRequiredMixin, DeleteView): for device in devices_for_user(self.request.user): if ( device.persistent_id != kwargs['persistent_id'] - and not isinstance(device, StaticDevice) + and not isinstance(device, EncryptedRecoveryDevice) ): # there are other non-recovery devices, it's fine to delete this one return super().get(request, *args, **kwargs) diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index e802708..fd4105c 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -22,8 +22,6 @@ from django.views.decorators.cache import never_cache from django.views.generic import TemplateView, FormView from django.views.generic.base import View from django.views.generic.edit import UpdateView -from django_otp import devices_for_user -from otp_agents.forms import OTPTokenForm import loginas.utils import oauth2_provider.models as oauth2_models import otp_agents.views diff --git a/blenderid/settings.py b/blenderid/settings.py index e832b27..f383ab8 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -36,9 +36,25 @@ PREFERRED_SCHEME = "https" # Update this to something unique for your machine. SECRET_KEY = os.getenv('SECRET_KEY', 'default-dev-secret') +# NACL_FIELDS_KEY is used to encrypt MFA shared secrets +# !!!!!!!!!!!! +# !!!DANGER!!! its loss or bad rotation will lock out all users with MFA!!! +# !!!!!!!!!!!! +# generate a prod key with ./manage.py createkey +# and put it as a string in the .env file, +# .encode('ascii') takes care of making the variable populated with a byte array +NACL_FIELDS_KEY = os.getenv( + 'NACL_FIELDS_KEY', 'N5W|iA&lZ7iGsy92=qlr>~heUoH4ZX16pCEGG*R+' +).encode('ascii') + # SECURITY WARNING: don't run with debug turned on in production! DEBUG = bool(os.getenv('DEBUG', False)) +if not DEBUG and NACL_FIELDS_KEY == b'N5W|iA&lZ7iGsy92=qlr>~heUoH4ZX16pCEGG*R+': + raise Exception('please override NACL_FIELDS_KEY in .env') +if not DEBUG and SECRET_KEY == 'default-dev-secret': + raise Exception('please override SECRET_KEY in .env') + TESTING = sys.argv[1:2] == ['test'] ALLOWED_HOSTS = os.getenv('ALLOWED_HOSTS', 'id.local').split(',') @@ -65,10 +81,12 @@ INSTALLED_APPS = [ "sorl.thumbnail", "django_admin_select2", "loginas", + "nacl_encrypted_fields", "bid_main", "bid_api", "bid_addon_support", "background_task", + "mfa", ] MIDDLEWARE = [ diff --git a/mfa/__init__.py b/mfa/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/mfa/apps.py b/mfa/apps.py new file mode 100644 index 0000000..6804758 --- /dev/null +++ b/mfa/apps.py @@ -0,0 +1,6 @@ +from django.apps import AppConfig + + +class MfaConfig(AppConfig): + default_auto_field = 'django.db.models.BigAutoField' + name = "mfa" diff --git a/mfa/migrations/0001_initial.py b/mfa/migrations/0001_initial.py new file mode 100644 index 0000000..f3d92ba --- /dev/null +++ b/mfa/migrations/0001_initial.py @@ -0,0 +1,70 @@ +# Generated by Django 4.2.13 on 2024-08-13 11:33 + +from django.db import migrations, models +import django.db.models.deletion +import nacl_encrypted_fields.fields + + +class Migration(migrations.Migration): + + initial = True + + dependencies = [ + ('otp_static', '0003_add_timestamps'), + ('otp_totp', '0003_add_timestamps'), + ] + + operations = [ + migrations.CreateModel( + name='EncryptedTOTPDevice', + fields=[ + ( + 'totpdevice_ptr', + models.OneToOneField( + auto_created=True, + on_delete=django.db.models.deletion.CASCADE, + parent_link=True, + primary_key=True, + serialize=False, + to='otp_totp.totpdevice', + ), + ), + ('encrypted_key', nacl_encrypted_fields.fields.NaClCharField(max_length=255)), + ], + options={ + 'abstract': False, + }, + bases=('otp_totp.totpdevice',), + ), + migrations.CreateModel( + name='EncryptedRecoveryDevice', + fields=[], + options={ + 'abstract': False, + 'proxy': True, + 'indexes': [], + 'constraints': [], + }, + bases=('otp_static.staticdevice',), + ), + migrations.CreateModel( + name='EncryptedStaticToken', + fields=[ + ( + 'id', + models.BigAutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name='ID' + ), + ), + ('encrypted_token', nacl_encrypted_fields.fields.NaClCharField(max_length=255)), + ( + 'device', + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name='encryptedtoken_set', + to='mfa.encryptedrecoverydevice', + ), + ), + ], + ), + ] diff --git a/mfa/migrations/__init__.py b/mfa/migrations/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/mfa/models.py b/mfa/models.py new file mode 100644 index 0000000..c5700c4 --- /dev/null +++ b/mfa/models.py @@ -0,0 +1,50 @@ +from binascii import unhexlify + +from django.db import models +from django_otp.plugins.otp_static.models import StaticDevice +from django_otp.plugins.otp_totp.models import TOTPDevice +from nacl_encrypted_fields.fields import NaClCharField + + +class EncryptedRecoveryDevice(StaticDevice): + class Meta(StaticDevice.Meta): + abstract = False + proxy = True + + def verify_token(self, token): + verify_allowed, _ = self.verify_is_allowed() + if verify_allowed: + match = self.encryptedtoken_set.filter(encrypted_token=token).first() + if match is not None: + match.delete() + self.throttle_reset(commit=False) + self.set_last_used_timestamp(commit=False) + self.save() + else: + self.throttle_increment() + else: + match = None + + return match is not None + + +class EncryptedStaticToken(models.Model): + device = models.ForeignKey( + EncryptedRecoveryDevice, related_name='encryptedtoken_set', on_delete=models.CASCADE + ) + encrypted_token = NaClCharField(max_length=255) + + +class EncryptedTOTPDevice(TOTPDevice): + encrypted_key = NaClCharField(max_length=255) + + @property + def bin_key(self): + return unhexlify(self.encrypted_key.encode()) + + +def devices_for_user(user): + return [ + *EncryptedRecoveryDevice.objects.filter(user=user).all(), + *EncryptedTOTPDevice.objects.filter(user=user).all(), + ] diff --git a/requirements.txt b/requirements.txt index 2cc0767..9bf8944 100644 --- a/requirements.txt +++ b/requirements.txt @@ -15,6 +15,7 @@ django-background-tasks-updated @ git+https://projects.blender.org/infrastructur django[bcrypt]==4.2.13 ; python_version >= "3.8" and python_version < "4" django-compat==1.0.15 ; python_version >= "3.8" and python_version < "4" django-loginas==0.3.11 ; python_version >= "3.8" and python_version < "4" +django-nacl-fields==4.1.0 django-oauth-toolkit @ git+https://projects.blender.org/Oleg-Komarov/django-oauth-toolkit.git@0b056a99ca943771615b859f48aaff0e12357f22 ; python_version >= "3.8" and python_version < "4" django-otp==1.5.1 django-otp-agents==1.0.1 @@ -41,6 +42,7 @@ pycparser==2.19 ; python_version >= "3.8" and python_version < "4" pygments==2.17.2 ; python_version >= "3.8" and python_version < "4" pyinstrument==4.6.0 ; python_version >= "3.8" and python_version < "4" pymdown-extensions==10.7 ; python_version >= "3.8" and python_version < "4" +pynacl==1.5.0 pypng==0.20220715.0 pypugjs==5.9.12 ; python_version >= "3.8" and python_version < "4" python-dateutil==2.8.1 ; python_version >= "3.8" and python_version < "4" -- 2.30.2 From 93f501f289f517d95d14c95d578a502ab0c166e3 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 13 Aug 2024 15:00:18 +0200 Subject: [PATCH 13/44] move forms to mfa app --- bid_main/forms.py | 123 ------------------------------- bid_main/views/mfa.py | 2 +- bid_main/views/normal_pages.py | 5 +- mfa/forms.py | 127 +++++++++++++++++++++++++++++++++ 4 files changed, 131 insertions(+), 126 deletions(-) create mode 100644 mfa/forms.py diff --git a/bid_main/forms.py b/bid_main/forms.py index 2443653..fc04797 100644 --- a/bid_main/forms.py +++ b/bid_main/forms.py @@ -1,23 +1,17 @@ -from binascii import unhexlify import logging import pathlib from django import forms from django.conf import settings from django.contrib.auth import forms as auth_forms, password_validation, authenticate -from django.core.signing import BadSignature, TimestampSigner -from django.core.validators import RegexValidator from django.db import transaction from django.forms import ValidationError from django.template import defaultfilters from django.utils import timezone from django.utils.translation import gettext_lazy as _ -from django_otp.oath import TOTP -from otp_agents.views import OTPTokenForm from .models import User, OAuth2Application from .recaptcha import check_recaptcha -from mfa.models import EncryptedTOTPDevice import bid_main.tasks log = logging.getLogger(__name__) @@ -214,52 +208,6 @@ class AuthenticationForm(auth_forms.AuthenticationForm): self.fields["password"].widget.attrs["placeholder"] = "Enter your password" -class MfaForm(BootstrapModelFormMixin, OTPTokenForm): - """Restyle the form widgets to do less work in the template.""" - - def __init__(self, *args, **kwargs): - use_recovery = kwargs.pop('use_recovery', False) - super().__init__(*args, **kwargs) - - otp_token = self.fields["otp_token"] - otp_token.label = _('Code') - otp_token.required = True - if use_recovery: - otp_token.validators = [RegexValidator(r'^[A-F0-9]{16}$')] - otp_token.widget = forms.TextInput( - attrs={ - "autocomplete": "one-time-code", - "maxlength": 16, - "pattern": "[A-F0-9]{16}", - "placeholder": "123456890ABCDEF", - }, - ) - else: - otp_token.validators = [RegexValidator(r'^[0-9]{6}$')] - otp_token.widget = forms.TextInput( - attrs={ - "autocomplete": "one-time-code", - "inputmode": "numeric", - "maxlength": 6, - "pattern": "[0-9]{6}", - "placeholder": "123456", - }, - ) - - otp_trust_agent = self.fields["otp_trust_agent"] - otp_trust_agent.help_text = _( - "We won't ask for MFA next time you sign-in on this device. " - "Use only on your private device." - ) - otp_trust_agent.initial = False - otp_trust_agent.label = _("Remember this device") - otp_trust_agent.widget = forms.CheckboxInput( - attrs={ - "autocomplete": "off", - }, - ) - - class UserProfileForm(BootstrapModelFormMixin, forms.ModelForm): """Edits full name and email address. @@ -461,74 +409,3 @@ class OAuth2ApplicationForm(forms.ModelForm): super().__init__(*args, **kwargs) for key in readonly_fields: self.fields[key].widget.attrs['readonly'] = True - - -class DisableMfaForm(forms.Form): - disable_mfa_confirm = forms.BooleanField( - help_text="Confirming disabling of multi-factor authentication", - initial=False, - label=_('Confirm'), - required=True, - widget=forms.CheckboxInput( - attrs={ - "autocomplete": "off", - }, - ), - ) - - -class TotpMfaForm(forms.Form): - code = forms.CharField( - validators=[RegexValidator(r'^[0-9]{6}$')], - widget=forms.TextInput( - attrs={ - "autocomplete": "one-time-code", - "inputmode": "numeric", - "maxlength": 6, - "pattern": "[0-9]{6}", - "placeholder": "123456", - }, - ), - ) - key = forms.CharField(widget=forms.HiddenInput) - name = forms.CharField( - max_length=EncryptedTOTPDevice._meta.get_field('name').max_length, - widget=forms.TextInput( - attrs={"placeholder": "device name (for your convenience)"}, - ), - ) - signature = forms.CharField(widget=forms.HiddenInput) - - def __init__(self, *args, **kwargs): - self.user = kwargs.pop('user', None) - super().__init__(*args, **kwargs) - - def clean(self): - super().clean() - code = self.data.get('code') - key = self.data.get('key') - signature = self.cleaned_data.get('signature') - if not self.verify_signature(self.user, key, signature): - raise forms.ValidationError(_('Invalid signature')) - self.cleaned_data['key'] = key - if not TOTP(unhexlify(key)).verify(int(code), tolerance=1): - self.add_error('code', _('Invalid code')) - return self.cleaned_data - - def save(self): - key = self.cleaned_data.get('key') - name = self.cleaned_data.get('name') - EncryptedTOTPDevice.objects.create(encrypted_key=key, key='', name=name, user=self.user) - - @classmethod - def sign(cls, user, key): - signer = TimestampSigner() - return signer.sign(f'{user.email}_{key}') - - @classmethod - def verify_signature(cls, user, key, signature, max_age=3600): - signer = TimestampSigner() - try: - return f'{user.email}_{key}' == signer.unsign(signature, max_age=max_age) - except BadSignature: - return False diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 9b00dee..2bf6446 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -14,7 +14,7 @@ from django_otp.util import random_hex import qrcode from . import mixins -from bid_main.forms import DisableMfaForm, TotpMfaForm +from mfa.forms import DisableMfaForm, TotpMfaForm from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, devices_for_user diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index fd4105c..b2cfe78 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -29,6 +29,7 @@ import otp_agents.views from .. import forms, email from . import mixins from bid_main.email import send_verify_address +from mfa.forms import MfaForm import bid_main.file_utils User = get_user_model() @@ -74,7 +75,7 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agen """Shows the login view.""" otp_authentication_form = forms.AuthenticationForm - otp_token_form = forms.MfaForm + otp_token_form = MfaForm page_id = "login" redirect_authenticated_user = False success_url_allowed_hosts = settings.NEXT_REDIR_AFTER_LOGIN_ALLOWED_HOSTS @@ -86,7 +87,7 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agen ctx = super().get_context_data(**kwargs) self.find_oauth_flow(ctx) form = self.get_form() - if isinstance(form, forms.MfaForm): + if isinstance(form, MfaForm): ctx["is_mfa_form"] = True ctx["devices"] = self.request.user.mfa_devices_per_category() ctx["use_recovery"] = self.request.GET.get("use_recovery", False) diff --git a/mfa/forms.py b/mfa/forms.py new file mode 100644 index 0000000..901d4df --- /dev/null +++ b/mfa/forms.py @@ -0,0 +1,127 @@ +from binascii import unhexlify + +from django import forms +from django.core.signing import BadSignature, TimestampSigner +from django.core.validators import RegexValidator +from django.utils.translation import gettext_lazy as _ +from django_otp.oath import TOTP +from otp_agents.views import OTPTokenForm + +from mfa.models import EncryptedTOTPDevice + + +class MfaForm(OTPTokenForm): + """Restyle the form widgets to do less work in the template.""" + + def __init__(self, *args, **kwargs): + use_recovery = kwargs.pop('use_recovery', False) + super().__init__(*args, **kwargs) + + otp_token = self.fields["otp_token"] + otp_token.label = _('Code') + otp_token.required = True + if use_recovery: + otp_token.validators = [RegexValidator(r'^[A-F0-9]{16}$')] + otp_token.widget = forms.TextInput( + attrs={ + "autocomplete": "one-time-code", + "maxlength": 16, + "pattern": "[A-F0-9]{16}", + "placeholder": "123456890ABCDEF", + }, + ) + else: + otp_token.validators = [RegexValidator(r'^[0-9]{6}$')] + otp_token.widget = forms.TextInput( + attrs={ + "autocomplete": "one-time-code", + "inputmode": "numeric", + "maxlength": 6, + "pattern": "[0-9]{6}", + "placeholder": "123456", + }, + ) + + otp_trust_agent = self.fields["otp_trust_agent"] + otp_trust_agent.help_text = _( + "We won't ask for MFA next time you sign-in on this device. " + "Use only on your private device." + ) + otp_trust_agent.initial = False + otp_trust_agent.label = _("Remember this device") + otp_trust_agent.widget = forms.CheckboxInput( + attrs={ + "autocomplete": "off", + }, + ) + + +class DisableMfaForm(forms.Form): + disable_mfa_confirm = forms.BooleanField( + help_text="Confirming disabling of multi-factor authentication", + initial=False, + label=_('Confirm'), + required=True, + widget=forms.CheckboxInput( + attrs={ + "autocomplete": "off", + }, + ), + ) + + +class TotpMfaForm(forms.Form): + code = forms.CharField( + validators=[RegexValidator(r'^[0-9]{6}$')], + widget=forms.TextInput( + attrs={ + "autocomplete": "one-time-code", + "inputmode": "numeric", + "maxlength": 6, + "pattern": "[0-9]{6}", + "placeholder": "123456", + }, + ), + ) + key = forms.CharField(widget=forms.HiddenInput) + name = forms.CharField( + max_length=EncryptedTOTPDevice._meta.get_field('name').max_length, + widget=forms.TextInput( + attrs={"placeholder": "device name (for your convenience)"}, + ), + ) + signature = forms.CharField(widget=forms.HiddenInput) + + def __init__(self, *args, **kwargs): + self.user = kwargs.pop('user', None) + super().__init__(*args, **kwargs) + + def clean(self): + super().clean() + code = self.data.get('code') + key = self.data.get('key') + signature = self.cleaned_data.get('signature') + if not self.verify_signature(self.user, key, signature): + raise forms.ValidationError(_('Invalid signature')) + self.cleaned_data['key'] = key + if not TOTP(unhexlify(key)).verify(int(code), tolerance=1): + self.add_error('code', _('Invalid code')) + return self.cleaned_data + + def save(self): + key = self.cleaned_data.get('key') + name = self.cleaned_data.get('name') + EncryptedTOTPDevice.objects.create(encrypted_key=key, key='', name=name, user=self.user) + + @classmethod + def sign(cls, user, key): + signer = TimestampSigner() + return signer.sign(f'{user.email}_{key}') + + @classmethod + def verify_signature(cls, user, key, signature, max_age=3600): + signer = TimestampSigner() + try: + return f'{user.email}_{key}' == signer.unsign(signature, max_age=max_age) + except BadSignature: + return False -- 2.30.2 From 7356a989bf9e3fca7003998200eb9bbcdf90f427 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 13 Aug 2024 20:54:23 +0200 Subject: [PATCH 14/44] docs, comments etc --- bid_main/templates/bid_main/mfa/setup.html | 24 ++++++++++++------ bid_main/templates/bid_main/mfa/totp.html | 18 ++++++++++++-- bid_main/views/mfa.py | 2 ++ docs/mfa.md | 29 ++++++++++++++++++---- mfa/models.py | 26 +++++++++++++++++++ 5 files changed, 85 insertions(+), 14 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index fca12db..2849f99 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -12,12 +12,14 @@ Multi-factor Authentication Setup You have configured MFA for your account. You can disable MFA at any time, but you have to sign-in using your authentication device or a recovery code.

+

TODO explain remember me and trusted days

{% else %}

MFA makes your account more secure against account takeover attacks. + You can read more in a guide from Electronic Frontier Foundation.

If you have privileged access (admin or moderator) on any of Blender websites, you should setup MFA to avoid potential harm done to other community members through misuse of your account. @@ -27,16 +29,24 @@ Multi-factor Authentication Setup

Time-based one-time password (TOTP)

-

TODO Explain how to use TOTP, TOTP applications

- {% with devices=devices_per_category.totp %} - {% if devices %} +

+ Also known as authenticator application. +

+

+ If you don't have an authenticator application, you can choose one from a list of TOTP applications. +

+ {% if devices_per_category.totp and not devices_per_category.recovery %} +

+ Please make sure that you do not lock yourself out: + generate and store recovery codes as a backup verification method. + If you lose your authenticator device you can use a recovery code to login and reconfigure your MFA methods. +

+ {% endif %}
    - {% for d in devices %} + {% for d in devices_per_category.totp %}
  • {{ d.name }}
    • {% endfor %}
- {% endif %} - {% endwith %} Configure a new TOTP device
@@ -44,7 +54,7 @@ Multi-factor Authentication Setup

Recovery codes

- Store your recovery codes safely (e.g. in a password manager) and don't share them. + Store your recovery codes safely (e.g. in a password manager or use a printed copy) and don't share them. Each code can be used only once. You can generate a new set of recovery codes at any time, any remaining old codes will become invalidated.

diff --git a/bid_main/templates/bid_main/mfa/totp.html b/bid_main/templates/bid_main/mfa/totp.html index 4cb4489..9efe30d 100644 --- a/bid_main/templates/bid_main/mfa/totp.html +++ b/bid_main/templates/bid_main/mfa/totp.html @@ -12,11 +12,25 @@ Multi-factor Authentication Setup
QR code -
show code for manual entry{{ manual_secret_key }}
+
show secret key for manual entry{{ manual_secret_key }}
-

TODO explain what's happening

+
    +
  1. + Open your authenticator app and add a new entry by scanning the QR code from this page. + If you can't scan the QR code, you can enter the secret key manually. +
    2. +
  2. Enter a 6-digit code shown in the authenticator.
    3. +
  3. Pick any device name for your authenticator that you would recognize (e.g. "FreeOTP on my phone") and submit the form.
    4. + {% if first_device %} +
  4. Since this is your first MFA device, you will be promted to enter a new code once more to sign-in using MFA.
    5. + {% endif %} +
+
+
+
+
{% with form=form|add_form_classes %} {% csrf_token %} {% with field=form.name %} diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 2bf6446..6d7fd13 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -106,11 +106,13 @@ class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): context['config_url'] = device.config_url image = qrcode.make( device.config_url, + border=1, error_correction=qrcode.constants.ERROR_CORRECT_H, ) buf = BytesIO() image.save(buf) context['qrcode'] = b64encode(buf.getvalue()).decode('utf-8') + context['first_device'] = not devices_for_user(self.request.user) return context def form_valid(self, form): diff --git a/docs/mfa.md b/docs/mfa.md index b29bf55..8c35733 100644 --- a/docs/mfa.md +++ b/docs/mfa.md @@ -10,11 +10,30 @@ include_toc: true - TOTP authenticators - TOTP + recovery codes -TODO: yubikeys, one-time code via email +TODO: yubikeys, one-time recovery code via email ## Implementation details -Using a combination of -- django-otp and its device plugins -- django-trusted-agents -- django-otp-agents +Dependencies: +- django-otp and its device plugins: + for implementing verification and throttling logic +- django=trusted-agents and django-otp-agents: + for otp_required decorator that supports accept_trusted_agent to implement persistence +- django-nacl-fields and pynacl: + for transparent handling of encrypted secret fields + +Settings: +- AGENT_COOKIE_SECURE +- AGENT_INACTIVITY_DAYS +- AGENT_TRUST_DAYS +- NACL_FIELDS_KEY +- OTP_TOTP_ISSUER + +### Considered alternatives + +django-allauth: + +- seems to be less modular/pluggable, and requires a more careful consideration of compatibility +with our existing code base, e.g. our current OAuth flows implementation; +- is more opinionated, but does a lot of things correctly, is actively developed, and may be +considered in the future. diff --git a/mfa/models.py b/mfa/models.py index c5700c4..5dc7d90 100644 --- a/mfa/models.py +++ b/mfa/models.py @@ -1,3 +1,13 @@ +"""MFA device models. + +In a perfect world this code would have not be needed, but django_otp seems to be the best match for +our codebase at this point, despite the fact that it doesn't handle secrets correctly (stores them +in plain text). + +It seems like it's simpler to patch this one shortcoming by defining the following models and reuse +the upstream implementation as much as possible. +""" + from binascii import unhexlify from django.db import models @@ -7,11 +17,13 @@ from nacl_encrypted_fields.fields import NaClCharField class EncryptedRecoveryDevice(StaticDevice): + """Using a proxy model and pretending that upstream StaticToken does not exist.""" class Meta(StaticDevice.Meta): abstract = False proxy = True def verify_token(self, token): + """Copy-pasted verbatim from StaticDevice, replacing token_set with encryptedtoken_set.""" verify_allowed, _ = self.verify_is_allowed() if verify_allowed: match = self.encryptedtoken_set.filter(encrypted_token=token).first() @@ -36,14 +48,28 @@ class EncryptedStaticToken(models.Model): class EncryptedTOTPDevice(TOTPDevice): + """Using multi-table inheritance to introduce an encrypted field. + + A separate table is a bit ugly, but the only alternative I see at this point is a full + copy-paste, since the upstream key field is too small to accommodate an encrypted value. + + Performance overhead of an additional JOIN should be negligible for this table's access pattern: + TOTP setup and verification. + """ encrypted_key = NaClCharField(max_length=255) @property def bin_key(self): + """This override is sufficient to make verify_token use the new field.""" return unhexlify(self.encrypted_key.encode()) def devices_for_user(user): + """ A more specific replacement for upstream devices_for_user. + + Can't use the upstream devices_for_user because using multi-table model inheritance shows up as + duplicate devices. + """ return [ *EncryptedRecoveryDevice.objects.filter(user=user).all(), *EncryptedTOTPDevice.objects.filter(user=user).all(), -- 2.30.2 From ee03f062a49221da922457c4bdb2298b22a44019 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 15 Aug 2024 10:31:14 +0200 Subject: [PATCH 15/44] improve copy --- bid_main/templates/bid_main/mfa/setup.html | 8 ++++++-- bid_main/views/mfa.py | 3 +++ mfa/forms.py | 5 +++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 2849f99..a4dcbc8 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -10,9 +10,13 @@ Multi-factor Authentication Setup {% if user_has_mfa_configured %}

You have configured MFA for your account. - You can disable MFA at any time, but you have to sign-in using your authentication device or a recovery code. + You can disable MFA at any time, but you have to pass the verification using your authentication device or a recovery code. +

+

+ Every time you sign-in on a new device you will be asked to pass the MFA verification. + If you use the "remember this device" option, you won't be prompted for MFA verification for that device in the next {{ agent_trust_days }} days. + Verification also expires after {{ agent_inactivity_days }} days of inactivity.

-

TODO explain remember me and trusted days

diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 6d7fd13..ae59b05 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -2,6 +2,7 @@ from base64 import b32encode, b64encode from binascii import unhexlify from io import BytesIO +from django.conf import settings from django.db import transaction from django.http import Http404, HttpResponseBadRequest from django.shortcuts import redirect @@ -37,6 +38,8 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): if devices_per_category.keys() - {'recovery'}: user_can_setup_recovery = True return { + 'agent_inactivity_days': settings.AGENT_INACTIVITY_DAYS, + 'agent_trust_days': settings.AGENT_TRUST_DAYS, 'devices_per_category': devices_per_category, 'recovery_codes': recovery_codes, 'user_can_setup_recovery': user_can_setup_recovery, diff --git a/mfa/forms.py b/mfa/forms.py index 901d4df..2356690 100644 --- a/mfa/forms.py +++ b/mfa/forms.py @@ -1,6 +1,7 @@ from binascii import unhexlify from django import forms +from django.conf import settings from django.core.signing import BadSignature, TimestampSigner from django.core.validators import RegexValidator from django.utils.translation import gettext_lazy as _ @@ -44,8 +45,8 @@ class MfaForm(OTPTokenForm): otp_trust_agent = self.fields["otp_trust_agent"] otp_trust_agent.help_text = _( - "We won't ask for MFA next time you sign-in on this device. " - "Use only on your private device." + f"We won't ask for MFA on this device in the next {settings.AGENT_TRUST_DAYS} days. " + f"Use only on your private device." ) otp_trust_agent.initial = False otp_trust_agent.label = _("Remember this device") -- 2.30.2 From d38eafb7cb3aa18ee2d5ee72256c089600c8e463 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 15 Aug 2024 11:44:32 +0200 Subject: [PATCH 16/44] send email notifications --- bid_main/email.py | 37 ++++++++++++++ bid_main/signals.py | 8 +++ bid_main/tasks.py | 51 +++++++++++++++++++ .../bid_main/emails/mfa_disabled.txt | 9 ++++ .../bid_main/emails/mfa_recovery_used.txt | 11 ++++ .../bid_main/emails/new_mfa_device.txt | 11 ++++ bid_main/views/mfa.py | 6 +++ mfa/models.py | 11 +++- mfa/signals.py | 3 ++ 9 files changed, 145 insertions(+), 2 deletions(-) create mode 100644 bid_main/templates/bid_main/emails/mfa_disabled.txt create mode 100644 bid_main/templates/bid_main/emails/mfa_recovery_used.txt create mode 100644 bid_main/templates/bid_main/emails/new_mfa_device.txt create mode 100644 mfa/signals.py diff --git a/bid_main/email.py b/bid_main/email.py index 0bf5491..9198bfb 100644 --- a/bid_main/email.py +++ b/bid_main/email.py @@ -282,3 +282,40 @@ def construct_password_changed(user): subject = "Security alert: password changed" return email_body_txt, subject + + +def construct_new_mfa_device(user, device_type): + context = { + "device_type": device_type, + "user": user, + } + email_body_txt = loader.render_to_string( + "bid_main/emails/new_mfa_device.txt", context + ) + subject = "Security alert: a new multi-factor authentication device added" + + return email_body_txt, subject + + +def construct_mfa_disabled(user): + context = { + "user": user, + } + email_body_txt = loader.render_to_string( + "bid_main/emails/mfa_disabled.txt", context + ) + subject = "Security alert: multi-factor authentication disabled" + + return email_body_txt, subject + + +def construct_mfa_recovery_used(user): + context = { + "user": user, + } + email_body_txt = loader.render_to_string( + "bid_main/emails/mfa_recovery_used.txt", context + ) + subject = "Security alert: recovery code used" + + return email_body_txt, subject diff --git a/bid_main/signals.py b/bid_main/signals.py index 1e9b443..9b9d2dc 100644 --- a/bid_main/signals.py +++ b/bid_main/signals.py @@ -7,6 +7,7 @@ from django.db.models.signals import m2m_changed, post_delete from django.dispatch import receiver from . import models +from mfa.signals import recovery_used import bid_main.utils as utils import bid_main.file_utils import bid_main.tasks @@ -79,3 +80,10 @@ def delete_orphaned_avatar_files(sender, instance, **kwargs): return bid_main.file_utils.delete_avatar_files(instance.avatar.name) + + +@receiver(recovery_used) +def send_mfa_recovery_used_email(sender, **kwargs): + user = kwargs['device'].user + if user.confirmed_email_at: + bid_main.tasks.send_mfa_recovery_used_email(user.pk) diff --git a/bid_main/tasks.py b/bid_main/tasks.py index 1e8e86c..77891f1 100644 --- a/bid_main/tasks.py +++ b/bid_main/tasks.py @@ -43,3 +43,54 @@ def send_password_changed_email(user_pk): from_email=None, # just use the configured default From-address. recipient_list=[email], ) + + +@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) +def send_new_mfa_device_email(user_pk, device_type): + user = User.objects.get(pk=user_pk) + log.info("sending a new mfa device email for account %s", user.pk) + + # sending only a text/plain email to reduce the room for look-alike phishing emails + email_body_txt, subject = bid_main.email.construct_new_mfa_device(user, device_type) + + email = user.email + send_mail( + subject=subject, + message=email_body_txt, + from_email=None, # just use the configured default From-address. + recipient_list=[email], + ) + + +@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) +def send_mfa_disabled_email(user_pk): + user = User.objects.get(pk=user_pk) + log.info("sending an mfa disabled email for account %s", user.pk) + + # sending only a text/plain email to reduce the room for look-alike phishing emails + email_body_txt, subject = bid_main.email.construct_mfa_disabled(user) + + email = user.email + send_mail( + subject=subject, + message=email_body_txt, + from_email=None, # just use the configured default From-address. + recipient_list=[email], + ) + + +@background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) +def send_mfa_recovery_used_email(user_pk): + user = User.objects.get(pk=user_pk) + log.info("sending an mfa recovery used email for account %s", user.pk) + + # sending only a text/plain email to reduce the room for look-alike phishing emails + email_body_txt, subject = bid_main.email.construct_mfa_recovery_used(user) + + email = user.email + send_mail( + subject=subject, + message=email_body_txt, + from_email=None, # just use the configured default From-address. + recipient_list=[email], + ) diff --git a/bid_main/templates/bid_main/emails/mfa_disabled.txt b/bid_main/templates/bid_main/emails/mfa_disabled.txt new file mode 100644 index 0000000..fe955b3 --- /dev/null +++ b/bid_main/templates/bid_main/emails/mfa_disabled.txt @@ -0,0 +1,9 @@ +{% autoescape off %} +Dear {{ user.full_name|default:user.email }}! + +Multi-factor authentication has been disabled for your Blender ID account {{ user.email }} + +-- +Kind regards, +The Blender Web Team +{% endautoescape %} diff --git a/bid_main/templates/bid_main/emails/mfa_recovery_used.txt b/bid_main/templates/bid_main/emails/mfa_recovery_used.txt new file mode 100644 index 0000000..1f6bce8 --- /dev/null +++ b/bid_main/templates/bid_main/emails/mfa_recovery_used.txt @@ -0,0 +1,11 @@ +{% autoescape off %} +Dear {{ user.full_name|default:user.email }}! + +A recovery code was used to pass multi-factor authentication for your Blender ID account {{ user.email }} + +If this wasn't done by you, please reset your password immediately, re-generate your MFA recovery codes, and contact blenderid@blender.org for support. + +-- +Kind regards, +The Blender Web Team +{% endautoescape %} diff --git a/bid_main/templates/bid_main/emails/new_mfa_device.txt b/bid_main/templates/bid_main/emails/new_mfa_device.txt new file mode 100644 index 0000000..67a846f --- /dev/null +++ b/bid_main/templates/bid_main/emails/new_mfa_device.txt @@ -0,0 +1,11 @@ +{% autoescape off %} +Dear {{ user.full_name|default:user.email }}! + +A new {{ device_type }} multi-factor authenticator has been added to your Blender ID account {{ user.email }} + +If this wasn't done by you, please reset your password immediately and contact blenderid@blender.org for support. + +-- +Kind regards, +The Blender Web Team +{% endautoescape %} diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index ae59b05..cd3debc 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -17,6 +17,7 @@ import qrcode from . import mixins from mfa.forms import DisableMfaForm, TotpMfaForm from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, devices_for_user +import bid_main.tasks class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): @@ -56,6 +57,8 @@ class DisableView(mixins.MfaRequiredMixin, FormView): def form_valid(self, form): for device in devices_for_user(self.request.user): device.delete() + if self.request.user.confirmed_email_at: + bid_main.tasks.send_mfa_disabled_email(self.request.user.pk) return super().form_valid(form) @@ -118,8 +121,11 @@ class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): context['first_device'] = not devices_for_user(self.request.user) return context + @transaction.atomic def form_valid(self, form): form.save() + if self.request.user.confirmed_email_at: + bid_main.tasks.send_new_mfa_device_email(self.request.user.pk, 'totp') return super().form_valid(form) diff --git a/mfa/models.py b/mfa/models.py index 5dc7d90..30730db 100644 --- a/mfa/models.py +++ b/mfa/models.py @@ -10,11 +10,13 @@ the upstream implementation as much as possible. from binascii import unhexlify -from django.db import models +from django.db import models, transaction from django_otp.plugins.otp_static.models import StaticDevice from django_otp.plugins.otp_totp.models import TOTPDevice from nacl_encrypted_fields.fields import NaClCharField +from mfa.signals import recovery_used + class EncryptedRecoveryDevice(StaticDevice): """Using a proxy model and pretending that upstream StaticToken does not exist.""" @@ -22,8 +24,12 @@ class EncryptedRecoveryDevice(StaticDevice): abstract = False proxy = True + @transaction.atomic def verify_token(self, token): - """Copy-pasted verbatim from StaticDevice, replacing token_set with encryptedtoken_set.""" + """Copy-pasted verbatim from StaticDevice, replacing token_set with encryptedtoken_set. + + Also added a signal for email notification. + """ verify_allowed, _ = self.verify_is_allowed() if verify_allowed: match = self.encryptedtoken_set.filter(encrypted_token=token).first() @@ -32,6 +38,7 @@ class EncryptedRecoveryDevice(StaticDevice): self.throttle_reset(commit=False) self.set_last_used_timestamp(commit=False) self.save() + recovery_used.send(EncryptedRecoveryDevice, device=self) else: self.throttle_increment() else: diff --git a/mfa/signals.py b/mfa/signals.py new file mode 100644 index 0000000..81ab83a --- /dev/null +++ b/mfa/signals.py @@ -0,0 +1,3 @@ +from django.dispatch import Signal + +recovery_used = Signal() -- 2.30.2 From f5c1cdc15ab168873b836b95e2b931d3b89a65f3 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 15 Aug 2024 12:43:17 +0200 Subject: [PATCH 17/44] improve presentation --- bid_main/templates/bid_main/mfa/setup.html | 7 +++++-- blenderid/settings.py | 6 +++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index a4dcbc8..275f44b 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -1,5 +1,5 @@ {% extends 'layout.html' %} -{% load pipeline static %} +{% load humanize pipeline static %} {% block page_title %} Multi-factor Authentication Setup {% endblock %} @@ -48,7 +48,10 @@ Multi-factor Authentication Setup {% endif %}
    {% for d in devices_per_category.totp %} -
  • {{ d.name }}
    • +
  • + {{ d.name }} + {% if d.last_used_at %}(Last used {{ d.last_used_at|naturaltime }}){% endif %} +
    • {% endfor %}
Configure a new TOTP device diff --git a/blenderid/settings.py b/blenderid/settings.py index f383ab8..9d625fe 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -72,10 +72,10 @@ INSTALLED_APPS = [ "django.contrib.staticfiles", "django.contrib.sites", "django.contrib.flatpages", - "django_otp", - "django_otp.plugins.otp_totp", - "django_otp.plugins.otp_static", "django_agent_trust", + "django_otp", + "django_otp.plugins.otp_static", + "django_otp.plugins.otp_totp", "oauth2_provider", "pipeline", "sorl.thumbnail", -- 2.30.2 From 0326c78568cae32580a03f645a5ff105c1550a3e Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 15 Aug 2024 13:21:49 +0200 Subject: [PATCH 18/44] totp setup test --- bid_main/tests/test_mfa.py | 86 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+) create mode 100644 bid_main/tests/test_mfa.py diff --git a/bid_main/tests/test_mfa.py b/bid_main/tests/test_mfa.py new file mode 100644 index 0000000..faaf859 --- /dev/null +++ b/bid_main/tests/test_mfa.py @@ -0,0 +1,86 @@ +from unittest.mock import patch +import re + +from django.test.client import Client +from django.test import TestCase +from django.utils import timezone + +from bid_main.tests.factories import UserFactory +from mfa.models import devices_for_user + + +@patch( + 'django.contrib.auth.base_user.AbstractBaseUser.check_password', + new=lambda _, pwd: pwd == 'hunter2', +) +@patch( + 'django_otp.oath.TOTP.verify', + new=lambda _, token, *args, **kwargs: int(token) == 123456, +) +class TestMfaRequredIfConfigured(TestCase): + def setUp(self): + self.user = UserFactory( + confirmed_email_at=timezone.now(), + privacy_policy_agreed=timezone.now(), + ) + + def test_no_mfa(self): + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + # showing account page, after a redirect + self.assertEqual(response.status_code, 200) + self.assertContains(response, '

Account

') + + def test_setup_totp(self): + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + self.assertEqual(response.status_code, 200) + response = client.get('/mfa/totp/') + match = re.search( + r'input type="hidden" name="key" value="([^"]+)"', str(response.content) + ) + key = match.group(1) + match = re.search( + r'input type="hidden" name="signature" value="([^"]+)"', str(response.content) + ) + signature = match.group(1) + response = client.post( + '/mfa/totp/', + {'name': 'test totp device', 'code': '123456', 'key': key, 'signature': signature}, + follow=True + ) + self.assertEqual(response.status_code, 200) + self.assertEqual(devices_for_user(self.user)[0].name, 'test totp device') + + # emulating a different browser + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + self.assertEqual(response.status_code, 200) + # haven't reached the account page + self.assertNotContains(response, '

Account

') + self.assertContains(response, 'autocomplete="one-time-code"') + match = re.search( + r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) + ) + otp_device = match.group(1) + + response = client.post( + '/login', + {'otp_device': otp_device, 'otp_token': '123456'}, + follow=True, + ) + # have reached the account page + self.assertEqual(response.status_code, 200) + self.assertContains(response, '

Account

') -- 2.30.2 From f8e1655be5d8ba90f8e8016f64003032a23aaaa7 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 16 Aug 2024 13:01:39 +0200 Subject: [PATCH 19/44] fix recovery codes counter --- bid_main/templates/bid_main/mfa/setup.html | 19 +++++++++---------- bid_main/views/mfa.py | 3 ++- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 275f44b..9229970 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -12,6 +12,13 @@ Multi-factor Authentication Setup You have configured MFA for your account. You can disable MFA at any time, but you have to pass the verification using your authentication device or a recovery code.

+ {% if devices_per_category.totp and not devices_per_category.recovery %} +

+ Please make sure that you do not lock yourself out: + generate and store recovery codes as a backup verification method. + If you lose your authenticator device you can use a recovery code to login and reconfigure your MFA methods. +

+ {% endif %}

Every time you sign-in on a new device you will be asked to pass the MFA verification. If you use the "remember this device" option, you won't be prompted for MFA verification for that device in the next {{ agent_trust_days }} days. @@ -39,13 +46,6 @@ Multi-factor Authentication Setup

If you don't have an authenticator application, you can choose one from a list of TOTP applications.

- {% if devices_per_category.totp and not devices_per_category.recovery %} -

- Please make sure that you do not lock yourself out: - generate and store recovery codes as a backup verification method. - If you lose your authenticator device you can use a recovery code to login and reconfigure your MFA methods. -

- {% endif %}
    {% for d in devices_per_category.totp %}
  • @@ -70,7 +70,7 @@ Multi-factor Authentication Setup
    {% with code_count=recovery_codes|length %} {{ code_count }} recovery code{{ code_count|pluralize }} remaining - {% if recovery_codes %} + {% if display_recovery_codes %} Hide {% else %} Display @@ -78,8 +78,7 @@ Multi-factor Authentication Setup {% csrf_token %} - {# populated on display_recovery_codes=1 #} - {% if recovery_codes %} + {% if display_recovery_codes %}
      {% for code in recovery_codes %}
    • {{ code }}
      • diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index cd3debc..1649bff 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -33,7 +33,7 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): recovery_codes = [] user_can_setup_recovery = False devices_per_category = user.mfa_devices_per_category() - if self.request.GET.get('display_recovery_codes') and 'recovery' in devices_per_category: + if 'recovery' in devices_per_category: recovery_device = devices_per_category['recovery'][0] recovery_codes = [t.encrypted_token for t in recovery_device.encryptedtoken_set.all()] if devices_per_category.keys() - {'recovery'}: @@ -42,6 +42,7 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): 'agent_inactivity_days': settings.AGENT_INACTIVITY_DAYS, 'agent_trust_days': settings.AGENT_TRUST_DAYS, 'devices_per_category': devices_per_category, + 'display_recovery_codes': self.request.GET.get('display_recovery_codes'), 'recovery_codes': recovery_codes, 'user_can_setup_recovery': user_can_setup_recovery, 'user_has_mfa_configured': bool(devices_per_category), -- 2.30.2 From aeca8ed656df4f619100c9807ed81e06f3736344 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 16 Aug 2024 15:37:07 +0200 Subject: [PATCH 20/44] fix EncryptedRecoverDevice + test --- bid_main/tests/test_mfa.py | 56 ++++++++++++++++++++++++++++++++++++-- mfa/models.py | 6 +++- 2 files changed, 58 insertions(+), 4 deletions(-) diff --git a/bid_main/tests/test_mfa.py b/bid_main/tests/test_mfa.py index faaf859..8d80754 100644 --- a/bid_main/tests/test_mfa.py +++ b/bid_main/tests/test_mfa.py @@ -3,10 +3,11 @@ import re from django.test.client import Client from django.test import TestCase +from django.urls import reverse from django.utils import timezone from bid_main.tests.factories import UserFactory -from mfa.models import devices_for_user +from mfa.models import EncryptedTOTPDevice, devices_for_user @patch( @@ -43,7 +44,7 @@ class TestMfaRequredIfConfigured(TestCase): follow=True, ) self.assertEqual(response.status_code, 200) - response = client.get('/mfa/totp/') + response = client.get(reverse('bid_main:mfa_totp')) match = re.search( r'input type="hidden" name="key" value="([^"]+)"', str(response.content) ) @@ -53,7 +54,7 @@ class TestMfaRequredIfConfigured(TestCase): ) signature = match.group(1) response = client.post( - '/mfa/totp/', + reverse('bid_main:mfa_totp'), {'name': 'test totp device', 'code': '123456', 'key': key, 'signature': signature}, follow=True ) @@ -84,3 +85,52 @@ class TestMfaRequredIfConfigured(TestCase): # have reached the account page self.assertEqual(response.status_code, 200) self.assertContains(response, '

      Account

      ') + + def test_recovery_codes(self): + # shortcut: create a totp device via a model + EncryptedTOTPDevice.objects.create(encrypted_key='00', key='', name='totp', user=self.user) + + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + match = re.search( + r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) + ) + otp_device = match.group(1) + response = client.post( + '/login', + {'otp_device': otp_device, 'otp_token': '123456'}, + follow=True, + ) + self.assertContains(response, '

      Account

      ') + + response = client.post(reverse('bid_main:mfa_generate_recovery'), follow=True) + self.assertContains(response, '10 recovery codes remaining') + + # hope that we don't add any other code elements + match = re.search( + r'([0-9A-F]{16})', str(response.content) + ) + recovery_code = match.group(1) + + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + response = client.get('/login?use_recovery=1') + match = re.search( + r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) + ) + otp_device = match.group(1) + response = client.post( + '/login?use_recovery=1', + {'otp_device': otp_device, 'otp_token': recovery_code}, + ) + self.assertEqual(response.status_code, 302) + response = client.get(reverse('bid_main:mfa')) + self.assertContains(response, '9 recovery codes remaining') diff --git a/mfa/models.py b/mfa/models.py index 30730db..fc327d9 100644 --- a/mfa/models.py +++ b/mfa/models.py @@ -28,11 +28,15 @@ class EncryptedRecoveryDevice(StaticDevice): def verify_token(self, token): """Copy-pasted verbatim from StaticDevice, replacing token_set with encryptedtoken_set. + ORM filter is rewritten to a loop, because looking up encrypted values doesn't work. Also added a signal for email notification. """ verify_allowed, _ = self.verify_is_allowed() if verify_allowed: - match = self.encryptedtoken_set.filter(encrypted_token=token).first() + match = None + for t in self.encryptedtoken_set.all(): + if t.encrypted_token == token: + match = t if match is not None: match.delete() self.throttle_reset(commit=False) -- 2.30.2 From 15f3d6fc5b192b3d50f5d7df2a7ac3676b9fff26 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 16 Aug 2024 15:38:43 +0200 Subject: [PATCH 21/44] more tests --- bid_main/tests/test_mfa.py | 111 ++++++++++++++++++++++++++++++++++++- 1 file changed, 110 insertions(+), 1 deletion(-) diff --git a/bid_main/tests/test_mfa.py b/bid_main/tests/test_mfa.py index 8d80754..16d1d10 100644 --- a/bid_main/tests/test_mfa.py +++ b/bid_main/tests/test_mfa.py @@ -18,7 +18,7 @@ from mfa.models import EncryptedTOTPDevice, devices_for_user 'django_otp.oath.TOTP.verify', new=lambda _, token, *args, **kwargs: int(token) == 123456, ) -class TestMfaRequredIfConfigured(TestCase): +class TestMfa(TestCase): def setUp(self): self.user = UserFactory( confirmed_email_at=timezone.now(), @@ -134,3 +134,112 @@ class TestMfaRequredIfConfigured(TestCase): self.assertEqual(response.status_code, 302) response = client.get(reverse('bid_main:mfa')) self.assertContains(response, '9 recovery codes remaining') + + # test that can't reuse the same code, repeating the steps + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + response = client.get('/login?use_recovery=1') + match = re.search( + r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) + ) + otp_device = match.group(1) + response = client.post( + '/login?use_recovery=1', + {'otp_device': otp_device, 'otp_token': recovery_code}, + ) + self.assertEqual(response.status_code, 200) + + def test_disable_mfa(self): + # shortcut: create a totp device via a model + EncryptedTOTPDevice.objects.create(encrypted_key='00', key='', name='totp', user=self.user) + + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + match = re.search( + r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) + ) + otp_device = match.group(1) + response = client.post( + '/login', + {'otp_device': otp_device, 'otp_token': '123456'}, + follow=True, + ) + + client.post(reverse('bid_main:mfa_disable'), {'disable_mfa_confirm': 'True'}) + + # no mfa requried now + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + self.assertContains(response, '

      Account

      ') + + def test_remember_device(self): + # shortcut: create a totp device via a model + EncryptedTOTPDevice.objects.create(encrypted_key='00', key='', name='totp', user=self.user) + + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + match = re.search( + r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) + ) + otp_device = match.group(1) + response = client.post( + '/login', + {'otp_device': otp_device, 'otp_token': '123456', 'otp_trust_agent': 'True'}, + follow=True, + ) + + client.get('/logout', follow=True) + + # no mfa requried now, same client + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + self.assertContains(response, '

      Account

      ') + + def test_dont_remember_device(self): + # shortcut: create a totp device via a model + EncryptedTOTPDevice.objects.create(encrypted_key='00', key='', name='totp', user=self.user) + + client = Client() + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + match = re.search( + r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) + ) + otp_device = match.group(1) + response = client.post( + '/login', + {'otp_device': otp_device, 'otp_token': '123456', 'otp_trust_agent': ''}, + follow=True, + ) + + client.get('/logout', follow=True) + + # no mfa requried now, same client + response = client.post( + '/login', + {'username': self.user.email, 'password': 'hunter2'}, + follow=True, + ) + self.assertNotContains(response, '

      Account

      ') -- 2.30.2 From 47b042c422165ee1cc0bfba0312020c7829bd32e Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Fri, 16 Aug 2024 16:45:39 +0200 Subject: [PATCH 22/44] add sslserver for testing u2f with devserver --- blenderid/settings.py | 2 +- requirements_dev.txt | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/blenderid/settings.py b/blenderid/settings.py index 1928ae6..b2d6c4d 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -379,7 +379,7 @@ if TESTING: if DEBUG: AGENT_COOKIE_SECURE = False CSRF_COOKIE_SECURE = False - INSTALLED_APPS += ['debug_toolbar'] + INSTALLED_APPS += ['debug_toolbar', 'sslserver'] INTERNAL_IPS = ["127.0.0.1"] MIDDLEWARE += ['debug_toolbar.middleware.DebugToolbarMiddleware'] SESSION_COOKIE_SECURE = False diff --git a/requirements_dev.txt b/requirements_dev.txt index ec9dba2..7a8af27 100644 --- a/requirements_dev.txt +++ b/requirements_dev.txt @@ -2,6 +2,7 @@ Faker==20.1.0 charset-normalizer==3.3.2 django-debug-toolbar==4.4.6 +django-sslserver==0.22 factory-boy==3.3.0 flake8==6.1.0 freezegun==1.3.1 -- 2.30.2 From 2d22c25536431d4d062bbf408e6747fa03480528 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Sat, 17 Aug 2024 21:16:57 +0200 Subject: [PATCH 23/44] contain signature handling in the form --- bid_main/views/mfa.py | 1 - mfa/forms.py | 14 +++++++++----- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 1649bff..9d60f93 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -96,7 +96,6 @@ class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): kwargs = self.get_form_kwargs() key = self.request.POST.get('key', random_hex(20)) kwargs['initial']['key'] = key - kwargs['initial']['signature'] = TotpMfaForm.sign(kwargs['user'], key) return TotpMfaForm(**kwargs) def get_form_kwargs(self): diff --git a/mfa/forms.py b/mfa/forms.py index 2356690..9a2cfbc 100644 --- a/mfa/forms.py +++ b/mfa/forms.py @@ -95,6 +95,9 @@ class TotpMfaForm(forms.Form): def __init__(self, *args, **kwargs): self.user = kwargs.pop('user', None) + kwargs['initial']['signature'] = self.sign( + f"{self.user.email}_{kwargs['initial']['key']}" + ) super().__init__(*args, **kwargs) def clean(self): @@ -102,7 +105,7 @@ class TotpMfaForm(forms.Form): code = self.data.get('code') key = self.data.get('key') signature = self.cleaned_data.get('signature') - if not self.verify_signature(self.user, key, signature): + if not self.verify_signature(f'{self.user.email}_{key}', signature): raise forms.ValidationError(_('Invalid signature')) self.cleaned_data['key'] = key if not TOTP(unhexlify(key)).verify(int(code), tolerance=1): @@ -115,14 +118,15 @@ class TotpMfaForm(forms.Form): EncryptedTOTPDevice.objects.create(encrypted_key=key, key='', name=name, user=self.user) @classmethod - def sign(cls, user, key): + def sign(cls, payload): signer = TimestampSigner() - return signer.sign(f'{user.email}_{key}') + return signer.sign(payload) @classmethod - def verify_signature(cls, user, key, signature, max_age=3600): + def verify_signature(cls, payload, signature, max_age=3600): + signer = TimestampSigner() try: - return f'{user.email}_{key}' == signer.unsign(signature, max_age=max_age) + return payload == signer.unsign(signature, max_age=max_age) except BadSignature: return False -- 2.30.2 From 89851cc87e232450540dc5342affcfe1e3013c7b Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Mon, 19 Aug 2024 17:39:38 +0200 Subject: [PATCH 24/44] register a u2f device --- bid_main/models.py | 4 +- bid_main/templates/bid_main/mfa/setup.html | 16 ++ bid_main/templates/bid_main/mfa/u2f.html | 47 ++++ bid_main/urls.py | 5 + bid_main/views/mfa.py | 50 +++- blenderid/settings.py | 1 + mfa/fido2.py | 38 +++ mfa/forms.py | 58 +++-- mfa/migrations/0002_u2fdevice.py | 81 ++++++ mfa/models.py | 9 + mfa/static/mfa/js/webauthn-json.js | 289 +++++++++++++++++++++ requirements.txt | 1 + 12 files changed, 575 insertions(+), 24 deletions(-) create mode 100644 bid_main/templates/bid_main/mfa/u2f.html create mode 100644 mfa/fido2.py create mode 100644 mfa/migrations/0002_u2fdevice.py create mode 100644 mfa/static/mfa/js/webauthn-json.js diff --git a/bid_main/models.py b/bid_main/models.py index 1ef136e..7186f19 100644 --- a/bid_main/models.py +++ b/bid_main/models.py @@ -24,7 +24,7 @@ import user_agents from . import fields from . import hashers -from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, devices_for_user +from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, U2fDevice, devices_for_user import bid_main.file_utils import bid_main.utils @@ -550,6 +550,8 @@ class User(AbstractBaseUser, PermissionsMixin): devices_per_category['recovery'].append(device) if isinstance(device, EncryptedTOTPDevice): devices_per_category['totp'].append(device) + if isinstance(device, U2fDevice): + devices_per_category['u2f'].append(device) return devices_per_category diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 9229970..efbdbc1 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -57,6 +57,22 @@ Multi-factor Authentication Setup Configure a new TOTP device
    +
    +

    Security keys (U2F, WebAuthn, FIDO2)

    +

    + E.g. yubikey. +

    +
      + {% for d in devices_per_category.u2f %} +
    • + {{ d.name }} + {% if d.last_used_at %}(Last used {{ d.last_used_at|naturaltime }}){% endif %} +
      • + {% endfor %} +
    + Configure a new security key +
    + {% if user_can_setup_recovery %}

    Recovery codes

    diff --git a/bid_main/templates/bid_main/mfa/u2f.html b/bid_main/templates/bid_main/mfa/u2f.html new file mode 100644 index 0000000..ecc2973 --- /dev/null +++ b/bid_main/templates/bid_main/mfa/u2f.html @@ -0,0 +1,47 @@ +{% extends 'layout.html' %} +{% load pipeline static %} +{% load add_form_classes from forms %} +{% block page_title %} +Multi-factor Authentication Setup +{% endblock %} + +{% block body %} +
    +

    New U2F device

    +
    +
    + {% with form=form|add_form_classes %} +
    {% csrf_token %} + {% with field=form.name %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.credential %} + {% include "components/forms/field.html" %} + {% endwith %} + + {% if form.non_field_errors %} +
    + something went wrong +
    + {% endif %} +
    + {% endwith %} +
    +
    +
    + + +{% endblock %} diff --git a/bid_main/urls.py b/bid_main/urls.py index 5f6bda3..156285f 100644 --- a/bid_main/urls.py +++ b/bid_main/urls.py @@ -171,6 +171,11 @@ urlpatterns = [ mfa.TotpView.as_view(), name='mfa_totp', ), + path( + 'mfa/u2f/', + mfa.U2fView.as_view(), + name='mfa_u2f', + ), path( # using `path` converter because persistent_id contains a slash 'mfa/delete-device//', diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 9d60f93..63979ef 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -1,6 +1,7 @@ from base64 import b32encode, b64encode from binascii import unhexlify from io import BytesIO +import json from django.conf import settings from django.db import transaction @@ -12,11 +13,13 @@ from django.views.generic.base import View from django.views.generic.edit import DeleteView, FormView from django_otp.models import Device from django_otp.util import random_hex +from fido2.webauthn import AttestedCredentialData import qrcode from . import mixins -from mfa.forms import DisableMfaForm, TotpMfaForm -from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, devices_for_user +from mfa.fido2 import register_begin +from mfa.forms import DisableMfaForm, TotpMfaForm, U2fMfaForm +from mfa.models import EncryptedRecoveryDevice, EncryptedTOTPDevice, U2fDevice, devices_for_user import bid_main.tasks @@ -89,17 +92,14 @@ class InvalidateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): + form_class = TotpMfaForm success_url = reverse_lazy('bid_main:mfa') template_name = "bid_main/mfa/totp.html" - def get_form(self, *args, **kwargs): - kwargs = self.get_form_kwargs() - key = self.request.POST.get('key', random_hex(20)) - kwargs['initial']['key'] = key - return TotpMfaForm(**kwargs) - def get_form_kwargs(self): kwargs = super().get_form_kwargs() + key = self.request.POST.get('key', random_hex(20)) + kwargs['initial']['key'] = key kwargs['user'] = self.request.user return kwargs @@ -129,6 +129,40 @@ class TotpView(mixins.MfaRequiredIfConfiguredMixin, FormView): return super().form_valid(form) +class U2fView(mixins.MfaRequiredIfConfiguredMixin, FormView): + form_class = U2fMfaForm + success_url = reverse_lazy('bid_main:mfa') + template_name = "bid_main/mfa/u2f.html" + + def get_form_kwargs(self): + kwargs = super().get_form_kwargs() + rp_id = self.request.get_host().split(':')[0] # remove port, required by webauthn + kwargs['rp_id'] = rp_id + kwargs['user'] = self.request.user + + if self.request.method == 'GET': + credentials = [ + AttestedCredentialData(d.credential) + for d in U2fDevice.objects.filter(user=self.request.user).all() + ] + credential_creation_options, state = register_begin( + rp_id, self.request.user, credentials, + ) + self.request.session['u2f_create_state'] = dict(state) + kwargs['credential_creation_options'] = json.dumps(dict(credential_creation_options)) + if self.request.method == 'POST': + kwargs['state'] = self.request.session.pop('u2f_create_state', None) + + return kwargs + + @transaction.atomic + def form_valid(self, form): + form.save() + if self.request.user.confirmed_email_at: + bid_main.tasks.send_new_mfa_device_email(self.request.user.pk, 'u2f') + return super().form_valid(form) + + class DeleteDeviceView(mixins.MfaRequiredMixin, DeleteView): model = Device template_name = "bid_main/mfa/delete_device.html" diff --git a/blenderid/settings.py b/blenderid/settings.py index b2d6c4d..c20688c 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -108,6 +108,7 @@ AUTHENTICATION_BACKENDS = [ "django.contrib.auth.backends.ModelBackend", ] +FIDO2_RP_NAME = "Blender ID" OTP_TOTP_ISSUER = "id.blender.org" ROOT_URLCONF = "blenderid.urls" diff --git a/mfa/fido2.py b/mfa/fido2.py new file mode 100644 index 0000000..c111b7e --- /dev/null +++ b/mfa/fido2.py @@ -0,0 +1,38 @@ +from django.conf import settings +from fido2.server import Fido2Server +from fido2.webauthn import ( + PublicKeyCredentialRpEntity, + PublicKeyCredentialUserEntity, + ResidentKeyRequirement, + UserVerificationRequirement, +) +import fido2.features + +# needed to automatically convert between bytes and base64url +fido2.features.webauthn_json_mapping.enabled = True + + +def get_fido2server(rp_id): + return Fido2Server( + PublicKeyCredentialRpEntity( + id=rp_id, + name=settings.FIDO2_RP_NAME, + ) + ) + + +def register_begin(rp_id, user, credentials=[]): + return get_fido2server(rp_id).register_begin( + PublicKeyCredentialUserEntity( + display_name=user.email, + id=user.pk.to_bytes(8, 'big'), + name=user.email, + ), + credentials, + resident_key_requirement=ResidentKeyRequirement.DISCOURAGED, + user_verification=UserVerificationRequirement.DISCOURAGED, + ) + + +def register_complete(rp_id, state, credential): + return get_fido2server(rp_id).register_complete(state, credential) diff --git a/mfa/forms.py b/mfa/forms.py index 9a2cfbc..962b636 100644 --- a/mfa/forms.py +++ b/mfa/forms.py @@ -8,7 +8,21 @@ from django.utils.translation import gettext_lazy as _ from django_otp.oath import TOTP from otp_agents.views import OTPTokenForm -from mfa.models import EncryptedTOTPDevice +from mfa.fido2 import register_complete +from mfa.models import EncryptedTOTPDevice, U2fDevice + + +def _sign(payload): + signer = TimestampSigner() + return signer.sign(payload) + + +def _verify_signature(payload, signature, max_age=3600): + signer = TimestampSigner() + try: + return payload == signer.unsign(signature, max_age=max_age) + except BadSignature: + return False class MfaForm(OTPTokenForm): @@ -95,9 +109,7 @@ class TotpMfaForm(forms.Form): def __init__(self, *args, **kwargs): self.user = kwargs.pop('user', None) - kwargs['initial']['signature'] = self.sign( - f"{self.user.email}_{kwargs['initial']['key']}" - ) + kwargs['initial']['signature'] = _sign(f"{self.user.email}_{kwargs['initial']['key']}") super().__init__(*args, **kwargs) def clean(self): @@ -105,7 +117,7 @@ class TotpMfaForm(forms.Form): code = self.data.get('code') key = self.data.get('key') signature = self.cleaned_data.get('signature') - if not self.verify_signature(f'{self.user.email}_{key}', signature): + if not _verify_signature(f'{self.user.email}_{key}', signature): raise forms.ValidationError(_('Invalid signature')) self.cleaned_data['key'] = key if not TOTP(unhexlify(key)).verify(int(code), tolerance=1): @@ -117,16 +129,32 @@ class TotpMfaForm(forms.Form): name = self.cleaned_data.get('name') EncryptedTOTPDevice.objects.create(encrypted_key=key, key='', name=name, user=self.user) - @classmethod - def sign(cls, payload): - signer = TimestampSigner() - return signer.sign(payload) - @classmethod - def verify_signature(cls, payload, signature, max_age=3600): +class U2fMfaForm(forms.Form): + credential = forms.JSONField(widget=forms.HiddenInput) + name = forms.CharField( + max_length=U2fDevice._meta.get_field('name').max_length, + widget=forms.TextInput( + attrs={"placeholder": "device name (for your convenience)"}, + ), + ) - signer = TimestampSigner() + def __init__(self, *args, **kwargs): + credential_creation_options = kwargs.pop('credential_creation_options', None) + self.rp_id = kwargs.pop('rp_id', None) + self.state = kwargs.pop('state', None) + self.user = kwargs.pop('user', None) + super().__init__(*args, **kwargs) + self.fields['credential'].widget.attrs['creation_options'] = credential_creation_options + + def save(self): + credential = self.cleaned_data.get('credential') + name = self.cleaned_data.get('name') + auth_data = None try: - return payload == signer.unsign(signature, max_age=max_age) - except BadSignature: - return False + auth_data = register_complete(self.rp_id, self.state, credential) + except Exception: + raise forms.ValidationError(_('Verification failed')) + U2fDevice.objects.create( + user=self.user, credential=auth_data.credential_data, name=name, + ) diff --git a/mfa/migrations/0002_u2fdevice.py b/mfa/migrations/0002_u2fdevice.py new file mode 100644 index 0000000..4dabc97 --- /dev/null +++ b/mfa/migrations/0002_u2fdevice.py @@ -0,0 +1,81 @@ +# Generated by Django 4.2.13 on 2024-08-19 14:38 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('mfa', '0001_initial'), + ] + + operations = [ + migrations.CreateModel( + name='U2fDevice', + fields=[ + ( + 'id', + models.BigAutoField( + auto_created=True, primary_key=True, serialize=False, verbose_name='ID' + ), + ), + ( + 'name', + models.CharField( + help_text='The human-readable name of this device.', max_length=64 + ), + ), + ( + 'confirmed', + models.BooleanField(default=True, help_text='Is this device ready for use?'), + ), + ( + 'throttling_failure_timestamp', + models.DateTimeField( + blank=True, + default=None, + help_text='A timestamp of the last failed verification attempt. Null if last attempt succeeded.', + null=True, + ), + ), + ( + 'throttling_failure_count', + models.PositiveIntegerField( + default=0, help_text='Number of successive failed attempts.' + ), + ), + ( + 'created_at', + models.DateTimeField( + auto_now_add=True, + help_text='The date and time when this device was initially created in the system.', + null=True, + ), + ), + ( + 'last_used_at', + models.DateTimeField( + blank=True, + help_text='The most recent date and time this device was used.', + null=True, + ), + ), + ('credential', models.BinaryField()), + ( + 'user', + models.ForeignKey( + help_text='The user that this device belongs to.', + on_delete=django.db.models.deletion.CASCADE, + to=settings.AUTH_USER_MODEL, + ), + ), + ], + options={ + 'verbose_name': 'U2F device', + 'abstract': False, + }, + ), + ] diff --git a/mfa/models.py b/mfa/models.py index fc327d9..233d865 100644 --- a/mfa/models.py +++ b/mfa/models.py @@ -11,6 +11,7 @@ the upstream implementation as much as possible. from binascii import unhexlify from django.db import models, transaction +from django_otp.models import Device, ThrottlingMixin, TimestampMixin from django_otp.plugins.otp_static.models import StaticDevice from django_otp.plugins.otp_totp.models import TOTPDevice from nacl_encrypted_fields.fields import NaClCharField @@ -75,6 +76,13 @@ class EncryptedTOTPDevice(TOTPDevice): return unhexlify(self.encrypted_key.encode()) +class U2fDevice(TimestampMixin, ThrottlingMixin, Device): + credential = models.BinaryField() + + class Meta(Device.Meta): + verbose_name = "U2F device" + + def devices_for_user(user): """ A more specific replacement for upstream devices_for_user. @@ -84,4 +92,5 @@ def devices_for_user(user): return [ *EncryptedRecoveryDevice.objects.filter(user=user).all(), *EncryptedTOTPDevice.objects.filter(user=user).all(), + *U2fDevice.objects.filter(user=user).all(), ] diff --git a/mfa/static/mfa/js/webauthn-json.js b/mfa/static/mfa/js/webauthn-json.js new file mode 100644 index 0000000..ad7de9b --- /dev/null +++ b/mfa/static/mfa/js/webauthn-json.js @@ -0,0 +1,289 @@ +// https://github.com/github/webauthn-json +// via https://github.com/pennersr/django-allauth/blob/main/allauth/mfa/static/mfa/js/webauthn-json.js +// +// The MIT License (MIT) +// +// Copyright (c) 2019 GitHub, Inc. +// Copyright (c) 2010-2021 Raymond Penners and contributors +// +// Permission is hereby granted, free of charge, to any person +// obtaining a copy of this software and associated documentation +// files (the "Software"), to deal in the Software without +// restriction, including without limitation the rights to use, +// copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the +// Software is furnished to do so, subject to the following +// conditions: +// +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES +// OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT +// HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +// WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +// FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +// OTHER DEALINGS IN THE SOFTWARE. + +'use strict'; +(() => { + const __defProp = Object.defineProperty + const __export = (target, all) => { + for (const name in all) { __defProp(target, name, { get: all[name], enumerable: true }) } + } + const __async = (__this, __arguments, generator) => { + return new Promise((resolve, reject) => { + const fulfilled = (value) => { + try { + step(generator.next(value)) + } catch (e) { + reject(e) + } + } + const rejected = (value) => { + try { + step(generator.throw(value)) + } catch (e) { + reject(e) + } + } + var step = (x) => x.done ? resolve(x.value) : Promise.resolve(x.value).then(fulfilled, rejected) + step((generator = generator.apply(__this, __arguments)).next()) + }) + } + + // src/webauthn-json/index.ts + const webauthn_json_exports = {} + __export(webauthn_json_exports, { + create: () => create, + get: () => get, + schema: () => schema, + supported: () => supported + }) + + // src/webauthn-json/base64url.ts + function base64urlToBuffer (baseurl64String) { + const padding = '=='.slice(0, (4 - baseurl64String.length % 4) % 4) + const base64String = baseurl64String.replace(/-/g, '+').replace(/_/g, '/') + padding + const str = atob(base64String) + const buffer = new ArrayBuffer(str.length) + const byteView = new Uint8Array(buffer) + for (let i = 0; i < str.length; i++) { + byteView[i] = str.charCodeAt(i) + } + return buffer + } + function bufferToBase64url (buffer) { + const byteView = new Uint8Array(buffer) + let str = '' + for (const charCode of byteView) { + str += String.fromCharCode(charCode) + } + const base64String = btoa(str) + const base64urlString = base64String.replace(/\+/g, '-').replace( + /\//g, + '_' + ).replace(/=/g, '') + return base64urlString + } + + // src/webauthn-json/convert.ts + const copyValue = 'copy' + const convertValue = 'convert' + function convert (conversionFn, schema2, input) { + if (schema2 === copyValue) { + return input + } + if (schema2 === convertValue) { + return conversionFn(input) + } + if (schema2 instanceof Array) { + return input.map((v) => convert(conversionFn, schema2[0], v)) + } + if (schema2 instanceof Object) { + const output = {} + for (const [key, schemaField] of Object.entries(schema2)) { + if (schemaField.derive) { + const v = schemaField.derive(input) + if (v !== void 0) { + input[key] = v + } + } + if (!(key in input)) { + if (schemaField.required) { + throw new Error(`Missing key: ${key}`) + } + continue + } + if (input[key] == null) { + output[key] = null + continue + } + output[key] = convert( + conversionFn, + schemaField.schema, + input[key] + ) + } + return output + } + } + function derived (schema2, derive) { + return { + required: true, + schema: schema2, + derive + } + } + function required (schema2) { + return { + required: true, + schema: schema2 + } + } + function optional (schema2) { + return { + required: false, + schema: schema2 + } + } + + // src/webauthn-json/basic/schema.ts + const publicKeyCredentialDescriptorSchema = { + type: required(copyValue), + id: required(convertValue), + transports: optional(copyValue) + } + const simplifiedExtensionsSchema = { + appid: optional(copyValue), + appidExclude: optional(copyValue), + credProps: optional(copyValue) + } + const simplifiedClientExtensionResultsSchema = { + appid: optional(copyValue), + appidExclude: optional(copyValue), + credProps: optional(copyValue) + } + const credentialCreationOptions = { + publicKey: required({ + rp: required(copyValue), + user: required({ + id: required(convertValue), + name: required(copyValue), + displayName: required(copyValue) + }), + challenge: required(convertValue), + pubKeyCredParams: required(copyValue), + timeout: optional(copyValue), + excludeCredentials: optional([publicKeyCredentialDescriptorSchema]), + authenticatorSelection: optional(copyValue), + attestation: optional(copyValue), + extensions: optional(simplifiedExtensionsSchema) + }), + signal: optional(copyValue) + } + const publicKeyCredentialWithAttestation = { + type: required(copyValue), + id: required(copyValue), + rawId: required(convertValue), + authenticatorAttachment: optional(copyValue), + response: required({ + clientDataJSON: required(convertValue), + attestationObject: required(convertValue), + transports: derived( + copyValue, + (response) => { + let _a + return ((_a = response.getTransports) == null ? void 0 : _a.call(response)) || [] + } + ) + }), + clientExtensionResults: derived( + simplifiedClientExtensionResultsSchema, + (pkc) => pkc.getClientExtensionResults() + ) + } + const credentialRequestOptions = { + mediation: optional(copyValue), + publicKey: required({ + challenge: required(convertValue), + timeout: optional(copyValue), + rpId: optional(copyValue), + allowCredentials: optional([publicKeyCredentialDescriptorSchema]), + userVerification: optional(copyValue), + extensions: optional(simplifiedExtensionsSchema) + }), + signal: optional(copyValue) + } + const publicKeyCredentialWithAssertion = { + type: required(copyValue), + id: required(copyValue), + rawId: required(convertValue), + authenticatorAttachment: optional(copyValue), + response: required({ + clientDataJSON: required(convertValue), + authenticatorData: required(convertValue), + signature: required(convertValue), + userHandle: required(convertValue) + }), + clientExtensionResults: derived( + simplifiedClientExtensionResultsSchema, + (pkc) => pkc.getClientExtensionResults() + ) + } + var schema = { + credentialCreationOptions, + publicKeyCredentialWithAttestation, + credentialRequestOptions, + publicKeyCredentialWithAssertion + } + + // src/webauthn-json/basic/api.ts + function createRequestFromJSON (requestJSON) { + return convert(base64urlToBuffer, credentialCreationOptions, requestJSON) + } + function createResponseToJSON (credential) { + return convert( + bufferToBase64url, + publicKeyCredentialWithAttestation, + credential + ) + } + function create (requestJSON) { + return __async(this, null, function * () { + const credential = yield navigator.credentials.create( + createRequestFromJSON(requestJSON) + ) + return createResponseToJSON(credential) + }) + } + function getRequestFromJSON (requestJSON) { + return convert(base64urlToBuffer, credentialRequestOptions, requestJSON) + } + function getResponseToJSON (credential) { + return convert( + bufferToBase64url, + publicKeyCredentialWithAssertion, + credential + ) + } + function get (requestJSON) { + return __async(this, null, function * () { + const credential = yield navigator.credentials.get( + getRequestFromJSON(requestJSON) + ) + return getResponseToJSON(credential) + }) + } + + // src/webauthn-json/basic/supported.ts + function supported () { + return !!(navigator.credentials && navigator.credentials.create && navigator.credentials.get && window.PublicKeyCredential) + } + + // src/webauthn-json/browser-global.ts + globalThis.webauthnJSON = webauthn_json_exports +})() +// # sourceMappingURL=webauthn-json.browser-global.js.map diff --git a/requirements.txt b/requirements.txt index aa3ae03..a40315b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,6 +22,7 @@ django-otp-agents==1.0.1 django-pipeline==3.1.0 ; python_version >= "3.8" and python_version < "4" dj-database-url==2.2.0 docutils==0.14 ; python_version >= "3.8" and python_version < "4" +fido2==1.1.3 htmlmin==0.1.12 ; python_version >= "3.8" and python_version < "4" idna==2.8 ; python_version >= "3.8" and python_version < "4" importlib-metadata==3.6.0 ; python_version >= "3.8" and python_version < "4" -- 2.30.2 From a91afd25974e691bd33fdf5f91e1b7e2b61cb392 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 20 Aug 2024 12:34:12 +0200 Subject: [PATCH 25/44] wip webauthn authenticate --- .../bid_main/components/u2f_form.html | 43 ++++++++++++ bid_main/templates/bid_main/login.html | 2 + bid_main/templates/bid_main/mfa/u2f.html | 8 +-- bid_main/views/mfa.py | 4 +- bid_main/views/normal_pages.py | 36 ++++++++-- mfa/fido2.py | 13 ++++ mfa/forms.py | 68 +++++++++++++++++-- 7 files changed, 160 insertions(+), 14 deletions(-) create mode 100644 bid_main/templates/bid_main/components/u2f_form.html diff --git a/bid_main/templates/bid_main/components/u2f_form.html b/bid_main/templates/bid_main/components/u2f_form.html new file mode 100644 index 0000000..c313a00 --- /dev/null +++ b/bid_main/templates/bid_main/components/u2f_form.html @@ -0,0 +1,43 @@ +{% load add_form_classes from forms %} +{% load common static %} + +
    +
    +

    Multi-factor Authentication

    +
    + {% with form=form|add_form_classes %} +
    {% csrf_token %} +
    +

    Use a security key.

    + {% with field=form.otp_trust_agent %} + {% include "components/forms/field.html" with with_help_text=True %} + {% endwith %} + {% with field=form.response %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.signature %} + {% include "components/forms/field.html" %} + {% endwith %} + {% with field=form.state %} + {% include "components/forms/field.html" %} + {% endwith %} +
    + {{ form.non_field_errors }} +
    + {% endwith %} + {% if devices.totp %} + Use an authenticator + Lost your authenticator? Use a recovery code + {% endif %} +
    + + diff --git a/bid_main/templates/bid_main/login.html b/bid_main/templates/bid_main/login.html index 49c3dd8..268e703 100644 --- a/bid_main/templates/bid_main/login.html +++ b/bid_main/templates/bid_main/login.html @@ -7,5 +7,7 @@ {% include 'bid_main/components/login_form.html' %} {% elif is_mfa_form %} {% include 'bid_main/components/mfa_form.html' %} +{% elif is_u2f_form %} + {% include 'bid_main/components/u2f_form.html' %} {% endif %} {% endblock form %} diff --git a/bid_main/templates/bid_main/mfa/u2f.html b/bid_main/templates/bid_main/mfa/u2f.html index ecc2973..726cd9c 100644 --- a/bid_main/templates/bid_main/mfa/u2f.html +++ b/bid_main/templates/bid_main/mfa/u2f.html @@ -11,14 +11,14 @@ Multi-factor Authentication Setup
    {% with form=form|add_form_classes %} -
    {% csrf_token %} + {% csrf_token %} {% with field=form.name %} {% include "components/forms/field.html" %} {% endwith %} {% with field=form.credential %} {% include "components/forms/field.html" %} {% endwith %} - + {% if form.non_field_errors %}
    something went wrong @@ -31,14 +31,14 @@ Multi-factor Authentication Setup
    diff --git a/bid_main/templates/bid_main/login.html b/bid_main/templates/bid_main/login.html index 268e703..13b6bb6 100644 --- a/bid_main/templates/bid_main/login.html +++ b/bid_main/templates/bid_main/login.html @@ -3,11 +3,13 @@ {% block page_title %}Sign in{% endblock %} {% block form %} -{% if is_authentication_form %} +{% if form_type == 'login' %} {% include 'bid_main/components/login_form.html' %} -{% elif is_mfa_form %} +{% elif form_type == 'mfa' %} {% include 'bid_main/components/mfa_form.html' %} -{% elif is_u2f_form %} +{% elif form_type == 'u2f' %} {% include 'bid_main/components/u2f_form.html' %} +{% else %} +
    Something went wrong
    {% endif %} {% endblock form %} diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index 097a210..59f73f4 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -10,7 +10,7 @@ import urllib.parse from django.conf import settings from django.contrib.auth import views as auth_views, logout, get_user_model -from django.core.exceptions import ValidationError +from django.core.exceptions import ImproperlyConfigured, ValidationError from django.utils.translation import gettext as _ from django.db import transaction, IntegrityError from django.db.models import Count @@ -96,15 +96,19 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agen ctx = super().get_context_data(**kwargs) self.find_oauth_flow(ctx) form = self.get_form() + form_type = None if isinstance(form, U2fAuthenticateForm): ctx["devices"] = self.request.user.mfa_devices_per_category() - ctx["is_u2f_form"] = True + form_type = 'u2f' elif isinstance(form, MfaAuthenticateForm): ctx["devices"] = self.request.user.mfa_devices_per_category() - ctx["is_mfa_form"] = True ctx["use_recovery"] = self._use_recovery() + form_type = 'mfa' + elif isinstance(form, forms.AuthenticationForm): + form_type = 'login' else: - ctx["is_authentication_form"] = isinstance(form, forms.AuthenticationForm) + raise ImproperlyConfigured('unexpected form object') + ctx["form_type"] = form_type return ctx def get_form(self): @@ -120,7 +124,7 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agen kwargs['rp_id'] = rp_id kwargs['state'] = dict(state) return U2fAuthenticateForm(**kwargs) - # this will switch between MfaForm and AuthenticationForm + # this will switch between MfaAuthenticateForm and AuthenticationForm return super().get_form() def get_form_kwargs(self): -- 2.30.2 From d648135f4a53c74a2e88c83cd0022a0e54e377f6 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 22 Aug 2024 12:43:01 +0200 Subject: [PATCH 33/44] add otp_requried decorator for oauth/authorize/ --- blenderid/oauth2_urls.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/blenderid/oauth2_urls.py b/blenderid/oauth2_urls.py index 995bc27..b2e8894 100644 --- a/blenderid/oauth2_urls.py +++ b/blenderid/oauth2_urls.py @@ -8,13 +8,20 @@ I've also removed the "management" URLs, as we use the admin interface for that. from django.urls import re_path from django.views.generic import RedirectView - from oauth2_provider import views as default_oauth2_views from oauth2_provider import urls as default_oauth2_urls +from otp_agents.decorators import otp_required + app_name = "oauth2_provider" urlpatterns = ( - re_path(r"^authorize/?$", default_oauth2_views.AuthorizationView.as_view(), name="authorize"), + re_path( + r"^authorize/?$", + otp_required(accept_trusted_agent=True, if_configured=True)( + default_oauth2_views.AuthorizationView.as_view() + ), + name="authorize", + ), re_path(r"^token/?$", default_oauth2_views.TokenView.as_view(), name="token"), re_path( r"^revoke/?$", -- 2.30.2 From 51898e6838fd1eb80e19e304e6fe9ad68a0a5707 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 22 Aug 2024 18:37:12 +0200 Subject: [PATCH 34/44] cleaning up and adding comments --- bid_main/models.py | 12 +-- .../bid_main/components/mfa_form.html | 4 +- .../bid_main/components/u2f_form.html | 8 +- bid_main/templates/bid_main/mfa/setup.html | 6 +- bid_main/tests/test_mfa.py | 8 +- bid_main/views/mfa.py | 18 ++-- bid_main/views/normal_pages.py | 102 +++++++++++++----- mfa/forms.py | 7 +- 8 files changed, 110 insertions(+), 55 deletions(-) diff --git a/bid_main/models.py b/bid_main/models.py index 7186f19..85f1dcc 100644 --- a/bid_main/models.py +++ b/bid_main/models.py @@ -543,16 +543,16 @@ class User(AbstractBaseUser, PermissionsMixin): return bid_main.file_utils.get_absolute_url(static(settings.AVATAR_DEFAULT_FILENAME)) return bid_main.file_utils.get_absolute_url(self.avatar.storage.url(default_thumbnail_path)) - def mfa_devices_per_category(self): - devices_per_category = defaultdict(list) + def mfa_devices_per_type(self): + devices_per_type = defaultdict(list) for device in devices_for_user(self): if isinstance(device, EncryptedRecoveryDevice): - devices_per_category['recovery'].append(device) + devices_per_type['recovery'].append(device) if isinstance(device, EncryptedTOTPDevice): - devices_per_category['totp'].append(device) + devices_per_type['totp'].append(device) if isinstance(device, U2fDevice): - devices_per_category['u2f'].append(device) - return devices_per_category + devices_per_type['u2f'].append(device) + return devices_per_type class SettingValueField(models.CharField): diff --git a/bid_main/templates/bid_main/components/mfa_form.html b/bid_main/templates/bid_main/components/mfa_form.html index 11d2987..70cefbf 100644 --- a/bid_main/templates/bid_main/components/mfa_form.html +++ b/bid_main/templates/bid_main/components/mfa_form.html @@ -39,9 +39,9 @@ {% if devices.recovery %}
    {% if use_recovery %} - Use an authenticator + Use an authenticator {% else %} - Lost your authenticator? Use a recovery code + Use a recovery code {% endif %}
    {% endif %} diff --git a/bid_main/templates/bid_main/components/u2f_form.html b/bid_main/templates/bid_main/components/u2f_form.html index 9abfaf2..10ea7cc 100644 --- a/bid_main/templates/bid_main/components/u2f_form.html +++ b/bid_main/templates/bid_main/components/u2f_form.html @@ -8,7 +8,7 @@ {% with form=form|add_form_classes %} {% csrf_token %}
    -

    Please use a security key you have configured. Tick the checkbox below before using the key

    +

    Please use a security key you have configured. Tick the checkbox below before using the key if you want to remember this device.

    {% with field=form.otp_trust_agent %} {% include "components/forms/field.html" with with_help_text=True %} {% endwith %} @@ -27,8 +27,10 @@ {% endwith %} {% if devices.totp %} - Use an authenticator - Lost your authenticator? Use a recovery code +
    + Use an authenticator + Use a recovery code +
    {% endif %}
    diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 07bfeb3..61591f6 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -47,7 +47,7 @@ Multi-factor Authentication Setup If you don't have an authenticator application, you can choose one from a list of TOTP applications.

      - {% for d in devices_per_category.totp %} + {% for d in devices_per_type.totp %}
    • {{ d.name }} {% if d.last_used_at %}(Last used {{ d.last_used_at|naturaltime }}){% endif %} @@ -63,7 +63,7 @@ Multi-factor Authentication Setup E.g. yubikey.

        - {% for d in devices_per_category.u2f %} + {% for d in devices_per_type.u2f %}
      • {{ d.name }} {% if d.last_used_at %}(Last used {{ d.last_used_at|naturaltime }}){% endif %} @@ -81,7 +81,7 @@ Multi-factor Authentication Setup Each code can be used only once. You can generate a new set of recovery codes at any time, any remaining old codes will become invalidated.

        - {% with recovery=devices_per_category.recovery.0 %} + {% with recovery=devices_per_type.recovery.0 %} {% if recovery %}
        {% with code_count=recovery_codes|length %} diff --git a/bid_main/tests/test_mfa.py b/bid_main/tests/test_mfa.py index 16d1d10..8bee3aa 100644 --- a/bid_main/tests/test_mfa.py +++ b/bid_main/tests/test_mfa.py @@ -122,13 +122,13 @@ class TestMfa(TestCase): {'username': self.user.email, 'password': 'hunter2'}, follow=True, ) - response = client.get('/login?use_recovery=1') + response = client.get('/login?mfa_device_type=recovery') match = re.search( r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) ) otp_device = match.group(1) response = client.post( - '/login?use_recovery=1', + '/login?mfa_device_type=recovery', {'otp_device': otp_device, 'otp_token': recovery_code}, ) self.assertEqual(response.status_code, 302) @@ -142,13 +142,13 @@ class TestMfa(TestCase): {'username': self.user.email, 'password': 'hunter2'}, follow=True, ) - response = client.get('/login?use_recovery=1') + response = client.get('/login?mfa_device_type=recovery') match = re.search( r'input type="hidden" name="otp_device" value="([^"]+)"', str(response.content) ) otp_device = match.group(1) response = client.post( - '/login?use_recovery=1', + '/login?mfa_device_type=recovery', {'otp_device': otp_device, 'otp_token': recovery_code}, ) self.assertEqual(response.status_code, 200) diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 35192f7..16e7815 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -24,8 +24,10 @@ import bid_main.tasks class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): - """Don't allow to setup recovery codes unless the user has already configured some other method. + """Mfa setup. + Important in current implementation: + Don't allow to setup recovery codes unless the user has already configured some other method. Otherwise MfaRequiredIfConfiguredMixin locks the user out immediately, not giving a chance to copy the recovery codes. """ @@ -36,24 +38,24 @@ class MfaView(mixins.MfaRequiredIfConfiguredMixin, TemplateView): recovery_codes = [] show_missing_recovery_codes_warning = False user_can_setup_recovery = False - devices_per_category = user.mfa_devices_per_category() - if 'recovery' in devices_per_category: - recovery_device = devices_per_category['recovery'][0] + devices_per_type = user.mfa_devices_per_type() + if 'recovery' in devices_per_type: + recovery_device = devices_per_type['recovery'][0] recovery_codes = [t.encrypted_token for t in recovery_device.encryptedtoken_set.all()] - if devices_per_category.keys() - {'recovery'}: + if devices_per_type.keys() - {'recovery'}: user_can_setup_recovery = True - if user_can_setup_recovery and 'recovery' not in devices_per_category: + if user_can_setup_recovery and 'recovery' not in devices_per_type: show_missing_recovery_codes_warning = True return { 'agent_inactivity_days': settings.AGENT_INACTIVITY_DAYS, 'agent_trust_days': settings.AGENT_TRUST_DAYS, - 'devices_per_category': devices_per_category, + 'devices_per_type': devices_per_type, 'display_recovery_codes': self.request.GET.get('display_recovery_codes'), 'recovery_codes': recovery_codes, 'show_missing_recovery_codes_warning': show_missing_recovery_codes_warning, 'user_can_setup_recovery': user_can_setup_recovery, - 'user_has_mfa_configured': bool(devices_per_category), + 'user_has_mfa_configured': bool(devices_per_type), } diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index 59f73f4..18a3d16 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -10,7 +10,7 @@ import urllib.parse from django.conf import settings from django.contrib.auth import views as auth_views, logout, get_user_model -from django.core.exceptions import ImproperlyConfigured, ValidationError +from django.core.exceptions import ValidationError from django.utils.translation import gettext as _ from django.db import transaction, IntegrityError from django.db.models import Count @@ -20,13 +20,16 @@ from django.urls import reverse_lazy from django.utils import timezone from django.utils.decorators import method_decorator from django.views.decorators.cache import never_cache +from django.views.decorators.csrf import csrf_protect +from django.views.decorators.debug import sensitive_post_parameters from django.views.generic import TemplateView, FormView from django.views.generic.base import View from django.views.generic.edit import UpdateView +from django_otp import user_has_device from fido2.webauthn import AttestedCredentialData +import django_otp.views import loginas.utils import oauth2_provider.models as oauth2_models -import otp_agents.views from .. import forms, email from . import mixins @@ -74,62 +77,105 @@ class IndexView(mixins.MfaRequiredIfConfiguredMixin, mixins.PageIdMixin, Templat } -class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agents.views.LoginView): - """Shows the login view.""" +class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, django_otp.views.LoginView): + """Shows the login view. + + This view also handles MFA forms and OAuth redirects. + + django_otp introduces additional indirection, it works as follows: + - django.contrib.auth.views.LoginView.get_form_class allows to define the form dynamically + and django_otp makes use of that to switch between otp_authentication_form and otp_token_form + - we overwrite otp_authentication_form to exclude the otp_device and otp_token fields, because + we don't have mandatory MFA for everyone + On top of that we have a separate helper for injecting U2fAuthenticateForm if a user has a u2f + device. + + Because get_form is called on both GET and POST requests we put the branching logic there, + following the example of django_otp. + """ otp_authentication_form = forms.AuthenticationForm otp_token_form = MfaAuthenticateForm page_id = "login" - redirect_authenticated_user = False + redirect_authenticated_user = True success_url_allowed_hosts = settings.NEXT_REDIR_AFTER_LOGIN_ALLOWED_HOSTS template_name = "bid_main/login.html" authorize_url = reverse_lazy("oauth2_provider:authorize") + def _mfa_device_type(self): + return self.request.GET.get("mfa_device_type", None) + def _use_recovery(self): - return self.request.GET.get("use_recovery", False) + return self._mfa_device_type() == "recovery" def _use_totp(self): - return self.request.GET.get("use_totp", False) + return self._mfa_device_type() == "totp" + + def _use_u2f(self): + return self._mfa_device_type() == "u2f" + + @method_decorator(sensitive_post_parameters()) + @method_decorator(csrf_protect) + @method_decorator(never_cache) + def dispatch(self, request, *args, **kwargs): + """This is a tweaked implementation of django.contrib.auth.view.LoginView.dispatch method. + + It accounts for the second step MfaAuthenticateForm and doesn't try to redirect too soon. + """ + user_is_verified = self.request.agent.is_trusted or not user_has_device(self.request.user) + ready_to_redirect = self.request.user.is_authenticated and user_is_verified + if self.redirect_authenticated_user and ready_to_redirect: + redirect_to = self.get_success_url() + if redirect_to == self.request.path: + raise ValueError( + "Redirection loop for authenticated user detected. Check that " + "your LOGIN_REDIRECT_URL doesn't point to a login page." + ) + return HttpResponseRedirect(redirect_to) + # not using a simple super() because we need to jump higher in view inheritance hierarchy + # and avoid execution of django.contrib.auth.view.LoginView.dispatch + return super(FormView, self).dispatch(request, *args, **kwargs) def get_context_data(self, **kwargs) -> dict: ctx = super().get_context_data(**kwargs) self.find_oauth_flow(ctx) + ctx["form_type"] = "login" + form = self.get_form() - form_type = None if isinstance(form, U2fAuthenticateForm): - ctx["devices"] = self.request.user.mfa_devices_per_category() - form_type = 'u2f' + ctx["devices"] = self.request.user.mfa_devices_per_type() + ctx["form_type"] = "u2f" elif isinstance(form, MfaAuthenticateForm): - ctx["devices"] = self.request.user.mfa_devices_per_category() + ctx["devices"] = self.request.user.mfa_devices_per_type() ctx["use_recovery"] = self._use_recovery() - form_type = 'mfa' - elif isinstance(form, forms.AuthenticationForm): - form_type = 'login' - else: - raise ImproperlyConfigured('unexpected form object') - ctx["form_type"] = form_type + ctx["form_type"] = "mfa" + return ctx + def _get_u2f_form(self, devices): + credentials = [AttestedCredentialData(d.credential) for d in devices] + rp_id = self.request.get_host().split(':')[0] # remove port, required by webauthn + request_options, state = authenticate_begin(rp_id, credentials) + kwargs = self.get_form_kwargs() + kwargs['credentials'] = credentials + kwargs['request_options'] = json.dumps(dict(request_options)) + kwargs['rp_id'] = rp_id + kwargs['state'] = dict(state) + return U2fAuthenticateForm(**kwargs) + def get_form(self): if self.request.user.is_authenticated: - devices = self.request.user.mfa_devices_per_category().get('u2f', None) + devices = self.request.user.mfa_devices_per_type().get('u2f', None) if devices and not self._use_recovery() and not self._use_totp(): - credentials = [AttestedCredentialData(d.credential) for d in devices] - rp_id = self.request.get_host().split(':')[0] # remove port, required by webauthn - request_options, state = authenticate_begin(rp_id, credentials) - kwargs = self.get_form_kwargs() - kwargs['credentials'] = credentials - kwargs['request_options'] = json.dumps(dict(request_options)) - kwargs['rp_id'] = rp_id - kwargs['state'] = dict(state) - return U2fAuthenticateForm(**kwargs) + return self._get_u2f_form(devices) # this will switch between MfaAuthenticateForm and AuthenticationForm + # as defined in django_otp.views.LoginView.authentication_form implementation return super().get_form() def get_form_kwargs(self): kwargs = super().get_form_kwargs() - if self.request.GET.get("use_recovery", False): + if self._use_recovery(): kwargs["use_recovery"] = True return kwargs diff --git a/mfa/forms.py b/mfa/forms.py index cbcdf68..6a63d3c 100644 --- a/mfa/forms.py +++ b/mfa/forms.py @@ -28,7 +28,12 @@ def _verify_signature(payload, signature, max_age=3600): class MfaAuthenticateForm(OTPTokenForm): - """Restyle the form widgets to do less work in the template.""" + """Restyle the form widgets to do less work in the template. + + This form inherits from otp_agents form that takes care of agent_trust. + Even though the LoginView itself skips one layer and inherits directly from + django_otp.views.LoginView. + """ def __init__(self, *args, **kwargs): self.use_recovery = kwargs.pop('use_recovery', False) -- 2.30.2 From 1377addf3e3191a494e9d3c648f812f7800b570f Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 22 Aug 2024 20:38:34 +0200 Subject: [PATCH 35/44] mfa alternatives --- bid_main/models.py | 20 ++--- .../bid_main/components/mfa_form.html | 14 ++-- .../bid_main/components/u2f_form.html | 7 +- bid_main/views/normal_pages.py | 74 +++++++++++++------ 4 files changed, 73 insertions(+), 42 deletions(-) diff --git a/bid_main/models.py b/bid_main/models.py index 85f1dcc..137b421 100644 --- a/bid_main/models.py +++ b/bid_main/models.py @@ -544,15 +544,17 @@ class User(AbstractBaseUser, PermissionsMixin): return bid_main.file_utils.get_absolute_url(self.avatar.storage.url(default_thumbnail_path)) def mfa_devices_per_type(self): - devices_per_type = defaultdict(list) - for device in devices_for_user(self): - if isinstance(device, EncryptedRecoveryDevice): - devices_per_type['recovery'].append(device) - if isinstance(device, EncryptedTOTPDevice): - devices_per_type['totp'].append(device) - if isinstance(device, U2fDevice): - devices_per_type['u2f'].append(device) - return devices_per_type + if not hasattr(self, '_mfa_devices'): + devices_per_type = defaultdict(list) + for device in devices_for_user(self): + if isinstance(device, EncryptedRecoveryDevice): + devices_per_type['recovery'].append(device) + if isinstance(device, EncryptedTOTPDevice): + devices_per_type['totp'].append(device) + if isinstance(device, U2fDevice): + devices_per_type['u2f'].append(device) + self._mfa_devices = devices_per_type + return self._mfa_devices class SettingValueField(models.CharField): diff --git a/bid_main/templates/bid_main/components/mfa_form.html b/bid_main/templates/bid_main/components/mfa_form.html index 70cefbf..2516fdc 100644 --- a/bid_main/templates/bid_main/components/mfa_form.html +++ b/bid_main/templates/bid_main/components/mfa_form.html @@ -8,10 +8,10 @@ {% with form=form|add_form_classes %}
        {% csrf_token %}
        - {% if use_recovery %} + {% if mfa_device_type == 'recovery' %}

        Use a recovery code

        - {% else %} + {% elif mfa_device_type == 'totp' %} {% if devices.totp|length == 1 %} {% else %} @@ -36,13 +36,11 @@ {% endwith %} - {% if devices.recovery %} + {% if mfa_alternatives %}
        - {% if use_recovery %} - Use an authenticator - {% else %} - Use a recovery code - {% endif %} + {% for item in mfa_alternatives %} + {{ item.label }} + {% endfor %}
        {% endif %}
        diff --git a/bid_main/templates/bid_main/components/u2f_form.html b/bid_main/templates/bid_main/components/u2f_form.html index 10ea7cc..dff2bbf 100644 --- a/bid_main/templates/bid_main/components/u2f_form.html +++ b/bid_main/templates/bid_main/components/u2f_form.html @@ -26,10 +26,11 @@
        {% endwith %} - {% if devices.totp %} + {% if mfa_alternatives %}
        - Use an authenticator - Use a recovery code + {% for item in mfa_alternatives %} + {{ item.label }} + {% endfor %}
        {% endif %}
    diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index 18a3d16..35ac95a 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -34,6 +34,7 @@ import oauth2_provider.models as oauth2_models from .. import forms, email from . import mixins from bid_main.email import send_verify_address +from bid_main.templatetags.common import query_transform from mfa.fido2 import authenticate_begin from mfa.forms import MfaAuthenticateForm, U2fAuthenticateForm import bid_main.file_utils @@ -87,11 +88,12 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, django_o and django_otp makes use of that to switch between otp_authentication_form and otp_token_form - we overwrite otp_authentication_form to exclude the otp_device and otp_token fields, because we don't have mandatory MFA for everyone - On top of that we have a separate helper for injecting U2fAuthenticateForm if a user has a u2f - device. + On top of that we have additional logic in get_form for injecting U2fAuthenticateForm if a user + has a u2f device. - Because get_form is called on both GET and POST requests we put the branching logic there, + Because get_form is called on both GET and POST requests we put our branching logic there, following the example of django_otp. + Then get_context_data checks the form class and supplies the data needed by a corresponding UI. """ otp_authentication_form = forms.AuthenticationForm @@ -104,16 +106,39 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, django_o authorize_url = reverse_lazy("oauth2_provider:authorize") def _mfa_device_type(self): - return self.request.GET.get("mfa_device_type", None) + default_type = None + available_types = self.request.user.mfa_devices_per_type() + if "u2f" in available_types: + default_type = "u2f" + elif "totp" in available_types: + default_type = "totp" + return self.request.GET.get("mfa_device_type", default_type) - def _use_recovery(self): - return self._mfa_device_type() == "recovery" - - def _use_totp(self): - return self._mfa_device_type() == "totp" - - def _use_u2f(self): - return self._mfa_device_type() == "u2f" + def _mfa_alternatives(self): + r = {"request": self.request} + alternatives = [ + { + "href": "?" + query_transform(r, mfa_device_type="u2f"), + "label": _("Use U2F security key"), + "type": "u2f", + }, + { + "href": "?" + query_transform(r, mfa_device_type="totp"), + "label": _("Use TOTP authenticator"), + "type": "totp", + }, + { + "href": "?" + query_transform(r, mfa_device_type="recovery"), + "label": _("Use recovery code"), + "type": "recovery", + }, + ] + available_types = self.request.user.mfa_devices_per_type() + mfa_device_type = self._mfa_device_type() + return [ + item for item in alternatives + if item["type"] in available_types and item["type"] != mfa_device_type + ] @method_decorator(sensitive_post_parameters()) @method_decorator(csrf_protect) @@ -146,36 +171,41 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, django_o if isinstance(form, U2fAuthenticateForm): ctx["devices"] = self.request.user.mfa_devices_per_type() ctx["form_type"] = "u2f" + ctx["mfa_alternatives"] = self._mfa_alternatives() + ctx["mfa_device_type"] = "u2f" elif isinstance(form, MfaAuthenticateForm): ctx["devices"] = self.request.user.mfa_devices_per_type() - ctx["use_recovery"] = self._use_recovery() ctx["form_type"] = "mfa" + ctx["mfa_alternatives"] = self._mfa_alternatives() + ctx["mfa_device_type"] = self._mfa_device_type() return ctx def _get_u2f_form(self, devices): credentials = [AttestedCredentialData(d.credential) for d in devices] - rp_id = self.request.get_host().split(':')[0] # remove port, required by webauthn + rp_id = self.request.get_host().split(":")[0] # remove port, required by webauthn request_options, state = authenticate_begin(rp_id, credentials) kwargs = self.get_form_kwargs() - kwargs['credentials'] = credentials - kwargs['request_options'] = json.dumps(dict(request_options)) - kwargs['rp_id'] = rp_id - kwargs['state'] = dict(state) + kwargs["credentials"] = credentials + kwargs["request_options"] = json.dumps(dict(request_options)) + kwargs["rp_id"] = rp_id + kwargs["state"] = dict(state) return U2fAuthenticateForm(**kwargs) def get_form(self): if self.request.user.is_authenticated: - devices = self.request.user.mfa_devices_per_type().get('u2f', None) - if devices and not self._use_recovery() and not self._use_totp(): - return self._get_u2f_form(devices) + u2f_devices = self.request.user.mfa_devices_per_type().get("u2f") + mfa_device_type = self._mfa_device_type() + if u2f_devices and (not mfa_device_type or mfa_device_type == "u2f"): + return self._get_u2f_form(u2f_devices) # this will switch between MfaAuthenticateForm and AuthenticationForm # as defined in django_otp.views.LoginView.authentication_form implementation return super().get_form() def get_form_kwargs(self): kwargs = super().get_form_kwargs() - if self._use_recovery(): + # this will affect only MfaAuthenticateForm, but making an explicit check here is cumbersome + if self.request.user.is_authenticated and self._mfa_device_type() == "recovery": kwargs["use_recovery"] = True return kwargs -- 2.30.2 From 40f6f0555fbcc472c1a74e8f72a52c8d0399a4ea Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 27 Aug 2024 12:10:06 +0200 Subject: [PATCH 36/44] show mfa button only to internal users --- bid_main/templates/bid_main/index.html | 2 ++ bid_main/views/normal_pages.py | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/bid_main/templates/bid_main/index.html b/bid_main/templates/bid_main/index.html index 87df289..0a5b919 100644 --- a/bid_main/templates/bid_main/index.html +++ b/bid_main/templates/bid_main/index.html @@ -139,9 +139,11 @@ Profile Active Sessions + {% if show_mfa %} Multi-factor Authentication + {% endif %}
    diff --git a/bid_main/views/normal_pages.py b/bid_main/views/normal_pages.py index 35ac95a..e13ab7f 100644 --- a/bid_main/views/normal_pages.py +++ b/bid_main/views/normal_pages.py @@ -67,14 +67,20 @@ class IndexView(mixins.MfaRequiredIfConfiguredMixin, mixins.PageIdMixin, Templat name for name, roles in self.BID_APP_TO_ROLES.items() if roles.intersection(role_names) } + show_mfa = ( + user.mfa_devices_per_type + or (user.email.endswith('@blender.org') and user.confirmed_email_at) + ) + return { **super().get_context_data(**kwargs), "apps": apps, "cloud_needs_renewal": ( "cloud_has_subscription" in role_names and "cloud_subscriber" not in role_names ), - "show_confirm_address": not user.has_confirmed_email, "private_badge_ids": {role.id for role in user.private_badges.all()}, + "show_confirm_address": not user.has_confirmed_email, + "show_mfa": show_mfa, } -- 2.30.2 From 94df1fcd8060d4b6ab7331a9801a9c213233a97f Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 27 Aug 2024 12:37:46 +0200 Subject: [PATCH 37/44] some css updates --- bid_main/templates/bid_main/mfa/disable.html | 6 ++++-- bid_main/templates/bid_main/mfa/setup.html | 12 ++++++------ bid_main/templates/bid_main/mfa/totp_register.html | 6 ++++-- bid_main/templates/bid_main/mfa/u2f_register.html | 6 ++++-- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/disable.html b/bid_main/templates/bid_main/mfa/disable.html index 136a2c3..9c8e961 100644 --- a/bid_main/templates/bid_main/mfa/disable.html +++ b/bid_main/templates/bid_main/mfa/disable.html @@ -17,7 +17,9 @@ Disable Multi-factor Authentication {% include "components/forms/field.html" %} {% endwith %} {% endwith %} - - Cancel +
    + + Cancel +
    {% endblock %} diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index 61591f6..a948ceb 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -25,7 +25,7 @@ Multi-factor Authentication Setup Verification also expires after {{ agent_inactivity_days }} days of inactivity.

    {% else %}

    @@ -60,7 +60,7 @@ Multi-factor Authentication Setup

    Security keys (U2F, WebAuthn, FIDO2)

    - E.g. yubikey. + E.g. a yubikey.

      {% for d in devices_per_type.u2f %} @@ -87,12 +87,12 @@ Multi-factor Authentication Setup {% with code_count=recovery_codes|length %} {{ code_count }} recovery code{{ code_count|pluralize }} remaining {% if display_recovery_codes %} - Hide + Hide {% else %} - Display + Display {% endif %}
      {% csrf_token %} - +
      {% if display_recovery_codes %}
        @@ -105,7 +105,7 @@ Multi-factor Authentication Setup
    {% endif %}
    {% csrf_token %} - +
    {% endwith %}
    diff --git a/bid_main/templates/bid_main/mfa/totp_register.html b/bid_main/templates/bid_main/mfa/totp_register.html index ec57163..a22d683 100644 --- a/bid_main/templates/bid_main/mfa/totp_register.html +++ b/bid_main/templates/bid_main/mfa/totp_register.html @@ -45,8 +45,10 @@ Multi-factor Authentication Setup {% with field=form.signature %} {% include "components/forms/field.html" %} {% endwith %} - - Cancel +
    + + Cancel +
    {% if form.non_field_errors %}
    something went wrong diff --git a/bid_main/templates/bid_main/mfa/u2f_register.html b/bid_main/templates/bid_main/mfa/u2f_register.html index 0601ef3..5e56806 100644 --- a/bid_main/templates/bid_main/mfa/u2f_register.html +++ b/bid_main/templates/bid_main/mfa/u2f_register.html @@ -24,8 +24,10 @@ Multi-factor Authentication Setup {% with field=form.state %} {% include "components/forms/field.html" %} {% endwith %} - - Cancel +
    + + Cancel +
    {% if form.non_field_errors %}
    something went wrong -- 2.30.2 From e90e875c1b0c4cb8926eca7c11b3dc19ad23a223 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 27 Aug 2024 12:44:13 +0200 Subject: [PATCH 38/44] rename for consistency --- bid_main/email.py | 4 ++-- bid_main/tasks.py | 4 ++-- .../emails/{new_mfa_device.txt => mfa_new_device.txt} | 0 bid_main/views/mfa.py | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) rename bid_main/templates/bid_main/emails/{new_mfa_device.txt => mfa_new_device.txt} (100%) diff --git a/bid_main/email.py b/bid_main/email.py index 9198bfb..98f66db 100644 --- a/bid_main/email.py +++ b/bid_main/email.py @@ -284,13 +284,13 @@ def construct_password_changed(user): return email_body_txt, subject -def construct_new_mfa_device(user, device_type): +def construct_mfa_new_device(user, device_type): context = { "device_type": device_type, "user": user, } email_body_txt = loader.render_to_string( - "bid_main/emails/new_mfa_device.txt", context + "bid_main/emails/mfa_new_device.txt", context ) subject = "Security alert: a new multi-factor authentication device added" diff --git a/bid_main/tasks.py b/bid_main/tasks.py index 77891f1..9070738 100644 --- a/bid_main/tasks.py +++ b/bid_main/tasks.py @@ -46,12 +46,12 @@ def send_password_changed_email(user_pk): @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) -def send_new_mfa_device_email(user_pk, device_type): +def send_mfa_new_device_email(user_pk, device_type): user = User.objects.get(pk=user_pk) log.info("sending a new mfa device email for account %s", user.pk) # sending only a text/plain email to reduce the room for look-alike phishing emails - email_body_txt, subject = bid_main.email.construct_new_mfa_device(user, device_type) + email_body_txt, subject = bid_main.email.construct_mfa_new_device(user, device_type) email = user.email send_mail( diff --git a/bid_main/templates/bid_main/emails/new_mfa_device.txt b/bid_main/templates/bid_main/emails/mfa_new_device.txt similarity index 100% rename from bid_main/templates/bid_main/emails/new_mfa_device.txt rename to bid_main/templates/bid_main/emails/mfa_new_device.txt diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 16e7815..1e64b30 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -132,7 +132,7 @@ class TotpRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView): def form_valid(self, form): form.save() if self.request.user.confirmed_email_at: - bid_main.tasks.send_new_mfa_device_email(self.request.user.pk, 'totp') + bid_main.tasks.send_mfa_new_device_email(self.request.user.pk, 'totp') return super().form_valid(form) @@ -161,7 +161,7 @@ class U2fRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView): def form_valid(self, form): form.save() if self.request.user.confirmed_email_at: - bid_main.tasks.send_new_mfa_device_email(self.request.user.pk, 'u2f') + bid_main.tasks.send_mfa_new_device_email(self.request.user.pk, 'u2f') return super().form_valid(form) -- 2.30.2 From 89d52689489ac4832fc73b4f3092eccacc9259ff Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 27 Aug 2024 12:51:20 +0200 Subject: [PATCH 39/44] more css fixes --- bid_main/templates/bid_main/mfa/delete_device.html | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/bid_main/templates/bid_main/mfa/delete_device.html b/bid_main/templates/bid_main/mfa/delete_device.html index 13d6af4..5837909 100644 --- a/bid_main/templates/bid_main/mfa/delete_device.html +++ b/bid_main/templates/bid_main/mfa/delete_device.html @@ -12,7 +12,9 @@ Delete {{ object.name }} {% with form=form|add_form_classes %} {{ form }} {% endwith %} - - Cancel +
    + + Cancel +
    {% endblock %} -- 2.30.2 From f62ea85d1f9637d1216beed80f358f30a3eaf02a Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 27 Aug 2024 12:51:36 +0200 Subject: [PATCH 40/44] simplify include-with --- .../templates/bid_main/components/mfa_form.html | 8 ++------ .../templates/bid_main/components/u2f_form.html | 16 ++++------------ bid_main/templates/bid_main/mfa/disable.html | 4 +--- .../templates/bid_main/mfa/totp_register.html | 16 ++++------------ .../templates/bid_main/mfa/u2f_register.html | 16 ++++------------ 5 files changed, 15 insertions(+), 45 deletions(-) diff --git a/bid_main/templates/bid_main/components/mfa_form.html b/bid_main/templates/bid_main/components/mfa_form.html index 2516fdc..891e2ee 100644 --- a/bid_main/templates/bid_main/components/mfa_form.html +++ b/bid_main/templates/bid_main/components/mfa_form.html @@ -25,12 +25,8 @@
    {% endif %} {% endif %} - {% with field=form.otp_token %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.otp_trust_agent %} - {% include "components/forms/field.html" with with_help_text=True %} - {% endwith %} + {% include "components/forms/field.html" with field=form.otp_token %} + {% include "components/forms/field.html" with field=form.otp_trust_agent with_help_text=True %} {{ form.non_field_errors }} diff --git a/bid_main/templates/bid_main/components/u2f_form.html b/bid_main/templates/bid_main/components/u2f_form.html index dff2bbf..6677635 100644 --- a/bid_main/templates/bid_main/components/u2f_form.html +++ b/bid_main/templates/bid_main/components/u2f_form.html @@ -9,18 +9,10 @@
    {% csrf_token %}

    Please use a security key you have configured. Tick the checkbox below before using the key if you want to remember this device.

    - {% with field=form.otp_trust_agent %} - {% include "components/forms/field.html" with with_help_text=True %} - {% endwith %} - {% with field=form.response %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.signature %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.state %} - {% include "components/forms/field.html" %} - {% endwith %} + {% include "components/forms/field.html" with field=form.otp_trust_agent with_help_text=True %} + {% include "components/forms/field.html" with field=form.response %} + {% include "components/forms/field.html" with field=form.signature %} + {% include "components/forms/field.html" with field=form.state %}
    {{ form.non_field_errors }}
    diff --git a/bid_main/templates/bid_main/mfa/disable.html b/bid_main/templates/bid_main/mfa/disable.html index 9c8e961..ef04bf6 100644 --- a/bid_main/templates/bid_main/mfa/disable.html +++ b/bid_main/templates/bid_main/mfa/disable.html @@ -13,9 +13,7 @@ Disable Multi-factor Authentication

    {% csrf_token %} {% with form=form|add_form_classes %} - {% with field=form.disable_mfa_confirm %} - {% include "components/forms/field.html" %} - {% endwith %} + {% include "components/forms/field.html" with field=form.disable_mfa_confirm %} {% endwith %}
    diff --git a/bid_main/templates/bid_main/mfa/totp_register.html b/bid_main/templates/bid_main/mfa/totp_register.html index a22d683..0d1bb94 100644 --- a/bid_main/templates/bid_main/mfa/totp_register.html +++ b/bid_main/templates/bid_main/mfa/totp_register.html @@ -33,18 +33,10 @@ Multi-factor Authentication Setup
    {% with form=form|add_form_classes %} {% csrf_token %} - {% with field=form.name %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.code %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.key %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.signature %} - {% include "components/forms/field.html" %} - {% endwith %} + {% include "components/forms/field.html" with field=form.name %} + {% include "components/forms/field.html" with field=form.code %} + {% include "components/forms/field.html" with field=form.key %} + {% include "components/forms/field.html" with field=form.signature %}
    Cancel diff --git a/bid_main/templates/bid_main/mfa/u2f_register.html b/bid_main/templates/bid_main/mfa/u2f_register.html index 5e56806..73991a7 100644 --- a/bid_main/templates/bid_main/mfa/u2f_register.html +++ b/bid_main/templates/bid_main/mfa/u2f_register.html @@ -12,18 +12,10 @@ Multi-factor Authentication Setup
    {% with form=form|add_form_classes %} {% csrf_token %} - {% with field=form.credential %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.name %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.signature %} - {% include "components/forms/field.html" %} - {% endwith %} - {% with field=form.state %} - {% include "components/forms/field.html" %} - {% endwith %} + {% include "components/forms/field.html" with field=form.credential %} + {% include "components/forms/field.html" with field=form.name %} + {% include "components/forms/field.html" with field=form.signature %} + {% include "components/forms/field.html" with field=form.state %}
    Cancel -- 2.30.2 From f4beaab51e60d560a16dd11e15efa77cc708bcb7 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Tue, 27 Aug 2024 13:03:07 +0200 Subject: [PATCH 41/44] remove useless css class --- bid_main/templates/components/forms/field.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bid_main/templates/components/forms/field.html b/bid_main/templates/components/forms/field.html index 0670a9c..61439be 100644 --- a/bid_main/templates/components/forms/field.html +++ b/bid_main/templates/components/forms/field.html @@ -5,7 +5,7 @@ {% if not field.is_hidden %} {% include 'components/forms/label.html' with label_class="form-check-label" %} {% if with_help_text and field.help_text %} - {{ field.help_text|safe }} + {{ field.help_text|safe }} {% endif %} {% endif %}
    {{ field.errors }}
    @@ -18,7 +18,7 @@ {{ field }}
    {{ field.errors }}
    {% if with_help_text and field.help_text %} - {{ field.help_text|safe }} + {{ field.help_text|safe }} {% endif %}
    {% endif %} -- 2.30.2 From 9def65b4343be632288ff3482bd266bd74b3884f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1rton=20Lente?= Date: Tue, 27 Aug 2024 16:04:22 +0200 Subject: [PATCH 42/44] Fix: Template component fields layout checkbox base --- bid_main/templates/components/forms/field.html | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/bid_main/templates/components/forms/field.html b/bid_main/templates/components/forms/field.html index 61439be..6854628 100644 --- a/bid_main/templates/components/forms/field.html +++ b/bid_main/templates/components/forms/field.html @@ -4,12 +4,13 @@ {{ field }} {% if not field.is_hidden %} {% include 'components/forms/label.html' with label_class="form-check-label" %} - {% if with_help_text and field.help_text %} - {{ field.help_text|safe }} {% endif %} +
    + + {% if with_help_text and field.help_text %} + {{ field.help_text|safe }} {% endif %}
    {{ field.errors }}
    -
    {% else %}
    {% if not field.is_hidden %} -- 2.30.2 From 585dc3527414123e34a10717687afe5bcf1e483a Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 29 Aug 2024 11:28:58 +0200 Subject: [PATCH 43/44] review fixes --- bid_main/email.py | 2 ++ bid_main/forms.py | 4 ++-- bid_main/signals.py | 6 +++--- bid_main/tasks.py | 10 +++++----- .../bid_main/emails/mfa_new_device.txt | 2 +- .../bid_main/emails/mfa_recovery_used.txt | 2 +- bid_main/templates/bid_main/mfa/setup.html | 7 +++++-- .../templates/bid_main/mfa/u2f_register.html | 8 ++++++++ bid_main/tests/test_password_change.py | 4 ++-- bid_main/tests/test_user_sessions.py | 4 ++-- bid_main/views/mfa.py | 17 +++++++++++------ blenderid/settings.py | 3 +++ mfa/forms.py | 3 --- 13 files changed, 45 insertions(+), 27 deletions(-) diff --git a/bid_main/email.py b/bid_main/email.py index 98f66db..8252261 100644 --- a/bid_main/email.py +++ b/bid_main/email.py @@ -287,6 +287,7 @@ def construct_password_changed(user): def construct_mfa_new_device(user, device_type): context = { "device_type": device_type, + "support_email": settings.SUPPORT_EMAIL, "user": user, } email_body_txt = loader.render_to_string( @@ -311,6 +312,7 @@ def construct_mfa_disabled(user): def construct_mfa_recovery_used(user): context = { + "support_email": settings.SUPPORT_EMAIL, "user": user, } email_body_txt = loader.render_to_string( diff --git a/bid_main/forms.py b/bid_main/forms.py index fc04797..4a1ac29 100644 --- a/bid_main/forms.py +++ b/bid_main/forms.py @@ -316,7 +316,7 @@ class PasswordChangeForm(BootstrapModelFormMixin, auth_forms.PasswordChangeForm) def save(self, *args, **kwargs): user = super().save(*args, **kwargs) if user.has_confirmed_email: - bid_main.tasks.send_password_changed_email(user_pk=user.pk) + bid_main.tasks.send_mail_password_changed(user_pk=user.pk) return user @@ -335,7 +335,7 @@ class SetPasswordForm(auth_forms.SetPasswordForm): def save(self, *args, **kwargs): user = super().save(*args, **kwargs) if user.has_confirmed_email: - bid_main.tasks.send_password_changed_email(user_pk=user.pk) + bid_main.tasks.send_mail_password_changed(user_pk=user.pk) return user diff --git a/bid_main/signals.py b/bid_main/signals.py index 9b9d2dc..25389c2 100644 --- a/bid_main/signals.py +++ b/bid_main/signals.py @@ -39,7 +39,7 @@ def process_new_login(sender, request, user, **kwargs): fields.update({"last_login_ip", "current_login_ip"}) if user.has_confirmed_email: - bid_main.tasks.send_new_user_session_email( + bid_main.tasks.send_mail_new_user_session( user_pk=user.pk, session_data={ 'device': str(user_session.device or 'Unknown'), @@ -83,7 +83,7 @@ def delete_orphaned_avatar_files(sender, instance, **kwargs): @receiver(recovery_used) -def send_mfa_recovery_used_email(sender, **kwargs): +def send_mail_mfa_recovery_used(sender, **kwargs): user = kwargs['device'].user if user.confirmed_email_at: - bid_main.tasks.send_mfa_recovery_used_email(user.pk) + bid_main.tasks.send_mail_mfa_recovery_used(user.pk) diff --git a/bid_main/tasks.py b/bid_main/tasks.py index 9070738..d36f5e4 100644 --- a/bid_main/tasks.py +++ b/bid_main/tasks.py @@ -12,7 +12,7 @@ log = logging.getLogger(__name__) @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) -def send_new_user_session_email(user_pk, session_data): +def send_mail_new_user_session(user_pk, session_data): user = User.objects.get(pk=user_pk) log.info("sending a new user session email for account %s", user.pk) @@ -29,7 +29,7 @@ def send_new_user_session_email(user_pk, session_data): @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) -def send_password_changed_email(user_pk): +def send_mail_password_changed(user_pk): user = User.objects.get(pk=user_pk) log.info("sending a password change email for account %s", user.pk) @@ -46,7 +46,7 @@ def send_password_changed_email(user_pk): @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) -def send_mfa_new_device_email(user_pk, device_type): +def send_mail_mfa_new_device(user_pk, device_type): user = User.objects.get(pk=user_pk) log.info("sending a new mfa device email for account %s", user.pk) @@ -63,7 +63,7 @@ def send_mfa_new_device_email(user_pk, device_type): @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) -def send_mfa_disabled_email(user_pk): +def send_mail_mfa_disabled(user_pk): user = User.objects.get(pk=user_pk) log.info("sending an mfa disabled email for account %s", user.pk) @@ -80,7 +80,7 @@ def send_mfa_disabled_email(user_pk): @background(schedule={'action': TaskSchedule.RESCHEDULE_EXISTING}) -def send_mfa_recovery_used_email(user_pk): +def send_mail_mfa_recovery_used(user_pk): user = User.objects.get(pk=user_pk) log.info("sending an mfa recovery used email for account %s", user.pk) diff --git a/bid_main/templates/bid_main/emails/mfa_new_device.txt b/bid_main/templates/bid_main/emails/mfa_new_device.txt index 67a846f..1ea9638 100644 --- a/bid_main/templates/bid_main/emails/mfa_new_device.txt +++ b/bid_main/templates/bid_main/emails/mfa_new_device.txt @@ -3,7 +3,7 @@ Dear {{ user.full_name|default:user.email }}! A new {{ device_type }} multi-factor authenticator has been added to your Blender ID account {{ user.email }} -If this wasn't done by you, please reset your password immediately and contact blenderid@blender.org for support. +If this wasn't done by you, please reset your password immediately and contact {{ support_email }} for support. -- Kind regards, diff --git a/bid_main/templates/bid_main/emails/mfa_recovery_used.txt b/bid_main/templates/bid_main/emails/mfa_recovery_used.txt index 1f6bce8..a16f82f 100644 --- a/bid_main/templates/bid_main/emails/mfa_recovery_used.txt +++ b/bid_main/templates/bid_main/emails/mfa_recovery_used.txt @@ -3,7 +3,7 @@ Dear {{ user.full_name|default:user.email }}! A recovery code was used to pass multi-factor authentication for your Blender ID account {{ user.email }} -If this wasn't done by you, please reset your password immediately, re-generate your MFA recovery codes, and contact blenderid@blender.org for support. +If this wasn't done by you, please reset your password immediately, re-generate your MFA recovery codes, and contact {{ support_email }} for support. -- Kind regards, diff --git a/bid_main/templates/bid_main/mfa/setup.html b/bid_main/templates/bid_main/mfa/setup.html index a948ceb..3a1e499 100644 --- a/bid_main/templates/bid_main/mfa/setup.html +++ b/bid_main/templates/bid_main/mfa/setup.html @@ -60,7 +60,10 @@ Multi-factor Authentication Setup

    Security keys (U2F, WebAuthn, FIDO2)

    - E.g. a yubikey. + Hardware security keys, e.g. Yubikeys. +

    +

    + Blender ID supports these keys only as a second factor and does not provide a passwordless sign-in.

      {% for d in devices_per_type.u2f %} @@ -79,7 +82,7 @@ Multi-factor Authentication Setup

      Store your recovery codes safely (e.g. in a password manager or use a printed copy) and don't share them. Each code can be used only once. - You can generate a new set of recovery codes at any time, any remaining old codes will become invalidated. + You can generate a new set of recovery codes at any time, any remaining old codes will be invalidated.

      {% with recovery=devices_per_type.recovery.0 %} {% if recovery %} diff --git a/bid_main/templates/bid_main/mfa/u2f_register.html b/bid_main/templates/bid_main/mfa/u2f_register.html index 73991a7..7ce4c36 100644 --- a/bid_main/templates/bid_main/mfa/u2f_register.html +++ b/bid_main/templates/bid_main/mfa/u2f_register.html @@ -8,6 +8,14 @@ Multi-factor Authentication Setup {% block body %}

      New U2F device

      +
      +
      +

      Please watch setup video if you are not familiar with yubikeys.

      + {% if first_device %} +

      Since this is your first MFA device, you will be promted to use your security key immediately after setup to sign-in using MFA.

      + {% endif %} +
      +
      {% with form=form|add_form_classes %} diff --git a/bid_main/tests/test_password_change.py b/bid_main/tests/test_password_change.py index 6d37da7..3a5686d 100644 --- a/bid_main/tests/test_password_change.py +++ b/bid_main/tests/test_password_change.py @@ -12,8 +12,8 @@ import bid_main.tasks @patch( - 'bid_main.tasks.send_password_changed_email', - new=bid_main.tasks.send_password_changed_email.task_function, + 'bid_main.tasks.send_mail_password_changed', + new=bid_main.tasks.send_mail_password_changed.task_function, ) @patch( 'django.contrib.auth.base_user.AbstractBaseUser.check_password', diff --git a/bid_main/tests/test_user_sessions.py b/bid_main/tests/test_user_sessions.py index d4034d4..086a22e 100644 --- a/bid_main/tests/test_user_sessions.py +++ b/bid_main/tests/test_user_sessions.py @@ -50,8 +50,8 @@ class TestActiveSessions(TestCase): class TestNewUserSessionEmail(TestCase): @patch( - 'bid_main.tasks.send_new_user_session_email', - new=bid_main.tasks.send_new_user_session_email.task_function, + 'bid_main.tasks.send_mail_new_user_session', + new=bid_main.tasks.send_mail_new_user_session.task_function, ) @patch( 'django.contrib.auth.base_user.AbstractBaseUser.check_password', diff --git a/bid_main/views/mfa.py b/bid_main/views/mfa.py index 1e64b30..cb9e635 100644 --- a/bid_main/views/mfa.py +++ b/bid_main/views/mfa.py @@ -69,7 +69,7 @@ class DisableView(mixins.MfaRequiredMixin, FormView): for device in devices_for_user(self.request.user): device.delete() if self.request.user.confirmed_email_at: - bid_main.tasks.send_mfa_disabled_email(self.request.user.pk) + bid_main.tasks.send_mail_mfa_disabled(self.request.user.pk) return super().form_valid(form) @@ -82,7 +82,7 @@ class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): ): # Forbid setting up recovery codes unless the user already has some other method return HttpResponseBadRequest("can't setup recovery codes before other methods") - user.staticdevice_set.all().delete() + EncryptedRecoveryDevice.objects.filter(user=user).delete() device = EncryptedRecoveryDevice.objects.create(name='recovery', user=user) for _ in range(10): # https://pages.nist.gov/800-63-3/sp800-63b.html#5122-look-up-secret-verifiers @@ -91,10 +91,10 @@ class GenerateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): return redirect(reverse('bid_main:mfa') + '?display_recovery_codes=1#recovery-codes') -class InvalidateRecoveryView(mixins.MfaRequiredIfConfiguredMixin, View): +class InvalidateRecoveryView(mixins.MfaRequiredMixin, View): def post(self, request, *args, **kwargs): user = self.request.user - user.staticdevice_set.all().delete() + EncryptedRecoveryDevice.objects.filter(user=user).delete() return redirect('bid_main:mfa') @@ -132,7 +132,7 @@ class TotpRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView): def form_valid(self, form): form.save() if self.request.user.confirmed_email_at: - bid_main.tasks.send_mfa_new_device_email(self.request.user.pk, 'totp') + bid_main.tasks.send_mail_mfa_new_device(self.request.user.pk, 'totp') return super().form_valid(form) @@ -141,6 +141,11 @@ class U2fRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView): success_url = reverse_lazy('bid_main:mfa') template_name = "bid_main/mfa/u2f_register.html" + def get_context_data(self, **kwargs): + context = super().get_context_data(**kwargs) + context['first_device'] = not devices_for_user(self.request.user) + return context + def get_form_kwargs(self): credentials = [ AttestedCredentialData(d.credential) @@ -161,7 +166,7 @@ class U2fRegisterView(mixins.MfaRequiredIfConfiguredMixin, FormView): def form_valid(self, form): form.save() if self.request.user.confirmed_email_at: - bid_main.tasks.send_mfa_new_device_email(self.request.user.pk, 'u2f') + bid_main.tasks.send_mail_mfa_new_device(self.request.user.pk, 'u2f') return super().form_valid(form) diff --git a/blenderid/settings.py b/blenderid/settings.py index 6a3ecf8..e2c6128 100644 --- a/blenderid/settings.py +++ b/blenderid/settings.py @@ -428,3 +428,6 @@ if os.environ.get('ADMINS') is not None: ADMINS = [[_.strip() for _ in adm.split(':')] for adm in os.environ.get('ADMINS').split(',')] EMAIL_SUBJECT_PREFIX = f'[{ALLOWED_HOSTS[0]}]' SERVER_EMAIL = f'django@{ALLOWED_HOSTS[0]}' + + +SUPPORT_EMAIL = 'blenderid@blender.org' diff --git a/mfa/forms.py b/mfa/forms.py index 6a63d3c..1047587 100644 --- a/mfa/forms.py +++ b/mfa/forms.py @@ -132,9 +132,6 @@ class U2fAuthenticateForm(OTPAgentFormMixin, forms.Form): self.clean_agent() return self.cleaned_data - def save(self): - pass - class DisableMfaForm(forms.Form): disable_mfa_confirm = forms.BooleanField( -- 2.30.2 From 3439efb9a8bdf4933424c61acead1c0910516791 Mon Sep 17 00:00:00 2001 From: Oleg Komarov Date: Thu, 29 Aug 2024 11:34:02 +0200 Subject: [PATCH 44/44] doc update --- docs/mfa.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/mfa.md b/docs/mfa.md index 8c35733..cd7a0a2 100644 --- a/docs/mfa.md +++ b/docs/mfa.md @@ -7,10 +7,13 @@ include_toc: true ## Supported configurations -- TOTP authenticators +- TOTP authenticators (having multiple is supported) +- Yubikey (having multiple keys per account is supported) - TOTP + recovery codes +- Yubikey + recovery codes +- Yubikey + TOTP + recovery codes -TODO: yubikeys, one-time recovery code via email +TODO: one-time recovery code via email ## Implementation details -- 2.30.2