From 93a7dd9cf3ee3b67982ab63965050c67ee2bc829 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 07:50:28 -0800 Subject: [PATCH 01/10] Erase user data and statuses on account deletion --- bookwyrm/models/user.py | 24 ++++++++++++++- bookwyrm/tests/models/test_user_model.py | 38 ++++++++++++++++++++++-- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index c152cf445..625a7d289 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -1,6 +1,7 @@ """ database schema for user data """ import re from urllib.parse import urlparse +from uuid import uuid4 from django.apps import apps from django.contrib.auth.models import AbstractUser @@ -394,10 +395,31 @@ class User(OrderedCollectionPageMixin, AbstractUser): """We don't actually delete the database entry""" # pylint: disable=attribute-defined-outside-init self.is_active = False - self.avatar = "" + self.allow_reactivation = False + + self.erase_user_data() + self.erase_user_statuses() + # skip the logic in this class's save() super().save(*args, **kwargs) + def erase_user_data(self): + """Wipe a user's custom data""" + # mangle email address + self.email = f"{uuid4()}@deleted.user" + + # erase data fields + self.avatar = "" + self.preview_image = "" + self.summary = None + self.name = None + self.favorites.set([]) + + def erase_user_statuses(self): + """Wipe the data on all the user's statuses""" + for status in self.status_set.all(): + status.delete() + def deactivate(self): """Disable the user but allow them to reactivate""" # pylint: disable=attribute-defined-outside-init diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index 838dd2e49..de39d5467 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -26,6 +26,7 @@ class User(TestCase): local=True, localname="mouse", name="hi", + summary="a summary", bookwyrm_user=False, ) self.another_user = models.User.objects.create_user( @@ -218,19 +219,52 @@ class User(TestCase): @patch("bookwyrm.suggested_users.remove_user_task.delay") def test_delete_user(self, _): - """deactivate a user""" + """permanently delete a user""" self.assertTrue(self.user.is_active) + self.assertEqual(self.user.name, "hi") + self.assertEqual(self.user.summary, "a summary") + self.assertEqual(self.user.email, "mouse@mouse.mouse") with patch( "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" - ) as broadcast_mock: + ) as broadcast_mock, patch( + "bookwyrm.models.user.User.erase_user_statuses" + ) as erase_statuses_mock: self.user.delete() + self.assertEqual(erase_statuses_mock.call_count, 1) + + # make sure the deletion is broadcast self.assertEqual(broadcast_mock.call_count, 1) activity = json.loads(broadcast_mock.call_args[1]["args"][1]) self.assertEqual(activity["type"], "Delete") self.assertEqual(activity["object"], self.user.remote_id) + + self.user.refresh_from_db() + + # the user's account data should be deleted + self.assertIsNone(self.user.name) + self.assertIsNone(self.user.summary) + self.assertNotEqual(self.user.email, "mouse@mouse.mouse") self.assertFalse(self.user.is_active) + @patch("bookwyrm.suggested_users.remove_user_task.delay") + @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async") + @patch("bookwyrm.activitystreams.add_status_task.delay") + @patch("bookwyrm.activitystreams.remove_status_task.delay") + def test_delete_user_erase_statuses(self, *_): + """erase user statuses when user is deleted""" + status = models.Status.objects.create(user=self.user, content="hello") + self.assertFalse(status.deleted) + self.assertIsNotNone(status.content) + self.assertIsNone(status.deleted_date) + + self.user.delete() + status.refresh_from_db() + + self.assertTrue(status.deleted) + self.assertIsNone(status.content) + self.assertIsNotNone(status.deleted_date) + def test_admins_no_admins(self): """list of admins""" result = models.User.admins() From 5e42afd85a94c5a7ab6d216533acd5f06c14f54a Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 08:10:03 -0800 Subject: [PATCH 02/10] Pass args and kwargs through status deletion --- bookwyrm/models/status.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 11646431b..cc44fe2bf 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -102,7 +102,7 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): if hasattr(self, "quotation"): self.quotation = None # pylint: disable=attribute-defined-outside-init self.deleted_date = timezone.now() - self.save() + self.save(*args, **kwargs) @property def recipients(self): From 61caeed5a3bcf9508524963489896488b52f7ef2 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 08:51:42 -0800 Subject: [PATCH 03/10] Adds migration and more tests --- .../migrations/0183_auto_20231105_1607.py | 25 ++++++++ bookwyrm/models/user.py | 27 ++++++++- bookwyrm/tests/models/test_user_model.py | 57 +++++++++++++++++++ 3 files changed, 106 insertions(+), 3 deletions(-) create mode 100644 bookwyrm/migrations/0183_auto_20231105_1607.py diff --git a/bookwyrm/migrations/0183_auto_20231105_1607.py b/bookwyrm/migrations/0183_auto_20231105_1607.py new file mode 100644 index 000000000..390b56a9f --- /dev/null +++ b/bookwyrm/migrations/0183_auto_20231105_1607.py @@ -0,0 +1,25 @@ +# Generated by Django 3.2.20 on 2023-11-05 16:07 + +from django.db import migrations +from bookwyrm.models import User + + +def erase_deleted_user_data(apps, schema_editor): + """Retroactively clear user data""" + for user in User.get_permanently_deleted_users(): + user.erase_user_data() + user.save(broadcast=False) + user.erase_user_statuses(broadcast=False) + + +class Migration(migrations.Migration): + + dependencies = [ + ("bookwyrm", "0182_auto_20231027_1122"), + ] + + operations = [ + migrations.RunPython( + erase_deleted_user_data, reverse_code=migrations.RunPython.noop + ) + ] diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 625a7d289..017db31d3 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -8,7 +8,7 @@ from django.contrib.auth.models import AbstractUser from django.contrib.postgres.fields import ArrayField, CICharField from django.core.exceptions import PermissionDenied, ObjectDoesNotExist from django.dispatch import receiver -from django.db import models, transaction +from django.db import models, transaction, IntegrityError from django.utils import timezone from django.utils.translation import gettext_lazy as _ from model_utils import FieldTracker @@ -263,6 +263,13 @@ class User(OrderedCollectionPageMixin, AbstractUser): is_active=True, ).distinct() + @classmethod + def get_permanently_deleted_users(cls): + return cls.objects.filter( + is_active=False, + deactivation_reason__in=["self_deletion", "moderator_deletion"], + ).distinct() + def update_active_date(self): """this user is here! they are doing things!""" self.last_active_date = timezone.now() @@ -415,10 +422,24 @@ class User(OrderedCollectionPageMixin, AbstractUser): self.name = None self.favorites.set([]) - def erase_user_statuses(self): + def erase_user_statuses(self, broadcast=True): """Wipe the data on all the user's statuses""" + # safety valve: make sure the user is deleted + if not self.is_permanently_deleted: + raise IntegrityError( + "Attempted to delete statuses for improperly deleted user" + ) + for status in self.status_set.all(): - status.delete() + status.delete(broadcast=broadcast) + + @property + def is_permanently_deleted(self): + """is this user inactive, or really truly deleted?""" + return not self.is_active and self.deactivation_reason in [ + "self_deletion", + "moderator_deletion", + ] def deactivate(self): """Disable the user but allow them to reactivate""" diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index de39d5467..47db1bc9a 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -2,6 +2,7 @@ import json from unittest.mock import patch from django.contrib.auth.models import Group +from django.db import IntegrityError from django.test import TestCase import responses @@ -265,6 +266,25 @@ class User(TestCase): self.assertIsNone(status.content) self.assertIsNotNone(status.deleted_date) + @patch("bookwyrm.suggested_users.remove_user_task.delay") + @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async") + @patch("bookwyrm.activitystreams.add_status_task.delay") + def test_delete_user_erase_statuses(self, *_): + """erase user statuses when user is deleted""" + status = models.Status.objects.create(user=self.user, content="hello") + self.assertFalse(status.deleted) + self.assertIsNotNone(status.content) + self.assertIsNone(status.deleted_date) + + self.user.deactivate() + with self.assertRaises(IntegrityError): + self.user.erase_user_statuses() + + status.refresh_from_db() + self.assertFalse(status.deleted) + self.assertIsNotNone(status.content) + self.assertIsNone(status.deleted_date) + def test_admins_no_admins(self): """list of admins""" result = models.User.admins() @@ -302,3 +322,40 @@ class User(TestCase): results = models.User.admins() self.assertEqual(results.count(), 1) self.assertEqual(results.first(), self.user) + + def test_get_permanently_deleted_users(self): + + with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( + "bookwyrm.activitystreams.populate_stream_task.delay" + ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): + active_user = models.User.objects.create_user( + f"activeuser@{DOMAIN}", + "activeuser@activeuser.activeuser", + "activeuserword", + local=True, + localname="active", + ) + deleted_user = models.User.objects.create_user( + f"deleteduser@{DOMAIN}", + "deleteduser@deleteduser.deleteduser", + "deleteduserword", + local=True, + localname="deleted", + is_active=False, + deactivation_reason="self_deletion", + ) + inactive_user = models.User.objects.create_user( + f"inactiveuser@{DOMAIN}", + "inactiveuser@inactiveuser.inactiveuser", + "inactiveuserword", + local=True, + localname="inactive", + is_active=False, + deactivation_reason="self_deactivation", + ) + + deleted_users = models.User.get_permanently_deleted_users() + + self.assertTrue(deleted_users.filter(localname="deleted").exists()) + self.assertFalse(deleted_users.filter(localname="active").exists()) + self.assertFalse(deleted_users.filter(localname="inactive").exists()) From 4de99074566a233cd765c8e41a0e9f5047296ac9 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 09:25:50 -0800 Subject: [PATCH 04/10] Adds migration tests --- .../migrations/0183_auto_20231105_1607.py | 5 +- bookwyrm/models/user.py | 10 +- bookwyrm/tests/migrations/test_0183.py | 126 ++++++++++++++++++ 3 files changed, 139 insertions(+), 2 deletions(-) create mode 100644 bookwyrm/tests/migrations/test_0183.py diff --git a/bookwyrm/migrations/0183_auto_20231105_1607.py b/bookwyrm/migrations/0183_auto_20231105_1607.py index 390b56a9f..2716a0737 100644 --- a/bookwyrm/migrations/0183_auto_20231105_1607.py +++ b/bookwyrm/migrations/0183_auto_20231105_1607.py @@ -8,7 +8,10 @@ def erase_deleted_user_data(apps, schema_editor): """Retroactively clear user data""" for user in User.get_permanently_deleted_users(): user.erase_user_data() - user.save(broadcast=False) + user.save( + broadcast=False, + update_fields=["email", "avatar", "preview_image", "summary", "name"], + ) user.erase_user_statuses(broadcast=False) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 017db31d3..43df39291 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -408,10 +408,18 @@ class User(OrderedCollectionPageMixin, AbstractUser): self.erase_user_statuses() # skip the logic in this class's save() - super().save(*args, **kwargs) + super().save( + *args, + update_fields=["email", "avatar", "preview_image", "summary", "name"], + **kwargs, + ) def erase_user_data(self): """Wipe a user's custom data""" + if not self.is_permanently_deleted: + raise IntegrityError( + "Attempted to delete user data for improperly deleted user" + ) # mangle email address self.email = f"{uuid4()}@deleted.user" diff --git a/bookwyrm/tests/migrations/test_0183.py b/bookwyrm/tests/migrations/test_0183.py new file mode 100644 index 000000000..8c8f27d5a --- /dev/null +++ b/bookwyrm/tests/migrations/test_0183.py @@ -0,0 +1,126 @@ +""" testing migrations """ +import json +from unittest.mock import patch + +from django.apps import apps +from django.test import TestCase +from django.db.migrations.executor import MigrationExecutor +from django.db import connection +import responses + +from bookwyrm import models +from bookwyrm.management.commands import initdb +from bookwyrm.settings import USE_HTTPS, DOMAIN + +# pylint: disable=missing-class-docstring +# pylint: disable=missing-function-docstring +class EraseDeletedUserDataMigration(TestCase): + + migrate_from = "0182_auto_20231027_1122" + migrate_to = "0183_auto_20231105_1607" + + # pylint: disable=invalid-name + def setUp(self): + with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( + "bookwyrm.activitystreams.populate_stream_task.delay" + ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): + self.active_user = models.User.objects.create_user( + f"activeuser@{DOMAIN}", + "activeuser@activeuser.activeuser", + "activeuserword", + local=True, + localname="active", + name="a name", + ) + self.inactive_user = models.User.objects.create_user( + f"inactiveuser@{DOMAIN}", + "inactiveuser@inactiveuser.inactiveuser", + "inactiveuserword", + local=True, + localname="inactive", + is_active=False, + deactivation_reason="self_deactivation", + name="name name", + ) + self.deleted_user = models.User.objects.create_user( + f"deleteduser@{DOMAIN}", + "deleteduser@deleteduser.deleteduser", + "deleteduserword", + local=True, + localname="deleted", + is_active=False, + deactivation_reason="self_deletion", + name="cool name", + ) + with patch( + "bookwyrm.models.activitypub_mixin.broadcast_task.apply_async" + ), patch("bookwyrm.activitystreams.add_status_task.delay"): + self.active_status = models.Status.objects.create( + user=self.active_user, content="don't delete me" + ) + self.inactive_status = models.Status.objects.create( + user=self.inactive_user, content="also don't delete me" + ) + self.deleted_status = models.Status.objects.create( + user=self.deleted_user, content="yes, delete me" + ) + + initdb.init_groups() + initdb.init_permissions() + + assert ( + self.migrate_from and self.migrate_to + ), "TestCase '{}' must define migrate_from and migrate_to properties".format( + type(self).__name__ + ) + self.migrate_from = [("bookwyrm", self.migrate_from)] + self.migrate_to = [("bookwyrm", self.migrate_to)] + executor = MigrationExecutor(connection) + old_apps = executor.loader.project_state(self.migrate_from).apps + + # Reverse to the original migration + executor.migrate(self.migrate_from) + + self.setUpBeforeMigration(old_apps) + + # Run the migration to test + executor = MigrationExecutor(connection) + executor.loader.build_graph() # reload. + with patch("bookwyrm.activitystreams.remove_status_task.delay"): + executor.migrate(self.migrate_to) + + self.apps = executor.loader.project_state(self.migrate_to).apps + + def setUpBeforeMigration(self, apps): + pass + + def test_user_data_deleted(self): + """Make sure that only the right data was deleted""" + self.active_user.refresh_from_db() + self.inactive_user.refresh_from_db() + self.deleted_user.refresh_from_db() + self.active_status.refresh_from_db() + self.inactive_status.refresh_from_db() + self.deleted_status.refresh_from_db() + + self.assertTrue(self.active_user.is_active) + self.assertEqual(self.active_user.name, "a name") + self.assertNotEqual(self.deleted_user.email, "activeuser@activeuser.activeuser") + self.assertFalse(self.active_status.deleted) + self.assertEqual(self.active_status.content, "don't delete me") + + self.assertFalse(self.inactive_user.is_active) + self.assertEqual(self.inactive_user.name, "name name") + self.assertNotEqual( + self.deleted_user.email, "inactiveuser@inactiveuser.inactiveuser" + ) + self.assertFalse(self.inactive_status.deleted) + self.assertEqual(self.inactive_status.content, "also don't delete me") + + self.assertFalse(self.deleted_user.is_active) + self.assertIsNone(self.deleted_user.name) + self.assertNotEqual( + self.deleted_user.email, "deleteduser@deleteduser.deleteduser" + ) + self.assertTrue(self.deleted_status.deleted) + self.assertIsNone(self.deleted_status.content) From 47953c84d7823aaf6102fb85409fd70d53b6e9aa Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 09:49:38 -0800 Subject: [PATCH 05/10] Fixes linting errors Apparently I didn't have a linter working! --- bookwyrm/tests/models/test_user_model.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index 47db1bc9a..3576417de 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -1,5 +1,6 @@ """ testing models """ import json + from unittest.mock import patch from django.contrib.auth.models import Group from django.db import IntegrityError @@ -10,9 +11,11 @@ from bookwyrm import models from bookwyrm.management.commands import initdb from bookwyrm.settings import USE_HTTPS, DOMAIN + # pylint: disable=missing-class-docstring # pylint: disable=missing-function-docstring class User(TestCase): + protocol = "https://" if USE_HTTPS else "http://" # pylint: disable=invalid-name @@ -269,7 +272,7 @@ class User(TestCase): @patch("bookwyrm.suggested_users.remove_user_task.delay") @patch("bookwyrm.models.activitypub_mixin.broadcast_task.apply_async") @patch("bookwyrm.activitystreams.add_status_task.delay") - def test_delete_user_erase_statuses(self, *_): + def test_delete_user_erase_statuses_invalid(self, *_): """erase user statuses when user is deleted""" status = models.Status.objects.create(user=self.user, content="hello") self.assertFalse(status.deleted) @@ -328,14 +331,14 @@ class User(TestCase): with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( "bookwyrm.activitystreams.populate_stream_task.delay" ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): - active_user = models.User.objects.create_user( + models.User.objects.create_user( f"activeuser@{DOMAIN}", "activeuser@activeuser.activeuser", "activeuserword", local=True, localname="active", ) - deleted_user = models.User.objects.create_user( + models.User.objects.create_user( f"deleteduser@{DOMAIN}", "deleteduser@deleteduser.deleteduser", "deleteduserword", @@ -344,7 +347,7 @@ class User(TestCase): is_active=False, deactivation_reason="self_deletion", ) - inactive_user = models.User.objects.create_user( + models.User.objects.create_user( f"inactiveuser@{DOMAIN}", "inactiveuser@inactiveuser.inactiveuser", "inactiveuserword", From f353b49d36b5778fefdc820f8adf529ce1d2b055 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 09:53:57 -0800 Subject: [PATCH 06/10] Another linting issues --- bookwyrm/models/user.py | 1 + 1 file changed, 1 insertion(+) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 43df39291..48f536ed1 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -265,6 +265,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): @classmethod def get_permanently_deleted_users(cls): + """a list of users who are permanently deleted""" return cls.objects.filter( is_active=False, deactivation_reason__in=["self_deletion", "moderator_deletion"], From d3668e413db48be6495ff2d8c1f02337158d52f8 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 09:59:49 -0800 Subject: [PATCH 07/10] Removes updates fields that was causing problems --- bookwyrm/models/user.py | 1 - 1 file changed, 1 deletion(-) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 48f536ed1..b9e9ae486 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -411,7 +411,6 @@ class User(OrderedCollectionPageMixin, AbstractUser): # skip the logic in this class's save() super().save( *args, - update_fields=["email", "avatar", "preview_image", "summary", "name"], **kwargs, ) From c17a2ec55ba18d52c5f83a19e1d30550063151a4 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 10:18:04 -0800 Subject: [PATCH 08/10] Creates snippet for user tag in admin view The existing display wasn't showing the correct colors and was repeating code unnecessarily --- .../templates/settings/users/user_admin.html | 26 +------------------ .../templates/settings/users/user_info.html | 19 +------------- .../templates/snippets/user_active_tag.html | 17 ++++++++++++ .../snippets/user_active_tag_item.html | 19 ++++++++++++++ 4 files changed, 38 insertions(+), 43 deletions(-) create mode 100644 bookwyrm/templates/snippets/user_active_tag.html create mode 100644 bookwyrm/templates/snippets/user_active_tag_item.html diff --git a/bookwyrm/templates/settings/users/user_admin.html b/bookwyrm/templates/settings/users/user_admin.html index a1d93ddd0..cc5c51ba7 100644 --- a/bookwyrm/templates/settings/users/user_admin.html +++ b/bookwyrm/templates/settings/users/user_admin.html @@ -74,31 +74,7 @@ {{ user.created_date }} {{ user.last_active_date }} - {% if user.is_active %} - {% if user.moved_to %} - - {% trans "Moved" %} - {% else %} - - {% trans "Active" %} - {% endif %} - {% elif user.deactivation_reason == "moderator_deletion" or user.deactivation_reason == "self_deletion" %} - - {% trans "Deleted" %} - ({{ user.get_deactivation_reason_display }}) - {% else %} - - {% trans "Inactive" %} - ({{ user.get_deactivation_reason_display }}) - {% endif %} + {% include "snippets/user_active_tag.html" with user=user %} {% if status == "federated" %} diff --git a/bookwyrm/templates/settings/users/user_info.html b/bookwyrm/templates/settings/users/user_info.html index 368045a0d..f35c60db9 100644 --- a/bookwyrm/templates/settings/users/user_info.html +++ b/bookwyrm/templates/settings/users/user_info.html @@ -23,24 +23,7 @@

{% trans "Status" %}

- {% if user.is_active %} - {% if user.moved_to %} -

- {% trans "Moved" %} -

- {% else %} -

- {% trans "Active" %} -

- {% endif %} - {% else %} -

- {% trans "Inactive" %} - {% if user.deactivation_reason %} - ({% trans user.get_deactivation_reason_display %}) - {% endif %} -

- {% endif %} + {% include "snippets/user_active_tag.html" with large=True %}

{% if user.local %} {% trans "Local" %} diff --git a/bookwyrm/templates/snippets/user_active_tag.html b/bookwyrm/templates/snippets/user_active_tag.html new file mode 100644 index 000000000..c3f067b43 --- /dev/null +++ b/bookwyrm/templates/snippets/user_active_tag.html @@ -0,0 +1,17 @@ +{% load i18n %} + +{% if user.is_active %} + {% if user.moved_to %} + {% trans "Moved" as text %} + {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="info" %} + {% else %} + {% trans "Active" as text %} + {% include "snippets/user_active_tag_item.html" with icon="check" text=text level="success" %} + {% endif %} +{% elif user.is_permanently_deleted %} + {% trans "Deleted" as text %} + {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="danger" deactivation_reason=user.get_deactivation_reason_display %} +{% else %} + {% trans "Inactive" as text %} + {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="warning" deactivation_reason=user.get_deactivation_reason_display %} +{% endif %} diff --git a/bookwyrm/templates/snippets/user_active_tag_item.html b/bookwyrm/templates/snippets/user_active_tag_item.html new file mode 100644 index 000000000..e722150f2 --- /dev/null +++ b/bookwyrm/templates/snippets/user_active_tag_item.html @@ -0,0 +1,19 @@ +{% if large %} + +

+ + {{ text }} + {% if deactivation_reason %} + ({{ deactivation_reason }}) + {% endif %} +

+ +{% else %} + + +{{ text }} + +{% endif %} + From 27d99a009412a2b48ebf063f1ba03b5bdd50f714 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 19:47:32 -0800 Subject: [PATCH 09/10] Removes failsafe that was overzealous --- bookwyrm/models/user.py | 18 ++++++------------ .../tests/views/inbox/test_inbox_delete.py | 4 +++- 2 files changed, 9 insertions(+), 13 deletions(-) diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index b9e9ae486..19006f772 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -8,7 +8,7 @@ from django.contrib.auth.models import AbstractUser from django.contrib.postgres.fields import ArrayField, CICharField from django.core.exceptions import PermissionDenied, ObjectDoesNotExist from django.dispatch import receiver -from django.db import models, transaction, IntegrityError +from django.db import models, transaction from django.utils import timezone from django.utils.translation import gettext_lazy as _ from model_utils import FieldTracker @@ -268,7 +268,10 @@ class User(OrderedCollectionPageMixin, AbstractUser): """a list of users who are permanently deleted""" return cls.objects.filter( is_active=False, - deactivation_reason__in=["self_deletion", "moderator_deletion"], + deactivation_reason__in=[ + "self_deletion", + "moderator_deletion", + ], ).distinct() def update_active_date(self): @@ -416,10 +419,6 @@ class User(OrderedCollectionPageMixin, AbstractUser): def erase_user_data(self): """Wipe a user's custom data""" - if not self.is_permanently_deleted: - raise IntegrityError( - "Attempted to delete user data for improperly deleted user" - ) # mangle email address self.email = f"{uuid4()}@deleted.user" @@ -432,12 +431,6 @@ class User(OrderedCollectionPageMixin, AbstractUser): def erase_user_statuses(self, broadcast=True): """Wipe the data on all the user's statuses""" - # safety valve: make sure the user is deleted - if not self.is_permanently_deleted: - raise IntegrityError( - "Attempted to delete statuses for improperly deleted user" - ) - for status in self.status_set.all(): status.delete(broadcast=broadcast) @@ -447,6 +440,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): return not self.is_active and self.deactivation_reason in [ "self_deletion", "moderator_deletion", + "activitypub_deletion", ] def deactivate(self): diff --git a/bookwyrm/tests/views/inbox/test_inbox_delete.py b/bookwyrm/tests/views/inbox/test_inbox_delete.py index 0fb108e22..7b4c12564 100644 --- a/bookwyrm/tests/views/inbox/test_inbox_delete.py +++ b/bookwyrm/tests/views/inbox/test_inbox_delete.py @@ -11,6 +11,7 @@ from bookwyrm import models, views class InboxActivities(TestCase): """inbox tests""" + # pylint: disable=invalid-name def setUp(self): """basic user and book data""" with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( @@ -97,7 +98,8 @@ class InboxActivities(TestCase): self.assertEqual(models.Notification.objects.get(), notif) @patch("bookwyrm.suggested_users.remove_user_task.delay") - def test_delete_user(self, _): + @patch("bookwyrm.activitystreams.remove_status_task.delay") + def test_delete_user(self, *_): """delete a user""" self.assertTrue(models.User.objects.get(username="rat@example.com").is_active) activity = { From ee6e3ed7eb3ef6be989cd4bd594901d68be7b8a0 Mon Sep 17 00:00:00 2001 From: Mouse Reeve Date: Sun, 5 Nov 2023 20:25:51 -0800 Subject: [PATCH 10/10] Adds a database field for is_deleted on user --- .../migrations/0183_auto_20231105_1607.py | 22 +++------ .../migrations/0184_auto_20231106_0421.py | 49 +++++++++++++++++++ bookwyrm/models/user.py | 34 +++++-------- .../templates/snippets/user_active_tag.html | 2 +- .../migrations/{test_0183.py => test_0184.py} | 17 +++---- bookwyrm/tests/models/test_user_model.py | 37 -------------- 6 files changed, 75 insertions(+), 86 deletions(-) create mode 100644 bookwyrm/migrations/0184_auto_20231106_0421.py rename bookwyrm/tests/migrations/{test_0183.py => test_0184.py} (92%) diff --git a/bookwyrm/migrations/0183_auto_20231105_1607.py b/bookwyrm/migrations/0183_auto_20231105_1607.py index 2716a0737..0c8376adc 100644 --- a/bookwyrm/migrations/0183_auto_20231105_1607.py +++ b/bookwyrm/migrations/0183_auto_20231105_1607.py @@ -1,18 +1,6 @@ # Generated by Django 3.2.20 on 2023-11-05 16:07 -from django.db import migrations -from bookwyrm.models import User - - -def erase_deleted_user_data(apps, schema_editor): - """Retroactively clear user data""" - for user in User.get_permanently_deleted_users(): - user.erase_user_data() - user.save( - broadcast=False, - update_fields=["email", "avatar", "preview_image", "summary", "name"], - ) - user.erase_user_statuses(broadcast=False) +from django.db import migrations, models class Migration(migrations.Migration): @@ -22,7 +10,9 @@ class Migration(migrations.Migration): ] operations = [ - migrations.RunPython( - erase_deleted_user_data, reverse_code=migrations.RunPython.noop - ) + migrations.AddField( + model_name="user", + name="is_deleted", + field=models.BooleanField(default=False), + ), ] diff --git a/bookwyrm/migrations/0184_auto_20231106_0421.py b/bookwyrm/migrations/0184_auto_20231106_0421.py new file mode 100644 index 000000000..e8197dea1 --- /dev/null +++ b/bookwyrm/migrations/0184_auto_20231106_0421.py @@ -0,0 +1,49 @@ +# Generated by Django 3.2.20 on 2023-11-06 04:21 + +from django.db import migrations +from bookwyrm.models import User + + +def update_deleted_users(apps, schema_editor): + """Find all the users who are deleted, not just inactive, and set deleted""" + users = apps.get_model("bookwyrm", "User") + db_alias = schema_editor.connection.alias + users.objects.using(db_alias).filter( + is_active=False, + deactivation_reason__in=[ + "self_deletion", + "moderator_deletion", + ], + ).update(is_deleted=True) + + # differente rules for remote users + users.objects.using(db_alias).filter(is_active=False, local=False,).exclude( + deactivation_reason="moderator_deactivation", + ).update(is_deleted=True) + + +def erase_deleted_user_data(apps, schema_editor): + """Retroactively clear user data""" + for user in User.objects.filter(is_deleted=True): + user.erase_user_data() + user.save( + broadcast=False, + update_fields=["email", "avatar", "preview_image", "summary", "name"], + ) + user.erase_user_statuses(broadcast=False) + + +class Migration(migrations.Migration): + + dependencies = [ + ("bookwyrm", "0183_auto_20231105_1607"), + ] + + operations = [ + migrations.RunPython( + update_deleted_users, reverse_code=migrations.RunPython.noop + ), + migrations.RunPython( + erase_deleted_user_data, reverse_code=migrations.RunPython.noop + ), + ] diff --git a/bookwyrm/models/user.py b/bookwyrm/models/user.py index 19006f772..75ca1d527 100644 --- a/bookwyrm/models/user.py +++ b/bookwyrm/models/user.py @@ -8,7 +8,7 @@ from django.contrib.auth.models import AbstractUser from django.contrib.postgres.fields import ArrayField, CICharField from django.core.exceptions import PermissionDenied, ObjectDoesNotExist from django.dispatch import receiver -from django.db import models, transaction +from django.db import models, transaction, IntegrityError from django.utils import timezone from django.utils.translation import gettext_lazy as _ from model_utils import FieldTracker @@ -54,6 +54,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): username = fields.UsernameField() email = models.EmailField(unique=True, null=True) + is_deleted = models.BooleanField(default=False) key_pair = fields.OneToOneField( "KeyPair", @@ -263,17 +264,6 @@ class User(OrderedCollectionPageMixin, AbstractUser): is_active=True, ).distinct() - @classmethod - def get_permanently_deleted_users(cls): - """a list of users who are permanently deleted""" - return cls.objects.filter( - is_active=False, - deactivation_reason__in=[ - "self_deletion", - "moderator_deletion", - ], - ).distinct() - def update_active_date(self): """this user is here! they are doing things!""" self.last_active_date = timezone.now() @@ -407,6 +397,7 @@ class User(OrderedCollectionPageMixin, AbstractUser): # pylint: disable=attribute-defined-outside-init self.is_active = False self.allow_reactivation = False + self.is_deleted = True self.erase_user_data() self.erase_user_statuses() @@ -419,6 +410,11 @@ class User(OrderedCollectionPageMixin, AbstractUser): def erase_user_data(self): """Wipe a user's custom data""" + if not self.is_deleted: + raise IntegrityError( + "Trying to erase user data on user that is not deleted" + ) + # mangle email address self.email = f"{uuid4()}@deleted.user" @@ -431,18 +427,14 @@ class User(OrderedCollectionPageMixin, AbstractUser): def erase_user_statuses(self, broadcast=True): """Wipe the data on all the user's statuses""" + if not self.is_deleted: + raise IntegrityError( + "Trying to erase user data on user that is not deleted" + ) + for status in self.status_set.all(): status.delete(broadcast=broadcast) - @property - def is_permanently_deleted(self): - """is this user inactive, or really truly deleted?""" - return not self.is_active and self.deactivation_reason in [ - "self_deletion", - "moderator_deletion", - "activitypub_deletion", - ] - def deactivate(self): """Disable the user but allow them to reactivate""" # pylint: disable=attribute-defined-outside-init diff --git a/bookwyrm/templates/snippets/user_active_tag.html b/bookwyrm/templates/snippets/user_active_tag.html index c3f067b43..1d85ae68b 100644 --- a/bookwyrm/templates/snippets/user_active_tag.html +++ b/bookwyrm/templates/snippets/user_active_tag.html @@ -8,7 +8,7 @@ {% trans "Active" as text %} {% include "snippets/user_active_tag_item.html" with icon="check" text=text level="success" %} {% endif %} -{% elif user.is_permanently_deleted %} +{% elif user.is_deleted %} {% trans "Deleted" as text %} {% include "snippets/user_active_tag_item.html" with icon="x" text=text level="danger" deactivation_reason=user.get_deactivation_reason_display %} {% else %} diff --git a/bookwyrm/tests/migrations/test_0183.py b/bookwyrm/tests/migrations/test_0184.py similarity index 92% rename from bookwyrm/tests/migrations/test_0183.py rename to bookwyrm/tests/migrations/test_0184.py index 8c8f27d5a..4bf1b66c9 100644 --- a/bookwyrm/tests/migrations/test_0183.py +++ b/bookwyrm/tests/migrations/test_0184.py @@ -1,23 +1,20 @@ """ testing migrations """ -import json from unittest.mock import patch -from django.apps import apps from django.test import TestCase from django.db.migrations.executor import MigrationExecutor from django.db import connection -import responses from bookwyrm import models from bookwyrm.management.commands import initdb -from bookwyrm.settings import USE_HTTPS, DOMAIN +from bookwyrm.settings import DOMAIN # pylint: disable=missing-class-docstring # pylint: disable=missing-function-docstring class EraseDeletedUserDataMigration(TestCase): - migrate_from = "0182_auto_20231027_1122" - migrate_to = "0183_auto_20231105_1607" + migrate_from = "0183_auto_20231105_1607" + migrate_to = "0184_auto_20231106_0421" # pylint: disable=invalid-name def setUp(self): @@ -68,11 +65,6 @@ class EraseDeletedUserDataMigration(TestCase): initdb.init_groups() initdb.init_permissions() - assert ( - self.migrate_from and self.migrate_to - ), "TestCase '{}' must define migrate_from and migrate_to properties".format( - type(self).__name__ - ) self.migrate_from = [("bookwyrm", self.migrate_from)] self.migrate_to = [("bookwyrm", self.migrate_to)] executor = MigrationExecutor(connection) @@ -104,12 +96,14 @@ class EraseDeletedUserDataMigration(TestCase): self.deleted_status.refresh_from_db() self.assertTrue(self.active_user.is_active) + self.assertFalse(self.active_user.is_deleted) self.assertEqual(self.active_user.name, "a name") self.assertNotEqual(self.deleted_user.email, "activeuser@activeuser.activeuser") self.assertFalse(self.active_status.deleted) self.assertEqual(self.active_status.content, "don't delete me") self.assertFalse(self.inactive_user.is_active) + self.assertFalse(self.inactive_user.is_deleted) self.assertEqual(self.inactive_user.name, "name name") self.assertNotEqual( self.deleted_user.email, "inactiveuser@inactiveuser.inactiveuser" @@ -118,6 +112,7 @@ class EraseDeletedUserDataMigration(TestCase): self.assertEqual(self.inactive_status.content, "also don't delete me") self.assertFalse(self.deleted_user.is_active) + self.assertTrue(self.deleted_user.is_deleted) self.assertIsNone(self.deleted_user.name) self.assertNotEqual( self.deleted_user.email, "deleteduser@deleteduser.deleteduser" diff --git a/bookwyrm/tests/models/test_user_model.py b/bookwyrm/tests/models/test_user_model.py index 3576417de..30d7918c0 100644 --- a/bookwyrm/tests/models/test_user_model.py +++ b/bookwyrm/tests/models/test_user_model.py @@ -325,40 +325,3 @@ class User(TestCase): results = models.User.admins() self.assertEqual(results.count(), 1) self.assertEqual(results.first(), self.user) - - def test_get_permanently_deleted_users(self): - - with patch("bookwyrm.suggested_users.rerank_suggestions_task.delay"), patch( - "bookwyrm.activitystreams.populate_stream_task.delay" - ), patch("bookwyrm.lists_stream.populate_lists_task.delay"): - models.User.objects.create_user( - f"activeuser@{DOMAIN}", - "activeuser@activeuser.activeuser", - "activeuserword", - local=True, - localname="active", - ) - models.User.objects.create_user( - f"deleteduser@{DOMAIN}", - "deleteduser@deleteduser.deleteduser", - "deleteduserword", - local=True, - localname="deleted", - is_active=False, - deactivation_reason="self_deletion", - ) - models.User.objects.create_user( - f"inactiveuser@{DOMAIN}", - "inactiveuser@inactiveuser.inactiveuser", - "inactiveuserword", - local=True, - localname="inactive", - is_active=False, - deactivation_reason="self_deactivation", - ) - - deleted_users = models.User.get_permanently_deleted_users() - - self.assertTrue(deleted_users.filter(localname="deleted").exists()) - self.assertFalse(deleted_users.filter(localname="active").exists()) - self.assertFalse(deleted_users.filter(localname="inactive").exists())