From 7b02cb72c6d7d6a31446a789a9af2359e1fd149f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 13 Jun 2019 19:00:17 +0100 Subject: [PATCH 1/3] add option to suppress platform admin temporarily so that platform admins (us) can view pages as regular users do easily. Simply adds a flag in the session cookie that overrides the actual platform admin flag on the user model if set. This way it's safe, since this only downgrades existing functionality, so if someone managed to alter it they could only get less permissions, not more. You can change this value from the user profile page if either: * you're a platform admin * the flag is set (to any value) on the cookie. This slightly weird check means that we don't check the underlying `user._platform_admin` flag anywhere in the code, even when toggling the suppression. --- app/main/views/user_profile.py | 26 ++++++++- app/models/user.py | 5 ++ app/templates/views/user-profile.html | 8 +++ .../user-profile/suppress-platform-admin.html | 27 +++++++++ tests/app/main/views/test_user_profile.py | 58 +++++++++++++++++++ tests/app/models/test_user.py | 21 ++++++- tests/conftest.py | 6 +- 7 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 app/templates/views/user-profile/suppress-platform-admin.html diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index c52b41f86..d69398102 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -1,6 +1,6 @@ import json -from flask import current_app, redirect, render_template, session, url_for +from flask import current_app, redirect, render_template, session, url_for, abort from flask_login import current_user, login_required from notifications_utils.url_safe_token import check_token @@ -12,6 +12,7 @@ from app.main.forms import ( ChangeNameForm, ChangePasswordForm, ConfirmPasswordForm, + ServiceOnOffSettingForm, TwoFactorForm, ) from app.models.user import User @@ -192,3 +193,26 @@ def user_profile_password(): 'views/user-profile/change-password.html', form=form ) + + +@main.route("/user-profile/suppress-platform-admin", methods=['GET', 'POST']) +@login_required +def user_profile_suppress_platform_admin(): + if not current_user.platform_admin and not session.get('suppress_platform_admin'): + abort(403) + + form = ServiceOnOffSettingForm( + name="This setting will be cleared if you sign out and sign in again", + enabled=session.get('suppress_platform_admin', False), + truthy='Yes (view as regular user)', + falsey='No (view as platform admin)', + ) + + if form.validate_on_submit(): + session['suppress_platform_admin'] = form.enabled.data + return redirect(url_for('.user_profile')) + + return render_template( + 'views/user-profile/suppress-platform-admin.html', + form=form + ) diff --git a/app/models/user.py b/app/models/user.py index 087157795..10921dee4 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -49,6 +49,7 @@ class User(JSONModel, UserMixin): super().__init__(_dict) self.permissions = _dict.get('permissions', {}) self.max_failed_login_count = current_app.config['MAX_FAILED_LOGIN_COUNT'] + self._platform_admin = _dict['platform_admin'] @classmethod def from_id(cls, user_id): @@ -173,6 +174,10 @@ class User(JSONModel, UserMixin): super(User, self).is_authenticated ) + @property + def platform_admin(self): + return self._platform_admin and not session.get('suppress_platform_admin', False) + def has_permissions(self, *permissions, restrict_admin_usage=False): unknown_permissions = set(permissions) - all_permissions if unknown_permissions: diff --git a/app/templates/views/user-profile.html b/app/templates/views/user-profile.html index 24f3cdc02..b259a2d93 100644 --- a/app/templates/views/user-profile.html +++ b/app/templates/views/user-profile.html @@ -45,6 +45,14 @@ {{ edit_field('Change', url_for('.user_profile_password')) }} {% endcall %} + {% if current_user.platform_admin or session.get('suppress_platform_admin') %} + {% call row() %} + {{ text_field('Suppress Platform Admin') }} + {{ text_field('On' if session.get('suppress_platform_admin') else 'Off') }} + {{ edit_field('Change', url_for('.user_profile_suppress_platform_admin')) }} + {% endcall %} + {% endif %} + {% endcall %} diff --git a/app/templates/views/user-profile/suppress-platform-admin.html b/app/templates/views/user-profile/suppress-platform-admin.html new file mode 100644 index 000000000..a579f001c --- /dev/null +++ b/app/templates/views/user-profile/suppress-platform-admin.html @@ -0,0 +1,27 @@ +{% extends "withoutnav_template.html" %} +{% from "components/form.html" import form_wrapper %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/page-header.html" import page_header %} +{% from "components/radios.html" import radios %} + +{% block per_page_title %} + Suppress platform admin view +{% endblock %} + +{% block maincolumn_content %} + +
+
+ {{ page_header( + 'Suppress platform admin', + back_link=url_for('.user_profile') + ) }} + + {% call form_wrapper() %} + {{ radios(form.enabled) }} + {{ page_footer('Save') }} + {% endcall %} +
+
+ +{% endblock %} diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index 519eba426..f77756e7c 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -14,6 +14,18 @@ def test_should_show_overview_page( ): page = client_request.get(('main.user_profile')) assert page.select_one('h1').text.strip() == 'Your profile' + assert 'Suppress Platform Admin' not in page + + +def test_overview_page_shows_suppress_for_platform_admin( + client_request, + platform_admin_user +): + client_request.login(platform_admin_user) + page = client_request.get(('main.user_profile')) + assert page.select_one('h1').text.strip() == 'Your profile' + suppress_platform_admin_row = page.select('tr')[-1] + assert ' '.join(suppress_platform_admin_row.text.split()) == 'Suppress Platform Admin Off Change' def test_should_show_name_page( @@ -264,3 +276,49 @@ def test_non_gov_user_cannot_access_change_email_page( ): client_request.login(api_nongov_user_active) client_request.get('main.user_profile_email', _expected_status=403) + + +def test_normal_user_doesnt_see_suppress_platform_admin(client_request): + client_request.get('main.user_profile_suppress_platform_admin', _expected_status=403) + + +def test_platform_admin_can_see_suppress_platform_admin_page(client_request, platform_admin_user): + client_request.login(platform_admin_user) + page = client_request.get('main.user_profile_suppress_platform_admin') + + assert page.select_one('h1').text.strip() == 'Suppress platform admin' + assert page.select_one('input[checked]')['value'] == 'False' + + +def test_can_suppress_platform_admin(client_request, platform_admin_user): + client_request.login(platform_admin_user) + + with client_request.session_transaction() as session: + assert 'suppress_platform_admin' not in session + + client_request.post( + 'main.user_profile_suppress_platform_admin', + _data={'enabled': True}, + _expected_status=302, + _expected_redirect=url_for('main.user_profile', _external=True), + ) + + with client_request.session_transaction() as session: + assert session['suppress_platform_admin'] is True + + +def test_can_turn_off_suppress_platform_admin(client_request, platform_admin_user): + client_request.login(platform_admin_user) + + with client_request.session_transaction() as session: + session['suppress_platform_admin'] = True + + client_request.post( + 'main.user_profile_suppress_platform_admin', + _data={'enabled': False}, + _expected_status=302, + _expected_redirect=url_for('main.user_profile', _external=True), + ) + + with client_request.session_transaction() as session: + assert session['suppress_platform_admin'] is False diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index 92fabc0af..bae3189bd 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -9,7 +9,8 @@ def test_user(app_): 'email_address': 'test@user.gov.uk', 'mobile_number': '+4412341234', 'state': 'pending', - 'failed_login_count': 0 + 'failed_login_count': 0, + 'platform_admin': False, } user = User(user_data) @@ -40,3 +41,21 @@ def test_activate_user(app_, api_user_pending, mock_activate_user): def test_activate_user_already_active(app_, api_user_active, mock_activate_user): assert User(api_user_active).activate() == User(api_user_active) assert mock_activate_user.called is False + + +@pytest.mark.parametrize('is_platform_admin, value_in_session, expected_result', [ + (True, True, False), + (True, False, True), + (True, None, True), + (False, True, False), + (False, False, False), + (False, None, False), +]) +def test_platform_admin_checks_flag_set_in_session(client, mocker, is_platform_admin, value_in_session, expected_result): + session_dict = {} + if value_in_session is not None: + session_dict['suppress_platform_admin'] = value_in_session + + mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) + + assert User({'platform_admin': is_platform_admin}).platform_admin == expected_result diff --git a/tests/conftest.py b/tests/conftest.py index aa65d3cf4..33efce64a 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1091,6 +1091,7 @@ def api_user_pending(fake_uuid): 'permissions': {}, 'organisations': [], 'current_session_id': None, + 'password_changed_at': str(datetime.utcnow()), } return user_data @@ -1114,6 +1115,7 @@ def platform_admin_user(fake_uuid): 'view_activity']}, 'platform_admin': True, 'auth_type': 'sms_auth', + 'password_changed_at': str(datetime.utcnow()), 'services': [], 'organisations': [], 'current_session_id': None, @@ -1443,7 +1445,7 @@ def api_user_request_password_reset(fake_uuid): 'state': 'active', 'failed_login_count': 5, 'permissions': {}, - 'password_changed_at': None, + 'password_changed_at': str(datetime.utcnow()), 'auth_type': 'sms_auth', 'organisations': [], 'current_session_id': None, @@ -2165,7 +2167,7 @@ def mock_get_users_by_service(mocker): 'manage_settings', 'manage_api_keys']}, 'state': 'active', - 'password_changed_at': None, + 'password_changed_at': str(datetime.utcnow()), 'name': 'Test User', 'email_address': 'notify@digital.cabinet-office.gov.uk', 'auth_type': 'sms_auth', From c724f84c2354a374719fde97d64d6390eee8bdf5 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 14 Jun 2019 12:32:47 +0100 Subject: [PATCH 2/3] change wording of platform admin toggle to positive rather than negative CHS Approved Wording :+1: also rename suppress_platform_admin -> disable_platform_admin_view in the backend, as suppress is a kinda weird word. --- app/main/views/user_profile.py | 27 +++++--- app/models/user.py | 2 +- app/templates/views/user-profile.html | 8 +-- ....html => disable-platform-admin-view.html} | 4 +- tests/app/main/views/test_user_profile.py | 62 +++++++++---------- tests/app/models/test_user.py | 4 +- 6 files changed, 57 insertions(+), 50 deletions(-) rename app/templates/views/user-profile/{suppress-platform-admin.html => disable-platform-admin-view.html} (90%) diff --git a/app/main/views/user_profile.py b/app/main/views/user_profile.py index d69398102..f136c61b7 100644 --- a/app/main/views/user_profile.py +++ b/app/main/views/user_profile.py @@ -1,6 +1,13 @@ import json -from flask import current_app, redirect, render_template, session, url_for, abort +from flask import ( + abort, + current_app, + redirect, + render_template, + session, + url_for, +) from flask_login import current_user, login_required from notifications_utils.url_safe_token import check_token @@ -195,24 +202,24 @@ def user_profile_password(): ) -@main.route("/user-profile/suppress-platform-admin", methods=['GET', 'POST']) +@main.route("/user-profile/disable-platform-admin-view", methods=['GET', 'POST']) @login_required -def user_profile_suppress_platform_admin(): - if not current_user.platform_admin and not session.get('suppress_platform_admin'): +def user_profile_disable_platform_admin_view(): + if not current_user.platform_admin and not session.get('disable_platform_admin_view'): abort(403) form = ServiceOnOffSettingForm( - name="This setting will be cleared if you sign out and sign in again", - enabled=session.get('suppress_platform_admin', False), - truthy='Yes (view as regular user)', - falsey='No (view as platform admin)', + name="Signing in again clears this setting", + enabled=not session.get('disable_platform_admin_view'), + truthy='Yes', + falsey='No', ) if form.validate_on_submit(): - session['suppress_platform_admin'] = form.enabled.data + session['disable_platform_admin_view'] = not form.enabled.data return redirect(url_for('.user_profile')) return render_template( - 'views/user-profile/suppress-platform-admin.html', + 'views/user-profile/disable-platform-admin-view.html', form=form ) diff --git a/app/models/user.py b/app/models/user.py index 10921dee4..2fa83b318 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -176,7 +176,7 @@ class User(JSONModel, UserMixin): @property def platform_admin(self): - return self._platform_admin and not session.get('suppress_platform_admin', False) + return self._platform_admin and not session.get('disable_platform_admin_view', False) def has_permissions(self, *permissions, restrict_admin_usage=False): unknown_permissions = set(permissions) - all_permissions diff --git a/app/templates/views/user-profile.html b/app/templates/views/user-profile.html index b259a2d93..a39853c32 100644 --- a/app/templates/views/user-profile.html +++ b/app/templates/views/user-profile.html @@ -45,11 +45,11 @@ {{ edit_field('Change', url_for('.user_profile_password')) }} {% endcall %} - {% if current_user.platform_admin or session.get('suppress_platform_admin') %} + {% if current_user.platform_admin or session.get('disable_platform_admin_view') %} {% call row() %} - {{ text_field('Suppress Platform Admin') }} - {{ text_field('On' if session.get('suppress_platform_admin') else 'Off') }} - {{ edit_field('Change', url_for('.user_profile_suppress_platform_admin')) }} + {{ text_field('Use platform admin view') }} + {{ text_field('Yes' if not session.get('disable_platform_admin_view') else 'No') }} + {{ edit_field('Change', url_for('.user_profile_disable_platform_admin_view')) }} {% endcall %} {% endif %} diff --git a/app/templates/views/user-profile/suppress-platform-admin.html b/app/templates/views/user-profile/disable-platform-admin-view.html similarity index 90% rename from app/templates/views/user-profile/suppress-platform-admin.html rename to app/templates/views/user-profile/disable-platform-admin-view.html index a579f001c..334de0b13 100644 --- a/app/templates/views/user-profile/suppress-platform-admin.html +++ b/app/templates/views/user-profile/disable-platform-admin-view.html @@ -5,7 +5,7 @@ {% from "components/radios.html" import radios %} {% block per_page_title %} - Suppress platform admin view + Use platform admin view {% endblock %} {% block maincolumn_content %} @@ -13,7 +13,7 @@
{{ page_header( - 'Suppress platform admin', + 'Use platform admin view', back_link=url_for('.user_profile') ) }} diff --git a/tests/app/main/views/test_user_profile.py b/tests/app/main/views/test_user_profile.py index f77756e7c..afd0f9a79 100644 --- a/tests/app/main/views/test_user_profile.py +++ b/tests/app/main/views/test_user_profile.py @@ -14,18 +14,18 @@ def test_should_show_overview_page( ): page = client_request.get(('main.user_profile')) assert page.select_one('h1').text.strip() == 'Your profile' - assert 'Suppress Platform Admin' not in page + assert 'Use platform admin view' not in page -def test_overview_page_shows_suppress_for_platform_admin( +def test_overview_page_shows_disable_for_platform_admin( client_request, platform_admin_user ): client_request.login(platform_admin_user) page = client_request.get(('main.user_profile')) assert page.select_one('h1').text.strip() == 'Your profile' - suppress_platform_admin_row = page.select('tr')[-1] - assert ' '.join(suppress_platform_admin_row.text.split()) == 'Suppress Platform Admin Off Change' + disable_platform_admin_row = page.select('tr')[-1] + assert ' '.join(disable_platform_admin_row.text.split()) == 'Use platform admin view Yes Change' def test_should_show_name_page( @@ -278,47 +278,47 @@ def test_non_gov_user_cannot_access_change_email_page( client_request.get('main.user_profile_email', _expected_status=403) -def test_normal_user_doesnt_see_suppress_platform_admin(client_request): - client_request.get('main.user_profile_suppress_platform_admin', _expected_status=403) +def test_normal_user_doesnt_see_disable_platform_admin(client_request): + client_request.get('main.user_profile_disable_platform_admin_view', _expected_status=403) -def test_platform_admin_can_see_suppress_platform_admin_page(client_request, platform_admin_user): +def test_platform_admin_can_see_disable_platform_admin_page(client_request, platform_admin_user): client_request.login(platform_admin_user) - page = client_request.get('main.user_profile_suppress_platform_admin') + page = client_request.get('main.user_profile_disable_platform_admin_view') - assert page.select_one('h1').text.strip() == 'Suppress platform admin' - assert page.select_one('input[checked]')['value'] == 'False' + assert page.select_one('h1').text.strip() == 'Use platform admin view' + assert page.select_one('input[checked]')['value'] == 'True' -def test_can_suppress_platform_admin(client_request, platform_admin_user): +def test_can_disable_platform_admin(client_request, platform_admin_user): client_request.login(platform_admin_user) with client_request.session_transaction() as session: - assert 'suppress_platform_admin' not in session + assert 'disable_platform_admin_view' not in session client_request.post( - 'main.user_profile_suppress_platform_admin', - _data={'enabled': True}, - _expected_status=302, - _expected_redirect=url_for('main.user_profile', _external=True), - ) - - with client_request.session_transaction() as session: - assert session['suppress_platform_admin'] is True - - -def test_can_turn_off_suppress_platform_admin(client_request, platform_admin_user): - client_request.login(platform_admin_user) - - with client_request.session_transaction() as session: - session['suppress_platform_admin'] = True - - client_request.post( - 'main.user_profile_suppress_platform_admin', + 'main.user_profile_disable_platform_admin_view', _data={'enabled': False}, _expected_status=302, _expected_redirect=url_for('main.user_profile', _external=True), ) with client_request.session_transaction() as session: - assert session['suppress_platform_admin'] is False + assert session['disable_platform_admin_view'] is True + + +def test_can_reenable_platform_admin(client_request, platform_admin_user): + client_request.login(platform_admin_user) + + with client_request.session_transaction() as session: + session['disable_platform_admin_view'] = True + + client_request.post( + 'main.user_profile_disable_platform_admin_view', + _data={'enabled': True}, + _expected_status=302, + _expected_redirect=url_for('main.user_profile', _external=True), + ) + + with client_request.session_transaction() as session: + assert session['disable_platform_admin_view'] is False diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index bae3189bd..edc69b51e 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -51,10 +51,10 @@ def test_activate_user_already_active(app_, api_user_active, mock_activate_user) (False, False, False), (False, None, False), ]) -def test_platform_admin_checks_flag_set_in_session(client, mocker, is_platform_admin, value_in_session, expected_result): +def test_platform_admin_flag_set_in_session(client, mocker, is_platform_admin, value_in_session, expected_result): session_dict = {} if value_in_session is not None: - session_dict['suppress_platform_admin'] = value_in_session + session_dict['disable_platform_admin_view'] = value_in_session mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) From d752a6335f63f53db7879281284162701470a505 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 14 Jun 2019 15:14:00 +0100 Subject: [PATCH 3/3] make sure tests all populate the platform admin field it's always returned by the API --- app/navigation.py | 4 ++++ tests/conftest.py | 7 ++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/app/navigation.py b/app/navigation.py index 2a2366d63..63b6e2fa4 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -77,6 +77,7 @@ class HeaderNavigation(Navigation): 'user_profile_mobile_number_confirm', 'user_profile_name', 'user_profile_password', + 'user_profile_disable_platform_admin_view', }, 'platform-admin': { 'add_organisation', @@ -571,6 +572,7 @@ class MainNavigation(Navigation): 'user_profile_mobile_number_confirm', 'user_profile_name', 'user_profile_password', + 'user_profile_disable_platform_admin_view', 'using_notify', 'verify', 'verify_email', @@ -838,6 +840,7 @@ class CaseworkNavigation(Navigation): 'user_profile_mobile_number_confirm', 'user_profile_name', 'user_profile_password', + 'user_profile_disable_platform_admin_view', 'using_notify', 'verify', 'verify_email', @@ -1106,6 +1109,7 @@ class OrgNavigation(Navigation): 'user_profile_mobile_number_confirm', 'user_profile_name', 'user_profile_password', + 'user_profile_disable_platform_admin_view', 'using_notify', 'verify', 'verify_email', diff --git a/tests/conftest.py b/tests/conftest.py index 33efce64a..1e8b8fa68 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1431,6 +1431,7 @@ def api_user_locked(fake_uuid): 'auth_type': 'sms_auth', 'organisations': [], 'current_session_id': None, + 'platform_admin': False, } return user_data @@ -1449,6 +1450,7 @@ def api_user_request_password_reset(fake_uuid): 'auth_type': 'sms_auth', 'organisations': [], 'current_session_id': None, + 'platform_admin': False, } return user_data @@ -1467,6 +1469,7 @@ def api_user_changed_password(fake_uuid): 'password_changed_at': str(datetime.utcnow() + timedelta(minutes=1)), 'organisations': [], 'current_session_id': None, + 'platform_admin': False, } return user_data @@ -2172,7 +2175,9 @@ def mock_get_users_by_service(mocker): 'email_address': 'notify@digital.cabinet-office.gov.uk', 'auth_type': 'sms_auth', 'failed_login_count': 0, - 'organisations': []}] + 'organisations': [], + 'platform_admin': False, + }] return [data[0]] # You shouldn’t be calling the user API client directly, so it’s the