From 84d4c1e0b5d0f234f381bb4557acfd43e53ef9e5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 10:38:34 +0100 Subject: [PATCH 01/10] Remove catching of exception that is never raised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit View functions won’t raise an `Unauthorized` exception, they will return a redirect to the login page instead. --- tests/app/utils/test_user.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index d9811e71c..129a9e955 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -1,6 +1,6 @@ import pytest from flask import request -from werkzeug.exceptions import Forbidden, Unauthorized +from werkzeug.exceptions import Forbidden from app.main.views.index import index from app.utils.user import user_has_permissions @@ -30,7 +30,7 @@ def _test_permissions( response.status_code == 302 ): pytest.fail("Failed to throw a forbidden or unauthorised exception") - except (Forbidden, Unauthorized): + except Forbidden: pass From 684fc5057a11f35228acaa77e68ca348687ca3ae Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 10:46:23 +0100 Subject: [PATCH 02/10] Move non-success handling out of helper The helper function handles both tests that pass, and tests that are expected to fail (either by raising an exception or returning a redirect to the login page). By moving the handling of cases which are expected to fail out of the helper function we can make the helper function less complex, which will make further refactoring easier. --- tests/app/utils/test_user.py | 58 ++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 33 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index 129a9e955..4c3296085 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -10,7 +10,7 @@ def _test_permissions( client_request, usr, permissions, - will_succeed, + will_succeed=True, kwargs=None, ): request.view_args.update({'service_id': 'foo'}) @@ -19,19 +19,7 @@ def _test_permissions( decorator = user_has_permissions(*permissions, **(kwargs or {})) decorated_index = decorator(index) - - if will_succeed: - decorated_index() - else: - try: - response = decorated_index() - if not ( - response.location.startswith('/sign-in?next=') and - response.status_code == 302 - ): - pytest.fail("Failed to throw a forbidden or unauthorised exception") - except Forbidden: - pass + return decorated_index() def test_user_has_permissions_on_endpoint_fail( @@ -41,11 +29,12 @@ def test_user_has_permissions_on_endpoint_fail( ): user = _user_with_permissions() mocker.patch('app.user_api_client.get_user', return_value=user) - _test_permissions( - client_request, - user, - ['send_messages'], - will_succeed=False) + with pytest.raises(Forbidden): + _test_permissions( + client_request, + user, + ['send_messages'], + ) def test_user_has_permissions_success( @@ -58,7 +47,7 @@ def test_user_has_permissions_success( client_request, user, ['manage_service'], - will_succeed=True) + ) def test_user_has_permissions_or( @@ -71,7 +60,7 @@ def test_user_has_permissions_or( client_request, user, ['send_messages', 'manage_service'], - will_succeed=True) + ) def test_user_has_permissions_multiple( @@ -84,7 +73,7 @@ def test_user_has_permissions_multiple( client_request, user, ['manage_templates', 'manage_service'], - will_succeed=True) + ) def test_exact_permissions( @@ -97,7 +86,7 @@ def test_exact_permissions( client_request, user, ['manage_service', 'manage_templates'], - will_succeed=True) + ) def test_platform_admin_user_can_access_page_that_has_no_permissions( @@ -110,7 +99,7 @@ def test_platform_admin_user_can_access_page_that_has_no_permissions( client_request, platform_admin_user, [], - will_succeed=True) + ) def test_platform_admin_user_can_not_access_page( @@ -120,23 +109,26 @@ def test_platform_admin_user_can_not_access_page( mock_get_service, ): mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) - _test_permissions( - client_request, - platform_admin_user, - [], - will_succeed=False, - kwargs={'restrict_admin_usage': True}) + with pytest.raises(Forbidden): + _test_permissions( + client_request, + platform_admin_user, + [], + kwargs={'restrict_admin_usage': True}, + ) -def test_no_user_returns_401_unauth( +def test_no_user_returns_redirect_to_sign_in( client_request ): client_request.logout() - _test_permissions( + response = _test_permissions( client_request, None, [], - will_succeed=False) + ) + assert response.status_code == 302 + assert response.location.startswith('/sign-in?next=') def test_user_has_permissions_for_organisation( From 15131d003b10633910e251a22144705a79286ae4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 10:54:38 +0100 Subject: [PATCH 03/10] Let `client_request` do user mocking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `client_request.login` already mocks calls to get the current user, so we don’t need to do it manually. --- tests/app/utils/test_user.py | 52 +++++++----------------------------- 1 file changed, 10 insertions(+), 42 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index 4c3296085..f6ee7f3cc 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -10,12 +10,10 @@ def _test_permissions( client_request, usr, permissions, - will_succeed=True, kwargs=None, ): request.view_args.update({'service_id': 'foo'}) - if usr: - client_request.login(usr) + client_request.login(usr) decorator = user_has_permissions(*permissions, **(kwargs or {})) decorated_index = decorator(index) @@ -24,15 +22,12 @@ def _test_permissions( def test_user_has_permissions_on_endpoint_fail( client_request, - mocker, mock_get_service, ): - user = _user_with_permissions() - mocker.patch('app.user_api_client.get_user', return_value=user) with pytest.raises(Forbidden): _test_permissions( client_request, - user, + _user_with_permissions(), ['send_messages'], ) @@ -41,50 +36,39 @@ def test_user_has_permissions_success( client_request, mocker, ): - user = _user_with_permissions() - mocker.patch('app.user_api_client.get_user', return_value=user) _test_permissions( client_request, - user, + _user_with_permissions(), ['manage_service'], ) def test_user_has_permissions_or( client_request, - mocker, ): - user = _user_with_permissions() - mocker.patch('app.user_api_client.get_user', return_value=user) _test_permissions( client_request, - user, + _user_with_permissions(), ['send_messages', 'manage_service'], ) def test_user_has_permissions_multiple( client_request, - mocker, ): - user = _user_with_permissions() - mocker.patch('app.user_api_client.get_user', return_value=user) _test_permissions( client_request, - user, + _user_with_permissions(), ['manage_templates', 'manage_service'], ) def test_exact_permissions( client_request, - mocker, ): - user = _user_with_permissions() - mocker.patch('app.user_api_client.get_user', return_value=user) _test_permissions( client_request, - user, + _user_with_permissions(), ['manage_service', 'manage_templates'], ) @@ -92,9 +76,7 @@ def test_exact_permissions( def test_platform_admin_user_can_access_page_that_has_no_permissions( client_request, platform_admin_user, - mocker, ): - mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) _test_permissions( client_request, platform_admin_user, @@ -105,10 +87,8 @@ def test_platform_admin_user_can_access_page_that_has_no_permissions( def test_platform_admin_user_can_not_access_page( client_request, platform_admin_user, - mocker, mock_get_service, ): - mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) with pytest.raises(Forbidden): _test_permissions( client_request, @@ -122,22 +102,18 @@ def test_no_user_returns_redirect_to_sign_in( client_request ): client_request.logout() - response = _test_permissions( - client_request, - None, - [], - ) + decorator = user_has_permissions() + decorated_index = decorator(index) + response = decorated_index() assert response.status_code == 302 assert response.location.startswith('/sign-in?next=') def test_user_has_permissions_for_organisation( client_request, - mocker, ): user = _user_with_permissions() user['organisations'] = ['org_1', 'org_2'] - mocker.patch('app.user_api_client.get_user', return_value=user) client_request.login(user) request.view_args = {'org_id': 'org_2'} @@ -152,10 +128,8 @@ def test_user_has_permissions_for_organisation( def test_platform_admin_can_see_orgs_they_dont_have( client_request, platform_admin_user, - mocker, ): platform_admin_user['organisations'] = [] - mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) client_request.login(platform_admin_user) request.view_args = {'org_id': 'org_2'} @@ -170,9 +144,7 @@ def test_platform_admin_can_see_orgs_they_dont_have( def test_cant_use_decorator_without_view_args( client_request, platform_admin_user, - mocker, ): - mocker.patch('app.user_api_client.get_user', return_value=platform_admin_user) client_request.login(platform_admin_user) request.view_args = {} @@ -187,11 +159,9 @@ def test_cant_use_decorator_without_view_args( def test_user_doesnt_have_permissions_for_organisation( client_request, - mocker, ): user = _user_with_permissions() user['organisations'] = ['org_1', 'org_2'] - mocker.patch('app.user_api_client.get_user', return_value=user) client_request.login(user) request.view_args = {'org_id': 'org_3'} @@ -205,11 +175,9 @@ def test_user_doesnt_have_permissions_for_organisation( def test_user_with_no_permissions_to_service_goes_to_templates( - client_request, - mocker + client_request, ): user = _user_with_permissions() - mocker.patch('app.user_api_client.get_user', return_value=user) client_request.login(user) request.view_args = {'service_id': 'bar'} From e6e2770a049755a740147d15e808c6c9e3910c2c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 11:07:02 +0100 Subject: [PATCH 04/10] Rewrite `test_permissions` with parametrize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s easier to see what the different test cases are when they are laid out with `parametrize`, rather than separate functions with lots of boilerplate. --- tests/app/utils/test_user.py | 162 ++++++++++++++--------------------- 1 file changed, 62 insertions(+), 100 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index f6ee7f3cc..ae1edc847 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -4,98 +4,77 @@ from werkzeug.exceptions import Forbidden from app.main.views.index import index from app.utils.user import user_has_permissions +from tests.conftest import create_platform_admin_user -def _test_permissions( +def _user_with_permissions(): + user_data = {'id': 999, + 'name': 'Test User', + 'password': 'somepassword', + 'email_address': 'test@user.gov.uk', + 'mobile_number': '+4412341234', + 'state': 'active', + 'failed_login_count': 0, + 'permissions': {'foo': ['manage_users', 'manage_templates', 'manage_settings']}, + 'platform_admin': False, + 'organisations': ['org_1', 'org_2'], + 'services': ['foo', 'bar'], + 'current_session_id': None, + } + return user_data + + +@pytest.mark.parametrize('user, permissions, kwargs', ( + pytest.param( + _user_with_permissions(), + ['send_messages'], + {}, + marks=pytest.mark.xfail(raises=Forbidden), + ), + ( + _user_with_permissions(), + ['manage_service'], + {}, + ), + ( + _user_with_permissions(), + ['send_messages', 'manage_service'], + {}, + ), + ( + _user_with_permissions(), + ['manage_templates', 'manage_service'], + {}, + ), + ( + _user_with_permissions(), + ['manage_service', 'manage_templates'], + {}, + ), + ( + create_platform_admin_user(), + [], + {}, + ), + pytest.param( + create_platform_admin_user(), + [], + {'restrict_admin_usage': True}, + marks=pytest.mark.xfail(raises=Forbidden), + ), +)) +def test_permissions( client_request, - usr, + user, permissions, - kwargs=None, + kwargs, ): request.view_args.update({'service_id': 'foo'}) - client_request.login(usr) + client_request.login(user) decorator = user_has_permissions(*permissions, **(kwargs or {})) decorated_index = decorator(index) - return decorated_index() - - -def test_user_has_permissions_on_endpoint_fail( - client_request, - mock_get_service, -): - with pytest.raises(Forbidden): - _test_permissions( - client_request, - _user_with_permissions(), - ['send_messages'], - ) - - -def test_user_has_permissions_success( - client_request, - mocker, -): - _test_permissions( - client_request, - _user_with_permissions(), - ['manage_service'], - ) - - -def test_user_has_permissions_or( - client_request, -): - _test_permissions( - client_request, - _user_with_permissions(), - ['send_messages', 'manage_service'], - ) - - -def test_user_has_permissions_multiple( - client_request, -): - _test_permissions( - client_request, - _user_with_permissions(), - ['manage_templates', 'manage_service'], - ) - - -def test_exact_permissions( - client_request, -): - _test_permissions( - client_request, - _user_with_permissions(), - ['manage_service', 'manage_templates'], - ) - - -def test_platform_admin_user_can_access_page_that_has_no_permissions( - client_request, - platform_admin_user, -): - _test_permissions( - client_request, - platform_admin_user, - [], - ) - - -def test_platform_admin_user_can_not_access_page( - client_request, - platform_admin_user, - mock_get_service, -): - with pytest.raises(Forbidden): - _test_permissions( - client_request, - platform_admin_user, - [], - kwargs={'restrict_admin_usage': True}, - ) + decorated_index() def test_no_user_returns_redirect_to_sign_in( @@ -186,20 +165,3 @@ def test_user_with_no_permissions_to_service_goes_to_templates( pass index() - - -def _user_with_permissions(): - user_data = {'id': 999, - 'name': 'Test User', - 'password': 'somepassword', - 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', - 'state': 'active', - 'failed_login_count': 0, - 'permissions': {'foo': ['manage_users', 'manage_templates', 'manage_settings']}, - 'platform_admin': False, - 'organisations': ['org_1', 'org_2'], - 'services': ['foo', 'bar'], - 'current_session_id': None, - } - return user_data From f79c3f27e326b4102e940734931bfa3f3eea741e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 11:40:10 +0100 Subject: [PATCH 05/10] Use fixture to create user `dict` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This removes a bunch of dummy data which isn’t relevant to the tests being run. --- tests/app/utils/test_user.py | 50 +++++++++++++++--------------------- 1 file changed, 21 insertions(+), 29 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index ae1edc847..51459aed7 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -4,55 +4,44 @@ from werkzeug.exceptions import Forbidden from app.main.views.index import index from app.utils.user import user_has_permissions -from tests.conftest import create_platform_admin_user +from tests.conftest import create_platform_admin_user, create_user, sample_uuid - -def _user_with_permissions(): - user_data = {'id': 999, - 'name': 'Test User', - 'password': 'somepassword', - 'email_address': 'test@user.gov.uk', - 'mobile_number': '+4412341234', - 'state': 'active', - 'failed_login_count': 0, - 'permissions': {'foo': ['manage_users', 'manage_templates', 'manage_settings']}, - 'platform_admin': False, - 'organisations': ['org_1', 'org_2'], - 'services': ['foo', 'bar'], - 'current_session_id': None, - } - return user_data +_user_with_permissions = create_user( + id=sample_uuid(), + permissions={'foo': ['manage_users', 'manage_templates', 'manage_settings']}, + services=['foo', 'bar'], +) @pytest.mark.parametrize('user, permissions, kwargs', ( pytest.param( - _user_with_permissions(), + _user_with_permissions, ['send_messages'], {}, marks=pytest.mark.xfail(raises=Forbidden), ), ( - _user_with_permissions(), + _user_with_permissions, ['manage_service'], {}, ), ( - _user_with_permissions(), + _user_with_permissions, ['send_messages', 'manage_service'], {}, ), ( - _user_with_permissions(), + _user_with_permissions, ['manage_templates', 'manage_service'], {}, ), ( - _user_with_permissions(), + _user_with_permissions, ['manage_service', 'manage_templates'], {}, ), ( - create_platform_admin_user(), + _user_with_permissions, [], {}, ), @@ -91,8 +80,10 @@ def test_no_user_returns_redirect_to_sign_in( def test_user_has_permissions_for_organisation( client_request, ): - user = _user_with_permissions() - user['organisations'] = ['org_1', 'org_2'] + user = create_user( + id=sample_uuid(), + organisations=['org_1', 'org_2'], + ) client_request.login(user) request.view_args = {'org_id': 'org_2'} @@ -139,8 +130,10 @@ def test_cant_use_decorator_without_view_args( def test_user_doesnt_have_permissions_for_organisation( client_request, ): - user = _user_with_permissions() - user['organisations'] = ['org_1', 'org_2'] + user = create_user( + id=sample_uuid(), + organisations=['org_1', 'org_2'], + ) client_request.login(user) request.view_args = {'org_id': 'org_3'} @@ -156,8 +149,7 @@ def test_user_doesnt_have_permissions_for_organisation( def test_user_with_no_permissions_to_service_goes_to_templates( client_request, ): - user = _user_with_permissions() - client_request.login(user) + client_request.login(_user_with_permissions) request.view_args = {'service_id': 'bar'} @user_has_permissions() From 60870c69a7213c9cc028ec546e6788fc7a5c3c79 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 11:45:10 +0100 Subject: [PATCH 06/10] Split platform admin test out By making the one platform admin case a separate test we no longer need to pass in a `user` or `kwargs` to the parametrize every time, making it easier to read. --- tests/app/utils/test_user.py | 62 +++++++++++++----------------------- 1 file changed, 23 insertions(+), 39 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index 51459aed7..d6e1c207c 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -4,7 +4,7 @@ from werkzeug.exceptions import Forbidden from app.main.views.index import index from app.utils.user import user_has_permissions -from tests.conftest import create_platform_admin_user, create_user, sample_uuid +from tests.conftest import create_user, sample_uuid _user_with_permissions = create_user( id=sample_uuid(), @@ -13,59 +13,43 @@ _user_with_permissions = create_user( ) -@pytest.mark.parametrize('user, permissions, kwargs', ( +@pytest.mark.parametrize('permissions', ( pytest.param( - _user_with_permissions, ['send_messages'], - {}, - marks=pytest.mark.xfail(raises=Forbidden), - ), - ( - _user_with_permissions, - ['manage_service'], - {}, - ), - ( - _user_with_permissions, - ['send_messages', 'manage_service'], - {}, - ), - ( - _user_with_permissions, - ['manage_templates', 'manage_service'], - {}, - ), - ( - _user_with_permissions, - ['manage_service', 'manage_templates'], - {}, - ), - ( - _user_with_permissions, - [], - {}, - ), - pytest.param( - create_platform_admin_user(), - [], - {'restrict_admin_usage': True}, marks=pytest.mark.xfail(raises=Forbidden), ), + ['manage_service'], + ['send_messages', 'manage_service'], + ['manage_templates', 'manage_service'], + ['manage_service', 'manage_templates'], + [], )) def test_permissions( client_request, - user, permissions, - kwargs, ): request.view_args.update({'service_id': 'foo'}) - client_request.login(user) + client_request.login(_user_with_permissions) - decorator = user_has_permissions(*permissions, **(kwargs or {})) + decorator = user_has_permissions(*permissions) decorated_index = decorator(index) decorated_index() +def test_restrict_admin_usage( + client_request, + platform_admin_user, +): + request.view_args.update({'service_id': 'foo'}) + client_request.login(platform_admin_user) + + decorator = user_has_permissions(restrict_admin_usage=True) + decorated_index = decorator(index) + + with pytest.raises(Forbidden): + decorated_index() + + def test_no_user_returns_redirect_to_sign_in( client_request ): From 122a045142c63a0ad47431ad8f9ff183ccbf7a3b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 11:51:01 +0100 Subject: [PATCH 07/10] Define user inside each test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This user is only re-used once, which isn’t a big saving. By putting it inside the test it’s easy to see what special conditions are being set up that result in the expected outcometest result. --- tests/app/utils/test_user.py | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index d6e1c207c..655e027d2 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -6,12 +6,6 @@ from app.main.views.index import index from app.utils.user import user_has_permissions from tests.conftest import create_user, sample_uuid -_user_with_permissions = create_user( - id=sample_uuid(), - permissions={'foo': ['manage_users', 'manage_templates', 'manage_settings']}, - services=['foo', 'bar'], -) - @pytest.mark.parametrize('permissions', ( pytest.param( @@ -29,7 +23,12 @@ def test_permissions( permissions, ): request.view_args.update({'service_id': 'foo'}) - client_request.login(_user_with_permissions) + user = create_user( + id=sample_uuid(), + permissions={'foo': ['manage_users', 'manage_templates', 'manage_settings']}, + services=['foo', 'bar'], + ) + client_request.login(user) decorator = user_has_permissions(*permissions) decorated_index = decorator(index) @@ -133,7 +132,12 @@ def test_user_doesnt_have_permissions_for_organisation( def test_user_with_no_permissions_to_service_goes_to_templates( client_request, ): - client_request.login(_user_with_permissions) + user = create_user( + id=sample_uuid(), + permissions={'foo': ['manage_users', 'manage_templates', 'manage_settings']}, + services=['foo', 'bar'], + ) + client_request.login(user) request.view_args = {'service_id': 'bar'} @user_has_permissions() From f779a97b5c7c88f408ea44d254a9b1d1d307ca8a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 13:55:25 +0100 Subject: [PATCH 08/10] Use decorator as decorator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This syntax makes it clearer what is being tested here, because it’s unusual to see a decorator being manually called with function as its first argument. It’s also consistent with how the later tests in this file are written. --- tests/app/utils/test_user.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index 655e027d2..04c56c588 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -2,7 +2,6 @@ import pytest from flask import request from werkzeug.exceptions import Forbidden -from app.main.views.index import index from app.utils.user import user_has_permissions from tests.conftest import create_user, sample_uuid @@ -30,9 +29,11 @@ def test_permissions( ) client_request.login(user) - decorator = user_has_permissions(*permissions) - decorated_index = decorator(index) - decorated_index() + @user_has_permissions(*permissions) + def index(): + pass + + index() def test_restrict_admin_usage( @@ -42,20 +43,24 @@ def test_restrict_admin_usage( request.view_args.update({'service_id': 'foo'}) client_request.login(platform_admin_user) - decorator = user_has_permissions(restrict_admin_usage=True) - decorated_index = decorator(index) + @user_has_permissions(restrict_admin_usage=True) + def index(): + pass with pytest.raises(Forbidden): - decorated_index() + index() def test_no_user_returns_redirect_to_sign_in( client_request ): client_request.logout() - decorator = user_has_permissions() - decorated_index = decorator(index) - response = decorated_index() + + @user_has_permissions() + def index(): + pass + + response = index() assert response.status_code == 302 assert response.location.startswith('/sign-in?next=') From ea9c7e6102fbb2bfcc4778d0e8b1ede108cb9b9b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 15:00:37 +0100 Subject: [PATCH 09/10] Use existing user fixture --- tests/app/utils/test_user.py | 39 +++++++++++++++--------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index 04c56c588..9d9535ae4 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -3,7 +3,6 @@ from flask import request from werkzeug.exceptions import Forbidden from app.utils.user import user_has_permissions -from tests.conftest import create_user, sample_uuid @pytest.mark.parametrize('permissions', ( @@ -20,14 +19,14 @@ from tests.conftest import create_user, sample_uuid def test_permissions( client_request, permissions, + api_user_active, ): request.view_args.update({'service_id': 'foo'}) - user = create_user( - id=sample_uuid(), - permissions={'foo': ['manage_users', 'manage_templates', 'manage_settings']}, - services=['foo', 'bar'], - ) - client_request.login(user) + + api_user_active['permissions'] = {'foo': ['manage_users', 'manage_templates', 'manage_settings']} + api_user_active['services'] = ['foo', 'bar'] + + client_request.login(api_user_active) @user_has_permissions(*permissions) def index(): @@ -67,12 +66,10 @@ def test_no_user_returns_redirect_to_sign_in( def test_user_has_permissions_for_organisation( client_request, + api_user_active, ): - user = create_user( - id=sample_uuid(), - organisations=['org_1', 'org_2'], - ) - client_request.login(user) + api_user_active['organisations'] = ['org_1', 'org_2'] + client_request.login(api_user_active) request.view_args = {'org_id': 'org_2'} @@ -117,12 +114,10 @@ def test_cant_use_decorator_without_view_args( def test_user_doesnt_have_permissions_for_organisation( client_request, + api_user_active, ): - user = create_user( - id=sample_uuid(), - organisations=['org_1', 'org_2'], - ) - client_request.login(user) + api_user_active['organisations'] = ['org_1', 'org_2'] + client_request.login(api_user_active) request.view_args = {'org_id': 'org_3'} @@ -136,13 +131,11 @@ def test_user_doesnt_have_permissions_for_organisation( def test_user_with_no_permissions_to_service_goes_to_templates( client_request, + api_user_active, ): - user = create_user( - id=sample_uuid(), - permissions={'foo': ['manage_users', 'manage_templates', 'manage_settings']}, - services=['foo', 'bar'], - ) - client_request.login(user) + api_user_active['permissions'] = {'foo': ['manage_users', 'manage_templates', 'manage_settings']} + api_user_active['services'] = ['foo', 'bar'] + client_request.login(api_user_active) request.view_args = {'service_id': 'bar'} @user_has_permissions() From 30eebf35868b7f64339c6f14df06c20f171a091f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 6 Jun 2022 15:07:53 +0100 Subject: [PATCH 10/10] Reorder and label test cases This commit takes the existing test cases, removes duplicates, and tries to add a human-readable comment explaining what each one is testing. --- tests/app/utils/test_user.py | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/tests/app/utils/test_user.py b/tests/app/utils/test_user.py index 9d9535ae4..6a6378911 100644 --- a/tests/app/utils/test_user.py +++ b/tests/app/utils/test_user.py @@ -6,15 +6,25 @@ from app.utils.user import user_has_permissions @pytest.mark.parametrize('permissions', ( - pytest.param( - ['send_messages'], - marks=pytest.mark.xfail(raises=Forbidden), - ), - ['manage_service'], - ['send_messages', 'manage_service'], - ['manage_templates', 'manage_service'], - ['manage_service', 'manage_templates'], - [], + pytest.param([ + # Route has a permission which the user doesn’t have + 'send_messages' + ], marks=pytest.mark.xfail(raises=Forbidden)), + [ + # Route has one of the permissions which the user has + 'manage_service' + ], + [ + # Route has more than one of the permissions which the user has + 'manage_templates', 'manage_service' + ], + [ + # Route has one of the permissions which the user has, and one they do not + 'manage_service', 'send_messages', + ], + [ + # Route has no specific permissions required + ], )) def test_permissions( client_request,