From 3512a71bfe89a8efdf8be7ac917cbe13dfe132e2 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Sun, 20 Oct 2019 13:27:44 +0100 Subject: GH signup prevention, views and templates for settings pages --- pydis_site/apps/home/urls.py | 8 +++++--- pydis_site/apps/home/views/__init__.py | 3 ++- pydis_site/apps/home/views/account/__init__.py | 4 ++++ pydis_site/apps/home/views/account/delete.py | 20 ++++++++++++++++++++ pydis_site/apps/home/views/account/settings.py | 20 ++++++++++++++++++++ 5 files changed, 51 insertions(+), 4 deletions(-) create mode 100644 pydis_site/apps/home/views/account/__init__.py create mode 100644 pydis_site/apps/home/views/account/delete.py create mode 100644 pydis_site/apps/home/views/account/settings.py (limited to 'pydis_site/apps') diff --git a/pydis_site/apps/home/urls.py b/pydis_site/apps/home/urls.py index 211a7ad1..70a41177 100644 --- a/pydis_site/apps/home/urls.py +++ b/pydis_site/apps/home/urls.py @@ -1,5 +1,4 @@ from allauth.account.views import LogoutView -from allauth.socialaccount.views import ConnectionsView from django.conf import settings from django.conf.urls.static import static from django.contrib import admin @@ -7,7 +6,7 @@ from django.contrib.messages import ERROR from django.urls import include, path from pydis_site.utils.views import MessageRedirectView -from .views import HomeView +from .views import AccountDeleteView, AccountSettingsView, HomeView app_name = 'home' urlpatterns = [ @@ -15,6 +14,7 @@ urlpatterns = [ path('pages/', include('wiki.urls')), path('accounts/', include('allauth.socialaccount.providers.discord.urls')), + path('accounts/', include('allauth.socialaccount.providers.github.urls')), path( 'accounts/login/cancelled', MessageRedirectView.as_view( @@ -28,7 +28,9 @@ urlpatterns = [ ), name='socialaccount_login_error' ), - path('connections', ConnectionsView.as_view()), + path('accounts/settings', AccountSettingsView.as_view(), name="account_settings"), + path('accounts/delete', AccountDeleteView.as_view(), name="account_delete"), + path('logout', LogoutView.as_view(), name="logout"), path('admin/', admin.site.urls), diff --git a/pydis_site/apps/home/views/__init__.py b/pydis_site/apps/home/views/__init__.py index 971d73a3..801fd398 100644 --- a/pydis_site/apps/home/views/__init__.py +++ b/pydis_site/apps/home/views/__init__.py @@ -1,3 +1,4 @@ +from .account import DeleteView as AccountDeleteView, SettingsView as AccountSettingsView from .home import HomeView -__all__ = ["HomeView"] +__all__ = ["AccountDeleteView", "AccountSettingsView", "HomeView"] diff --git a/pydis_site/apps/home/views/account/__init__.py b/pydis_site/apps/home/views/account/__init__.py new file mode 100644 index 00000000..3b3250ea --- /dev/null +++ b/pydis_site/apps/home/views/account/__init__.py @@ -0,0 +1,4 @@ +from .delete import DeleteView +from .settings import SettingsView + +__all__ = ["DeleteView", "SettingsView"] diff --git a/pydis_site/apps/home/views/account/delete.py b/pydis_site/apps/home/views/account/delete.py new file mode 100644 index 00000000..f80089d5 --- /dev/null +++ b/pydis_site/apps/home/views/account/delete.py @@ -0,0 +1,20 @@ +from django.contrib.auth.mixins import LoginRequiredMixin +from django.http import HttpRequest, HttpResponse +from django.shortcuts import render +from django.urls import reverse +from django.views import View + + +class DeleteView(LoginRequiredMixin, View): + """Account deletion view, for removing linked user accounts from the DB.""" + + def __init__(self, *args, **kwargs): + self.login_url = reverse("home") + super().__init__(*args, **kwargs) + + def get(self, request: HttpRequest) -> HttpResponse: + """HTTP GET: Return the view template.""" + return render(request, "home/account/delete.html") + + def post(self, request: HttpRequest) -> HttpResponse: + """HTTP POST: Process the deletion, as requested by the user.""" diff --git a/pydis_site/apps/home/views/account/settings.py b/pydis_site/apps/home/views/account/settings.py new file mode 100644 index 00000000..aa272552 --- /dev/null +++ b/pydis_site/apps/home/views/account/settings.py @@ -0,0 +1,20 @@ +from django.contrib.auth.mixins import LoginRequiredMixin +from django.http import HttpRequest, HttpResponse +from django.shortcuts import render +from django.urls import reverse +from django.views import View + + +class SettingsView(LoginRequiredMixin, View): + """Account settings view, for managing and deleting user accounts and connections.""" + + def __init__(self, *args, **kwargs): + self.login_url = reverse("home") + super().__init__(*args, **kwargs) + + def get(self, request: HttpRequest) -> HttpResponse: + """HTTP GET: Return the view template.""" + return render(request, "home/account/settings.html") + + def post(self, request: HttpRequest) -> HttpResponse: + """HTTP POST: Process account changes, as requested by the user.""" -- cgit v1.2.3 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') 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') 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') 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 701711594686d8bda3e2786649b5f798eccbf935 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Sun, 20 Oct 2019 16:51:10 +0100 Subject: Signals: Complete coverage in tests --- pydis_site/apps/home/tests/test_signal_listener.py | 41 ++++++++++++++++++++++ 1 file changed, 41 insertions(+) (limited to 'pydis_site/apps') diff --git a/pydis_site/apps/home/tests/test_signal_listener.py b/pydis_site/apps/home/tests/test_signal_listener.py index 163ad6af..66a67252 100644 --- a/pydis_site/apps/home/tests/test_signal_listener.py +++ b/pydis_site/apps/home/tests/test_signal_listener.py @@ -341,6 +341,33 @@ class SignalListenerTests(TestCase): self.discord_admin.roles.add(self.admin_role) self.discord_admin.save() + def test_apply_groups_moderator(self): + """Test application of groups by role, relating to a non-`is_staff` moderator user.""" + handler = AllauthSignalListener() + + self.assertEqual(self.django_user_discordless.groups.all().count(), 0) + + # Apply groups based on moderator role being present on Discord + handler._apply_groups(self.discord_moderator, self.social_moderator) + self.assertTrue(self.moderator_group in self.django_moderator.groups.all()) + + # Remove groups based on the user apparently leaving the server + handler._apply_groups(self.discord_moderator, self.social_moderator, True) + self.assertEqual(self.django_user_discordless.groups.all().count(), 0) + + # Apply the moderator role again + handler._apply_groups(self.discord_moderator, self.social_moderator) + + # Remove all of the roles from the user + self.discord_moderator.roles.clear() + + # Remove groups based on the user no longer having the moderator role on Discord + handler._apply_groups(self.discord_moderator, self.social_moderator) + self.assertEqual(self.django_user_discordless.groups.all().count(), 0) + + self.discord_moderator.roles.add(self.moderator_role) + self.discord_moderator.save() + def test_apply_groups_other(self): """Test application of groups by role, relating to non-standard cases.""" handler = AllauthSignalListener() @@ -415,6 +442,20 @@ class SignalListenerTests(TestCase): self.django_admin.refresh_from_db(fields=("is_staff",)) self.assertTrue(self.django_admin.is_staff) + new_mapping.delete() + + # Test mapping creation (without is_staff) + 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()) + + self.django_admin.refresh_from_db(fields=("is_staff",)) + self.assertFalse(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') 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 From 6cc062d88e7b70a61d52e6eab633de57c4114cb2 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Tue, 22 Oct 2019 16:00:16 +0100 Subject: Implement account deletion at /accounts/delete --- pydis_site/apps/home/forms/__init__.py | 0 pydis_site/apps/home/forms/account_deletion.py | 24 ++++++++++++++++++ pydis_site/apps/home/views/account/delete.py | 21 ++++++++++++++-- pydis_site/templates/home/account/delete.html | 34 +++++++++++++++++++++++++- 4 files changed, 76 insertions(+), 3 deletions(-) create mode 100644 pydis_site/apps/home/forms/__init__.py create mode 100644 pydis_site/apps/home/forms/account_deletion.py (limited to 'pydis_site/apps') diff --git a/pydis_site/apps/home/forms/__init__.py b/pydis_site/apps/home/forms/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/pydis_site/apps/home/forms/account_deletion.py b/pydis_site/apps/home/forms/account_deletion.py new file mode 100644 index 00000000..17ffe5c1 --- /dev/null +++ b/pydis_site/apps/home/forms/account_deletion.py @@ -0,0 +1,24 @@ +from crispy_forms.helper import FormHelper +from crispy_forms.layout import Layout +from django.forms import CharField, Form +from django_crispy_bulma.layout import IconField, Submit + + +class AccountDeletionForm(Form): + """Account deletion form, to collect username for confirmation of removal.""" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.helper = FormHelper() + + self.helper.form_method = "post" + self.helper.add_input(Submit("submit", "I understand, delete my account")) + + self.helper.layout = Layout( + IconField("username", icon_prepend="user") + ) + + username = CharField( + label="Username", + required=True + ) diff --git a/pydis_site/apps/home/views/account/delete.py b/pydis_site/apps/home/views/account/delete.py index f80089d5..798b8a33 100644 --- a/pydis_site/apps/home/views/account/delete.py +++ b/pydis_site/apps/home/views/account/delete.py @@ -1,9 +1,12 @@ from django.contrib.auth.mixins import LoginRequiredMixin +from django.contrib.messages import ERROR, INFO, add_message from django.http import HttpRequest, HttpResponse -from django.shortcuts import render +from django.shortcuts import redirect, render from django.urls import reverse from django.views import View +from pydis_site.apps.home.forms.account_deletion import AccountDeletionForm + class DeleteView(LoginRequiredMixin, View): """Account deletion view, for removing linked user accounts from the DB.""" @@ -14,7 +17,21 @@ class DeleteView(LoginRequiredMixin, View): def get(self, request: HttpRequest) -> HttpResponse: """HTTP GET: Return the view template.""" - return render(request, "home/account/delete.html") + return render( + request, "home/account/delete.html", + context={"form": AccountDeletionForm()} + ) def post(self, request: HttpRequest) -> HttpResponse: """HTTP POST: Process the deletion, as requested by the user.""" + form = AccountDeletionForm(request.POST) + + if not form.is_valid() or request.user.username != form.cleaned_data["username"]: + add_message(request, ERROR, "Please enter your username exactly as shown.") + + return redirect(reverse("account_delete")) + + request.user.delete() + add_message(request, INFO, "Your account has been deleted.") + + return redirect(reverse("home")) diff --git a/pydis_site/templates/home/account/delete.html b/pydis_site/templates/home/account/delete.html index 8d68a0e3..1020a82b 100644 --- a/pydis_site/templates/home/account/delete.html +++ b/pydis_site/templates/home/account/delete.html @@ -1,4 +1,6 @@ {% extends 'base/base.html' %} + +{% load crispy_forms_tags %} {% load static %} {% block title %}Delete Account{% endblock %} @@ -6,7 +8,37 @@ {% block content %} {% include "base/navbar.html" %} -
+
+
+

Account Deletion

+ +
+
+ +
+
+

+ You have requested to delete the account with username + {{ user.username }}. +

+ +

+ Please note that this cannot be undone. +

+ +

+ To verify that you'd like to remove your account, please type your username into the box below. +

+
+
+
+
+
+
+ {% crispy form %} +
+
+
{% endblock %} -- cgit v1.2.3 From 3892a5fb8e077726ac8fbe59f716b56a501c9ce3 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Tue, 22 Oct 2019 17:54:20 +0100 Subject: Full test coverage, better redirect testing --- pydis_site/apps/home/tests/test_views.py | 111 ++++++++++++++++++++++++- pydis_site/apps/home/views/account/settings.py | 3 - 2 files changed, 110 insertions(+), 4 deletions(-) (limited to 'pydis_site/apps') diff --git a/pydis_site/apps/home/tests/test_views.py b/pydis_site/apps/home/tests/test_views.py index 7aeaddd2..85423c1c 100644 --- a/pydis_site/apps/home/tests/test_views.py +++ b/pydis_site/apps/home/tests/test_views.py @@ -1,5 +1,112 @@ +from django.contrib.auth.models import User +from django.http import HttpResponseRedirect from django.test import TestCase -from django_hosts.resolvers import reverse +from django_hosts.resolvers import get_host, reverse, reverse_host + + +def check_redirect_url( + response: HttpResponseRedirect, reversed_url: str, strip_params=True +) -> bool: + """ + Check whether a given redirect response matches a specific reversed URL. + + Arguments: + * `response`: The HttpResponseRedirect returned by the test client + * `reversed_url`: The URL returned by `reverse()` + * `strip_params`: Whether to strip URL parameters (following a "?") from the URL given in the + `response` object + """ + host = get_host(None) + hostname = reverse_host(host) + + redirect_url = response.url + + if strip_params and "?" in redirect_url: + redirect_url = redirect_url.split("?", 1)[0] + + result = reversed_url == f"//{hostname}{redirect_url}" + return result + + +class TestAccountDeleteView(TestCase): + def setUp(self) -> None: + """Create an authorized Django user for testing purposes.""" + self.user = User.objects.create( + username="user#0000" + ) + + def test_redirect_when_logged_out(self): + """Test that the user is redirected to the homepage when not logged in.""" + url = reverse("account_delete") + resp = self.client.get(url) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + def test_get_when_logged_in(self): + """Test that the view returns a HTTP 200 when the user is logged in.""" + url = reverse("account_delete") + + self.client.force_login(self.user) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + def test_post_invalid(self): + """Test that the user is redirected when the form is filled out incorrectly.""" + url = reverse("account_delete") + + self.client.force_login(self.user) + + resp = self.client.post(url, {}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, url)) + + resp = self.client.post(url, {"username": "user"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, url)) + + self.client.logout() + + def test_post_valid(self): + """Test that the account is deleted when the form is filled out correctly..""" + url = reverse("account_delete") + + self.client.force_login(self.user) + + resp = self.client.post(url, {"username": "user#0000"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + with self.assertRaises(User.DoesNotExist): + User.objects.get(username=self.user.username) + + self.client.logout() + + +class TestAccountSettingsView(TestCase): + def setUp(self) -> None: + """Create an authorized Django user for testing purposes.""" + self.user = User.objects.create( + username="user#0000" + ) + + def test_redirect_when_logged_out(self): + """Check that the user is redirected to the homepage when not logged in.""" + url = reverse("account_settings") + resp = self.client.get(url) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + def test_get_when_logged_in(self): + """Test that the view returns a HTTP 200 when the user is logged in.""" + url = reverse("account_settings") + + self.client.force_login(self.user) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) class TestIndexReturns200(TestCase): @@ -16,6 +123,7 @@ class TestLoginCancelledReturns302(TestCase): url = reverse('socialaccount_login_cancelled') resp = self.client.get(url) self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) class TestLoginErrorReturns302(TestCase): @@ -24,3 +132,4 @@ class TestLoginErrorReturns302(TestCase): url = reverse('socialaccount_login_error') resp = self.client.get(url) self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) diff --git a/pydis_site/apps/home/views/account/settings.py b/pydis_site/apps/home/views/account/settings.py index aa272552..ee9011da 100644 --- a/pydis_site/apps/home/views/account/settings.py +++ b/pydis_site/apps/home/views/account/settings.py @@ -15,6 +15,3 @@ class SettingsView(LoginRequiredMixin, View): def get(self, request: HttpRequest) -> HttpResponse: """HTTP GET: Return the view template.""" return render(request, "home/account/settings.html") - - def post(self, request: HttpRequest) -> HttpResponse: - """HTTP POST: Process account changes, as requested by the user.""" -- cgit v1.2.3 From 2b6513fb6ff455b5d3fccc5786764122759bfae1 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Wed, 23 Oct 2019 01:06:03 +0100 Subject: Add role mapping migration I forgot to commit --- .../migrations/0002_add_is_staff_to_role_mappings.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 pydis_site/apps/staff/migrations/0002_add_is_staff_to_role_mappings.py (limited to 'pydis_site/apps') diff --git a/pydis_site/apps/staff/migrations/0002_add_is_staff_to_role_mappings.py b/pydis_site/apps/staff/migrations/0002_add_is_staff_to_role_mappings.py new file mode 100644 index 00000000..0404d270 --- /dev/null +++ b/pydis_site/apps/staff/migrations/0002_add_is_staff_to_role_mappings.py @@ -0,0 +1,18 @@ +# Generated by Django 2.2.6 on 2019-10-20 14:19 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('staff', '0001_initial'), + ] + + operations = [ + migrations.AddField( + model_name='rolemapping', + name='is_staff', + field=models.BooleanField(default=False, help_text='Whether this role mapping relates to a Django staff group'), + ), + ] -- cgit v1.2.3 From 49376a76289ab22f1aff55d8971b0ea198ec9316 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Wed, 23 Oct 2019 01:07:24 +0100 Subject: Add user settings modal, with connections management and account deletion --- pydis_site/apps/home/urls.py | 3 + pydis_site/apps/home/views/account/settings.py | 48 +++++++- pydis_site/apps/staff/models/role_mapping.py | 2 +- pydis_site/static/css/base/base.css | 10 ++ pydis_site/static/js/base/modal.js | 100 +++++++++++++++++ pydis_site/templates/base/base.html | 1 + pydis_site/templates/base/navbar.html | 18 ++- pydis_site/templates/home/account/settings.html | 141 ++++++++++++++++++++++-- 8 files changed, 310 insertions(+), 13 deletions(-) create mode 100644 pydis_site/static/js/base/modal.js (limited to 'pydis_site/apps') diff --git a/pydis_site/apps/home/urls.py b/pydis_site/apps/home/urls.py index 70a41177..61e87a39 100644 --- a/pydis_site/apps/home/urls.py +++ b/pydis_site/apps/home/urls.py @@ -10,7 +10,10 @@ from .views import AccountDeleteView, AccountSettingsView, HomeView app_name = 'home' urlpatterns = [ + # We do this twice because Allauth expects specific view names to exist path('', HomeView.as_view(), name='home'), + path('', HomeView.as_view(), name='socialaccount_connections'), + path('pages/', include('wiki.urls')), path('accounts/', include('allauth.socialaccount.providers.discord.urls')), diff --git a/pydis_site/apps/home/views/account/settings.py b/pydis_site/apps/home/views/account/settings.py index ee9011da..84a2dea0 100644 --- a/pydis_site/apps/home/views/account/settings.py +++ b/pydis_site/apps/home/views/account/settings.py @@ -1,12 +1,20 @@ +from allauth.socialaccount.models import SocialAccount +from allauth.socialaccount.providers import registry from django.contrib.auth.mixins import LoginRequiredMixin +from django.contrib.messages import ERROR, INFO, add_message from django.http import HttpRequest, HttpResponse -from django.shortcuts import render +from django.shortcuts import redirect, render from django.urls import reverse from django.views import View class SettingsView(LoginRequiredMixin, View): - """Account settings view, for managing and deleting user accounts and connections.""" + """ + Account settings view, for managing and deleting user accounts and connections. + + This view actually renders a template with a bare modal, and is intended to be + inserted into another template using JavaScript. + """ def __init__(self, *args, **kwargs): self.login_url = reverse("home") @@ -14,4 +22,38 @@ class SettingsView(LoginRequiredMixin, View): def get(self, request: HttpRequest) -> HttpResponse: """HTTP GET: Return the view template.""" - return render(request, "home/account/settings.html") + context = { + "groups": request.user.groups.all(), + + "discord": None, + "github": None, + + "discord_provider": registry.provider_map.get("discord"), + "github_provider": registry.provider_map.get("github"), + } + + for account in SocialAccount.objects.filter(user=request.user).all(): + if account.provider == "discord": + context["discord"] = account + + elif account.provider == "github": + context["github"] = account + + return render(request, "home/account/settings.html", context=context) + + def post(self, request: HttpRequest) -> HttpResponse: + """HTTP POST: Process account disconnections.""" + provider = request.POST["provider"] + + if provider == "github": + try: + account = SocialAccount.objects.get(user=request.user, provider=provider) + except SocialAccount.DoesNotExist: + add_message(request, ERROR, "You do not have a GitHub account linked.") + else: + account.delete() + add_message(request, INFO, "The social account has been disconnected.") + else: + add_message(request, ERROR, f"Unknown provider: {provider}") + + return redirect(reverse("home")) diff --git a/pydis_site/apps/staff/models/role_mapping.py b/pydis_site/apps/staff/models/role_mapping.py index dff8081a..8a1fac2e 100644 --- a/pydis_site/apps/staff/models/role_mapping.py +++ b/pydis_site/apps/staff/models/role_mapping.py @@ -22,7 +22,7 @@ class RoleMapping(models.Model): ) is_staff = models.BooleanField( - help_text="Whether this role mapping related to a Django staff group", + help_text="Whether this role mapping relates to a Django staff group", default=False ) diff --git a/pydis_site/static/css/base/base.css b/pydis_site/static/css/base/base.css index 7db9499d..75702d47 100644 --- a/pydis_site/static/css/base/base.css +++ b/pydis_site/static/css/base/base.css @@ -96,3 +96,13 @@ button.is-size-navbar-menu, a.is-size-navbar-menu { padding-right: 1rem; } } + +/* Fix for modals being behind the navbar */ + +.modal * { + z-index: 1020; +} + +.modal-background { + z-index: 1010; +} diff --git a/pydis_site/static/js/base/modal.js b/pydis_site/static/js/base/modal.js new file mode 100644 index 00000000..eccc8845 --- /dev/null +++ b/pydis_site/static/js/base/modal.js @@ -0,0 +1,100 @@ +/* + modal.js: A simple way to wire up Bulma modals. + + This library is intended to be used with Bulma's modals, as described in the + official Bulma documentation. It's based on the JavaScript that Bulma + themselves use for this purpose on the modals documentation page. + + Note that, just like that piece of JavaScript, this library assumes that + you will only ever want to have one modal open at once. + */ + +"use strict"; + +// Event handler for the "esc" key, for closing modals. + +document.addEventListener("keydown", (event) => { + const e = event || window.event; + + if (e.code === "Escape" || e.keyCode === 27) { + closeModals(); + } +}); + +// An array of all the modal buttons we've already set up + +const modal_buttons = []; + +// Public API functions + +function setupModal(target) { + // Set up a modal's events, given a DOM element. This can be + // used later in order to set up a modal that was added after + // this library has been run. + + // We need to collect a bunch of elements to work with + const modal_background = Array.from(target.getElementsByClassName("modal-background")); + const modal_close = Array.from(target.getElementsByClassName("modal-close")); + + const modal_head = Array.from(target.getElementsByClassName("modal-card-head")); + const modal_foot = Array.from(target.getElementsByClassName("modal-card-foot")); + + const modal_delete = []; + const modal_button = []; + + modal_head.forEach((element) => modal_delete.concat(Array.from(element.getElementsByClassName("delete")))); + modal_foot.forEach((element) => modal_button.concat(Array.from(element.getElementsByClassName("button")))); + + // Collect all the elements that can be used to close modals + const modal_closers = modal_background.concat(modal_close).concat(modal_delete).concat(modal_button); + + // Assign click events for closing modals + modal_closers.forEach((element) => { + element.addEventListener("click", () => { + closeModals(); + }); + }); + + setupOpeningButtons(); +} + +function setupOpeningButtons() { + // Wire up all the opening buttons, avoiding buttons we've already wired up. + const modal_opening_buttons = Array.from(document.getElementsByClassName("modal-button")); + + modal_opening_buttons.forEach((element) => { + if (!modal_buttons.includes(element)) { + element.addEventListener("click", () => { + openModal(element.dataset.target); + }); + + modal_buttons.push(element); + } + }); +} + +function openModal(target) { + // Open a modal, given a string ID + const element = document.getElementById(target); + + document.documentElement.classList.add("is-clipped"); + element.classList.add("is-active"); +} + +function closeModals() { + // Close all open modals + const modals = Array.from(document.getElementsByClassName("modal")); + document.documentElement.classList.remove("is-clipped"); + + modals.forEach((element) => { + element.classList.remove("is-active"); + }); +} + +(function () { + // Set up all the modals currently on the page + const modals = Array.from(document.getElementsByClassName("modal")); + + modals.forEach((modal) => setupModal(modal)); + setupOpeningButtons(); +}()); diff --git a/pydis_site/templates/base/base.html b/pydis_site/templates/base/base.html index a9b31c0f..4c70d778 100644 --- a/pydis_site/templates/base/base.html +++ b/pydis_site/templates/base/base.html @@ -28,6 +28,7 @@ {# Font-awesome here is defined explicitly so that we can have Pro #} + diff --git a/pydis_site/templates/base/navbar.html b/pydis_site/templates/base/navbar.html index bd0bab40..6943c2b6 100644 --- a/pydis_site/templates/base/navbar.html +++ b/pydis_site/templates/base/navbar.html @@ -105,7 +105,7 @@
- + @@ -124,3 +124,19 @@
+ +{% if user.is_authenticated %} + +{% endif %} diff --git a/pydis_site/templates/home/account/settings.html b/pydis_site/templates/home/account/settings.html index ba1d38a2..9f48d736 100644 --- a/pydis_site/templates/home/account/settings.html +++ b/pydis_site/templates/home/account/settings.html @@ -1,12 +1,137 @@ -{% extends 'base/base.html' %} -{% load static %} +{% load socialaccount %} -{% block title %}My Account{% endblock %} +{# This template is just for a modal, which is actually inserted into the navbar #} +{# template. Take a look at `navbar.html` to see how it's inserted. #} -{% block content %} - {% include "base/navbar.html" %} + -
-{% endblock %} -- cgit v1.2.3 From ebc3a8ae61caa8f5d12992295f0bc122282fbcb4 Mon Sep 17 00:00:00 2001 From: Gareth Coles Date: Wed, 23 Oct 2019 01:33:06 +0100 Subject: Full coverage and finish cleanup --- pydis_site/apps/home/tests/test_views.py | 86 ++++++++++++++++++++++++++ pydis_site/apps/home/views/account/settings.py | 2 +- pydis_site/templates/base/navbar.html | 13 ++-- 3 files changed, 96 insertions(+), 5 deletions(-) (limited to 'pydis_site/apps') diff --git a/pydis_site/apps/home/tests/test_views.py b/pydis_site/apps/home/tests/test_views.py index 85423c1c..572317a7 100644 --- a/pydis_site/apps/home/tests/test_views.py +++ b/pydis_site/apps/home/tests/test_views.py @@ -1,3 +1,4 @@ +from allauth.socialaccount.models import SocialAccount from django.contrib.auth.models import User from django.http import HttpResponseRedirect from django.test import TestCase @@ -91,6 +92,42 @@ class TestAccountSettingsView(TestCase): username="user#0000" ) + self.user_unlinked = User.objects.create( + username="user#9999" + ) + + self.user_unlinked_discord = User.objects.create( + username="user#1234" + ) + + self.user_unlinked_github = User.objects.create( + username="user#1111" + ) + + self.github_account = SocialAccount.objects.create( + user=self.user, + provider="github", + uid="0" + ) + + self.discord_account = SocialAccount.objects.create( + user=self.user, + provider="discord", + uid="0000" + ) + + self.github_account_secondary = SocialAccount.objects.create( + user=self.user_unlinked_discord, + provider="github", + uid="1" + ) + + self.discord_account_secondary = SocialAccount.objects.create( + user=self.user_unlinked_github, + provider="discord", + uid="1111" + ) + def test_redirect_when_logged_out(self): """Check that the user is redirected to the homepage when not logged in.""" url = reverse("account_settings") @@ -108,6 +145,55 @@ class TestAccountSettingsView(TestCase): self.assertEqual(resp.status_code, 200) + self.client.force_login(self.user_unlinked) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + self.client.force_login(self.user_unlinked_discord) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + self.client.force_login(self.user_unlinked_github) + resp = self.client.get(url) + self.client.logout() + + self.assertEqual(resp.status_code, 200) + + def test_post_invalid(self): + """Test the behaviour of invalid POST submissions.""" + url = reverse("account_settings") + + self.client.force_login(self.user_unlinked) + + resp = self.client.post(url, {"provider": "discord"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + resp = self.client.post(url, {"provider": "github"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + self.client.logout() + + def test_post_valid(self): + """Ensure that GitHub is unlinked with a valid POST submission.""" + url = reverse("account_settings") + + self.client.force_login(self.user) + + resp = self.client.post(url, {"provider": "github"}) + self.assertEqual(resp.status_code, 302) + self.assertTrue(check_redirect_url(resp, reverse("home"))) + + with self.assertRaises(SocialAccount.DoesNotExist): + SocialAccount.objects.get(user=self.user, provider="github") + + self.client.logout() + class TestIndexReturns200(TestCase): def test_index_returns_200(self): diff --git a/pydis_site/apps/home/views/account/settings.py b/pydis_site/apps/home/views/account/settings.py index 84a2dea0..3a817dbc 100644 --- a/pydis_site/apps/home/views/account/settings.py +++ b/pydis_site/apps/home/views/account/settings.py @@ -36,7 +36,7 @@ class SettingsView(LoginRequiredMixin, View): if account.provider == "discord": context["discord"] = account - elif account.provider == "github": + if account.provider == "github": context["github"] = account return render(request, "home/account/settings.html", context=context) diff --git a/pydis_site/templates/base/navbar.html b/pydis_site/templates/base/navbar.html index 6943c2b6..2ba5bdd4 100644 --- a/pydis_site/templates/base/navbar.html +++ b/pydis_site/templates/base/navbar.html @@ -127,16 +127,21 @@ {% if user.is_authenticated %} {% endif %} -- cgit v1.2.3