From 60eb512122b5713ca5256d820dcf769e77cf7de1 Mon Sep 17 00:00:00 2001 From: Baptiste Mispelon Date: Fri, 29 Nov 2024 13:46:17 +0100 Subject: [PATCH 1/3] Used PROTECT for foundation.BoardMember.account.on_delete --- .../0007_boardmember_account_protect.py | 21 +++++++++++++++++++ foundation/models.py | 2 +- 2 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 foundation/migrations/0007_boardmember_account_protect.py diff --git a/foundation/migrations/0007_boardmember_account_protect.py b/foundation/migrations/0007_boardmember_account_protect.py new file mode 100644 index 000000000..07d607b38 --- /dev/null +++ b/foundation/migrations/0007_boardmember_account_protect.py @@ -0,0 +1,21 @@ +# Generated by Django 5.0.9 on 2024-11-29 06:45 + +import django.db.models.deletion +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('foundation', '0006_hardcode_currency_choices'), + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ] + + operations = [ + migrations.AlterField( + model_name='boardmember', + name='account', + field=models.ForeignKey(on_delete=django.db.models.deletion.PROTECT, to=settings.AUTH_USER_MODEL), + ), + ] diff --git a/foundation/models.py b/foundation/models.py index 598ee2116..7ae1fa4ac 100644 --- a/foundation/models.py +++ b/foundation/models.py @@ -42,7 +42,7 @@ class BoardMember(models.Model): """ - account = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE) + account = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.PROTECT) office = models.ForeignKey(Office, related_name="holders", on_delete=models.CASCADE) term = models.ForeignKey( Term, related_name="board_members", on_delete=models.CASCADE From f4234de2de5530b78ad43c3b3cfbb76c1207fe16 Mon Sep 17 00:00:00 2001 From: Baptiste Mispelon Date: Sun, 1 Dec 2024 17:21:28 +0100 Subject: [PATCH 2/3] Only sent next parameter to login view if not empty Setting it to "/" by default makes settings.LOGIN_REDIRECT_URL useless, and makes the profile edit page harder to discover. --- djangoproject/templates/registration/login.html | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/djangoproject/templates/registration/login.html b/djangoproject/templates/registration/login.html index b1b1ef046..9a4c320c0 100644 --- a/djangoproject/templates/registration/login.html +++ b/djangoproject/templates/registration/login.html @@ -15,7 +15,9 @@

{% translate "Log in" %}

{% csrf_token %} - + {% if next %} + + {% endif %}
From 73e73c9d25eb8dcbe3e83a37bd9b9efaefd4e8c9 Mon Sep 17 00:00:00 2001 From: Baptiste Mispelon Date: Fri, 29 Nov 2024 13:43:59 +0100 Subject: [PATCH 3/3] Fixed #1782 -- Added page to delete one's user account --- accounts/forms.py | 51 +++++++++++++++++++ accounts/tests.py | 51 ++++++++++++++++++- accounts/urls.py | 10 ++++ accounts/views.py | 40 ++++++++++++++- .../templates/accounts/delete_profile.html | 40 +++++++++++++++ .../accounts/delete_profile_success.html | 17 +++++++ .../templates/accounts/edit_profile.html | 6 +++ 7 files changed, 213 insertions(+), 2 deletions(-) create mode 100644 djangoproject/templates/accounts/delete_profile.html create mode 100644 djangoproject/templates/accounts/delete_profile_success.html diff --git a/accounts/forms.py b/accounts/forms.py index 39547d3b1..a63c3116f 100644 --- a/accounts/forms.py +++ b/accounts/forms.py @@ -1,4 +1,6 @@ from django import forms +from django.db import transaction +from django.db.models import ProtectedError from django.utils.translation import gettext_lazy as _ from .models import Profile @@ -36,3 +38,52 @@ def save(self, commit=True): if commit: instance.user.save() return instance + + +class DeleteProfileForm(forms.Form): + """ + A form for delete the request's user and their associated data. + + This form has no fields, it's used as a container for validation and deltion + logic. + """ + + class InvalidFormError(Exception): + pass + + def __init__(self, *args, user=None, **kwargs): + if user.is_anonymous: + raise TypeError("DeleteProfileForm only accepts actual User instances") + self.user = user + super().__init__(*args, **kwargs) + + def clean(self): + cleaned_data = super().clean() + if self.user.is_staff: + # Prevent potentially deleting some important history (admin.LogEntry) + raise forms.ValidationError(_("Staff users cannot be deleted")) + return cleaned_data + + def add_errors_from_protectederror(self, exception): + """ + Convert the given ProtectedError exception object into validation + errors on the instance. + """ + self.add_error(None, _("User has protected data and cannot be deleted")) + + @transaction.atomic() + def delete(self): + """ + Delete the form's user (self.instance). + """ + if not self.is_valid(): + raise self.InvalidFormError( + "DeleteProfileForm.delete() can only be called on valid forms" + ) + + try: + self.user.delete() + except ProtectedError as e: + self.add_errors_from_protectederror(e) + return None + return self.user diff --git a/accounts/tests.py b/accounts/tests.py index bc6c48e50..1c84ce44f 100644 --- a/accounts/tests.py +++ b/accounts/tests.py @@ -1,7 +1,9 @@ -from django.contrib.auth.models import User +from django.contrib.auth.models import AnonymousUser, User from django.test import TestCase, override_settings from django_hosts.resolvers import reverse +from accounts.forms import DeleteProfileForm +from foundation import models as foundationmodels from tracdb.models import Revision, Ticket, TicketChange from tracdb.testutils import TracDBCreateDatabaseMixin @@ -169,3 +171,50 @@ def test_profile_view_reversal(self): """ for username in ["asdf", "@asdf", "asd-f", "as.df", "as+df"]: reverse("user_profile", host="www", args=[username]) + + +class UserDeletionTestCase(TestCase): + def create_user_and_form(self, bound=True, **userkwargs): + userkwargs.setdefault("username", "test") + userkwargs.setdefault("email", "test@example.com") + userkwargs.setdefault("password", "password") + + formkwargs = {"user": User.objects.create_user(**userkwargs)} + if bound: + formkwargs["data"] = {} + + return DeleteProfileForm(**formkwargs) + + def test_deletion(self): + form = self.create_user_and_form() + self.assertFormError(form, None, []) + form.delete() + self.assertQuerySetEqual(User.objects.all(), []) + + def test_anonymous_user_error(self): + self.assertRaises(TypeError, DeleteProfileForm, user=AnonymousUser) + + def test_deletion_staff_forbidden(self): + form = self.create_user_and_form(is_staff=True) + self.assertFormError(form, None, ["Staff users cannot be deleted"]) + + def test_user_with_protected_data(self): + form = self.create_user_and_form() + form.user.boardmember_set.create( + office=foundationmodels.Office.objects.create(name="test"), + term=foundationmodels.Term.objects.create(year=2000), + ) + form.delete() + self.assertFormError( + form, None, ["User has protected data and cannot be deleted"] + ) + + def test_form_delete_method_requires_valid_form(self): + form = self.create_user_and_form(is_staff=True) + self.assertRaises(form.InvalidFormError, form.delete) + + def test_view_deletion_also_logs_out(self): + user = self.create_user_and_form().user + self.client.force_login(user) + self.client.post(reverse("delete_profile")) + self.assertEqual(self.client.cookies["sessionid"].value, "") diff --git a/accounts/urls.py b/accounts/urls.py index 5558d130e..0e535c756 100644 --- a/accounts/urls.py +++ b/accounts/urls.py @@ -15,6 +15,16 @@ account_views.edit_profile, name="edit_profile", ), + path( + "delete/", + account_views.delete_profile, + name="delete_profile", + ), + path( + "delete/success/", + account_views.delete_profile_success, + name="delete_profile_success", + ), path("", include("django.contrib.auth.urls")), path("", include("registration.backends.default.urls")), ] diff --git a/accounts/views.py b/accounts/views.py index bbdf888db..8c3c7dd0f 100644 --- a/accounts/views.py +++ b/accounts/views.py @@ -1,5 +1,7 @@ import hashlib +from urllib.parse import urlencode +from django.contrib.auth import logout from django.contrib.auth.decorators import login_required from django.contrib.auth.models import User from django.core.cache import caches @@ -7,7 +9,7 @@ from tracdb import stats as trac_stats -from .forms import ProfileForm +from .forms import DeleteProfileForm, ProfileForm from .models import Profile @@ -34,6 +36,42 @@ def edit_profile(request): return render(request, "accounts/edit_profile.html", {"form": form}) +@login_required +def delete_profile(request): + if request.method == "POST": + form = DeleteProfileForm(data=request.POST, user=request.user) + if form.is_valid(): + if form.delete(): + logout(request) + return redirect("delete_profile_success") + else: + form = DeleteProfileForm(user=request.user) + + context = { + "form": form, + # Strings are left translated on purpose (ops prefer english :D) + "OPS_EMAIL_PRESETS": urlencode( + { + "subject": "[djangoproject.com] Manual account deletion", + "body": ( + "Hello lovely Django Ops,\n\n" + "I would like to delete my djangoproject.com user account (" + f"username {request.user.username}) but the system is not letting " + "me do it myself. Could you help me out please?\n\n" + "Thanks in advance,\n" + "You're amazing\n" + f"{request.user.get_full_name() or request.user.username}" + ), + } + ), + } + return render(request, "accounts/delete_profile.html", context) + + +def delete_profile_success(request): + return render(request, "accounts/delete_profile_success.html") + + def get_user_stats(user): c = caches["default"] username = user.username.encode("ascii", "ignore") diff --git a/djangoproject/templates/accounts/delete_profile.html b/djangoproject/templates/accounts/delete_profile.html new file mode 100644 index 000000000..48bfc4cfd --- /dev/null +++ b/djangoproject/templates/accounts/delete_profile.html @@ -0,0 +1,40 @@ +{% extends "base.html" %} +{% load i18n %} + +{% block title %}{% translate "Confirmation: delete your profile" %}{% endblock %} + +{% block content %} + {% if form.errors %} +

{% translate "Could not delete account" %}

+ +

{% blocktranslate trimmed %} + Sorry, something went wrong when trying to delete your account. + That means there's probably some protected data still associated + with your account. + Please contact + the operations team + and we'll sort it out for you. + {% endblocktranslate %}

+ {% else %} +

{% translate "Are you sure?" %}

+ +

{% blocktranslate trimmed with username=request.user.username %} + ⚠️ You are about to delete all data associated with the username + {{ username}}. + {% endblocktranslate %}

+ +

{% blocktranslate trimmed %} + Deleting your account is permanent and cannot be reversed. + Are you sure you want to continue? + {% endblocktranslate %}

+ + {% csrf_token %} +
+ + + {% translate "No, cancel and go back" %} + +
+ + {% endif %} +{% endblock %} diff --git a/djangoproject/templates/accounts/delete_profile_success.html b/djangoproject/templates/accounts/delete_profile_success.html new file mode 100644 index 000000000..411b3fcf7 --- /dev/null +++ b/djangoproject/templates/accounts/delete_profile_success.html @@ -0,0 +1,17 @@ +{% extends "base.html" %} + +{% load i18n %} + +{% block content %} +

{% translate "Account deleted" %}

+

+ {% translate "Your account and its data was successfully deleted and you've been logged out." %} +

+

+ {% url "community-index" as community_index_url %} + {% blocktranslate trimmed %} + Thanks for spending your time with us, we hope we'll still see you + around on our various community spaces, online and off. + {% endblocktranslate %} +

+{% endblock %} diff --git a/djangoproject/templates/accounts/edit_profile.html b/djangoproject/templates/accounts/edit_profile.html index 536ac5bdd..be41208a4 100644 --- a/djangoproject/templates/accounts/edit_profile.html +++ b/djangoproject/templates/accounts/edit_profile.html @@ -50,5 +50,11 @@

{% translate "Help" %}

emails. We'll also use this email to try to fetch a Gravatar. You can change the image for this email at Gravatar.{% endblocktranslate %}

+ +

+ + {% translate "Want to delete your account?" %} + +

{% endblock %}