improve security and fix error msg

- Instead of passing the user as a hidden form element, we use a session variable.
- Introduces a 60 second limit on completing the login, and an exponentially increasing delay to attempt to login with 2FA if the code is entered incorrectly.
- use proper Django form error when incorrect otp value entered

bookwyrm/forms/edit_user.py

@@ -4,8 +4,6 @@ from django.contrib.auth.password_validation import validate_password
 from django.core.exceptions import ValidationError
 from django.utils.translation import gettext_lazy as _
 
-import pyotp
-
 from bookwyrm import models
 from bookwyrm.models.fields import ClearableFileInputWithWarning
 from .custom_form import CustomForm
@@ -118,32 +116,3 @@ class ConfirmPasswordForm(CustomForm):
 
         if not self.instance.check_password(password):
             self.add_error("password", _("Incorrect Password"))
-
-
-class Confirm2FAForm(CustomForm):
-    otp = forms.CharField(max_length=6, min_length=6, widget=forms.TextInput)
-
-    # IDK if we need this?
-    class Meta:
-        model = models.User
-        fields = ["otp_secret"]
-
-    def clean(self):
-        """Check otp matches"""
-        otp = self.data.get("otp")
-        totp = pyotp.TOTP(self.instance.otp_secret)
-
-        if not totp.verify(otp):
-            # maybe it's a backup code?
-            hotp = pyotp.HOTP(self.instance.otp_secret)
-            hotp_count = (
-                self.instance.hotp_count if self.instance.hotp_count is not None else 0
-            )
-
-            if not hotp.verify(otp, hotp_count):
-                self.add_error("otp", _("Code does not match"))
-
-            # TODO: backup codes
-            # increment the user hotp_count if it was an HOTP
-            # self.instance.hotp_count = hotp_count + 1
-            # self.instance.save(broadcast=False, update_fields=["hotp_count"])

bookwyrm/forms/landing.py

@@ -4,6 +4,8 @@ from django.contrib.auth.password_validation import validate_password
 from django.core.exceptions import ValidationError
 from django.utils.translation import gettext_lazy as _
 
+import pyotp
+
 from bookwyrm import models
 from .custom_form import CustomForm
 
@@ -74,3 +76,31 @@ class PasswordResetForm(CustomForm):
             validate_password(new_password)
         except ValidationError as err:
             self.add_error("password", err)
+
+
+class Confirm2FAForm(CustomForm):
+    otp = forms.CharField(max_length=6, min_length=6, widget=forms.TextInput)
+
+    class Meta:
+        model = models.User
+        fields = ["otp_secret", "hotp_count"]
+
+    def clean_otp(self):
+        """Check otp matches"""
+        otp = self.data.get("otp")
+        totp = pyotp.TOTP(self.instance.otp_secret)
+
+        if not totp.verify(otp):
+            # maybe it's a backup code?
+            hotp = pyotp.HOTP(self.instance.otp_secret)
+            hotp_count = (
+                self.instance.hotp_count if self.instance.hotp_count is not None else 0
+            )
+
+            if not hotp.verify(otp, hotp_count):
+                self.add_error("otp", _("Code does not match"))
+
+            # TODO: backup codes
+            # increment the user hotp_count if it was an HOTP
+            # self.instance.hotp_count = hotp_count + 1
+            # self.instance.save(broadcast=False, update_fields=["hotp_count"])

bookwyrm/templates/two_factor_auth/two_factor_login.html

@@ -30,23 +30,14 @@
                     </h1>
                     {% endblock %}
                 </header>
-                {% if error %}
-                <div class="notification is-danger is-light">
-                    <!-- TODO: how do we translate dynamic errors? -->
-                    <span>
-                        {{ error }}
-                    </span>
-                </div>
-                {% endif %}
                 <div class="is-centered">
                     <form name="confirm-2fa" action="{% url 'login-with-2fa' %}" method="post" enctype="multipart/form-data">
                         {% csrf_token %}
                         <div class="field">
                             <label class="label" for="id_otp">{% trans "Enter the code from your authenticator app:" %}</label>
                             {{ form.otp }}
-                            {% include 'snippets/form_errors.html' with errors_list=form.current_password.errors id="desc_current_password" %}
+                            {% include 'snippets/form_errors.html' with errors_list=form.otp.errors id="desc_otp" %}
                         </div>
-                        <input name="2fa_user" value="{{ 2fa_user }}" hidden>
                         <button class="button is-primary" type="submit">{% trans "Confirm and Log In" %}</button>
                     </form>
                 </div>

bookwyrm/views/landing/login.py

@@ -1,4 +1,5 @@
 """ class views for login/register views """
+from datetime import datetime
 from django.contrib.auth import authenticate, login, logout
 from django.contrib.auth.decorators import login_required
 from django.shortcuts import redirect
@@ -29,7 +30,7 @@ class Login(View):
         }
         return TemplateResponse(request, "landing/login.html", data)
 
-    #pylint: disable=too-many-return-statements
+    # pylint: disable=too-many-return-statements
     @sensitive_variables("password")
     @method_decorator(sensitive_post_parameters("password"))
     def post(self, request):
@@ -54,12 +55,9 @@ class Login(View):
         if user is not None:
             # if 2fa is set, don't log them in until they enter the right code
             if user.two_factor_auth is True:
-                form = forms.Confirm2FAForm(request.GET, user)
+                request.session["2fa_user"] = user.username
-                return TemplateResponse(
+                request.session["2fa_auth_time"] = datetime.now()
-                    request,
+                return redirect("login-with-2fa")
-                    "two_factor_auth/two_factor_login.html",
-                    {"form": form, "2fa_user": user},
-                )
 
             # otherwise, successful login
             login(request, user)

bookwyrm/views/preferences/two_factor_auth.py

@@ -1,4 +1,5 @@
 """ class views for 2FA management """
+from datetime import datetime, timedelta
 import time
 import pyotp
 import qrcode
@@ -94,17 +95,31 @@ class Disable2FA(View):
 class LoginWith2FA(View):
     """Check 2FA code matches before allowing login"""
 
+    def get(self, request):
+        """Display 2FA form"""
+
+        data = {"form": forms.Confirm2FAForm()}
+        return TemplateResponse(request, "two_factor_auth/two_factor_login.html", data)
+
     def post(self, request):
         """Check 2FA code and allow/disallow login"""
-        user = models.User.objects.get(username=request.POST.get("2fa_user"))
+        user = models.User.objects.get(username=request.session["2fa_user"])
+        elapsed_time = datetime.now() - request.session["2fa_auth_time"]
         form = forms.Confirm2FAForm(request.POST, instance=user)
+        # don't allow the login credentials to last too long before completing login
+        if elapsed_time > timedelta(seconds=60):
+            request.session["2fa_user"] = None
+            request.session["2fa_auth_time"] = 0
+            return redirect("/")
         if not form.is_valid():
-            time.sleep(2)  # make life slightly harder for bots
+            # make life harder for bots
-            data = {
+            # humans are unlikely to get it wrong more than twice
-                "form": form,
+            if not request.session["2fa_attempts"]:
-                "2fa_user": user,
+                request.session["2fa_attempts"] = 0
-                "error": "Code does not match, try again",
+            request.session["2fa_attempts"] = request.session["2fa_attempts"] + 1
-            }
+            time.sleep(2 ** request.session["2fa_attempts"])
+
+            data = {"form": form, "2fa_user": user}
             return TemplateResponse(
                 request, "two_factor_auth/two_factor_login.html", data
             )