From 9dd5c892527f6c687f87871d71b91b27d3c25136 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 13 Jul 2021 14:43:26 +0100 Subject: [PATCH 1/3] Move two calls to `str.lower` next to each other MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This means that we can rewrite `validate_email_address` to do a different comparison without having to also change `__init__` I’ve moved the platform admin check into its own conditional to keep the line length manageable. --- app/main/forms.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index af03280d1..c543dc2a2 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1044,10 +1044,12 @@ class BaseInviteUserForm(): def __init__(self, invalid_email_address, *args, **kwargs): super().__init__(*args, **kwargs) - self.invalid_email_address = invalid_email_address.lower() + self.invalid_email_address = invalid_email_address def validate_email_address(self, field): - if field.data.lower() == self.invalid_email_address and not current_user.platform_admin: + if current_user.platform_admin: + return + if field.data.lower() == self.invalid_email_address.lower(): raise ValidationError("You cannot send an invitation to yourself") From c3091223a95b8d28f764f689cce70dbe487f5236 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 13 Jul 2021 15:26:19 +0100 Subject: [PATCH 2/3] Be strict about similar email addresses for alerts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t want a single person to have two accounts on an emergency alerts service because it would let them circumvent the two eyes approval process. We can go some way to mitigating against this by stopping people using common methods that email providers use to alias email addresses. These are: - being case insensitive - being insensitive to the position or number of dots in the local part of an email address - using ‘plus addressing’ We already prevent the first one, this commit adds normalisation which strip out the second two before doing the comparision with the current user’s email address. --- app/main/forms.py | 5 ++ app/utils/user.py | 11 ++++ tests/app/main/views/test_manage_users.py | 64 +++++++++++++++++++++++ 3 files changed, 80 insertions(+) diff --git a/app/main/forms.py b/app/main/forms.py index c543dc2a2..b2fc0573e 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -62,6 +62,7 @@ from app.models.roles_and_permissions import ( roles, ) from app.utils import merge_jsonlike +from app.utils.user import distinct_email_addresses def get_time_value_and_label(future_time): @@ -1060,6 +1061,10 @@ class InviteUserForm(BaseInviteUserForm, PermissionsForm): class BroadcastInviteUserForm(BaseInviteUserForm, BroadcastPermissionsForm): email_address = email_address(gov_user=True) + def validate_email_address(self, field): + if not distinct_email_addresses(field.data, self.invalid_email_address): + raise ValidationError("You cannot send an invitation to yourself") + class InviteOrgUserForm(StripWhitespaceForm): email_address = email_address(gov_user=False) diff --git a/app/utils/user.py b/app/utils/user.py index 577abb305..23a12ccb1 100644 --- a/app/utils/user.py +++ b/app/utils/user.py @@ -66,3 +66,14 @@ def _email_address_ends_with(email_address, known_domains): )) for known in known_domains ) + + +def normalise_email_address_aliases(email_address): + local_part, domain = email_address.split('@') + local_part = local_part.split('+')[0].replace('.', '') + + return f'{local_part}@{domain}'.lower() + + +def distinct_email_addresses(*args): + return len(args) == len(set(map(normalise_email_address_aliases, args))) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 11a74420c..3d69afe6e 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1443,6 +1443,70 @@ def test_user_cant_invite_themselves( assert not mock_create_invite.called +@pytest.mark.parametrize('email_address', ( + 'test@user.gov.uk', + 'TEST@user.gov.uk', + 'test@USER.gov.uk', + 'test+test@user.gov.uk', + 'te.st@user.gov.uk', + pytest.param('test2@user.gov.uk', marks=pytest.mark.xfail), + pytest.param('test@other.gov.uk', marks=pytest.mark.xfail), +)) +def test_broadcast_user_cant_invite_themselves_or_their_aliases( + client_request, + service_one, + mocker, + active_user_with_permissions, + mock_create_invite, + mock_get_template_folders, + email_address, +): + service_one['permissions'] += ['broadcast'] + page = client_request.post( + 'main.invite_user', + service_id=SERVICE_ONE_ID, + _data={ + 'email_address': email_address, + 'permissions_field': [] + }, + _expected_status=200, + ) + assert normalize_spaces(page.select_one('span.govuk-error-message').text) == ( + "Error: You cannot send an invitation to yourself" + ) + assert mock_create_invite.called is False + + +@pytest.mark.parametrize('extra_service_permissions', ( + pytest.param([], marks=pytest.mark.xfail), + ['broadcast'], +)) +def test_platform_admin_cant_invite_themselves_to_broadcast_services( + client_request, + service_one, + mocker, + platform_admin_user, + mock_create_invite, + mock_get_template_folders, + extra_service_permissions, +): + service_one['permissions'] += extra_service_permissions + client_request.login(platform_admin_user) + page = client_request.post( + 'main.invite_user', + service_id=SERVICE_ONE_ID, + _data={ + 'email_address': platform_admin_user['email_address'], + 'permissions_field': [] + }, + _expected_status=200, + ) + assert normalize_spaces(page.select_one('span.govuk-error-message').text) == ( + "Error: You cannot send an invitation to yourself" + ) + assert mock_create_invite.called is False + + def test_no_permission_manage_users_page( client_request, service_one, From d749ee5ceac6b44bc8d539a8f21695ad07f387ee Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 15 Jul 2021 14:21:58 +0100 Subject: [PATCH 3/3] Rename confusing variable The reason the email address is considered invalid is because it is the address of the person doing the inviting. This commit renames the variable to be more specific and avoid confusion with the email address of the person being invited. --- app/main/forms.py | 14 +++++++------- app/main/views/manage_users.py | 2 +- app/main/views/organisations.py | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index b2fc0573e..d0685d7dd 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1043,14 +1043,14 @@ class BroadcastPermissionsForm(BasePermissionsForm): class BaseInviteUserForm(): email_address = email_address(gov_user=False) - def __init__(self, invalid_email_address, *args, **kwargs): + def __init__(self, inviter_email_address, *args, **kwargs): super().__init__(*args, **kwargs) - self.invalid_email_address = invalid_email_address + self.inviter_email_address = inviter_email_address def validate_email_address(self, field): if current_user.platform_admin: return - if field.data.lower() == self.invalid_email_address.lower(): + if field.data.lower() == self.inviter_email_address.lower(): raise ValidationError("You cannot send an invitation to yourself") @@ -1062,19 +1062,19 @@ class BroadcastInviteUserForm(BaseInviteUserForm, BroadcastPermissionsForm): email_address = email_address(gov_user=True) def validate_email_address(self, field): - if not distinct_email_addresses(field.data, self.invalid_email_address): + if not distinct_email_addresses(field.data, self.inviter_email_address): raise ValidationError("You cannot send an invitation to yourself") class InviteOrgUserForm(StripWhitespaceForm): email_address = email_address(gov_user=False) - def __init__(self, invalid_email_address, *args, **kwargs): + def __init__(self, inviter_email_address, *args, **kwargs): super(InviteOrgUserForm, self).__init__(*args, **kwargs) - self.invalid_email_address = invalid_email_address.lower() + self.inviter_email_address = inviter_email_address.lower() def validate_email_address(self, field): - if field.data.lower() == self.invalid_email_address and not current_user.platform_admin: + if field.data.lower() == self.inviter_email_address and not current_user.platform_admin: raise ValidationError("You cannot send an invitation to yourself") diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 57e00ac3f..7b52d14d3 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -59,7 +59,7 @@ def invite_user(service_id, user_id=None): form_class = InviteUserForm form = form_class( - invalid_email_address=current_user.email_address, + inviter_email_address=current_user.email_address, all_template_folders=current_service.all_template_folders, folder_permissions=[f['id'] for f in current_service.all_template_folders] ) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 12405cbd8..867337266 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -184,7 +184,7 @@ def manage_org_users(org_id): @user_has_permissions() def invite_org_user(org_id): form = InviteOrgUserForm( - invalid_email_address=current_user.email_address + inviter_email_address=current_user.email_address ) if form.validate_on_submit(): email_address = form.email_address.data