From 93f2c9f83c0f35300ce48ae2a5e5c541d08dfbeb Mon Sep 17 00:00:00 2001 From: Eliot Berriot Date: Wed, 1 Apr 2020 14:34:56 +0200 Subject: [PATCH] Fix #1039: setting to enforce email signup verification --- api/config/settings/common.py | 9 ++- api/funkwhale_api/common/authentication.py | 52 +++++++++++++++- api/funkwhale_api/subsonic/authentication.py | 19 ++++-- api/funkwhale_api/users/adapters.py | 3 + api/funkwhale_api/users/auth_backends.py | 45 ++++++++++++-- api/funkwhale_api/users/factories.py | 28 +++++++++ api/funkwhale_api/users/models.py | 25 +++++++- api/funkwhale_api/users/oauth/server.py | 16 +++++ api/tests/common/test_authentication.py | 62 ++++++++++++++++++++ api/tests/users/oauth/test_views.py | 31 ++++++++++ api/tests/users/test_models.py | 21 +++++++ api/tests/users/test_views.py | 34 +++++++++++ changes/1039.feature | 1 + changes/notes.rst | 12 +++- docs/admin/configuration.rst | 29 ++++++--- front/src/components/auth/LoginForm.vue | 8 +-- 16 files changed, 365 insertions(+), 30 deletions(-) create mode 100644 api/tests/common/test_authentication.py create mode 100644 changes/1039.feature diff --git a/api/config/settings/common.py b/api/config/settings/common.py index c4a7e1ef8..fe68a2746 100644 --- a/api/config/settings/common.py +++ b/api/config/settings/common.py @@ -431,13 +431,18 @@ SECURE_CONTENT_TYPE_NOSNIFF = True # ------------------------------------------------------------------------------ AUTHENTICATION_BACKENDS = ( "funkwhale_api.users.auth_backends.ModelBackend", - "allauth.account.auth_backends.AuthenticationBackend", + "funkwhale_api.users.auth_backends.AllAuthBackend", ) SESSION_COOKIE_HTTPONLY = False # Some really nice defaults ACCOUNT_AUTHENTICATION_METHOD = "username_email" ACCOUNT_EMAIL_REQUIRED = True -ACCOUNT_EMAIL_VERIFICATION = "mandatory" +ACCOUNT_EMAIL_VERIFICATION_ENFORCE = env.bool( + "ACCOUNT_EMAIL_VERIFICATION_ENFORCE", default=False +) +ACCOUNT_EMAIL_VERIFICATION = ( + "mandatory" if ACCOUNT_EMAIL_VERIFICATION_ENFORCE else "optional" +) ACCOUNT_USERNAME_VALIDATORS = "funkwhale_api.users.serializers.username_validators" # Custom user app defaults diff --git a/api/funkwhale_api/common/authentication.py b/api/funkwhale_api/common/authentication.py index 415b84cb2..6d2b433bc 100644 --- a/api/funkwhale_api/common/authentication.py +++ b/api/funkwhale_api/common/authentication.py @@ -1,3 +1,4 @@ +from django.conf import settings from django.utils.encoding import smart_text from django.utils.translation import ugettext as _ from rest_framework import exceptions @@ -5,7 +6,48 @@ from rest_framework_jwt import authentication from rest_framework_jwt.settings import api_settings -class JSONWebTokenAuthenticationQS(authentication.BaseJSONWebTokenAuthentication): +def should_verify_email(user): + if user.is_superuser: + return False + has_unverified_email = not user.has_verified_primary_email + mandatory_verification = settings.ACCOUNT_EMAIL_VERIFICATION != "optional" + return has_unverified_email and mandatory_verification + + +class BaseJsonWebTokenAuth(object): + def authenticate_credentials(self, payload): + """ + We have to implement this method by hand to ensure we can check that the + User has a verified email, if required + """ + User = authentication.get_user_model() + username = authentication.jwt_get_username_from_payload(payload) + + if not username: + msg = _("Invalid payload.") + raise exceptions.AuthenticationFailed(msg) + + try: + user = User.objects.get_by_natural_key(username) + except User.DoesNotExist: + msg = _("Invalid signature.") + raise exceptions.AuthenticationFailed(msg) + + if not user.is_active: + msg = _("User account is disabled.") + raise exceptions.AuthenticationFailed(msg) + + if should_verify_email(user): + + msg = _("You need to verify your email address.") + raise exceptions.AuthenticationFailed(msg) + + return user + + +class JSONWebTokenAuthenticationQS( + BaseJsonWebTokenAuth, authentication.BaseJSONWebTokenAuthentication +): www_authenticate_realm = "api" @@ -22,7 +64,9 @@ class JSONWebTokenAuthenticationQS(authentication.BaseJSONWebTokenAuthentication ) -class BearerTokenHeaderAuth(authentication.BaseJSONWebTokenAuthentication): +class BearerTokenHeaderAuth( + BaseJsonWebTokenAuth, authentication.BaseJSONWebTokenAuthentication +): """ For backward compatibility purpose, we used Authorization: JWT but Authorization: Bearer is probably better. @@ -65,7 +109,9 @@ class BearerTokenHeaderAuth(authentication.BaseJSONWebTokenAuthentication): return auth -class JSONWebTokenAuthentication(authentication.JSONWebTokenAuthentication): +class JSONWebTokenAuthentication( + BaseJsonWebTokenAuth, authentication.JSONWebTokenAuthentication +): def authenticate(self, request): auth = super().authenticate(request) diff --git a/api/funkwhale_api/subsonic/authentication.py b/api/funkwhale_api/subsonic/authentication.py index 2d1e04f17..dd6a60f42 100644 --- a/api/funkwhale_api/subsonic/authentication.py +++ b/api/funkwhale_api/subsonic/authentication.py @@ -3,6 +3,7 @@ import hashlib from rest_framework import authentication, exceptions +from funkwhale_api.common import authentication as common_authentication from funkwhale_api.users.models import User @@ -18,19 +19,26 @@ def authenticate(username, password): if password.startswith("enc:"): password = password.replace("enc:", "", 1) password = binascii.unhexlify(password).decode("utf-8") - user = User.objects.select_related("actor").get( - username__iexact=username, is_active=True, subsonic_api_token=password + user = ( + User.objects.all() + .for_auth() + .get(username__iexact=username, is_active=True, subsonic_api_token=password) ) except (User.DoesNotExist, binascii.Error): raise exceptions.AuthenticationFailed("Wrong username or password.") + if common_authentication.should_verify_email(user): + raise exceptions.AuthenticationFailed("You need to verify your email.") + return (user, None) def authenticate_salt(username, salt, token): try: - user = User.objects.select_related("actor").get( - username=username, is_active=True, subsonic_api_token__isnull=False + user = ( + User.objects.all() + .for_auth() + .get(username=username, is_active=True, subsonic_api_token__isnull=False) ) except User.DoesNotExist: raise exceptions.AuthenticationFailed("Wrong username or password.") @@ -38,6 +46,9 @@ def authenticate_salt(username, salt, token): if expected != token: raise exceptions.AuthenticationFailed("Wrong username or password.") + if common_authentication.should_verify_email(user): + raise exceptions.AuthenticationFailed("You need to verify your email.") + return (user, None) diff --git a/api/funkwhale_api/users/adapters.py b/api/funkwhale_api/users/adapters.py index 77171ff1b..c44a7ce9f 100644 --- a/api/funkwhale_api/users/adapters.py +++ b/api/funkwhale_api/users/adapters.py @@ -22,3 +22,6 @@ class FunkwhaleAccountAdapter(DefaultAccountAdapter): def send_mail(self, template_prefix, email, context): context.update(get_email_context()) return super().send_mail(template_prefix, email, context) + + def get_login_redirect_url(self, request): + return "noop" diff --git a/api/funkwhale_api/users/auth_backends.py b/api/funkwhale_api/users/auth_backends.py index 404b34f4d..b274bcee2 100644 --- a/api/funkwhale_api/users/auth_backends.py +++ b/api/funkwhale_api/users/auth_backends.py @@ -1,4 +1,33 @@ from django.contrib.auth import backends, get_user_model +from allauth.account import auth_backends + +from funkwhale_api.common import authentication + + +# ugly but allauth doesn't offer an easy way to override the querysets +# used to retrieve users, so we monkey patch +def decorate_for_auth(func): + def inner(*args, **kwargs): + qs = func(*args, **kwargs) + try: + return qs.for_auth() + except AttributeError: + return ( + get_user_model() + .objects.all() + .for_auth() + .filter(pk__in=[u.pk for u in qs]) + ) + + return inner + + +auth_backends.filter_users_by_email = decorate_for_auth( + auth_backends.filter_users_by_email +) +auth_backends.filter_users_by_username = decorate_for_auth( + auth_backends.filter_users_by_username +) class ModelBackend(backends.ModelBackend): @@ -7,11 +36,17 @@ class ModelBackend(backends.ModelBackend): Select related to avoid two additional queries """ try: - user = ( - get_user_model() - ._default_manager.select_related("actor__domain") - .get(pk=user_id) - ) + user = get_user_model().objects.all().for_auth().get(pk=user_id) except get_user_model().DoesNotExist: return None + return user if self.user_can_authenticate(user) else None + + def user_can_authenticate(self, user): + return super().user_can_authenticate( + user + ) and not authentication.should_verify_email(user) + + +class AllAuthBackend(auth_backends.AuthenticationBackend, ModelBackend): + pass diff --git a/api/funkwhale_api/users/factories.py b/api/funkwhale_api/users/factories.py index 0588fa07c..ea079f47b 100644 --- a/api/funkwhale_api/users/factories.py +++ b/api/funkwhale_api/users/factories.py @@ -31,6 +31,23 @@ class GroupFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): self.permissions.add(*perms) +@registry.register +class EmailAddressFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): + verified = False + primary = True + + class Meta: + model = "account.EmailAddress" + + +@registry.register +class EmailConfirmationFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): + email_address = factory.SubFactory(EmailAddressFactory) + + class Meta: + model = "account.EmailConfirmation" + + @registry.register class InvitationFactory(NoUpdateOnCreate, factory.django.DjangoModelFactory): owner = factory.LazyFunction(lambda: UserFactory()) @@ -91,6 +108,16 @@ class UserFactory(factory.django.DjangoModelFactory): self.save(update_fields=["actor"]) return self.actor + @factory.post_generation + def verified_email(self, create, extracted, **kwargs): + if not create or extracted is None: + return + return EmailConfirmationFactory( + email_address__verified=extracted, + email_address__email=self.email, + email_address__user=self, + ) + @registry.register(name="users.SuperUser") class SuperUserFactory(UserFactory): @@ -129,6 +156,7 @@ class AccessTokenFactory(factory.django.DjangoModelFactory): user = factory.SubFactory(UserFactory) expires = factory.Faker("future_datetime", tzinfo=pytz.UTC) token = factory.Faker("uuid4") + scope = "read" class Meta: model = "users.AccessToken" diff --git a/api/funkwhale_api/users/models.py b/api/funkwhale_api/users/models.py index db30199c6..914fd6e63 100644 --- a/api/funkwhale_api/users/models.py +++ b/api/funkwhale_api/users/models.py @@ -9,13 +9,14 @@ import string import uuid from django.conf import settings -from django.contrib.auth.models import AbstractUser +from django.contrib.auth.models import AbstractUser, UserManager as BaseUserManager from django.db import models from django.dispatch import receiver from django.urls import reverse from django.utils import timezone from django.utils.translation import ugettext_lazy as _ +from allauth.account.models import EmailAddress from django_auth_ldap.backend import populate_user as ldap_populate_user from oauth2_provider import models as oauth2_models from oauth2_provider import validators as oauth2_validators @@ -94,6 +95,26 @@ def get_default_funkwhale_support_message_display_date(): ) +class UserQuerySet(models.QuerySet): + def for_auth(self): + """Optimization to avoid additional queries during authentication""" + qs = self.select_related("actor__domain") + verified_emails = EmailAddress.objects.filter( + user=models.OuterRef("id"), primary=True + ).values("verified")[:1] + subquery = models.Subquery(verified_emails) + return qs.annotate(has_verified_primary_email=subquery) + + +class UserManager(BaseUserManager): + def get_queryset(self): + return UserQuerySet(self.model, using=self._db) + + def get_by_natural_key(self, key): + obj = BaseUserManager.get_by_natural_key(self.all().for_auth(), key) + return obj + + class User(AbstractUser): # First Name and Last Name do not cover name patterns @@ -169,6 +190,8 @@ class User(AbstractUser): blank=True, ) + objects = UserManager() + def __str__(self): return self.username diff --git a/api/funkwhale_api/users/oauth/server.py b/api/funkwhale_api/users/oauth/server.py index f62ebf48a..e4f0947ec 100644 --- a/api/funkwhale_api/users/oauth/server.py +++ b/api/funkwhale_api/users/oauth/server.py @@ -1,11 +1,24 @@ import urllib.parse import oauthlib.oauth2 +from funkwhale_api.common import authentication + + +def check(request): + user = request.user + request.user = user.__class__.objects.all().for_auth().get(pk=user.pk) + if authentication.should_verify_email(request.user): + setattr(request, "oauth2_error", {"error": "unverified_email"}) + return False + return True + class OAuth2Server(oauthlib.oauth2.Server): def verify_request(self, uri, *args, **kwargs): valid, request = super().verify_request(uri, *args, **kwargs) if valid: + if not check(request): + return False, request return valid, request # maybe the token was given in the querystring? @@ -21,5 +34,8 @@ class OAuth2Server(oauthlib.oauth2.Server): valid = self.request_validator.validate_bearer_token( token, request.scopes, request ) + if valid: + if not check(request): + return False, request return valid, request diff --git a/api/tests/common/test_authentication.py b/api/tests/common/test_authentication.py new file mode 100644 index 000000000..e249d5260 --- /dev/null +++ b/api/tests/common/test_authentication.py @@ -0,0 +1,62 @@ +import pytest + +from rest_framework import exceptions +from rest_framework_jwt.settings import api_settings as jwt_settings + +from funkwhale_api.common import authentication + + +@pytest.mark.parametrize( + "setting_value, is_superuser, has_verified_primary_email, expected", + [ + ("mandatory", False, False, True), + ("mandatory", False, True, False), + ("mandatory", True, False, False), + ("mandatory", True, True, False), + ("optional", False, False, False), + ("optional", False, True, False), + ("optional", True, False, False), + ("optional", True, True, False), + ], +) +def test_should_verify_email( + setting_value, + is_superuser, + has_verified_primary_email, + expected, + factories, + settings, +): + settings.ACCOUNT_EMAIL_VERIFICATION = setting_value + user = factories["users.User"](is_superuser=is_superuser) + setattr(user, "has_verified_primary_email", has_verified_primary_email) + assert authentication.should_verify_email(user) is expected + + +@pytest.mark.parametrize( + "setting_value, verified_email, expected", + [ + ("mandatory", False, True), + ("optional", False, False), + ("mandatory", True, False), + ("optional", True, False), + ], +) +def test_json_webtoken_auth_verify_email_validity( + setting_value, verified_email, expected, factories, settings, mocker, api_request +): + settings.ACCOUNT_EMAIL_VERIFICATION = setting_value + user = factories["users.User"](verified_email=verified_email) + should_verify = mocker.spy(authentication, "should_verify_email") + payload = jwt_settings.JWT_PAYLOAD_HANDLER(user) + token = jwt_settings.JWT_ENCODE_HANDLER(payload) + request = api_request.get("/", HTTP_AUTHORIZATION="JWT {}".format(token)) + + auth = authentication.JSONWebTokenAuthentication() + if expected is False: + assert auth.authenticate(request)[0] == user + else: + with pytest.raises(exceptions.AuthenticationFailed, match=r".*verify.*"): + auth.authenticate(request) + + should_verify.assert_called_once_with(user) diff --git a/api/tests/users/oauth/test_views.py b/api/tests/users/oauth/test_views.py index 19d258709..0bbbe0c57 100644 --- a/api/tests/users/oauth/test_views.py +++ b/api/tests/users/oauth/test_views.py @@ -289,6 +289,14 @@ def test_token_view_post(api_client, factories): with pytest.raises(grant.DoesNotExist): grant.refresh_from_db() + token = payload["access_token"] + + # Now check we can use the token for auth + response = api_client.get( + reverse("api:v1:users:users-me"), HTTP_AUTHORIZATION="Bearer {}".format(token) + ) + assert response.status_code == 200 + def test_revoke_view_post(logged_in_client, factories): token = factories["users.AccessToken"]() @@ -361,3 +369,26 @@ def test_grant_delete(factories, logged_in_api_client, mocker, now): for t in to_keep: t.refresh_from_db() + + +@pytest.mark.parametrize( + "setting_value, verified_email, expected_status_code", + [ + ("mandatory", False, 401), + ("mandatory", True, 200), + ("optional", True, 200), + ("optional", False, 200), + ], +) +def test_token_auth( + setting_value, verified_email, expected_status_code, api_client, factories, settings +): + + user = factories["users.User"](verified_email=verified_email) + token = factories["users.AccessToken"](user=user) + settings.ACCOUNT_EMAIL_VERIFICATION = setting_value + response = api_client.get( + reverse("api:v1:users:users-me"), + HTTP_AUTHORIZATION="Bearer {}".format(token.token), + ) + assert response.status_code == expected_status_code diff --git a/api/tests/users/test_models.py b/api/tests/users/test_models.py index b13ba58c2..f98a54d64 100644 --- a/api/tests/users/test_models.py +++ b/api/tests/users/test_models.py @@ -239,3 +239,24 @@ def test_creating_user_set_support_display_date( user = factories["users.User"]() assert getattr(user, field) == expected + + +def test_get_by_natural_key_annotates_primary_email_verified_no_email(factories): + user = factories["users.User"]() + user = models.User.objects.get_by_natural_key(user.username) + + assert user.has_verified_primary_email is None + + +def test_get_by_natural_key_annotates_primary_email_verified_true(factories): + user = factories["users.User"](verified_email=True) + user = models.User.objects.get_by_natural_key(user.username) + + assert user.has_verified_primary_email is True + + +def test_get_by_natural_key_annotates_primary_email_verified_false(factories): + user = factories["users.User"](verified_email=False) + user = models.User.objects.get_by_natural_key(user.username) + + assert user.has_verified_primary_email is False diff --git a/api/tests/users/test_views.py b/api/tests/users/test_views.py index 2a453ebcc..f02c16538 100644 --- a/api/tests/users/test_views.py +++ b/api/tests/users/test_views.py @@ -477,3 +477,37 @@ def test_signup_with_approval_enabled_validation_error( } response = api_client.post(url, data, format="json") assert response.status_code == 400 + + +def test_user_login_jwt(factories, api_client): + user = factories["users.User"]() + data = { + "username": user.username, + "password": "test", + } + url = reverse("api:v1:token") + response = api_client.post(url, data) + assert response.status_code == 200 + + +@pytest.mark.parametrize( + "setting_value, verified_email, expected_status_code", + [ + ("mandatory", False, 400), + ("mandatory", True, 200), + ("optional", False, 200), + ("optional", True, 200), + ], +) +def test_user_login_jwt_honor_email_verification( + setting_value, verified_email, expected_status_code, settings, factories, api_client +): + settings.ACCOUNT_EMAIL_VERIFICATION = setting_value + user = factories["users.User"](verified_email=verified_email) + data = { + "username": user.username, + "password": "test", + } + url = reverse("api:v1:token") + response = api_client.post(url, data) + assert response.status_code == expected_status_code diff --git a/changes/1039.feature b/changes/1039.feature new file mode 100644 index 000000000..bc1fee4fc --- /dev/null +++ b/changes/1039.feature @@ -0,0 +1 @@ +Make it possible to enforce email verification (#1039) diff --git a/changes/notes.rst b/changes/notes.rst index 044a739c7..21c81d8c2 100644 --- a/changes/notes.rst +++ b/changes/notes.rst @@ -28,9 +28,6 @@ In addition, it's also possible to customize the sign-up form by: - Providing a custom help text, in markdown format - Including additional fields in the form, for instance to ask the user why they want to join. Data collected through these fields is included in the sign-up request and viewable by the mods - - - Federated reports ^^^^^^^^^^^^^^^^^ @@ -52,6 +49,15 @@ or hard drives. We plan to remove the old engine in an upcoming release. In the meantime, if anything goes wrong, you can switch back by setting ``USE_FULL_TEXT_SEARCH=false`` in your ``.env`` file. +Enforced email verification +^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The brand new ``ACCOUNT_EMAIL_VERIFICATION_ENFORCE`` setting can be used to make email verification +mandatory for your users. It defaults to ``false``, and doesn't apply to superuser accounts created through +the CLI. + +If you enable this, ensure you have a SMTP server configured too. + User management through the server CLI ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/docs/admin/configuration.rst b/docs/admin/configuration.rst index 223ae7fb5..b3f206378 100644 --- a/docs/admin/configuration.rst +++ b/docs/admin/configuration.rst @@ -16,16 +16,16 @@ and technical aspects of your instance, such as database credentials. You should restart all Funkwhale processes when you change the values on environment variables. - + .. note:: Some characters are unsafe to use in configuration variables that are URLs, such as the user and password in the database and SMTP sections. - If those variables contain such characters, they must be urlencoded, for - instance using the following command: + If those variables contain such characters, they must be urlencoded, for + instance using the following command: ``python3 -c 'import urllib.parse; print(urllib.parse.quote_plus("p@ssword"))`` - + cf. https://github.com/joke2k/django-environ#using-unsafe-characters-in-urls .. _instance-settings: @@ -58,6 +58,19 @@ settings in this interface. Configuration reference ----------------------- +.. _setting-ACCOUNT_EMAIL_VERIFICATION_ENFORCE: + +``ACCOUNT_EMAIL_VERIFICATION_ENFORCE`` +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Determine wether users need to verify their email address before using the service. Enabling this can be useful +to reduce spam or bots accounts, however, you'll need to configure a SMTP server so that your users can receive the +verification emails. + +Note that regardless of the setting value, superusers created through the command line will never require verification. + +Default: ``false`` + .. _setting-EMAIL_CONFIG: ``EMAIL_CONFIG`` @@ -76,13 +89,13 @@ Possible values: - ``smtp+tls://user:password@youremail.host:587``: Send emails via SMTP via youremail.host on port 587, using TLS encryption, authenticating as user "user" with password "password" .. note:: - - If ``user`` or ``password`` contain special characters (eg. + + If ``user`` or ``password`` contain special characters (eg. ``noreply@youremail.host`` as ``user``), be sure to urlencode them, using for example the command: - ``python3 -c 'import urllib.parse; print(urllib.parse.quote_plus("noreply@youremail.host"))'`` + ``python3 -c 'import urllib.parse; print(urllib.parse.quote_plus("noreply@youremail.host"))'`` (returns ``noreply%40youremail.host``) - + .. _setting-DEFAULT_FROM_EMAIL: diff --git a/front/src/components/auth/LoginForm.vue b/front/src/components/auth/LoginForm.vue index 43e1b45a3..d14d893dc 100644 --- a/front/src/components/auth/LoginForm.vue +++ b/front/src/components/auth/LoginForm.vue @@ -3,11 +3,11 @@
We cannot log you in
    -
  • - Please double-check your username/password couple is correct -
  • - If you signed-up recently, you may need to wait before our moderation team review your account + If you signed-up recently, you may need to wait before our moderation team review your account, or verify your email. +
    • +
  • + Please double-check your username/password couple is correct and ensure you verified your email.
  • {{ error }}