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.
   </p>
+  <p>TODO explain remember me and trusted days</p>
   <div>
     <a class="btn btn-danger" href="{% url 'bid_main:mfa_disable' %}">Disable</a>
   </div>
   {% else %}
   <p>
     MFA makes your account more secure against account takeover attacks.
+    You can read more in <a href="https://ssd.eff.org/module/how-enable-two-factor-authentication">a guide</a> from Electronic Frontier Foundation.
   </p>
   <p>
     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
 
 <div class="bid box mt-3">
   <h3>Time-based one-time password (TOTP)</h3>
-  <p>TODO Explain how to use TOTP, <a href="https://en.wikipedia.org/wiki/Comparison_of_OTP_applications">TOTP applications</a></p>
-  {% with devices=devices_per_category.totp %}
-  {% if devices %}
+  <p>
+    Also known as authenticator application.
+  </p>
+  <p>
+    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>
+  {% if devices_per_category.totp and not devices_per_category.recovery %}
+  <p class="text-danger">
+    Please make sure that you do not lock yourself out:
+    generate and store <a href="#recovery-codes">recovery codes</a> as a backup verification method.
+    If you lose your authenticator device you can use a recovery code to login and reconfigure your MFA methods.
+  </p>
+  {% endif %}
   <ul>
-    {% for d in devices %}
+    {% for d in devices_per_category.totp %}
     <li>{{ d.name }} <a class="btn btn-danger" href="{% url 'bid_main:mfa_delete_device' d.persistent_id %}"><i class="i-trash"></i></a></li>
     {% endfor %}
   </ul>
-  {% endif %}
-  {% endwith %}
   <a href="{% url 'bid_main:mfa_totp' %}" class="btn">Configure a new TOTP device</a>
 </div>
 
@@ -44,7 +54,7 @@ Multi-factor Authentication Setup
 <div class="bid box mt-3">
   <h3 id="recovery-codes">Recovery codes</h3>
   <p>
-    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.
   </p>

bid_main/templates/bid_main/mfa/totp.html

@@ -12,11 +12,25 @@ Multi-factor Authentication Setup
     <div class="col-md-6">
       <figure>
         <img src="data:image/png;base64,{{ qrcode }}" alt="QR code" />
-        <figcaption class="text-center helptext"><details><summary>show code for manual entry</summary>{{ manual_secret_key }}</details></figcaption>
+        <figcaption class="text-center text-secondary"><details><summary>show secret key for manual entry</summary><code class="text-nowrap">{{ manual_secret_key }}</code></details></figcaption>
       </figure>
     </div>
     <div class="col-md-6">
-      <p>TODO explain what's happening</p>
+      <ol class="pl-3">
+          <li>
+            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.
+          </li>
+          <li>Enter a 6-digit code shown in the authenticator.</li>
+          <li>Pick any device name for your authenticator that you would recognize <span class="text-secondary">(e.g. "FreeOTP on my phone")</span> and submit the form.</li>
+          {% if first_device %}
+          <li>Since this is your first MFA device, you will be promted to enter a new code once more to sign-in using MFA.</li>
+          {% endif %}
+      </ol>
+    </div>
+  </div>
+  <div class="row">
+    <div class="col-md-6">
       {% with form=form|add_form_classes %}
       <form method="post">{% csrf_token %}
         {% with field=form.name %}

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

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.

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