From 54daade9f9b00f357b108392de93c7bf562a2210 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 13:48:52 +1000 Subject: [PATCH 01/31] prepare for 2FA - add and migrate User fields for 2FA - add views for 2FA - add new forms for 2FA - update package list in requirements.txt - add URLs for 2FA views --- bookwyrm/forms/edit_user.py | 35 +++++++ .../migrations/0158_auto_20220911_0008.py | 28 ++++++ bookwyrm/models/user.py | 5 + bookwyrm/urls.py | 15 +++ bookwyrm/views/__init__.py | 1 + bookwyrm/views/preferences/two_factor_auth.py | 93 +++++++++++++++++++ requirements.txt | 2 + 7 files changed, 179 insertions(+) create mode 100644 bookwyrm/migrations/0158_auto_20220911_0008.py create mode 100644 bookwyrm/views/preferences/two_factor_auth.py diff --git a/bookwyrm/forms/edit_user.py b/bookwyrm/forms/edit_user.py index a291c6441..a7effaf08 100644 --- a/bookwyrm/forms/edit_user.py +++ b/bookwyrm/forms/edit_user.py @@ -8,6 +8,7 @@ from bookwyrm import models from bookwyrm.models.fields import ClearableFileInputWithWarning from .custom_form import CustomForm +import pyotp # pylint: disable=missing-class-docstring class EditUserForm(CustomForm): @@ -99,3 +100,37 @@ class ChangePasswordForm(CustomForm): validate_password(new_password) except ValidationError as err: self.add_error("password", err) + + +class ConfirmPasswordForm(CustomForm): + password = forms.CharField(widget=forms.PasswordInput) + + class Meta: + model = models.User + fields = ["password"] + widgets = { + "password": forms.PasswordInput(), + } + + def clean(self): + """Make sure password is correct""" + password = self.data.get("password") + + 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) + + 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): + self.add_error("otp", _("Code does not match")) diff --git a/bookwyrm/migrations/0158_auto_20220911_0008.py b/bookwyrm/migrations/0158_auto_20220911_0008.py new file mode 100644 index 000000000..d5a98d94a --- /dev/null +++ b/bookwyrm/migrations/0158_auto_20220911_0008.py @@ -0,0 +1,28 @@ +# Generated by Django 3.2.15 on 2022-09-11 00:08 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("bookwyrm", "0157_auto_20220909_2338"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="htop_count", + field=models.IntegerField(blank=True, default=0, null=True), + ), + migrations.AddField( + model_name="user", + name="otp_secret", + field=models.CharField(blank=True, default=None, max_length=32, null=True), + ), + migrations.AddField( + model_name="user", + name="two_factor_auth", + field=models.BooleanField(blank=True, default=None, null=True), + ), + ] diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 055941d8c..8703d01fc 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -175,6 +175,11 @@ class User(OrderedCollectionPageMixin, AbstractUser): property_fields = [("following_link", "following")] field_tracker = FieldTracker(fields=["name", "avatar"]) + # two factor authentication + two_factor_auth = models.BooleanField(default=None, blank=True, null=True) + otp_secret = models.CharField(max_length=32, default=None, blank=True, null=True) + htop_count = models.IntegerField(default=0, blank=True, null=True) + @property def active_follower_requests(self): """Follow requests from active users""" diff --git a/bookwyrm/urls.py b/bookwyrm/urls.py index 449b1d723..7b6e36cde 100644 --- a/bookwyrm/urls.py +++ b/bookwyrm/urls.py @@ -479,6 +479,21 @@ urlpatterns = [ views.ChangePassword.as_view(), name="prefs-password", ), + re_path( + r"^preferences/2fa/?$", + views.Edit2FA.as_view(), + name="prefs-2fa", + ), + re_path( + r"^preferences/confirm-2fa/?$", + views.Confirm2FA.as_view(), + name="conf-2fa", + ), + re_path( + r"^preferences/disable-2fa/?$", + views.Disable2FA.as_view(), + name="disable-2fa", + ), re_path(r"^preferences/export/?$", views.Export.as_view(), name="prefs-export"), re_path(r"^preferences/delete/?$", views.DeleteUser.as_view(), name="prefs-delete"), re_path(r"^preferences/block/?$", views.Block.as_view(), name="prefs-block"), diff --git a/bookwyrm/views/__init__.py b/bookwyrm/views/__init__.py index 257adc932..1b9d74181 100644 --- a/bookwyrm/views/__init__.py +++ b/bookwyrm/views/__init__.py @@ -32,6 +32,7 @@ from .preferences.edit_user import EditUser from .preferences.export import Export from .preferences.delete_user import DeleteUser from .preferences.block import Block, unblock +from .preferences.two_factor_auth import Edit2FA, Confirm2FA, Disable2FA # books from .books.books import ( diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py new file mode 100644 index 000000000..2441bae97 --- /dev/null +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -0,0 +1,93 @@ +""" class views for 2FA management """ +import base64 +import io +from pipes import Template +from turtle import fillcolor +import pyotp +import qrcode +import qrcode.image.svg + +from django.contrib.auth import login +from django.contrib.auth.decorators import login_required +from django.template.response import TemplateResponse +from django.shortcuts import redirect +from django.utils.decorators import method_decorator +from django.views import View +from django.views.decorators.debug import sensitive_variables, sensitive_post_parameters + +from bookwyrm import forms +from bookwyrm.settings import DOMAIN + +# pylint: disable= no-self-use +@method_decorator(login_required, name="dispatch") +class Edit2FA(View): + """change 2FA settings as logged in user""" + + def get(self, request): + """Two Factor auth page""" + data = {"form": forms.ConfirmPasswordForm()} + return TemplateResponse(request, "preferences/2fa.html", data) + + @method_decorator(sensitive_post_parameters("password")) + def post(self, request): + """check the user's password""" + form = forms.ConfirmPasswordForm(request.POST, instance=request.user) + # TODO: display an error + if not form.is_valid(): + data = {"form": form} + return TemplateResponse(request, "preferences/2fa.html", data) + qr_form = forms.Confirm2FAForm(request.POST, instance=request.user) + data = { + "password_confirmed": True, + "qrcode": self.create_qr_code(request.user), + "form": qr_form, + } + return TemplateResponse(request, "preferences/2fa.html", data) + + def create_qr_code(self, user): + """generate and save a qr code for 2FA""" + otp_secret = pyotp.random_base32() + # save the secret to the user record - we'll need it to check codes in future + user.otp_secret = otp_secret + user.save(broadcast=False, update_fields=["otp_secret"]) + # now we create the qr code + provisioning_url = pyotp.totp.TOTP(otp_secret).provisioning_uri( + name=user.localname, issuer_name=DOMAIN + ) + qr = qrcode.QRCode(image_factory=qrcode.image.svg.SvgPathImage) + qr.add_data(provisioning_url) + qr.make(fit=True) + img = qr.make_image(attrib={"fill": "black", "background": "white"}) + return img.to_string() + + +class Confirm2FA(View): + """confirm user's 2FA settings""" + + def post(self, request): + """confirm the 2FA works before requiring it""" + form = forms.Confirm2FAForm(request.POST, instance=request.user) + # TODO: show an error here + if not form.is_valid(): + data = {"form": form} + return redirect("prefs-2fa") + # set the user's 2FA setting on + request.user.two_factor_auth = True + request.user.save(broadcast=False, update_fields=["two_factor_auth"]) + data = {"form": form, "success": True} + return TemplateResponse(request, "preferences/2fa.html", data) + + +class Disable2FA(View): + """Turn off 2FA on this user account""" + + def get(self, request): + """Confirmation page to turn off 2FA""" + return TemplateResponse(request, "preferences/disable-2fa.html") + + def post(self, request): + """Turn off 2FA on this user account""" + request.user.two_factor_auth = False + request.user.save(broadcast=False, update_fields=["two_factor_auth"]) + data = {"form": forms.ConfirmPasswordForm(), "success": True} + return TemplateResponse(request, "preferences/2fa.html", data) diff --git a/requirements.txt b/requirements.txt index 03778264c..dc9df703f 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,6 +29,8 @@ opentelemetry-instrumentation-celery==0.30b1 opentelemetry-instrumentation-django==0.30b1 opentelemetry-sdk==1.11.1 protobuf==3.20.* +pyotp==2.6.0 +qrcode==7.3.1 # Dev pytest-django==4.1.0 From aca5c19f706fb02c2310eda7910b2987280a41b8 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 13:51:59 +1000 Subject: [PATCH 02/31] 2fa templates - new page templates for 2FA - add 2FA to menu in user preferences --- bookwyrm/templates/preferences/2fa.html | 55 +++++++++++++++++++ .../templates/preferences/disable-2fa.html | 23 ++++++++ bookwyrm/templates/preferences/layout.html | 4 ++ 3 files changed, 82 insertions(+) create mode 100644 bookwyrm/templates/preferences/2fa.html create mode 100644 bookwyrm/templates/preferences/disable-2fa.html diff --git a/bookwyrm/templates/preferences/2fa.html b/bookwyrm/templates/preferences/2fa.html new file mode 100644 index 000000000..798716ac4 --- /dev/null +++ b/bookwyrm/templates/preferences/2fa.html @@ -0,0 +1,55 @@ +{% extends 'preferences/layout.html' %} +{% load i18n %} + +{% block title %}{% trans "Two Factor Authentication" %}{% endblock %} + +{% block header %} +{% trans "Two Factor Authentication" %} +{% endblock %} + +{% block panel %} +
+ {% if success %} +
+ + + {% trans "Successfully updated 2FA settings" %} + +
+ {% endif %} + {% if request.user.two_factor_auth %} +

Two Factor Authentication is active on your account.

+ {% trans "Disable 2FA" %} + {% elif password_confirmed %} +
+ {% csrf_token %} +

Scan the QR code with your authentication app and then enter the code from your app below to confirm your app is set up.

+
+ + {{ qrcode | safe }} + +
+
+ + {{ form.otp }} + {% include 'snippets/form_errors.html' with errors_list=form.current_password.errors id="desc_current_password" %} +
+ +
+ {% else %} +

+ {% trans "You can make your account more secure by using Two Factor Authentication (2FA). This will require you to enter a one-time code using a phone app like Authy, Google Authenticator or Microsoft Authenticator each time you log in." %} +

+

{% trans "Confirm your password to begin setting up 2FA." %}

+
+ {% csrf_token %} +
+ + {{ form.password }} + {% include 'snippets/form_errors.html' with errors_list=form.current_password.errors id="desc_current_password" %} +
+ +
+ {% endif %} +
+{% endblock %} diff --git a/bookwyrm/templates/preferences/disable-2fa.html b/bookwyrm/templates/preferences/disable-2fa.html new file mode 100644 index 000000000..dbb980852 --- /dev/null +++ b/bookwyrm/templates/preferences/disable-2fa.html @@ -0,0 +1,23 @@ +{% extends 'preferences/layout.html' %} +{% load i18n %} + +{% block title %}{% trans "Disable 2FA" %}{% endblock %} + +{% block header %} +{% trans "Disable 2FA" %} +{% endblock %} + +{% block panel %} +
+

{% trans "Disable Two Factor Authentication" %}

+

+ {% trans "Disabling 2FA will allow anyone with your username and password to log in to your account." %} +

+ +
+ {% csrf_token %} + {% trans "Cancel" %} + +
+
+{% endblock %} diff --git a/bookwyrm/templates/preferences/layout.html b/bookwyrm/templates/preferences/layout.html index 27d91c480..ca63ec93d 100644 --- a/bookwyrm/templates/preferences/layout.html +++ b/bookwyrm/templates/preferences/layout.html @@ -19,6 +19,10 @@ {% url 'prefs-password' as url %} {% trans "Change Password" %} +
  • + {% url 'prefs-2fa' as url %} + {% trans "Two Factor Authentication" %} +
  • {% url 'prefs-delete' as url %} {% trans "Delete Account" %} From 514762c233e0c8ffc1df1453b524727a94755688 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 16:24:42 +1000 Subject: [PATCH 03/31] fix typo in new user fields oopsie --- ...{0158_auto_20220911_0008.py => 0158_auto_20220911_0525.py} | 4 ++-- bookwyrm/models/user.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) rename bookwyrm/migrations/{0158_auto_20220911_0008.py => 0158_auto_20220911_0525.py} (89%) diff --git a/bookwyrm/migrations/0158_auto_20220911_0008.py b/bookwyrm/migrations/0158_auto_20220911_0525.py similarity index 89% rename from bookwyrm/migrations/0158_auto_20220911_0008.py rename to bookwyrm/migrations/0158_auto_20220911_0525.py index d5a98d94a..764c6172f 100644 --- a/bookwyrm/migrations/0158_auto_20220911_0008.py +++ b/bookwyrm/migrations/0158_auto_20220911_0525.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.15 on 2022-09-11 00:08 +# Generated by Django 3.2.15 on 2022-09-11 05:25 from django.db import migrations, models @@ -12,7 +12,7 @@ class Migration(migrations.Migration): operations = [ migrations.AddField( model_name="user", - name="htop_count", + name="hotp_count", field=models.IntegerField(blank=True, default=0, null=True), ), migrations.AddField( diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 8703d01fc..87d7d5207 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -178,7 +178,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): # two factor authentication two_factor_auth = models.BooleanField(default=None, blank=True, null=True) otp_secret = models.CharField(max_length=32, default=None, blank=True, null=True) - htop_count = models.IntegerField(default=0, blank=True, null=True) + hotp_count = models.IntegerField(default=0, blank=True, null=True) @property def active_follower_requests(self): From 0e1751eb57a5add94acca6564c3d36100820986c Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 16:38:15 +1000 Subject: [PATCH 04/31] prep for 2fa login check - new 2fa checker page to be inserted between initial login and completion of login - new views and forms for above --- bookwyrm/forms/edit_user.py | 15 ++- bookwyrm/templates/preferences/2fa.html | 2 +- bookwyrm/templates/two_factor_login.html | 104 ++++++++++++++++++ bookwyrm/urls.py | 5 + bookwyrm/views/__init__.py | 2 +- bookwyrm/views/preferences/two_factor_auth.py | 28 ++++- 6 files changed, 148 insertions(+), 8 deletions(-) create mode 100644 bookwyrm/templates/two_factor_login.html diff --git a/bookwyrm/forms/edit_user.py b/bookwyrm/forms/edit_user.py index a7effaf08..51be68a51 100644 --- a/bookwyrm/forms/edit_user.py +++ b/bookwyrm/forms/edit_user.py @@ -123,6 +123,7 @@ class ConfirmPasswordForm(CustomForm): 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"] @@ -133,4 +134,16 @@ class Confirm2FAForm(CustomForm): totp = pyotp.TOTP(self.instance.otp_secret) if not totp.verify(otp): - self.add_error("otp", _("Code does not match")) + # 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"]) diff --git a/bookwyrm/templates/preferences/2fa.html b/bookwyrm/templates/preferences/2fa.html index 798716ac4..e5e9684ce 100644 --- a/bookwyrm/templates/preferences/2fa.html +++ b/bookwyrm/templates/preferences/2fa.html @@ -29,7 +29,7 @@ {{ qrcode | safe }} -
    +
    {{ form.otp }} {% include 'snippets/form_errors.html' with errors_list=form.current_password.errors id="desc_current_password" %} diff --git a/bookwyrm/templates/two_factor_login.html b/bookwyrm/templates/two_factor_login.html new file mode 100644 index 000000000..7d31a714b --- /dev/null +++ b/bookwyrm/templates/two_factor_login.html @@ -0,0 +1,104 @@ +{% load layout %} +{% load sass_tags %} +{% load i18n %} +{% load static %} + + + + + {% block title %}BookWyrm{% endblock %} - {{ site.name }} + + + + + +
    +
    +
    +
    + {% block header %} +

    + {% trans "2FA check" %} +

    + {% endblock %} +
    + {% if error %} +
    + + + {{ error }} + +
    + {% endif %} +
    +
    + {% csrf_token %} +
    + + {{ form.otp }} + {% include 'snippets/form_errors.html' with errors_list=form.current_password.errors id="desc_current_password" %} +
    + +
    +
    +
    +
    +
    + + + + diff --git a/bookwyrm/urls.py b/bookwyrm/urls.py index 7b6e36cde..4e09cb550 100644 --- a/bookwyrm/urls.py +++ b/bookwyrm/urls.py @@ -494,6 +494,11 @@ urlpatterns = [ views.Disable2FA.as_view(), name="disable-2fa", ), + re_path( + r"^login-2FA-check/?$", + views.LoginWith2FA.as_view(), + name="login-with-2fa", + ), re_path(r"^preferences/export/?$", views.Export.as_view(), name="prefs-export"), re_path(r"^preferences/delete/?$", views.DeleteUser.as_view(), name="prefs-delete"), re_path(r"^preferences/block/?$", views.Block.as_view(), name="prefs-block"), diff --git a/bookwyrm/views/__init__.py b/bookwyrm/views/__init__.py index 1b9d74181..9c0c7adea 100644 --- a/bookwyrm/views/__init__.py +++ b/bookwyrm/views/__init__.py @@ -32,7 +32,7 @@ from .preferences.edit_user import EditUser from .preferences.export import Export from .preferences.delete_user import DeleteUser from .preferences.block import Block, unblock -from .preferences.two_factor_auth import Edit2FA, Confirm2FA, Disable2FA +from .preferences.two_factor_auth import Edit2FA, Confirm2FA, Disable2FA, LoginWith2FA # books from .books.books import ( diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 2441bae97..bbe7628e2 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -1,11 +1,8 @@ """ class views for 2FA management """ -import base64 -import io -from pipes import Template -from turtle import fillcolor import pyotp import qrcode import qrcode.image.svg +import time from django.contrib.auth import login from django.contrib.auth.decorators import login_required @@ -13,7 +10,7 @@ from django.template.response import TemplateResponse from django.shortcuts import redirect from django.utils.decorators import method_decorator from django.views import View -from django.views.decorators.debug import sensitive_variables, sensitive_post_parameters +from django.views.decorators.debug import sensitive_post_parameters from bookwyrm import forms from bookwyrm.settings import DOMAIN @@ -91,3 +88,24 @@ class Disable2FA(View): request.user.save(broadcast=False, update_fields=["two_factor_auth"]) data = {"form": forms.ConfirmPasswordForm(), "success": True} return TemplateResponse(request, "preferences/2fa.html", data) + + +class LoginWith2FA(View): + """Check 2FA code matches before allowing login""" + + def get(self, request): + """Load 2FA checking page""" + form = forms.Confirm2FAForm(request.GET, instance=request.user) + return TemplateResponse(request, "two_factor_login.html", {"form": form}) + + def post(self, request): + """Check 2FA code and allow/disallow login""" + form = forms.Confirm2FAForm(request.POST, instance=request.user) + + if not form.is_valid(): + time.sleep(2) # make life harder for bots + data = {"form": form, "error": "Code does not match, try again"} + return TemplateResponse(request, "two_factor_login.html", data) + + # TODO: actually log the user in - we will be bypassing normal login + return redirect("/") From f26ac1ccde027faf26778cff46cf9223112e0cb7 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 20:56:30 +1000 Subject: [PATCH 05/31] 2fa page templates --- bookwyrm/templates/preferences/2fa.html | 2 +- .../two_factor_login.html | 1 + .../two_factor_auth/two_factor_prompt.html | 92 +++++++++++++++++++ 3 files changed, 94 insertions(+), 1 deletion(-) rename bookwyrm/templates/{ => two_factor_auth}/two_factor_login.html (98%) create mode 100644 bookwyrm/templates/two_factor_auth/two_factor_prompt.html diff --git a/bookwyrm/templates/preferences/2fa.html b/bookwyrm/templates/preferences/2fa.html index e5e9684ce..00553b16c 100644 --- a/bookwyrm/templates/preferences/2fa.html +++ b/bookwyrm/templates/preferences/2fa.html @@ -25,7 +25,7 @@ {% csrf_token %}

    Scan the QR code with your authentication app and then enter the code from your app below to confirm your app is set up.

    - + {{ qrcode | safe }}
    diff --git a/bookwyrm/templates/two_factor_login.html b/bookwyrm/templates/two_factor_auth/two_factor_login.html similarity index 98% rename from bookwyrm/templates/two_factor_login.html rename to bookwyrm/templates/two_factor_auth/two_factor_login.html index 7d31a714b..b110b82d5 100644 --- a/bookwyrm/templates/two_factor_login.html +++ b/bookwyrm/templates/two_factor_auth/two_factor_login.html @@ -46,6 +46,7 @@ {{ form.otp }} {% include 'snippets/form_errors.html' with errors_list=form.current_password.errors id="desc_current_password" %}
    +
    diff --git a/bookwyrm/templates/two_factor_auth/two_factor_prompt.html b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html new file mode 100644 index 000000000..650b429c3 --- /dev/null +++ b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html @@ -0,0 +1,92 @@ +{% load layout %} +{% load sass_tags %} +{% load i18n %} +{% load static %} + + + + + {% block title %}BookWyrm{% endblock %} - {{ site.name }} + + + + + +
    +
    +
    +
    + {% block header %} +

    + {% trans "2FA is available" %} +

    + {% endblock %} +
    +
    +

    You can secure your account by setting up two factor authentication in your user preferences. This will require a one-time code from your phone in addition to your password each time you log in.

    +
    + No thanks + Set up 2FA +
    +
    +
    +
    +
    + + + + From 2ec343c5db4c97467051964701af902d3e0e1f97 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 20:58:11 +1000 Subject: [PATCH 06/31] new views for capturing user for 2fa check --- bookwyrm/urls.py | 5 +++ bookwyrm/views/__init__.py | 2 +- bookwyrm/views/preferences/two_factor_auth.py | 32 +++++++++++-------- 3 files changed, 24 insertions(+), 15 deletions(-) diff --git a/bookwyrm/urls.py b/bookwyrm/urls.py index 4e09cb550..040fed727 100644 --- a/bookwyrm/urls.py +++ b/bookwyrm/urls.py @@ -499,6 +499,11 @@ urlpatterns = [ views.LoginWith2FA.as_view(), name="login-with-2fa", ), + re_path( + r"^login-2FA-prompt/?$", + views.Prompt2FA.as_view(), + name="prompt-2fa", + ), re_path(r"^preferences/export/?$", views.Export.as_view(), name="prefs-export"), re_path(r"^preferences/delete/?$", views.DeleteUser.as_view(), name="prefs-delete"), re_path(r"^preferences/block/?$", views.Block.as_view(), name="prefs-block"), diff --git a/bookwyrm/views/__init__.py b/bookwyrm/views/__init__.py index 9c0c7adea..52456ccc9 100644 --- a/bookwyrm/views/__init__.py +++ b/bookwyrm/views/__init__.py @@ -32,7 +32,7 @@ from .preferences.edit_user import EditUser from .preferences.export import Export from .preferences.delete_user import DeleteUser from .preferences.block import Block, unblock -from .preferences.two_factor_auth import Edit2FA, Confirm2FA, Disable2FA, LoginWith2FA +from .preferences.two_factor_auth import Edit2FA, Confirm2FA, Disable2FA, LoginWith2FA, Prompt2FA # books from .books.books import ( diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index bbe7628e2..b98643abe 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -12,8 +12,9 @@ from django.utils.decorators import method_decorator from django.views import View from django.views.decorators.debug import sensitive_post_parameters -from bookwyrm import forms +from bookwyrm import forms, models from bookwyrm.settings import DOMAIN +from bookwyrm.views.helpers import set_language # pylint: disable= no-self-use @method_decorator(login_required, name="dispatch") @@ -54,7 +55,7 @@ class Edit2FA(View): qr = qrcode.QRCode(image_factory=qrcode.image.svg.SvgPathImage) qr.add_data(provisioning_url) qr.make(fit=True) - img = qr.make_image(attrib={"fill": "black", "background": "white"}) + img = qr.make_image(attrib={"fill": "black"}) return img.to_string() @@ -93,19 +94,22 @@ class Disable2FA(View): class LoginWith2FA(View): """Check 2FA code matches before allowing login""" - def get(self, request): - """Load 2FA checking page""" - form = forms.Confirm2FAForm(request.GET, instance=request.user) - return TemplateResponse(request, "two_factor_login.html", {"form": form}) - def post(self, request): """Check 2FA code and allow/disallow login""" - form = forms.Confirm2FAForm(request.POST, instance=request.user) - + user = models.User.objects.get(username=request.POST.get('2fa_user')) + form = forms.Confirm2FAForm(request.POST, instance=user) if not form.is_valid(): - time.sleep(2) # make life harder for bots - data = {"form": form, "error": "Code does not match, try again"} - return TemplateResponse(request, "two_factor_login.html", data) + time.sleep(2) # make life slightly harder for bots + data = {"form": form, "2fa_user": user, "error": "Code does not match, try again"} + return TemplateResponse(request, "two_factor_auth/two_factor_login.html", data) - # TODO: actually log the user in - we will be bypassing normal login - return redirect("/") + # log the user in - we are bypassing standard login + login(request, user) + user.update_active_date() + return set_language(user, redirect("/")) + +class Prompt2FA(View): + """Alert user to the existence of 2FA""" + + def get(self, request): + return TemplateResponse(request, "two_factor_auth/two_factor_prompt.html") From 8837495ffd13b806051e0049b1bf32d924cbda85 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 21:00:01 +1000 Subject: [PATCH 07/31] redirect login to 2fa check if active --- bookwyrm/views/landing/login.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/bookwyrm/views/landing/login.py b/bookwyrm/views/landing/login.py index 98a2b6e61..0c8d596d6 100644 --- a/bookwyrm/views/landing/login.py +++ b/bookwyrm/views/landing/login.py @@ -51,11 +51,25 @@ class Login(View): # perform authentication user = authenticate(request, username=username, password=password) if user is not None: - # successful login + # if 2fa is set, don't log them in until they enter the right code + if user.two_factor_auth == True: + form = forms.Confirm2FAForm(request.GET, user) + return TemplateResponse(request, "two_factor_auth/two_factor_login.html", {"form": form, "2fa_user": user}) + + # otherwise, successful login login(request, user) user.update_active_date() if request.POST.get("first_login"): return set_language(user, redirect("get-started-profile")) + + if user.two_factor_auth == None: + # set to false so this page doesn't pop up again + user.two_factor_auth = False + user.save(broadcast=False, update_fields=["two_factor_auth"]) + + # show the 2fa prompt page + return set_language(user, redirect("prompt-2fa")) + return set_language(user, redirect("/")) # maybe the user is pending email confirmation @@ -70,7 +84,6 @@ class Login(View): data = {"login_form": login_form, "register_form": register_form} return TemplateResponse(request, "landing/login.html", data) - @method_decorator(login_required, name="dispatch") class Logout(View): """log out""" From 1d13f0ab4f5e18cb206878edad9a8976c0d75615 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 21:03:23 +1000 Subject: [PATCH 08/31] lint --- bookwyrm/views/__init__.py | 8 +++++++- bookwyrm/views/landing/login.py | 7 ++++++- bookwyrm/views/preferences/two_factor_auth.py | 13 ++++++++++--- 3 files changed, 23 insertions(+), 5 deletions(-) diff --git a/bookwyrm/views/__init__.py b/bookwyrm/views/__init__.py index 52456ccc9..98bca4bf7 100644 --- a/bookwyrm/views/__init__.py +++ b/bookwyrm/views/__init__.py @@ -32,7 +32,13 @@ from .preferences.edit_user import EditUser from .preferences.export import Export from .preferences.delete_user import DeleteUser from .preferences.block import Block, unblock -from .preferences.two_factor_auth import Edit2FA, Confirm2FA, Disable2FA, LoginWith2FA, Prompt2FA +from .preferences.two_factor_auth import ( + Edit2FA, + Confirm2FA, + Disable2FA, + LoginWith2FA, + Prompt2FA, +) # books from .books.books import ( diff --git a/bookwyrm/views/landing/login.py b/bookwyrm/views/landing/login.py index 0c8d596d6..6feb1d5d0 100644 --- a/bookwyrm/views/landing/login.py +++ b/bookwyrm/views/landing/login.py @@ -54,7 +54,11 @@ class Login(View): # if 2fa is set, don't log them in until they enter the right code if user.two_factor_auth == True: form = forms.Confirm2FAForm(request.GET, user) - return TemplateResponse(request, "two_factor_auth/two_factor_login.html", {"form": form, "2fa_user": user}) + return TemplateResponse( + request, + "two_factor_auth/two_factor_login.html", + {"form": form, "2fa_user": user}, + ) # otherwise, successful login login(request, user) @@ -84,6 +88,7 @@ class Login(View): data = {"login_form": login_form, "register_form": register_form} return TemplateResponse(request, "landing/login.html", data) + @method_decorator(login_required, name="dispatch") class Logout(View): """log out""" diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index b98643abe..17cec5290 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -96,18 +96,25 @@ class LoginWith2FA(View): 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.POST.get("2fa_user")) form = forms.Confirm2FAForm(request.POST, instance=user) if not form.is_valid(): time.sleep(2) # make life slightly harder for bots - data = {"form": form, "2fa_user": user, "error": "Code does not match, try again"} - return TemplateResponse(request, "two_factor_auth/two_factor_login.html", data) + data = { + "form": form, + "2fa_user": user, + "error": "Code does not match, try again", + } + return TemplateResponse( + request, "two_factor_auth/two_factor_login.html", data + ) # log the user in - we are bypassing standard login login(request, user) user.update_active_date() return set_language(user, redirect("/")) + class Prompt2FA(View): """Alert user to the existence of 2FA""" From 9d12b7caff54ea1d4b24640a65f37adf67d24b1a Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 11 Sep 2022 21:36:30 +1000 Subject: [PATCH 09/31] make pylint stop grumbling --- bookwyrm/forms/edit_user.py | 4 ++-- bookwyrm/views/landing/login.py | 5 +++-- bookwyrm/views/preferences/two_factor_auth.py | 11 ++++++----- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/bookwyrm/forms/edit_user.py b/bookwyrm/forms/edit_user.py index 51be68a51..d86aed31d 100644 --- a/bookwyrm/forms/edit_user.py +++ b/bookwyrm/forms/edit_user.py @@ -4,12 +4,12 @@ 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 -import pyotp - # pylint: disable=missing-class-docstring class EditUserForm(CustomForm): class Meta: diff --git a/bookwyrm/views/landing/login.py b/bookwyrm/views/landing/login.py index 6feb1d5d0..364b721d2 100644 --- a/bookwyrm/views/landing/login.py +++ b/bookwyrm/views/landing/login.py @@ -29,6 +29,7 @@ class Login(View): } return TemplateResponse(request, "landing/login.html", data) + #pylint: disable=too-many-return-statements @sensitive_variables("password") @method_decorator(sensitive_post_parameters("password")) def post(self, request): @@ -52,7 +53,7 @@ class Login(View): user = authenticate(request, username=username, password=password) 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 == True: + if user.two_factor_auth is True: form = forms.Confirm2FAForm(request.GET, user) return TemplateResponse( request, @@ -66,7 +67,7 @@ class Login(View): if request.POST.get("first_login"): return set_language(user, redirect("get-started-profile")) - if user.two_factor_auth == None: + if user.two_factor_auth is None: # set to false so this page doesn't pop up again user.two_factor_auth = False user.save(broadcast=False, update_fields=["two_factor_auth"]) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 17cec5290..26cb87f21 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -1,8 +1,8 @@ """ class views for 2FA management """ +import time import pyotp import qrcode import qrcode.image.svg -import time from django.contrib.auth import login from django.contrib.auth.decorators import login_required @@ -52,10 +52,10 @@ class Edit2FA(View): provisioning_url = pyotp.totp.TOTP(otp_secret).provisioning_uri( name=user.localname, issuer_name=DOMAIN ) - qr = qrcode.QRCode(image_factory=qrcode.image.svg.SvgPathImage) - qr.add_data(provisioning_url) - qr.make(fit=True) - img = qr.make_image(attrib={"fill": "black"}) + qr_code = qrcode.QRCode(image_factory=qrcode.image.svg.SvgPathImage) + qr_code.add_data(provisioning_url) + qr_code.make(fit=True) + img = qr_code.make_image(attrib={"fill": "black"}) return img.to_string() @@ -119,4 +119,5 @@ class Prompt2FA(View): """Alert user to the existence of 2FA""" def get(self, request): + """Alert user to the existence of 2FA""" return TemplateResponse(request, "two_factor_auth/two_factor_prompt.html") From 6db4fb39ed4fc1ecc55b4f89ad9cbedb246513b2 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 18 Sep 2022 16:32:42 +1000 Subject: [PATCH 10/31] 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 | 31 ------------------- bookwyrm/forms/landing.py | 30 ++++++++++++++++++ .../two_factor_auth/two_factor_login.html | 11 +------ bookwyrm/views/landing/login.py | 12 +++---- bookwyrm/views/preferences/two_factor_auth.py | 29 ++++++++++++----- 5 files changed, 58 insertions(+), 55 deletions(-) diff --git a/bookwyrm/forms/edit_user.py b/bookwyrm/forms/edit_user.py index d86aed31d..ce7bb6d07 100644 --- a/bookwyrm/forms/edit_user.py +++ b/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"]) diff --git a/bookwyrm/forms/landing.py b/bookwyrm/forms/landing.py index a31e8a7c4..ba7f69bb6 100644 --- a/bookwyrm/forms/landing.py +++ b/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"]) diff --git a/bookwyrm/templates/two_factor_auth/two_factor_login.html b/bookwyrm/templates/two_factor_auth/two_factor_login.html index b110b82d5..f4a2fdf04 100644 --- a/bookwyrm/templates/two_factor_auth/two_factor_login.html +++ b/bookwyrm/templates/two_factor_auth/two_factor_login.html @@ -30,23 +30,14 @@ {% endblock %} - {% if error %} -
    - - - {{ error }} - -
    - {% endif %}
    {% csrf_token %}
    {{ 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" %}
    -
    diff --git a/bookwyrm/views/landing/login.py b/bookwyrm/views/landing/login.py index 364b721d2..b9a877687 100644 --- a/bookwyrm/views/landing/login.py +++ b/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) - return TemplateResponse( - request, - "two_factor_auth/two_factor_login.html", - {"form": form, "2fa_user": user}, - ) + request.session["2fa_user"] = user.username + request.session["2fa_auth_time"] = datetime.now() + return redirect("login-with-2fa") # otherwise, successful login login(request, user) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 26cb87f21..2a1656092 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/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 - data = { - "form": form, - "2fa_user": user, - "error": "Code does not match, try again", - } + # make life harder for bots + # humans are unlikely to get it wrong more than twice + if not request.session["2fa_attempts"]: + request.session["2fa_attempts"] = 0 + 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 ) From 5b244f06d671c80808576cd50a47bae00cb8ffd6 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 18 Sep 2022 17:10:51 +1000 Subject: [PATCH 11/31] fix error messages when setting up 2FA --- bookwyrm/templates/preferences/2fa.html | 4 ++-- bookwyrm/views/preferences/two_factor_auth.py | 14 +++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/bookwyrm/templates/preferences/2fa.html b/bookwyrm/templates/preferences/2fa.html index 00553b16c..a2b66aea0 100644 --- a/bookwyrm/templates/preferences/2fa.html +++ b/bookwyrm/templates/preferences/2fa.html @@ -32,7 +32,7 @@
    {{ 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" %}
    @@ -46,7 +46,7 @@
    {{ form.password }} - {% 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.password.errors id="desc_password" %}
    diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 2a1656092..423a42d74 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -31,11 +31,10 @@ class Edit2FA(View): def post(self, request): """check the user's password""" form = forms.ConfirmPasswordForm(request.POST, instance=request.user) - # TODO: display an error if not form.is_valid(): data = {"form": form} return TemplateResponse(request, "preferences/2fa.html", data) - qr_form = forms.Confirm2FAForm(request.POST, instance=request.user) + qr_form = forms.Confirm2FAForm() data = { "password_confirmed": True, "qrcode": self.create_qr_code(request.user), @@ -66,10 +65,15 @@ class Confirm2FA(View): def post(self, request): """confirm the 2FA works before requiring it""" form = forms.Confirm2FAForm(request.POST, instance=request.user) - # TODO: show an error here + if not form.is_valid(): - data = {"form": form} - return redirect("prefs-2fa") + data = { + "password_confirmed": True, + "qrcode": Edit2FA.create_qr_code(self, request.user), + "form": form, + } + return TemplateResponse(request, "preferences/2fa.html", data) + # set the user's 2FA setting on request.user.two_factor_auth = True request.user.save(broadcast=False, update_fields=["two_factor_auth"]) From 9616abb6bd6395221668749b4e73aee6f6259d0d Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 18 Sep 2022 17:42:49 +1000 Subject: [PATCH 12/31] clean up 2fa prompt page --- .../templates/two_factor_auth/two_factor_prompt.html | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/bookwyrm/templates/two_factor_auth/two_factor_prompt.html b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html index 650b429c3..7fa1ff326 100644 --- a/bookwyrm/templates/two_factor_auth/two_factor_prompt.html +++ b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html @@ -21,8 +21,8 @@
    -
    -
    +
    +
    {% block header %}

    @@ -30,10 +30,10 @@

    {% endblock %}
    -
    -

    You can secure your account by setting up two factor authentication in your user preferences. This will require a one-time code from your phone in addition to your password each time you log in.

    -
    - No thanks +
    +

    You can secure your account by setting up two factor authentication in your user preferences. This will require a one-time code from your phone in addition to your password each time you log in.

    +
    + No thanks Set up 2FA
    From 9b74c2674223e26b74eda8cc3af8de73436dc801 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 18 Sep 2022 19:52:53 +1000 Subject: [PATCH 13/31] backup codes - add hotp_secret to user model - view to create backup codes in user prefs - check backup code if otp doesn't work - increment hotp count if used - show correct errors if code wrong --- bookwyrm/forms/landing.py | 33 +++++++++++------- ...911_0525.py => 0158_auto_20220918_0936.py} | 7 +++- bookwyrm/models/user.py | 1 + bookwyrm/templates/preferences/2fa.html | 25 ++++++++++++-- bookwyrm/urls.py | 5 +++ bookwyrm/views/__init__.py | 1 + bookwyrm/views/preferences/two_factor_auth.py | 34 ++++++++++++++++++- 7 files changed, 89 insertions(+), 17 deletions(-) rename bookwyrm/migrations/{0158_auto_20220911_0525.py => 0158_auto_20220918_0936.py} (75%) diff --git a/bookwyrm/forms/landing.py b/bookwyrm/forms/landing.py index ba7f69bb6..d5f1b196f 100644 --- a/bookwyrm/forms/landing.py +++ b/bookwyrm/forms/landing.py @@ -79,7 +79,9 @@ class PasswordResetForm(CustomForm): class Confirm2FAForm(CustomForm): - otp = forms.CharField(max_length=6, min_length=6, widget=forms.TextInput) + otp = forms.CharField( + max_length=6, min_length=6, widget=forms.TextInput(attrs={"autofocus": True}) + ) class Meta: model = models.User @@ -91,16 +93,23 @@ class Confirm2FAForm(CustomForm): 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")) + if self.instance.hotp_secret: + # maybe it's a backup code? + hotp = pyotp.HOTP(self.instance.hotp_secret) + hotp_count = ( + self.instance.hotp_count + if self.instance.hotp_count is not None + else 0 + ) - # 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"]) + if not hotp.verify(otp, hotp_count): + self.add_error("otp", _("Incorrect code")) + + # increment the user hotp_count + else: + self.instance.hotp_count = hotp_count + 1 + self.instance.save(broadcast=False, update_fields=["hotp_count"]) + + else: + self.add_error("otp", _("Incorrect code")) diff --git a/bookwyrm/migrations/0158_auto_20220911_0525.py b/bookwyrm/migrations/0158_auto_20220918_0936.py similarity index 75% rename from bookwyrm/migrations/0158_auto_20220911_0525.py rename to bookwyrm/migrations/0158_auto_20220918_0936.py index 764c6172f..6cab013a3 100644 --- a/bookwyrm/migrations/0158_auto_20220911_0525.py +++ b/bookwyrm/migrations/0158_auto_20220918_0936.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.15 on 2022-09-11 05:25 +# Generated by Django 3.2.15 on 2022-09-18 09:36 from django.db import migrations, models @@ -15,6 +15,11 @@ class Migration(migrations.Migration): name="hotp_count", field=models.IntegerField(blank=True, default=0, null=True), ), + migrations.AddField( + model_name="user", + name="hotp_secret", + field=models.CharField(blank=True, default=None, max_length=32, null=True), + ), migrations.AddField( model_name="user", name="otp_secret", diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 87d7d5207..79f6fafbd 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -178,6 +178,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): # two factor authentication two_factor_auth = models.BooleanField(default=None, blank=True, null=True) otp_secret = models.CharField(max_length=32, default=None, blank=True, null=True) + hotp_secret = models.CharField(max_length=32, default=None, blank=True, null=True) hotp_count = models.IntegerField(default=0, blank=True, null=True) @property diff --git a/bookwyrm/templates/preferences/2fa.html b/bookwyrm/templates/preferences/2fa.html index a2b66aea0..0cd8edb63 100644 --- a/bookwyrm/templates/preferences/2fa.html +++ b/bookwyrm/templates/preferences/2fa.html @@ -17,9 +17,28 @@
    {% endif %} - {% if request.user.two_factor_auth %} -

    Two Factor Authentication is active on your account.

    - {% trans "Disable 2FA" %} + {% if backup_codes %} +
    +

    Backup codes

    +
    +

    {% trans "Write down or copy and paste these codes somewhere safe." %}

    +

    {% trans "You must use them in order, and they will not be displayed again." %}

    +
    +
      + {% for code in backup_codes %} +
    • {{ code }}
      • + {% endfor%} +
    +
    + {% elif request.user.two_factor_auth %} +
    +

    Two Factor Authentication is active on your account.

    + {% trans "Disable 2FA" %} +
    +
    +

    {% trans "You can generate backup codes to use in case you do not have access to your authentication app. If you generate new codes, any backup codes previously generated will no longer work." %}

    + {% trans "Generate backup codes" %} +
    {% elif password_confirmed %}
    {% csrf_token %} diff --git a/bookwyrm/urls.py b/bookwyrm/urls.py index 040fed727..dbe6c8609 100644 --- a/bookwyrm/urls.py +++ b/bookwyrm/urls.py @@ -484,6 +484,11 @@ urlpatterns = [ views.Edit2FA.as_view(), name="prefs-2fa", ), + re_path( + r"^preferences/2fa-backup-codes/?$", + views.GenerateBackupCodes.as_view(), + name="generate-2fa-backup-codes", + ), re_path( r"^preferences/confirm-2fa/?$", views.Confirm2FA.as_view(), diff --git a/bookwyrm/views/__init__.py b/bookwyrm/views/__init__.py index 98bca4bf7..f6ed9f4f5 100644 --- a/bookwyrm/views/__init__.py +++ b/bookwyrm/views/__init__.py @@ -36,6 +36,7 @@ from .preferences.two_factor_auth import ( Edit2FA, Confirm2FA, Disable2FA, + GenerateBackupCodes, LoginWith2FA, Prompt2FA, ) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 423a42d74..716c35bb8 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -59,6 +59,7 @@ class Edit2FA(View): return img.to_string() +@method_decorator(login_required, name="dispatch") class Confirm2FA(View): """confirm user's 2FA settings""" @@ -81,6 +82,7 @@ class Confirm2FA(View): return TemplateResponse(request, "preferences/2fa.html", data) +@method_decorator(login_required, name="dispatch") class Disable2FA(View): """Turn off 2FA on this user account""" @@ -118,7 +120,7 @@ class LoginWith2FA(View): if not form.is_valid(): # make life harder for bots # humans are unlikely to get it wrong more than twice - if not request.session["2fa_attempts"]: + if "2fa_attempts" not in request.session: request.session["2fa_attempts"] = 0 request.session["2fa_attempts"] = request.session["2fa_attempts"] + 1 time.sleep(2 ** request.session["2fa_attempts"]) @@ -134,6 +136,36 @@ class LoginWith2FA(View): return set_language(user, redirect("/")) +@method_decorator(login_required, name="dispatch") +class GenerateBackupCodes(View): + """Generate and display backup 2FA codes""" + + def get(self, request): + """Generate and display backup 2FA codes""" + data = {"backup_codes": self.generate_backup_codes(request.user)} + return TemplateResponse(request, "preferences/2fa.html", data) + + def generate_backup_codes(self, user): + """generate fresh backup codes for 2FA""" + + # create fresh hotp secrets and count + hotp_secret = pyotp.random_base32() + user.hotp_count = 0 + # save the secret to the user record + user.hotp_secret = hotp_secret + user.save(broadcast=False, update_fields=["hotp_count", "hotp_secret"]) + + # generate codes + hotp = pyotp.HOTP(hotp_secret) + counter = 0 + codes = [] + while counter < 10: + codes.append(hotp.at(counter)) + counter = counter + 1 + + return codes + + class Prompt2FA(View): """Alert user to the existence of 2FA""" From e1b1bb20dcb238e9b3575b005edca0f063d94f3a Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sun, 18 Sep 2022 20:30:45 +1000 Subject: [PATCH 14/31] make password field less goofy in 2fa screen --- bookwyrm/templates/preferences/2fa.html | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/bookwyrm/templates/preferences/2fa.html b/bookwyrm/templates/preferences/2fa.html index 0cd8edb63..5d56e828d 100644 --- a/bookwyrm/templates/preferences/2fa.html +++ b/bookwyrm/templates/preferences/2fa.html @@ -60,15 +60,19 @@ {% trans "You can make your account more secure by using Two Factor Authentication (2FA). This will require you to enter a one-time code using a phone app like Authy, Google Authenticator or Microsoft Authenticator each time you log in." %}

    {% trans "Confirm your password to begin setting up 2FA." %}

    - - {% csrf_token %} -
    - - {{ form.password }} - {% include 'snippets/form_errors.html' with errors_list=form.password.errors id="desc_password" %} +
    +
    + + {% csrf_token %} +
    + + {{ form.password }} + {% include 'snippets/form_errors.html' with errors_list=form.password.errors id="desc_password" %} +
    + +
    - - +
    {% endif %}
    {% endblock %} From 28329c1781bec1d3b6f3d5c4cd2dc92c527373f9 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 24 Sep 2022 15:48:20 +1000 Subject: [PATCH 15/31] use string for datetime in session It seemed to work when testing manually, but both pytest and the django documentation indicate that you can't pass datetimes around as session values. --- bookwyrm/views/landing/login.py | 5 +++-- bookwyrm/views/preferences/two_factor_auth.py | 3 +-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bookwyrm/views/landing/login.py b/bookwyrm/views/landing/login.py index b9a877687..506617cc1 100644 --- a/bookwyrm/views/landing/login.py +++ b/bookwyrm/views/landing/login.py @@ -1,5 +1,6 @@ """ class views for login/register views """ -from datetime import datetime +import time + from django.contrib.auth import authenticate, login, logout from django.contrib.auth.decorators import login_required from django.shortcuts import redirect @@ -56,7 +57,7 @@ class Login(View): # if 2fa is set, don't log them in until they enter the right code if user.two_factor_auth is True: request.session["2fa_user"] = user.username - request.session["2fa_auth_time"] = datetime.now() + request.session["2fa_auth_time"] = time.time() return redirect("login-with-2fa") # otherwise, successful login diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 716c35bb8..63f47710b 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -103,14 +103,13 @@ class LoginWith2FA(View): 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.session["2fa_user"]) - elapsed_time = datetime.now() - request.session["2fa_auth_time"] + elapsed_time = datetime.now() - datetime.fromtimestamp(int(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): From b63d4bec60ef5ac5ec2b9dafea87f9990c22f6d7 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 24 Sep 2022 15:50:02 +1000 Subject: [PATCH 16/31] add tests for 2fa --- bookwyrm/tests/views/landing/test_login.py | 49 ++++- .../views/preferences/test_two_factor_auth.py | 203 ++++++++++++++++++ 2 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 bookwyrm/tests/views/preferences/test_two_factor_auth.py diff --git a/bookwyrm/tests/views/landing/test_login.py b/bookwyrm/tests/views/landing/test_login.py index 24987c8ef..cb2037c7d 100644 --- a/bookwyrm/tests/views/landing/test_login.py +++ b/bookwyrm/tests/views/landing/test_login.py @@ -2,6 +2,7 @@ from unittest.mock import patch from django.contrib.auth.models import AnonymousUser +from django.contrib.sessions.middleware import SessionMiddleware from django.template.response import TemplateResponse from django.test import TestCase from django.test.client import RequestFactory @@ -28,10 +29,25 @@ class LoginViews(TestCase): "password", local=True, localname="mouse", + two_factor_auth=False + ) + self.rat = models.User.objects.create_user( + "rat@your.domain.here", + "rat@rat.com", + "password", + local=True, + localname="rat", + ) + self.badger = models.User.objects.create_user( + "badger@your.domain.here", + "badger@badger.com", + "password", + local=True, + localname="badger", + two_factor_auth=True ) self.anonymous_user = AnonymousUser self.anonymous_user.is_authenticated = False - models.SiteSettings.objects.create(id=1, require_confirm_email=False) def test_login_get(self, *_): @@ -109,3 +125,34 @@ class LoginViews(TestCase): result.context_data["login_form"].non_field_errors, "Username or password are incorrect", ) + + def test_login_post_no_2fa_set(self, *_): + """test user with 2FA null value is redirected to 2FA prompt page""" + view = views.Login.as_view() + form = forms.LoginForm() + form.data["localname"] = "rat" + form.data["password"] = "password" + request = self.factory.post("", form.data) + request.user = self.anonymous_user + + with patch("bookwyrm.views.landing.login.login"): + result = view(request) + self.assertEqual(result.url, "/login-2FA-prompt") + self.assertEqual(result.status_code, 302) + + def test_login_post_with_2fa(self, *_): + """test user with 2FA turned on is redirected to 2FA login page""" + view = views.Login.as_view() + form = forms.LoginForm() + form.data["localname"] = "badger" + form.data["password"] = "password" + request = self.factory.post("", form.data) + request.user = self.anonymous_user + middleware = SessionMiddleware(request) + middleware.process_request(request) + request.session.save() + + with patch("bookwyrm.views.landing.login.login"): + result = view(request) + self.assertEqual(result.url, "/login-2FA-check") + self.assertEqual(result.status_code, 302) diff --git a/bookwyrm/tests/views/preferences/test_two_factor_auth.py b/bookwyrm/tests/views/preferences/test_two_factor_auth.py new file mode 100644 index 000000000..907605238 --- /dev/null +++ b/bookwyrm/tests/views/preferences/test_two_factor_auth.py @@ -0,0 +1,203 @@ +""" test for app two factor auth functionality """ +from unittest.mock import patch +import pyotp +from datetime import datetime +import time + +from django.contrib.auth.models import AnonymousUser +from django.contrib.sessions.middleware import SessionMiddleware +from django.template.response import TemplateResponse +from django.test import TestCase +from django.test.client import RequestFactory + +from bookwyrm import forms, models, views +from bookwyrm.tests.validate_html import validate_html + +# pylint: disable=too-many-public-methods +@patch("bookwyrm.suggested_users.rerank_suggestions_task.delay") +@patch("bookwyrm.activitystreams.populate_stream_task.delay") +class TwoFactorViews(TestCase): + """Two Factor Authentication management""" + + def setUp(self): + """we need basic test data and mocks""" + self.factory = RequestFactory() + with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( + "bookwyrm.activitystreams.populate_stream_task.delay" + ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): + self.local_user = models.User.objects.create_user( + "mouse@your.domain.here", + "mouse@mouse.com", + "password", + local=True, + localname="mouse", + two_factor_auth=True, + otp_secret="UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X", + hotp_secret="DRMNMOU7ZRKH5YPW7PADOEYUF7MRIH46", + hotp_count=0 + + ) + self.anonymous_user = AnonymousUser + self.anonymous_user.is_authenticated = False + + def test_get_edit_2fa(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Edit2FA.as_view() + request = self.factory.get("") + request.user = self.local_user + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) + + def test_get_edit_2fa_logged_out(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Edit2FA.as_view() + request = self.factory.get("") + request.user = self.anonymous_user + result = view(request) + self.assertEqual(result.status_code, 302) + + def test_post_edit_2fa(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Edit2FA.as_view() + form = forms.ConfirmPasswordForm() + form.data["password"] = "password" + request = self.factory.post("", form.data) + request.user = self.local_user + + with patch("bookwyrm.views.preferences.two_factor_auth.Edit2FA"): + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) + + def test_post_confirm_2fa(self, *_): + """check 2FA login works""" + view = views.Confirm2FA.as_view() + form = forms.Confirm2FAForm() + totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') + form.data["otp"] = totp.now() + request = self.factory.post("", form.data) + request.user = self.local_user + + with patch("bookwyrm.views.preferences.two_factor_auth.Confirm2FA"): + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) + + + def test_get_disable_2fa(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Disable2FA.as_view() + request = self.factory.get("") + request.user = self.local_user + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) + + def test_post_disable_2fa(self, *_): + """check 2FA login works""" + view = views.Disable2FA.as_view() + request = self.factory.post("") + request.user = self.local_user + + with patch("bookwyrm.views.preferences.two_factor_auth.Disable2FA"): + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) + + def test_get_login_with_2fa(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.LoginWith2FA.as_view() + request = self.factory.get("") + request.user = self.local_user + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) + + def test_post_login_with_2fa(self, *_): + """check 2FA login works""" + # NOTE this throws a redis error. + # Possibly because the function it tests wants to check the user database? + # can we mock that? + # view = views.LoginWith2FA.as_view() + # form = forms.Confirm2FAForm() + # totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') + + # form.data["otp"] = totp.now() + # request = self.factory.post("", form.data) + # request.user = self.local_user + + # middleware = SessionMiddleware(request) + # middleware.process_request(request) + # request.session['2fa_auth_time'] = time.time() + # request.session['2fa_user'] = self.local_user.username + # request.session.save() + + # with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): + # result = view(request) + # self.assertIsInstance(result, TemplateResponse) + # self.assertEqual(result.status_code, 200) + pass + + def test_post_login_with_2fa_wrong_code(self, *_): + """check 2FA login fails""" + view = views.LoginWith2FA.as_view() + form = forms.Confirm2FAForm() + form.data["otp"] = "111111" + request = self.factory.post("", form.data) + request.user = self.local_user + + middleware = SessionMiddleware(request) + middleware.process_request(request) + request.session['2fa_auth_time'] = time.time() + request.session['2fa_user'] = self.local_user.username + request.session.save() + + with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): + result = view(request) + self.assertEqual(result.status_code, 200) + self.assertEqual( + result.context_data["form"]["otp"].errors[0], + 'Incorrect code', + ) + + def test_post_login_with_2fa_expired(self, *_): + """check 2FA login fails""" + view = views.LoginWith2FA.as_view() + form = forms.Confirm2FAForm() + totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') + + form.data["otp"] = totp.now() + request = self.factory.post("", form.data) + request.user = self.local_user + + middleware = SessionMiddleware(request) + middleware.process_request(request) + request.session['2fa_user'] = self.local_user.username + request.session['2fa_auth_time'] = "1663977030" + + with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): + result = view(request) + self.assertEqual(result.url, "/") + self.assertEqual(result.status_code, 302) + +""" +Edit2FA + - get ✔ + - post ✔ + - create_qr_code +Confirm2FA + - post ✔ +Disable2FA + - get ✔ + - post ✔ +LoginWith2FA + - get ✔ + - post ✔ +GenerateBackupCodes + - get + - generate_backup_codes +Prompt2FA + - get + +""" From 9d36722783173423d149f1a5faee6def738b2458 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 24 Sep 2022 15:57:16 +1000 Subject: [PATCH 17/31] code formatting --- bookwyrm/tests/views/landing/test_login.py | 4 ++-- .../views/preferences/test_two_factor_auth.py | 19 +++++++++---------- bookwyrm/views/preferences/two_factor_auth.py | 4 +++- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/bookwyrm/tests/views/landing/test_login.py b/bookwyrm/tests/views/landing/test_login.py index cb2037c7d..23d35f303 100644 --- a/bookwyrm/tests/views/landing/test_login.py +++ b/bookwyrm/tests/views/landing/test_login.py @@ -29,7 +29,7 @@ class LoginViews(TestCase): "password", local=True, localname="mouse", - two_factor_auth=False + two_factor_auth=False, ) self.rat = models.User.objects.create_user( "rat@your.domain.here", @@ -44,7 +44,7 @@ class LoginViews(TestCase): "password", local=True, localname="badger", - two_factor_auth=True + two_factor_auth=True, ) self.anonymous_user = AnonymousUser self.anonymous_user.is_authenticated = False diff --git a/bookwyrm/tests/views/preferences/test_two_factor_auth.py b/bookwyrm/tests/views/preferences/test_two_factor_auth.py index 907605238..28d15764a 100644 --- a/bookwyrm/tests/views/preferences/test_two_factor_auth.py +++ b/bookwyrm/tests/views/preferences/test_two_factor_auth.py @@ -34,8 +34,7 @@ class TwoFactorViews(TestCase): two_factor_auth=True, otp_secret="UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X", hotp_secret="DRMNMOU7ZRKH5YPW7PADOEYUF7MRIH46", - hotp_count=0 - + hotp_count=0, ) self.anonymous_user = AnonymousUser self.anonymous_user.is_authenticated = False @@ -74,7 +73,7 @@ class TwoFactorViews(TestCase): """check 2FA login works""" view = views.Confirm2FA.as_view() form = forms.Confirm2FAForm() - totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') + totp = pyotp.TOTP("UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X") form.data["otp"] = totp.now() request = self.factory.post("", form.data) request.user = self.local_user @@ -84,7 +83,6 @@ class TwoFactorViews(TestCase): self.assertIsInstance(result, TemplateResponse) self.assertEqual(result.status_code, 200) - def test_get_disable_2fa(self, *_): """there are so many views, this just makes sure it LOADS""" view = views.Disable2FA.as_view() @@ -149,8 +147,8 @@ class TwoFactorViews(TestCase): middleware = SessionMiddleware(request) middleware.process_request(request) - request.session['2fa_auth_time'] = time.time() - request.session['2fa_user'] = self.local_user.username + request.session["2fa_auth_time"] = time.time() + request.session["2fa_user"] = self.local_user.username request.session.save() with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): @@ -158,14 +156,14 @@ class TwoFactorViews(TestCase): self.assertEqual(result.status_code, 200) self.assertEqual( result.context_data["form"]["otp"].errors[0], - 'Incorrect code', + "Incorrect code", ) def test_post_login_with_2fa_expired(self, *_): """check 2FA login fails""" view = views.LoginWith2FA.as_view() form = forms.Confirm2FAForm() - totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') + totp = pyotp.TOTP("UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X") form.data["otp"] = totp.now() request = self.factory.post("", form.data) @@ -173,14 +171,15 @@ class TwoFactorViews(TestCase): middleware = SessionMiddleware(request) middleware.process_request(request) - request.session['2fa_user'] = self.local_user.username - request.session['2fa_auth_time'] = "1663977030" + request.session["2fa_user"] = self.local_user.username + request.session["2fa_auth_time"] = "1663977030" with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): result = view(request) self.assertEqual(result.url, "/") self.assertEqual(result.status_code, 302) + """ Edit2FA - get ✔ diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 63f47710b..6fce49372 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -109,7 +109,9 @@ class LoginWith2FA(View): def post(self, request): """Check 2FA code and allow/disallow login""" user = models.User.objects.get(username=request.session["2fa_user"]) - elapsed_time = datetime.now() - datetime.fromtimestamp(int(request.session["2fa_auth_time"])) + elapsed_time = datetime.now() - datetime.fromtimestamp( + int(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): From 119b4bf2ff3dc6a1e6b128c5af531beb06f73084 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 24 Sep 2022 16:11:34 +1000 Subject: [PATCH 18/31] clean up tests - remove unnecessary crap - add missing tests --- .../views/preferences/test_two_factor_auth.py | 81 +++++++++---------- 1 file changed, 37 insertions(+), 44 deletions(-) diff --git a/bookwyrm/tests/views/preferences/test_two_factor_auth.py b/bookwyrm/tests/views/preferences/test_two_factor_auth.py index 28d15764a..a3a4515f6 100644 --- a/bookwyrm/tests/views/preferences/test_two_factor_auth.py +++ b/bookwyrm/tests/views/preferences/test_two_factor_auth.py @@ -1,8 +1,7 @@ """ test for app two factor auth functionality """ from unittest.mock import patch -import pyotp -from datetime import datetime import time +import pyotp from django.contrib.auth.models import AnonymousUser from django.contrib.sessions.middleware import SessionMiddleware @@ -11,7 +10,6 @@ from django.test import TestCase from django.test.client import RequestFactory from bookwyrm import forms, models, views -from bookwyrm.tests.validate_html import validate_html # pylint: disable=too-many-public-methods @patch("bookwyrm.suggested_users.rerank_suggestions_task.delay") @@ -112,30 +110,29 @@ class TwoFactorViews(TestCase): self.assertIsInstance(result, TemplateResponse) self.assertEqual(result.status_code, 200) - def test_post_login_with_2fa(self, *_): - """check 2FA login works""" - # NOTE this throws a redis error. - # Possibly because the function it tests wants to check the user database? - # can we mock that? - # view = views.LoginWith2FA.as_view() - # form = forms.Confirm2FAForm() - # totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') + # def test_post_login_with_2fa(self, *_): + # """check 2FA login works""" + # NOTE this throws a redis error. + # Possibly because the function it tests wants to check the user database? + # can we mock that? + # view = views.LoginWith2FA.as_view() + # form = forms.Confirm2FAForm() + # totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') - # form.data["otp"] = totp.now() - # request = self.factory.post("", form.data) - # request.user = self.local_user + # form.data["otp"] = totp.now() + # request = self.factory.post("", form.data) + # request.user = self.local_user - # middleware = SessionMiddleware(request) - # middleware.process_request(request) - # request.session['2fa_auth_time'] = time.time() - # request.session['2fa_user'] = self.local_user.username - # request.session.save() + # middleware = SessionMiddleware(request) + # middleware.process_request(request) + # request.session['2fa_auth_time'] = time.time() + # request.session['2fa_user'] = self.local_user.username + # request.session.save() - # with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): - # result = view(request) - # self.assertIsInstance(result, TemplateResponse) - # self.assertEqual(result.status_code, 200) - pass + # with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): + # result = view(request) + # self.assertIsInstance(result, TemplateResponse) + # self.assertEqual(result.status_code, 200) def test_post_login_with_2fa_wrong_code(self, *_): """check 2FA login fails""" @@ -179,24 +176,20 @@ class TwoFactorViews(TestCase): self.assertEqual(result.url, "/") self.assertEqual(result.status_code, 302) + def test_get_generate_backup_codes(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.GenerateBackupCodes.as_view() + request = self.factory.get("") + request.user = self.local_user + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) -""" -Edit2FA - - get ✔ - - post ✔ - - create_qr_code -Confirm2FA - - post ✔ -Disable2FA - - get ✔ - - post ✔ -LoginWith2FA - - get ✔ - - post ✔ -GenerateBackupCodes - - get - - generate_backup_codes -Prompt2FA - - get - -""" + def test_get_prompt_2fa(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Prompt2FA.as_view() + request = self.factory.get("") + request.user = self.local_user + result = view(request) + self.assertIsInstance(result, TemplateResponse) + self.assertEqual(result.status_code, 200) From fda150fa0d0a83509c09d5bd6504c92a5aedf881 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 24 Sep 2022 16:56:20 +1000 Subject: [PATCH 19/31] resolve migration conflict --- ...918_0936.py => 0159_auto_20220924_0634.py} | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) rename bookwyrm/migrations/{0158_auto_20220918_0936.py => 0159_auto_20220924_0634.py} (63%) diff --git a/bookwyrm/migrations/0158_auto_20220918_0936.py b/bookwyrm/migrations/0159_auto_20220924_0634.py similarity index 63% rename from bookwyrm/migrations/0158_auto_20220918_0936.py rename to bookwyrm/migrations/0159_auto_20220924_0634.py index 6cab013a3..966577fda 100644 --- a/bookwyrm/migrations/0158_auto_20220918_0936.py +++ b/bookwyrm/migrations/0159_auto_20220924_0634.py @@ -1,4 +1,4 @@ -# Generated by Django 3.2.15 on 2022-09-18 09:36 +# Generated by Django 3.2.15 on 2022-09-24 06:34 from django.db import migrations, models @@ -6,28 +6,28 @@ from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ - ("bookwyrm", "0157_auto_20220909_2338"), + ('bookwyrm', '0158_auto_20220919_1634'), ] operations = [ migrations.AddField( - model_name="user", - name="hotp_count", + model_name='user', + name='hotp_count', field=models.IntegerField(blank=True, default=0, null=True), ), migrations.AddField( - model_name="user", - name="hotp_secret", + model_name='user', + name='hotp_secret', field=models.CharField(blank=True, default=None, max_length=32, null=True), ), migrations.AddField( - model_name="user", - name="otp_secret", + model_name='user', + name='otp_secret', field=models.CharField(blank=True, default=None, max_length=32, null=True), ), migrations.AddField( - model_name="user", - name="two_factor_auth", + model_name='user', + name='two_factor_auth', field=models.BooleanField(blank=True, default=None, null=True), ), ] From da613c9b262c08fc0e40e5c6da18dc791bf0433b Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 24 Sep 2022 17:07:18 +1000 Subject: [PATCH 20/31] ugh forgot to run black --- bookwyrm/migrations/0159_auto_20220924_0634.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/bookwyrm/migrations/0159_auto_20220924_0634.py b/bookwyrm/migrations/0159_auto_20220924_0634.py index 966577fda..c223d9061 100644 --- a/bookwyrm/migrations/0159_auto_20220924_0634.py +++ b/bookwyrm/migrations/0159_auto_20220924_0634.py @@ -6,28 +6,28 @@ from django.db import migrations, models class Migration(migrations.Migration): dependencies = [ - ('bookwyrm', '0158_auto_20220919_1634'), + ("bookwyrm", "0158_auto_20220919_1634"), ] operations = [ migrations.AddField( - model_name='user', - name='hotp_count', + model_name="user", + name="hotp_count", field=models.IntegerField(blank=True, default=0, null=True), ), migrations.AddField( - model_name='user', - name='hotp_secret', + model_name="user", + name="hotp_secret", field=models.CharField(blank=True, default=None, max_length=32, null=True), ), migrations.AddField( - model_name='user', - name='otp_secret', + model_name="user", + name="otp_secret", field=models.CharField(blank=True, default=None, max_length=32, null=True), ), migrations.AddField( - model_name='user', - name='two_factor_auth', + model_name="user", + name="two_factor_auth", field=models.BooleanField(blank=True, default=None, null=True), ), ] From e1513bf98d26b5a6a238e60aa6e097ebfa6b1e11 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Oct 2022 21:53:51 +1100 Subject: [PATCH 21/31] amend nginx rate limiting urls --- bookwyrm/urls.py | 4 ++-- nginx/development | 2 +- nginx/production | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bookwyrm/urls.py b/bookwyrm/urls.py index dbe6c8609..3838619e7 100644 --- a/bookwyrm/urls.py +++ b/bookwyrm/urls.py @@ -500,12 +500,12 @@ urlpatterns = [ name="disable-2fa", ), re_path( - r"^login-2FA-check/?$", + r"^2fa-check/?$", views.LoginWith2FA.as_view(), name="login-with-2fa", ), re_path( - r"^login-2FA-prompt/?$", + r"^2fa-prompt/?$", views.Prompt2FA.as_view(), name="prompt-2fa", ), diff --git a/nginx/development b/nginx/development index fbb25c1b2..4a7896249 100644 --- a/nginx/development +++ b/nginx/development @@ -7,7 +7,7 @@ upstream web { server { listen 80; - location ~ ^/(login|password-reset|resend-link) { + location ~ ^/(login[^-]|password-reset|resend-link|2fa-check) { limit_req zone=loginlimit; proxy_pass http://web; diff --git a/nginx/production b/nginx/production index 3a3aeb7dd..b74fe409c 100644 --- a/nginx/production +++ b/nginx/production @@ -41,7 +41,7 @@ server { # root /var/www/certbot; # } # -# location ~ ^/(login|password-reset|resend-link) { +# location ~ ^/(login[^-]|password-reset|resend-link|2fa-check) { # limit_req zone=loginlimit; # # proxy_pass http://web; From aefc7a23bcf3c4bc2c492eec384ce853974b988f Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Oct 2022 21:54:52 +1100 Subject: [PATCH 22/31] fix 2fa templates - translate all strings - do not embed svg element inside svg element - fix sizing of input for confirming 2fa setup --- bookwyrm/templates/preferences/2fa.html | 26 +++++++++---------- .../two_factor_auth/two_factor_login.html | 1 + .../two_factor_auth/two_factor_prompt.html | 7 ++--- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/bookwyrm/templates/preferences/2fa.html b/bookwyrm/templates/preferences/2fa.html index 5d56e828d..b0703bc4a 100644 --- a/bookwyrm/templates/preferences/2fa.html +++ b/bookwyrm/templates/preferences/2fa.html @@ -32,7 +32,7 @@
    {% elif request.user.two_factor_auth %}
    -

    Two Factor Authentication is active on your account.

    +

    {% trans "Two Factor Authentication is active on your account." %}

    {% trans "Disable 2FA" %}
    @@ -42,21 +42,21 @@ {% elif password_confirmed %}
    {% csrf_token %} -

    Scan the QR code with your authentication app and then enter the code from your app below to confirm your app is set up.

    -
    - - {{ qrcode | safe }} - +

    {% trans "Scan the QR code with your authentication app and then enter the code from your app below to confirm your app is set up." %}

    +
    +
    +
    {{ qrcode | safe }}
    +
    + + {{ form.otp }} + {% include 'snippets/form_errors.html' with errors_list=form.otp.errors id="desc_otp" %} +
    + +
    -
    - - {{ form.otp }} - {% include 'snippets/form_errors.html' with errors_list=form.otp.errors id="desc_otp" %} -
    - {% else %} -

    +

    {% trans "You can make your account more secure by using Two Factor Authentication (2FA). This will require you to enter a one-time code using a phone app like Authy, Google Authenticator or Microsoft Authenticator each time you log in." %}

    {% trans "Confirm your password to begin setting up 2FA." %}

    diff --git a/bookwyrm/templates/two_factor_auth/two_factor_login.html b/bookwyrm/templates/two_factor_auth/two_factor_login.html index f4a2fdf04..bea1c4f0e 100644 --- a/bookwyrm/templates/two_factor_auth/two_factor_login.html +++ b/bookwyrm/templates/two_factor_auth/two_factor_login.html @@ -44,6 +44,7 @@
    +
    diff --git a/bookwyrm/templates/two_factor_auth/two_factor_prompt.html b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html index 7fa1ff326..479f46c9c 100644 --- a/bookwyrm/templates/two_factor_auth/two_factor_prompt.html +++ b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html @@ -31,15 +31,16 @@ {% endblock %}
    -

    You can secure your account by setting up two factor authentication in your user preferences. This will require a one-time code from your phone in addition to your password each time you log in.

    +

    {% trans "You can secure your account by setting up two factor authentication in your user preferences. This will require a one-time code from your phone in addition to your password each time you log in." %}

    - No thanks - Set up 2FA + {% trans "No thanks" %} + {% trans "Set up 2FA" %}
    +
    From 79b04c22400ef18ada884286ba85dc5a3ef3bb56 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Oct 2022 21:56:38 +1100 Subject: [PATCH 23/31] various 2fa improvements - cleaner code - use TWO_FACTOR_LOGIN_MAX_SECONDS instead of hardcoded number - render qrcode properly - use nginx to rate limit login attempts - do not throw error if session user is undefined --- bookwyrm/settings.py | 2 ++ bookwyrm/views/landing/login.py | 2 +- bookwyrm/views/preferences/two_factor_auth.py | 21 +++++++------------ 3 files changed, 11 insertions(+), 14 deletions(-) diff --git a/bookwyrm/settings.py b/bookwyrm/settings.py index 44bf84466..7e6afbc6b 100644 --- a/bookwyrm/settings.py +++ b/bookwyrm/settings.py @@ -358,3 +358,5 @@ else: OTEL_EXPORTER_OTLP_ENDPOINT = env("OTEL_EXPORTER_OTLP_ENDPOINT", None) OTEL_EXPORTER_OTLP_HEADERS = env("OTEL_EXPORTER_OTLP_HEADERS", None) OTEL_SERVICE_NAME = env("OTEL_SERVICE_NAME", None) + +TWO_FACTOR_LOGIN_MAX_SECONDS = 60 diff --git a/bookwyrm/views/landing/login.py b/bookwyrm/views/landing/login.py index 506617cc1..3b7b10f0d 100644 --- a/bookwyrm/views/landing/login.py +++ b/bookwyrm/views/landing/login.py @@ -55,7 +55,7 @@ class Login(View): user = authenticate(request, username=username, password=password) 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: + if user.two_factor_auth: request.session["2fa_user"] = user.username request.session["2fa_auth_time"] = time.time() return redirect("login-with-2fa") diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 6fce49372..5c7a8f0ee 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -14,7 +14,7 @@ from django.views import View from django.views.decorators.debug import sensitive_post_parameters from bookwyrm import forms, models -from bookwyrm.settings import DOMAIN +from bookwyrm.settings import DOMAIN, TWO_FACTOR_LOGIN_MAX_SECONDS from bookwyrm.views.helpers import set_language # pylint: disable= no-self-use @@ -56,7 +56,7 @@ class Edit2FA(View): qr_code.add_data(provisioning_url) qr_code.make(fit=True) img = qr_code.make_image(attrib={"fill": "black"}) - return img.to_string() + return str(img.to_string(), 'utf-8') # to_string() returns a byte string @method_decorator(login_required, name="dispatch") @@ -108,24 +108,19 @@ class LoginWith2FA(View): def post(self, request): """Check 2FA code and allow/disallow login""" + if "2fa_user" not in request.session: + request.session["2fa_auth_time"] = 0 + return redirect("/") user = models.User.objects.get(username=request.session["2fa_user"]) - elapsed_time = datetime.now() - datetime.fromtimestamp( - int(request.session["2fa_auth_time"]) - ) + session_time = int(request.session["2fa_auth_time"]) if request.session["2fa_auth_time"] else 0 + elapsed_time = datetime.now() - datetime.fromtimestamp(session_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): + if elapsed_time > timedelta(seconds=TWO_FACTOR_LOGIN_MAX_SECONDS): request.session["2fa_user"] = None request.session["2fa_auth_time"] = 0 return redirect("/") if not form.is_valid(): - # make life harder for bots - # humans are unlikely to get it wrong more than twice - if "2fa_attempts" not in request.session: - request.session["2fa_attempts"] = 0 - 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 From f3768c3d92b46f1231dd2f1e10b37e7f45ebc727 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Oct 2022 22:02:00 +1100 Subject: [PATCH 24/31] code formatting fix --- bookwyrm/views/preferences/two_factor_auth.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 5c7a8f0ee..46a434862 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -56,7 +56,7 @@ class Edit2FA(View): qr_code.add_data(provisioning_url) qr_code.make(fit=True) img = qr_code.make_image(attrib={"fill": "black"}) - return str(img.to_string(), 'utf-8') # to_string() returns a byte string + return str(img.to_string(), "utf-8") # to_string() returns a byte string @method_decorator(login_required, name="dispatch") @@ -112,7 +112,11 @@ class LoginWith2FA(View): request.session["2fa_auth_time"] = 0 return redirect("/") user = models.User.objects.get(username=request.session["2fa_user"]) - session_time = int(request.session["2fa_auth_time"]) if request.session["2fa_auth_time"] else 0 + session_time = ( + int(request.session["2fa_auth_time"]) + if request.session["2fa_auth_time"] + else 0 + ) elapsed_time = datetime.now() - datetime.fromtimestamp(session_time) form = forms.Confirm2FAForm(request.POST, instance=user) # don't allow the login credentials to last too long before completing login From cffbf82ddb496ba561faf1b2fa904a5536dc0750 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Oct 2022 22:11:46 +1100 Subject: [PATCH 25/31] DRY footer for 2FA pages --- bookwyrm/templates/snippets/2fa_footer.html | 49 +++++++++++++++++ .../two_factor_auth/two_factor_login.html | 50 +----------------- .../two_factor_auth/two_factor_prompt.html | 52 +------------------ 3 files changed, 52 insertions(+), 99 deletions(-) create mode 100644 bookwyrm/templates/snippets/2fa_footer.html diff --git a/bookwyrm/templates/snippets/2fa_footer.html b/bookwyrm/templates/snippets/2fa_footer.html new file mode 100644 index 000000000..22c1fd1c5 --- /dev/null +++ b/bookwyrm/templates/snippets/2fa_footer.html @@ -0,0 +1,49 @@ +{% load i18n %} + + diff --git a/bookwyrm/templates/two_factor_auth/two_factor_login.html b/bookwyrm/templates/two_factor_auth/two_factor_login.html index bea1c4f0e..9df9d9f38 100644 --- a/bookwyrm/templates/two_factor_auth/two_factor_login.html +++ b/bookwyrm/templates/two_factor_auth/two_factor_login.html @@ -44,54 +44,6 @@
    - - - + {% include 'snippets/2fa_footer.html' %} diff --git a/bookwyrm/templates/two_factor_auth/two_factor_prompt.html b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html index 479f46c9c..eb9257895 100644 --- a/bookwyrm/templates/two_factor_auth/two_factor_prompt.html +++ b/bookwyrm/templates/two_factor_auth/two_factor_prompt.html @@ -21,7 +21,7 @@
    -
    +
    {% block header %} @@ -40,54 +40,6 @@
    - - - + {% include 'snippets/2fa_footer.html' %} From f55adbadf4adb9a3b4397cd3aa3ba4f41832ec5e Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Oct 2022 22:14:04 +1100 Subject: [PATCH 26/31] fix 2fa tests --- bookwyrm/tests/views/landing/test_login.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bookwyrm/tests/views/landing/test_login.py b/bookwyrm/tests/views/landing/test_login.py index 23d35f303..6c1188b09 100644 --- a/bookwyrm/tests/views/landing/test_login.py +++ b/bookwyrm/tests/views/landing/test_login.py @@ -137,7 +137,7 @@ class LoginViews(TestCase): with patch("bookwyrm.views.landing.login.login"): result = view(request) - self.assertEqual(result.url, "/login-2FA-prompt") + self.assertEqual(result.url, "/2fa-prompt") self.assertEqual(result.status_code, 302) def test_login_post_with_2fa(self, *_): @@ -154,5 +154,5 @@ class LoginViews(TestCase): with patch("bookwyrm.views.landing.login.login"): result = view(request) - self.assertEqual(result.url, "/login-2FA-check") + self.assertEqual(result.url, "/2fa-check") self.assertEqual(result.status_code, 302) From a1c3f15d8074eac94dcfcec11d40022091a33b24 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Oct 2022 22:23:39 +1100 Subject: [PATCH 27/31] remove unused import --- bookwyrm/views/preferences/two_factor_auth.py | 1 - 1 file changed, 1 deletion(-) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index 46a434862..f94e3cb52 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -1,6 +1,5 @@ """ class views for 2FA management """ from datetime import datetime, timedelta -import time import pyotp import qrcode import qrcode.image.svg From 905aa66f3843a23cf5fbff5350ae7360d5a0053a Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 15 Oct 2022 07:11:03 +1100 Subject: [PATCH 28/31] add test_post_login_with_2fa --- .../views/preferences/test_two_factor_auth.py | 40 +++++++++---------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/bookwyrm/tests/views/preferences/test_two_factor_auth.py b/bookwyrm/tests/views/preferences/test_two_factor_auth.py index a3a4515f6..ac6bd654c 100644 --- a/bookwyrm/tests/views/preferences/test_two_factor_auth.py +++ b/bookwyrm/tests/views/preferences/test_two_factor_auth.py @@ -110,29 +110,29 @@ class TwoFactorViews(TestCase): self.assertIsInstance(result, TemplateResponse) self.assertEqual(result.status_code, 200) - # def test_post_login_with_2fa(self, *_): - # """check 2FA login works""" - # NOTE this throws a redis error. - # Possibly because the function it tests wants to check the user database? - # can we mock that? - # view = views.LoginWith2FA.as_view() - # form = forms.Confirm2FAForm() - # totp = pyotp.TOTP('UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X') + def test_post_login_with_2fa(self, *_): + """check 2FA login works""" + view = views.LoginWith2FA.as_view() + form = forms.Confirm2FAForm() + totp = pyotp.TOTP("UEWMVJHO23G5XLMVSOCL6TNTSSACJH2X") - # form.data["otp"] = totp.now() - # request = self.factory.post("", form.data) - # request.user = self.local_user + form.data["otp"] = totp.now() + request = self.factory.post("", form.data) + request.user = self.local_user - # middleware = SessionMiddleware(request) - # middleware.process_request(request) - # request.session['2fa_auth_time'] = time.time() - # request.session['2fa_user'] = self.local_user.username - # request.session.save() + middleware = SessionMiddleware(request) + middleware.process_request(request) + request.session["2fa_auth_time"] = time.time() + request.session["2fa_user"] = self.local_user.username + request.session.save() - # with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"): - # result = view(request) - # self.assertIsInstance(result, TemplateResponse) - # self.assertEqual(result.status_code, 200) + with patch("bookwyrm.views.preferences.two_factor_auth.LoginWith2FA"), patch( + "bookwyrm.views.preferences.two_factor_auth.login" + ): + result = view(request) + self.assertEqual(result.url, "/") + self.assertEqual(result.status_code, 302) + self.assertTrue(request.user.is_authenticated) def test_post_login_with_2fa_wrong_code(self, *_): """check 2FA login fails""" From cf1fae6af83d1582a679b9469d9e3cc5e9e2bd05 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 15 Oct 2022 07:11:28 +1100 Subject: [PATCH 29/31] return Bad Request if 2fa user does not exist --- bookwyrm/views/preferences/two_factor_auth.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index f94e3cb52..acf8361a2 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -6,6 +6,7 @@ import qrcode.image.svg from django.contrib.auth import login from django.contrib.auth.decorators import login_required +from django.http import HttpResponseBadRequest from django.template.response import TemplateResponse from django.shortcuts import redirect from django.utils.decorators import method_decorator @@ -107,10 +108,12 @@ class LoginWith2FA(View): def post(self, request): """Check 2FA code and allow/disallow login""" - if "2fa_user" not in request.session: + try: + user = models.User.objects.get(username=request.session["2fa_user"]) + except: request.session["2fa_auth_time"] = 0 - return redirect("/") - user = models.User.objects.get(username=request.session["2fa_user"]) + return HttpResponseBadRequest("Invalid user") + session_time = ( int(request.session["2fa_auth_time"]) if request.session["2fa_auth_time"] From 32e4f7718ebd98bb3ec67954b7a5c1835bd98ea7 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 15 Oct 2022 07:23:27 +1100 Subject: [PATCH 30/31] pylint is being pedantic --- bookwyrm/views/preferences/two_factor_auth.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index acf8361a2..d0bba3e19 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -110,7 +110,7 @@ class LoginWith2FA(View): """Check 2FA code and allow/disallow login""" try: user = models.User.objects.get(username=request.session["2fa_user"]) - except: + except Exception: request.session["2fa_auth_time"] = 0 return HttpResponseBadRequest("Invalid user") From 3d95916b5553faa2a38acb08548fe309b238b570 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Sat, 15 Oct 2022 07:35:43 +1100 Subject: [PATCH 31/31] handle 2fa user exception properly --- bookwyrm/views/preferences/two_factor_auth.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bookwyrm/views/preferences/two_factor_auth.py b/bookwyrm/views/preferences/two_factor_auth.py index d0bba3e19..61003777f 100644 --- a/bookwyrm/views/preferences/two_factor_auth.py +++ b/bookwyrm/views/preferences/two_factor_auth.py @@ -6,6 +6,7 @@ import qrcode.image.svg from django.contrib.auth import login from django.contrib.auth.decorators import login_required +from django.core.exceptions import ObjectDoesNotExist from django.http import HttpResponseBadRequest from django.template.response import TemplateResponse from django.shortcuts import redirect @@ -109,8 +110,8 @@ class LoginWith2FA(View): def post(self, request): """Check 2FA code and allow/disallow login""" try: - user = models.User.objects.get(username=request.session["2fa_user"]) - except Exception: + user = models.User.objects.get(username=request.session.get("2fa_user")) + except ObjectDoesNotExist: request.session["2fa_auth_time"] = 0 return HttpResponseBadRequest("Invalid user")