From 54ab4af489bc23859a3b07cee212db813f67bc5a 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 | 37 ++++++ pydis_site/apps/api/models/bot/user.py | 37 ++++++ pydis_site/apps/api/serializers.py | 6 +- pydis_site/apps/api/tests/test_infractions.py | 2 +- pydis_site/apps/api/tests/test_users.py | 120 ++++++++++++++++++ pydis_site/apps/api/viewsets/bot/user.py | 49 ++++++- 6 files changed, 244 insertions(+), 7 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..74c7341d57 --- /dev/null +++ b/pydis_site/apps/api/migrations/0093_user_alts.py @@ -0,0 +1,37 @@ +# Generated by Django 5.0 on 2023-12-12 09:01 + +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')), + ('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/bot/user.py b/pydis_site/apps/api/models/bot/user.py index afc5ba1eef..81e74984b8 100644 --- a/pydis_site/apps/api/models/bot/user.py +++ b/pydis_site/apps/api/models/bot/user.py @@ -61,6 +61,12 @@ class User(ModelReprMixin, models.Model): help_text="Whether this user is in our server.", verbose_name="In Guild" ) + alts = models.ManyToManyField( + 'self', + through='UserAltRelationship', + 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 +92,34 @@ def username(self) -> str: For usability in read-only fields such as Django Admin. """ return str(self) + +class UserAltRelationship(ModelReprMixin, 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", + ) + + 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..9a1a510df6 100644 --- a/pydis_site/apps/api/serializers.py +++ b/pydis_site/apps/api/serializers.py @@ -669,7 +669,7 @@ def update(self, queryset: QuerySet, validated_data: list) -> list: return updated -class UserSerializer(ModelSerializer): +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 +679,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..e55114d4a9 100644 --- a/pydis_site/apps/api/tests/test_users.py +++ b/pydis_site/apps/api/tests/test_users.py @@ -60,6 +60,7 @@ def setUpTestData(cls): def test_accepts_valid_data(self): url = reverse('api:bot:user-list') data = { + 'alts': [], 'id': 42, 'name': "Test", 'discriminator': 42, @@ -199,6 +200,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 +208,7 @@ def test_multiple_users_patch(self): "in_guild": False }, { + "alts": [], "id": 2, "name": "User 2 patched!" } @@ -646,3 +649,120 @@ 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_alts(self) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + data = [self.user_2.id] + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 200) + + updated_user = response.json() + self.assertEqual(len(updated_user['alts']), 1) + self.assertEqual(updated_user['alts'][0]['id'], self.user_2.id) + + 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_removing_existing_alts(self) -> None: + self.user_1.alts.set((self.user_2.id,)) + self.user_1.save() + + data = [] + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + response = self.client.patch(url, data) + + self.assertEqual(response.status_code, 200) + updated_user = response.json() + self.assertEqual(updated_user['alts'], []) + + 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_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 = [self.user_2.id] + response = self.client.patch(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 = [self.user_1.id + self.user_2.id] + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + 'non_field_errors': ["One or more unknown user IDs were passed"] + }) + + 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 = [self.user_1.id] + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + 'non_field_errors': ["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.""" + 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..461cf64fdf 100644 --- a/pydis_site/apps/api/viewsets/bot/user.py +++ b/pydis_site/apps/api/viewsets/bot/user.py @@ -1,5 +1,6 @@ from collections import OrderedDict +from django.db import IntegrityError from django.db.models import Q from django_filters.rest_framework import DjangoFilterBackend from rest_framework import fields, status @@ -203,8 +204,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 +222,24 @@ 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 + ### PATCH /bot/users//alts + Update the alternative accounts for the given user snowflake. The request + body contains the user IDs to link to this user, as a plain list. IDs will + be linked symmetrically. Returns the updated user. + + #### Request body + >>> [int, ...] + + #### Status codes + - 200: returned on success + - 400: if the request body was invalid, including if one of the users in + the request body could not be found in the database + - 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 +301,30 @@ def bulk_patch(self, request: Request) -> Response: return Response(serializer.data, status=status.HTTP_200_OK) + @action(detail=True, methods=['PATCH'], name='user-alts-update', url_name='alts', url_path='alts') + def update_alts(self, request: Request, pk: str) -> Response: + """Update the user's alternate accounts to the given list.""" + user = self.get_object() + if not isinstance(request.data, list): + raise ParseError(detail={"non_field_errors": ["Request body should be a list"]}) + if not all(isinstance(item, int) for item in request.data): + raise ParseError(detail={"non_field_errors": ["List payload should only contain numbers"]}) + users = User.objects.filter(id__in=request.data) + if len(users) != len(request.data): + raise ParseError(detail={"non_field_errors": ["One or more unknown user IDs were passed"]}) + try: + user.alts.set(users) + except IntegrityError as err: + if err.__cause__.diag.constraint_name == 'api_useraltrelationship_prevent_alt_to_self': + raise ParseError(detail={ + "non_field_errors": ["The user may not be an alternate account of itself"] + }) + + raise + + serializer = self.get_serializer(instance=user) + return Response(serializer.data, status=status.HTTP_200_OK) + @action(detail=True) def metricity_data(self, request: Request, pk: str | None = None) -> Response: """Request handler for metricity_data endpoint."""