From 92c6cca6a175fa277f2fe14c19486e94e560fc45 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 8 Jun 2020 14:39:36 +0100 Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20populate=20invite=20with=20user?= =?UTF-8?q?s=20from=20other=20orgs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We shouldn’t have a page where someone can look up any other user’s email address based on their user ID. We also don’t want a page where a malicious user could send someone an link which would get them invited to the service. Restricting the invite to be populated just from users in their own organisation doesn’t mitigate against this stuff completely, but they probably have a way of finding out the email address of someone in their organisation already. --- app/main/views/manage_users.py | 4 ++ tests/app/main/views/test_manage_users.py | 60 ++++++++++++++++++++++- tests/conftest.py | 4 +- 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index f624f3f10..c61b312d9 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -70,6 +70,10 @@ def invite_user(service_id, user_id=None): 'views/user-already-invited.html', user_to_invite=user_to_invite, ) + if not user_to_invite.default_organisation: + abort(403) + if user_to_invite.default_organisation.id != current_service.organisation_id: + abort(403) form.email_address.data = user_to_invite.email_address else: user_to_invite = None diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index f56b4b273..2b25ec340 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -7,6 +7,8 @@ from flask import url_for import app from app.utils import is_gov_user from tests.conftest import ( + ORGANISATION_ID, + ORGANISATION_TWO_ID, SERVICE_ONE_ID, USER_ONE_ID, create_active_user_empty_permissions, @@ -807,13 +809,16 @@ def test_should_show_page_for_inviting_user( def test_should_show_page_for_inviting_user_with_email_prefilled( - mocker, client_request, + mocker, + service_one, mock_get_template_folders, fake_uuid, active_user_with_permissions, active_user_with_permission_to_other_service, + mock_get_organisation_by_domain, ): + service_one['organisation'] = ORGANISATION_ID mocker.patch('app.models.user.user_api_client.get_user', side_effect=[ # First call is to get the current user active_user_with_permissions, @@ -876,6 +881,56 @@ def test_should_show_page_if_prefilled_user_is_already_a_team_member( assert not page.select("form") +def test_should_403_if_trying_to_prefill_email_address_for_user_with_no_organisation( + mocker, + client_request, + service_one, + mock_get_template_folders, + fake_uuid, + active_user_with_permissions, + active_user_with_permission_to_other_service, + mock_get_no_organisation_by_domain, +): + service_one['organisation'] = ORGANISATION_ID + mocker.patch('app.models.user.user_api_client.get_user', side_effect=[ + # First call is to get the current user + active_user_with_permissions, + # Second call gets the user to invite + active_user_with_permission_to_other_service, + ]) + client_request.get( + 'main.invite_user', + service_id=SERVICE_ONE_ID, + user_id=fake_uuid, + _expected_status=403, + ) + + +def test_should_403_if_trying_to_prefill_email_address_for_user_from_other_organisation( + mocker, + client_request, + service_one, + mock_get_template_folders, + fake_uuid, + active_user_with_permissions, + active_user_with_permission_to_other_service, + mock_get_organisation_by_domain, +): + service_one['organisation'] = ORGANISATION_TWO_ID + mocker.patch('app.models.user.user_api_client.get_user', side_effect=[ + # First call is to get the current user + active_user_with_permissions, + # Second call gets the user to invite + active_user_with_permission_to_other_service, + ]) + client_request.get( + 'main.invite_user', + service_id=SERVICE_ONE_ID, + user_id=fake_uuid, + _expected_status=403, + ) + + def test_should_show_folder_permission_form_if_service_has_folder_permissions_enabled( client_request, mocker, @@ -949,13 +1004,16 @@ def test_invite_user( def test_invite_user_when_email_address_is_prefilled( client_request, + service_one, active_user_with_permissions, active_user_with_permission_to_other_service, fake_uuid, mocker, sample_invite, mock_get_template_folders, + mock_get_organisation_by_domain, ): + service_one['organisation'] = ORGANISATION_ID mocker.patch('app.models.user.user_api_client.get_user', side_effect=[ # First call is to get the current user active_user_with_permissions, diff --git a/tests/conftest.py b/tests/conftest.py index a7339c876..0ce2ad7fc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3417,8 +3417,8 @@ def mock_get_organisation(mocker): @pytest.fixture(scope='function') def mock_get_organisation_by_domain(mocker): - def _get_organisation_by_domain(org_id): - return organisation_json(org_id) + def _get_organisation_by_domain(domain): + return organisation_json(ORGANISATION_ID) return mocker.patch( 'app.organisations_client.get_organisation_by_domain',