From d97d08c18ccd23f5950e201ecea4655b57784360 Mon Sep 17 00:00:00 2001 From: Johannes Christ Date: Tue, 12 Dec 2023 10:09:43 +0100 Subject: [PATCH] Add alternate accounts to the user model Introduce a way to store alternate accounts on the user, and add the `PATCH /bot/users//alts` endpoint, which allows updating the user's alt accounts to the alt accounts in the request.. --- .../apps/api/migrations/0093_user_alts.py | 41 ++++ pydis_site/apps/api/models/__init__.py | 3 +- pydis_site/apps/api/models/bot/__init__.py | 2 +- pydis_site/apps/api/models/bot/user.py | 51 +++- pydis_site/apps/api/serializers.py | 25 +- pydis_site/apps/api/tests/test_infractions.py | 2 +- pydis_site/apps/api/tests/test_users.py | 217 +++++++++++++++++- pydis_site/apps/api/viewsets/bot/user.py | 107 ++++++++- 8 files changed, 434 insertions(+), 14 deletions(-) create mode 100644 pydis_site/apps/api/migrations/0093_user_alts.py diff --git a/pydis_site/apps/api/migrations/0093_user_alts.py b/pydis_site/apps/api/migrations/0093_user_alts.py new file mode 100644 index 0000000000..c38b3e0b66 --- /dev/null +++ b/pydis_site/apps/api/migrations/0093_user_alts.py @@ -0,0 +1,41 @@ +# Generated by Django 5.0 on 2023-12-14 08:50 + +import django.db.models.deletion +import pydis_site.apps.api.models.mixins +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0092_remove_redirect_filter_list'), + ] + + operations = [ + migrations.CreateModel( + name='UserAltRelationship', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created_at', models.DateTimeField(auto_now_add=True)), + ('updated_at', models.DateTimeField(auto_now=True)), + ('context', models.TextField(help_text='The reason for why this account was associated as an alt.', max_length=1900)), + ('actor', models.ForeignKey(help_text='The moderator that associated these accounts together.', on_delete=django.db.models.deletion.CASCADE, related_name='alts_marked', to='api.user')), + ('source', models.ForeignKey(help_text='The source of this user to alternate account relationship', on_delete=django.db.models.deletion.CASCADE, to='api.user', verbose_name='Source')), + ('target', models.ForeignKey(help_text='The target of this user to alternate account relationship', on_delete=django.db.models.deletion.CASCADE, related_name='+', to='api.user', verbose_name='Target')), + ], + bases=(pydis_site.apps.api.models.mixins.ModelReprMixin, models.Model), + ), + migrations.AddField( + model_name='user', + name='alts', + field=models.ManyToManyField(help_text='Known alternate accounts of this user. Manually linked.', through='api.UserAltRelationship', to='api.user', verbose_name='Alternative accounts'), + ), + migrations.AddConstraint( + model_name='useraltrelationship', + constraint=models.UniqueConstraint(fields=('source', 'target'), name='api_useraltrelationship_unique_relationships'), + ), + migrations.AddConstraint( + model_name='useraltrelationship', + constraint=models.CheckConstraint(check=models.Q(('source', models.F('target')), _negated=True), name='api_useraltrelationship_prevent_alt_to_self'), + ), + ] diff --git a/pydis_site/apps/api/models/__init__.py b/pydis_site/apps/api/models/__init__.py index fee4c8d590..c6eb5d1516 100644 --- a/pydis_site/apps/api/models/__init__.py +++ b/pydis_site/apps/api/models/__init__.py @@ -17,5 +17,6 @@ OffTopicChannelName, Reminder, Role, - User + User, + UserAltRelationship ) diff --git a/pydis_site/apps/api/models/bot/__init__.py b/pydis_site/apps/api/models/bot/__init__.py index 6f09473d27..4c9ed67644 100644 --- a/pydis_site/apps/api/models/bot/__init__.py +++ b/pydis_site/apps/api/models/bot/__init__.py @@ -14,4 +14,4 @@ from .offensive_message import OffensiveMessage from .reminder import Reminder from .role import Role -from .user import User +from .user import User, UserAltRelationship diff --git a/pydis_site/apps/api/models/bot/user.py b/pydis_site/apps/api/models/bot/user.py index afc5ba1eef..07ac013430 100644 --- a/pydis_site/apps/api/models/bot/user.py +++ b/pydis_site/apps/api/models/bot/user.py @@ -4,7 +4,7 @@ from django.db import models from pydis_site.apps.api.models.bot.role import Role -from pydis_site.apps.api.models.mixins import ModelReprMixin +from pydis_site.apps.api.models.mixins import ModelReprMixin, ModelTimestampMixin def _validate_existing_role(value: int) -> None: @@ -61,6 +61,13 @@ class User(ModelReprMixin, models.Model): help_text="Whether this user is in our server.", verbose_name="In Guild" ) + alts = models.ManyToManyField( + 'self', + through='UserAltRelationship', + through_fields=('source', 'target'), + help_text="Known alternate accounts of this user. Manually linked.", + verbose_name="Alternative accounts" + ) def __str__(self): """Returns the name and discriminator for the current user, for display purposes.""" @@ -86,3 +93,45 @@ def username(self) -> str: For usability in read-only fields such as Django Admin. """ return str(self) + + +class UserAltRelationship(ModelReprMixin, ModelTimestampMixin, models.Model): + """A relationship between a Discord user and its alts.""" + + source = models.ForeignKey( + User, + on_delete=models.CASCADE, + verbose_name="Source", + help_text="The source of this user to alternate account relationship", + ) + target = models.ForeignKey( + User, + on_delete=models.CASCADE, + verbose_name="Target", + related_name='+', + help_text="The target of this user to alternate account relationship", + ) + context = models.TextField( + help_text="The reason for why this account was associated as an alt.", + max_length=1900 + ) + actor = models.ForeignKey( + User, + on_delete=models.CASCADE, + related_name='alts_marked', + help_text="The moderator that associated these accounts together." + ) + + class Meta: + """Add constraints to prevent users from being an alt of themselves.""" + + constraints = [ + models.UniqueConstraint( + name="%(app_label)s_%(class)s_unique_relationships", + fields=["source", "target"] + ), + models.CheckConstraint( + name="%(app_label)s_%(class)s_prevent_alt_to_self", + check=~models.Q(source=models.F("target")), + ), + ] diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py index 87fd6190d5..84fda59551 100644 --- a/pydis_site/apps/api/serializers.py +++ b/pydis_site/apps/api/serializers.py @@ -33,7 +33,8 @@ OffensiveMessage, Reminder, Role, - User + User, + UserAltRelationship ) class FrozenFieldsMixin: @@ -669,7 +670,23 @@ def update(self, queryset: QuerySet, validated_data: list) -> list: return updated -class UserSerializer(ModelSerializer): +class UserAltRelationshipSerializer(FrozenFieldsMixin, ModelSerializer): + """A class providing (de-)serialization of `UserAltRelationship` instances.""" + + actor = PrimaryKeyRelatedField(queryset=User.objects.all()) + source = PrimaryKeyRelatedField(queryset=User.objects.all()) + target = PrimaryKeyRelatedField(queryset=User.objects.all()) + + class Meta: + """Metadata defined for the Django REST Framework.""" + + model = UserAltRelationship + fields = ('source', 'target', 'actor', 'context', 'created_at', 'updated_at') + frozen_fields = ('source', 'target', 'actor') + depth = 1 + + +class UserSerializer(FrozenFieldsMixin, ModelSerializer): """A class providing (de-)serialization of `User` instances.""" # ID field must be explicitly set as the default id field is read-only. @@ -679,7 +696,9 @@ class Meta: """Metadata defined for the Django REST Framework.""" model = User - fields = ('id', 'name', 'discriminator', 'roles', 'in_guild') + fields = ('id', 'name', 'discriminator', 'roles', 'in_guild', 'alts') + # This should be edited on a separate endpoint only + frozen_fields = ('alts',) depth = 1 list_serializer_class = UserListSerializer diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index f1e54b1e06..c1c859ee9b 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -747,7 +747,7 @@ def setUpTestData(cls): def check_expanded_fields(self, infraction): for key in ('user', 'actor'): obj = infraction[key] - for field in ('id', 'name', 'discriminator', 'roles', 'in_guild'): + for field in ('id', 'name', 'discriminator', 'roles', 'in_guild', 'alts'): self.assertTrue(field in obj, msg=f'field "{field}" missing from {key}') def test_list_expanded(self): diff --git a/pydis_site/apps/api/tests/test_users.py b/pydis_site/apps/api/tests/test_users.py index cff4a825e6..79c1e99c07 100644 --- a/pydis_site/apps/api/tests/test_users.py +++ b/pydis_site/apps/api/tests/test_users.py @@ -60,6 +60,8 @@ def setUpTestData(cls): def test_accepts_valid_data(self): url = reverse('api:bot:user-list') data = { + # This field should be optional. + # 'alts': [], 'id': 42, 'name': "Test", 'discriminator': 42, @@ -68,10 +70,11 @@ def test_accepts_valid_data(self): ], 'in_guild': True } + data_with_alts = {'alts': [], **data} response = self.client.post(url, data=data) self.assertEqual(response.status_code, 201) - self.assertEqual(response.json(), data) + self.assertEqual(response.json(), data_with_alts) user = User.objects.get(id=42) self.assertEqual(user.name, data['name']) @@ -199,6 +202,7 @@ def test_multiple_users_patch(self): url = reverse("api:bot:user-bulk-patch") data = [ { + "alts": [], "id": 1, "name": "User 1 patched!", "discriminator": 1010, @@ -206,6 +210,7 @@ def test_multiple_users_patch(self): "in_guild": False }, { + "alts": [], "id": 2, "name": "User 2 patched!" } @@ -646,3 +651,213 @@ def test_search_lookup_of_unknown_user(self) -> None: result = response.json() self.assertEqual(result['count'], 0) self.assertEqual(result['results'], []) + + +class UserAltUpdateTests(AuthenticatedAPITestCase): + @classmethod + def setUpTestData(cls): + cls.user_1 = User.objects.create( + id=12095219, + name=f"Test user {random.randint(100, 1000)}", + discriminator=random.randint(1, 9999), + in_guild=True, + ) + cls.user_2 = User.objects.create( + id=18259125, + name=f"Test user {random.randint(100, 1000)}", + discriminator=random.randint(1, 9999), + in_guild=True, + ) + + def test_adding_existing_alt(self) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = { + 'target': self.user_2.id, + 'actor': self.user_1.id, + 'context': "Joe's trolling account" + } + response = self.client.post(url, data) + self.assertEqual(response.status_code, 204) + + self.user_1.refresh_from_db() + self.user_2.refresh_from_db() + + self.assertQuerySetEqual(self.user_1.alts.all(), [self.user_2]) + self.assertQuerySetEqual(self.user_2.alts.all(), [self.user_1]) + + def test_adding_existing_alt_twice_via_source(self) -> None: + self.verify_adding_existing_alt(add_on_source=True) + + def test_adding_existing_alt_twice_via_target(self) -> None: + self.verify_adding_existing_alt(add_on_source=False) + + def verify_adding_existing_alt(self, add_on_source: bool) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = { + 'target': self.user_2.id, + 'actor': self.user_1.id, + 'context': "Lemon's trolling account" + } + initial_response = self.client.post(url, data) + self.assertEqual(initial_response.status_code, 204) + + if add_on_source: + repeated_url = url + repeated_data = data + else: + repeated_url = reverse('api:bot:user-alts', args=(self.user_2.id,)) + repeated_data = { + 'target': self.user_1.id, + 'actor': self.user_1.id, + 'context': data['context'] + } + + response = self.client.post(repeated_url, repeated_data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + 'source': ["This relationship has already been established"] + }) + + def test_removing_existing_alt_source_from_target(self) -> None: + self.verify_deletion(delete_on_source=False) + + def test_removing_existing_alt_target_from_source(self) -> None: + self.verify_deletion(delete_on_source=True) + + def verify_deletion(self, delete_on_source: bool) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = { + 'target': self.user_2.id, + 'actor': self.user_1.id, + 'context': "Lemon's trolling account" + } + initial_response = self.client.post(url, data) + self.assertEqual(initial_response.status_code, 204) + + self.assertTrue(self.user_1.alts.all().exists()) + self.assertTrue(self.user_2.alts.all().exists()) + + if delete_on_source: + data = self.user_2.id + alts_url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + else: + data = self.user_1.id + alts_url = reverse('api:bot:user-alts', args=(self.user_2.id,)) + + response = self.client.delete(alts_url, data) + + self.assertEqual(response.status_code, 204) + + self.user_1.refresh_from_db() + self.user_2.refresh_from_db() + + self.assertFalse(self.user_1.alts.all().exists()) + self.assertFalse(self.user_2.alts.all().exists()) + + def test_removing_unknown_alt(self) -> None: + data = self.user_1.id + self.user_2.id + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + response = self.client.delete(url, data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + 'non_field_errors': ["Specified account is not a known alternate account of this user"] + }) + + def test_add_alt_returns_error_for_missing_keys_in_request_body(self) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = {'hello': 'joe'} + response = self.client.post(url, data) + self.assertEqual(response.status_code, 400) + + def test_remove_alt_returns_error_for_non_int_request_body(self) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = {'hello': 'joe'} + response = self.client.delete(url, data) + self.assertEqual(response.status_code, 400) + + def test_adding_alt_to_user_that_does_not_exist(self) -> None: + """Patching a user's alts for a user that doesn't exist should return a 404.""" + url = reverse('api:bot:user-alts', args=(self.user_1.id + self.user_2.id,)) + data = { + 'target': self.user_2.id, + 'actor': self.user_1.id, + 'context': "Chris's trolling account" + } + response = self.client.post(url, data) + self.assertEqual(response.status_code, 404) + + def test_adding_alt_that_does_not_exist_to_user(self) -> None: + """Patching a user's alts with an alt that is unknown should return a 400.""" + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = { + 'target': self.user_1.id + self.user_2.id, + 'actor': self.user_1.id, + 'context': "Hello, Joe" + } + response = self.client.post(url, data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + 'target': [f'Invalid pk "{data["target"]}" - object does not exist.'] + }) + + def test_cannot_add_self_as_alt_account(self) -> None: + """The existing account may not be an alt of itself.""" + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = { + 'target': self.user_1.id, + 'actor': self.user_1.id, + 'context': "Schizophrenia" + } + response = self.client.post(url, data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + 'source': ["The user may not be an alternate account of itself"] + }) + + def test_cannot_update_alts_on_regular_user_patch_route(self) -> None: + """The regular user update route does not allow editing the alts.""" + url = reverse('api:bot:user-detail', args=(self.user_1.id,)) + data = {'alts': [self.user_2.id]} + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 200) # XXX: This seems to be a DRF bug + self.assertEqual(response.json()['alts'], []) + + self.user_1.refresh_from_db() + self.assertQuerySetEqual(self.user_1.alts.all(), ()) + self.user_2.refresh_from_db() + self.assertQuerySetEqual(self.user_2.alts.all(), ()) + + def test_cannot_update_alts_on_bulk_user_patch_route(self) -> None: + """The bulk user update route does not allow editing the alts.""" + url = reverse('api:bot:user-bulk-patch') + data = [{'id': self.user_1.id, 'alts': [self.user_2.id]}] + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), {'non_field_errors': ['Insufficient data provided.']}) + + self.user_1.refresh_from_db() + self.assertQuerySetEqual(self.user_1.alts.all(), ()) + self.user_2.refresh_from_db() + self.assertQuerySetEqual(self.user_2.alts.all(), ()) + + def test_user_bulk_patch_does_not_discard_alts(self) -> None: + """The bulk user update route should not modify the alts.""" + alts_url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = { + 'target': self.user_2.id, + 'actor': self.user_2.id, + 'context': "This is my testing account" + } + alts_response = self.client.post(alts_url, data) + self.assertEqual(alts_response.status_code, 204) + + url = reverse('api:bot:user-bulk-patch') + self.user_1.alts.set((self.user_2,)) + data = [{'id': self.user_1.id, 'name': "Joe Armstrong"}] + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 200) + + self.user_1.refresh_from_db() + self.assertQuerySetEqual(self.user_1.alts.all(), (self.user_2,)) + self.user_2.refresh_from_db() + self.assertQuerySetEqual(self.user_2.alts.all(), (self.user_1,)) diff --git a/pydis_site/apps/api/viewsets/bot/user.py b/pydis_site/apps/api/viewsets/bot/user.py index 77378336e7..5a6854d8da 100644 --- a/pydis_site/apps/api/viewsets/bot/user.py +++ b/pydis_site/apps/api/viewsets/bot/user.py @@ -1,5 +1,7 @@ -from collections import OrderedDict +from collections import ChainMap, OrderedDict +from django.core.exceptions import ObjectDoesNotExist +from django.db import IntegrityError, transaction from django.db.models import Q from django_filters.rest_framework import DjangoFilterBackend from rest_framework import fields, status @@ -14,7 +16,7 @@ from pydis_site.apps.api.models.bot.infraction import Infraction from pydis_site.apps.api.models.bot.metricity import Metricity, NotFoundError from pydis_site.apps.api.models.bot.user import User -from pydis_site.apps.api.serializers import UserSerializer +from pydis_site.apps.api.serializers import UserSerializer, UserAltRelationshipSerializer class UserListPagination(PageNumberPagination): @@ -203,8 +205,9 @@ class UserViewSet(ModelViewSet): - 404: if the user with the given `snowflake` could not be found ### PATCH /bot/users/ - Update the user with the given `snowflake`. - All fields in the request body are optional. + Update the user with the given `snowflake`. All fields in the request body + are optional. Note that editing the `'alts'` field is not possible this + way, use the `alts` endpoint documented below for this. #### Request body >>> { @@ -220,9 +223,45 @@ class UserViewSet(ModelViewSet): - 400: if the request body was invalid, see response body for details - 404: if the user with the given `snowflake` could not be found + ### POST /bot/users//alts + Add the alternative account given in the request body to the alternative + accounts for the given user snowflake. Users will be linked symmetrically. + Returns the updated user. + + #### Request body + >>> { + ... # The target alternate account to associate the user (from the URL) with. + ... 'target': int, + ... # A description for why this relationship was established. + ... 'context': str, + ... # The moderator that associated the accounts together. + ... 'actor': int + ... } + + #### Status codes + - 200: returned on success + - 400: if the request body was invalid, including if the user in the + request body could not be found in the database + - 404: if the user with the given `snowflake` could not be found + + ### DELETE /bot/users//alts + Remove the alternative account given in the request body from the + alternative accounts of the given user snowflake. The request body contains + the user ID to remove from the association to this user, as a plain + integer. Users will be linked symmetrically. Returns the updated user. + + #### Request body + >>> int + + #### Status codes + - 204: returned on success + - 400: if the user in the request body was not found as an alt account + - 404: if the user with the given `snowflake` could not be found + ### BULK PATCH /bot/users/bulk_patch - Update users with the given `ids` and `details`. - `id` field and at least one other field is mandatory. + Update users with the given `ids` and `details`. `id` field and at least + one other field is mandatory. Note that editing the `'alts'` field is not + possible using this endpoint. #### Request body >>> [ @@ -284,6 +323,62 @@ def bulk_patch(self, request: Request) -> Response: return Response(serializer.data, status=status.HTTP_200_OK) + @action(detail=True, methods=['POST'], name="Add alternate account", + url_name='alts', url_path='alts') + def add_alt(self, request: Request, pk: str) -> Response: + """Associate the given account to the user's alternate accounts.""" + user_id = self.get_object().id + source_serializer_data = ChainMap({'source': user_id}, request.data) + source_serializer = UserAltRelationshipSerializer(data=source_serializer_data) + source_serializer.is_valid(raise_exception=True) + + target_id = source_serializer.validated_data['target'].id + target_serializer_data = ChainMap({'source': target_id, 'target': user_id}, request.data) + target_serializer = UserAltRelationshipSerializer(data=target_serializer_data) + target_serializer.is_valid(raise_exception=True) # should not fail, or inconsistent db + + with transaction.atomic(): + try: + source_serializer.save() + target_serializer.save() + except IntegrityError as err: + if err.__cause__.diag.constraint_name == 'api_useraltrelationship_prevent_alt_to_self': + raise ParseError(detail={ + "source": ["The user may not be an alternate account of itself"] + }) + if err.__cause__.diag.constraint_name == 'api_useraltrelationship_unique_relationships': + raise ParseError(detail={ + "source": ["This relationship has already been established"] + }) + + raise + + return Response(status=status.HTTP_204_NO_CONTENT) + + # See https://www.django-rest-framework.org/api-guide/viewsets/#routing-additional-http-methods-for-extra-actions + @add_alt.mapping.delete + def remove_alt(self, request: Request, pk: str) -> Response: + """Disassociate the given account from the user's alternate accounts.""" + user = self.get_object() + if not isinstance(request.data, int): + raise ParseError(detail={"non_field_errors": ["Request body should be an integer"]}) + try: + alt = user.alts.get(id=request.data) + except ObjectDoesNotExist: + raise ParseError(detail={ + 'non_field_errors': ["Specified account is not a known alternate account of this user"] + }) + + # It is mandatory that this query performs a symmetrical delete, + # because our custom ManyToManyField through model specifies more than + # two foreign keys to the user model, causing Django to no longer + # uphold the symmetry of the model. Correct function is verified by + # the tests, but in case you end up changing it, make sure that it + # works as expected! + user.alts.remove(alt) + + return Response(status=status.HTTP_204_NO_CONTENT) + @action(detail=True) def metricity_data(self, request: Request, pk: str | None = None) -> Response: """Request handler for metricity_data endpoint."""