Allauth: Address reviews by @lemonsaurus

1 files changed, 76 insertions, 8 deletions

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: