From 75cc2ee164ef6c4e29730e8e5e122ff31c525649 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Tue, 7 Sep 2021 13:11:44 -0700 Subject: [PATCH 1/3] Split authentication views into login and register --- bookwyrm/tests/views/test_authentication.py | 16 ++-- bookwyrm/views/__init__.py | 4 +- bookwyrm/views/invite.py | 2 +- bookwyrm/views/login.py | 80 +++++++++++++++++++ .../views/{authentication.py => register.py} | 73 +---------------- 5 files changed, 92 insertions(+), 83 deletions(-) create mode 100644 bookwyrm/views/login.py rename bookwyrm/views/{authentication.py => register.py} (61%) diff --git a/bookwyrm/tests/views/test_authentication.py b/bookwyrm/tests/views/test_authentication.py index 95a4d9a0b..74f3c0902 100644 --- a/bookwyrm/tests/views/test_authentication.py +++ b/bookwyrm/tests/views/test_authentication.py @@ -63,7 +63,7 @@ class AuthenticationViews(TestCase): request = self.factory.post("", form.data) request.user = self.anonymous_user - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): result = view(request) self.assertEqual(result.url, "/") self.assertEqual(result.status_code, 302) @@ -77,7 +77,7 @@ class AuthenticationViews(TestCase): request = self.factory.post("", form.data) request.user = self.anonymous_user - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): result = view(request) self.assertEqual(result.url, "/") self.assertEqual(result.status_code, 302) @@ -91,7 +91,7 @@ class AuthenticationViews(TestCase): request = self.factory.post("", form.data) request.user = self.anonymous_user - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): result = view(request) self.assertEqual(result.url, "/") self.assertEqual(result.status_code, 302) @@ -105,7 +105,7 @@ class AuthenticationViews(TestCase): request = self.factory.post("", form.data) request.user = self.anonymous_user - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): result = view(request) result.render() self.assertEqual(result.status_code, 200) @@ -126,7 +126,7 @@ class AuthenticationViews(TestCase): "email": "aa@bb.cccc", }, ) - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): response = view(request) self.assertEqual(models.User.objects.count(), 2) self.assertEqual(response.status_code, 302) @@ -151,7 +151,7 @@ class AuthenticationViews(TestCase): "email": "aa@bb.cccc", }, ) - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): response = view(request) self.assertEqual(response.status_code, 302) nutria = models.User.objects.get(localname="nutria") @@ -169,7 +169,7 @@ class AuthenticationViews(TestCase): "register/", {"localname": "nutria ", "password": "mouseword", "email": "aa@bb.ccc"}, ) - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): response = view(request) self.assertEqual(models.User.objects.count(), 2) self.assertEqual(response.status_code, 302) @@ -248,7 +248,7 @@ class AuthenticationViews(TestCase): "invite_code": "testcode", }, ) - with patch("bookwyrm.views.authentication.login"): + with patch("bookwyrm.views.login.login"): response = view(request) self.assertEqual(models.User.objects.count(), 2) self.assertEqual(response.status_code, 302) diff --git a/bookwyrm/views/__init__.py b/bookwyrm/views/__init__.py index ca52800c4..841026a54 100644 --- a/bookwyrm/views/__init__.py +++ b/bookwyrm/views/__init__.py @@ -1,7 +1,5 @@ """ make sure all our nice views are available """ from .announcements import Announcements, Announcement, delete_announcement -from .authentication import Login, Register, Logout -from .authentication import ConfirmEmail, ConfirmEmailCode, resend_link from .author import Author, EditAuthor from .block import Block, unblock from .books import Book, EditBook, ConfirmEditBook @@ -28,11 +26,13 @@ from .landing import About, Home, Landing from .list import Lists, SavedLists, List, Curate, UserLists from .list import save_list, unsave_list from .list import delete_list +from .login import Login, Logout from .notifications import Notifications from .outbox import Outbox from .reading import edit_readthrough, create_readthrough from .reading import delete_readthrough, delete_progressupdate from .reading import ReadingStatus +from .register import Register, ConfirmEmail, ConfirmEmailCode, resend_link from .reports import Report, Reports, make_report, resolve_report, suspend_user from .rss_feed import RssFeed from .password import PasswordResetRequest, PasswordReset, ChangePassword diff --git a/bookwyrm/views/invite.py b/bookwyrm/views/invite.py index 005d57cf8..6a9bfedb9 100644 --- a/bookwyrm/views/invite.py +++ b/bookwyrm/views/invite.py @@ -83,7 +83,7 @@ class Invite(View): } return TemplateResponse(request, "invite.html", data) - # post handling is in views.authentication.Register + # post handling is in views.register.Register class ManageInviteRequests(View): diff --git a/bookwyrm/views/login.py b/bookwyrm/views/login.py new file mode 100644 index 000000000..b213590fb --- /dev/null +++ b/bookwyrm/views/login.py @@ -0,0 +1,80 @@ +""" class views for login/register views """ +from django.contrib.auth import authenticate, login, logout +from django.contrib.auth.decorators import login_required +from django.shortcuts import redirect +from django.template.response import TemplateResponse +from django.utils import timezone +from django.utils.decorators import method_decorator +from django.utils.translation import gettext_lazy as _ +from django.views.decorators.csrf import csrf_exempt +from django.views import View + +from bookwyrm import forms, models +from bookwyrm.settings import DOMAIN + + +# pylint: disable=no-self-use +@method_decorator(csrf_exempt, name="dispatch") +class Login(View): + """authenticate an existing user""" + + def get(self, request, confirmed=None): + """login page""" + if request.user.is_authenticated: + return redirect("/") + # send user to the login page + data = { + "show_confirmed_email": confirmed, + "login_form": forms.LoginForm(), + "register_form": forms.RegisterForm(), + } + return TemplateResponse(request, "login.html", data) + + def post(self, request): + """authentication action""" + if request.user.is_authenticated: + return redirect("/") + login_form = forms.LoginForm(request.POST) + + localname = login_form.data["localname"] + if "@" in localname: # looks like an email address to me + try: + username = models.User.objects.get(email=localname).username + except models.User.DoesNotExist: # maybe it's a full username? + username = localname + else: + username = "%s@%s" % (localname, DOMAIN) + password = login_form.data["password"] + + # perform authentication + user = authenticate(request, username=username, password=password) + if user is not None: + # successful login + login(request, user) + user.last_active_date = timezone.now() + user.save(broadcast=False, update_fields=["last_active_date"]) + if request.POST.get("first_login"): + return redirect("get-started-profile") + return redirect(request.GET.get("next", "/")) + + # maybe the user is pending email confirmation + if models.User.objects.filter( + username=username, is_active=False, deactivation_reason="pending" + ).exists(): + return redirect("confirm-email") + + # login errors + login_form.non_field_errors = _("Username or password are incorrect") + register_form = forms.RegisterForm() + data = {"login_form": login_form, "register_form": register_form} + return TemplateResponse(request, "login.html", data) + + +@method_decorator(login_required, name="dispatch") +class Logout(View): + """log out""" + + def get(self, request): + """done with this place! outa here!""" + logout(request) + return redirect("/") diff --git a/bookwyrm/views/authentication.py b/bookwyrm/views/register.py similarity index 61% rename from bookwyrm/views/authentication.py rename to bookwyrm/views/register.py index 70f51864d..334b29687 100644 --- a/bookwyrm/views/authentication.py +++ b/bookwyrm/views/register.py @@ -1,13 +1,8 @@ """ class views for login/register views """ -from django.contrib.auth import authenticate, login, logout -from django.contrib.auth.decorators import login_required +from django.contrib.auth import login from django.core.exceptions import PermissionDenied from django.shortcuts import get_object_or_404, redirect from django.template.response import TemplateResponse -from django.utils import timezone -from django.utils.decorators import method_decorator -from django.utils.translation import gettext_lazy as _ -from django.views.decorators.csrf import csrf_exempt from django.views.decorators.http import require_POST from django.views import View @@ -16,72 +11,6 @@ from bookwyrm.settings import DOMAIN # pylint: disable=no-self-use -@method_decorator(csrf_exempt, name="dispatch") -class Login(View): - """authenticate an existing user""" - - def get(self, request, confirmed=None): - """login page""" - if request.user.is_authenticated: - return redirect("/") - # send user to the login page - data = { - "show_confirmed_email": confirmed, - "login_form": forms.LoginForm(), - "register_form": forms.RegisterForm(), - } - return TemplateResponse(request, "login.html", data) - - def post(self, request): - """authentication action""" - if request.user.is_authenticated: - return redirect("/") - login_form = forms.LoginForm(request.POST) - - localname = login_form.data["localname"] - if "@" in localname: # looks like an email address to me - try: - username = models.User.objects.get(email=localname).username - except models.User.DoesNotExist: # maybe it's a full username? - username = localname - else: - username = "%s@%s" % (localname, DOMAIN) - password = login_form.data["password"] - - # perform authentication - user = authenticate(request, username=username, password=password) - if user is not None: - # successful login - login(request, user) - user.last_active_date = timezone.now() - user.save(broadcast=False, update_fields=["last_active_date"]) - if request.POST.get("first_login"): - return redirect("get-started-profile") - return redirect(request.GET.get("next", "/")) - - # maybe the user is pending email confirmation - if models.User.objects.filter( - username=username, is_active=False, deactivation_reason="pending" - ).exists(): - return redirect("confirm-email") - - # login errors - login_form.non_field_errors = _("Username or password are incorrect") - register_form = forms.RegisterForm() - data = {"login_form": login_form, "register_form": register_form} - return TemplateResponse(request, "login.html", data) - - -@method_decorator(login_required, name="dispatch") -class Logout(View): - """log out""" - - def get(self, request): - """done with this place! outa here!""" - logout(request) - return redirect("/") - - class Register(View): """register a user""" From ec501dfee9fe615767d7d139d7237bb8f9932c16 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Tue, 7 Sep 2021 13:21:40 -0700 Subject: [PATCH 2/3] Make sure passwords aren't exposed in error reporting --- bookwyrm/views/login.py | 5 ++++- bookwyrm/views/register.py | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/bookwyrm/views/login.py b/bookwyrm/views/login.py index b213590fb..1ca65f2ff 100644 --- a/bookwyrm/views/login.py +++ b/bookwyrm/views/login.py @@ -6,8 +6,9 @@ from django.template.response import TemplateResponse from django.utils import timezone from django.utils.decorators import method_decorator from django.utils.translation import gettext_lazy as _ -from django.views.decorators.csrf import csrf_exempt from django.views import View +from django.views.decorators.csrf import csrf_exempt +from django.views.decorators.debug import sensitive_variables, sensitive_post_parameters from bookwyrm import forms, models from bookwyrm.settings import DOMAIN @@ -30,6 +31,8 @@ class Login(View): } return TemplateResponse(request, "login.html", data) + @sensitive_variables("password") + @sensitive_post_parameters("password") def post(self, request): """authentication action""" if request.user.is_authenticated: diff --git a/bookwyrm/views/register.py b/bookwyrm/views/register.py index 334b29687..1ffa16ec6 100644 --- a/bookwyrm/views/register.py +++ b/bookwyrm/views/register.py @@ -3,8 +3,9 @@ from django.contrib.auth import login from django.core.exceptions import PermissionDenied from django.shortcuts import get_object_or_404, redirect from django.template.response import TemplateResponse -from django.views.decorators.http import require_POST from django.views import View +from django.views.decorators.http import require_POST +from django.views.decorators.debug import sensitive_variables, sensitive_post_parameters from bookwyrm import emailing, forms, models from bookwyrm.settings import DOMAIN @@ -14,6 +15,8 @@ from bookwyrm.settings import DOMAIN class Register(View): """register a user""" + @sensitive_variables("password") + @sensitive_post_parameters("password") def post(self, request): """join the server""" settings = models.SiteSettings.get() From 47ba2478b6b08f99649df6d152970f87f4a2450d Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Tue, 7 Sep 2021 15:03:15 -0700 Subject: [PATCH 3/3] Split out test files --- bookwyrm/tests/views/test_login.py | 110 ++++++++++++++++++ ...est_authentication.py => test_register.py} | 88 +------------- bookwyrm/views/login.py | 2 +- bookwyrm/views/register.py | 3 +- 4 files changed, 119 insertions(+), 84 deletions(-) create mode 100644 bookwyrm/tests/views/test_login.py rename bookwyrm/tests/views/{test_authentication.py => test_register.py} (76%) diff --git a/bookwyrm/tests/views/test_login.py b/bookwyrm/tests/views/test_login.py new file mode 100644 index 000000000..c37eaa514 --- /dev/null +++ b/bookwyrm/tests/views/test_login.py @@ -0,0 +1,110 @@ +""" test for app action functionality """ +from unittest.mock import patch + +from django.contrib.auth.models import AnonymousUser +from django.template.response import TemplateResponse +from django.test import TestCase +from django.test.client import RequestFactory + +from bookwyrm import forms, models, views + + +# pylint: disable=too-many-public-methods +@patch("bookwyrm.suggested_users.rerank_suggestions_task.delay") +@patch("bookwyrm.activitystreams.populate_stream_task.delay") +class LoginViews(TestCase): + """login and password 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" + ): + self.local_user = models.User.objects.create_user( + "mouse@your.domain.here", + "mouse@mouse.com", + "password", + local=True, + localname="mouse", + ) + 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, *_): + """there are so many views, this just makes sure it LOADS""" + login = views.Login.as_view() + request = self.factory.get("") + request.user = self.anonymous_user + + result = login(request) + self.assertIsInstance(result, TemplateResponse) + result.render() + self.assertEqual(result.status_code, 200) + + request.user = self.local_user + result = login(request) + self.assertEqual(result.url, "/") + self.assertEqual(result.status_code, 302) + + def test_login_post_localname(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Login.as_view() + form = forms.LoginForm() + form.data["localname"] = "mouse@mouse.com" + form.data["password"] = "password" + request = self.factory.post("", form.data) + request.user = self.anonymous_user + + with patch("bookwyrm.views.login.login"): + result = view(request) + self.assertEqual(result.url, "/") + self.assertEqual(result.status_code, 302) + + def test_login_post_username(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Login.as_view() + form = forms.LoginForm() + form.data["localname"] = "mouse@your.domain.here" + form.data["password"] = "password" + request = self.factory.post("", form.data) + request.user = self.anonymous_user + + with patch("bookwyrm.views.login.login"): + result = view(request) + self.assertEqual(result.url, "/") + self.assertEqual(result.status_code, 302) + + def test_login_post_email(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Login.as_view() + form = forms.LoginForm() + form.data["localname"] = "mouse" + form.data["password"] = "password" + request = self.factory.post("", form.data) + request.user = self.anonymous_user + + with patch("bookwyrm.views.login.login"): + result = view(request) + self.assertEqual(result.url, "/") + self.assertEqual(result.status_code, 302) + + def test_login_post_invalid_credentials(self, *_): + """there are so many views, this just makes sure it LOADS""" + view = views.Login.as_view() + form = forms.LoginForm() + form.data["localname"] = "mouse" + form.data["password"] = "passsword1" + request = self.factory.post("", form.data) + request.user = self.anonymous_user + + with patch("bookwyrm.views.login.login"): + result = view(request) + result.render() + self.assertEqual(result.status_code, 200) + self.assertEqual( + result.context_data["login_form"].non_field_errors, + "Username or password are incorrect", + ) diff --git a/bookwyrm/tests/views/test_authentication.py b/bookwyrm/tests/views/test_register.py similarity index 76% rename from bookwyrm/tests/views/test_authentication.py rename to bookwyrm/tests/views/test_register.py index 74f3c0902..45e748807 100644 --- a/bookwyrm/tests/views/test_authentication.py +++ b/bookwyrm/tests/views/test_register.py @@ -8,14 +8,14 @@ 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 import models, views from bookwyrm.settings import DOMAIN # pylint: disable=too-many-public-methods @patch("bookwyrm.suggested_users.rerank_suggestions_task.delay") @patch("bookwyrm.activitystreams.populate_stream_task.delay") -class AuthenticationViews(TestCase): +class RegisterViews(TestCase): """login and password management""" def setUp(self): @@ -38,82 +38,6 @@ class AuthenticationViews(TestCase): id=1, require_confirm_email=False ) - def test_login_get(self, *_): - """there are so many views, this just makes sure it LOADS""" - login = views.Login.as_view() - request = self.factory.get("") - request.user = self.anonymous_user - - result = login(request) - self.assertIsInstance(result, TemplateResponse) - result.render() - self.assertEqual(result.status_code, 200) - - request.user = self.local_user - result = login(request) - self.assertEqual(result.url, "/") - self.assertEqual(result.status_code, 302) - - def test_login_post_localname(self, *_): - """there are so many views, this just makes sure it LOADS""" - view = views.Login.as_view() - form = forms.LoginForm() - form.data["localname"] = "mouse@mouse.com" - form.data["password"] = "password" - request = self.factory.post("", form.data) - request.user = self.anonymous_user - - with patch("bookwyrm.views.login.login"): - result = view(request) - self.assertEqual(result.url, "/") - self.assertEqual(result.status_code, 302) - - def test_login_post_username(self, *_): - """there are so many views, this just makes sure it LOADS""" - view = views.Login.as_view() - form = forms.LoginForm() - form.data["localname"] = "mouse@your.domain.here" - form.data["password"] = "password" - request = self.factory.post("", form.data) - request.user = self.anonymous_user - - with patch("bookwyrm.views.login.login"): - result = view(request) - self.assertEqual(result.url, "/") - self.assertEqual(result.status_code, 302) - - def test_login_post_email(self, *_): - """there are so many views, this just makes sure it LOADS""" - view = views.Login.as_view() - form = forms.LoginForm() - form.data["localname"] = "mouse" - form.data["password"] = "password" - request = self.factory.post("", form.data) - request.user = self.anonymous_user - - with patch("bookwyrm.views.login.login"): - result = view(request) - self.assertEqual(result.url, "/") - self.assertEqual(result.status_code, 302) - - def test_login_post_invalid_credentials(self, *_): - """there are so many views, this just makes sure it LOADS""" - view = views.Login.as_view() - form = forms.LoginForm() - form.data["localname"] = "mouse" - form.data["password"] = "passsword1" - request = self.factory.post("", form.data) - request.user = self.anonymous_user - - with patch("bookwyrm.views.login.login"): - result = view(request) - result.render() - self.assertEqual(result.status_code, 200) - self.assertEqual( - result.context_data["login_form"].non_field_errors, - "Username or password are incorrect", - ) - def test_register(self, *_): """create a user""" view = views.Register.as_view() @@ -126,7 +50,7 @@ class AuthenticationViews(TestCase): "email": "aa@bb.cccc", }, ) - with patch("bookwyrm.views.login.login"): + with patch("bookwyrm.views.register.login"): response = view(request) self.assertEqual(models.User.objects.count(), 2) self.assertEqual(response.status_code, 302) @@ -151,7 +75,7 @@ class AuthenticationViews(TestCase): "email": "aa@bb.cccc", }, ) - with patch("bookwyrm.views.login.login"): + with patch("bookwyrm.views.register.login"): response = view(request) self.assertEqual(response.status_code, 302) nutria = models.User.objects.get(localname="nutria") @@ -169,7 +93,7 @@ class AuthenticationViews(TestCase): "register/", {"localname": "nutria ", "password": "mouseword", "email": "aa@bb.ccc"}, ) - with patch("bookwyrm.views.login.login"): + with patch("bookwyrm.views.register.login"): response = view(request) self.assertEqual(models.User.objects.count(), 2) self.assertEqual(response.status_code, 302) @@ -248,7 +172,7 @@ class AuthenticationViews(TestCase): "invite_code": "testcode", }, ) - with patch("bookwyrm.views.login.login"): + with patch("bookwyrm.views.register.login"): response = view(request) self.assertEqual(models.User.objects.count(), 2) self.assertEqual(response.status_code, 302) diff --git a/bookwyrm/views/login.py b/bookwyrm/views/login.py index 1ca65f2ff..97d541690 100644 --- a/bookwyrm/views/login.py +++ b/bookwyrm/views/login.py @@ -32,7 +32,7 @@ class Login(View): return TemplateResponse(request, "login.html", data) @sensitive_variables("password") - @sensitive_post_parameters("password") + @method_decorator(sensitive_post_parameters("password")) def post(self, request): """authentication action""" if request.user.is_authenticated: diff --git a/bookwyrm/views/register.py b/bookwyrm/views/register.py index 1ffa16ec6..1ecb97b16 100644 --- a/bookwyrm/views/register.py +++ b/bookwyrm/views/register.py @@ -3,6 +3,7 @@ from django.contrib.auth import login from django.core.exceptions import PermissionDenied from django.shortcuts import get_object_or_404, redirect from django.template.response import TemplateResponse +from django.utils.decorators import method_decorator from django.views import View from django.views.decorators.http import require_POST from django.views.decorators.debug import sensitive_variables, sensitive_post_parameters @@ -16,7 +17,7 @@ class Register(View): """register a user""" @sensitive_variables("password") - @sensitive_post_parameters("password") + @method_decorator(sensitive_post_parameters("password")) def post(self, request): """join the server""" settings = models.SiteSettings.get()