From ea149e7ee58b8889d71b6f4153530a1347465f2c Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Sun, 20 Oct 2019 13:28:30 +0100 Subject: Signals: Move group changes outside of the loop --- pydis_site/apps/home/signals.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 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 9f286882..4bf50b78 100644 --- a/pydis_site/apps/home/signals.py +++ b/pydis_site/apps/home/signals.py @@ -251,10 +251,10 @@ class AllauthSignalListener: except RoleMapping.DoesNotExist: continue # No mapping exists - account.user.groups.add( - *[group for group in new_groups if group not in current_groups] - ) + account.user.groups.add( + *[group for group in new_groups if group not in current_groups] + ) - account.user.groups.remove( - *[mapping.group for mapping in mappings if mapping.group not in new_groups] - ) + account.user.groups.remove( + *[mapping.group for mapping in mappings if mapping.group not in new_groups] + ) -- cgit v1.2.3 From fa27a54e34b6912e671254a3103495aa9b765b7e Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Sun, 20 Oct 2019 16:09:56 +0100 Subject: Add is_staff to role mappings, and the logic to go with it --- pydis_site/apps/home/signals.py | 69 +++++++++++++++++++++++----- pydis_site/apps/staff/models/role_mapping.py | 5 ++ 2 files changed, 63 insertions(+), 11 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 4bf50b78..76d29631 100644 --- a/pydis_site/apps/home/signals.py +++ b/pydis_site/apps/home/signals.py @@ -1,3 +1,4 @@ +from contextlib import suppress from typing import List, Optional, Type from allauth.account.signals import user_logged_in @@ -8,7 +9,7 @@ from allauth.socialaccount.signals import ( pre_social_login, social_account_added, social_account_removed, social_account_updated) from django.contrib.auth.models import Group, User as DjangoUser -from django.db.models.signals import post_save, pre_delete, pre_save +from django.db.models.signals import post_delete, post_save, pre_save from pydis_site.apps.api.models import User as DiscordUser from pydis_site.apps.staff.models import RoleMapping @@ -37,7 +38,7 @@ class AllauthSignalListener: def __init__(self): post_save.connect(self.user_model_updated, sender=DiscordUser) - pre_delete.connect(self.mapping_model_deleted, sender=RoleMapping) + post_delete.connect(self.mapping_model_deleted, sender=RoleMapping) pre_save.connect(self.mapping_model_updated, sender=RoleMapping) pre_social_login.connect(self.social_account_updated) @@ -133,13 +134,29 @@ class AllauthSignalListener: Processes deletion signals from the RoleMapping model, removing perms from users. We need to do this to ensure that users aren't left with permissions groups that - they shouldn't have assigned to them when a RoleMapping is deleted from the database. + they shouldn't have assigned to them when a RoleMapping is deleted from the database, + and to remove their staff status if they should no longer have it. """ instance: RoleMapping = kwargs["instance"] for user in instance.group.user_set.all(): + # Firstly, remove their related user group user.groups.remove(instance.group) + with suppress(SocialAccount.DoesNotExist, DiscordUser.DoesNotExist): + # If we get either exception, then the user could not have been assigned staff + # with our system in the first place. + + social_account = SocialAccount.objects.get(user=user, provider=DiscordProvider.id) + discord_user = DiscordUser.objects.get(id=int(social_account.uid)) + + mappings = RoleMapping.objects.filter(role__in=discord_user.roles.all()).all() + is_staff = any(m.is_staff for m in mappings) + + if user.is_staff != is_staff: + user.is_staff = is_staff + user.save(update_fields=("is_staff", )) + def mapping_model_updated(self, sender: Type[RoleMapping], **kwargs) -> None: """ Processes update signals from the RoleMapping model. @@ -174,6 +191,21 @@ class AllauthSignalListener: for account in accounts: account.user.groups.add(instance.group) + if instance.is_staff and not account.user.is_staff: + account.user.is_staff = True + account.user.save(update_fields=("is_staff", )) + else: + discord_user = DiscordUser.objects.get(id=int(account.uid)) + + mappings = RoleMapping.objects.filter( + role__in=discord_user.roles.all() + ).exclude(id=instance.id).all() + is_staff = any(m.is_staff for m in mappings) + + if account.user.is_staff != is_staff: + account.user.is_staff = is_staff + account.user.save(update_fields=("is_staff",)) + def user_model_updated(self, sender: Type[DiscordUser], **kwargs) -> None: """ Processes update signals from the Discord User model, assigning perms as required. @@ -235,22 +267,33 @@ class AllauthSignalListener: 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) - ) + # and their staff status + + if current_groups: + # They do have groups, so let's remove them + account.user.groups.remove( + *(mapping.group for mapping in mappings) + ) + + if account.user.is_staff: + # They're marked as a staff user and they shouldn't be, so let's fix that + account.user.is_staff = False + account.user.save(update_fields=("is_staff", )) else: new_groups = [] + is_staff = False for role in user.roles.all(): try: - new_groups.append(mappings.get(role=role).group) + mapping = mappings.get(role=role) except RoleMapping.DoesNotExist: continue # No mapping exists + new_groups.append(mapping.group) + + if mapping.is_staff: + is_staff = True + account.user.groups.add( *[group for group in new_groups if group not in current_groups] ) @@ -258,3 +301,7 @@ class AllauthSignalListener: account.user.groups.remove( *[mapping.group for mapping in mappings if mapping.group not in new_groups] ) + + if account.user.is_staff != is_staff: + account.user.is_staff = is_staff + account.user.save(update_fields=("is_staff", )) diff --git a/pydis_site/apps/staff/models/role_mapping.py b/pydis_site/apps/staff/models/role_mapping.py index 10c09cf1..dff8081a 100644 --- a/pydis_site/apps/staff/models/role_mapping.py +++ b/pydis_site/apps/staff/models/role_mapping.py @@ -21,6 +21,11 @@ class RoleMapping(models.Model): unique=True, # Unique in order to simplify group assignment logic ) + is_staff = models.BooleanField( + help_text="Whether this role mapping related to a Django staff group", + default=False + ) + def __str__(self): """Returns the mapping, for display purposes.""" return f"@{self.role.name} -> {self.group.name}" -- cgit v1.2.3 From b9e759e11c53cf5de0e98efe810a9020a8d1d3cc Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Sun, 20 Oct 2019 16:29:03 +0100 Subject: Tests for the new signal listener is_staff changes --- pydis_site/apps/home/signals.py | 2 +- pydis_site/apps/home/tests/test_signal_listener.py | 29 +++++++++++++++++++--- 2 files changed, 26 insertions(+), 5 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 76d29631..43e861d2 100644 --- a/pydis_site/apps/home/signals.py +++ b/pydis_site/apps/home/signals.py @@ -192,7 +192,7 @@ class AllauthSignalListener: account.user.groups.add(instance.group) if instance.is_staff and not account.user.is_staff: - account.user.is_staff = True + account.user.is_staff = instance.is_staff account.user.save(update_fields=("is_staff", )) else: discord_user = DiscordUser.objects.get(id=int(account.uid)) diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py index 27fc7710..163ad6af 100644 --- a/pydis_site/apps/home/tests/test_signal_listener.py +++ b/pydis_site/apps/home/tests/test_signal_listener.py @@ -67,12 +67,14 @@ class SignalListenerTests(TestCase): cls.admin_mapping = RoleMapping.objects.create( role=cls.admin_role, - group=cls.admin_group + group=cls.admin_group, + is_staff=True ) cls.moderator_mapping = RoleMapping.objects.create( role=cls.moderator_role, - group=cls.moderator_group + group=cls.moderator_group, + is_staff=False ) cls.discord_user = DiscordUser.objects.create( @@ -166,7 +168,7 @@ class SignalListenerTests(TestCase): cls.django_moderator = DjangoUser.objects.create( username="moderator", - is_staff=True, + is_staff=False, is_superuser=False ) @@ -373,10 +375,25 @@ class SignalListenerTests(TestCase): self.assertEqual(self.django_moderator.groups.all().count(), 1) self.assertEqual(self.django_admin.groups.all().count(), 1) + # Test is_staff changes + self.admin_mapping.is_staff = False + self.admin_mapping.save() + + self.assertFalse(self.django_moderator.is_staff) + self.assertFalse(self.django_admin.is_staff) + + self.admin_mapping.is_staff = True + self.admin_mapping.save() + + self.django_admin.refresh_from_db(fields=("is_staff", )) + self.assertTrue(self.django_admin.is_staff) + # Test mapping deletion self.admin_mapping.delete() + self.django_admin.refresh_from_db(fields=("is_staff",)) self.assertEqual(self.django_admin.groups.all().count(), 0) + self.assertFalse(self.django_admin.is_staff) # Test mapping update self.moderator_mapping.group = self.admin_group @@ -388,12 +405,16 @@ class SignalListenerTests(TestCase): # Test mapping creation new_mapping = RoleMapping.objects.create( role=self.admin_role, - group=self.moderator_group + group=self.moderator_group, + is_staff=True ) self.assertEqual(self.django_admin.groups.all().count(), 1) self.assertTrue(self.moderator_group in self.django_admin.groups.all()) + self.django_admin.refresh_from_db(fields=("is_staff",)) + self.assertTrue(self.django_admin.is_staff) + # Test that nothing happens when fixtures are loaded pre_save.send(RoleMapping, instance=new_mapping, raw=True) -- cgit v1.2.3 From 3caa7675f9c5c33b6a8e91c633945d5152383a76 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Sun, 20 Oct 2019 18:05:33 +0100 Subject: Update Django users with Discord username+discrim. This sets both the Django User's `username` and `first_name` params. --- pydis_site/__init__.py | 5 +++++ pydis_site/apps/home/signals.py | 11 +++++++++-- pydis_site/settings.py | 7 ++++++- pydis_site/utils/account.py | 20 ++++++++++++++++++++ 4 files changed, 40 insertions(+), 3 deletions(-) (limited to 'pydis_site/apps/home/signals.py') diff --git a/pydis_site/__init__.py b/pydis_site/__init__.py index c6146450..df67cf71 100644 --- a/pydis_site/__init__.py +++ b/pydis_site/__init__.py @@ -2,3 +2,8 @@ from wiki.plugins.macros.mdx import toc # Remove the toc header prefix. There's no option for this, so we gotta monkey patch it. toc.HEADER_ID_PREFIX = '' + +# Empty list of validators for Allauth to ponder over. This is referred to in settings.py +# by a string because Allauth won't let us just give it a list _there_, we have to point +# at a list _somewhere else_ instead. +VALIDATORS = [] diff --git a/pydis_site/apps/home/signals.py b/pydis_site/apps/home/signals.py index 43e861d2..4cb4564b 100644 --- a/pydis_site/apps/home/signals.py +++ b/pydis_site/apps/home/signals.py @@ -262,6 +262,13 @@ class AllauthSignalListener: except SocialAccount.user.RelatedObjectDoesNotExist: return # There's no user account yet, this will be handled by another receiver + # Ensure that the username on this account is correct + new_username = f"{user.name}#{user.discriminator}" + + if account.user.username != new_username: + account.user.username = new_username + account.user.first_name = new_username + if not user.in_guild: deletion = True @@ -278,7 +285,6 @@ class AllauthSignalListener: if account.user.is_staff: # They're marked as a staff user and they shouldn't be, so let's fix that account.user.is_staff = False - account.user.save(update_fields=("is_staff", )) else: new_groups = [] is_staff = False @@ -304,4 +310,5 @@ class AllauthSignalListener: if account.user.is_staff != is_staff: account.user.is_staff = is_staff - account.user.save(update_fields=("is_staff", )) + + account.user.save() diff --git a/pydis_site/settings.py b/pydis_site/settings.py index d6ca4860..0d893b2c 100644 --- a/pydis_site/settings.py +++ b/pydis_site/settings.py @@ -409,7 +409,12 @@ AUTHENTICATION_BACKENDS = ( ) ACCOUNT_ADAPTER = "pydis_site.utils.account.AccountAdapter" -ACCOUNT_EMAIL_REQUIRED = False # Undocumented allauth setting; don't require emails +ACCOUNT_EMAIL_REQUIRED = False # Undocumented allauth setting; don't require emails ACCOUNT_EMAIL_VERIFICATION = "none" # No verification required; we don't use emails for anything + +# We use this validator because Allauth won't let us actually supply a list with no validators +# in it, and we can't just give it a lambda - that'd be too easy, I suppose. +ACCOUNT_USERNAME_VALIDATORS = "pydis_site.VALIDATORS" + LOGIN_REDIRECT_URL = "home" SOCIALACCOUNT_ADAPTER = "pydis_site.utils.account.SocialAccountAdapter" diff --git a/pydis_site/utils/account.py b/pydis_site/utils/account.py index adafcea9..9faad986 100644 --- a/pydis_site/utils/account.py +++ b/pydis_site/utils/account.py @@ -1,7 +1,10 @@ +from typing import Any, Dict + from allauth.account.adapter import DefaultAccountAdapter from allauth.exceptions import ImmediateHttpResponse from allauth.socialaccount.adapter import DefaultSocialAccountAdapter from allauth.socialaccount.models import SocialLogin +from django.contrib.auth.models import User from django.contrib.messages import ERROR, add_message from django.http import HttpRequest from django.shortcuts import redirect @@ -42,3 +45,20 @@ class SocialAccountAdapter(DefaultSocialAccountAdapter): raise ImmediateHttpResponse(redirect(reverse("home"))) return True + + def populate_user(self, request: HttpRequest, + social_login: SocialLogin, + data: Dict[str, Any]) -> User: + """ + Method used to populate a Django User with data. + + We override this so that the Django user is created with the username#discriminator, + instead of just the username, as Django users must have unique usernames. For display + purposes, we also set the `name` key, which is used for `first_name` in the database. + """ + if social_login.account.provider == "discord": + discriminator = social_login.account.extra_data["discriminator"] + data["username"] = f"{data['username']}#{discriminator}" + data["name"] = data["username"] + + return super().populate_user(request, social_login, data) -- cgit v1.2.3