From f44abfecfb551e1848e1d5f5301b61b7f9ec06c1 Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Sat, 21 Sep 2019 16:11:08 +0200 Subject: [PATCH] Fix #883: Prevent usage of too weak passwords --- api/config/settings/common.py | 11 +++++++ api/funkwhale_api/users/serializers.py | 11 ++++++- api/tests/users/test_admin.py | 12 +++++-- api/tests/users/test_serializers.py | 44 ++++++++++++++++++++++++++ api/tests/users/test_views.py | 25 +++++++++------ changes/changelog.d/883.enhancement | 1 + 6 files changed, 91 insertions(+), 13 deletions(-) create mode 100644 api/tests/users/test_serializers.py create mode 100644 changes/changelog.d/883.enhancement diff --git a/api/config/settings/common.py b/api/config/settings/common.py index c11d847e4..251167f4e 100644 --- a/api/config/settings/common.py +++ b/api/config/settings/common.py @@ -583,6 +583,17 @@ JWT_AUTH = { "JWT_GET_USER_SECRET_KEY": get_user_secret_key, } OLD_PASSWORD_FIELD_ENABLED = True +AUTH_PASSWORD_VALIDATORS = [ + { + "NAME": "django.contrib.auth.password_validation.UserAttributeSimilarityValidator" + }, + { + "NAME": "django.contrib.auth.password_validation.MinimumLengthValidator", + "OPTIONS": {"min_length": env.int("PASSWORD_MIN_LENGTH", default=8)}, + }, + {"NAME": "django.contrib.auth.password_validation.CommonPasswordValidator"}, + {"NAME": "django.contrib.auth.password_validation.NumericPasswordValidator"}, +] ACCOUNT_ADAPTER = "funkwhale_api.users.adapters.FunkwhaleAccountAdapter" CORS_ORIGIN_ALLOW_ALL = True # CORS_ORIGIN_WHITELIST = ( diff --git a/api/funkwhale_api/users/serializers.py b/api/funkwhale_api/users/serializers.py index a45d414b4..9db081378 100644 --- a/api/funkwhale_api/users/serializers.py +++ b/api/funkwhale_api/users/serializers.py @@ -5,7 +5,7 @@ from django.utils.deconstruct import deconstructible from django.utils.translation import gettext_lazy as _ from rest_auth.serializers import PasswordResetSerializer as PRS -from rest_auth.registration.serializers import RegisterSerializer as RS +from rest_auth.registration.serializers import RegisterSerializer as RS, get_adapter from rest_framework import serializers from versatileimagefield.serializers import VersatileImageFieldSerializer @@ -42,6 +42,15 @@ class RegisterSerializer(RS): except models.Invitation.DoesNotExist: raise serializers.ValidationError("Invalid invitation code") + def validate(self, validated_data): + data = super().validate(validated_data) + # we create a fake user obj with validated data so we can validate + # password properly (we have a password validator that requires + # a user object) + user = models.User(username=data["username"], email=data["email"]) + get_adapter().clean_password(data["password1"], user) + return data + def save(self, request): user = super().save(request) if self.validated_data.get("invitation"): diff --git a/api/tests/users/test_admin.py b/api/tests/users/test_admin.py index 03b316eb0..0af38c0ee 100644 --- a/api/tests/users/test_admin.py +++ b/api/tests/users/test_admin.py @@ -4,7 +4,11 @@ from funkwhale_api.users.admin import MyUserCreationForm def test_clean_username_success(db): # Instantiate the form with a new username form = MyUserCreationForm( - {"username": "alamode", "password1": "123456", "password2": "123456"} + { + "username": "alamode", + "password1": "thisismypassword", + "password2": "thisismypassword", + } ) # Run is_valid() to trigger the validation valid = form.is_valid() @@ -19,7 +23,11 @@ def test_clean_username_false(factories): user = factories["users.User"]() # Instantiate the form with the same username as self.user form = MyUserCreationForm( - {"username": user.username, "password1": "123456", "password2": "123456"} + { + "username": user.username, + "password1": "thisismypassword", + "password2": "thisismypassword", + } ) # Run is_valid() to trigger the validation, which is going to fail # because the username is already taken diff --git a/api/tests/users/test_serializers.py b/api/tests/users/test_serializers.py new file mode 100644 index 000000000..02d1ac052 --- /dev/null +++ b/api/tests/users/test_serializers.py @@ -0,0 +1,44 @@ +import pytest + +from funkwhale_api.users import serializers + + +@pytest.mark.parametrize( + "data, expected_error", + [ + ( + { + "username": "myusername", + "email": "test@hello.com", + "password1": "myusername", + }, + r".*password is too similar.*", + ), + ( + {"username": "myusername", "email": "test@hello.com", "password1": "test"}, + r".*must contain at least 8 characters.*", + ), + ( + { + "username": "myusername", + "email": "test@hello.com", + "password1": "superman", + }, + r".*password is too common.*", + ), + ( + { + "username": "myusername", + "email": "test@hello.com", + "password1": "123457809878", + }, + r".*password is entirely numeric.*", + ), + ], +) +def test_registration_serializer_validates_password_properly(data, expected_error, db): + data["password2"] = data["password1"] + serializer = serializers.RegisterSerializer(data=data) + + with pytest.raises(serializers.serializers.ValidationError, match=expected_error): + serializer.is_valid(raise_exception=True) diff --git a/api/tests/users/test_views.py b/api/tests/users/test_views.py index 956a7178c..db8f870f3 100644 --- a/api/tests/users/test_views.py +++ b/api/tests/users/test_views.py @@ -9,8 +9,8 @@ def test_can_create_user_via_api(preferences, api_client, db): data = { "username": "test1", "email": "test1@test.com", - "password1": "testtest", - "password2": "testtest", + "password1": "thisismypassword", + "password2": "thisismypassword", } preferences["users__registration_enabled"] = True response = api_client.post(url, data) @@ -72,8 +72,8 @@ def test_can_signup_with_invitation(preferences, factories, api_client): data = { "username": "test1", "email": "test1@test.com", - "password1": "testtest", - "password2": "testtest", + "password1": "thisismypassword", + "password2": "thisismypassword", "invitation": "hello", } preferences["users__registration_enabled"] = False @@ -157,11 +157,16 @@ def test_changing_password_updates_secret_key(logged_in_api_client): user = logged_in_api_client.user password = user.password secret_key = user.secret_key - payload = {"old_password": "test", "new_password1": "new", "new_password2": "new"} + payload = { + "old_password": "test", + "new_password1": "thisismypassword", + "new_password2": "thisismypassword", + } url = reverse("change_password") - logged_in_api_client.post(url, payload) + response = logged_in_api_client.post(url, payload) + assert response.status_code == 200 user.refresh_from_db() assert user.secret_key != secret_key @@ -295,8 +300,8 @@ def test_creating_user_creates_actor_as_well( data = { "username": "test1", "email": "test1@test.com", - "password1": "testtest", - "password2": "testtest", + "password1": "thisismypassword", + "password2": "thisismypassword", } preferences["users__registration_enabled"] = True mocker.patch("funkwhale_api.users.models.create_actor", return_value=actor) @@ -316,8 +321,8 @@ def test_creating_user_sends_confirmation_email( data = { "username": "test1", "email": "test1@test.com", - "password1": "testtest", - "password2": "testtest", + "password1": "thisismypassword", + "password2": "thisismypassword", } preferences["users__registration_enabled"] = True preferences["instance__name"] = "Hello world" diff --git a/changes/changelog.d/883.enhancement b/changes/changelog.d/883.enhancement new file mode 100644 index 000000000..00284a309 --- /dev/null +++ b/changes/changelog.d/883.enhancement @@ -0,0 +1 @@ +Prevent usage of too weak passwords (#883)