diff --git a/config/settings.py b/config/settings.py index b46af8bb..771e63d2 100755 --- a/config/settings.py +++ b/config/settings.py @@ -293,6 +293,11 @@ ACCOUNT_AUTHENTICATION_METHOD = "email" SOCIALACCOUNT_QUERY_EMAIL = True ACCOUNT_UNIQUE_EMAIL = True +# Allow us to override some of allauth's forms +ACCOUNT_FORMS = { + "reset_password_from_key": "users.forms.CustomResetPasswordFromKeyForm", +} + SOCIALACCOUNT_PROVIDERS = { "google": { "SCOPE": [ diff --git a/docs/README.md b/docs/README.md index c3fdc23f..b5b03d90 100644 --- a/docs/README.md +++ b/docs/README.md @@ -3,6 +3,7 @@ - [Development Setup Notes](./development_setup_notes.md) - [Environment Variables](./env_vars.md) - [Example Files](./examples/README.md) - Contains samples of `libraries.json`. `.gitmodules`, and other files that Boost data depends on +- [Handling "Unclaimed" User Accounts](./unclaimed_user_accounts.md) - Describes how we allow authors and maintainers to "claim" the accounts that we create for them as part of the library upload process - [Management Commands](./commands.md) - [Retrieving Static Content from the Boost Amazon S3 Bucket](./static_content.md) - [Syncing Data about Boost Versions and Libraries with GitHub](./syncing_data_with_github.md) diff --git a/docs/unclaimed_user_accounts.md b/docs/unclaimed_user_accounts.md new file mode 100644 index 00000000..0ba04cb4 --- /dev/null +++ b/docs/unclaimed_user_accounts.md @@ -0,0 +1,24 @@ +# Handling "Unclaimed" User Accounts + +This page covers how to handle the User records that are created as part of the upload process from the [first-time data import](./first_time_data_import.md) and through [syncing with GitHub](./syncing_data_with_github.md). + +The code for this page lives in the `users/` app. + +## About Registration and Login + +- We use Django Allauth to handle user accounts +- We do some overriding of their logic for the profile page +- We override the password reset logic as part of allowing users to claim their unclaimed accounts + +## About Unclaimed User Accounts + +- When libraries are created and updated from GitHub, we receive information on Library Maintainers and Authors. +- Those authors and maintainers are added as Users and then linked to the Library or LibraryVersion record they belong to. +- When they are created, the User accounts have their `claimed` field marked as False. This field defaults to `True`, and will only be `False` for users who were created by an automated process. +- We use the email address and name in the `libraries.json` file for that library to create the User record + +## When An Unclaimed User Tries to Register + +- If a user tries to register with the same email address as an existing user (a user with `claimed` set to `False`), we interrupt the Django Allauth registration error that happens in this case to check whether the user has been claimed +- If the user has `claimed` set to `False`, we send the user a custom message and send them a password reset email +- On the backend, we interrupt the Django Allauth password reset process to mark users as claimed once their password has been successfully reset diff --git a/users/forms.py b/users/forms.py index 982c592d..862e44ae 100644 --- a/users/forms.py +++ b/users/forms.py @@ -1,6 +1,8 @@ from django.contrib.auth import get_user_model from django import forms +from allauth.account.forms import ResetPasswordKeyForm + from .models import Preferences from news.models import NEWS_MODELS from news.acl import can_approve @@ -11,6 +13,15 @@ User = get_user_model() NEWS_ENTRY_CHOICES = [(m.news_type, m._meta.verbose_name.title()) for m in NEWS_MODELS] +class CustomResetPasswordFromKeyForm(ResetPasswordKeyForm): + def save(self, **kwargs): + """Override default reset password form so we can mark unclaimed + users as claimed once they have reset their passwords.""" + result = super().save(**kwargs) + self.user.claim() + return result + + class PreferencesForm(forms.ModelForm): allow_notification_own_news_approved = forms.MultipleChoiceField( choices=NEWS_ENTRY_CHOICES, diff --git a/users/models.py b/users/models.py index d7273651..f0e1dbf6 100644 --- a/users/models.py +++ b/users/models.py @@ -241,6 +241,12 @@ class User(BaseUser): else: return self.first_name or self.last_name + def claim(self): + """Claim the user""" + if not self.claimed: + self.claimed = True + self.save() + class LastSeen(models.Model): """ diff --git a/users/tests/test_forms.py b/users/tests/test_forms.py index cd708b62..39c00c13 100644 --- a/users/tests/test_forms.py +++ b/users/tests/test_forms.py @@ -1,10 +1,31 @@ import pytest -from ..forms import PreferencesForm, UserProfileForm +from ..forms import CustomResetPasswordFromKeyForm, PreferencesForm, UserProfileForm from ..models import Preferences from news.models import NEWS_MODELS +def test_custom_reset_password_form(user): + user.claimed = False + user.save() + user.refresh_from_db() + + reset_key = "your_reset_key" + form = CustomResetPasswordFromKeyForm( + data={ + "key": reset_key, + "email": user.email, + "password1": "new_password", + "password2": "new_password", + }, + user=user, + ) + assert form.is_valid() + form.save() + user.refresh_from_db() + assert user.claimed is True + + def test_preferences_form_fields_no_user(): form = PreferencesForm() assert sorted(form.fields.keys()) == [ diff --git a/users/tests/test_models.py b/users/tests/test_models.py index 619500b0..8044923e 100644 --- a/users/tests/test_models.py +++ b/users/tests/test_models.py @@ -60,6 +60,17 @@ def test_user_save_image_from_github(user): pass +def test_claim(user): + user.claimed = False + user.save() + user.refresh_from_db() + + assert not user.claimed + user.claim() + user.refresh_from_db() + assert user.claimed + + def test_find_contributor_by_email(user): found_user = User.objects.find_contributor(email=user.email) assert found_user == user diff --git a/users/tests/test_views.py b/users/tests/test_views.py index c0f81992..433c06ab 100644 --- a/users/tests/test_views.py +++ b/users/tests/test_views.py @@ -41,6 +41,29 @@ def test_current_user_profile_view_post_valid_password(user, tp): user.check_password("new_password") +@pytest.mark.django_db +def test_current_user_profile_view_post_valid_password_unclaimed_user(user, tp): + user.claimed = False + user.save() + user.refresh_from_db() + with tp.login(user): + response = tp.post( + tp.reverse("profile-account"), + data={ + "email": user.email, + "oldpassword": "password", + "password1": "new_password", + "password2": "new_password", + "change_password": True, + }, + follow=True, + ) + assert response.status_code == 200 + user.refresh_from_db() + user.check_password("new_password") + assert user.claimed + + @pytest.mark.django_db def test_current_user_profile_view_post_invalid_password(user, tp): old_password = "password" diff --git a/users/views.py b/users/views.py index 599a5362..6ce469f0 100644 --- a/users/views.py +++ b/users/views.py @@ -16,7 +16,6 @@ from rest_framework import viewsets from rest_framework.permissions import IsAuthenticated from .forms import PreferencesForm, UserProfileForm, UserProfilePhotoForm - from .models import User from .permissions import CustomUserPermissions from .serializers import UserSerializer, FullUserSerializer, CurrentUserSerializer @@ -141,6 +140,10 @@ class CurrentUserProfileView(LoginRequiredMixin, SuccessMessageMixin, TemplateVi self.object = request.user self.object.set_password(form.cleaned_data["password1"]) self.object.save() + + # Resetting the password acts as confirmation that the user has + # claimed their account, so mark it claimed. + self.object.claim() messages.success(request, "Your password was successfully updated.") else: for error in form.errors.values():