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
 

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.
 

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

bid_main/templates/bid_main/components/mfa_form.html

@@ -1,5 +1,5 @@
 {% load add_form_classes from forms %}
-{% load static %}
+{% load common static %}
 
 <div class="bid box">
   <div>
@@ -7,22 +7,42 @@
   </div>
   {% with form=form|add_form_classes %}
     <form role="login" action="" method="POST">{% csrf_token %}
-    <fieldset>
-    {% 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 %}
-      <p class="text-danger">{{ form.errors }}</p>
-    {% endif %}
-    </fieldset>
-    <button class="btn btn-block btn-accent" id="register">Continue</button>
+      <fieldset class="mb-4">
+        {% if use_recovery %}
+          <p>Use a recovery code</p>
+          <input type="hidden" name="otp_device" value="{{ devices.recovery.0.persistent_id }}" />
+        {% else %}
+          {% if devices.totp|length == 1 %}
+            <input type="hidden" name="otp_device" value="{{ devices.totp.0.persistent_id }}" />
+          {% else %}
+            <div class="form-check-inline mb-3">
+            {% for device in devices.totp %}
+              <label class="btn form-check-input">
+                <input type="radio" name="otp_device" value="{{ device.persistent_id }}" required="required" />
+                {{ device.name }}
+              </label>
+            {% endfor %}
+            </div>
+          {% 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 %}
+      </fieldset>
+      {{ form.non_field_errors }}
+      <button class="btn btn-block btn-accent">Continue</button>
     </form>
   {% endwith %}
+  {% if devices.recovery %}
+  <div class="bid-links">
+  {% if use_recovery %}
+  <a href="?{% query_transform use_recovery='' %}">Use an authenticator</a>
+  {% else %}
+  Lost your authenticator? <a href="?{% query_transform use_recovery=1 %}">Use a recovery code</a>
+  {% endif %}
+  </div>
+  {% endif %}
 </div>

bid_main/templates/bid_main/mfa/setup.html

@@ -42,7 +42,7 @@ Multi-factor Authentication Setup
 
 {% if user_can_setup_recovery %}
 <div class="bid box mt-3">
-  <h3>Recovery codes</h3>
+  <h3 id="recovery-codes">Recovery codes</h3>
   <p>
     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 %}
     <a href="?display_recovery_codes=" class="btn">Hide</a>
     {% else %}
-    <a href="?display_recovery_codes=1" class="btn">Display</a>
+    <a href="?display_recovery_codes=1#recovery-codes" class="btn">Display</a>
     {% endif %}
     <form action="{% url 'bid_main:mfa_invalidate_recovery' %}" method="post" class="d-inline-flex">{% csrf_token %}
       <button class="btn-danger" type="submit">Invalidate</button>

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 %}
-        <small class="form-text">{{ form.new_password1.help_text|safe }}</small>
+        <small class="form-text d-block">{{ field.help_text|safe }}</small>
       {% endif %}
     {% endif %}
     <div class="text-danger">{{ field.errors }}</div>
@@ -18,7 +18,7 @@
     {{ field }}
     <div class="text-danger">{{ field.errors }}</div>
     {% if with_help_text and field.help_text %}
-      <small class="form-text">{{ form.new_password1.help_text|safe }}</small>
+      <small class="form-text d-block">{{ field.help_text|safe }}</small>
     {% endif %}
   </div>
   {% endif %}

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()

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):

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."""
 

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