From 699f1a15d2524fa550a61d872c6cddd185e3be98 Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Thu, 3 Oct 2019 22:17:31 +0100
Subject: Hook up Allauth and model signals to handle group assignments

---
 pydis_site/apps/home/signals.py | 156 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)
 create mode 100644 pydis_site/apps/home/signals.py

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
new file mode 100644
index 00000000..687a99fe
--- /dev/null
+++ b/pydis_site/apps/home/signals.py
@@ -0,0 +1,156 @@
+from typing import List, Optional, Type
+
+from allauth.account.signals import user_logged_in
+from allauth.socialaccount.models import SocialAccount, SocialLogin
+from allauth.socialaccount.providers.base import Provider
+from allauth.socialaccount.providers.discord.provider import DiscordProvider
+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
+
+from pydis_site.apps.api.models import User as DiscordUser
+from pydis_site.apps.staff.models import RoleMapping
+
+
+class SignalListener:
+    """
+    Listens to and processes events via the Django Signals system.
+
+    Django Signals is basically an event dispatcher. It consists of Signals (which are the events)
+    and Receivers, which listen for and handle those events. Signals are triggered by Senders,
+    which are essentially just any class at all, and Receivers can filter the Signals they listen
+    for by choosing a Sender, if required.
+
+    Signals themselves define a set of arguments that they will provide to Receivers when the
+    Signal is sent. They are always keyword arguments, and Django recommends that all Receiver
+    functions accept them as `**kwargs` (and will supposedly error if you don't do this),
+    supposedly because Signals can change in the future and your receivers should still work.
+
+    Signals do provide a list of their arguments when they're initially constructed, but this
+    is purely for documentation purposes only and Django does not enforce it.
+
+    The Django Signals docs are here: https://docs.djangoproject.com/en/2.2/topics/signals/
+    """
+
+    def __init__(self):
+        post_save.connect(self.model_updated, sender=DiscordUser)
+
+        pre_social_login.connect(self.social_account_updated)
+        social_account_added.connect(self.social_account_updated)
+        social_account_updated.connect(self.social_account_updated)
+        social_account_removed.connect(self.social_account_removed)
+
+        user_logged_in.connect(self.user_logged_in)
+
+    def user_logged_in(self, sender: Type[DjangoUser], **kwargs) -> None:
+        """Handles signals relating to Allauth logins."""
+        user: DjangoUser = kwargs["user"]
+
+        try:
+            account: SocialAccount = SocialAccount.objects.get(
+                user=user, provider=DiscordProvider.id
+            )
+        except SocialAccount.DoesNotExist:
+            return  # User's never linked a Discord account
+        try:
+            discord_user: DiscordUser = DiscordUser.objects.get(id=int(account.uid))
+        except DiscordUser.DoesNotExist:
+            return
+
+        self._apply_groups(discord_user, account)
+
+    def social_account_updated(self, sender: Type[SocialLogin], **kwargs) -> None:
+        """Handle signals relating to new/existing social accounts."""
+        social_login: SocialLogin = kwargs["sociallogin"]
+
+        account: SocialAccount = social_login.account
+        provider: Provider = account.get_provider()
+
+        if not isinstance(provider, DiscordProvider):
+            return
+
+        try:
+            user: DiscordUser = DiscordUser.objects.get(id=int(account.uid))
+        except DiscordUser.DoesNotExist:
+            return
+
+        self._apply_groups(user, account)
+
+    def social_account_removed(self, sender: Type[SocialLogin], **kwargs) -> None:
+        """Handle signals relating to removal of social accounts."""
+        account: SocialAccount = kwargs["socialaccount"]
+        provider: Provider = account.get_provider()
+
+        if not isinstance(provider, DiscordProvider):
+            return
+
+        try:
+            user: DiscordUser = DiscordUser.objects.get(id=int(account.uid))
+        except DiscordUser.DoesNotExist:
+            return
+
+        self._apply_groups(user, account, True)
+
+    def model_updated(self, sender: Type[DiscordUser], **kwargs) -> None:
+        """Handle signals related to the updating of Discord User model entries."""
+        instance: DiscordUser = kwargs["instance"]
+        raw: bool = kwargs["raw"]
+
+        # `update_fields` could be used for checking changes, but it's None here due to how the
+        # model is saved without using that argument - so we can't use it.
+
+        if raw:
+            # Fixtures are being loaded, so don't touch anything
+            return
+
+        try:
+            account: SocialAccount = SocialAccount.objects.get(uid=str(instance.id))
+        except SocialAccount.DoesNotExist:
+            return  # User has never logged in with Discord on the site
+
+        self._apply_groups(instance, account)
+
+    def _apply_groups(
+            self, user: DiscordUser, account: Optional[SocialAccount], deletion: bool = False
+    ) -> None:
+        mappings = RoleMapping.objects.all()
+
+        try:
+            current_groups: List[Group] = list(account.user.groups.all())
+        except SocialAccount.user.RelatedObjectDoesNotExist:
+            return  # There's no user account yet, this will be handled by another receiver
+
+        if not user.in_guild:
+            deletion = True
+
+        if deletion:
+            # They've unlinked Discord or left the server, so we have to remove their groups
+
+            if not current_groups:
+                return  # They have no groups anyway, no point in processing
+
+            account.user.groups.remove(
+                *(mapping.group for mapping in mappings)
+            )
+        else:
+            new_groups = []
+
+            for role in user.roles.all():
+                try:
+                    new_groups.append(mappings.get(role=role).group)
+                except RoleMapping.DoesNotExist:
+                    continue  # No mapping exists
+
+            remove_groups = [
+                mapping.group for mapping in mappings if mapping.group not in new_groups
+            ]
+
+            add_groups = [group for group in new_groups if group not in current_groups]
+
+            if remove_groups:
+                account.user.groups.remove(*remove_groups)
+
+            if add_groups:
+                account.user.groups.add(*add_groups)
-- 
cgit v1.2.3


From 292aed85964ae5ed226d329d61c49d4c67bf6c81 Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Fri, 4 Oct 2019 22:31:19 +0100
Subject: Tests for signal handlers

---
 pydis_site/apps/home/signals.py                    |  13 +-
 pydis_site/apps/home/tests/test_signal_listener.py | 283 +++++++++++++++++++++
 2 files changed, 291 insertions(+), 5 deletions(-)
 create mode 100644 pydis_site/apps/home/tests/test_signal_listener.py

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index 687a99fe..691332d2 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -1,4 +1,4 @@
-from typing import List, Optional, Type
+from typing import List, Type
 
 from allauth.account.signals import user_logged_in
 from allauth.socialaccount.models import SocialAccount, SocialLogin
@@ -42,9 +42,9 @@ class SignalListener:
         social_account_updated.connect(self.social_account_updated)
         social_account_removed.connect(self.social_account_removed)
 
-        user_logged_in.connect(self.user_logged_in)
+        user_logged_in.connect(self.user_login)
 
-    def user_logged_in(self, sender: Type[DjangoUser], **kwargs) -> None:
+    def user_login(self, sender: Type[DjangoUser], **kwargs) -> None:
         """Handles signals relating to Allauth logins."""
         user: DjangoUser = kwargs["user"]
 
@@ -54,6 +54,7 @@ class SignalListener:
             )
         except SocialAccount.DoesNotExist:
             return  # User's never linked a Discord account
+
         try:
             discord_user: DiscordUser = DiscordUser.objects.get(id=int(account.uid))
         except DiscordUser.DoesNotExist:
@@ -106,14 +107,16 @@ class SignalListener:
             return
 
         try:
-            account: SocialAccount = SocialAccount.objects.get(uid=str(instance.id))
+            account: SocialAccount = SocialAccount.objects.get(
+                uid=str(instance.id), provider=DiscordProvider.id
+            )
         except SocialAccount.DoesNotExist:
             return  # User has never logged in with Discord on the site
 
         self._apply_groups(instance, account)
 
     def _apply_groups(
-            self, user: DiscordUser, account: Optional[SocialAccount], deletion: bool = False
+            self, user: DiscordUser, account: SocialAccount, deletion: bool = False
     ) -> None:
         mappings = RoleMapping.objects.all()
 
diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py
new file mode 100644
index 00000000..507714b9
--- /dev/null
+++ b/pydis_site/apps/home/tests/test_signal_listener.py
@@ -0,0 +1,283 @@
+from unittest import mock
+
+from allauth.account.signals import user_logged_in
+from allauth.socialaccount.models import SocialAccount, SocialLogin
+from allauth.socialaccount.providers.discord.provider import DiscordProvider
+from allauth.socialaccount.providers.github.provider import GitHubProvider
+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
+from django.test import TestCase
+
+from pydis_site.apps.api.models import Role, User as DiscordUser
+from pydis_site.apps.home.signals import SignalListener
+from pydis_site.apps.staff.models import RoleMapping
+
+
+class SignalListenerTests(TestCase):
+    @classmethod
+    def setUpTestData(cls):
+        cls.admin_role = Role.objects.create(
+            id=0,
+            name="admin",
+            colour=0,
+            permissions=0,
+            position=0
+        )
+
+        cls.admin_group = Group.objects.create(
+            name="admin"
+        )
+
+        cls.role_mapping = RoleMapping.objects.create(
+            role=cls.admin_role,
+            group=cls.admin_group
+        )
+
+        cls.unmapped_role = Role.objects.create(
+            id=1,
+            name="unmapped",
+            colour=0,
+            permissions=0,
+            position=1
+        )
+
+        cls.discord_user = DiscordUser.objects.create(
+            id=0,
+            name="user",
+            discriminator=0,
+            avatar_hash=None
+        )
+
+        cls.discord_unmapped = DiscordUser.objects.create(
+            id=2,
+            name="unmapped",
+            discriminator=0,
+            avatar_hash=None
+        )
+
+        cls.discord_unmapped.roles.add(cls.unmapped_role)
+        cls.discord_unmapped.save()
+
+        cls.discord_not_in_guild = DiscordUser.objects.create(
+            id=3,
+            name="not-in-guild",
+            discriminator=0,
+            avatar_hash=None,
+            in_guild=False
+        )
+
+        cls.discord_admin = DiscordUser.objects.create(
+            id=1,
+            name="admin",
+            discriminator=0,
+            avatar_hash=None
+        )
+
+        cls.discord_admin.roles.set([cls.admin_role])
+        cls.discord_admin.save()
+
+        cls.django_user_discordless = DjangoUser.objects.create(
+            username="no-discord"
+        )
+
+        cls.django_user_never_joined = DjangoUser.objects.create(
+            username="never-joined"
+        )
+
+        cls.social_never_joined = SocialAccount.objects.create(
+            user=cls.django_user_never_joined,
+            provider=DiscordProvider.id,
+            uid=5
+        )
+
+        cls.django_user = DjangoUser.objects.create(
+            username="user"
+        )
+
+        cls.social_user = SocialAccount.objects.create(
+            user=cls.django_user,
+            provider=DiscordProvider.id,
+            uid=cls.discord_user.id
+        )
+
+        cls.social_user_github = SocialAccount.objects.create(
+            user=cls.django_user,
+            provider=GitHubProvider.id,
+            uid=cls.discord_user.id
+        )
+
+        cls.social_unmapped = SocialAccount(
+            # We instantiate it and don't put it in the DB, this is (surprisingly)
+            # a realistic test case, so we need to check for it
+
+            provider=DiscordProvider.id,
+            uid=5,
+            user_id=None  # Doesn't exist (yes, this is possible)
+        )
+
+        cls.django_admin = DjangoUser.objects.create(
+            username="admin",
+            is_staff=True,
+            is_superuser=True
+        )
+
+        cls.social_admin = SocialAccount.objects.create(
+            user=cls.django_admin,
+            provider=DiscordProvider.id,
+            uid=cls.discord_admin.id
+        )
+
+    def test_model_save(self):
+        mock_obj = mock.Mock()
+
+        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
+            _ = SignalListener()
+
+            post_save.send(
+                DiscordUser,
+                instance=self.discord_user,
+                raw=True,
+                created=None,  # Not realistic, but we don't use it
+                using=None,   # Again, we don't use it
+                update_fields=False  # Always false during integration testing
+            )
+
+            mock_obj.assert_not_called()
+
+            post_save.send(
+                DiscordUser,
+                instance=self.discord_user,
+                raw=False,
+                created=None,  # Not realistic, but we don't use it
+                using=None,   # Again, we don't use it
+                update_fields=False  # Always false during integration testing
+            )
+
+            mock_obj.assert_called_with(self.discord_user, self.social_user)
+
+    def test_pre_social_login(self):
+        mock_obj = mock.Mock()
+
+        discord_login = SocialLogin(self.django_user, self.social_user)
+        github_login = SocialLogin(self.django_user, self.social_user_github)
+        unmapped_login = SocialLogin(self.django_user, self.social_unmapped)
+
+        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
+            _ = SignalListener()
+
+            pre_social_login.send(SocialLogin, sociallogin=github_login)
+            mock_obj.assert_not_called()
+
+            pre_social_login.send(SocialLogin, sociallogin=unmapped_login)
+            mock_obj.assert_not_called()
+
+            pre_social_login.send(SocialLogin, sociallogin=discord_login)
+            mock_obj.assert_called_with(self.discord_user, self.social_user)
+
+    def test_social_added(self):
+        mock_obj = mock.Mock()
+
+        discord_login = SocialLogin(self.django_user, self.social_user)
+        github_login = SocialLogin(self.django_user, self.social_user_github)
+        unmapped_login = SocialLogin(self.django_user, self.social_unmapped)
+
+        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
+            _ = SignalListener()
+
+            social_account_added.send(SocialLogin, sociallogin=github_login)
+            mock_obj.assert_not_called()
+
+            social_account_added.send(SocialLogin, sociallogin=unmapped_login)
+            mock_obj.assert_not_called()
+
+            social_account_added.send(SocialLogin, sociallogin=discord_login)
+            mock_obj.assert_called_with(self.discord_user, self.social_user)
+
+    def test_social_updated(self):
+        mock_obj = mock.Mock()
+
+        discord_login = SocialLogin(self.django_user, self.social_user)
+        github_login = SocialLogin(self.django_user, self.social_user_github)
+        unmapped_login = SocialLogin(self.django_user, self.social_unmapped)
+
+        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
+            _ = SignalListener()
+
+            social_account_updated.send(SocialLogin, sociallogin=github_login)
+            mock_obj.assert_not_called()
+
+            social_account_updated.send(SocialLogin, sociallogin=unmapped_login)
+            mock_obj.assert_not_called()
+
+            social_account_updated.send(SocialLogin, sociallogin=discord_login)
+            mock_obj.assert_called_with(self.discord_user, self.social_user)
+
+    def test_social_removed(self):
+        mock_obj = mock.Mock()
+
+        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
+            _ = SignalListener()
+
+            social_account_removed.send(SocialLogin, socialaccount=self.social_user_github)
+            mock_obj.assert_not_called()
+
+            social_account_removed.send(SocialLogin, socialaccount=self.social_unmapped)
+            mock_obj.assert_not_called()
+
+            social_account_removed.send(SocialLogin, socialaccount=self.social_user)
+            mock_obj.assert_called_with(self.discord_user, self.social_user, True)
+
+    def test_logged_in(self):
+        mock_obj = mock.Mock()
+
+        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
+            _ = SignalListener()
+
+            user_logged_in.send(DjangoUser, user=self.django_user_discordless)
+            mock_obj.assert_not_called()
+
+            user_logged_in.send(DjangoUser, user=self.django_user_never_joined)
+            mock_obj.assert_not_called()
+
+            user_logged_in.send(DjangoUser, user=self.django_user)
+            mock_obj.assert_called_with(self.discord_user, self.social_user)
+
+    def test_apply_groups_admin(self):
+        handler = SignalListener()
+
+        self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
+
+        handler._apply_groups(self.discord_admin, self.social_admin)
+        self.assertTrue(self.admin_group in self.django_admin.groups.all())
+
+        handler._apply_groups(self.discord_admin, self.social_admin, True)
+        self.assertTrue(self.admin_group not in self.django_admin.groups.all())
+
+        handler._apply_groups(self.discord_admin, self.social_admin)
+        self.discord_admin.roles.clear()
+
+        handler._apply_groups(self.discord_admin, self.social_admin)
+        self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
+
+        self.discord_admin.roles.add(self.admin_role)
+        self.discord_admin.save()
+
+    def test_apply_groups_other(self):
+        handler = SignalListener()
+
+        self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
+
+        handler._apply_groups(self.discord_unmapped, self.social_unmapped)
+        self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
+
+        handler._apply_groups(self.discord_unmapped, self.social_user)
+        self.assertTrue(self.django_user.groups.all().count() == 0)
+
+        handler._apply_groups(self.discord_not_in_guild, self.social_user)
+        self.assertTrue(self.django_user.groups.all().count() == 0)
+
+        handler._apply_groups(self.discord_not_in_guild, self.social_user)
+        self.assertTrue(self.django_user.groups.all().count() == 0)
-- 
cgit v1.2.3


From f8367074de14102354378c706c781096fecb9225 Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Fri, 4 Oct 2019 22:49:18 +0100
Subject: Clean up and comment signal tests

---
 pydis_site/apps/home/signals.py                    |  4 +-
 pydis_site/apps/home/tests/test_signal_listener.py | 46 ++++++++++++++--------
 2 files changed, 31 insertions(+), 19 deletions(-)

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index 691332d2..a5577f41 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -42,9 +42,9 @@ class SignalListener:
         social_account_updated.connect(self.social_account_updated)
         social_account_removed.connect(self.social_account_removed)
 
-        user_logged_in.connect(self.user_login)
+        user_logged_in.connect(self.user_logged_in)
 
-    def user_login(self, sender: Type[DjangoUser], **kwargs) -> None:
+    def user_logged_in(self, sender: Type[DjangoUser], **kwargs) -> None:
         """Handles signals relating to Allauth logins."""
         user: DjangoUser = kwargs["user"]
 
diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py
index 5ae17dea..b7400558 100644
--- a/pydis_site/apps/home/tests/test_signal_listener.py
+++ b/pydis_site/apps/home/tests/test_signal_listener.py
@@ -28,9 +28,7 @@ class SignalListenerTests(TestCase):
             position=0
         )
 
-        cls.admin_group = Group.objects.create(
-            name="admin"
-        )
+        cls.admin_group = Group.objects.create(name="admin")
 
         cls.role_mapping = RoleMapping.objects.create(
             role=cls.admin_role,
@@ -80,13 +78,8 @@ class SignalListenerTests(TestCase):
         cls.discord_admin.roles.set([cls.admin_role])
         cls.discord_admin.save()
 
-        cls.django_user_discordless = DjangoUser.objects.create(
-            username="no-discord"
-        )
-
-        cls.django_user_never_joined = DjangoUser.objects.create(
-            username="never-joined"
-        )
+        cls.django_user_discordless = DjangoUser.objects.create(username="no-discord")
+        cls.django_user_never_joined = DjangoUser.objects.create(username="never-joined")
 
         cls.social_never_joined = SocialAccount.objects.create(
             user=cls.django_user_never_joined,
@@ -94,9 +87,7 @@ class SignalListenerTests(TestCase):
             uid=5
         )
 
-        cls.django_user = DjangoUser.objects.create(
-            username="user"
-        )
+        cls.django_user = DjangoUser.objects.create(username="user")
 
         cls.social_user = SocialAccount.objects.create(
             user=cls.django_user,
@@ -171,12 +162,15 @@ class SignalListenerTests(TestCase):
         with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
             _ = SignalListener()
 
+            # Don't attempt to apply groups if the user doesn't have a linked Discord account
             pre_social_login.send(SocialLogin, sociallogin=github_login)
             mock_obj.assert_not_called()
 
+            # Don't attempt to apply groups if the user hasn't joined the Discord server
             pre_social_login.send(SocialLogin, sociallogin=unmapped_login)
             mock_obj.assert_not_called()
 
+            # Attempt to apply groups if everything checks out
             pre_social_login.send(SocialLogin, sociallogin=discord_login)
             mock_obj.assert_called_with(self.discord_user, self.social_user)
 
@@ -191,12 +185,15 @@ class SignalListenerTests(TestCase):
         with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
             _ = SignalListener()
 
+            # Don't attempt to apply groups if the user doesn't have a linked Discord account
             social_account_added.send(SocialLogin, sociallogin=github_login)
             mock_obj.assert_not_called()
 
+            # Don't attempt to apply groups if the user hasn't joined the Discord server
             social_account_added.send(SocialLogin, sociallogin=unmapped_login)
             mock_obj.assert_not_called()
 
+            # Attempt to apply groups if everything checks out
             social_account_added.send(SocialLogin, sociallogin=discord_login)
             mock_obj.assert_called_with(self.discord_user, self.social_user)
 
@@ -211,12 +208,15 @@ class SignalListenerTests(TestCase):
         with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
             _ = SignalListener()
 
+            # Don't attempt to apply groups if the user doesn't have a linked Discord account
             social_account_updated.send(SocialLogin, sociallogin=github_login)
             mock_obj.assert_not_called()
 
+            # Don't attempt to apply groups if the user hasn't joined the Discord server
             social_account_updated.send(SocialLogin, sociallogin=unmapped_login)
             mock_obj.assert_not_called()
 
+            # Attempt to apply groups if everything checks out
             social_account_updated.send(SocialLogin, sociallogin=discord_login)
             mock_obj.assert_called_with(self.discord_user, self.social_user)
 
@@ -227,12 +227,15 @@ class SignalListenerTests(TestCase):
         with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
             _ = SignalListener()
 
+            # Don't attempt to remove groups if the user doesn't have a linked Discord account
             social_account_removed.send(SocialLogin, socialaccount=self.social_user_github)
             mock_obj.assert_not_called()
 
+            # Don't attempt to remove groups if the social account doesn't map to a Django user
             social_account_removed.send(SocialLogin, socialaccount=self.social_unmapped)
             mock_obj.assert_not_called()
 
+            # Attempt to remove groups if everything checks out
             social_account_removed.send(SocialLogin, socialaccount=self.social_user)
             mock_obj.assert_called_with(self.discord_user, self.social_user, True)
 
@@ -243,12 +246,15 @@ class SignalListenerTests(TestCase):
         with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
             _ = SignalListener()
 
+            # Don't attempt to apply groups if the user doesn't have a linked Discord account
             user_logged_in.send(DjangoUser, user=self.django_user_discordless)
             mock_obj.assert_not_called()
 
+            # Don't attempt to apply groups if the user hasn't joined the Discord server
             user_logged_in.send(DjangoUser, user=self.django_user_never_joined)
             mock_obj.assert_not_called()
 
+            # Attempt to apply groups if everything checks out
             user_logged_in.send(DjangoUser, user=self.django_user)
             mock_obj.assert_called_with(self.discord_user, self.social_user)
 
@@ -258,15 +264,21 @@ class SignalListenerTests(TestCase):
 
         self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
 
+        # Apply groups based on admin role being present on Discord
         handler._apply_groups(self.discord_admin, self.social_admin)
         self.assertTrue(self.admin_group in self.django_admin.groups.all())
 
+        # Remove groups based on the user apparently leaving the server
         handler._apply_groups(self.discord_admin, self.social_admin, True)
-        self.assertTrue(self.admin_group not in self.django_admin.groups.all())
+        self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
 
+        # Apply the admin role again
         handler._apply_groups(self.discord_admin, self.social_admin)
+
+        # Remove all of the roles from the user
         self.discord_admin.roles.clear()
 
+        # Remove groups based on the user no longer having the admin role on Discord
         handler._apply_groups(self.discord_admin, self.social_admin)
         self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
 
@@ -279,15 +291,15 @@ class SignalListenerTests(TestCase):
 
         self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
 
+        # No groups should be applied when there's no user account yet
         handler._apply_groups(self.discord_unmapped, self.social_unmapped)
         self.assertTrue(self.django_user_discordless.groups.all().count() == 0)
 
+        # No groups should be applied when there are only unmapped roles to match
         handler._apply_groups(self.discord_unmapped, self.social_user)
         self.assertTrue(self.django_user.groups.all().count() == 0)
 
-        handler._apply_groups(self.discord_not_in_guild, self.social_user)
-        self.assertTrue(self.django_user.groups.all().count() == 0)
-
+        # No groups should be applied when the user isn't in the guild
         handler._apply_groups(self.discord_not_in_guild, self.social_user)
         self.assertTrue(self.django_user.groups.all().count() == 0)
 
-- 
cgit v1.2.3


From c55ff9a4964a63af18879834abefc5224aa68cdf Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Fri, 11 Oct 2019 19:16:37 +0100
Subject: Clarify _apply_groups kwarg (Thanks, @jchristgit)

Co-Authored-By: Johannes Christ <jc@jchri.st>
---
 pydis_site/apps/home/signals.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index a5577f41..f0be3db2 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -92,7 +92,7 @@ class SignalListener:
         except DiscordUser.DoesNotExist:
             return
 
-        self._apply_groups(user, account, True)
+        self._apply_groups(user, account, deletion=True)
 
     def model_updated(self, sender: Type[DiscordUser], **kwargs) -> None:
         """Handle signals related to the updating of Discord User model entries."""
-- 
cgit v1.2.3


From f6db7cc458e63a6c88a3142110bb4f744a4c5920 Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Fri, 11 Oct 2019 19:26:13 +0100
Subject: Simplify signals.py as per @jchristgit's review

---
 pydis_site/apps/home/signals.py | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index f0be3db2..1d094d50 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -146,14 +146,10 @@ class SignalListener:
                 except RoleMapping.DoesNotExist:
                     continue  # No mapping exists
 
-            remove_groups = [
-                mapping.group for mapping in mappings if mapping.group not in new_groups
-            ]
+                account.user.groups.add(
+                    group for group in new_groups if group not in current_groups
+                )
 
-            add_groups = [group for group in new_groups if group not in current_groups]
-
-            if remove_groups:
-                account.user.groups.remove(*remove_groups)
-
-            if add_groups:
-                account.user.groups.add(*add_groups)
+                account.user.groups.remove(
+                    mapping.group for mapping in mappings if mapping.group not in new_groups
+                )
-- 
cgit v1.2.3


From 84a809677d6bff54c88d6e67d209d2f6f7e2b6e7 Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Fri, 11 Oct 2019 19:46:58 +0100
Subject: Signals: `add()` does require a list, a generator won't do.

---
 pydis_site/apps/home/signals.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index 1d094d50..c8bf5376 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -147,9 +147,9 @@ class SignalListener:
                     continue  # No mapping exists
 
                 account.user.groups.add(
-                    group for group in new_groups if group not in current_groups
+                    *[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
+                    *[mapping.group for mapping in mappings if mapping.group not in new_groups]
                 )
-- 
cgit v1.2.3


From 953195e93c667d39bf213b8c94082bc8f826344d Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Fri, 11 Oct 2019 22:32:08 +0100
Subject: Signals: Handle (and test) mapping updates/deletions

This also enforces unique values for both attributes on the RoleMapping model. Supporting configurations where this isn't the case would introduce quite a lot of added complexity.
---
 pydis_site/apps/home/signals.py                    | 45 ++++++++++-
 pydis_site/apps/home/tests/test_signal_listener.py | 88 ++++++++++++++++++++--
 pydis_site/apps/staff/models/role_mapping.py       |  6 +-
 3 files changed, 125 insertions(+), 14 deletions(-)

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index c8bf5376..65b24ddf 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -1,4 +1,4 @@
-from typing import List, Type
+from typing import List, Optional, Type
 
 from allauth.account.signals import user_logged_in
 from allauth.socialaccount.models import SocialAccount, SocialLogin
@@ -8,7 +8,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
+from django.db.models.signals import post_save, pre_delete, pre_save
 
 from pydis_site.apps.api.models import User as DiscordUser
 from pydis_site.apps.staff.models import RoleMapping
@@ -35,7 +35,10 @@ class SignalListener:
     """
 
     def __init__(self):
-        post_save.connect(self.model_updated, sender=DiscordUser)
+        post_save.connect(self.user_model_updated, sender=DiscordUser)
+
+        pre_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)
         social_account_added.connect(self.social_account_updated)
@@ -94,7 +97,41 @@ class SignalListener:
 
         self._apply_groups(user, account, deletion=True)
 
-    def model_updated(self, sender: Type[DiscordUser], **kwargs) -> None:
+    def mapping_model_deleted(self, sender: Type[RoleMapping], **kwargs) -> None:
+        """Handle signals related to the deletion of Role Mapping model entries."""
+        instance: RoleMapping = kwargs["instance"]
+
+        for user in instance.group.user_set.all():
+            user.groups.remove(instance.group)
+
+    def mapping_model_updated(self, sender: Type[RoleMapping], **kwargs) -> None:
+        """Handle signals related to the updating of Role Mapping model entries."""
+        instance: RoleMapping = kwargs["instance"]
+        raw: bool = kwargs["raw"]
+
+        if raw:
+            # Fixtures are being loaded, so don't touch anything
+            return
+
+        old_instance: Optional[RoleMapping] = None
+
+        if instance.id is not None:
+            # We don't try to catch DoesNotExist here because we can't test for it,
+            # it should never happen (unless we have a bad DB failure)
+
+            old_instance = RoleMapping.objects.get(id=instance.id)
+
+        if old_instance:
+            self.mapping_model_deleted(RoleMapping, instance=old_instance)
+
+        accounts = SocialAccount.objects.filter(
+            uid__in=(u.id for u in instance.role.user_set.all())
+        )
+
+        for account in accounts:
+            account.user.groups.add(instance.group)
+
+    def user_model_updated(self, sender: Type[DiscordUser], **kwargs) -> None:
         """Handle signals related to the updating of Discord User model entries."""
         instance: DiscordUser = kwargs["instance"]
         raw: bool = kwargs["raw"]
diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py
index 2eaaa945..ee78999f 100644
--- a/pydis_site/apps/home/tests/test_signal_listener.py
+++ b/pydis_site/apps/home/tests/test_signal_listener.py
@@ -9,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
+from django.db.models.signals import post_save, pre_save
 from django.test import TestCase
 
 from pydis_site.apps.api.models import Role, User as DiscordUser
@@ -37,21 +37,35 @@ class SignalListenerTests(TestCase):
             position=0
         )
 
-        cls.admin_group = Group.objects.create(name="admin")
-
-        cls.role_mapping = RoleMapping.objects.create(
-            role=cls.admin_role,
-            group=cls.admin_group
+        cls.moderator_role = Role.objects.create(
+            id=1,
+            name="moderator",
+            colour=0,
+            permissions=0,
+            position=1
         )
 
         cls.unmapped_role = Role.objects.create(
-            id=1,
+            id=2,
             name="unmapped",
             colour=0,
             permissions=0,
             position=1
         )
 
+        cls.admin_group = Group.objects.create(name="admin")
+        cls.moderator_group = Group.objects.create(name="moderator")
+
+        cls.admin_mapping = RoleMapping.objects.create(
+            role=cls.admin_role,
+            group=cls.admin_group
+        )
+
+        cls.moderator_mapping = RoleMapping.objects.create(
+            role=cls.moderator_role,
+            group=cls.moderator_group
+        )
+
         cls.discord_user = DiscordUser.objects.create(
             id=0,
             name="user",
@@ -87,6 +101,16 @@ class SignalListenerTests(TestCase):
         cls.discord_admin.roles.set([cls.admin_role])
         cls.discord_admin.save()
 
+        cls.discord_moderator = DiscordUser.objects.create(
+            id=4,
+            name="admin",
+            discriminator=0,
+            avatar_hash=None
+        )
+
+        cls.discord_moderator.roles.set([cls.moderator_role])
+        cls.discord_moderator.save()
+
         cls.django_user_discordless = DjangoUser.objects.create(username="no-discord")
         cls.django_user_never_joined = DjangoUser.objects.create(username="never-joined")
 
@@ -131,6 +155,18 @@ class SignalListenerTests(TestCase):
             uid=cls.discord_admin.id
         )
 
+        cls.django_moderator = DjangoUser.objects.create(
+            username="moderator",
+            is_staff=True,
+            is_superuser=False
+        )
+
+        cls.social_moderator = SocialAccount.objects.create(
+            user=cls.django_moderator,
+            provider=DiscordProvider.id,
+            uid=cls.discord_moderator.id
+        )
+
     def test_model_save(self):
         """Test signal handling for when Discord user model objects are saved to DB."""
         mock_obj = mock.Mock()
@@ -315,6 +351,42 @@ class SignalListenerTests(TestCase):
     def test_role_mapping_str(self):
         """Test that role mappings stringify correctly."""
         self.assertEqual(
-            str(self.role_mapping),
+            str(self.admin_mapping),
             f"@{self.admin_role.name} -> {self.admin_group.name}"
         )
+
+    def test_role_mapping_changes(self):
+        """Test that role mapping listeners work when changes are made."""
+        # Set up (just for this test)
+        self.django_moderator.groups.add(self.moderator_group)
+        self.django_admin.groups.add(self.admin_group)
+
+        self.assertEqual(self.django_moderator.groups.all().count(), 1)
+        self.assertEqual(self.django_admin.groups.all().count(), 1)
+
+        # Test mapping deletion
+        self.admin_mapping.delete()
+
+        self.assertEqual(self.django_admin.groups.all().count(), 0)
+
+        # Test mapping update
+        self.moderator_mapping.group = self.admin_group
+        self.moderator_mapping.save()
+
+        self.assertEqual(self.django_moderator.groups.all().count(), 1)
+        self.assertTrue(self.admin_group in self.django_moderator.groups.all())
+
+        # Test mapping creation
+        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())
+
+        # Test that nothing happens when fixtures are loaded
+        pre_save.send(RoleMapping, instance=new_mapping, raw=True)
+
+        self.assertEqual(self.django_admin.groups.all().count(), 1)
+        self.assertTrue(self.moderator_group in self.django_admin.groups.all())
diff --git a/pydis_site/apps/staff/models/role_mapping.py b/pydis_site/apps/staff/models/role_mapping.py
index 5c728283..10c09cf1 100644
--- a/pydis_site/apps/staff/models/role_mapping.py
+++ b/pydis_site/apps/staff/models/role_mapping.py
@@ -10,13 +10,15 @@ class RoleMapping(models.Model):
     role = models.OneToOneField(
         Role,
         on_delete=models.CASCADE,
-        help_text="The Discord role to use for this mapping."
+        help_text="The Discord role to use for this mapping.",
+        unique=True,  # Unique in order to simplify group assignment logic
     )
 
     group = models.OneToOneField(
         Group,
         on_delete=models.CASCADE,
-        help_text="The Django permissions group to use for this mapping."
+        help_text="The Django permissions group to use for this mapping.",
+        unique=True,  # Unique in order to simplify group assignment logic
     )
 
     def __str__(self):
-- 
cgit v1.2.3


From 2055e0d54ca743557e6209e4b3e3f85e15c59fe2 Mon Sep 17 00:00:00 2001
From: Gareth Coles <gdude2002@gmail.com>
Date: Mon, 14 Oct 2019 11:23:50 +0100
Subject: Allauth: Address reviews by @lemonsaurus

---
 pydis_site/apps/home/apps.py                       |  4 +-
 pydis_site/apps/home/signals.py                    | 84 +++++++++++++++++++---
 pydis_site/apps/home/tests/test_signal_listener.py | 49 +++++++------
 pydis_site/apps/home/tests/test_views.py           |  3 +
 pydis_site/apps/home/urls.py                       |  2 +-
 pydis_site/settings.py                             | 16 ++---
 6 files changed, 119 insertions(+), 39 deletions(-)

(limited to 'pydis_site/apps/home/signals.py')

diff --git a/pydis_site/apps/home/apps.py b/pydis_site/apps/home/apps.py
index a7c47dc5..55a393a9 100644
--- a/pydis_site/apps/home/apps.py
+++ b/pydis_site/apps/home/apps.py
@@ -11,9 +11,9 @@ class HomeConfig(AppConfig):
 
     def ready(self) -> None:
         """Run when the app has been loaded and is ready to serve requests."""
-        from pydis_site.apps.home.signals import SignalListener
+        from pydis_site.apps.home.signals import AllauthSignalListener
 
-        self.signal_listener = SignalListener()
+        self.signal_listener = AllauthSignalListener()
         self.patch_allauth()
 
     def patch_allauth(self) -> None:
diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py
index 65b24ddf..9f286882 100644
--- a/pydis_site/apps/home/signals.py
+++ b/pydis_site/apps/home/signals.py
@@ -14,7 +14,7 @@ from pydis_site.apps.api.models import User as DiscordUser
 from pydis_site.apps.staff.models import RoleMapping
 
 
-class SignalListener:
+class AllauthSignalListener:
     """
     Listens to and processes events via the Django Signals system.
 
@@ -48,7 +48,16 @@ class SignalListener:
         user_logged_in.connect(self.user_logged_in)
 
     def user_logged_in(self, sender: Type[DjangoUser], **kwargs) -> None:
-        """Handles signals relating to Allauth logins."""
+        """
+        Processes Allauth login signals to ensure a user has the correct perms.
+
+        This method tries to find a Discord SocialAccount for a user - this should always
+        be the case, but the admin user likely won't have one, so we do check for it.
+
+        After that, we try to find the user's stored Discord account details, provided by the
+        bot on the server. Finally, we pass the relevant information over to the
+        `_apply_groups()` method for final processing.
+        """
         user: DjangoUser = kwargs["user"]
 
         try:
@@ -66,7 +75,16 @@ class SignalListener:
         self._apply_groups(discord_user, account)
 
     def social_account_updated(self, sender: Type[SocialLogin], **kwargs) -> None:
-        """Handle signals relating to new/existing social accounts."""
+        """
+        Processes Allauth social account update signals to ensure a user has the correct perms.
+
+        In this case, a SocialLogin is provided that we can check against. We check that this
+        is a Discord login in order to ensure that future OAuth logins using other providers
+        don't break things.
+
+        Like most of the other methods that handle signals, this method defers to the
+        `_apply_groups()` method for final processing.
+        """
         social_login: SocialLogin = kwargs["sociallogin"]
 
         account: SocialAccount = social_login.account
@@ -83,7 +101,20 @@ class SignalListener:
         self._apply_groups(user, account)
 
     def social_account_removed(self, sender: Type[SocialLogin], **kwargs) -> None:
-        """Handle signals relating to removal of social accounts."""
+        """
+        Processes Allauth social account reomval signals to ensure a user has the correct perms.
+
+        In this case, a SocialAccount is provided that we can check against. If this is a
+        Discord OAuth being removed from the account, we want to ensure that the user loses
+        their permissions groups as well.
+
+        While this isn't a realistic scenario to reach in our current setup, I've provided it
+        for the sake of covering any edge cases and ensuring that SocialAccounts can be removed
+        from Django users in the future if required.
+
+        Like most of the other methods that handle signals, this method defers to the
+        `_apply_groups()` method for final processing.
+        """
         account: SocialAccount = kwargs["socialaccount"]
         provider: Provider = account.get_provider()
 
@@ -98,14 +129,25 @@ class SignalListener:
         self._apply_groups(user, account, deletion=True)
 
     def mapping_model_deleted(self, sender: Type[RoleMapping], **kwargs) -> None:
-        """Handle signals related to the deletion of Role Mapping model entries."""
+        """
+        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.
+        """
         instance: RoleMapping = kwargs["instance"]
 
         for user in instance.group.user_set.all():
             user.groups.remove(instance.group)
 
     def mapping_model_updated(self, sender: Type[RoleMapping], **kwargs) -> None:
-        """Handle signals related to the updating of Role Mapping model entries."""
+        """
+        Processes update signals from the RoleMapping model.
+
+        This method is in charge of figuring out what changed when a RoleMapping is updated
+        (via the Django admin or otherwise). It operates based on what was changed, and can
+        handle changes to both the role and permissions group assigned to it.
+        """
         instance: RoleMapping = kwargs["instance"]
         raw: bool = kwargs["raw"]
 
@@ -117,7 +159,8 @@ class SignalListener:
 
         if instance.id is not None:
             # We don't try to catch DoesNotExist here because we can't test for it,
-            # it should never happen (unless we have a bad DB failure)
+            # it should never happen (unless we have a bad DB failure) but I'm still
+            # kind of antsy about not having the extra security here.
 
             old_instance = RoleMapping.objects.get(id=instance.id)
 
@@ -132,7 +175,17 @@ class SignalListener:
             account.user.groups.add(instance.group)
 
     def user_model_updated(self, sender: Type[DiscordUser], **kwargs) -> None:
-        """Handle signals related to the updating of Discord User model entries."""
+        """
+        Processes update signals from the Discord User model, assigning perms as required.
+
+        When a user's roles are changed on the Discord server, this method will ensure that
+        the user has only the permissions groups that they should have based on the RoleMappings
+        that have been set up in the Django admin.
+
+        Like some of the other signal handlers, this method ensures that a SocialAccount exists
+        for this Discord User, and defers to `_apply_groups()` to do the heavy lifting of
+        ensuring the permissions groups are correct.
+        """
         instance: DiscordUser = kwargs["instance"]
         raw: bool = kwargs["raw"]
 
@@ -155,6 +208,21 @@ class SignalListener:
     def _apply_groups(
             self, user: DiscordUser, account: SocialAccount, deletion: bool = False
     ) -> None:
+        """
+        Ensures that the correct permissions are set for a Django user based on the RoleMappings.
+
+        This (private) method is designed to check a Discord User against a given SocialAccount,
+        and makes sure that the Django user associated with the SocialAccount has the correct
+        permissions groups.
+
+        While it would be possible to get the Discord User object with just the SocialAccount
+        object, the current approach results in less queries.
+
+        The `deletion` parameter is used to signify that the user's SocialAccount is about
+        to be removed, and so we should always remove all of their permissions groups. The
+        same thing will happen if the user is no longer actually on the Discord server, as
+        leaving the server does not currently remove their SocialAccount from the database.
+        """
         mappings = RoleMapping.objects.all()
 
         try:
diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py
index ee78999f..27fc7710 100644
--- a/pydis_site/apps/home/tests/test_signal_listener.py
+++ b/pydis_site/apps/home/tests/test_signal_listener.py
@@ -13,14 +13,23 @@ from django.db.models.signals import post_save, pre_save
 from django.test import TestCase
 
 from pydis_site.apps.api.models import Role, User as DiscordUser
-from pydis_site.apps.home.signals import SignalListener
+from pydis_site.apps.home.signals import AllauthSignalListener
 from pydis_site.apps.staff.models import RoleMapping
 
 
 class SignalListenerTests(TestCase):
     @classmethod
     def setUpTestData(cls):
-        """Executed when testing begins."""
+        """
+        Executed when testing begins in order to set up database fixtures required for testing.
+
+        This sets up quite a lot of stuff, in order to try to cover every eventuality while
+        ensuring that everything works when every possible situation is in the database
+        at the same time.
+
+        That does unfortunately mean that half of this file is just test fixtures, but I couldn't
+        think of a better way to do this.
+        """
         # This needs to be registered so we can test the role linking logic with a user that
         # doesn't have a Discord account linked, but is logged in somehow with another account
         # type anyway. The logic this is testing was designed so that the system would be
@@ -171,15 +180,15 @@ class SignalListenerTests(TestCase):
         """Test signal handling for when Discord user model objects are saved to DB."""
         mock_obj = mock.Mock()
 
-        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
-            _ = SignalListener()
+        with mock.patch.object(AllauthSignalListener, "_apply_groups", mock_obj):
+            AllauthSignalListener()
 
             post_save.send(
                 DiscordUser,
                 instance=self.discord_user,
                 raw=True,
-                created=None,  # Not realistic, but we don't use it
-                using=None,  # Again, we don't use it
+                created=None,        # Not realistic, but we don't use it
+                using=None,          # Again, we don't use it
                 update_fields=False  # Always false during integration testing
             )
 
@@ -189,8 +198,8 @@ class SignalListenerTests(TestCase):
                 DiscordUser,
                 instance=self.discord_user,
                 raw=False,
-                created=None,  # Not realistic, but we don't use it
-                using=None,  # Again, we don't use it
+                created=None,        # Not realistic, but we don't use it
+                using=None,          # Again, we don't use it
                 update_fields=False  # Always false during integration testing
             )
 
@@ -204,8 +213,8 @@ class SignalListenerTests(TestCase):
         github_login = SocialLogin(self.django_user, self.social_user_github)
         unmapped_login = SocialLogin(self.django_user, self.social_unmapped)
 
-        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
-            _ = SignalListener()
+        with mock.patch.object(AllauthSignalListener, "_apply_groups", mock_obj):
+            AllauthSignalListener()
 
             # Don't attempt to apply groups if the user doesn't have a linked Discord account
             pre_social_login.send(SocialLogin, sociallogin=github_login)
@@ -227,8 +236,8 @@ class SignalListenerTests(TestCase):
         github_login = SocialLogin(self.django_user, self.social_user_github)
         unmapped_login = SocialLogin(self.django_user, self.social_unmapped)
 
-        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
-            _ = SignalListener()
+        with mock.patch.object(AllauthSignalListener, "_apply_groups", mock_obj):
+            AllauthSignalListener()
 
             # Don't attempt to apply groups if the user doesn't have a linked Discord account
             social_account_added.send(SocialLogin, sociallogin=github_login)
@@ -250,8 +259,8 @@ class SignalListenerTests(TestCase):
         github_login = SocialLogin(self.django_user, self.social_user_github)
         unmapped_login = SocialLogin(self.django_user, self.social_unmapped)
 
-        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
-            _ = SignalListener()
+        with mock.patch.object(AllauthSignalListener, "_apply_groups", mock_obj):
+            AllauthSignalListener()
 
             # Don't attempt to apply groups if the user doesn't have a linked Discord account
             social_account_updated.send(SocialLogin, sociallogin=github_login)
@@ -269,8 +278,8 @@ class SignalListenerTests(TestCase):
         """Test the social-account-removed Allauth signal handling."""
         mock_obj = mock.Mock()
 
-        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
-            _ = SignalListener()
+        with mock.patch.object(AllauthSignalListener, "_apply_groups", mock_obj):
+            AllauthSignalListener()
 
             # Don't attempt to remove groups if the user doesn't have a linked Discord account
             social_account_removed.send(SocialLogin, socialaccount=self.social_user_github)
@@ -288,8 +297,8 @@ class SignalListenerTests(TestCase):
         """Test the user-logged-in Allauth signal handling."""
         mock_obj = mock.Mock()
 
-        with mock.patch.object(SignalListener, "_apply_groups", mock_obj):
-            _ = SignalListener()
+        with mock.patch.object(AllauthSignalListener, "_apply_groups", mock_obj):
+            AllauthSignalListener()
 
             # Don't attempt to apply groups if the user doesn't have a linked Discord account
             user_logged_in.send(DjangoUser, user=self.django_user_discordless)
@@ -305,7 +314,7 @@ class SignalListenerTests(TestCase):
 
     def test_apply_groups_admin(self):
         """Test application of groups by role, relating to an admin user."""
-        handler = SignalListener()
+        handler = AllauthSignalListener()
 
         self.assertEqual(self.django_user_discordless.groups.all().count(), 0)
 
@@ -332,7 +341,7 @@ class SignalListenerTests(TestCase):
 
     def test_apply_groups_other(self):
         """Test application of groups by role, relating to non-standard cases."""
-        handler = SignalListener()
+        handler = AllauthSignalListener()
 
         self.assertEqual(self.django_user_discordless.groups.all().count(), 0)
 
diff --git a/pydis_site/apps/home/tests/test_views.py b/pydis_site/apps/home/tests/test_views.py
index aa434605..7aeaddd2 100644
--- a/pydis_site/apps/home/tests/test_views.py
+++ b/pydis_site/apps/home/tests/test_views.py
@@ -4,6 +4,7 @@ from django_hosts.resolvers import reverse
 
 class TestIndexReturns200(TestCase):
     def test_index_returns_200(self):
+        """Check that the index page returns a HTTP 200 response."""
         url = reverse('home')
         resp = self.client.get(url)
         self.assertEqual(resp.status_code, 200)
@@ -11,6 +12,7 @@ class TestIndexReturns200(TestCase):
 
 class TestLoginCancelledReturns302(TestCase):
     def test_login_cancelled_returns_302(self):
+        """Check that the login cancelled redirect returns a HTTP 302 response."""
         url = reverse('socialaccount_login_cancelled')
         resp = self.client.get(url)
         self.assertEqual(resp.status_code, 302)
@@ -18,6 +20,7 @@ class TestLoginCancelledReturns302(TestCase):
 
 class TestLoginErrorReturns302(TestCase):
     def test_login_error_returns_302(self):
+        """Check that the login error redirect returns a HTTP 302 response."""
         url = reverse('socialaccount_login_error')
         resp = self.client.get(url)
         self.assertEqual(resp.status_code, 302)
diff --git a/pydis_site/apps/home/urls.py b/pydis_site/apps/home/urls.py
index 8428327f..211a7ad1 100644
--- a/pydis_site/apps/home/urls.py
+++ b/pydis_site/apps/home/urls.py
@@ -23,7 +23,7 @@ urlpatterns = [
     ),
     path(
         'accounts/login/error', MessageRedirectView.as_view(
-            pattern_name="home", message="Login failed due to an error, please try again.",
+            pattern_name="home", message="Login encountered an unknown error, please try again.",
             message_level=ERROR
         ), name='socialaccount_login_error'
     ),
diff --git a/pydis_site/settings.py b/pydis_site/settings.py
index 21d6b427..9c88a056 100644
--- a/pydis_site/settings.py
+++ b/pydis_site/settings.py
@@ -295,16 +295,16 @@ CRISPY_TEMPLATE_PACK = "bulma"
 # Custom settings for django-simple-bulma
 BULMA_SETTINGS = {
     "variables": {  # If you update these colours, please update the notification.css file
-        "primary": "#7289DA",  # Discord blurple
+        "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
+        "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
+        "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
 
         "link": "$primary",
 
-- 
cgit v1.2.3