diff options
Diffstat (limited to '')
| -rw-r--r-- | pydis_site/apps/api/migrations/0093_user_alts.py | 41 | ||||
| -rw-r--r-- | pydis_site/apps/api/migrations/0096_merge_0093_user_alts_0095_user_display_name.py | 14 | ||||
| -rw-r--r-- | pydis_site/apps/api/models/__init__.py | 3 | ||||
| -rw-r--r-- | pydis_site/apps/api/models/bot/__init__.py | 2 | ||||
| -rw-r--r-- | pydis_site/apps/api/models/bot/user.py | 51 | ||||
| -rw-r--r-- | pydis_site/apps/api/serializers.py | 56 | ||||
| -rw-r--r-- | pydis_site/apps/api/tests/test_infractions.py | 2 | ||||
| -rw-r--r-- | pydis_site/apps/api/tests/test_users.py | 446 | ||||
| -rw-r--r-- | pydis_site/apps/api/viewsets/bot/user.py | 183 | ||||
| -rw-r--r-- | pyproject.toml | 2 |
10 files changed, 785 insertions, 15 deletions
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 00000000..fa5f2102 --- /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 13:14 + +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='+', 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/migrations/0096_merge_0093_user_alts_0095_user_display_name.py b/pydis_site/apps/api/migrations/0096_merge_0093_user_alts_0095_user_display_name.py new file mode 100644 index 00000000..eaf2b66e --- /dev/null +++ b/pydis_site/apps/api/migrations/0096_merge_0093_user_alts_0095_user_display_name.py @@ -0,0 +1,14 @@ +# Generated by Django 5.0 on 2024-05-20 05:14 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0093_user_alts'), + ('api', '0095_user_display_name'), + ] + + operations = [ + ] diff --git a/pydis_site/apps/api/models/__init__.py b/pydis_site/apps/api/models/__init__.py index 5901c978..7ac463b1 100644 --- a/pydis_site/apps/api/models/__init__.py +++ b/pydis_site/apps/api/models/__init__.py @@ -19,5 +19,6 @@ from .bot import ( 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 c07a3238..a17c70ff 100644 --- a/pydis_site/apps/api/models/bot/__init__.py +++ b/pydis_site/apps/api/models/bot/__init__.py @@ -16,4 +16,4 @@ from .off_topic_channel_name import OffTopicChannelName 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 1cb10988..4d317b8e 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.core.validators import MaxValueValidator, MinValueValidator 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: @@ -66,6 +66,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.""" @@ -91,3 +98,45 @@ class User(ModelReprMixin, models.Model): 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='+', + 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 60d3637c..2b4bf395 100644 --- a/pydis_site/apps/api/serializers.py +++ b/pydis_site/apps/api/serializers.py @@ -11,6 +11,7 @@ from rest_framework.serializers import ( ListSerializer, ModelSerializer, PrimaryKeyRelatedField, + SerializerMethodField, ValidationError ) from rest_framework.settings import api_settings @@ -35,7 +36,8 @@ from .models import ( OffensiveMessage, Reminder, Role, - User + User, + UserAltRelationship ) class FrozenFieldsMixin: @@ -507,7 +509,7 @@ class ExpandedInfractionSerializer(InfractionSerializer): """Return the dictionary representation of this infraction.""" ret = super().to_representation(instance) - ret['user'] = UserSerializer(instance.user).data + ret['user'] = UserWithAltsSerializer(instance.user).data ret['actor'] = UserSerializer(instance.actor).data return ret @@ -663,6 +665,36 @@ class UserListSerializer(ListSerializer): return updated +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 + + def to_representation(self, instance: UserAltRelationship) -> dict: + """Add the alts of the target to the representation.""" + representation = super().to_representation(instance) + representation['alts'] = tuple( + user_id + for (user_id,) in ( + UserAltRelationship.objects + .filter(source=instance.target) + .values_list('target_id') + ) + ) + return representation + + + class UserSerializer(ModelSerializer): """A class providing (de-)serialization of `User` instances.""" @@ -685,6 +717,26 @@ class UserSerializer(ModelSerializer): raise ValidationError({"id": ["User with ID already present."]}) +class UserWithAltsSerializer(FrozenFieldsMixin, UserSerializer): + """A class providing (de-)serialization of `User` instances, expanding their alternate accounts.""" + + alts = SerializerMethodField() + + class Meta: + """Metadata defined for the Django REST Framework.""" + + model = User + fields = ('id', 'name', 'display_name', 'discriminator', 'roles', 'in_guild', 'alts') + frozen_fields = ('alts',) + + def get_alts(self, user: User) -> list[dict]: + """Retrieve the alts with all additional data on them.""" + return [ + UserAltRelationshipSerializer(alt).data + for alt in user.alts.through.objects.filter(source=user) + ] + + class NominationEntrySerializer(ModelSerializer): """A class providing (de-)serialization of `NominationEntry` instances.""" diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index f1e54b1e..82722285 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -749,6 +749,8 @@ class ExpandedTests(AuthenticatedAPITestCase): obj = infraction[key] for field in ('id', 'name', 'discriminator', 'roles', 'in_guild'): self.assertTrue(field in obj, msg=f'field "{field}" missing from {key}') + if key == 'user': + self.assertIn('alts', obj) def test_list_expanded(self): url = reverse('api:bot:infraction-list-expanded') diff --git a/pydis_site/apps/api/tests/test_users.py b/pydis_site/apps/api/tests/test_users.py index 5dda6344..3799b631 100644 --- a/pydis_site/apps/api/tests/test_users.py +++ b/pydis_site/apps/api/tests/test_users.py @@ -4,7 +4,7 @@ from unittest.mock import Mock, patch from django.urls import reverse from .base import AuthenticatedAPITestCase -from pydis_site.apps.api.models import Infraction, Role, User +from pydis_site.apps.api.models import Infraction, Role, User, UserAltRelationship from pydis_site.apps.api.models.bot.metricity import NotFoundError from pydis_site.apps.api.viewsets.bot.user import UserListPagination @@ -651,3 +651,447 @@ class UserViewSetTests(AuthenticatedAPITestCase): 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]) + + detail_url = reverse('api:bot:user-detail', args=(self.user_1.id,)) + detail_response = self.client.get(detail_url) + self.assertEqual(detail_response.status_code, 200) + + parsed_detail = detail_response.json() + [parsed_alt] = parsed_detail.pop('alts') + parsed_alt.pop('created_at') + parsed_alt.pop('updated_at') + + self.assertEqual( + parsed_detail, + { + 'id': self.user_1.id, + 'name': self.user_1.name, + 'display_name': self.user_1.display_name, + 'discriminator': self.user_1.discriminator, + 'roles': self.user_1.roles, + 'in_guild': self.user_1.in_guild, + } + ) + self.assertEqual( + parsed_alt, + { + 'source': self.user_1.id, + 'target': data['target'], + 'alts': [self.user_1.id], + 'actor': data['actor'], + 'context': data['context'], + } + ) + + 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.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,)) + + +class UserAltUpdateWithExistingAltsTests(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, + ) + cls.relationship_1 = UserAltRelationship.objects.create( + source=cls.user_1, + target=cls.user_2, + context="Test user's trolling account", + actor=cls.user_1 + ) + cls.relationship_2 = UserAltRelationship.objects.create( + source=cls.user_2, + target=cls.user_1, + context="Test user's trolling account", + actor=cls.user_1 + ) + + def test_returns_404_for_unknown_user(self) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id + self.user_2.id,)) + response = self.client.patch(url, {'target': self.user_2.id, 'context': "Dinoman"}) + self.assertEqual(response.status_code, 404) + + def test_returns_400_for_unknown_alt_from_source(self) -> None: + self.verify_returns_400_for_unknown_alt(from_source=True) + + def test_returns_400_for_unknown_alt_from_target(self) -> None: + self.verify_returns_400_for_unknown_alt(from_source=False) + + def verify_returns_400_for_unknown_alt(self, from_source: bool) -> None: + if from_source: + source = self.user_1.id + else: + source = self.user_2.id + + target = self.user_1.id + self.user_2.id + + url = reverse('api:bot:user-alts', args=(source,)) + data = {'target': target, 'context': "Still a trolling account"} + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + 'target': ["User is not an associated alt account"] + }) + + def test_returns_400_for_missing_fields(self) -> None: + url = reverse('api:bot:user-alts', args=(self.user_1.id,)) + payloads = [{'target': self.user_2.id}, {'context': "Confirmed"}] + for payload in payloads: + key = next(iter(payload)) + (missing_key,) = tuple({'target', 'context'} - {key}) + with self.subTest(specified_key=next(iter(payload))): + response = self.client.patch(url, payload) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), { + missing_key: ["This field is required."] + }) + + def test_accepts_valid_update_from_source(self) -> None: + self.verify_accepts_valid_update(from_source=True) + + def test_accepts_valid_update_from_target(self) -> None: + self.verify_accepts_valid_update(from_source=False) + + def verify_accepts_valid_update(self, from_source: bool) -> None: + if from_source: + source = self.user_1.id + target = self.user_2.id + else: + source = self.user_2.id + target = self.user_1.id + + url = reverse('api:bot:user-alts', args=(source,)) + data = {'target': target, 'context': "Still a trolling account"} + response = self.client.patch(url, data) + self.assertEqual(response.status_code, 204) + + self.relationship_1.refresh_from_db() + self.relationship_2.refresh_from_db() + self.assertEqual(self.relationship_1.context, data['context']) + self.assertEqual(self.relationship_2.context, data['context']) + + def test_retrieving_alts_via_source(self) -> None: + self.verify_retrieving_alts(from_source=True) + + def test_retrieving_alts_via_target(self) -> None: + self.verify_retrieving_alts(from_source=False) + + def verify_retrieving_alts(self, from_source: bool) -> None: + if from_source: + source = self.user_1.id + target = self.user_2.id + else: + source = self.user_2.id + target = self.user_1.id + + url = reverse('api:bot:user-detail', args=(source,)) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + body = response.json() + [alt] = body['alts'] + alt.pop('created_at') + alt.pop('updated_at') + self.assertEqual(alt, + { + 'actor': self.relationship_1.actor.id, + 'source': source, + 'alts': [source], + 'target': target, + 'context': self.relationship_1.context + } + ) + + +class UserAltUpdateWithExistingTransitiveAltsTests(AuthenticatedAPITestCase): + """ + Test user alt methods via transitive alternate account relationships. + + Specifically, user 2 is an alt account of user 1, and user 3 is an alt + account of user 2. However, user 3 should not be an alt account of + user 1 in this case. + """ + + @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, + ) + cls.user_3 = User.objects.create( + id=18294612591, + name=f"Test user {random.randint(100, 1000)}", + discriminator=random.randint(1, 9999), + in_guild=True, + ) + cls.relationship_1 = UserAltRelationship.objects.create( + source=cls.user_1, + target=cls.user_2, + context="Test user's trolling account (rel 1, U1 -> U2)", + actor=cls.user_1 + ) + cls.relationship_2 = UserAltRelationship.objects.create( + source=cls.user_2, + target=cls.user_1, + context="Test user's trolling account (rel 2, U2 -> U1)", + actor=cls.user_1 + ) + cls.relationship_3 = UserAltRelationship.objects.create( + source=cls.user_2, + target=cls.user_3, + context="Test user's trolling account (rel 3, U2 -> U3)", + actor=cls.user_2 + ) + cls.relationship_4 = UserAltRelationship.objects.create( + source=cls.user_3, + target=cls.user_2, + context="Test user's trolling account (rel 4, U3 -> U2)", + actor=cls.user_2 + ) + + def test_retrieving_alts_via_source(self) -> None: + subtests = [ + # Source user, Expected sub-alts of each alt + # U1, ({U2 -> {U1, U3}},) + (self.user_1.id, ({self.user_1.id, self.user_3.id},)), + # U2, ({U1 -> U2}, {U3 -> U2},) + (self.user_2.id, ({self.user_2.id}, {self.user_2.id})), + # U3, ({U2 -> {U1, U3}},) + (self.user_3.id, ({self.user_1.id, self.user_3.id},)), + ] + + for (source, expected_subalts) in subtests: + with self.subTest(source=source): + url = reverse('api:bot:user-detail', args=(source,)) + response = self.client.get(url) + self.assertEqual(response.status_code, 200) + body = response.json() + for alt, subalts in zip(body['alts'], expected_subalts, strict=True): + alt.pop('created_at') + alt.pop('updated_at') + + self.assertEqual(len(set(alt['alts'])), len(subalts)) + self.assertEqual(set(alt['alts']), subalts) + self.assertEqual(alt['source'], source) diff --git a/pydis_site/apps/api/viewsets/bot/user.py b/pydis_site/apps/api/viewsets/bot/user.py index d06eb868..c0b4ca0f 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 @@ -13,8 +15,12 @@ from rest_framework.viewsets import ModelViewSet 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.models.bot.user import User, UserAltRelationship +from pydis_site.apps.api.serializers import ( + UserSerializer, + UserAltRelationshipSerializer, + UserWithAltsSerializer +) class UserListPagination(PageNumberPagination): @@ -93,6 +99,17 @@ class UserViewSet(ModelViewSet): #### Response format >>> { + ... 'alts': [ + ... { + ... 'actor': 1234, + ... # Note that alt relationships are not transitive. + ... 'alts': [409107086526644234, 9012571029], + ... 'context': "Testing account", + ... 'source': 409107086526644234, + ... 'target': 128025 + ... }, + ... # ... + ... ], ... 'id': 409107086526644234, ... 'name': "python", ... 'display_name': "Python", @@ -208,8 +225,9 @@ class UserViewSet(ModelViewSet): - 404: if the user with the given `snowflake` could not be found ### PATCH /bot/users/<snowflake:int> - 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 >>> { @@ -226,9 +244,61 @@ 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/<snowflake:int>/alts + Add the alternative account given in the request body to the alternative + accounts for the given user snowflake. Users will be linked symmetrically. + + #### 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 + - 204: 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 + + ### PATCH /bot/users/<snowflake:int>/alts + Update the context of the given alt account on the given user with the + context specified in the request body. + + #### Request body + >>> { + ... 'context': str, + ... 'target': int + ... } + + #### Status codes + - 204: returned on success + - 400: if the request body was invalid, including if the alternate + account could not be found as an associated account with the parent + user account record + - 404: if the user with the given `snowflake` could not be found + + ### DELETE /bot/users/<snowflake:int>/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 >>> [ @@ -271,10 +341,15 @@ class UserViewSet(ModelViewSet): filterset_fields = ('name', 'discriminator', 'display_name') def get_serializer(self, *args, **kwargs) -> ModelSerializer: - """Set Serializer many attribute to True if request body contains a list.""" + """Customize serializers used based on the requested action.""" if isinstance(kwargs.get('data', {}), list): kwargs['many'] = True + # If we are retrieving a single user, user the expanded serializer + # which also includes context information for each alternate account. + if self.action == 'retrieve' and self.detail: + return UserWithAltsSerializer(*args, **kwargs) + return super().get_serializer(*args, **kwargs) @action(detail=False, methods=["PATCH"], name='user-bulk-patch') @@ -292,6 +367,98 @@ class UserViewSet(ModelViewSet): 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) + + # This code is somewhat vulnerable to a race condition, in case the first validation + # from above passes and directly before validating again, one of the users that + # are referenced here are deleted. Unfortunately, since Django on its own does not + # query in "both directions", we just need to live with a race condition for validation. + # For inserts atomicity is guaranteed as we run it in a transaction. + 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 # pragma: no cover + + 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 + # I would really like to include the alternate account to patch in the URL + # here, but unfortunately it's not possible to do so because the mapping + # does not accept arguments, and if I were to use the same @action decorator, + # it would overwrite the other methods and keep them from working. + @add_alt.mapping.patch + def update_alt(self, request: Request, pk: str) -> Response: + """Update the context of a single alt.""" + for field in ('target', 'context'): + if field not in request.data: + raise ParseError(detail={field: ["This field is required."]}) + + source = self.get_object() + target = request.data['target'] + with transaction.atomic(): + associated_accounts = ( + UserAltRelationship.objects + .filter(Q(source=source, target=target) | Q(source=target, target=source)) + .select_for_update() + ) + count = associated_accounts.count() + # Sanity check + assert count in (0, 2), f"Inconsistent database for alts of {source.id} -> {target}" + + if count == 0: + raise ParseError(detail={'target': ["User is not an associated alt account"]}) + + updated = associated_accounts.update(context=request.data['context']) + assert updated == 2 + + return Response(status=status.HTTP_204_NO_CONTENT) + + @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.""" diff --git a/pyproject.toml b/pyproject.toml index 4ca2f02b..2a8130e0 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,7 +57,7 @@ ignore = [ "DJ001", "DJ008", "RET504", "RUF005", "RUF012", - "S311", + "S101", "S311", "SIM102", "SIM108", ] select = ["ANN", "B", "C4", "D", "DJ", "DTZ", "E", "F", "ISC", "INT", "N", "PGH", "PIE", "RET", "RSE", "RUF", "S", "SIM", "T20", "TID", "UP", "W"] |