From 309396c906193cffa4cd5577f2a31bc73badd9ab Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 21 Dec 2017 16:42:16 +0000 Subject: [PATCH 1/5] Make email addresses case insensitive when inviting users to services Email addresses in invites should be case insensitive. This is to stop the bug where a user creates their account using a lower case email address (e.g. user1@gov.uk), but is then invited to a service using their email address in a different case (e.g. USER1.gov.uk) and sees an error message telling them that they can't accept an invite for a different email address. --- app/main/views/invites.py | 2 +- app/main/views/sign_in.py | 2 +- tests/app/main/views/test_accept_invite.py | 23 ++++++++++++++++++ tests/app/main/views/test_sign_in.py | 27 ++++++++++++++++++++++ 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 43e994e06..2da96a154 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -38,7 +38,7 @@ def accept_invite(token): invited_user = invite_api_client.check_token(token) - if not current_user.is_anonymous and current_user.email_address != invited_user.email_address: + if not current_user.is_anonymous and current_user.email_address.lower() != invited_user.email_address.lower(): message = Markup(""" You’re signed in as {}. This invite is for another email address. diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index eb77ad7a7..20033afa0 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -40,7 +40,7 @@ def sign_in(): if user and session.get('invited_user'): invited_user = session.get('invited_user') - if user.email_address != invited_user['email_address']: + if user.email_address.lower() != invited_user['email_address'].lower(): flash("You can't accept an invite for another person.") session.pop('invited_user', None) abort(403) diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index ae94139c8..0ff67f30b 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -315,6 +315,29 @@ def test_signed_in_existing_user_cannot_use_anothers_invite( assert mock_accept_invite.call_count == 0 +def test_accept_invite_does_not_treat_email_addresses_as_case_sensitive( + logged_in_client, + mocker, + api_user_active, + sample_invite, + service_one, + mock_accept_invite, + mock_get_user_by_email +): + mocker.patch('app.main.views.invites.check_token') + + # the email address of api_user_active is 'test@user.gov.uk' + sample_invite['email_address'] = 'TEST@user.gov.uk' + invite = InvitedUser(**sample_invite) + mocker.patch('app.invite_api_client.check_token', return_value=invite) + mocker.patch('app.user_api_client.get_users_for_service', return_value=[api_user_active]) + + response = logged_in_client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) + + assert response.status_code == 302 + assert response.location == url_for('main.service_dashboard', service_id=service_one['id'], _external=True) + + def test_new_invited_user_verifies_and_added_to_service( client, service_one, diff --git a/tests/app/main/views/test_sign_in.py b/tests/app/main/views/test_sign_in.py index 6f44c8d4a..563d29751 100644 --- a/tests/app/main/views/test_sign_in.py +++ b/tests/app/main/views/test_sign_in.py @@ -170,3 +170,30 @@ def test_should_attempt_redirect_when_user_is_pending( 'password': 'val1dPassw0rd!'}) assert response.location == url_for('main.resend_email_verification', _external=True) assert response.status_code == 302 + + +def test_email_address_is_treated_case_insensitively_when_signing_in_as_invited_user( + client, + mocker, + mock_verify_password, + api_user_active, + sample_invite, + mock_accept_invite, + mock_send_verify_code +): + sample_invite['email_address'] = 'TEST@user.gov.uk' + + mocker.patch('app.user_api_client.get_user_by_email_or_none', return_value=api_user_active) + mocker.patch('app.main.views.sign_in._get_and_verify_user', return_value=api_user_active) + + with client.session_transaction() as session: + session['invited_user'] = sample_invite + + response = client.post( + url_for('main.sign_in'), data={ + 'email_address': 'test@user.gov.uk', + 'password': 'val1dPassw0rd!'}) + + assert mock_accept_invite.called + assert response.status_code == 302 + assert mock_send_verify_code.called From bb460bd300d516367b49515f3e518d011737d77a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 2 Jan 2018 11:23:32 +0000 Subject: [PATCH 2/5] Enable Markdown links in emails Depends on: - [ ] https://github.com/alphagov/notifications-utils/pull/293 - [ ] https://github.com/alphagov/notifications-api/pull/1535 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index bbc4342a4..80dff8cdd 100644 --- a/requirements.txt +++ b/requirements.txt @@ -20,4 +20,4 @@ notifications-python-client==4.7.1 awscli==1.14.16 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.3.5#egg=notifications-utils==23.3.5 +git+https://github.com/alphagov/notifications-utils.git@23.4.0#egg=notifications-utils==23.4.0 From 85b3978061fce718f260f8231392fd4bee14ea64 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 2 Jan 2018 14:06:39 +0000 Subject: [PATCH 3/5] Force example CSV to download, not open in browser MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We wouldn’t wan’t anyone just seeing the raw CSV data in their browser – it isn’t clear what a user would do with it at that point. And we wouldn’t want it navigating them away from the send page, because this might cause them to lose their place. This commit forces the file to download using the HTML5 `download` attribute: > This attribute instructs browsers to download a URL instead of navigating to it, so the user will be prompted to save it as a local file. – https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#attr-download --- app/templates/views/send.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 0049292f6..3215b4601 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -40,7 +40,7 @@ {% endcall %}

- Download this example + Download this example

Your file will populate this template ({{ template.name }})

From a604ed60b675eb69867193f3085cd01aceeaeb4f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 2 Jan 2018 14:38:14 +0000 Subject: [PATCH 4/5] Remove value of download attribute on links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We should standardise on rather than everywhere. The value of the download attribute tells the browser what filename to use, but is overridden by the Content-Disposition HTTP header. Since it’s not being used, we should remove it for the sake of disambiguation. --- app/templates/partials/jobs/notifications.html | 2 +- app/templates/views/check/ok.html | 2 +- app/templates/views/dashboard/_inbox_messages.html | 2 +- app/templates/views/notifications/check.html | 2 +- app/templates/views/notifications/notification.html | 2 +- app/templates/views/send.html | 2 +- tests/app/main/views/test_jobs.py | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 70a086715..5a9c3ffd7 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -31,7 +31,7 @@

{% elif notifications %}

- Download this report + Download this report   {{ time_left }}

diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html index 792f016d1..85f79b97b 100644 --- a/app/templates/views/check/ok.html +++ b/app/templates/views/check/ok.html @@ -39,7 +39,7 @@ {% if template.template_type != 'letter' or not request.args.from_test %} {% else %} - Download as a printable PDF + Download as a printable PDF {% endif %} Back diff --git a/app/templates/views/dashboard/_inbox_messages.html b/app/templates/views/dashboard/_inbox_messages.html index b46b16a8c..bba1e741d 100644 --- a/app/templates/views/dashboard/_inbox_messages.html +++ b/app/templates/views/dashboard/_inbox_messages.html @@ -4,7 +4,7 @@
{% if messages %}

- Download these messages + Download these messages

{% endif %} {% call(item, row_number) list_table( diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html index 19c5d34e4..a58eee691 100644 --- a/app/templates/views/notifications/check.html +++ b/app/templates/views/notifications/check.html @@ -53,7 +53,7 @@ {% if template.template_type != 'letter' or not request.args.from_test %} {% else %} - Download as a printable PDF + Download as a printable PDF {% endif %} {% endif %} Back diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index f0ef58dba..f50a3c634 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -35,7 +35,7 @@ Estimated delivery date: {{ estimated_letter_delivery_date|string|format_date_short }}

- Download as a PDF + Download as a PDF

{% endif %} diff --git a/app/templates/views/send.html b/app/templates/views/send.html index 3215b4601..0ab3345ca 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -40,7 +40,7 @@ {% endcall %}

- Download this example + Download this example

Your file will populate this template ({{ template.name }})

diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 43a9c784e..2b812ca3f 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -97,7 +97,7 @@ def test_should_show_page_for_one_job( job_id=fake_uuid, status=status_argument, ) - csv_link = page.find('a', {'download': 'download'}) + csv_link = page.select_one('a[download]') assert csv_link['href'] == url_for( 'main.view_job_csv', service_id=service_one['id'], From 86d76baa0d52b6050b2af90dd69d1a7dc565119a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 20 Dec 2017 17:17:17 +0000 Subject: [PATCH 5/5] Have admin specify host to use for invite links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we’re doing user research we often: - start the task by inviting the participant to a service on Notify - have them use a prototype version of the admin app, hosted on a different domain Currently we can’t do both of these things together, because the invite emails always send people to notifications.service.gov.uk (because it’s the API that sends the emails, and the prototype admin app points at the production API). This commit changes the admin app to tell the API which host to use when creating the invite links. Depends on: - [ ] https://github.com/alphagov/notifications-api/pull/1515 --- app/notify_client/invite_api_client.py | 4 +- tests/app/notify_client/test_invite_client.py | 41 +++++++++++++++++-- 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 352aaef77..a3b2589b7 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -9,6 +9,7 @@ class InviteApiClient(NotifyAdminAPIClient): def init_app(self, app): self.base_url = app.config['API_HOST_NAME'] + self.admin_url = app.config['ADMIN_BASE_URL'] self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] self.api_key = app.config['ADMIN_CLIENT_SECRET'] @@ -18,7 +19,8 @@ class InviteApiClient(NotifyAdminAPIClient): 'email_address': email_address, 'from_user': invite_from_id, 'permissions': permissions, - 'auth_type': auth_type + 'auth_type': auth_type, + 'invite_link_host': self.admin_url, } data = _attach_current_user(data) resp = self.post(url='/service/{}/invite'.format(service_id), data=data) diff --git a/tests/app/notify_client/test_invite_client.py b/tests/app/notify_client/test_invite_client.py index 94e80863d..528c74a36 100644 --- a/tests/app/notify_client/test_invite_client.py +++ b/tests/app/notify_client/test_invite_client.py @@ -1,4 +1,40 @@ -from app.notify_client.invite_api_client import InviteApiClient +from unittest.mock import ANY +from app import invite_api_client + + +def test_client_creates_invite( + app_, + mocker, + fake_uuid, + sample_invite, +): + + mocker.patch('app.notify_client.current_user') + + mock_post = mocker.patch( + 'app.invite_api_client.post', + return_value={'data': dict.fromkeys({ + 'id', 'service', 'from_user', 'email_address', + 'permissions', 'status', 'created_at', 'auth_type' + })} + ) + + invite_api_client.create_invite( + '12345', '67890', 'test@example.com', 'send_messages', 'sms_auth' + ) + + mock_post.assert_called_once_with( + url='/service/{}/invite'.format('67890'), + data={ + 'auth_type': 'sms_auth', + 'email_address': 'test@example.com', + 'from_user': '12345', + 'service': '67890', + 'created_by': ANY, + 'permissions': 'send_messages', + 'invite_link_host': 'http://localhost:6012', + } + ) def test_client_returns_invite(mocker, sample_invite): @@ -10,10 +46,9 @@ def test_client_returns_invite(mocker, sample_invite): expected_url = '/service/{}/invite'.format(service_id) - client = InviteApiClient() mock_get = mocker.patch('app.notify_client.invite_api_client.InviteApiClient.get', return_value=expected_data) - invites = client.get_invites_for_service(service_id) + invites = invite_api_client.get_invites_for_service(service_id) mock_get.assert_called_once_with(expected_url) assert len(invites) == 1