From ed6b15bfa9260458fde59fec1d22eee099942c94 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C4=93teris=20Caune?= Date: Mon, 16 Nov 2020 14:53:50 +0200 Subject: [PATCH] Update the "Set Password" function to use confirmation codes --- CHANGELOG.md | 2 ++ hc/accounts/models.py | 6 ---- hc/accounts/tests/test_add_credential.py | 14 +++----- hc/accounts/tests/test_profile.py | 17 --------- hc/accounts/tests/test_remove_credential.py | 17 +++++---- hc/accounts/tests/test_set_password.py | 36 ++++++++------------ hc/accounts/urls.py | 2 +- hc/accounts/views.py | 9 ++--- hc/lib/emails.py | 4 --- hc/test.py | 6 ++++ templates/accounts/profile.html | 7 ++-- templates/accounts/sudo.html | 3 +- templates/emails/set-password-body-html.html | 13 ------- templates/emails/set-password-body-text.html | 11 ------ templates/emails/set-password-subject.html | 2 -- 15 files changed, 42 insertions(+), 107 deletions(-) delete mode 100644 templates/emails/set-password-body-html.html delete mode 100644 templates/emails/set-password-body-text.html delete mode 100644 templates/emails/set-password-subject.html diff --git a/CHANGELOG.md b/CHANGELOG.md index 5b02a0ed..3b9ce26d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ All notable changes to this project will be documented in this file. - Improve phone number sanitization: remove spaces and hyphens - Change the "Test Integration" behavior for webhooks: don't retry failed requests - Add retries to the the email sending logic +- Require confirmation codes (sent to email) before sensitive actions +- Implement Webauthn two-factor authentication ## v1.17.0 - 2020-10-14 diff --git a/hc/accounts/models.py b/hc/accounts/models.py index 3ded1d75..cc239e30 100644 --- a/hc/accounts/models.py +++ b/hc/accounts/models.py @@ -118,12 +118,6 @@ class Profile(models.Model): } emails.transfer_request(self.user.email, ctx) - def send_set_password_link(self): - token = self.prepare_token("set-password") - path = reverse("hc-set-password", args=[token]) - ctx = {"button_text": "Set Password", "button_url": settings.SITE_ROOT + path} - emails.set_password(self.user.email, ctx) - def send_change_email_link(self): token = self.prepare_token("change-email") path = reverse("hc-change-email", args=[token]) diff --git a/hc/accounts/tests/test_add_credential.py b/hc/accounts/tests/test_add_credential.py index aa9f9f59..380be823 100644 --- a/hc/accounts/tests/test_add_credential.py +++ b/hc/accounts/tests/test_add_credential.py @@ -1,6 +1,5 @@ from unittest.mock import patch -from django.core.signing import TimestampSigner from hc.test import BaseTestCase from hc.accounts.models import Credential @@ -11,11 +10,6 @@ class AddCredentialTestCase(BaseTestCase): self.url = "/accounts/two_factor/add/" - def _set_sudo_flag(self): - session = self.client.session - session["sudo"] = TimestampSigner().sign("active") - session.save() - def test_it_requires_sudo_mode(self): self.client.login(username="alice@example.org", password="password") @@ -24,7 +18,7 @@ class AddCredentialTestCase(BaseTestCase): def test_it_shows_form(self): self.client.login(username="alice@example.org", password="password") - self._set_sudo_flag() + self.set_sudo_flag() r = self.client.get(self.url) self.assertContains(r, "Add Security Key") @@ -37,7 +31,7 @@ class AddCredentialTestCase(BaseTestCase): mock_get_credential_data.return_value = b"dummy-credential-data" self.client.login(username="alice@example.org", password="password") - self._set_sudo_flag() + self.set_sudo_flag() payload = { "name": "My New Key", @@ -54,7 +48,7 @@ class AddCredentialTestCase(BaseTestCase): def test_it_rejects_bad_base64(self): self.client.login(username="alice@example.org", password="password") - self._set_sudo_flag() + self.set_sudo_flag() payload = { "name": "My New Key", @@ -67,7 +61,7 @@ class AddCredentialTestCase(BaseTestCase): def test_it_requires_client_data_json(self): self.client.login(username="alice@example.org", password="password") - self._set_sudo_flag() + self.set_sudo_flag() payload = { "name": "My New Key", diff --git a/hc/accounts/tests/test_profile.py b/hc/accounts/tests/test_profile.py index 1cc6cb0c..f9c8071c 100644 --- a/hc/accounts/tests/test_profile.py +++ b/hc/accounts/tests/test_profile.py @@ -9,23 +9,6 @@ from hc.api.models import Check class ProfileTestCase(BaseTestCase): - def test_it_sends_set_password_link(self): - self.client.login(username="alice@example.org", password="password") - - form = {"set_password": "1"} - r = self.client.post("/accounts/profile/", form) - assert r.status_code == 302 - - # profile.token should be set now - self.profile.refresh_from_db() - token = self.profile.token - self.assertTrue(len(token) > 10) - - # And an email should have been sent - self.assertEqual(len(mail.outbox), 1) - expected_subject = "Set password on %s" % settings.SITE_NAME - self.assertEqual(mail.outbox[0].subject, expected_subject) - def test_it_sends_report(self): check = Check(project=self.project, name="Test Check") check.last_ping = now() diff --git a/hc/accounts/tests/test_remove_credential.py b/hc/accounts/tests/test_remove_credential.py index e8c82546..c7453284 100644 --- a/hc/accounts/tests/test_remove_credential.py +++ b/hc/accounts/tests/test_remove_credential.py @@ -1,5 +1,3 @@ -from django.core.signing import TimestampSigner - from hc.test import BaseTestCase from hc.accounts.models import Credential @@ -11,14 +9,15 @@ class RemoveCredentialTestCase(BaseTestCase): self.c = Credential.objects.create(user=self.alice, name="Alices Key") self.url = f"/accounts/two_factor/{self.c.code}/remove/" - def _set_sudo_flag(self): - session = self.client.session - session["sudo"] = TimestampSigner().sign("active") - session.save() + def test_it_requires_sudo_mode(self): + self.client.login(username="alice@example.org", password="password") + + r = self.client.get(self.url) + self.assertContains(r, "We have sent a confirmation code") def test_it_shows_form(self): self.client.login(username="alice@example.org", password="password") - self._set_sudo_flag() + self.set_sudo_flag() r = self.client.get(self.url) self.assertContains(r, "Remove Security Key") @@ -26,7 +25,7 @@ class RemoveCredentialTestCase(BaseTestCase): def test_it_removes_credential(self): self.client.login(username="alice@example.org", password="password") - self._set_sudo_flag() + self.set_sudo_flag() r = self.client.post(self.url, {"remove_credential": ""}, follow=True) self.assertRedirects(r, "/accounts/profile/") @@ -36,7 +35,7 @@ class RemoveCredentialTestCase(BaseTestCase): def test_it_checks_owner(self): self.client.login(username="charlie@example.org", password="password") - self._set_sudo_flag() + self.set_sudo_flag() r = self.client.post(self.url, {"remove_credential": ""}) self.assertEqual(r.status_code, 400) diff --git a/hc/accounts/tests/test_set_password.py b/hc/accounts/tests/test_set_password.py index a53cb354..54895493 100644 --- a/hc/accounts/tests/test_set_password.py +++ b/hc/accounts/tests/test_set_password.py @@ -2,45 +2,37 @@ from hc.test import BaseTestCase class SetPasswordTestCase(BaseTestCase): - def test_it_shows_form(self): - token = self.profile.prepare_token("set-password") - + def test_it_requires_sudo_mod(self): self.client.login(username="alice@example.org", password="password") - r = self.client.get("/accounts/set_password/%s/" % token) - self.assertEqual(r.status_code, 200) - self.assertContains(r, "Please pick a password") + r = self.client.get("/accounts/set_password/") + self.assertContains(r, "We have sent a confirmation code") - def test_it_checks_token(self): - self.profile.prepare_token("set-password") + def test_it_shows_form(self): self.client.login(username="alice@example.org", password="password") + self.set_sudo_flag() - # GET - r = self.client.get("/accounts/set_password/invalid-token/") - self.assertEqual(r.status_code, 400) - - # POST - r = self.client.post("/accounts/set_password/invalid-token/") - self.assertEqual(r.status_code, 400) + r = self.client.get("/accounts/set_password/") + self.assertContains(r, "Please pick a password") def test_it_sets_password(self): - token = self.profile.prepare_token("set-password") - self.client.login(username="alice@example.org", password="password") + self.set_sudo_flag() + payload = {"password": "correct horse battery staple"} - r = self.client.post("/accounts/set_password/%s/" % token, payload) - self.assertEqual(r.status_code, 302) + r = self.client.post("/accounts/set_password/", payload) + self.assertRedirects(r, "/accounts/profile/") old_password = self.alice.password self.alice.refresh_from_db() self.assertNotEqual(self.alice.password, old_password) def test_post_checks_length(self): - token = self.profile.prepare_token("set-password") - self.client.login(username="alice@example.org", password="password") + self.set_sudo_flag() + payload = {"password": "abc"} - r = self.client.post("/accounts/set_password/%s/" % token, payload) + r = self.client.post("/accounts/set_password/", payload) self.assertEqual(r.status_code, 200) old_password = self.alice.password diff --git a/hc/accounts/urls.py b/hc/accounts/urls.py index 35e8b2e5..7d9f72ab 100644 --- a/hc/accounts/urls.py +++ b/hc/accounts/urls.py @@ -21,7 +21,7 @@ urlpatterns = [ views.unsubscribe_reports, name="hc-unsubscribe-reports", ), - path("set_password//", views.set_password, name="hc-set-password"), + path("set_password/", views.set_password, name="hc-set-password"), path("change_email/done/", views.change_email_done, name="hc-change-email-done"), path("change_email//", views.change_email, name="hc-change-email"), path("two_factor/add/", views.add_credential, name="hc-add-credential"), diff --git a/hc/accounts/views.py b/hc/accounts/views.py index 666a16d1..97625a8c 100644 --- a/hc/accounts/views.py +++ b/hc/accounts/views.py @@ -239,9 +239,6 @@ def profile(request): if "change_email" in request.POST: profile.send_change_email_link() return redirect("hc-link-sent") - elif "set_password" in request.POST: - profile.send_set_password_link() - return redirect("hc-link-sent") elif "leave_project" in request.POST: code = request.POST["code"] try: @@ -466,10 +463,8 @@ def notifications(request): @login_required -def set_password(request, token): - if not request.profile.check_token(token, "set-password"): - return HttpResponseBadRequest() - +@require_sudo_mode +def set_password(request): if request.method == "POST": form = forms.SetPasswordForm(request.POST) if form.is_valid(): diff --git a/hc/lib/emails.py b/hc/lib/emails.py index 79bbcf0d..41459ca2 100644 --- a/hc/lib/emails.py +++ b/hc/lib/emails.py @@ -62,10 +62,6 @@ def transfer_request(to, ctx): send("transfer-request", to, ctx) -def set_password(to, ctx): - send("set-password", to, ctx) - - def change_email(to, ctx): send("change-email", to, ctx) diff --git a/hc/test.py b/hc/test.py index d4b97767..c5531755 100644 --- a/hc/test.py +++ b/hc/test.py @@ -1,4 +1,5 @@ from django.contrib.auth.models import User +from django.core.signing import TimestampSigner from django.test import TestCase from hc.accounts.models import Member, Profile, Project @@ -51,3 +52,8 @@ class BaseTestCase(TestCase): self.charlies_profile.save() self.channels_url = "/projects/%s/integrations/" % self.project.code + + def set_sudo_flag(self): + session = self.client.session + session["sudo"] = TimestampSigner().sign("active") + session.save() diff --git a/templates/accounts/profile.html b/templates/accounts/profile.html index b842b035..fa52129b 100644 --- a/templates/accounts/profile.html +++ b/templates/accounts/profile.html @@ -50,10 +50,9 @@

Attach a password to your {{ site_name }} account - + Set Password

diff --git a/templates/accounts/sudo.html b/templates/accounts/sudo.html index 6a9a853b..82378f7a 100644 --- a/templates/accounts/sudo.html +++ b/templates/accounts/sudo.html @@ -20,7 +20,8 @@ type="text" class="form-control input-lg" maxlength="6" - name="sudo_code" /> + name="sudo_code" + autofocus /> {% if wrong_code %}
diff --git a/templates/emails/set-password-body-html.html b/templates/emails/set-password-body-html.html deleted file mode 100644 index 9e76be04..00000000 --- a/templates/emails/set-password-body-html.html +++ /dev/null @@ -1,13 +0,0 @@ -{% extends "emails/base.html" %} -{% load hc_extras %} - -{% block content %} -Hello,
-To set up a password for your account on {% site_name %}, please press the -button below:

-{% endblock %} - -{% block content_more %} -Regards,
-The {% site_name %} Team -{% endblock %} diff --git a/templates/emails/set-password-body-text.html b/templates/emails/set-password-body-text.html deleted file mode 100644 index c5fc3d06..00000000 --- a/templates/emails/set-password-body-text.html +++ /dev/null @@ -1,11 +0,0 @@ -{% load hc_extras %} -Hello, - -Here's a link to set a password for your account on {% site_name %}: - -{{ button_url }} - - --- -Regards, -{% site_name %} diff --git a/templates/emails/set-password-subject.html b/templates/emails/set-password-subject.html deleted file mode 100644 index db5267fb..00000000 --- a/templates/emails/set-password-subject.html +++ /dev/null @@ -1,2 +0,0 @@ -{% load hc_extras %} -Set password on {% site_name %}