From 9523e224bcaa6a65676b7ae8b4bc5cd4d15453cf Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 9 Mar 2016 15:30:40 +0000 Subject: [PATCH 1/7] Removed links for deleting users. Also removed view function that allowed deletion of users. --- app/main/views/manage_users.py | 22 ------------------- .../views/edit-user-permissions.html | 16 +++++--------- app/templates/views/invite-user.html | 12 +--------- 3 files changed, 6 insertions(+), 44 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 5a0a7f5c9..6848794a5 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -103,28 +103,6 @@ def edit_user_permissions(service_id, user_id): ) -@main.route("/services//users//delete", methods=['GET', 'POST']) -@login_required -@user_has_permissions('manage_users', 'manage_templates', 'manage_settings') -def delete_user(service_id, user_id): - user = user_api_client.get_user(user_id) - service = get_service_by_id(service_id) - - if request.method == 'POST': - return redirect(url_for('.manage_users', service_id=service_id)) - - flash( - 'Are you sure you want to delete {}’s account?'.format(user.get('name') or user['email_localpart']), - 'delete' - ) - - return render_template( - 'views/invite-user.html', - user=user, - service_id=service_id - ) - - @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_users', 'manage_templates', 'manage_settings') def cancel_invited_user(service_id, invited_user_id): diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 291d7fa62..4af2cb2ce 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -26,17 +26,11 @@ Manage users – GOV.UK Notify {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }} - {% if user %} - {{ page_footer( - 'Save', - delete_link=url_for('.delete_user', service_id=service_id, user_id=user_id), - delete_link_text='Delete this account', - back_link=url_for('.manage_users', service_id=service_id), - back_link_text='Cancel' - ) }} - {% else %} - {{ page_footer('Send invitation email') }} - {% endif %} + {{ page_footer( + 'Save', + back_link=url_for('.manage_users', service_id=service_id), + back_link_text='Cancel' + ) }} diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index 407c2693f..dda36ff8c 100644 --- a/app/templates/views/invite-user.html +++ b/app/templates/views/invite-user.html @@ -28,17 +28,7 @@ Manage users – GOV.UK Notify {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }} - {% if user %} - {{ page_footer( - 'Save', - delete_link=url_for('.delete_user', service_id=service_id, user_id=user_id), - delete_link_text='Delete this account', - back_link=url_for('.manage_users', service_id=service_id), - back_link_text='Cancel' - ) }} - {% else %} - {{ page_footer('Send invitation email') }} - {% endif %} + {{ page_footer('Send invitation email') }} From 33c92a4e29ac1819ad2f986c40219ca0ae5be82e Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 9 Mar 2016 15:12:33 +0000 Subject: [PATCH 2/7] Fix for duplicate email registration that meant flash error not being displayed. The error message does not specify the error is a duplicate email address so as not to reveal which emails are current user accounts. --- app/main/views/register.py | 20 ++++++++++++++------ tests/app/main/views/test_register.py | 22 ++++++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/app/main/views/register.py b/app/main/views/register.py index eaeeaf150..c8759842d 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -30,9 +30,13 @@ def register(): form = RegisterUserForm() if form.validate_on_submit(): - return _do_registration(form) - else: - return render_template('views/register.html', form=form) + registered = _do_registration(form) + if registered: + return redirect(url_for('main.verify')) + else: + flash('There was an error registering your account') + + return render_template('views/register.html', form=form) @main.route('/register-from-invite', methods=['GET', 'POST']) @@ -46,7 +50,11 @@ def register_from_invite(): if form.validate_on_submit(): if form.service.data != invited_user['service'] or form.email_address.data != invited_user['email_address']: abort(400) - return _do_registration(form) + registered = _do_registration(form) + if registered: + return redirect(url_for('main.verify')) + else: + flash('There was an error registering your account') form.service.data = invited_user['service'] form.email_address.data = invited_user['email_address'] @@ -77,6 +85,6 @@ def _do_registration(form, service=None): users_dao.send_verify_code(user.id, 'email', user.email_address) session['expiry_date'] = str(datetime.now() + timedelta(hours=1)) session['user_details'] = {"email": user.email_address, "id": user.id} - return redirect(url_for('main.verify')) + return True else: - flash('There was an error registering your account') + return False diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index da83375c3..b0fd9ce93 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -1,4 +1,5 @@ from flask import url_for +from bs4 import BeautifulSoup def test_render_register_returns_template_with_form(app_): @@ -106,3 +107,24 @@ def test_should_return_400_if_password_is_blacklisted(app_, response.status_code == 200 assert 'That password is blacklisted, too common' in response.get_data(as_text=True) + + +def test_register_with_existing_email_returns_error(app_, + api_user_active, + mock_get_user_by_email): + user_data = { + 'name': 'Already Hasaccount', + 'email_address': api_user_active.email_address, + 'mobile_number': '+4407700900460', + 'password': 'validPassword!' + } + + with app_.test_request_context(): + response = app_.test_client().post(url_for('main.register'), + data=user_data) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + element = page.find('h1') + assert element.text == 'Create an account' + flash_banner = page.find('div', class_='banner-dangerous').string.strip() + assert flash_banner == 'There was an error registering your account' From 0095e4b6082423f9bbc1a0de4c4372c965aa5e9d Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 9 Mar 2016 15:46:45 +0000 Subject: [PATCH 3/7] Updated from git comments. Return status code 400 for duplicate email address. --- app/main/views/register.py | 1 + tests/app/main/views/test_register.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/main/views/register.py b/app/main/views/register.py index c8759842d..476032784 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -35,6 +35,7 @@ def register(): return redirect(url_for('main.verify')) else: flash('There was an error registering your account') + return render_template('views/register.html', form=form), 400 return render_template('views/register.html', form=form) diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index b0fd9ce93..d61513784 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -122,7 +122,7 @@ def test_register_with_existing_email_returns_error(app_, with app_.test_request_context(): response = app_.test_client().post(url_for('main.register'), data=user_data) - assert response.status_code == 200 + assert response.status_code == 400 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') element = page.find('h1') assert element.text == 'Create an account' From 836fe77c372eaa10bf6273b0efef54f6bcd709bb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 9 Mar 2016 16:53:02 +0000 Subject: [PATCH 4/7] =?UTF-8?q?Fix=20bug=20with=20=E2=80=98ghost=E2=80=99?= =?UTF-8?q?=20links=20in=20template=20textbox?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a bug with the code that highlights placeholders. It was taking the value of the textbox and copying it straight into the HTML of the layer that contains the blue lozenges. This meant that any HTML that the user typed into the textbox was rendered by the browser. This commit fixes the bug by: 1. taking the contents of the textbox 2. copying it to the _text_ (not inner HTML) of a temporary `
` 3. taking the inner HTML of that `
` (whose text has been encoded with HTML entities in step 2., eg `>` becomes `>`) 4. using that for the HTML content of the layer with the blue lozenges --- app/assets/javascripts/highlightTags.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/highlightTags.js b/app/assets/javascripts/highlightTags.js index f0f46fcd9..a1ecf2f37 100644 --- a/app/assets/javascripts/highlightTags.js +++ b/app/assets/javascripts/highlightTags.js @@ -33,7 +33,7 @@ }; this.update = () => this.$backgroundMaskForeground.html( - this.$textbox.val().replace( + $('
').text(this.$textbox.val()).html().replace( tagPattern, match => `${match}` ) ); From 4c323a9a9918f6a6959a0153950b0bb5eec38cea Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 9 Mar 2016 17:42:47 +0000 Subject: [PATCH 5/7] Added error message on template for failing to choose permissions. For error message over ride from WTF forms radio field created custom field. --- app/main/forms.py | 19 ++++++++++++++++--- app/main/views/styleguide.py | 4 ++++ app/templates/components/yes-no.html | 15 ++++++++++----- .../views/edit-user-permissions.html | 6 +++--- app/templates/views/invite-user.html | 6 +++--- app/templates/views/styleguide.html | 4 ++-- 6 files changed, 38 insertions(+), 16 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 7548c680c..474f90209 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -98,13 +98,26 @@ class RegisterUserFromInviteForm(Form): email_address = HiddenField('email_address') +# WTF forms does not give a handy way to customise error messages for +# radio button fields so just overriding the default here for use +# in permissions form. +class CustomRadioField(RadioField): + + def pre_validate(self, form): + for v, _ in self.choices: + if self.data == v: + break + else: + raise ValueError(self.gettext('Choose yes or no')) + + class PermisisonsForm(Form): # TODO fix this Radio field so we are not having to test for yes or no rather # use operator equality. - send_messages = RadioField("Send messages", choices=[('yes', 'yes'), ('no', 'no')]) - manage_service = RadioField("Manage service", choices=[('yes', 'yes'), ('no', 'no')]) - manage_api_keys = RadioField("Manage API keys", choices=[('yes', 'yes'), ('no', 'no')]) + send_messages = CustomRadioField("Send messages", choices=[('yes', 'yes'), ('no', 'no')]) + manage_service = CustomRadioField("Manage service", choices=[('yes', 'yes'), ('no', 'no')]) + manage_api_keys = CustomRadioField("Manage API keys", choices=[('yes', 'yes'), ('no', 'no')]) class InviteUserForm(PermisisonsForm): diff --git a/app/main/views/styleguide.py b/app/main/views/styleguide.py index eed30b20e..7d6186aa1 100644 --- a/app/main/views/styleguide.py +++ b/app/main/views/styleguide.py @@ -1,6 +1,7 @@ from flask import render_template, current_app, abort from flask_wtf import Form from wtforms import StringField, PasswordField, TextAreaField, FileField, validators +from app.main.forms import CustomRadioField from utils.template import Template from app.main import main @@ -17,11 +18,14 @@ def styleguide(): code = StringField('Enter code') message = TextAreaField(u'Message') file_upload = FileField('Upload a CSV file to add your recipients’ details') + manage_service = CustomRadioField('Manage service', choices=[('yes', 'yes'), ('no', 'no')]) + manage_templates = CustomRadioField('Manage templates', choices=[('yes', 'yes'), ('no', 'no')]) sms = "Your vehicle tax for ((registration number)) is due on ((date)). Renew online at www.gov.uk/vehicle-tax" form = FormExamples() form.message.data = sms + form.manage_service.data = 'yes' form.validate() template = Template({'content': sms}) diff --git a/app/templates/components/yes-no.html b/app/templates/components/yes-no.html index 0470ab3d6..0d58d14a0 100644 --- a/app/templates/components/yes-no.html +++ b/app/templates/components/yes-no.html @@ -1,15 +1,20 @@ -{% macro yes_no(name, label, current_value=None) %} -
+{% macro yes_no(field, current_value=None) %} +
- {{ label }} + {{ field.label }} + {% if field.errors %} + + {{ field.errors[0] }} + + {% endif %}
diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 4af2cb2ce..af8c94367 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -21,9 +21,9 @@ Manage users – GOV.UK Notify Permissions All team members can see message history - {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.data) }} - {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.data) }} - {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }} + {{ yes_no(form.send_messages, form.send_messages.data) }} + {{ yes_no(form.manage_service, form.manage_service.data) }} + {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }}
{{ page_footer( diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index dda36ff8c..d4fcf104f 100644 --- a/app/templates/views/invite-user.html +++ b/app/templates/views/invite-user.html @@ -23,9 +23,9 @@ Manage users – GOV.UK Notify Permissions All team members can see message history - {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.data) }} - {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.data) }} - {{ yes_no(form.manage_api_keys.name, form.manage_api_keys.label, form.manage_api_keys.data) }} + {{ yes_no(form.send_messages, form.send_messages.data) }} + {{ yes_no(form.manage_service, form.manage_service.data) }} + {{ yes_no(form.manage_api_keys, form.manage_api_keys.data) }}
{{ page_footer('Send invitation email') }} diff --git a/app/templates/views/styleguide.html b/app/templates/views/styleguide.html index a45f1a30e..d5addc1f9 100644 --- a/app/templates/views/styleguide.html +++ b/app/templates/views/styleguide.html @@ -247,8 +247,8 @@
- {{ yes_no('manage_service', 'Manage service', True) }} - {{ yes_no('templates', 'Create templates', True) }} + {{ yes_no(form.manage_service, form.manage_service.data) }} + {{ yes_no(form.manage_templates, form.manage_templates.data) }}
From 986edfa317f5eebe5a3e325df370e3be39da7de4 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 10 Mar 2016 11:57:40 +0000 Subject: [PATCH 6/7] Check user invite status not accepted before proceeding with flow. --- app/main/views/invites.py | 11 +++++++++-- tests/app/main/views/test_accept_invite.py | 21 +++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 44d00296c..72c63771d 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -3,7 +3,8 @@ from flask import ( url_for, session, abort, - render_template + render_template, + flash ) from notifications_python_client.errors import HTTPError @@ -20,8 +21,8 @@ from app import ( def accept_invite(token): try: - invited_user = invite_api_client.check_token(token) + if invited_user.status == 'cancelled': from_user = user_api_client.get_user(invited_user.from_user) service = get_service_by_id_or_404(invited_user.service) @@ -29,6 +30,11 @@ def accept_invite(token): from_user=from_user.name, service_name=service['name']) + if invited_user.status == 'accepted': + session.pop('invited_user', None) + flash('You have already accepted this invitation', 'default') + return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) + existing_user = user_api_client.get_user_by_email(invited_user.email_address) session['invited_user'] = invited_user.serialize() @@ -43,6 +49,7 @@ def accept_invite(token): return redirect(url_for('main.register_from_invite')) except HTTPError as e: + if e.status_code == 404: abort(404) else: diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 3e48e1f73..eaa796198 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -3,6 +3,8 @@ from flask import url_for from bs4 import BeautifulSoup import app + +from app.notify_client.models import InvitedUser from tests.conftest import sample_invite as create_sample_invite from tests.conftest import mock_check_invite_token as mock_check_token_invite @@ -34,6 +36,25 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard(app_, assert response.location == expected_redirect_location +def test_existing_user_cant_accept_twice(app_, + mocker, + sample_invite): + + sample_invite['status'] = 'accepted' + invite = InvitedUser(**sample_invite) + mocker.patch('app.invite_api_client.check_token', return_value=invite) + + with app_.test_request_context(): + with app_.test_client() as client: + response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Sign in' + flash_banners = page.find_all('div', class_='banner-default') + assert len(flash_banners) == 2 + assert flash_banners[0].text.strip() == 'You have already accepted this invitation' + + def test_existing_signed_out_user_accept_invite_redirects_to_sign_in(app_, service_one, api_user_active, From 6ea8491b39cb7f8564d4611ada2f1c7de2293e13 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 10 Mar 2016 14:29:31 +0000 Subject: [PATCH 7/7] Service name uniqueness handled in all cases and tests passing. --- app/main/forms.py | 19 ++++++- app/main/views/service_settings.py | 23 +++++--- tests/app/main/views/test_service_settings.py | 52 ++++++++++++++++++- tests/conftest.py | 21 ++++++++ 4 files changed, 106 insertions(+), 9 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 474f90209..1dbf8594f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -205,7 +205,24 @@ class AddServiceForm(Form): class ServiceNameForm(Form): - name = StringField(u'New name') + def __init__(self, names_func, *args, **kwargs): + """ + Keyword arguments: + names_func -- Returns a list of unique service_names already registered + on the system. + """ + self._names_func = names_func + super(ServiceNameForm, self).__init__(*args, **kwargs) + + name = StringField( + u'New name', + validators=[ + DataRequired(message='Service name can’t be empty') + ]) + + def validate_name(self, a): + if a.data in self._names_func(): + raise ValidationError('This service name is already in use') class ConfirmPasswordForm(Form): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index a57696375..89718a9f9 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -18,7 +18,8 @@ from notifications_python_client.errors import HTTPError from app.main.dao.services_dao import ( get_service_by_id, delete_service, - update_service + update_service, + find_all_service_names ) from app.main import main @@ -57,7 +58,7 @@ def service_name_change(service_id): else: raise e - form = ServiceNameForm() + form = ServiceNameForm(find_all_service_names) if form.validate_on_submit(): session['service_name_change'] = form.name.data @@ -89,10 +90,20 @@ def service_name_change_confirm(service_id): if form.validate_on_submit(): service['name'] = session['service_name_change'] - update_service(service) - session['service_name'] = service['name'] - session.pop('service_name_change') - return redirect(url_for('.service_settings', service_id=service_id)) + try: + update_service(service) + except HTTPError as e: + error_msg = "Duplicate service name '{}'".format(session['service_name_change']) + if e.status_code == 400 and error_msg in e.message['name']: + # Redirect the user back to the change service name screen + flash('This service name is already in use', 'error') + return redirect(url_for('main.service_name_change', service_id=service_id)) + else: + raise e + else: + session['service_name'] = service['name'] + session.pop('service_name_change') + return redirect(url_for('.service_settings', service_id=service_id)) return render_template( 'views/service-settings/confirm.html', heading='Change your service name', diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index f0210dd7e..65f7030e7 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -54,8 +54,9 @@ def test_should_redirect_after_change_service_name(app_, with app_.test_client() as client: client.login(api_user_active) service_id = 123 - response = client.post(url_for( - 'main.service_name_change', service_id=service_id)) + response = client.post( + url_for('main.service_name_change', service_id=service_id), + data={'name': "new name"}) assert response.status_code == 302 settings_url = url_for( @@ -64,6 +65,27 @@ def test_should_redirect_after_change_service_name(app_, assert mock_get_service.called +def test_should_not_allow_duplicate_names(app_, + api_user_active, + mock_get_service, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_has_permissions, + mock_get_services): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + service_id = 123 + response = client.post( + url_for('main.service_name_change', service_id=service_id), + data={'name': "service_one"}) + + assert response.status_code == 200 + resp_data = response.get_data(as_text=True) + assert 'This service name is already in use' in resp_data + + def test_should_show_service_name_confirmation(app_, api_user_active, mock_get_service, @@ -112,6 +134,32 @@ def test_should_redirect_after_service_name_confirmation(app_, assert mock_update_service.called +def test_should_raise_duplicate_name_handled(app_, + api_user_active, + mock_get_service, + mock_update_service_raise_httperror_duplicate_name, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_verify_password, + mock_has_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + service_id = 123 + service_new_name = 'New Name' + with client.session_transaction() as session: + session['service_name_change'] = service_new_name + response = client.post(url_for( + 'main.service_name_change_confirm', service_id=service_id)) + + assert response.status_code == 302 + name_change_url = url_for( + 'main.service_name_change', service_id=service_id, _external=True) + resp_data = response.get_data(as_text=True) + assert name_change_url == response.location + + def test_should_show_request_to_go_live(app_, api_user_active, mock_get_service, diff --git a/tests/conftest.py b/tests/conftest.py index 93dce2265..be599df6b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ import uuid from datetime import date, datetime, timedelta +from unittest.mock import Mock import pytest from app import create_app @@ -18,6 +19,8 @@ from app.notify_client.models import ( InvitedUser ) +from notifications_python_client.errors import HTTPError + @pytest.fixture(scope='session') def app_(request): @@ -90,6 +93,24 @@ def mock_update_service(mocker): 'app.notifications_api_client.update_service', side_effect=_update) +@pytest.fixture(scope='function') +def mock_update_service_raise_httperror_duplicate_name(mocker): + + def _update(service_id, + service_name, + active, + limit, + restricted, + users): + json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}}) + resp_mock = Mock(status_code=400, json=json_mock) + http_error = HTTPError(response=resp_mock, message="Default message") + raise http_error + + return mocker.patch( + 'app.notifications_api_client.update_service', side_effect=_update) + + SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb" SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52"