diff options
| -rw-r--r-- | pydis_site/apps/home/apps.py | 4 | ||||
| -rw-r--r-- | pydis_site/apps/home/signals.py | 84 | ||||
| -rw-r--r-- | pydis_site/apps/home/tests/test_signal_listener.py | 49 | ||||
| -rw-r--r-- | pydis_site/apps/home/tests/test_views.py | 3 | ||||
| -rw-r--r-- | pydis_site/apps/home/urls.py | 2 | ||||
| -rw-r--r-- | pydis_site/settings.py | 16 |
6 files changed, 119 insertions, 39 deletions
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", |