From 7b02cb72c6d7d6a31446a789a9af2359e1fd149f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 13 Jun 2019 19:00:17 +0100 Subject: [PATCH] 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',