aboutsummaryrefslogtreecommitdiffstats
path: root/pydis_site/apps
diff options
context:
space:
mode:
authorGravatar Gareth Coles <[email protected]>2019-10-14 11:23:50 +0100
committerGravatar Gareth Coles <[email protected]>2019-10-14 11:23:50 +0100
commit2055e0d54ca743557e6209e4b3e3f85e15c59fe2 (patch)
treebd6395916e48eafd71e6cf91536f9551f5025072 /pydis_site/apps
parentMerge branch 'master' into #201-django-allauth (diff)
Allauth: Address reviews by @lemonsaurus
Diffstat (limited to 'pydis_site/apps')
-rw-r--r--pydis_site/apps/home/apps.py4
-rw-r--r--pydis_site/apps/home/signals.py84
-rw-r--r--pydis_site/apps/home/tests/test_signal_listener.py49
-rw-r--r--pydis_site/apps/home/tests/test_views.py3
-rw-r--r--pydis_site/apps/home/urls.py2
5 files changed, 111 insertions, 31 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'
),