From 94c4151640e3306ae878725456b09d045c323253 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 21 Feb 2019 13:03:06 +0000 Subject: [PATCH 1/7] Edit user permissions page shows redacted phone number and Change link Also make plan for story development --- app/main/views/manage_users.py | 16 +++++-- app/navigation.py | 4 ++ .../views/edit-user-permissions.html | 6 ++- .../views/manage-users/permissions.html | 2 +- app/utils.py | 14 ++++-- tests/app/main/views/test_manage_users.py | 48 +++++++++++++++++++ 6 files changed, 82 insertions(+), 8 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index ada8dcb16..c0b85ab99 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -24,7 +24,7 @@ from app.main.forms import ( SearchUsersForm, ) from app.models.user import permissions -from app.utils import user_has_permissions +from app.utils import redact_mobile_number, user_has_permissions @main.route("/services//users") @@ -78,7 +78,10 @@ def invite_user(service_id): def edit_user_permissions(service_id, user_id): service_has_email_auth = current_service.has_permission('email_auth') user = current_service.get_team_member(user_id) - user_has_no_mobile_number = user.mobile_number is None + + mobile_number = None + if user.mobile_number: + mobile_number = redact_mobile_number(user.mobile_number) form = PermissionsForm.from_user(user, service_id) @@ -96,7 +99,7 @@ def edit_user_permissions(service_id, user_id): user=user, form=form, service_has_email_auth=service_has_email_auth, - user_has_no_mobile_number=user_has_no_mobile_number + mobile_number=mobile_number ) @@ -200,6 +203,13 @@ def confirm_edit_user_email(service_id, user_id): ) +@main.route("/services//users//edit-phone-number", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_service') +def edit_user_phone_number(service_id, user_id): + return True + + @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_service') def cancel_invited_user(service_id, invited_user_id): diff --git a/app/navigation.py b/app/navigation.py index 125c81bc3..7def9b505 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -159,6 +159,7 @@ class HeaderNavigation(Navigation): 'edit_template_postage', 'edit_user_org_permissions', 'edit_user_email', + 'edit_user_phone_number', 'edit_user_permissions', 'email_not_received', 'email_template', @@ -328,6 +329,7 @@ class MainNavigation(Navigation): 'team-members': { 'confirm_edit_user_email', 'edit_user_email', + 'edit_user_phone_number', 'edit_user_permissions', 'invite_user', 'manage_users', @@ -609,6 +611,7 @@ class CaseworkNavigation(Navigation): 'edit_service_template', 'edit_template_postage', 'edit_user_email', + 'edit_user_phone_number', 'edit_user_org_permissions', 'edit_user_permissions', 'email_branding', @@ -847,6 +850,7 @@ class OrgNavigation(Navigation): 'edit_service_template', 'edit_template_postage', 'edit_user_email', + 'edit_user_phone_number', 'edit_user_permissions', 'email_branding', 'email_not_received', diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 4bd585d96..84b92ba27 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -16,7 +16,11 @@

{{ user.email_address }} Change

- + {% if mobile_number %} +

+ {{ mobile_number }} Change +

+ {% endif %}
{% call form_wrapper(class="column-three-quarters") %} diff --git a/app/templates/views/manage-users/permissions.html b/app/templates/views/manage-users/permissions.html index 2c3414caf..fab879e4f 100644 --- a/app/templates/views/manage-users/permissions.html +++ b/app/templates/views/manage-users/permissions.html @@ -15,7 +15,7 @@

{% if service_has_email_auth %} - {% if user_has_no_mobile_number %} + {% if not mobile_number %} {{ radios( form.login_authentication, disable=['sms_auth'], diff --git a/app/utils.py b/app/utils.py index e9615c725..d29f21f63 100644 --- a/app/utils.py +++ b/app/utils.py @@ -650,9 +650,9 @@ def guess_name_from_email_address(email_address): def should_skip_template_page(template_type): return ( - current_user.has_permissions('send_messages') and - not current_user.has_permissions('manage_templates', 'manage_api_keys') and - template_type != 'letter' + current_user.has_permissions('send_messages') + and not current_user.has_permissions('manage_templates', 'manage_api_keys') + and template_type != 'letter' ) @@ -671,3 +671,11 @@ def printing_today_or_tomorrow(): return 'today' else: return 'tomorrow' + + +def redact_mobile_number(mobile_number): + indices = [-4, -5, -6, -7] + mobile_number_list = list(mobile_number.replace(" ", "")) + for i in indices: + mobile_number_list[i] = "*" + return "".join(mobile_number_list) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 11e87b737..a1244dae9 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1005,3 +1005,51 @@ def test_confirm_edit_user_email_doesnt_change_user_email_for_non_team_member( user_id=USER_ONE_ID, _expected_status=404, ) + + +def test_confirm_edit_user_email_with_no_permission_aborts(): + pass + + +def test_edit_user_permissions_page_displays_redacted_phone_number_and_change_link( + client_request, + active_user_with_permissions, + service_one, + mocker +): + user = active_user_with_permissions + mocker.patch('app.user_api_client.get_user', return_value=user) + + page = client_request.get( + 'main.edit_user_permissions', + service_id=service_one['id'], + user_id=user.id + ) + + assert user.name in page.find('h1').text + phone_number_paragraph = page.select('p[id=user_phone_number]')[0] + assert '0770****762' in phone_number_paragraph.text + change_link = phone_number_paragraph.findChild() + assert change_link.attrs['href'] == '/services/{}/users/{}/edit-phone-number'.format( + service_one['id'], user.id + ) + + +def test_edit_user_phone_number_page(): + pass + + +def test_edit_user_phone_number_redirects_to_confirmation(): + pass + + +def test_confirm_edit_user_phone_number_page(): + pass + + +def test_confirm_edit_user_phone_number_changes_user_mobile_number(): + pass + + +def test_confirm_edit_user_phone_number_with_no_permission_aborts(): + pass From 9d57a8102967f63caf3ee8082b301d115329ef22 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 Feb 2019 11:31:35 +0000 Subject: [PATCH 2/7] Edit mobile number page --- app/main/views/manage_users.py | 17 ++++++-- .../views/edit-user-permissions.html | 4 +- tests/app/main/views/test_manage_users.py | 43 +++++++++++++------ 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index c0b85ab99..9cdba07fd 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -22,6 +22,7 @@ from app.main.forms import ( InviteUserForm, PermissionsForm, SearchUsersForm, + ChangeMobileNumberForm ) from app.models.user import permissions from app.utils import redact_mobile_number, user_has_permissions @@ -203,11 +204,21 @@ def confirm_edit_user_email(service_id, user_id): ) -@main.route("/services//users//edit-phone-number", methods=['GET', 'POST']) +@main.route("/services//users//edit-mobile-number", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') -def edit_user_phone_number(service_id, user_id): - return True +def edit_user_mobile_number(service_id, user_id): + user = user_api_client.get_user(user_id) + user_mobile_number = redact_mobile_number(user.mobile_number) + + form = ChangeMobileNumberForm(mobile_number=user_mobile_number) + + return render_template( + 'views/manage-users/edit-user-mobile.html', + user=user, + form=form, + service_id=service_id + ) @main.route("/services//cancel-invited-user/", methods=['GET']) diff --git a/app/templates/views/edit-user-permissions.html b/app/templates/views/edit-user-permissions.html index 84b92ba27..17a4ef88f 100644 --- a/app/templates/views/edit-user-permissions.html +++ b/app/templates/views/edit-user-permissions.html @@ -17,8 +17,8 @@ {{ user.email_address }} Change

{% if mobile_number %} -

- {{ mobile_number }} Change +

+ {{ mobile_number }} Change

{% endif %}
diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index a1244dae9..b41bb76f3 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1011,7 +1011,7 @@ def test_confirm_edit_user_email_with_no_permission_aborts(): pass -def test_edit_user_permissions_page_displays_redacted_phone_number_and_change_link( +def test_edit_user_permissions_page_displays_redacted_mobile_number_and_change_link( client_request, active_user_with_permissions, service_one, @@ -1027,29 +1027,46 @@ def test_edit_user_permissions_page_displays_redacted_phone_number_and_change_li ) assert user.name in page.find('h1').text - phone_number_paragraph = page.select('p[id=user_phone_number]')[0] - assert '0770****762' in phone_number_paragraph.text - change_link = phone_number_paragraph.findChild() - assert change_link.attrs['href'] == '/services/{}/users/{}/edit-phone-number'.format( + mobile_number_paragraph = page.select('p[id=user_mobile_number]')[0] + assert '0770****762' in mobile_number_paragraph.text + change_link = mobile_number_paragraph.findChild() + assert change_link.attrs['href'] == '/services/{}/users/{}/edit-mobile-number'.format( service_one['id'], user.id ) -def test_edit_user_phone_number_page(): +def test_edit_user_mobile_number_page( + client_request, + active_user_with_permissions, + service_one, + mocker +): + user = active_user_with_permissions + mocker.patch('app.user_api_client.get_user', return_value=user) + + page = client_request.get( + 'main.edit_user_mobile_number', + service_id=service_one['id'], + user_id=user.id + ) + + assert page.find('h1').text == "Change team member’s mobile number" + assert page.select('p[id=user_name]')[0].text == "This will change the mobile number for {}.".format(user.name) + assert page.select('input[name=mobile_number]')[0].attrs["value"] == "0770****762" + assert page.select('button[type=submit]')[0].text == "Save" + + +def test_edit_user_mobile_number_redirects_to_confirmation(): pass -def test_edit_user_phone_number_redirects_to_confirmation(): +def test_confirm_edit_user_mobile_number_page(): pass -def test_confirm_edit_user_phone_number_page(): +def test_confirm_edit_user_mobile_number_changes_user_mobile_number(): pass -def test_confirm_edit_user_phone_number_changes_user_mobile_number(): - pass - - -def test_confirm_edit_user_phone_number_with_no_permission_aborts(): +def test_confirm_edit_user_mobile_number_with_no_permission_aborts(): pass From 5a59989497da52d314e8428cc71ddf034bc92618 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 Feb 2019 12:28:18 +0000 Subject: [PATCH 3/7] Redirect to mobile number confirmation page --- app/main/views/manage_users.py | 11 ++++++++ .../views/manage-users/edit-user-mobile.html | 27 +++++++++++++++++++ tests/app/main/views/test_manage_users.py | 23 ++++++++++++++-- 3 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 app/templates/views/manage-users/edit-user-mobile.html diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 9cdba07fd..9896130f3 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -212,6 +212,10 @@ def edit_user_mobile_number(service_id, user_id): user_mobile_number = redact_mobile_number(user.mobile_number) form = ChangeMobileNumberForm(mobile_number=user_mobile_number) + if form.validate_on_submit(): + session['team_member_mobile_change'] = form.mobile_number.data + + return redirect(url_for('.confirm_edit_user_mobile_number', user_id=user.id, service_id=service_id)) return render_template( 'views/manage-users/edit-user-mobile.html', @@ -221,6 +225,13 @@ def edit_user_mobile_number(service_id, user_id): ) +@main.route("/services//users//edit-mobile-number/confirm", methods=['GET', 'POST']) +@login_required +@user_has_permissions('manage_service') +def confirm_edit_user_mobile_number(service_id, user_id): + return True + + @main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_service') def cancel_invited_user(service_id, invited_user_id): diff --git a/app/templates/views/manage-users/edit-user-mobile.html b/app/templates/views/manage-users/edit-user-mobile.html new file mode 100644 index 000000000..860ea416e --- /dev/null +++ b/app/templates/views/manage-users/edit-user-mobile.html @@ -0,0 +1,27 @@ +{% extends "withnav_template.html" %} +{% from "components/textbox.html" import textbox %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/form.html" import form_wrapper %} + +{% block per_page_title %} + Change team member’s mobile number +{% endblock %} + +{% block maincolumn_content %} + +

Change team member’s mobile number

+

This will change the mobile number for {{ user.name }}.

+
+
+ {% call form_wrapper() %} + {{ textbox(form.mobile_number) }} + {{ page_footer( + 'Save', + back_link=url_for('.edit_user_permissions', service_id=service_id, user_id=user.id), + back_link_text="Back" + ) }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index b41bb76f3..ff997a563 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1056,8 +1056,27 @@ def test_edit_user_mobile_number_page( assert page.select('button[type=submit]')[0].text == "Save" -def test_edit_user_mobile_number_redirects_to_confirmation(): - pass +def test_edit_user_mobile_number_redirects_to_confirmation( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, +): + + data = {'mobile_number': '07554080636'} + response = logged_in_client.post( + url_for( + 'main.edit_user_mobile_number', + service_id=service_one['id'], + user_id=active_user_with_permissions.id), data=data) + assert response.status_code == 302 + assert response.location == url_for( + 'main.confirm_edit_user_mobile_number', + service_id=service_one['id'], + user_id=active_user_with_permissions.id, + _external=True + ) def test_confirm_edit_user_mobile_number_page(): From 94b78a764949452cb1f7452700fc23f289433ec3 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 Feb 2019 15:06:42 +0000 Subject: [PATCH 4/7] Confirm change of team member's phone number --- app/main/views/manage_users.py | 10 ++++++- .../confirm-edit-user-mobile-number.html | 30 +++++++++++++++++++ tests/app/main/views/test_manage_users.py | 27 +++++++++++++++-- 3 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 app/templates/views/manage-users/confirm-edit-user-mobile-number.html diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index 9896130f3..d2c0ce5e4 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -229,7 +229,15 @@ def edit_user_mobile_number(service_id, user_id): @login_required @user_has_permissions('manage_service') def confirm_edit_user_mobile_number(service_id, user_id): - return True + user = user_api_client.get_user(user_id) + new_number = session['team_member_mobile_change'] + + return render_template( + 'views/manage-users/confirm-edit-user-mobile-number.html', + user=user, + service_id=service_id, + new_mobile_number=new_number + ) @main.route("/services//cancel-invited-user/", methods=['GET']) diff --git a/app/templates/views/manage-users/confirm-edit-user-mobile-number.html b/app/templates/views/manage-users/confirm-edit-user-mobile-number.html new file mode 100644 index 000000000..99265137e --- /dev/null +++ b/app/templates/views/manage-users/confirm-edit-user-mobile-number.html @@ -0,0 +1,30 @@ +{% extends "withnav_template.html" %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/form.html" import form_wrapper %} + +{% block per_page_title %} + Confirm change of mobile number +{% endblock %} + +{% block maincolumn_content %} + +

Confirm change of mobile number

+ +
+
+ {% call form_wrapper() %} +

New mobile number:

+
+

{{ new_mobile_number }}

+
+

We will send {{ user.name }} an email to tell them about the change.

+ {{ page_footer( + 'Confirm', + destructive=destructive, + back_link=url_for('.edit_user_mobile_number', service_id=service_id, user_id=user.id) + ) }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index ff997a563..7f558c85f 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1079,8 +1079,31 @@ def test_edit_user_mobile_number_redirects_to_confirmation( ) -def test_confirm_edit_user_mobile_number_page(): - pass +def test_confirm_edit_user_mobile_number_page( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, +): + new_number = '07554080636' + with logged_in_client.session_transaction() as session: + session['team_member_mobile_change'] = new_number + response = logged_in_client.get(url_for( + 'main.confirm_edit_user_mobile_number', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + )) + + assert response.status_code == 200 + assert 'Confirm change of mobile number' in response.get_data(as_text=True) + for text in [ + 'New mobile number:', + new_number, + 'We will send {} an email to tell them about the change.'.format(active_user_with_permissions.name) + ]: + assert text in response.get_data(as_text=True) + assert 'Confirm' in response.get_data(as_text=True) def test_confirm_edit_user_mobile_number_changes_user_mobile_number(): From f7e54b7f5b9e0c0600294f4938e6779beb0884f3 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 22 Feb 2019 16:01:04 +0000 Subject: [PATCH 5/7] Change team member's mobile number upon confirmation Also add edit user mobile number urls to navigation exceptions --- app/main/views/manage_users.py | 18 +++++++++++++++++- app/navigation.py | 12 ++++++++---- tests/app/main/views/test_manage_users.py | 22 ++++++++++++++++++++-- 3 files changed, 45 insertions(+), 7 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index d2c0ce5e4..bcaf87853 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -19,10 +19,10 @@ from app import ( from app.main import main from app.main.forms import ( ChangeEmailForm, + ChangeMobileNumberForm, InviteUserForm, PermissionsForm, SearchUsersForm, - ChangeMobileNumberForm ) from app.models.user import permissions from app.utils import redact_mobile_number, user_has_permissions @@ -231,6 +231,22 @@ def edit_user_mobile_number(service_id, user_id): def confirm_edit_user_mobile_number(service_id, user_id): user = user_api_client.get_user(user_id) new_number = session['team_member_mobile_change'] + if request.method == 'POST': + try: + user_api_client.update_user_attribute(user_id, mobile_number=new_number) + except HTTPError as e: + if e.status_code == 403: + flash("You don't have permission to edit users' mobile numbers for this service", 'info') + return redirect(url_for( + '.manage_users', + service_id=service_id)) + else: + abort(500, e) + + return redirect(url_for( + '.manage_users', + service_id=service_id + )) return render_template( 'views/manage-users/confirm-edit-user-mobile-number.html', diff --git a/app/navigation.py b/app/navigation.py index 7def9b505..e19042980 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -136,6 +136,7 @@ class HeaderNavigation(Navigation): 'choose_template_to_copy', 'confirm_edit_organisation_name', 'confirm_edit_user_email', + 'confirm_edit_user_mobile_number', 'confirm_redact_template', 'conversation', 'conversation_reply', @@ -159,7 +160,7 @@ class HeaderNavigation(Navigation): 'edit_template_postage', 'edit_user_org_permissions', 'edit_user_email', - 'edit_user_phone_number', + 'edit_user_mobile_number', 'edit_user_permissions', 'email_not_received', 'email_template', @@ -328,8 +329,9 @@ class MainNavigation(Navigation): }, 'team-members': { 'confirm_edit_user_email', + 'confirm_edit_user_mobile_number', 'edit_user_email', - 'edit_user_phone_number', + 'edit_user_mobile_number', 'edit_user_permissions', 'invite_user', 'manage_users', @@ -586,6 +588,7 @@ class CaseworkNavigation(Navigation): 'clear_cache', 'confirm_edit_organisation_name', 'confirm_edit_user_email', + 'confirm_edit_user_mobile_number', 'confirm_redact_template', 'conversation', 'conversation_reply', @@ -611,7 +614,7 @@ class CaseworkNavigation(Navigation): 'edit_service_template', 'edit_template_postage', 'edit_user_email', - 'edit_user_phone_number', + 'edit_user_mobile_number', 'edit_user_org_permissions', 'edit_user_permissions', 'email_branding', @@ -826,6 +829,7 @@ class OrgNavigation(Navigation): 'choose_template_to_copy', 'clear_cache', 'confirm_edit_user_email', + 'confirm_edit_user_mobile_number', 'confirm_redact_template', 'conversation', 'conversation_reply', @@ -850,7 +854,7 @@ class OrgNavigation(Navigation): 'edit_service_template', 'edit_template_postage', 'edit_user_email', - 'edit_user_phone_number', + 'edit_user_mobile_number', 'edit_user_permissions', 'email_branding', 'email_not_received', diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 7f558c85f..79f86eb0e 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1106,8 +1106,26 @@ def test_confirm_edit_user_mobile_number_page( assert 'Confirm' in response.get_data(as_text=True) -def test_confirm_edit_user_mobile_number_changes_user_mobile_number(): - pass +def test_confirm_edit_user_mobile_number_changes_user_mobile_number( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, + mock_update_user_attribute +): + new_number = '07554080636' + with logged_in_client.session_transaction() as session: + session['team_member_mobile_change'] = new_number + response = logged_in_client.post( + url_for( + 'main.confirm_edit_user_mobile_number', + service_id=service_one['id'], + user_id=active_user_with_permissions.id)) + assert response.status_code == 302 + assert response.location == url_for( + 'main.manage_users', service_id=service_one['id'], _external=True) + mock_update_user_attribute.assert_called_once_with(active_user_with_permissions.id, mobile_number=new_number) def test_confirm_edit_user_mobile_number_with_no_permission_aborts(): From d60ab838a86f9446dec03bee655b0ecb0c269ba7 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 25 Feb 2019 14:27:37 +0000 Subject: [PATCH 6/7] Edit mobile page allows users to proceed without changing mobile number Also: - change formatting of mobile number redact characters - redirect if session empty - update a test with new mock after rebase --- app/main/views/manage_users.py | 37 ++++++++-------- .../views/manage-users/edit-user-mobile.html | 2 +- app/utils.py | 5 ++- tests/app/main/views/test_manage_users.py | 44 ++++++++++++++++--- 4 files changed, 61 insertions(+), 27 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index bcaf87853..ffd7239ab 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -82,7 +82,7 @@ def edit_user_permissions(service_id, user_id): mobile_number = None if user.mobile_number: - mobile_number = redact_mobile_number(user.mobile_number) + mobile_number = redact_mobile_number(user.mobile_number, " ") form = PermissionsForm.from_user(user, service_id) @@ -182,15 +182,9 @@ def confirm_edit_user_email(service_id, user_id): try: user_api_client.update_user_attribute(str(user_id), email_address=new_email) except HTTPError as e: - if e.status_code == 403: - flash("You don't have permission to edit users emails for this service", 'info') - return redirect(url_for( - '.manage_users', - service_id=service_id)) - else: - abort(500, e) + abort(500, e) finally: - session.pop("team_member_email_change", None) + session.pop('team_member_email_change', None) return redirect(url_for( '.manage_users', @@ -212,11 +206,15 @@ def edit_user_mobile_number(service_id, user_id): user_mobile_number = redact_mobile_number(user.mobile_number) form = ChangeMobileNumberForm(mobile_number=user_mobile_number) + if form.mobile_number.data == user_mobile_number and request.method == 'POST': + return redirect(url_for( + '.manage_users', + service_id=service_id + )) if form.validate_on_submit(): session['team_member_mobile_change'] = form.mobile_number.data return redirect(url_for('.confirm_edit_user_mobile_number', user_id=user.id, service_id=service_id)) - return render_template( 'views/manage-users/edit-user-mobile.html', user=user, @@ -230,18 +228,21 @@ def edit_user_mobile_number(service_id, user_id): @user_has_permissions('manage_service') def confirm_edit_user_mobile_number(service_id, user_id): user = user_api_client.get_user(user_id) - new_number = session['team_member_mobile_change'] + if 'team_member_mobile_change' in session: + new_number = session['team_member_mobile_change'] + else: + return redirect(url_for( + '.edit_user_mobile_number', + service_id=service_id, + user_id=user_id + )) if request.method == 'POST': try: user_api_client.update_user_attribute(user_id, mobile_number=new_number) except HTTPError as e: - if e.status_code == 403: - flash("You don't have permission to edit users' mobile numbers for this service", 'info') - return redirect(url_for( - '.manage_users', - service_id=service_id)) - else: - abort(500, e) + abort(500, e) + finally: + session.pop('team_member_mobile_change', None) return redirect(url_for( '.manage_users', diff --git a/app/templates/views/manage-users/edit-user-mobile.html b/app/templates/views/manage-users/edit-user-mobile.html index 860ea416e..08cadbc61 100644 --- a/app/templates/views/manage-users/edit-user-mobile.html +++ b/app/templates/views/manage-users/edit-user-mobile.html @@ -13,7 +13,7 @@

This will change the mobile number for {{ user.name }}.

- {% call form_wrapper() %} + {% call form_wrapper(class="extra-tracking") %} {{ textbox(form.mobile_number) }} {{ page_footer( 'Save', diff --git a/app/utils.py b/app/utils.py index d29f21f63..d0da4e6c3 100644 --- a/app/utils.py +++ b/app/utils.py @@ -673,9 +673,10 @@ def printing_today_or_tomorrow(): return 'tomorrow' -def redact_mobile_number(mobile_number): +def redact_mobile_number(mobile_number, spacing=""): indices = [-4, -5, -6, -7] + redact_character = spacing + "•" + spacing mobile_number_list = list(mobile_number.replace(" ", "")) for i in indices: - mobile_number_list[i] = "*" + mobile_number_list[i] = redact_character return "".join(mobile_number_list) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 79f86eb0e..f27e52982 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1014,6 +1014,7 @@ def test_confirm_edit_user_email_with_no_permission_aborts(): def test_edit_user_permissions_page_displays_redacted_mobile_number_and_change_link( client_request, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker ): @@ -1028,7 +1029,7 @@ def test_edit_user_permissions_page_displays_redacted_mobile_number_and_change_l assert user.name in page.find('h1').text mobile_number_paragraph = page.select('p[id=user_mobile_number]')[0] - assert '0770****762' in mobile_number_paragraph.text + assert '0770 • • • • 762' in mobile_number_paragraph.text change_link = mobile_number_paragraph.findChild() assert change_link.attrs['href'] == '/services/{}/users/{}/edit-mobile-number'.format( service_one['id'], user.id @@ -1052,7 +1053,7 @@ def test_edit_user_mobile_number_page( assert page.find('h1').text == "Change team member’s mobile number" assert page.select('p[id=user_name]')[0].text == "This will change the mobile number for {}.".format(user.name) - assert page.select('input[name=mobile_number]')[0].attrs["value"] == "0770****762" + assert page.select('input[name=mobile_number]')[0].attrs["value"] == "0770••••762" assert page.select('button[type=submit]')[0].text == "Save" @@ -1079,6 +1080,25 @@ def test_edit_user_mobile_number_redirects_to_confirmation( ) +def test_edit_user_mobile_number_redirects_to_manage_users_if_number_not_changed( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, +): + + data = {'mobile_number': '0770••••762'} + response = logged_in_client.post( + url_for( + 'main.edit_user_mobile_number', + service_id=service_one['id'], + user_id=active_user_with_permissions.id), data=data) + assert response.status_code == 302 + assert response.location == url_for( + 'main.manage_users', service_id=service_one['id'], _external=True) + + def test_confirm_edit_user_mobile_number_page( logged_in_client, active_user_with_permissions, @@ -1106,6 +1126,22 @@ def test_confirm_edit_user_mobile_number_page( assert 'Confirm' in response.get_data(as_text=True) +def test_confirm_edit_user_mobile_number_page_redirects_if_session_empty( + logged_in_client, + active_user_with_permissions, + service_one, + mocker, + mock_get_user, +): + response = logged_in_client.get(url_for( + 'main.confirm_edit_user_mobile_number', + service_id=service_one['id'], + user_id=active_user_with_permissions.id + )) + assert response.status_code == 302 + assert 'Confirm change of mobile number' not in response.get_data(as_text=True) + + def test_confirm_edit_user_mobile_number_changes_user_mobile_number( logged_in_client, active_user_with_permissions, @@ -1126,7 +1162,3 @@ def test_confirm_edit_user_mobile_number_changes_user_mobile_number( assert response.location == url_for( 'main.manage_users', service_id=service_one['id'], _external=True) mock_update_user_attribute.assert_called_once_with(active_user_with_permissions.id, mobile_number=new_number) - - -def test_confirm_edit_user_mobile_number_with_no_permission_aborts(): - pass From d93278f5f057ba2144d962d1bae8044a43b9ece7 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 26 Feb 2019 11:47:15 +0000 Subject: [PATCH 7/7] Ensure that mobile of user not belonging to service cannot be edited --- app/main/views/manage_users.py | 10 +++++----- tests/app/main/views/test_manage_users.py | 24 +++++++++++++++++++---- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index ffd7239ab..dd976d3d0 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -198,11 +198,11 @@ def confirm_edit_user_email(service_id, user_id): ) -@main.route("/services//users//edit-mobile-number", methods=['GET', 'POST']) +@main.route("/services//users//edit-mobile-number", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') def edit_user_mobile_number(service_id, user_id): - user = user_api_client.get_user(user_id) + user = current_service.get_team_member(user_id) user_mobile_number = redact_mobile_number(user.mobile_number) form = ChangeMobileNumberForm(mobile_number=user_mobile_number) @@ -223,11 +223,11 @@ def edit_user_mobile_number(service_id, user_id): ) -@main.route("/services//users//edit-mobile-number/confirm", methods=['GET', 'POST']) +@main.route("/services//users//edit-mobile-number/confirm", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') def confirm_edit_user_mobile_number(service_id, user_id): - user = user_api_client.get_user(user_id) + user = current_service.get_team_member(user_id) if 'team_member_mobile_change' in session: new_number = session['team_member_mobile_change'] else: @@ -238,7 +238,7 @@ def confirm_edit_user_mobile_number(service_id, user_id): )) if request.method == 'POST': try: - user_api_client.update_user_attribute(user_id, mobile_number=new_number) + user_api_client.update_user_attribute(str(user_id), mobile_number=new_number) except HTTPError as e: abort(500, e) finally: diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index f27e52982..d50910c62 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1007,10 +1007,6 @@ def test_confirm_edit_user_email_doesnt_change_user_email_for_non_team_member( ) -def test_confirm_edit_user_email_with_no_permission_aborts(): - pass - - def test_edit_user_permissions_page_displays_redacted_mobile_number_and_change_link( client_request, active_user_with_permissions, @@ -1039,6 +1035,7 @@ def test_edit_user_permissions_page_displays_redacted_mobile_number_and_change_l def test_edit_user_mobile_number_page( client_request, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker ): @@ -1060,6 +1057,7 @@ def test_edit_user_mobile_number_page( def test_edit_user_mobile_number_redirects_to_confirmation( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -1083,6 +1081,7 @@ def test_edit_user_mobile_number_redirects_to_confirmation( def test_edit_user_mobile_number_redirects_to_manage_users_if_number_not_changed( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -1102,6 +1101,7 @@ def test_edit_user_mobile_number_redirects_to_manage_users_if_number_not_changed def test_confirm_edit_user_mobile_number_page( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -1129,6 +1129,7 @@ def test_confirm_edit_user_mobile_number_page( def test_confirm_edit_user_mobile_number_page_redirects_if_session_empty( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -1145,6 +1146,7 @@ def test_confirm_edit_user_mobile_number_page_redirects_if_session_empty( def test_confirm_edit_user_mobile_number_changes_user_mobile_number( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -1162,3 +1164,17 @@ def test_confirm_edit_user_mobile_number_changes_user_mobile_number( assert response.location == url_for( 'main.manage_users', service_id=service_one['id'], _external=True) mock_update_user_attribute.assert_called_once_with(active_user_with_permissions.id, mobile_number=new_number) + + +def test_confirm_edit_user_mobile_number_doesnt_change_user_mobile_for_non_team_member( + client_request, + mock_get_users_by_service, +): + with client_request.session_transaction() as session: + session['team_member_mobile_change'] = '07554080636' + client_request.post( + 'main.confirm_edit_user_mobile_number', + service_id=SERVICE_ONE_ID, + user_id=USER_ONE_ID, + _expected_status=404, + )