From 9e710711cb9c9da723c8cb8d540592cb57d1754c Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 13:00:12 +0000 Subject: [PATCH 1/3] Updated form and fixed existing tests. --- app/main/dao/users_dao.py | 4 ++ app/main/forms.py | 4 ++ app/main/views/manage_users.py | 69 ++++++++++++----------- app/notify_client/models.py | 4 +- app/templates/views/invite-user.html | 14 ++--- app/templates/views/manage-users.html | 12 ++-- tests/app/main/views/test_manage_users.py | 24 ++++++-- 7 files changed, 73 insertions(+), 58 deletions(-) diff --git a/app/main/dao/users_dao.py b/app/main/dao/users_dao.py index c0da683cf..e494ddfd9 100644 --- a/app/main/dao/users_dao.py +++ b/app/main/dao/users_dao.py @@ -9,6 +9,10 @@ from app.main.encryption import hashpw from app import user_api_client +# TODO fix up this, do we really need this class why not just use the clients +# directly?? + + @login_manager.user_loader def load_user(user_id): return get_user_by_id(user_id) diff --git a/app/main/forms.py b/app/main/forms.py index ac5ef2f72..b68162b95 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -110,6 +110,10 @@ class RegisterUserFromInviteForm(Form): class InviteUserForm(Form): email_address = email_address('Their email address') + send_messages = BooleanField("Send messages") + manage_service = BooleanField("Manage service") + manage_api_keys = BooleanField("Manage API keys") + class TwoFactorForm(Form): def __init__(self, validate_code_func, *args, **kwargs): diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 513bfbdaf..13e02ed2c 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -15,9 +15,10 @@ from notifications_python_client.errors import HTTPError from app.main import main from app.main.forms import InviteUserForm -from app.main.dao.services_dao import get_service_by_id_or_404 +from app.main.dao.services_dao import get_service_by_id from app import user_api_client from app import invite_api_client +from app.utils import user_has_permissions fake_users = [ { @@ -32,45 +33,35 @@ fake_users = [ @main.route("/services//users") @login_required +@user_has_permissions('manage_users', 'manage_templates', 'manage_settings') def manage_users(service_id): - try: - users = user_api_client.get_users_for_service(service_id=service_id) - invited_users = invite_api_client.get_invites_for_service(service_id=service_id) - return render_template('views/manage-users.html', - service_id=service_id, - users=users, - current_user=current_user, - invited_users=invited_users) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + users = user_api_client.get_users_for_service(service_id=service_id) + invited_users = invite_api_client.get_invites_for_service(service_id=service_id) + return render_template('views/manage-users.html', + service_id=service_id, + users=users, + current_user=current_user, + invited_users=invited_users) @main.route("/services//users/invite", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_users', 'manage_templates', 'manage_settings') def invite_user(service_id): + service = get_service_by_id(service_id) + form = InviteUserForm() if form.validate_on_submit(): email_address = form.email_address.data permissions = _get_permissions(request.form) - try: - invited_user = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) - flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick') - return redirect(url_for('.manage_users', service_id=service_id)) - - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e + invited_user = invite_api_client.create_invite(current_user.id, service_id, email_address, permissions) + flash('Invite sent to {}'.format(invited_user.email_address), 'default_with_tick') + return redirect(url_for('.manage_users', service_id=service_id)) return render_template( 'views/invite-user.html', - user={}, - service=get_service_by_id_or_404(service_id), + user=None, service_id=service_id, form=form ) @@ -78,29 +69,40 @@ def invite_user(service_id): @main.route("/services//users/", methods=['GET', 'POST']) @login_required +@user_has_permissions('manage_users', 'manage_templates', 'manage_settings') def edit_user_permissions(service_id, user_id): + # TODO we should probably using the service id here in the get user + # call as well. eg. /user/?&service_id=service_id + user = user_api_client.get_user(user_id) + service = get_service_by_id(service_id) - if request.method == 'POST': + form = InviteUserForm(**{ + 'email_address': user.email_address, + 'send_messages': user.has_permissions(['send_texts', 'send_emails', 'send_letters']), + 'manage_service': user.has_permissions(['manage_users', 'manage_templates', 'manage_settings']), + 'manage_api_keys': user.has_permissions(['manage_api_keys', 'access_developer_docs']) + }) + if form.validate_on_submit(): return redirect(url_for('.manage_users', service_id=service_id)) return render_template( 'views/invite-user.html', - user=fake_users[int(user_id)], - user_id=user_id, - service=get_service_by_id_or_404(service_id), + user=user, + form=form, service_id=service_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)) - user = fake_users[int(user_id)] - flash( 'Are you sure you want to delete {}’s account?'.format(user.get('name') or user['email_localpart']), 'delete' @@ -109,13 +111,12 @@ def delete_user(service_id, user_id): return render_template( 'views/invite-user.html', user=user, - user_id=user_id, - service=get_service_by_id_or_404(service_id), 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): invite_api_client.cancel_invited_user(service_id=service_id, invited_user_id=invited_user_id) diff --git a/app/notify_client/models.py b/app/notify_client/models.py index df8d41306..bebeb7be9 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -130,8 +130,8 @@ class InvitedUser(object): self.status = status self.created_at = created_at - def has_permissions(self, permission): - return permission in self.permissions + def has_permissions(self, permissions): + return set(self.permissions) > set(permissions) def serialize(self): return {'id': self.id, diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index da40d42ff..4b326ba11 100644 --- a/app/templates/views/invite-user.html +++ b/app/templates/views/invite-user.html @@ -16,22 +16,16 @@ Manage users – GOV.UK Notify
- {% if user %} -

- {{ current_user.email_address }} -

- {% else %} - {{ textbox(form.email_address, hint='Email address must end in .gov.uk', width='1-1') }} - {% endif %} + {{ textbox(form.email_address, hint='Email address must end in .gov.uk', width='1-1') }}
Permissions All team members can see message history - {{ yes_no('send_messages', 'Send messages', user.permission_send_messages) }} - {{ yes_no('manage_service', 'Manage service', user.permission_manage_service) }} - {{ yes_no('manage_api_keys', 'Manage API keys', user.permission_manage_api_keys) }} + {{ yes_no(form.send_messages.name, form.send_messages.label, form.send_messages.value) }} + {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.value) }} + {{ yes_no(form.manage_api_keys, form.manage_api_keys, form.manage_api_keys.value) }}
{% if user %} diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index 161f966ec..ccc10a6c0 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -28,9 +28,9 @@ Manage users – GOV.UK Notify {% call field() %} {{ item.name }} {% endcall %} - {{ boolean_field(item.has_permissions(service_id, 'send_messages')) }} - {{ boolean_field(item.has_permissions(service_id, 'manage_service')) }} - {{ boolean_field(item.has_permissions(service_id, 'manage_api_keys')) }} + {{ boolean_field(item.has_permissions(['send_texts', 'send_emails', 'send_letters'], service_id=service_id)) }} + {{ boolean_field(item.has_permissions(['manage_users', 'manage_templates', 'manage_settings'], service_id=service_id)) }} + {{ boolean_field(item.has_permissions(['manage_api_keys', 'access_developer_docs'], service_id=service_id)) }} {% endcall %} {% if invited_users %} @@ -40,9 +40,9 @@ Manage users – GOV.UK Notify {% call field() %} {{ item.email_address }} {% endcall %} - {{ boolean_field(item.has_permissions('send_messages')) }} - {{ boolean_field(item.has_permissions('manage_service')) }} - {{ boolean_field(item.has_permissions('manage_api_keys')) }} + {{ boolean_field(item.has_permissions(['send_texts', 'send_emails', 'send_letters'])) }} + {{ boolean_field(item.has_permissions(['manage_users', 'manage_templates', 'manage_settings'])) }} + {{ boolean_field(item.has_permissions(['manage_api_keys', 'access_developer_docs'])) }} {% if item.status == 'pending' %} {% call field(align='right') %} Cancel invitation diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 0d4ecf093..22528d2e9 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -9,7 +9,8 @@ def test_should_show_overview_page( mock_login, mock_get_service, mock_get_users_by_service, - mock_get_invites_for_service + mock_get_invites_for_service, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -25,7 +26,8 @@ def test_should_show_page_for_one_user( app_, api_user_active, mock_login, - mock_get_service + mock_get_service, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -41,7 +43,8 @@ def test_redirect_after_saving_user( mock_login, mock_get_service, mock_get_users_by_service, - mock_get_invites_for_service + mock_get_invites_for_service, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -60,7 +63,9 @@ def test_should_show_page_for_inviting_user( app_, api_user_active, mock_login, - mock_get_service + mock_get_user, + mock_get_service, + mock_has_permissions ): with app_.test_request_context(): with app_.test_client() as client: @@ -76,9 +81,12 @@ def test_invite_user( service_one, api_user_active, mock_login, + mock_get_user, + mock_get_service, mock_get_users_by_service, mock_create_invite, - mock_get_invites_for_service + mock_get_invites_for_service, + mock_has_permissions ): from_user = api_user_active.id service_id = service_one['id'] @@ -106,7 +114,11 @@ def test_invite_user( assert flash_banner == 'Invite sent to test@example.gov.uk' -def test_cancel_invited_user_cancels_user_invitations(app_, api_user_active, mock_login, mocker): +def test_cancel_invited_user_cancels_user_invitations(app_, + api_user_active, + mock_login, + mocker, + mock_has_permissions): with app_.test_request_context(): with app_.test_client() as client: mocker.patch('app.invite_api_client.cancel_invited_user') From e5e9db88fddbef385bad19b68bd12d6b6d505193 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 14:32:19 +0000 Subject: [PATCH 2/3] Functionality_added, tests passing. --- app/main/dao/users_dao.py | 3 +- app/main/forms.py | 8 +-- app/main/views/manage_users.py | 31 ++++++++---- tests/app/main/views/test_manage_users.py | 59 +++++++++++++++++++++-- tests/conftest.py | 5 ++ 5 files changed, 87 insertions(+), 19 deletions(-) diff --git a/app/main/dao/users_dao.py b/app/main/dao/users_dao.py index e494ddfd9..411a22490 100644 --- a/app/main/dao/users_dao.py +++ b/app/main/dao/users_dao.py @@ -8,9 +8,10 @@ from app.main.encryption import hashpw from app import user_api_client - +# # TODO fix up this, do we really need this class why not just use the clients # directly?? +# @login_manager.user_loader diff --git a/app/main/forms.py b/app/main/forms.py index b68162b95..5aa1cec60 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -110,9 +110,11 @@ class RegisterUserFromInviteForm(Form): class InviteUserForm(Form): email_address = email_address('Their email address') - send_messages = BooleanField("Send messages") - manage_service = BooleanField("Manage service") - manage_api_keys = BooleanField("Manage API keys") + # 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')]) class TwoFactorForm(Form): diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 13e02ed2c..017509ac3 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -12,6 +12,7 @@ from flask_login import ( ) from notifications_python_client.errors import HTTPError +from app import user_api_client from app.main import main from app.main.forms import InviteUserForm @@ -20,16 +21,6 @@ from app import user_api_client from app import invite_api_client from app.utils import user_has_permissions -fake_users = [ - { - 'name': '', - 'permission_send_messages': True, - 'permission_manage_service': True, - 'permission_manage_api_keys': True, - 'active': True - } -] - @main.route("/services//users") @login_required @@ -83,6 +74,14 @@ def edit_user_permissions(service_id, user_id): 'manage_api_keys': user.has_permissions(['manage_api_keys', 'access_developer_docs']) }) if form.validate_on_submit(): + permissions = [] + permissions.extend( + _convert_role_to_permissions('send_messages') if form.send_messages.data == 'yes' else []) + permissions.extend( + _convert_role_to_permissions('manage_service') if form.manage_service.data == 'yes' else []) + permissions.extend( + _convert_role_to_permissions('manage_api_keys') if form.manage_api_keys.data == 'yes' else []) + user_api_client.set_user_permissions(user_id, service_id, permissions) return redirect(url_for('.manage_users', service_id=service_id)) return render_template( @@ -123,6 +122,18 @@ def cancel_invited_user(service_id, invited_user_id): return redirect(url_for('main.manage_users', service_id=service_id)) +def _convert_role_to_permissions(role): + if role == 'send_messages': + return ['send_texts', 'send_emails', 'send_letters'] + elif role == 'manage_service': + return ['manage_users', 'manage_templates', 'manage_settings'] + elif role == 'manage_api_keys': + return ['manage_api_keys', 'access_developer_docs'] + return [] + + +# TODO replace with method which converts each 'role' into the list +# of permissions like the method above :) def _get_permissions(form): permissions = [] if form.get('send_messages') and form['send_messages'] == 'yes': diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 22528d2e9..123a225e3 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -37,26 +37,75 @@ def test_should_show_page_for_one_user( assert response.status_code == 200 -def test_redirect_after_saving_user( +def test_edit_user_permissions( app_, api_user_active, mock_login, mock_get_service, mock_get_users_by_service, mock_get_invites_for_service, - mock_has_permissions + mock_has_permissions, + mock_set_user_permissions ): with app_.test_request_context(): with app_.test_client() as client: + service_id = '55555' client.login(api_user_active) response = client.post(url_for( - 'main.edit_user_permissions', service_id=55555, user_id=0 - )) + 'main.edit_user_permissions', service_id=service_id, user_id=api_user_active.id + ), data={'email_address': api_user_active.email_address, + 'send_messages': 'yes', + 'manage_service': 'yes', + 'manage_api_keys': 'yes'}) assert response.status_code == 302 assert response.location == url_for( - 'main.manage_users', service_id=55555, _external=True + 'main.manage_users', service_id=service_id, _external=True ) + mock_set_user_permissions.assert_called_with( + str(api_user_active.id), + service_id, + ['send_texts', + 'send_emails', + 'send_letters', + 'manage_users', + 'manage_templates', + 'manage_settings', + 'manage_api_keys', + 'access_developer_docs']) + + +def test_edit_some_user_permissions( + app_, + api_user_active, + mock_login, + mock_get_service, + mock_get_users_by_service, + mock_get_invites_for_service, + mock_has_permissions, + mock_set_user_permissions +): + with app_.test_request_context(): + with app_.test_client() as client: + service_id = '55555' + client.login(api_user_active) + response = client.post(url_for( + 'main.edit_user_permissions', service_id=service_id, user_id=api_user_active.id + ), data={'email_address': api_user_active.email_address, + 'send_messages': 'yes', + 'manage_service': 'no', + 'manage_api_keys': 'no'}) + + assert response.status_code == 302 + assert response.location == url_for( + 'main.manage_users', service_id=service_id, _external=True + ) + mock_set_user_permissions.assert_called_with( + str(api_user_active.id), + service_id, + ['send_texts', + 'send_emails', + 'send_letters']) def test_should_show_page_for_inviting_user( diff --git a/tests/conftest.py b/tests/conftest.py index 24471d539..2c8d8266d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -624,3 +624,8 @@ def mock_add_user_to_service(mocker, service_one, api_user_active): def _add_user(service_id, user_id): return api_user_active return mocker.patch('app.user_api_client.add_user_to_service', side_effect=_add_user) + + +@pytest.fixture(scope='function') +def mock_set_user_permissions(mocker): + return mocker.patch('app.user_api_client.set_user_permissions', return_value=None) From b3249831cfb3218cb278164d8f9b7fe544a6607a Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 3 Mar 2016 15:43:53 +0000 Subject: [PATCH 3/3] Fix up front end so you can navigate to the edit page. --- app/main/forms.py | 8 ++++---- app/main/views/manage_users.py | 13 +++++++++---- app/templates/components/yes-no.html | 4 ++-- app/templates/views/invite-user.html | 6 +++--- app/templates/views/manage-users.html | 5 +++++ 5 files changed, 23 insertions(+), 13 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 5aa1cec60..4d0cb8f80 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -108,13 +108,13 @@ class RegisterUserFromInviteForm(Form): class InviteUserForm(Form): - email_address = email_address('Their email address') + email_address = email_address('Their email address') # 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 = 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')]) class TwoFactorForm(Form): diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 017509ac3..b8dbfc4c2 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -66,13 +66,18 @@ def edit_user_permissions(service_id, user_id): # call as well. eg. /user/?&service_id=service_id user = user_api_client.get_user(user_id) service = get_service_by_id(service_id) - + # Need to make the email address read only, or a disabled field? + # Do it through the template or the form class? form = InviteUserForm(**{ 'email_address': user.email_address, - 'send_messages': user.has_permissions(['send_texts', 'send_emails', 'send_letters']), - 'manage_service': user.has_permissions(['manage_users', 'manage_templates', 'manage_settings']), - 'manage_api_keys': user.has_permissions(['manage_api_keys', 'access_developer_docs']) + 'send_messages': 'yes' if user.has_permissions( + ['send_texts', 'send_emails', 'send_letters']) else 'no', + 'manage_service': 'yes' if user.has_permissions( + ['manage_users', 'manage_templates', 'manage_settings']) else 'no', + 'manage_api_keys': 'yes' if user.has_permissions( + ['manage_api_keys', 'access_developer_docs']) else 'no' }) + if form.validate_on_submit(): permissions = [] permissions.extend( diff --git a/app/templates/components/yes-no.html b/app/templates/components/yes-no.html index 554a8e1fc..0470ab3d6 100644 --- a/app/templates/components/yes-no.html +++ b/app/templates/components/yes-no.html @@ -5,11 +5,11 @@
diff --git a/app/templates/views/invite-user.html b/app/templates/views/invite-user.html index 4b326ba11..407c2693f 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.value) }} - {{ yes_no(form.manage_service.name, form.manage_service.label, form.manage_service.value) }} - {{ yes_no(form.manage_api_keys, form.manage_api_keys, form.manage_api_keys.value) }} + {{ 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) }} {% if user %} diff --git a/app/templates/views/manage-users.html b/app/templates/views/manage-users.html index ccc10a6c0..0876f8f5b 100644 --- a/app/templates/views/manage-users.html +++ b/app/templates/views/manage-users.html @@ -31,6 +31,11 @@ Manage users – GOV.UK Notify {{ boolean_field(item.has_permissions(['send_texts', 'send_emails', 'send_letters'], service_id=service_id)) }} {{ boolean_field(item.has_permissions(['manage_users', 'manage_templates', 'manage_settings'], service_id=service_id)) }} {{ boolean_field(item.has_permissions(['manage_api_keys', 'access_developer_docs'], service_id=service_id)) }} + {% call field(align='right') %} + {% if current_user.id != item.id %} + Edit permission + {% endif %} + {% endcall %} {% endcall %} {% if invited_users %}