diff options
author | 2019-11-15 10:11:45 +0100 | |
---|---|---|
committer | 2019-11-15 10:11:45 +0100 | |
commit | 0e8f623d15f043f1c8712366bdaf83153a619bca (patch) | |
tree | 611c4f111855788c00a5e3ef6c909d9021b29c5c | |
parent | Delete useless migration file (diff) | |
parent | Add pipenv run start command (#308) (diff) |
Merge branch 'master' into bot#549-show-attachments-staff
33 files changed, 2122 insertions, 38 deletions
@@ -43,6 +43,7 @@ unittest-xml-reporting = "~=2.5.1" python_version = "3.7" [scripts] +start = "python manage.py run --debug" makemigrations = "python manage.py makemigrations" django_shell = "python manage.py shell" test = "coverage run manage.py test" diff --git a/pydis_site/__init__.py b/pydis_site/__init__.py index c6146450..df67cf71 100644 --- a/pydis_site/__init__.py +++ b/pydis_site/__init__.py @@ -2,3 +2,8 @@ from wiki.plugins.macros.mdx import toc # Remove the toc header prefix. There's no option for this, so we gotta monkey patch it. toc.HEADER_ID_PREFIX = '' + +# Empty list of validators for Allauth to ponder over. This is referred to in settings.py +# by a string because Allauth won't let us just give it a list _there_, we have to point +# at a list _somewhere else_ instead. +VALIDATORS = [] diff --git a/pydis_site/apps/api/migrations/0047_active_infractions_migration.py b/pydis_site/apps/api/migrations/0047_active_infractions_migration.py new file mode 100644 index 00000000..9ac791dc --- /dev/null +++ b/pydis_site/apps/api/migrations/0047_active_infractions_migration.py @@ -0,0 +1,105 @@ +# Generated by Django 2.2.6 on 2019-10-07 15:59 + +from django.db import migrations +from django.db.models import Count, Prefetch, QuerySet + + +class ExpirationWrapper: + """Wraps an expiration date to properly compare permanent and temporary infractions.""" + + def __init__(self, infraction): + self.expiration_date = infraction.expires_at + + def __lt__(self, other): + """An `expiration_date` is considered smaller when it comes earlier than the `other`.""" + if self.expiration_date is None: + # A permanent infraction can never end sooner than another infraction + return False + elif other.expiration_date is None: + # If `self` is temporary, but `other` is permanent, `self` is smaller + return True + else: + return self.expiration_date < other.expiration_date + + def __eq__(self, other): + """If both expiration dates are permanent they're equal, otherwise compare dates.""" + if self.expiration_date is None and other.expiration_date is None: + return True + elif self.expiration_date is None or other.expiration_date is None: + return False + else: + return self.expiration_date == other.expiration_date + + +def migrate_inactive_types_to_inactive(apps, schema_editor): + """Migrates infractions of non-active types to inactive.""" + infraction_model = apps.get_model('api', 'Infraction') + infraction_model.objects.filter(type__in=('note', 'warning', 'kick')).update(active=False) + + +def get_query(user_model, infraction_model, infr_type: str) -> QuerySet: + """ + Creates QuerySet to fetch users with multiple active infractions of the given `type`. + + The QuerySet will prefetch the infractions and attach them as an `.infractions` attribute to the + `User` instances. + """ + active_infractions = infraction_model.objects.filter(type=infr_type, active=True) + + # Build an SQL query by chaining methods together + + # Get users with active infraction(s) of the provided `infr_type` + query = user_model.objects.filter( + infractions_received__type=infr_type, infractions_received__active=True + ) + + # Prefetch their active received infractions of `infr_type` and attach `.infractions` attribute + query = query.prefetch_related( + Prefetch('infractions_received', queryset=active_infractions, to_attr='infractions') + ) + + # Count and only include them if they have at least 2 active infractions of the `type` + query = query.annotate(num_infractions=Count('infractions_received')) + query = query.filter(num_infractions__gte=2) + + # Make sure we return each individual only once + query = query.distinct() + + return query + + +def migrate_multiple_active_infractions_per_user_to_one(apps, schema_editor): + """ + Make sure a user only has one active infraction of a given "active" infraction type. + + If a user has multiple active infraction, we keep the one with longest expiration date active + and migrate the others to inactive. + """ + infraction_model = apps.get_model('api', 'Infraction') + user_model = apps.get_model('api', 'User') + + for infraction_type in ('ban', 'mute', 'superstar', 'watch'): + query = get_query(user_model, infraction_model, infraction_type) + for user in query: + infractions = sorted(user.infractions, key=ExpirationWrapper, reverse=True) + for infraction in infractions[1:]: + infraction.active = False + infraction.save() + + +def reverse_migration(apps, schema_editor): + """There's no need to do anything special to reverse these migrations.""" + return + + +class Migration(migrations.Migration): + """Data migration to get the database consistent with the new infraction validation rules.""" + + dependencies = [ + ('api', '0046_reminder_jump_url'), + ] + + operations = [ + migrations.RunPython(migrate_inactive_types_to_inactive, reverse_migration), + migrations.RunPython(migrate_multiple_active_infractions_per_user_to_one, reverse_migration) + ] diff --git a/pydis_site/apps/api/migrations/0048_add_infractions_unique_constraints_active.py b/pydis_site/apps/api/migrations/0048_add_infractions_unique_constraints_active.py new file mode 100644 index 00000000..4ea1fb90 --- /dev/null +++ b/pydis_site/apps/api/migrations/0048_add_infractions_unique_constraints_active.py @@ -0,0 +1,17 @@ +# Generated by Django 2.2.6 on 2019-10-07 18:27 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('api', '0047_active_infractions_migration'), + ] + + operations = [ + migrations.AddConstraint( + model_name='infraction', + constraint=models.UniqueConstraint(condition=models.Q(active=True), fields=('user', 'type'), name='unique_active_infraction_per_type_per_user'), + ), + ] diff --git a/pydis_site/apps/api/models/bot/infraction.py b/pydis_site/apps/api/models/bot/infraction.py index dfb32a97..108fd3a2 100644 --- a/pydis_site/apps/api/models/bot/infraction.py +++ b/pydis_site/apps/api/models/bot/infraction.py @@ -71,3 +71,10 @@ class Infraction(ModelReprMixin, models.Model): """Defines the meta options for the infraction model.""" ordering = ['-inserted_at'] + constraints = ( + models.UniqueConstraint( + fields=["user", "type"], + condition=models.Q(active=True), + name="unique_active_infraction_per_type_per_user" + ), + ) diff --git a/pydis_site/apps/api/models/bot/user.py b/pydis_site/apps/api/models/bot/user.py index 21617dc4..5140d2bf 100644 --- a/pydis_site/apps/api/models/bot/user.py +++ b/pydis_site/apps/api/models/bot/user.py @@ -50,7 +50,7 @@ class User(ModelReprMixin, models.Model): def __str__(self): """Returns the name and discriminator for the current user, for display purposes.""" - return f"{self.name}#{self.discriminator}" + return f"{self.name}#{self.discriminator:0>4}" @property def top_role(self) -> Role: diff --git a/pydis_site/apps/api/serializers.py b/pydis_site/apps/api/serializers.py index e091ff4f..2af04e0e 100644 --- a/pydis_site/apps/api/serializers.py +++ b/pydis_site/apps/api/serializers.py @@ -1,6 +1,6 @@ """Converters from Django models to data interchange formats and back.""" - from rest_framework.serializers import ModelSerializer, PrimaryKeyRelatedField, ValidationError +from rest_framework.validators import UniqueTogetherValidator from rest_framework_bulk import BulkSerializerMixin from .models import ( @@ -106,11 +106,22 @@ class InfractionSerializer(ModelSerializer): fields = ( 'id', 'inserted_at', 'expires_at', 'active', 'user', 'actor', 'type', 'reason', 'hidden' ) + validators = [ + UniqueTogetherValidator( + queryset=Infraction.objects.filter(active=True), + fields=['user', 'type'], + message='This user already has an active infraction of this type.', + ) + ] def validate(self, attrs: dict) -> dict: """Validate data constraints for the given data and abort if it is invalid.""" infr_type = attrs.get('type') + active = attrs.get('active') + if active and infr_type in ('note', 'warning', 'kick'): + raise ValidationError({'active': [f'{infr_type} infractions cannot be active.']}) + expires_at = attrs.get('expires_at') if expires_at and infr_type in ('kick', 'warning'): raise ValidationError({'expires_at': [f'{infr_type} infractions cannot expire.']}) diff --git a/pydis_site/apps/api/tests/migrations/__init__.py b/pydis_site/apps/api/tests/migrations/__init__.py new file mode 100644 index 00000000..38e42ffc --- /dev/null +++ b/pydis_site/apps/api/tests/migrations/__init__.py @@ -0,0 +1 @@ +"""This submodule contains tests for functions used in data migrations.""" diff --git a/pydis_site/apps/api/tests/migrations/base.py b/pydis_site/apps/api/tests/migrations/base.py new file mode 100644 index 00000000..0c0a5bd0 --- /dev/null +++ b/pydis_site/apps/api/tests/migrations/base.py @@ -0,0 +1,102 @@ +"""Includes utilities for testing migrations.""" +from django.db import connection +from django.db.migrations.executor import MigrationExecutor +from django.test import TestCase + + +class MigrationsTestCase(TestCase): + """ + A `TestCase` subclass to test migration files. + + To be able to properly test a migration, we will need to inject data into the test database + before the migrations we want to test are applied, but after the older migrations have been + applied. This makes sure that we are testing "as if" we were actually applying this migration + to a database in the state it was in before introducing the new migration. + + To set up a MigrationsTestCase, create a subclass of this class and set the following + class-level attributes: + + - app: The name of the app that contains the migrations (e.g., `'api'`) + - migration_prior: The name* of the last migration file before the migrations you want to test + - migration_target: The name* of the last migration file we want to test + + *) Specify the file names without a path or the `.py` file extension. + + Additionally, overwrite the `setUpMigrationData` in the subclass to inject data into the + database before the migrations we want to test are applied. Please read the docstring of the + method for more information. An optional hook, `setUpPostMigrationData` is also provided. + """ + + # These class-level attributes should be set in classes that inherit from this base class. + app = None + migration_prior = None + migration_target = None + + @classmethod + def setUpTestData(cls): + """ + Injects data into the test database prior to the migration we're trying to test. + + This class methods reverts the test database back to the state of the last migration file + prior to the migrations we want to test. It will then allow the user to inject data into the + test database by calling the `setUpMigrationData` hook. After the data has been injected, it + will apply the migrations we want to test and call the `setUpPostMigrationData` hook. The + user can now test if the migration correctly migrated the injected test data. + """ + if not cls.app: + raise ValueError("The `app` attribute was not set.") + + if not cls.migration_prior or not cls.migration_target: + raise ValueError("Both ` migration_prior` and `migration_target` need to be set.") + + cls.migrate_from = [(cls.app, cls.migration_prior)] + cls.migrate_to = [(cls.app, cls.migration_target)] + + # Reverse to database state prior to the migrations we want to test + executor = MigrationExecutor(connection) + executor.migrate(cls.migrate_from) + + # Call the data injection hook with the current state of the project + old_apps = executor.loader.project_state(cls.migrate_from).apps + cls.setUpMigrationData(old_apps) + + # Run the migrations we want to test + executor = MigrationExecutor(connection) + executor.loader.build_graph() + executor.migrate(cls.migrate_to) + + # Save the project state so we're able to work with the correct model states + cls.apps = executor.loader.project_state(cls.migrate_to).apps + + # Call `setUpPostMigrationData` to potentially set up post migration data used in testing + cls.setUpPostMigrationData(cls.apps) + + @classmethod + def setUpMigrationData(cls, apps): + """ + Override this method to inject data into the test database before the migration is applied. + + This method will be called after setting up the database according to the migrations that + come before the migration(s) we are trying to test, but before the to-be-tested migration(s) + are applied. This allows us to simulate a database state just prior to the migrations we are + trying to test. + + To make sure we're creating objects according to the state the models were in at this point + in the migration history, use `apps.get_model(app_name: str, model_name: str)` to get the + appropriate model, e.g.: + + >>> Infraction = apps.get_model('api', 'Infraction') + """ + pass + + @classmethod + def setUpPostMigrationData(cls, apps): + """ + Set up additional test data after the target migration has been applied. + + Use `apps.get_model(app_name: str, model_name: str)` to get the correct instances of the + model classes: + + >>> Infraction = apps.get_model('api', 'Infraction') + """ + pass diff --git a/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py new file mode 100644 index 00000000..8dc29b34 --- /dev/null +++ b/pydis_site/apps/api/tests/migrations/test_active_infraction_migration.py @@ -0,0 +1,496 @@ +"""Tests for the data migration in `filename`.""" +import logging +from collections import ChainMap, namedtuple +from datetime import timedelta +from itertools import count +from typing import Dict, Iterable, Type, Union + +from django.db.models import Q +from django.forms.models import model_to_dict +from django.utils import timezone + +from pydis_site.apps.api.models import Infraction, User +from .base import MigrationsTestCase + +log = logging.getLogger(__name__) +log.setLevel(logging.DEBUG) + + +InfractionHistory = namedtuple('InfractionHistory', ("user_id", "infraction_history")) + + +class InfractionFactory: + """Factory that creates infractions for a User instance.""" + + infraction_id = count(1) + user_id = count(1) + default_values = { + 'active': True, + 'expires_at': None, + 'hidden': False, + } + + @classmethod + def create( + cls, + actor: User, + infractions: Iterable[Dict[str, Union[str, int, bool]]], + infraction_model: Type[Infraction] = Infraction, + user_model: Type[User] = User, + ) -> InfractionHistory: + """ + Creates `infractions` for the `user` with the given `actor`. + + The `infractions` dictionary can contain the following fields: + - `type` (required) + - `active` (default: True) + - `expires_at` (default: None; i.e, permanent) + - `hidden` (default: False). + + The parameters `infraction_model` and `user_model` can be used to pass in an instance of + both model classes from a different migration/project state. + """ + user_id = next(cls.user_id) + user = user_model.objects.create( + id=user_id, + name=f"Infracted user {user_id}", + discriminator=user_id, + avatar_hash=None, + ) + infraction_history = [] + + for infraction in infractions: + infraction = dict(infraction) + infraction["id"] = next(cls.infraction_id) + infraction = ChainMap(infraction, cls.default_values) + new_infraction = infraction_model.objects.create( + user=user, + actor=actor, + type=infraction["type"], + reason=f"`{infraction['type']}` infraction (ID: {infraction['id']} of {user}", + active=infraction['active'], + hidden=infraction['hidden'], + expires_at=infraction['expires_at'], + ) + infraction_history.append(new_infraction) + + return InfractionHistory(user_id=user_id, infraction_history=infraction_history) + + +class InfractionFactoryTests(MigrationsTestCase): + """Tests for the InfractionFactory.""" + + app = "api" + migration_prior = "0046_reminder_jump_url" + migration_target = "0046_reminder_jump_url" + + @classmethod + def setUpPostMigrationData(cls, apps): + """Create a default actor for all infractions.""" + cls.infraction_model = apps.get_model('api', 'Infraction') + cls.user_model = apps.get_model('api', 'User') + + cls.actor = cls.user_model.objects.create( + id=9999, + name="Unknown Moderator", + discriminator=1040, + avatar_hash=None, + ) + + def test_infraction_factory_total_count(self): + """Does the test database hold as many infractions as we tried to create?""" + InfractionFactory.create( + actor=self.actor, + infractions=( + {'type': 'kick', 'active': False, 'hidden': False}, + {'type': 'ban', 'active': True, 'hidden': False}, + {'type': 'note', 'active': False, 'hidden': True}, + ), + infraction_model=self.infraction_model, + user_model=self.user_model, + ) + database_count = Infraction.objects.all().count() + self.assertEqual(3, database_count) + + def test_infraction_factory_multiple_users(self): + """Does the test database hold as many infractions as we tried to create?""" + for _user in range(5): + InfractionFactory.create( + actor=self.actor, + infractions=( + {'type': 'kick', 'active': False, 'hidden': True}, + {'type': 'ban', 'active': True, 'hidden': False}, + ), + infraction_model=self.infraction_model, + user_model=self.user_model, + ) + + # Check if infractions and users are recorded properly in the database + database_count = Infraction.objects.all().count() + self.assertEqual(database_count, 10) + + user_count = User.objects.all().count() + self.assertEqual(user_count, 5 + 1) + + def test_infraction_factory_sets_correct_fields(self): + """Does the InfractionFactory set the correct attributes?""" + infractions = ( + { + 'type': 'note', + 'active': False, + 'hidden': True, + 'expires_at': timezone.now() + }, + {'type': 'warning', 'active': False, 'hidden': False, 'expires_at': None}, + {'type': 'watch', 'active': False, 'hidden': True, 'expires_at': None}, + {'type': 'mute', 'active': True, 'hidden': False, 'expires_at': None}, + {'type': 'kick', 'active': True, 'hidden': True, 'expires_at': None}, + {'type': 'ban', 'active': True, 'hidden': False, 'expires_at': None}, + { + 'type': 'superstar', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + }, + ) + + InfractionFactory.create( + actor=self.actor, + infractions=infractions, + infraction_model=self.infraction_model, + user_model=self.user_model, + ) + + for infraction in infractions: + with self.subTest(**infraction): + self.assertTrue(Infraction.objects.filter(**infraction).exists()) + + +class ActiveInfractionMigrationTests(MigrationsTestCase): + """ + Tests the active infraction data migration. + + The active infraction data migration should do the following things: + + 1. migrates all active notes, warnings, and kicks to an inactive status; + 2. migrates all users with multiple active infractions of a single type to have only one active + infraction of that type. The infraction with the longest duration stays active. + """ + + app = "api" + migration_prior = "0046_reminder_jump_url" + migration_target = "0047_active_infractions_migration" + + @classmethod + def setUpMigrationData(cls, apps): + """Sets up an initial database state that contains the relevant test cases.""" + # Fetch the Infraction and User model in the current migration state + cls.infraction_model = apps.get_model('api', 'Infraction') + cls.user_model = apps.get_model('api', 'User') + + cls.created_infractions = {} + + # Moderator that serves as actor for all infractions + cls.user_moderator = cls.user_model.objects.create( + id=9999, + name="Olivier de Vienne", + discriminator=1040, + avatar_hash=None, + ) + + # User #1: clean user with no infractions + cls.created_infractions["no infractions"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=[], + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #2: One inactive note infraction + cls.created_infractions["one inactive note"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'note', 'active': False, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #3: One active note infraction + cls.created_infractions["one active note"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'note', 'active': True, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #4: One active and one inactive note infraction + cls.created_infractions["one active and one inactive note"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'note', 'active': False, 'hidden': True}, + {'type': 'note', 'active': True, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #5: Once active note, one active kick, once active warning + cls.created_infractions["active note, kick, warning"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'note', 'active': True, 'hidden': True}, + {'type': 'kick', 'active': True, 'hidden': True}, + {'type': 'warning', 'active': True, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #6: One inactive ban and one active ban + cls.created_infractions["one inactive and one active ban"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'ban', 'active': False, 'hidden': True}, + {'type': 'ban', 'active': True, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #7: Two active permanent bans + cls.created_infractions["two active perm bans"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'ban', 'active': True, 'hidden': True}, + {'type': 'ban', 'active': True, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #8: Multiple active temporary bans + cls.created_infractions["multiple active temp bans"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=1) + }, + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=10) + }, + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=20) + }, + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=5) + }, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #9: One active permanent ban, two active temporary bans + cls.created_infractions["active perm, two active temp bans"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=10) + }, + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': None, + }, + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=7) + }, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #10: One inactive permanent ban, two active temporary bans + cls.created_infractions["one inactive perm ban, two active temp bans"] = ( + InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=10) + }, + { + 'type': 'ban', + 'active': False, + 'hidden': True, + 'expires_at': None, + }, + { + 'type': 'ban', + 'active': True, + 'hidden': True, + 'expires_at': timezone.now() + timedelta(days=7) + }, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + ) + + # User #11: Active ban, active mute, active superstar + cls.created_infractions["active ban, mute, and superstar"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'ban', 'active': True, 'hidden': True}, + {'type': 'mute', 'active': True, 'hidden': True}, + {'type': 'superstar', 'active': True, 'hidden': True}, + {'type': 'watch', 'active': True, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + # User #12: Multiple active bans, active mutes, active superstars + cls.created_infractions["multiple active bans, mutes, stars"] = InfractionFactory.create( + actor=cls.user_moderator, + infractions=( + {'type': 'ban', 'active': True, 'hidden': True}, + {'type': 'ban', 'active': True, 'hidden': True}, + {'type': 'ban', 'active': True, 'hidden': True}, + {'type': 'mute', 'active': True, 'hidden': True}, + {'type': 'mute', 'active': True, 'hidden': True}, + {'type': 'mute', 'active': True, 'hidden': True}, + {'type': 'superstar', 'active': True, 'hidden': True}, + {'type': 'superstar', 'active': True, 'hidden': True}, + {'type': 'superstar', 'active': True, 'hidden': True}, + {'type': 'watch', 'active': True, 'hidden': True}, + {'type': 'watch', 'active': True, 'hidden': True}, + {'type': 'watch', 'active': True, 'hidden': True}, + ), + infraction_model=cls.infraction_model, + user_model=cls.user_model, + ) + + def test_all_never_active_types_became_inactive(self): + """Are all infractions of a non-active type inactive after the migration?""" + inactive_type_query = Q(type="note") | Q(type="warning") | Q(type="kick") + self.assertFalse( + self.infraction_model.objects.filter(inactive_type_query, active=True).exists() + ) + + def test_migration_left_clean_user_without_infractions(self): + """Do users without infractions have no infractions after the migration?""" + user_id, infraction_history = self.created_infractions["no infractions"] + self.assertFalse( + self.infraction_model.objects.filter(user__id=user_id).exists() + ) + + def test_migration_left_user_with_inactive_note_untouched(self): + """Did the migration leave users with only an inactive note untouched?""" + user_id, infraction_history = self.created_infractions["one inactive note"] + inactive_note = infraction_history[0] + self.assertTrue( + self.infraction_model.objects.filter(**model_to_dict(inactive_note)).exists() + ) + + def test_migration_only_touched_active_field_of_active_note(self): + """Does the migration only change the `active` field?""" + user_id, infraction_history = self.created_infractions["one active note"] + note = model_to_dict(infraction_history[0]) + note['active'] = False + self.assertTrue( + self.infraction_model.objects.filter(**note).exists() + ) + + def test_migration_only_touched_active_field_of_active_note_left_inactive_untouched(self): + """Does the migration only change the `active` field of active notes?""" + user_id, infraction_history = self.created_infractions["one active and one inactive note"] + for note in infraction_history: + with self.subTest(active=note.active): + note = model_to_dict(note) + note['active'] = False + self.assertTrue( + self.infraction_model.objects.filter(**note).exists() + ) + + def test_migration_migrates_all_nonactive_types_to_inactive(self): + """Do we set the `active` field of all non-active infractions to `False`?""" + user_id, infraction_history = self.created_infractions["active note, kick, warning"] + self.assertFalse( + self.infraction_model.objects.filter(user__id=user_id, active=True).exists() + ) + + def test_migration_leaves_user_with_one_active_ban_untouched(self): + """Do we leave a user with one active and one inactive ban untouched?""" + user_id, infraction_history = self.created_infractions["one inactive and one active ban"] + for infraction in infraction_history: + with self.subTest(active=infraction.active): + self.assertTrue( + self.infraction_model.objects.filter(**model_to_dict(infraction)).exists() + ) + + def test_migration_turns_double_active_perm_ban_into_single_active_perm_ban(self): + """Does the migration turn two active permanent bans into one active permanent ban?""" + user_id, infraction_history = self.created_infractions["two active perm bans"] + active_count = self.infraction_model.objects.filter(user__id=user_id, active=True).count() + self.assertEqual(active_count, 1) + + def test_migration_leaves_temporary_ban_with_longest_duration_active(self): + """Does the migration turn two active permanent bans into one active permanent ban?""" + user_id, infraction_history = self.created_infractions["multiple active temp bans"] + active_ban = self.infraction_model.objects.get(user__id=user_id, active=True) + self.assertEqual(active_ban.expires_at, infraction_history[2].expires_at) + + def test_migration_leaves_permanent_ban_active(self): + """Does the migration leave the permanent ban active?""" + user_id, infraction_history = self.created_infractions["active perm, two active temp bans"] + active_ban = self.infraction_model.objects.get(user__id=user_id, active=True) + self.assertIsNone(active_ban.expires_at) + + def test_migration_leaves_longest_temp_ban_active_with_inactive_permanent_ban(self): + """Does the longest temp ban stay active, even with an inactive perm ban present?""" + user_id, infraction_history = self.created_infractions[ + "one inactive perm ban, two active temp bans" + ] + active_ban = self.infraction_model.objects.get(user__id=user_id, active=True) + self.assertEqual(active_ban.expires_at, infraction_history[0].expires_at) + + def test_migration_leaves_all_active_types_active_if_one_of_each_exists(self): + """Do all active infractions stay active if only one of each is present?""" + user_id, infraction_history = self.created_infractions["active ban, mute, and superstar"] + active_count = self.infraction_model.objects.filter(user__id=user_id, active=True).count() + self.assertEqual(active_count, 4) + + def test_migration_reduces_all_active_types_to_a_single_active_infraction(self): + """Do we reduce all of the infraction types to one active infraction?""" + user_id, infraction_history = self.created_infractions["multiple active bans, mutes, stars"] + active_infractions = self.infraction_model.objects.filter(user__id=user_id, active=True) + self.assertEqual(len(active_infractions), 4) + types_observed = [infraction.type for infraction in active_infractions] + + for infraction_type in ('ban', 'mute', 'superstar', 'watch'): + with self.subTest(type=infraction_type): + self.assertIn(infraction_type, types_observed) diff --git a/pydis_site/apps/api/tests/migrations/test_base.py b/pydis_site/apps/api/tests/migrations/test_base.py new file mode 100644 index 00000000..f69bc92c --- /dev/null +++ b/pydis_site/apps/api/tests/migrations/test_base.py @@ -0,0 +1,135 @@ +import logging +from unittest.mock import call, patch + +from django.db.migrations.loader import MigrationLoader +from django.test import TestCase + +from .base import MigrationsTestCase, connection + +log = logging.getLogger(__name__) + + +class SpanishInquisition(MigrationsTestCase): + app = "api" + migration_prior = "scragly" + migration_target = "kosa" + + +@patch("pydis_site.apps.api.tests.migrations.base.MigrationExecutor") +class MigrationsTestCaseNoSideEffectsTests(TestCase): + """Tests the MigrationTestCase class with actual migration side effects disabled.""" + + def setUp(self): + """Set up an instance of MigrationsTestCase for use in tests.""" + self.test_case = SpanishInquisition() + + def test_missing_app_class_raises_value_error(self, _migration_executor): + """A MigrationsTestCase subclass should set the class-attribute `app`.""" + class Spam(MigrationsTestCase): + pass + + spam = Spam() + with self.assertRaises(ValueError, msg="The `app` attribute was not set."): + spam.setUpTestData() + + def test_missing_migration_class_attributes_raise_value_error(self, _migration_executor): + """A MigrationsTestCase subclass should set both `migration_prior` and `migration_target`""" + class Eggs(MigrationsTestCase): + app = "api" + migration_target = "lemon" + + class Bacon(MigrationsTestCase): + app = "api" + migration_prior = "mark" + + instances = (Eggs(), Bacon()) + + exception_message = "Both ` migration_prior` and `migration_target` need to be set." + for instance in instances: + with self.subTest( + migration_prior=instance.migration_prior, + migration_target=instance.migration_target, + ): + with self.assertRaises(ValueError, msg=exception_message): + instance.setUpTestData() + + @patch(f"{__name__}.SpanishInquisition.setUpMigrationData") + @patch(f"{__name__}.SpanishInquisition.setUpPostMigrationData") + def test_migration_data_hooks_are_called_once(self, pre_hook, post_hook, _migration_executor): + """The `setUpMigrationData` and `setUpPostMigrationData` hooks should be called once.""" + self.test_case.setUpTestData() + for hook in (pre_hook, post_hook): + with self.subTest(hook=repr(hook)): + hook.assert_called_once() + + def test_migration_executor_is_instantiated_twice(self, migration_executor): + """The `MigrationExecutor` should be instantiated with the database connection twice.""" + self.test_case.setUpTestData() + + expected_args = [call(connection), call(connection)] + self.assertEqual(migration_executor.call_args_list, expected_args) + + def test_project_state_is_loaded_for_correct_migration_files_twice(self, migration_executor): + """The `project_state` should first be loaded with `migrate_from`, then `migrate_to`.""" + self.test_case.setUpTestData() + + expected_args = [call(self.test_case.migrate_from), call(self.test_case.migrate_to)] + self.assertEqual(migration_executor().loader.project_state.call_args_list, expected_args) + + def test_loader_build_graph_gets_called_once(self, migration_executor): + """We should rebuild the migration graph before applying the second set of migrations.""" + self.test_case.setUpTestData() + + migration_executor().loader.build_graph.assert_called_once() + + def test_migration_executor_migrate_method_is_called_correctly_twice(self, migration_executor): + """The migrate method of the executor should be called twice with the correct arguments.""" + self.test_case.setUpTestData() + + self.assertEqual(migration_executor().migrate.call_count, 2) + calls = [call([('api', 'scragly')]), call([('api', 'kosa')])] + migration_executor().migrate.assert_has_calls(calls) + + +class LifeOfBrian(MigrationsTestCase): + app = "api" + migration_prior = "0046_reminder_jump_url" + migration_target = "0048_add_infractions_unique_constraints_active" + + @classmethod + def log_last_migration(cls): + """Parses the applied migrations dictionary to log the last applied migration.""" + loader = MigrationLoader(connection) + api_migrations = [ + migration for app, migration in loader.applied_migrations if app == cls.app + ] + last_migration = max(api_migrations, key=lambda name: int(name[:4])) + log.info(f"The last applied migration: {last_migration}") + + @classmethod + def setUpMigrationData(cls, apps): + """Method that logs the last applied migration at this point.""" + cls.log_last_migration() + + @classmethod + def setUpPostMigrationData(cls, apps): + """Method that logs the last applied migration at this point.""" + cls.log_last_migration() + + +class MigrationsTestCaseMigrationTest(TestCase): + """Tests if `MigrationsTestCase` travels to the right points in the migration history.""" + + def test_migrations_test_case_travels_to_correct_migrations_in_history(self): + """The test case should first revert to `migration_prior`, then go to `migration_target`.""" + brian = LifeOfBrian() + + with self.assertLogs(log, level=logging.INFO) as logs: + brian.setUpTestData() + + self.assertEqual(len(logs.records), 2) + + for time_point, record in zip(("migration_prior", "migration_target"), logs.records): + with self.subTest(time_point=time_point): + message = f"The last applied migration: {getattr(brian, time_point)}" + self.assertEqual(record.getMessage(), message) diff --git a/pydis_site/apps/api/tests/test_infractions.py b/pydis_site/apps/api/tests/test_infractions.py index c58c32e2..7a54640e 100644 --- a/pydis_site/apps/api/tests/test_infractions.py +++ b/pydis_site/apps/api/tests/test_infractions.py @@ -1,6 +1,8 @@ from datetime import datetime as dt, timedelta, timezone +from unittest.mock import patch from urllib.parse import quote +from django.db.utils import IntegrityError from django_hosts.resolvers import reverse from .base import APISubdomainTestCase @@ -167,6 +169,12 @@ class CreationTests(APISubdomainTestCase): discriminator=1, avatar_hash=None ) + cls.second_user = User.objects.create( + id=6, + name='carl', + discriminator=2, + avatar_hash=None + ) def test_accepts_valid_data(self): url = reverse('bot:infraction-list', host='api') @@ -305,6 +313,187 @@ class CreationTests(APISubdomainTestCase): 'hidden': [f'{data["type"]} infractions must be hidden.'] }) + def test_returns_400_for_active_infraction_of_type_that_cannot_be_active(self): + """Test if the API rejects active infractions for types that cannot be active.""" + url = reverse('bot:infraction-list', host='api') + restricted_types = ( + ('note', True), + ('warning', False), + ('kick', False), + ) + + for infraction_type, hidden in restricted_types: + with self.subTest(infraction_type=infraction_type): + invalid_infraction = { + 'user': self.user.id, + 'actor': self.user.id, + 'type': infraction_type, + 'reason': 'Take me on!', + 'hidden': hidden, + 'active': True, + 'expires_at': None, + } + response = self.client.post(url, data=invalid_infraction) + self.assertEqual(response.status_code, 400) + self.assertEqual( + response.json(), + {'active': [f'{infraction_type} infractions cannot be active.']} + ) + + def test_returns_400_for_second_active_infraction_of_the_same_type(self): + """Test if the API rejects a second active infraction of the same type for a given user.""" + url = reverse('bot:infraction-list', host='api') + active_infraction_types = ('mute', 'ban', 'superstar') + + for infraction_type in active_infraction_types: + with self.subTest(infraction_type=infraction_type): + first_active_infraction = { + 'user': self.user.id, + 'actor': self.user.id, + 'type': infraction_type, + 'reason': 'Take me on!', + 'active': True, + 'expires_at': '2019-10-04T12:52:00+00:00' + } + + # Post the first active infraction of a type and confirm it's accepted. + first_response = self.client.post(url, data=first_active_infraction) + self.assertEqual(first_response.status_code, 201) + + second_active_infraction = { + 'user': self.user.id, + 'actor': self.user.id, + 'type': infraction_type, + 'reason': 'Take on me!', + 'active': True, + 'expires_at': '2019-10-04T12:52:00+00:00' + } + second_response = self.client.post(url, data=second_active_infraction) + self.assertEqual(second_response.status_code, 400) + self.assertEqual( + second_response.json(), + { + 'non_field_errors': [ + 'This user already has an active infraction of this type.' + ] + } + ) + + def test_returns_201_for_second_active_infraction_of_different_type(self): + """Test if the API accepts a second active infraction of a different type than the first.""" + url = reverse('bot:infraction-list', host='api') + first_active_infraction = { + 'user': self.user.id, + 'actor': self.user.id, + 'type': 'mute', + 'reason': 'Be silent!', + 'hidden': True, + 'active': True, + 'expires_at': '2019-10-04T12:52:00+00:00' + } + second_active_infraction = { + 'user': self.user.id, + 'actor': self.user.id, + 'type': 'ban', + 'reason': 'Be gone!', + 'hidden': True, + 'active': True, + 'expires_at': '2019-10-05T12:52:00+00:00' + } + # Post the first active infraction of a type and confirm it's accepted. + first_response = self.client.post(url, data=first_active_infraction) + self.assertEqual(first_response.status_code, 201) + + # Post the first active infraction of a type and confirm it's accepted. + second_response = self.client.post(url, data=second_active_infraction) + self.assertEqual(second_response.status_code, 201) + + def test_unique_constraint_raises_integrity_error_on_second_active_of_same_type(self): + """Do we raise `IntegrityError` for the second active infraction of a type for a user?""" + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The first active ban" + ) + with self.assertRaises(IntegrityError): + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The second active ban" + ) + + def test_unique_constraint_accepts_active_infraction_after_inactive_infraction(self): + """Do we accept an active infraction if the others of the same type are inactive?""" + try: + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=False, + reason="The first inactive ban" + ) + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=False, + reason="The second inactive ban" + ) + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The first active ban" + ) + except IntegrityError: + self.fail("An unexpected IntegrityError was raised.") + + @patch(f"{__name__}.Infraction") + def test_if_accepts_active_infraction_test_catches_integrity_error(self, infraction_patch): + """Does the test properly catch the IntegrityError and raise an AssertionError?""" + infraction_patch.objects.create.side_effect = IntegrityError + with self.assertRaises(AssertionError, msg="An unexpected IntegrityError was raised."): + self.test_unique_constraint_accepts_active_infraction_after_inactive_infraction() + + def test_unique_constraint_accepts_second_active_of_different_type(self): + """Do we accept a second active infraction of a different type for a given user?""" + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="The first active ban" + ) + Infraction.objects.create( + user=self.user, + actor=self.user, + type="mute", + active=True, + reason="The first active mute" + ) + + def test_unique_constraint_accepts_active_infractions_for_different_users(self): + """Do we accept two active infractions of the same type for two different users?""" + Infraction.objects.create( + user=self.user, + actor=self.user, + type="ban", + active=True, + reason="An active ban for the first user" + ) + Infraction.objects.create( + user=self.second_user, + actor=self.second_user, + type="ban", + active=False, + reason="An active ban for the second user" + ) + class ExpandedTests(APISubdomainTestCase): @classmethod @@ -318,12 +507,14 @@ class ExpandedTests(APISubdomainTestCase): cls.kick = Infraction.objects.create( user_id=cls.user.id, actor_id=cls.user.id, - type='kick' + type='kick', + active=False ) cls.warning = Infraction.objects.create( user_id=cls.user.id, actor_id=cls.user.id, - type='warning' + type='warning', + active=False, ) def check_expanded_fields(self, infraction): diff --git a/pydis_site/apps/home/forms/__init__.py b/pydis_site/apps/home/forms/__init__.py new file mode 100644 index 00000000..e69de29b --- /dev/null +++ b/pydis_site/apps/home/forms/__init__.py diff --git a/pydis_site/apps/home/forms/account_deletion.py b/pydis_site/apps/home/forms/account_deletion.py new file mode 100644 index 00000000..17ffe5c1 --- /dev/null +++ b/pydis_site/apps/home/forms/account_deletion.py @@ -0,0 +1,24 @@ +from crispy_forms.helper import FormHelper +from crispy_forms.layout import Layout +from django.forms import CharField, Form +from django_crispy_bulma.layout import IconField, Submit + + +class AccountDeletionForm(Form): + """Account deletion form, to collect username for confirmation of removal.""" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.helper = FormHelper() + + self.helper.form_method = "post" + self.helper.add_input(Submit("submit", "I understand, delete my account")) + + self.helper.layout = Layout( + IconField("username", icon_prepend="user") + ) + + username = CharField( + label="Username", + required=True + ) diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py index 9f286882..4cb4564b 100644 --- a/pydis_site/apps/home/signals.py +++ b/pydis_site/apps/home/signals.py @@ -1,3 +1,4 @@ +from contextlib import suppress from typing import List, Optional, Type from allauth.account.signals import user_logged_in @@ -8,7 +9,7 @@ from allauth.socialaccount.signals import ( pre_social_login, social_account_added, social_account_removed, social_account_updated) from django.contrib.auth.models import Group, User as DjangoUser -from django.db.models.signals import post_save, pre_delete, pre_save +from django.db.models.signals import post_delete, post_save, pre_save from pydis_site.apps.api.models import User as DiscordUser from pydis_site.apps.staff.models import RoleMapping @@ -37,7 +38,7 @@ class AllauthSignalListener: def __init__(self): post_save.connect(self.user_model_updated, sender=DiscordUser) - pre_delete.connect(self.mapping_model_deleted, sender=RoleMapping) + post_delete.connect(self.mapping_model_deleted, sender=RoleMapping) pre_save.connect(self.mapping_model_updated, sender=RoleMapping) pre_social_login.connect(self.social_account_updated) @@ -133,13 +134,29 @@ class AllauthSignalListener: Processes deletion signals from the RoleMapping model, removing perms from users. We need to do this to ensure that users aren't left with permissions groups that - they shouldn't have assigned to them when a RoleMapping is deleted from the database. + they shouldn't have assigned to them when a RoleMapping is deleted from the database, + and to remove their staff status if they should no longer have it. """ instance: RoleMapping = kwargs["instance"] for user in instance.group.user_set.all(): + # Firstly, remove their related user group user.groups.remove(instance.group) + with suppress(SocialAccount.DoesNotExist, DiscordUser.DoesNotExist): + # If we get either exception, then the user could not have been assigned staff + # with our system in the first place. + + social_account = SocialAccount.objects.get(user=user, provider=DiscordProvider.id) + discord_user = DiscordUser.objects.get(id=int(social_account.uid)) + + mappings = RoleMapping.objects.filter(role__in=discord_user.roles.all()).all() + is_staff = any(m.is_staff for m in mappings) + + if user.is_staff != is_staff: + user.is_staff = is_staff + user.save(update_fields=("is_staff", )) + def mapping_model_updated(self, sender: Type[RoleMapping], **kwargs) -> None: """ Processes update signals from the RoleMapping model. @@ -174,6 +191,21 @@ class AllauthSignalListener: for account in accounts: account.user.groups.add(instance.group) + if instance.is_staff and not account.user.is_staff: + account.user.is_staff = instance.is_staff + account.user.save(update_fields=("is_staff", )) + else: + discord_user = DiscordUser.objects.get(id=int(account.uid)) + + mappings = RoleMapping.objects.filter( + role__in=discord_user.roles.all() + ).exclude(id=instance.id).all() + is_staff = any(m.is_staff for m in mappings) + + if account.user.is_staff != is_staff: + account.user.is_staff = is_staff + account.user.save(update_fields=("is_staff",)) + def user_model_updated(self, sender: Type[DiscordUser], **kwargs) -> None: """ Processes update signals from the Discord User model, assigning perms as required. @@ -230,31 +262,53 @@ class AllauthSignalListener: except SocialAccount.user.RelatedObjectDoesNotExist: return # There's no user account yet, this will be handled by another receiver + # Ensure that the username on this account is correct + new_username = f"{user.name}#{user.discriminator}" + + if account.user.username != new_username: + account.user.username = new_username + account.user.first_name = new_username + if not user.in_guild: deletion = True if deletion: # They've unlinked Discord or left the server, so we have to remove their groups + # and their staff status - if not current_groups: - return # They have no groups anyway, no point in processing + if current_groups: + # They do have groups, so let's remove them + account.user.groups.remove( + *(mapping.group for mapping in mappings) + ) - account.user.groups.remove( - *(mapping.group for mapping in mappings) - ) + if account.user.is_staff: + # They're marked as a staff user and they shouldn't be, so let's fix that + account.user.is_staff = False else: new_groups = [] + is_staff = False for role in user.roles.all(): try: - new_groups.append(mappings.get(role=role).group) + mapping = mappings.get(role=role) except RoleMapping.DoesNotExist: continue # No mapping exists - account.user.groups.add( - *[group for group in new_groups if group not in current_groups] - ) + new_groups.append(mapping.group) - account.user.groups.remove( - *[mapping.group for mapping in mappings if mapping.group not in new_groups] - ) + if mapping.is_staff: + is_staff = True + + account.user.groups.add( + *[group for group in new_groups if group not in current_groups] + ) + + account.user.groups.remove( + *[mapping.group for mapping in mappings if mapping.group not in new_groups] + ) + + if account.user.is_staff != is_staff: + account.user.is_staff = is_staff + + account.user.save() diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py index 27fc7710..66a67252 100644 --- a/pydis_site/apps/home/tests/test_signal_listener.py +++ b/pydis_site/apps/home/tests/test_signal_listener.py @@ -67,12 +67,14 @@ class SignalListenerTests(TestCase): cls.admin_mapping = RoleMapping.objects.create( role=cls.admin_role, - group=cls.admin_group + group=cls.admin_group, + is_staff=True ) cls.moderator_mapping = RoleMapping.objects.create( role=cls.moderator_role, - group=cls.moderator_group + group=cls.moderator_group, + is_staff=False ) cls.discord_user = DiscordUser.objects.create( @@ -166,7 +168,7 @@ class SignalListenerTests(TestCase): cls.django_moderator = DjangoUser.objects.create( username="moderator", - is_staff=True, + is_staff=False, is_superuser=False ) @@ -339,6 +341,33 @@ class SignalListenerTests(TestCase): self.discord_admin.roles.add(self.admin_role) self.discord_admin.save() + def test_apply_groups_moderator(self): + """Test application of groups by role, relating to a non-`is_staff` moderator user.""" + handler = AllauthSignalListener() + + self.assertEqual(self.django_user_discordless.groups.all().count(), 0) + + # Apply groups based on moderator role being present on Discord + handler._apply_groups(self.discord_moderator, self.social_moderator) + self.assertTrue(self.moderator_group in self.django_moderator.groups.all()) + + # Remove groups based on the user apparently leaving the server + handler._apply_groups(self.discord_moderator, self.social_moderator, True) + self.assertEqual(self.django_user_discordless.groups.all().count(), 0) + + # Apply the moderator role again + handler._apply_groups(self.discord_moderator, self.social_moderator) + + # Remove all of the roles from the user + self.discord_moderator.roles.clear() + + # Remove groups based on the user no longer having the moderator role on Discord + handler._apply_groups(self.discord_moderator, self.social_moderator) + self.assertEqual(self.django_user_discordless.groups.all().count(), 0) + + self.discord_moderator.roles.add(self.moderator_role) + self.discord_moderator.save() + def test_apply_groups_other(self): """Test application of groups by role, relating to non-standard cases.""" handler = AllauthSignalListener() @@ -373,10 +402,25 @@ class SignalListenerTests(TestCase): self.assertEqual(self.django_moderator.groups.all().count(), 1) self.assertEqual(self.django_admin.groups.all().count(), 1) + # Test is_staff changes + self.admin_mapping.is_staff = False + self.admin_mapping.save() + + self.assertFalse(self.django_moderator.is_staff) + self.assertFalse(self.django_admin.is_staff) + + self.admin_mapping.is_staff = True + self.admin_mapping.save() + + self.django_admin.refresh_from_db(fields=("is_staff", )) + self.assertTrue(self.django_admin.is_staff) + # Test mapping deletion self.admin_mapping.delete() + self.django_admin.refresh_from_db(fields=("is_staff",)) self.assertEqual(self.django_admin.groups.all().count(), 0) + self.assertFalse(self.django_admin.is_staff) # Test mapping update self.moderator_mapping.group = self.admin_group @@ -388,12 +432,30 @@ class SignalListenerTests(TestCase): # Test mapping creation new_mapping = RoleMapping.objects.create( role=self.admin_role, - group=self.moderator_group + group=self.moderator_group, + is_staff=True + ) + + self.assertEqual(self.django_admin.groups.all().count(), 1) + self.assertTrue(self.moderator_group in self.django_admin.groups.all()) + + self.django_admin.refresh_from_db(fields=("is_staff",)) + self.assertTrue(self.django_admin.is_staff) + + new_mapping.delete() + + # Test mapping creation (without is_staff) + new_mapping = RoleMapping.objects.create( + role=self.admin_role, + group=self.moderator_group, ) self.assertEqual(self.django_admin.groups.all().count(), 1) self.assertTrue(self.moderator_group in self.django_admin.groups.all()) + self.django_admin.refresh_from_db(fields=("is_staff",)) + self.assertFalse(self.django_admin.is_staff) + # Test that nothing happens when fixtures are loaded pre_save.send(RoleMapping, instance=new_mapping, raw=True) diff --git a/pydis_site/apps/home/tests/test_views.py b/pydis_site/apps/home/tests/test_views.py index 7aeaddd2..572317a7 100644 --- a/pydis_site/apps/home/tests/test_views.py +++ b/pydis_site/apps/home/tests/test_views.py @@ -1,5 +1,198 @@ +from allauth.socialaccount.models import SocialAccount +from django.contrib.auth.models import User +from django.http import HttpResponseRedirect from django.test import TestCase -from django_hosts.resolvers import reverse +from django_hosts.resolvers import get_host, reverse, reverse_host + + +def check_redirect_url( + response: HttpResponseRedirect, reversed_url: str, strip_params=True +) -> bool: + """ + Check whether a given redirect response matches a specific reversed URL. + + Arguments: + * `response`: The HttpResponseRedirect returned by the test client + * `reversed_url`: The URL returned by `reverse()` + * `strip_params`: Whether to strip URL parameters (following a "?") from the URL given in the + `response` object + """ + host = get_host(None) + hostname = reverse_host(host) + + redirect_url = response.url + + if strip_params and "?" in redirect_url: + redirect_url = redirect_url.split("?", 1)[0] + + result = reversed_url == f"//{hostname}{redirect_url}" + return result + + +class TestAccountDeleteView(TestCase): + def setUp(self) -> None: + """Create an authorized Django user for testing purposes.""" + self.user = User.objects.create( + username="user#0000" + ) + + def test_redirect_when_logged_out(self): + """Test that the user is redirected to the homepage when not logged in.""" + url = reverse("account_delete") + resp = self.client.get(url) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + def test_get_when_logged_in(self): + """Test that the view returns a HTTP 200 when the user is logged in.""" + url = reverse("account_delete") + + self.client.force_login(self.user) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + def test_post_invalid(self): + """Test that the user is redirected when the form is filled out incorrectly.""" + url = reverse("account_delete") + + self.client.force_login(self.user) + + resp = self.client.post(url, {}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, url)) + + resp = self.client.post(url, {"username": "user"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, url)) + + self.client.logout() + + def test_post_valid(self): + """Test that the account is deleted when the form is filled out correctly..""" + url = reverse("account_delete") + + self.client.force_login(self.user) + + resp = self.client.post(url, {"username": "user#0000"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + with self.assertRaises(User.DoesNotExist): + User.objects.get(username=self.user.username) + + self.client.logout() + + +class TestAccountSettingsView(TestCase): + def setUp(self) -> None: + """Create an authorized Django user for testing purposes.""" + self.user = User.objects.create( + username="user#0000" + ) + + self.user_unlinked = User.objects.create( + username="user#9999" + ) + + self.user_unlinked_discord = User.objects.create( + username="user#1234" + ) + + self.user_unlinked_github = User.objects.create( + username="user#1111" + ) + + self.github_account = SocialAccount.objects.create( + user=self.user, + provider="github", + uid="0" + ) + + self.discord_account = SocialAccount.objects.create( + user=self.user, + provider="discord", + uid="0000" + ) + + self.github_account_secondary = SocialAccount.objects.create( + user=self.user_unlinked_discord, + provider="github", + uid="1" + ) + + self.discord_account_secondary = SocialAccount.objects.create( + user=self.user_unlinked_github, + provider="discord", + uid="1111" + ) + + def test_redirect_when_logged_out(self): + """Check that the user is redirected to the homepage when not logged in.""" + url = reverse("account_settings") + resp = self.client.get(url) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + def test_get_when_logged_in(self): + """Test that the view returns a HTTP 200 when the user is logged in.""" + url = reverse("account_settings") + + self.client.force_login(self.user) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + self.client.force_login(self.user_unlinked) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + self.client.force_login(self.user_unlinked_discord) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + self.client.force_login(self.user_unlinked_github) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + def test_post_invalid(self): + """Test the behaviour of invalid POST submissions.""" + url = reverse("account_settings") + + self.client.force_login(self.user_unlinked) + + resp = self.client.post(url, {"provider": "discord"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + resp = self.client.post(url, {"provider": "github"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + self.client.logout() + + def test_post_valid(self): + """Ensure that GitHub is unlinked with a valid POST submission.""" + url = reverse("account_settings") + + self.client.force_login(self.user) + + resp = self.client.post(url, {"provider": "github"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + with self.assertRaises(SocialAccount.DoesNotExist): + SocialAccount.objects.get(user=self.user, provider="github") + + self.client.logout() class TestIndexReturns200(TestCase): @@ -16,6 +209,7 @@ class TestLoginCancelledReturns302(TestCase): url = reverse('socialaccount_login_cancelled') resp = self.client.get(url) self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) class TestLoginErrorReturns302(TestCase): @@ -24,3 +218,4 @@ class TestLoginErrorReturns302(TestCase): url = reverse('socialaccount_login_error') resp = self.client.get(url) self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) diff --git a/pydis_site/apps/home/urls.py b/pydis_site/apps/home/urls.py index 211a7ad1..61e87a39 100644 --- a/pydis_site/apps/home/urls.py +++ b/pydis_site/apps/home/urls.py @@ -1,5 +1,4 @@ from allauth.account.views import LogoutView -from allauth.socialaccount.views import ConnectionsView from django.conf import settings from django.conf.urls.static import static from django.contrib import admin @@ -7,14 +6,18 @@ from django.contrib.messages import ERROR from django.urls import include, path from pydis_site.utils.views import MessageRedirectView -from .views import HomeView +from .views import AccountDeleteView, AccountSettingsView, HomeView app_name = 'home' urlpatterns = [ + # We do this twice because Allauth expects specific view names to exist path('', HomeView.as_view(), name='home'), + path('', HomeView.as_view(), name='socialaccount_connections'), + path('pages/', include('wiki.urls')), path('accounts/', include('allauth.socialaccount.providers.discord.urls')), + path('accounts/', include('allauth.socialaccount.providers.github.urls')), path( 'accounts/login/cancelled', MessageRedirectView.as_view( @@ -28,7 +31,9 @@ urlpatterns = [ ), name='socialaccount_login_error' ), - path('connections', ConnectionsView.as_view()), + path('accounts/settings', AccountSettingsView.as_view(), name="account_settings"), + path('accounts/delete', AccountDeleteView.as_view(), name="account_delete"), + path('logout', LogoutView.as_view(), name="logout"), path('admin/', admin.site.urls), diff --git a/pydis_site/apps/home/views/__init__.py b/pydis_site/apps/home/views/__init__.py index 971d73a3..801fd398 100644 --- a/pydis_site/apps/home/views/__init__.py +++ b/pydis_site/apps/home/views/__init__.py @@ -1,3 +1,4 @@ +from .account import DeleteView as AccountDeleteView, SettingsView as AccountSettingsView from .home import HomeView -__all__ = ["HomeView"] +__all__ = ["AccountDeleteView", "AccountSettingsView", "HomeView"] diff --git a/pydis_site/apps/home/views/account/__init__.py b/pydis_site/apps/home/views/account/__init__.py new file mode 100644 index 00000000..3b3250ea --- /dev/null +++ b/pydis_site/apps/home/views/account/__init__.py @@ -0,0 +1,4 @@ +from .delete import DeleteView +from .settings import SettingsView + +__all__ = ["DeleteView", "SettingsView"] diff --git a/pydis_site/apps/home/views/account/delete.py b/pydis_site/apps/home/views/account/delete.py new file mode 100644 index 00000000..798b8a33 --- /dev/null +++ b/pydis_site/apps/home/views/account/delete.py @@ -0,0 +1,37 @@ +from django.contrib.auth.mixins import LoginRequiredMixin +from django.contrib.messages import ERROR, INFO, add_message +from django.http import HttpRequest, HttpResponse +from django.shortcuts import redirect, render +from django.urls import reverse +from django.views import View + +from pydis_site.apps.home.forms.account_deletion import AccountDeletionForm + + +class DeleteView(LoginRequiredMixin, View): + """Account deletion view, for removing linked user accounts from the DB.""" + + def __init__(self, *args, **kwargs): + self.login_url = reverse("home") + super().__init__(*args, **kwargs) + + def get(self, request: HttpRequest) -> HttpResponse: + """HTTP GET: Return the view template.""" + return render( + request, "home/account/delete.html", + context={"form": AccountDeletionForm()} + ) + + def post(self, request: HttpRequest) -> HttpResponse: + """HTTP POST: Process the deletion, as requested by the user.""" + form = AccountDeletionForm(request.POST) + + if not form.is_valid() or request.user.username != form.cleaned_data["username"]: + add_message(request, ERROR, "Please enter your username exactly as shown.") + + return redirect(reverse("account_delete")) + + request.user.delete() + add_message(request, INFO, "Your account has been deleted.") + + return redirect(reverse("home")) diff --git a/pydis_site/apps/home/views/account/settings.py b/pydis_site/apps/home/views/account/settings.py new file mode 100644 index 00000000..3a817dbc --- /dev/null +++ b/pydis_site/apps/home/views/account/settings.py @@ -0,0 +1,59 @@ +from allauth.socialaccount.models import SocialAccount +from allauth.socialaccount.providers import registry +from django.contrib.auth.mixins import LoginRequiredMixin +from django.contrib.messages import ERROR, INFO, add_message +from django.http import HttpRequest, HttpResponse +from django.shortcuts import redirect, render +from django.urls import reverse +from django.views import View + + +class SettingsView(LoginRequiredMixin, View): + """ + Account settings view, for managing and deleting user accounts and connections. + + This view actually renders a template with a bare modal, and is intended to be + inserted into another template using JavaScript. + """ + + def __init__(self, *args, **kwargs): + self.login_url = reverse("home") + super().__init__(*args, **kwargs) + + def get(self, request: HttpRequest) -> HttpResponse: + """HTTP GET: Return the view template.""" + context = { + "groups": request.user.groups.all(), + + "discord": None, + "github": None, + + "discord_provider": registry.provider_map.get("discord"), + "github_provider": registry.provider_map.get("github"), + } + + for account in SocialAccount.objects.filter(user=request.user).all(): + if account.provider == "discord": + context["discord"] = account + + if account.provider == "github": + context["github"] = account + + return render(request, "home/account/settings.html", context=context) + + def post(self, request: HttpRequest) -> HttpResponse: + """HTTP POST: Process account disconnections.""" + provider = request.POST["provider"] + + if provider == "github": + try: + account = SocialAccount.objects.get(user=request.user, provider=provider) + except SocialAccount.DoesNotExist: + add_message(request, ERROR, "You do not have a GitHub account linked.") + else: + account.delete() + add_message(request, INFO, "The social account has been disconnected.") + else: + add_message(request, ERROR, f"Unknown provider: {provider}") + + return redirect(reverse("home")) diff --git a/pydis_site/apps/staff/migrations/0002_add_is_staff_to_role_mappings.py b/pydis_site/apps/staff/migrations/0002_add_is_staff_to_role_mappings.py new file mode 100644 index 00000000..0404d270 --- /dev/null +++ b/pydis_site/apps/staff/migrations/0002_add_is_staff_to_role_mappings.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.6 on 2019-10-20 14:19 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('staff', '0001_initial'), + ] + + operations = [ + migrations.AddField( + model_name='rolemapping', + name='is_staff', + field=models.BooleanField(default=False, help_text='Whether this role mapping relates to a Django staff group'), + ), + ] diff --git a/pydis_site/apps/staff/models/role_mapping.py b/pydis_site/apps/staff/models/role_mapping.py index 10c09cf1..8a1fac2e 100644 --- a/pydis_site/apps/staff/models/role_mapping.py +++ b/pydis_site/apps/staff/models/role_mapping.py @@ -21,6 +21,11 @@ class RoleMapping(models.Model): unique=True, # Unique in order to simplify group assignment logic ) + is_staff = models.BooleanField( + help_text="Whether this role mapping relates to a Django staff group", + default=False + ) + def __str__(self): """Returns the mapping, for display purposes.""" return f"@{self.role.name} -> {self.group.name}" diff --git a/pydis_site/settings.py b/pydis_site/settings.py index 94718ec7..66376c4e 100644 --- a/pydis_site/settings.py +++ b/pydis_site/settings.py @@ -92,6 +92,7 @@ INSTALLED_APPS = [ 'allauth.socialaccount', 'allauth.socialaccount.providers.discord', + 'allauth.socialaccount.providers.github', 'crispy_forms', 'django_crispy_bulma', @@ -302,14 +303,14 @@ BULMA_SETTINGS = { "variables": { # If you update these colours, please update the notification.css file "primary": "#7289DA", # Discord blurple - "orange": "#ffb39b", # Bulma default, but at a saturation of 100 - "yellow": "#ffea9b", # Bulma default, but at a saturation of 100 - "green": "#7fd19c", # Bulma default, but at a saturation of 100 - "turquoise": "#7289DA", # Blurple, because Bulma uses this as the default primary - "cyan": "#91cbee", # Bulma default, but at a saturation of 100 - "blue": "#86a7dc", # Bulma default, but at a saturation of 100 - "purple": "#b86bff", # Bulma default, but at a saturation of 100 - "red": "#ffafc2", # Bulma default, but at a saturation of 80 + # "orange": "", # Apparently unused, but the default is fine + # "yellow": "", # The default yellow looks pretty good + "green": "#32ac66", # Colour picked after Discord discussion + "turquoise": "#7289DA", # Blurple, because Bulma uses this regardless of `primary` above + "blue": "#2482c1", # Colour picked after Discord discussion + "cyan": "#2482c1", # Colour picked after Discord discussion (matches the blue) + "purple": "#aa55e4", # Apparently unused, but changed for consistency + "red": "#d63852", # Colour picked after Discord discussion "link": "$primary", @@ -407,5 +408,13 @@ AUTHENTICATION_BACKENDS = ( 'allauth.account.auth_backends.AuthenticationBackend', ) +ACCOUNT_ADAPTER = "pydis_site.utils.account.AccountAdapter" +ACCOUNT_EMAIL_REQUIRED = False # Undocumented allauth setting; don't require emails ACCOUNT_EMAIL_VERIFICATION = "none" # No verification required; we don't use emails for anything + +# We use this validator because Allauth won't let us actually supply a list with no validators +# in it, and we can't just give it a lambda - that'd be too easy, I suppose. +ACCOUNT_USERNAME_VALIDATORS = "pydis_site.VALIDATORS" + LOGIN_REDIRECT_URL = "home" +SOCIALACCOUNT_ADAPTER = "pydis_site.utils.account.SocialAccountAdapter" diff --git a/pydis_site/static/css/base/base.css b/pydis_site/static/css/base/base.css index 3ca6b2a7..75702d47 100644 --- a/pydis_site/static/css/base/base.css +++ b/pydis_site/static/css/base/base.css @@ -84,7 +84,25 @@ div.card.has-equal-height { /* Fix for logout form submit button in navbar */ -button.is-size-navbar-menu { +button.is-size-navbar-menu, a.is-size-navbar-menu { font-size: 14px; + padding-left: 1.5rem; + padding-right: 1.5rem; } +@media screen and (min-width: 1088px) { + button.is-size-navbar-menu, a.is-size-navbar-menu { + padding-left: 1rem; + padding-right: 1rem; + } +} + +/* Fix for modals being behind the navbar */ + +.modal * { + z-index: 1020; +} + +.modal-background { + z-index: 1010; +} diff --git a/pydis_site/static/js/base/modal.js b/pydis_site/static/js/base/modal.js new file mode 100644 index 00000000..eccc8845 --- /dev/null +++ b/pydis_site/static/js/base/modal.js @@ -0,0 +1,100 @@ +/* + modal.js: A simple way to wire up Bulma modals. + + This library is intended to be used with Bulma's modals, as described in the + official Bulma documentation. It's based on the JavaScript that Bulma + themselves use for this purpose on the modals documentation page. + + Note that, just like that piece of JavaScript, this library assumes that + you will only ever want to have one modal open at once. + */ + +"use strict"; + +// Event handler for the "esc" key, for closing modals. + +document.addEventListener("keydown", (event) => { + const e = event || window.event; + + if (e.code === "Escape" || e.keyCode === 27) { + closeModals(); + } +}); + +// An array of all the modal buttons we've already set up + +const modal_buttons = []; + +// Public API functions + +function setupModal(target) { + // Set up a modal's events, given a DOM element. This can be + // used later in order to set up a modal that was added after + // this library has been run. + + // We need to collect a bunch of elements to work with + const modal_background = Array.from(target.getElementsByClassName("modal-background")); + const modal_close = Array.from(target.getElementsByClassName("modal-close")); + + const modal_head = Array.from(target.getElementsByClassName("modal-card-head")); + const modal_foot = Array.from(target.getElementsByClassName("modal-card-foot")); + + const modal_delete = []; + const modal_button = []; + + modal_head.forEach((element) => modal_delete.concat(Array.from(element.getElementsByClassName("delete")))); + modal_foot.forEach((element) => modal_button.concat(Array.from(element.getElementsByClassName("button")))); + + // Collect all the elements that can be used to close modals + const modal_closers = modal_background.concat(modal_close).concat(modal_delete).concat(modal_button); + + // Assign click events for closing modals + modal_closers.forEach((element) => { + element.addEventListener("click", () => { + closeModals(); + }); + }); + + setupOpeningButtons(); +} + +function setupOpeningButtons() { + // Wire up all the opening buttons, avoiding buttons we've already wired up. + const modal_opening_buttons = Array.from(document.getElementsByClassName("modal-button")); + + modal_opening_buttons.forEach((element) => { + if (!modal_buttons.includes(element)) { + element.addEventListener("click", () => { + openModal(element.dataset.target); + }); + + modal_buttons.push(element); + } + }); +} + +function openModal(target) { + // Open a modal, given a string ID + const element = document.getElementById(target); + + document.documentElement.classList.add("is-clipped"); + element.classList.add("is-active"); +} + +function closeModals() { + // Close all open modals + const modals = Array.from(document.getElementsByClassName("modal")); + document.documentElement.classList.remove("is-clipped"); + + modals.forEach((element) => { + element.classList.remove("is-active"); + }); +} + +(function () { + // Set up all the modals currently on the page + const modals = Array.from(document.getElementsByClassName("modal")); + + modals.forEach((modal) => setupModal(modal)); + setupOpeningButtons(); +}()); diff --git a/pydis_site/templates/base/base.html b/pydis_site/templates/base/base.html index a9b31c0f..4c70d778 100644 --- a/pydis_site/templates/base/base.html +++ b/pydis_site/templates/base/base.html @@ -28,6 +28,7 @@ {# Font-awesome here is defined explicitly so that we can have Pro #} <script src="https://kit.fontawesome.com/ae6a3152d8.js"></script> + <script src="{% static "js/base/modal.js" %}"></script> <link rel="stylesheet" href="{% static "css/base/base.css" %}"> <link rel="stylesheet" href="{% static "css/base/notification.css" %}"> diff --git a/pydis_site/templates/base/navbar.html b/pydis_site/templates/base/navbar.html index 8cdac0de..2ba5bdd4 100644 --- a/pydis_site/templates/base/navbar.html +++ b/pydis_site/templates/base/navbar.html @@ -102,7 +102,15 @@ {% else %} <form method="post" action="{% url 'logout' %}"> {% csrf_token %} - <button type="submit" class="navbar-item button is-white is-inline is-fullwidth has-text-left is-size-navbar-menu has-text-grey-dark">Logout</button> + + <div class="field navbar-item is-paddingless is-fullwidth is-grouped"> + <button type="submit" class="button is-white is-inline is-fullwidth has-text-left is-size-navbar-menu has-text-grey-dark">Logout</button> + <a title="Settings" class="button is-white is-inline has-text-right is-size-navbar-menu has-text-grey-dark modal-button" data-target="account-modal"> + <span class="is-icon"> + <i class="fas fa-cog"></i> + </span> + </a> + </div> </form> {% endif %} @@ -116,3 +124,24 @@ </a> </div> </nav> + +{% if user.is_authenticated %} + <script defer type="text/javascript"> + // Script which loads and sets up the account settings modal. + // This script must be placed in a template, or rewritten to take the fetch + // URL as a function argument, in order to be used. + + "use strict"; + + // Create and prepend a new div for this modal + let element = document.createElement("div"); + document.body.prepend(element); + + fetch("{% url "account_settings" %}") // Fetch the URL + .then((response) => response.text()) // Read in the data stream as text + .then((text) => { + element.outerHTML = text; // Replace the div's HTML with the loaded modal HTML + setupModal(document.getElementById("account-modal")); // Set up the modal + }); + </script> +{% endif %} diff --git a/pydis_site/templates/home/account/delete.html b/pydis_site/templates/home/account/delete.html new file mode 100644 index 00000000..1020a82b --- /dev/null +++ b/pydis_site/templates/home/account/delete.html @@ -0,0 +1,44 @@ +{% extends 'base/base.html' %} + +{% load crispy_forms_tags %} +{% load static %} + +{% block title %}Delete Account{% endblock %} + +{% block content %} + {% include "base/navbar.html" %} + + <section class="section content"> + <div class="container"> + <h2 class="is-size-2 has-text-centered">Account Deletion</h2> + + <div class="columns is-centered"> + <div class="column is-half-desktop is-full-tablet is-full-mobile"> + + <article class="message is-danger"> + <div class="message-body"> + <p> + You have requested to delete the account with username + <strong><span class="has-text-dark is-family-monospace">{{ user.username }}</span></strong>. + </p> + + <p> + Please note that this <strong>cannot be undone</strong>. + </p> + + <p> + To verify that you'd like to remove your account, please type your username into the box below. + </p> + </div> + </article> + </div> + </div> + + <div class="columns is-centered"> + <div class="column is-half-desktop is-full-tablet is-full-mobile"> + {% crispy form %} + </div> + </div> + </div> + </section> +{% endblock %} diff --git a/pydis_site/templates/home/account/settings.html b/pydis_site/templates/home/account/settings.html new file mode 100644 index 00000000..9f48d736 --- /dev/null +++ b/pydis_site/templates/home/account/settings.html @@ -0,0 +1,137 @@ +{% load socialaccount %} + +{# This template is just for a modal, which is actually inserted into the navbar #} +{# template. Take a look at `navbar.html` to see how it's inserted. #} + +<div class="modal" id="account-modal"> + <div class="modal-background"></div> + <div class="modal-card"> + <div class="modal-card-head"> + <div class="modal-card-title">Settings for {{ user.username }}</div> + + {% if groups %} + <div> + {% for group in groups %} + <span class="tag is-primary">{{ group.name }}</span> + {% endfor %} + </div> + {% else %} + <span class="tag is-dark">No groups</span> + {% endif %} + </div> + <div class="modal-card-body"> + <h3 class="title">Connections</h3> + <div class="columns"> + {% if discord_provider is not None %} + <div class="column"> + <div class="box"> + {% if not discord %} + <div class="media"> + <div class="media-left"> + <div class="image"> + <i class="fab fa-discord fa-3x has-text-primary"></i> + </div> + </div> + <div class="media-content"> + <div class="title is-5">Discord</div> + <div class="subtitle is-6">Not connected</div> + </div> + </div> + <div> + <br /> + <a class="button is-primary" href="{% provider_login_url "discord" process="connect" %}"> + <span class="icon"> + <i class="fad fa-link"></i> + </span> + <span>Connect</span> + </a> + </div> + {% else %} + <div class="media"> + <div class="media-left"> + <div class="image"> + <i class="fab fa-discord fa-3x has-text-primary"></i> + </div> + </div> + <div class="media-content"> + <div class="title is-5">Discord</div> + <div class="subtitle is-6">{{ user.username }}</div> + </div> + </div> + <div> + <br /> + <button class="button" disabled> + <span class="icon"> + <i class="fas fa-check"></i> + </span> + <span>Connected</span> + </button> + </div> + {% endif %} + </div> + </div> + {% endif %} + + {% if github_provider is not None %} + <div class="column"> + <div class="box"> + {% if not github %} + <div class="media"> + <div class="media-left"> + <div class="image"> + <i class="fab fa-github fa-3x"></i> + </div> + </div> + <div class="media-content"> + <div class="title is-5">GitHub</div> + <div class="subtitle is-6">Not connected</div> + </div> + </div> + <div> + <br /> + <a class="button is-primary" href="{% provider_login_url "github" process="connect" %}"> + <span class="icon"> + <i class="fad fa-link"></i> + </span> + <span>Connect</span> + </a> + </div> + {% else %} + <div class="media"> + <div class="media-left"> + <div class="image"> + <i class="fab fa-github fa-3x"></i> + </div> + </div> + <div class="media-content"> + <div class="title is-5">GitHub</div> + <div class="subtitle is-6">{{ github.extra_data.name }}</div> + </div> + </div> + <div> + <form method="post" action="{% url "account_settings" %}" type="submit"> + {% csrf_token %} + + <input type="hidden" name="provider" value="github" /> + + <br /> + <button type="submit" class="button is-danger"> + <span class="icon"> + <i class="fas fa-times"></i> + </span> + <span>Disconnect</span> + </button> + </form> + </div> + {% endif %} + </div> + </div> + {% endif %} + </div> + </div> + <div class="modal-card-foot"> + <a class="button is-danger" href="{% url "account_delete" %}">Delete Account</a> + </div> + </div> +</div> + diff --git a/pydis_site/tests/test_utils_account.py b/pydis_site/tests/test_utils_account.py new file mode 100644 index 00000000..d2946368 --- /dev/null +++ b/pydis_site/tests/test_utils_account.py @@ -0,0 +1,132 @@ +from unittest.mock import patch + +from allauth.exceptions import ImmediateHttpResponse +from allauth.socialaccount.models import SocialAccount, SocialLogin +from django.contrib.auth.models import User +from django.contrib.messages.storage.base import BaseStorage +from django.http import HttpRequest +from django.test import TestCase + +from pydis_site.apps.api.models import Role, User as DiscordUser +from pydis_site.utils.account import AccountAdapter, SocialAccountAdapter + + +class AccountUtilsTests(TestCase): + def setUp(self): + self.django_user = User.objects.create(username="user") + + self.discord_account = SocialAccount.objects.create( + user=self.django_user, provider="discord", uid=0 + ) + + self.discord_account_role = SocialAccount.objects.create( + user=self.django_user, provider="discord", uid=1 + ) + + self.discord_account_two_roles = SocialAccount.objects.create( + user=self.django_user, provider="discord", uid=2 + ) + + self.discord_account_not_present = SocialAccount.objects.create( + user=self.django_user, provider="discord", uid=3 + ) + + self.github_account = SocialAccount.objects.create( + user=self.django_user, provider="github", uid=0 + ) + + self.discord_user = DiscordUser.objects.create( + id=0, + name="user", + discriminator=0 + ) + + self.discord_user_role = DiscordUser.objects.create( + id=1, + name="user present", + discriminator=0 + ) + + self.discord_user_two_roles = DiscordUser.objects.create( + id=2, + name="user with both roles", + discriminator=0 + ) + + everyone_role = Role.objects.create( + id=0, + name="@everyone", + colour=0, + permissions=0, + position=0 + ) + + self.discord_user_role.roles.add(everyone_role) + self.discord_user_two_roles.roles.add(everyone_role) + + self.discord_user_two_roles.roles.add(Role.objects.create( + id=1, + name="Developers", + colour=0, + permissions=0, + position=1 + )) + + def test_account_adapter(self): + """Test that our Allauth account adapter functions correctly.""" + adapter = AccountAdapter() + + self.assertFalse(adapter.is_open_for_signup(HttpRequest())) + + def test_social_account_adapter_signup(self): + """Test that our Allauth social account adapter correctly handles signups.""" + adapter = SocialAccountAdapter() + + discord_login = SocialLogin(account=self.discord_account) + discord_login_role = SocialLogin(account=self.discord_account_role) + discord_login_two_roles = SocialLogin(account=self.discord_account_two_roles) + discord_login_not_present = SocialLogin(account=self.discord_account_not_present) + + github_login = SocialLogin(account=self.github_account) + + messages_request = HttpRequest() + messages_request._messages = BaseStorage(messages_request) + + with patch("pydis_site.utils.account.reverse") as mock_reverse: + with patch("pydis_site.utils.account.redirect") as mock_redirect: + with self.assertRaises(ImmediateHttpResponse): + adapter.is_open_for_signup(messages_request, github_login) + + with self.assertRaises(ImmediateHttpResponse): + adapter.is_open_for_signup(messages_request, discord_login) + + with self.assertRaises(ImmediateHttpResponse): + adapter.is_open_for_signup(messages_request, discord_login_role) + + with self.assertRaises(ImmediateHttpResponse): + adapter.is_open_for_signup(messages_request, discord_login_not_present) + + self.assertEqual(len(messages_request._messages._queued_messages), 4) + self.assertEqual(mock_redirect.call_count, 4) + self.assertEqual(mock_reverse.call_count, 4) + + self.assertTrue(adapter.is_open_for_signup(HttpRequest(), discord_login_two_roles)) + + def test_social_account_adapter_populate(self): + """Test that our Allauth social account adapter correctly handles data population.""" + adapter = SocialAccountAdapter() + + discord_login = SocialLogin( + account=self.discord_account, + user=self.django_user + ) + + discord_login.account.extra_data["discriminator"] = "0000" + + user = adapter.populate_user( + HttpRequest(), discord_login, + {"username": "user"} + ) + + self.assertEqual(user.username, "user#0000") + self.assertEqual(user.first_name, "user#0000") diff --git a/pydis_site/utils/account.py b/pydis_site/utils/account.py new file mode 100644 index 00000000..e64919de --- /dev/null +++ b/pydis_site/utils/account.py @@ -0,0 +1,79 @@ +from typing import Any, Dict + +from allauth.account.adapter import DefaultAccountAdapter +from allauth.exceptions import ImmediateHttpResponse +from allauth.socialaccount.adapter import DefaultSocialAccountAdapter +from allauth.socialaccount.models import SocialLogin +from django.contrib.auth.models import User as DjangoUser +from django.contrib.messages import ERROR, add_message +from django.http import HttpRequest +from django.shortcuts import redirect +from django.urls import reverse + +from pydis_site.apps.api.models import User as DiscordUser + +ERROR_CONNECT_DISCORD = ("You must login with Discord before connecting another account. " + "Your account details have not been saved.") +ERROR_JOIN_DISCORD = ("Please join the Discord server and verify that you accept the rules and " + "privacy policy.") + + +class AccountAdapter(DefaultAccountAdapter): + """An Allauth account adapter that prevents signups via form submission.""" + + def is_open_for_signup(self, request: HttpRequest) -> bool: + """ + Checks whether or not the site is open for signups. + + We override this to always return False so that users may never sign up using + Allauth's signup form endpoints, to be on the safe side - since we only want users + to sign up using their Discord account. + """ + return False + + +class SocialAccountAdapter(DefaultSocialAccountAdapter): + """An Allauth SocialAccount adapter that prevents signups via non-Discord connections.""" + + def is_open_for_signup(self, request: HttpRequest, social_login: SocialLogin) -> bool: + """ + Checks whether or not the site is open for signups. + + We override this method in order to prevent users from creating a new account using + a non-Discord connection, as we require this connection for our users. + """ + if social_login.account.provider != "discord": + add_message(request, ERROR, ERROR_CONNECT_DISCORD) + + raise ImmediateHttpResponse(redirect(reverse("home"))) + + try: + user = DiscordUser.objects.get(id=int(social_login.account.uid)) + except DiscordUser.DoesNotExist: + add_message(request, ERROR, ERROR_JOIN_DISCORD) + + raise ImmediateHttpResponse(redirect(reverse("home"))) + + if user.roles.count() <= 1: + add_message(request, ERROR, ERROR_JOIN_DISCORD) + + raise ImmediateHttpResponse(redirect(reverse("home"))) + + return True + + def populate_user(self, request: HttpRequest, + social_login: SocialLogin, + data: Dict[str, Any]) -> DjangoUser: + """ + Method used to populate a Django User with data. + + We override this so that the Django user is created with the username#discriminator, + instead of just the username, as Django users must have unique usernames. For display + purposes, we also set the `name` key, which is used for `first_name` in the database. + """ + if social_login.account.provider == "discord": + discriminator = social_login.account.extra_data["discriminator"] + data["username"] = f"{data['username']}#{discriminator}" + data["name"] = data["username"] + + return super().populate_user(request, social_login, data) |