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

bid_main/templates/bid_main/components/mfa_form.html

@@ -39,9 +39,9 @@
   {% if devices.recovery %}
   <div class="bid-links">
   {% if use_recovery %}
-  <a href="?{% query_transform use_recovery='' %}">Use an authenticator</a>
+  <a href="?{% query_transform mfa_device_type='totp' %}">Use an authenticator</a>
   {% else %}
-  Lost your authenticator? <a href="?{% query_transform use_recovery=1 %}">Use a recovery code</a>
+  <a href="?{% query_transform mfa_device_type='recovery' %}">Use a recovery code</a>
   {% endif %}
   </div>
   {% endif %}

bid_main/templates/bid_main/components/u2f_form.html

@@ -8,7 +8,7 @@
   {% with form=form|add_form_classes %}
     <form method="POST" id="u2f-authenticate-form">{% csrf_token %}
       <fieldset class="mb-4">
-        <p>Please use a security key you have configured. Tick the checkbox below before using the key</p>
+        <p>Please use a security key you have configured. Tick the checkbox below before using the key if you want to remember this device.</p>
         {% with field=form.otp_trust_agent %}
           {% include "components/forms/field.html" with with_help_text=True %}
         {% endwith %}
@@ -27,8 +27,10 @@
     </form>
   {% endwith %}
   {% if devices.totp %}
-  <a href="?{% query_transform use_totp=1 %}">Use an authenticator</a>
-  Lost your authenticator? <a href="?{% query_transform use_recovery=1 %}">Use a recovery code</a>
+  <div class="bid-links">
+    <a href="?{% query_transform mfa_device_type='totp' %}">Use an authenticator</a>
+    <a href="?{% query_transform mfa_device_type='recovery' %}">Use a recovery code</a>
+  </div>
   {% endif %}
 </div>
 <script src="{% static 'mfa/js/webauthn-json.js' %}"></script>

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 <a href="https://en.wikipedia.org/wiki/Comparison_of_OTP_applications">TOTP applications</a>.
   </p>
   <ul>
-    {% for d in devices_per_category.totp %}
+    {% for d in devices_per_type.totp %}
     <li>
       {{ d.name }}
       {% if d.last_used_at %}(Last used <abbr title="{{ d.last_used_at }}">{{ d.last_used_at|naturaltime }}</abbr>){% endif %}
@@ -63,7 +63,7 @@ Multi-factor Authentication Setup
     E.g. yubikey.
   </p>
   <ul>
-    {% for d in devices_per_category.u2f %}
+    {% for d in devices_per_type.u2f %}
     <li>
       {{ d.name }}
       {% if d.last_used_at %}(Last used <abbr title="{{ d.last_used_at }}">{{ d.last_used_at|naturaltime }}</abbr>){% 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.
   </p>
-  {% with recovery=devices_per_category.recovery.0 %}
+  {% with recovery=devices_per_type.recovery.0 %}
   {% if recovery %}
   <div class="mb-3">
     {% with code_count=recovery_codes|length %}

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)

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

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,47 +77,83 @@ 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_form(self):
-        if self.request.user.is_authenticated:
-            devices = self.request.user.mfa_devices_per_category().get('u2f', None)
-            if devices and not self._use_recovery() and not self._use_totp():
+    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)
@@ -124,12 +163,19 @@ class LoginView(mixins.RedirectToPrivacyAgreeMixin, mixins.PageIdMixin, otp_agen
         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)
         # 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
 

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)