From 4a4d4a5cf308be3faf81c8562cf39f8605863a94 Mon Sep 17 00:00:00 2001 From: Arona Jones Date: Sat, 29 Feb 2020 11:34:50 +0000 Subject: [PATCH] Add authorisation process for sign ups and allow access to EventDetail for basic users (#399) * CHANGE: First pass at opening up RIGS #233 Whilst it makes it something of a misnomer, the intent is to make the 'view_event' perm a permission to view event details like client/price. I don't see the point in giving everyone 'view_event' and adding a new 'view_event_detail'...Open to arguments the other way. * CHANGE: New user signups now require admin approval Given that I intend to reveal much more data to new users this seems necessary... * CHORE: Fix CI * FIX: Legacy Profiles are now auto-approved correctly * Add testing of approval mechanism This fixes the other functional tests failing because the user cannot login without being approved. * Superusers bypass approval check This should fix the remainder of the tests * Prevent unapproved users logging in through embeds Test suite doing its job...! * FIX: Require login on events and event embeds again Little too far to the open side there Arona... Whooooooops! * FIX: Use has_oembed decorator for events * FIX: Re-prevent basic seeing reversion This is to prevent financials/client data leaking when changed. Hopefully can show them a filtered version in future. * FIX: Remove mitigation for #264 Someone quietly fixed it, it appears * FEAT: Add admin email notif when an account is activated and awaiting approval No async or time-since shenanigans yet! * FIX: Whoops, undo accidental whitespace change * FEAT: Add a fifteen min cooldown between emails to admins Probably not the right way to go about it...but it does work! TODO: How to handle cooldown-emailing shared mailbox addresses? * FIX: Remove event modal history deadlink for basic users Also removes some links on the RIGS homepage that will deadlink for them * FIX: Wrong perms syntax for history pages * CHORE: Squash migrations * FIX: Use a setting for cooldown * FIX: Minor code improvements --- PyRIGS/settings.py | 9 +++-- RIGS/admin.py | 12 ++++++- RIGS/forms.py | 12 ++++++- RIGS/migrations/0036_profile_is_approved.py | 23 ++++++++++++ RIGS/migrations/0037_approve_legacy.py | 19 ++++++++++ RIGS/models.py | 10 ++++++ RIGS/signals.py | 36 +++++++++++++++++++ .../RIGS/admin_awaiting_approval.html | 9 +++++ .../RIGS/admin_awaiting_approval.txt | 5 +++ RIGS/templates/RIGS/event_detail.html | 22 ++++++++---- RIGS/templates/RIGS/event_embed.html | 10 +++--- RIGS/templates/RIGS/event_table.html | 2 +- RIGS/templates/RIGS/index.html | 7 ++-- RIGS/templates/RIGS/item_row.html | 8 +++-- RIGS/templates/RIGS/item_table.html | 6 ++++ RIGS/test_functional.py | 25 ++++++++++++- RIGS/urls.py | 5 ++- RIGS/views.py | 2 +- .../registration/activation_complete.html | 4 +-- 19 files changed, 196 insertions(+), 30 deletions(-) create mode 100644 RIGS/migrations/0036_profile_is_approved.py create mode 100644 RIGS/migrations/0037_approve_legacy.py create mode 100644 RIGS/templates/RIGS/admin_awaiting_approval.html create mode 100644 RIGS/templates/RIGS/admin_awaiting_approval.txt diff --git a/PyRIGS/settings.py b/PyRIGS/settings.py index 5877ad74..b787bdd1 100644 --- a/PyRIGS/settings.py +++ b/PyRIGS/settings.py @@ -12,6 +12,7 @@ https://docs.djangoproject.com/en/1.7/ref/settings/ import os import raven import secrets +import datetime BASE_DIR = os.path.dirname(os.path.dirname(__file__)) @@ -44,9 +45,9 @@ if not DEBUG: INTERNAL_IPS = ['127.0.0.1'] -ADMINS = ( - ('Tom Price', 'tomtom5152@gmail.com') -) +ADMINS = [('Tom Price', 'tomtom5152@gmail.com'), ('IT Manager', 'it@nottinghamtec.co.uk'), ('Arona Jones', 'arona.jones@nottinghamtec.co.uk')] +if DEBUG: + ADMINS.append(('Testing Superuser', 'superuser@example.com')) # Application definition @@ -182,6 +183,8 @@ if not DEBUG or EMAILER_TEST: else: EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend' +EMAIL_COOLDOWN = datetime.timedelta(minutes=15) + # Internationalization # https://docs.djangoproject.com/en/1.7/topics/i18n/ diff --git a/RIGS/admin.py b/RIGS/admin.py index 49b8aa1e..846f014a 100644 --- a/RIGS/admin.py +++ b/RIGS/admin.py @@ -22,13 +22,22 @@ admin.site.register(models.Invoice) admin.site.register(models.Payment) +def approve_user(modeladmin, request, queryset): + queryset.update(is_approved=True) + + +approve_user.short_description = "Approve selected users" + + @admin.register(models.Profile) class ProfileAdmin(UserAdmin): + # Don't know how to add 'is_approved' whilst preserving the default list... + list_filter = ('is_approved', 'is_active', 'is_staff', 'is_superuser', 'groups') fieldsets = ( (None, {'fields': ('username', 'password')}), (_('Personal info'), { 'fields': ('first_name', 'last_name', 'email', 'initials', 'phone')}), - (_('Permissions'), {'fields': ('is_active', 'is_staff', 'is_superuser', + (_('Permissions'), {'fields': ('is_approved', 'is_active', 'is_staff', 'is_superuser', 'groups', 'user_permissions')}), (_('Important dates'), { 'fields': ('last_login', 'date_joined')}), @@ -41,6 +50,7 @@ class ProfileAdmin(UserAdmin): ) form = forms.ProfileChangeForm add_form = forms.ProfileCreationForm + actions = [approve_user] class AssociateAdmin(VersionAdmin): diff --git a/RIGS/forms.py b/RIGS/forms.py index 47b0f062..a006bc87 100644 --- a/RIGS/forms.py +++ b/RIGS/forms.py @@ -2,8 +2,10 @@ from django import forms from django.utils import formats from django.conf import settings from django.core import serializers +from django.core.mail import EmailMessage, EmailMultiAlternatives from django.contrib.auth.forms import UserCreationForm, UserChangeForm, AuthenticationForm, PasswordResetForm from registration.forms import RegistrationFormUniqueEmail +from django.contrib.auth.forms import AuthenticationForm from captcha.fields import ReCaptchaField import simplejson @@ -33,8 +35,16 @@ class ProfileRegistrationFormUniqueEmail(RegistrationFormUniqueEmail): return self.cleaned_data['initials'] +class CheckApprovedForm(AuthenticationForm): + def confirm_login_allowed(self, user): + if user.is_approved or user.is_superuser: + return AuthenticationForm.confirm_login_allowed(self, user) + else: + raise forms.ValidationError("Your account hasn't been approved by an administrator yet. Please check back in a few minutes!") + + # Embedded Login form - remove the autofocus -class EmbeddedAuthenticationForm(AuthenticationForm): +class EmbeddedAuthenticationForm(CheckApprovedForm): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self.fields['username'].widget.attrs.pop('autofocus', None) diff --git a/RIGS/migrations/0036_profile_is_approved.py b/RIGS/migrations/0036_profile_is_approved.py new file mode 100644 index 00000000..3fdb5af7 --- /dev/null +++ b/RIGS/migrations/0036_profile_is_approved.py @@ -0,0 +1,23 @@ +# Generated by Django 2.0.13 on 2020-01-10 14:52 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('RIGS', '0035_auto_20191124_1319'), + ] + + operations = [ + migrations.AddField( + model_name='profile', + name='is_approved', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='profile', + name='last_emailed', + field=models.DateTimeField(blank=True, null=True), + ), + ] diff --git a/RIGS/migrations/0037_approve_legacy.py b/RIGS/migrations/0037_approve_legacy.py new file mode 100644 index 00000000..72348b3f --- /dev/null +++ b/RIGS/migrations/0037_approve_legacy.py @@ -0,0 +1,19 @@ +# Generated by Django 2.0.13 on 2020-01-11 18:29 +# This migration ensures that legacy Profiles from before approvals were implemented are automatically approved +from django.db import migrations + +def approve_legacy(apps, schema_editor): + Profile = apps.get_model('RIGS', 'Profile') + for person in Profile.objects.all(): + person.is_approved = True + person.save() + +class Migration(migrations.Migration): + + dependencies = [ + ('RIGS', '0036_profile_is_approved'), + ] + + operations = [ + migrations.RunPython(approve_legacy) + ] diff --git a/RIGS/models.py b/RIGS/models.py index 937d5354..0b97ca61 100644 --- a/RIGS/models.py +++ b/RIGS/models.py @@ -27,6 +27,8 @@ class Profile(AbstractUser): initials = models.CharField(max_length=5, unique=True, null=True, blank=False) phone = models.CharField(max_length=13, null=True, blank=True) api_key = models.CharField(max_length=40, blank=True, editable=False, null=True) + is_approved = models.BooleanField(default=False) + last_emailed = models.DateTimeField(blank=True, null=True) # Currently only populated by the admin approval email. TODO: Populate it each time we send any email, might need that... @classmethod def make_api_key(cls): @@ -53,6 +55,14 @@ class Profile(AbstractUser): def latest_events(self): return self.event_mic.order_by('-start_date').select_related('person', 'organisation', 'venue', 'mic') + @classmethod + def admins(cls): + return Profile.objects.filter(email__in=[y for x in settings.ADMINS for y in x]) + + @classmethod + def users_awaiting_approval_count(cls): + return Profile.objects.filter(models.Q(is_approved=False)).count() + def __str__(self): return self.name diff --git a/RIGS/signals.py b/RIGS/signals.py index ea0395d7..5c5e6c66 100644 --- a/RIGS/signals.py +++ b/RIGS/signals.py @@ -1,3 +1,4 @@ +import datetime import re import urllib.request import urllib.error @@ -10,6 +11,9 @@ from django.conf import settings from django.contrib.staticfiles.storage import staticfiles_storage from django.core.mail import EmailMessage, EmailMultiAlternatives from django.template.loader import get_template +from django.urls import reverse +from django.utils import timezone +from registration.signals import user_activated from premailer import Premailer from z3c.rml import rml2pdf @@ -102,3 +106,35 @@ def on_revision_commit(sender, instance, created, **kwargs): post_save.connect(on_revision_commit, sender=models.EventAuthorisation) + + +def send_admin_awaiting_approval_email(user, request, **kwargs): + # Bit more controlled than just emailing all superusers + for admin in models.Profile.admins(): + # Check we've ever emailed them before and if so, if cooldown has passed. + if admin.last_emailed is None or admin.last_emailed + settings.EMAIL_COOLDOWN <= timezone.now(): + context = { + 'request': request, + 'link_suffix': reverse("admin:RIGS_profile_changelist") + '?is_approved__exact=0', + 'number_of_users': models.Profile.users_awaiting_approval_count(), + 'to_name': admin.first_name + } + + email = EmailMultiAlternatives( + "%s new users awaiting approval on RIGS" % (context['number_of_users']), + get_template("RIGS/admin_awaiting_approval.txt").render(context), + to=[admin.email], + reply_to=[user.email], + ) + css = staticfiles_storage.path('css/email.css') + html = Premailer(get_template("RIGS/admin_awaiting_approval.html").render(context), + external_styles=css).transform() + email.attach_alternative(html, 'text/html') + email.send() + + # Update last sent + admin.last_emailed = timezone.now() + admin.save() + + +user_activated.connect(send_admin_awaiting_approval_email) diff --git a/RIGS/templates/RIGS/admin_awaiting_approval.html b/RIGS/templates/RIGS/admin_awaiting_approval.html new file mode 100644 index 00000000..8db5433c --- /dev/null +++ b/RIGS/templates/RIGS/admin_awaiting_approval.html @@ -0,0 +1,9 @@ +{% extends 'base_client_email.html' %} + +{% block content %} +

Hi {{ to_name|default_if_none:"Administrator" }},

+ +

{{ number_of_users|default_if_none:"Some" }} new users are awaiting administrator approval on RIGS. Click here to approve them.

+ +

TEC PA & Lighting

+{% endblock %} diff --git a/RIGS/templates/RIGS/admin_awaiting_approval.txt b/RIGS/templates/RIGS/admin_awaiting_approval.txt new file mode 100644 index 00000000..e89785ee --- /dev/null +++ b/RIGS/templates/RIGS/admin_awaiting_approval.txt @@ -0,0 +1,5 @@ +Hi {{ to_name|default_if_none:"Administrator" }}, + +{{ number_of_users|default_if_none:"Some" }} new users are awaiting administrator approval on RIGS. Use this link to approve them: {{ request.scheme }}://{{ request.get_host }}/{{ link_suffix }} + +TEC PA & Lighting diff --git a/RIGS/templates/RIGS/event_detail.html b/RIGS/templates/RIGS/event_detail.html index e22dafe9..10a947fe 100644 --- a/RIGS/templates/RIGS/event_detail.html +++ b/RIGS/templates/RIGS/event_detail.html @@ -10,12 +10,14 @@ | {{ object.name }} {% if event.dry_hire %}Dry Hire{% endif %} + {% if perms.RIGS.view_event %}
{% include 'RIGS/event_detail_buttons.html' %}
+ {% endif %} {% endif %} - {% if object.is_rig %} + {% if object.is_rig and perms.RIGS.view_event %} {# only need contact details for a rig #}
@@ -72,7 +74,7 @@ {% endif %}
{% endif %} -
+
Event Info
@@ -147,7 +149,7 @@
{{ object.collector }}
{% endif %} - {% if event.is_rig and not event.internal %} + {% if event.is_rig and not event.internal and perms.RIGS.view_event %}
 
PO
{{ object.purchase_order }}
@@ -156,7 +158,7 @@
- {% if event.is_rig and event.internal %} + {% if event.is_rig and event.internal and perms.RIGS.view_event %}
{% include 'RIGS/event_detail_buttons.html' %}
@@ -222,21 +224,23 @@
Event Details
+ {% if perms.RIGS.view_event %}

Notes

{{ event.notes|linebreaksbr }}
+ {% endif %} {% include 'RIGS/item_table.html' %}
- {% if not request.is_ajax %} + {% if not request.is_ajax and perms.RIGS.view_event %}
{% include 'RIGS/event_detail_buttons.html' %}
{% endif %} {% endif %} - {% if not request.is_ajax %} + {% if not request.is_ajax and perms.RIGS.view_event %}
@@ -251,12 +255,16 @@ {% if request.is_ajax %} {% block footer %}
+ {% if perms.RIGS.view_event %}
Last edited at {{ object.last_edited_at|default:'never' }} by {{ object.last_edited_by.name|default:'nobody' }}
+ {% else %} +
+ {% endif %}
Open Event Page diff --git a/RIGS/templates/RIGS/event_embed.html b/RIGS/templates/RIGS/event_embed.html index a6e3e586..78816cae 100644 --- a/RIGS/templates/RIGS/event_embed.html +++ b/RIGS/templates/RIGS/event_embed.html @@ -6,7 +6,7 @@
- Rig Information Gathering System + Rig Information Gathering System
@@ -20,9 +20,9 @@ {% endif %} - +

- + {% if object.is_rig %}N{{ object.pk|stringformat:"05d" }}{% else %}{{ object.pk }}{% endif %} | {{ object.name }} {% if object.venue %} @@ -72,7 +72,7 @@

- + {% if object.meet_at %}

Crew meet: @@ -97,7 +97,7 @@ {{ object.description|linebreaksbr }}

{% endif %} - +
diff --git a/RIGS/templates/RIGS/event_table.html b/RIGS/templates/RIGS/event_table.html index c84b6624..f1bdb5f6 100644 --- a/RIGS/templates/RIGS/event_table.html +++ b/RIGS/templates/RIGS/event_table.html @@ -33,7 +33,7 @@

- + {{ event.name }} {% if event.venue %} diff --git a/RIGS/templates/RIGS/index.html b/RIGS/templates/RIGS/index.html index acd1a707..29b7c320 100644 --- a/RIGS/templates/RIGS/index.html +++ b/RIGS/templates/RIGS/index.html @@ -11,7 +11,7 @@

- +

Quick Links

@@ -26,10 +26,11 @@ TEC Forum TEC Wiki + {% if perms.RIGS.view_event %} Pre-Event Risk Assessment Price List Subhire Insurance Form - + {% endif %}
@@ -73,7 +74,7 @@
{% if perms.RIGS.view_event %}
- {% include 'RIGS/activity_feed.html' %} + {% include 'RIGS/activity_feed.html' %}
{% endif %}
diff --git a/RIGS/templates/RIGS/item_row.html b/RIGS/templates/RIGS/item_row.html index 656d9812..b2ff0c44 100644 --- a/RIGS/templates/RIGS/item_row.html +++ b/RIGS/templates/RIGS/item_row.html @@ -6,17 +6,21 @@ {{item.description|linebreaksbr}}
+ {% if perms.RIGS.view_event %} £ {{item.cost|floatformat:2}} + {% endif %} {{item.quantity}} + {% if perms.RIGS.view_event %} £ {{item.total_cost|floatformat:2}} + {% endif %} {% if edit %} - - diff --git a/RIGS/templates/RIGS/item_table.html b/RIGS/templates/RIGS/item_table.html index 0c652daf..9f055aa9 100644 --- a/RIGS/templates/RIGS/item_table.html +++ b/RIGS/templates/RIGS/item_table.html @@ -3,9 +3,13 @@ Item + {% if perms.RIGS.view_event %} Price + {% endif %} Quantity + {% if perms.RIGS.view_event %} Sub-total + {% endif %} {% if edit %}
diff --git a/RIGS/test_functional.py b/RIGS/test_functional.py index 691f5294..dc7692a6 100644 --- a/RIGS/test_functional.py +++ b/RIGS/test_functional.py @@ -141,18 +141,41 @@ class UserRegistrationTest(LiveServerTestCase): self.assertEqual(password.get_attribute('placeholder'), 'Password') self.assertEqual(password.get_attribute('type'), 'password') + # Expected to fail as not approved username.send_keys('TestUsername') password.send_keys('correcthorsebatterystaple') self.browser.execute_script( "return function() {jQuery('#g-recaptcha-response').val('PASSED'); return 0}()") password.send_keys(Keys.ENTER) + # Test approval + profileObject = models.Profile.objects.all()[0] + self.assertFalse(profileObject.is_approved) + + # Read what the error is + alert = self.browser.find_element_by_css_selector( + 'div.alert-danger').text + self.assertIn("approved", alert) + + # Approve the user so we can proceed + profileObject.is_approved = True + profileObject.save() + + # Retry login + self.browser.get(self.live_server_url + '/user/login') + username = self.browser.find_element_by_id('id_username') + username.send_keys('TestUsername') + password = self.browser.find_element_by_id('id_password') + password.send_keys('correcthorsebatterystaple') + self.browser.execute_script( + "return function() {jQuery('#g-recaptcha-response').val('PASSED'); return 0}()") + password.send_keys(Keys.ENTER) + # Check we are logged in udd = self.browser.find_element_by_class_name('navbar').text self.assertIn('Hi John', udd) # Check all the data actually got saved - profileObject = models.Profile.objects.all()[0] self.assertEqual(profileObject.username, 'TestUsername') self.assertEqual(profileObject.first_name, 'John') self.assertEqual(profileObject.last_name, 'Smith') diff --git a/RIGS/urls.py b/RIGS/urls.py index 46e70f10..ab897d85 100644 --- a/RIGS/urls.py +++ b/RIGS/urls.py @@ -6,7 +6,7 @@ from RIGS import models, views, rigboard, finance, ical, versioning, forms from django.views.generic import RedirectView from django.views.decorators.clickjacking import xframe_options_exempt -from PyRIGS.decorators import permission_required_with_403 +from PyRIGS.decorators import permission_required_with_403, has_oembed from PyRIGS.decorators import api_key_required urlpatterns = [ @@ -87,8 +87,7 @@ urlpatterns = [ permission_required_with_403('RIGS.view_event')(versioning.ActivityFeed.as_view()), name='activity_feed'), - url(r'^event/(?P\d+)/$', - permission_required_with_403('RIGS.view_event', oembed_view="event_oembed")( + url(r'^event/(?P\d+)/$', has_oembed(oembed_view="event_oembed")( rigboard.EventDetail.as_view()), name='event_detail'), url(r'^event/(?P\d+)/embed/$', diff --git a/RIGS/views.py b/RIGS/views.py index f8494e25..1faa244d 100644 --- a/RIGS/views.py +++ b/RIGS/views.py @@ -41,7 +41,7 @@ def login(request, **kwargs): else: from django.contrib.auth.views import login - return login(request) + return login(request, authentication_form=forms.CheckApprovedForm) # This view should be exempt from requiring CSRF token. diff --git a/templates/registration/activation_complete.html b/templates/registration/activation_complete.html index 5aed33e9..470ee14d 100644 --- a/templates/registration/activation_complete.html +++ b/templates/registration/activation_complete.html @@ -5,6 +5,6 @@ {% block content %}

Activation Complete

-

You user account is now fully registered. Enjoy RIGS

+

Your user account is now awaiting administrator approval. Won't be long!

-{% endblock %} \ No newline at end of file +{% endblock %}