diff --git a/app/main/views/manage_users.py b/app/main/views/manage_users.py index b27eee8e7..ada8dcb16 100644 --- a/app/main/views/manage_users.py +++ b/app/main/views/manage_users.py @@ -77,9 +77,7 @@ def invite_user(service_id): @user_has_permissions('manage_service') def edit_user_permissions(service_id, user_id): service_has_email_auth = current_service.has_permission('email_auth') - # TODO we should probably using the service id here in the get user - # call as well. eg. /user/?&service=service_id - user = user_api_client.get_user(user_id) + user = current_service.get_team_member(user_id) user_has_no_mobile_number = user.mobile_number is None form = PermissionsForm.from_user(user, service_id) @@ -106,7 +104,7 @@ def edit_user_permissions(service_id, user_id): @login_required @user_has_permissions('manage_service') def remove_user_from_service(service_id, user_id): - user = user_api_client.get_user(user_id) + user = current_service.get_team_member(user_id) form = PermissionsForm.from_user(user, service_id) if request.method == 'POST': @@ -135,11 +133,11 @@ def remove_user_from_service(service_id, user_id): ) -@main.route("/services//users//edit-email", methods=['GET', 'POST']) +@main.route("/services//users//edit-email", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') def edit_user_email(service_id, user_id): - user = user_api_client.get_user(user_id) + user = current_service.get_team_member(user_id) user_email = user.email_address def _is_email_already_in_use(email): @@ -163,11 +161,11 @@ def edit_user_email(service_id, user_id): ) -@main.route("/services//users//edit-email/confirm", methods=['GET', 'POST']) +@main.route("/services//users//edit-email/confirm", methods=['GET', 'POST']) @login_required @user_has_permissions('manage_service') def confirm_edit_user_email(service_id, user_id): - user = user_api_client.get_user(user_id) + user = current_service.get_team_member(user_id) if 'team_member_email_change' in session: new_email = session['team_member_email_change'] else: @@ -178,7 +176,7 @@ def confirm_edit_user_email(service_id, user_id): )) if request.method == 'POST': try: - user_api_client.update_user_attribute(user_id, email_address=new_email) + 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') @@ -202,9 +200,9 @@ def confirm_edit_user_email(service_id, user_id): ) -@main.route("/services//cancel-invited-user/", methods=['GET']) +@main.route("/services//cancel-invited-user/", methods=['GET']) @user_has_permissions('manage_service') def cancel_invited_user(service_id, invited_user_id): - invite_api_client.cancel_invited_user(service_id=service_id, invited_user_id=invited_user_id) + current_service.cancel_invite(invited_user_id) return redirect(url_for('main.manage_users', service_id=service_id)) diff --git a/app/models/service.py b/app/models/service.py index 31703977f..4f308328d 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -98,13 +98,18 @@ class Service(): def has_jobs(self): return job_api_client.has_jobs(self.id) + @cached_property + def invited_users(self): + return invite_api_client.get_invites_for_service(service_id=self.id) + + @cached_property + def active_users(self): + return user_api_client.get_users_for_service(service_id=self.id) + @cached_property def team_members(self): return sorted( - ( - invite_api_client.get_invites_for_service(service_id=self.id) + - user_api_client.get_users_for_service(service_id=self.id) - ), + self.invited_users + self.active_users, key=lambda user: user.email_address.lower(), ) @@ -114,6 +119,23 @@ class Service(): self.id, 'manage_service' ) > 1 + def cancel_invite(self, invited_user_id): + + if str(invited_user_id) not in {user.id for user in self.invited_users}: + abort(404) + + return invite_api_client.cancel_invited_user( + service_id=self.id, + invited_user_id=str(invited_user_id), + ) + + def get_team_member(self, user_id): + + if str(user_id) not in {user.id for user in self.active_users}: + abort(404) + + return user_api_client.get_user(user_id) + @cached_property def all_templates(self): diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 3a4725f0d..62d49e546 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -9,6 +9,7 @@ import app from app.models.user import InvitedUser from tests.conftest import ( SERVICE_ONE_ID, + USER_ONE_ID, active_caseworking_user, active_user_with_permissions, ) @@ -22,7 +23,7 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( service_one, api_user_active, mock_check_invite_token, - mock_get_user_by_email, + mock_get_unknown_user_by_email, mock_get_users_by_service, mock_accept_invite, mock_add_user_to_service, @@ -35,9 +36,9 @@ def test_existing_user_accept_invite_calls_api_and_redirects_to_dashboard( response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) mock_check_invite_token.assert_called_with('thisisnotarealtoken') - mock_get_user_by_email.assert_called_with('invited_user@test.gov.uk') + mock_get_unknown_user_by_email.assert_called_with('invited_user@test.gov.uk') assert mock_accept_invite.call_count == 1 - mock_add_user_to_service.assert_called_with(expected_service, api_user_active.id, expected_permissions) + mock_add_user_to_service.assert_called_with(expected_service, USER_ONE_ID, expected_permissions) assert response.status_code == 302 assert response.location == url_for('main.service_dashboard', service_id=expected_service, _external=True) @@ -50,7 +51,7 @@ def test_existing_user_with_no_permissions_accept_invite( api_user_active, sample_invite, mock_check_invite_token, - mock_get_user_by_email, + mock_get_unknown_user_by_email, mock_get_users_by_service, mock_add_user_to_service, mock_get_service, @@ -61,7 +62,7 @@ def test_existing_user_with_no_permissions_accept_invite( mocker.patch('app.invite_api_client.accept_invite', return_value=sample_invite) response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken')) - mock_add_user_to_service.assert_called_with(expected_service, api_user_active.id, expected_permissions) + mock_add_user_to_service.assert_called_with(expected_service, USER_ONE_ID, expected_permissions) assert response.status_code == 302 @@ -197,7 +198,7 @@ def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( api_user_active, sample_invite, mock_check_invite_token, - mock_get_user_by_email, + mock_get_unknown_user_by_email, mock_get_users_by_service, mock_add_user_to_service, mock_accept_invite, @@ -210,8 +211,8 @@ def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=True) mock_check_invite_token.assert_called_with('thisisnotarealtoken') - mock_get_user_by_email.assert_called_with('invited_user@test.gov.uk') - mock_add_user_to_service.assert_called_with(expected_service, api_user_active.id, expected_permissions) + mock_get_unknown_user_by_email.assert_called_with('invited_user@test.gov.uk') + mock_add_user_to_service.assert_called_with(expected_service, USER_ONE_ID, expected_permissions) assert mock_accept_invite.call_count == 1 assert response.status_code == 200 @@ -504,7 +505,7 @@ def test_existing_user_accepts_and_sets_email_auth( api_user_active, service_one, sample_invite, - mock_get_user_by_email, + mock_get_unknown_user_by_email, mock_get_users_by_service, mock_accept_invite, mock_update_user_attribute, @@ -525,8 +526,9 @@ def test_existing_user_accepts_and_sets_email_auth( _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), ) - mock_update_user_attribute.assert_called_with(api_user_active.id, auth_type='email_auth') - mock_add_user_to_service.assert_called_with(ANY, api_user_active.id, ANY) + mock_get_unknown_user_by_email.assert_called_once_with('test@user.gov.uk') + mock_update_user_attribute.assert_called_once_with(USER_ONE_ID, auth_type='email_auth') + mock_add_user_to_service.assert_called_once_with(ANY, USER_ONE_ID, ANY) def test_existing_user_doesnt_get_auth_changed_by_service_without_permission( @@ -598,20 +600,16 @@ def test_existing_email_auth_user_with_phone_can_set_sms_auth( service_one, sample_invite, mock_get_users_by_service, + mock_get_unknown_user_by_email, mock_accept_invite, mock_update_user_attribute, mock_add_user_to_service, mocker ): sample_invite['email_address'] = api_user_active.email_address - service_one['permissions'].append('email_auth') sample_invite['auth_type'] = 'sms_auth' - api_user_active.auth_type = 'email_auth' - api_user_active.mobile_number = '07700900001' - mocker.patch('app.main.views.invites.user_api_client.get_user_by_email', return_value=api_user_active) - mocker.patch('app.main.views.invites.service_api_client.get_service', return_value={'data': service_one}) mocker.patch('app.invite_api_client.check_token', return_value=InvitedUser(**sample_invite)) client_request.get( @@ -621,4 +619,5 @@ def test_existing_email_auth_user_with_phone_can_set_sms_auth( _expected_redirect=url_for('main.service_dashboard', service_id=service_one['id'], _external=True), ) - mock_update_user_attribute.assert_called_with(api_user_active.id, auth_type='sms_auth') + mock_get_unknown_user_by_email.assert_called_once_with(sample_invite['email_address']) + mock_update_user_attribute.assert_called_once_with(USER_ONE_ID, auth_type='sms_auth') diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index 93914b79c..11e87b737 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -10,6 +10,7 @@ from app.models.user import InvitedUser from app.utils import is_gov_user from tests.conftest import ( SERVICE_ONE_ID, + USER_ONE_ID, active_caseworking_user, active_user_empty_permissions, active_user_manage_template_permission, @@ -17,6 +18,7 @@ from tests.conftest import ( active_user_view_permissions, active_user_with_permissions, normalize_spaces, + sample_uuid, ) from tests.conftest import service_one as create_sample_service @@ -195,13 +197,13 @@ def test_should_show_caseworker_on_overview_page( @pytest.mark.parametrize('endpoint, extra_args, service_has_email_auth, auth_options_hidden', [ ( 'main.edit_user_permissions', - {'user_id': 0}, + {'user_id': sample_uuid()}, True, False ), ( 'main.edit_user_permissions', - {'user_id': 0}, + {'user_id': sample_uuid()}, False, True ), @@ -224,7 +226,8 @@ def test_service_with_no_email_auth_hides_auth_type_options( extra_args, service_has_email_auth, auth_options_hidden, - service_one + service_one, + mock_get_users_by_service, ): if service_has_email_auth: service_one['permissions'].append('email_auth') @@ -236,7 +239,7 @@ def test_service_with_no_email_auth_hides_auth_type_options( @pytest.mark.parametrize('endpoint, extra_args', [ ( 'main.edit_user_permissions', - {'user_id': 0}, + {'user_id': sample_uuid()}, ), ( 'main.invite_user', @@ -245,6 +248,7 @@ def test_service_with_no_email_auth_hides_auth_type_options( ]) def test_service_without_caseworking_doesnt_show_admin_vs_caseworker( client_request, + mock_get_users_by_service, endpoint, service_has_caseworking, extra_args, @@ -299,6 +303,7 @@ def test_manage_users_page_shows_member_auth_type_if_service_has_email_auth_acti ]) def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( client_request, + mock_get_users_by_service, user, sms_option_disabled, expected_label, @@ -306,12 +311,12 @@ def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( mocker ): service_one['permissions'].append('email_auth') - test_user = mocker.patch('app.user_api_client.get_user', return_value=user(mocker)) + mocker.patch('app.user_api_client.get_user', return_value=user(mocker)) page = client_request.get( 'main.edit_user_permissions', service_id=service_one['id'], - user_id=test_user.id + user_id=sample_uuid(), ) sms_auth_radio_button = page.select_one('input[value="sms_auth"]') @@ -324,7 +329,7 @@ def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( @pytest.mark.parametrize('endpoint, extra_args, expected_checkboxes', [ ( 'main.edit_user_permissions', - {'user_id': 0}, + {'user_id': sample_uuid()}, [ ('view_activity', True), ('send_messages', True), @@ -347,6 +352,7 @@ def test_user_with_no_mobile_number_cant_be_set_to_sms_auth( ]) def test_should_show_page_for_one_user( client_request, + mock_get_users_by_service, endpoint, extra_args, expected_checkboxes, @@ -362,6 +368,18 @@ def test_should_show_page_for_one_user( assert checkboxes[index].has_attr('checked') == expected_checked +def test_should_not_show_page_for_non_team_member( + client_request, + mock_get_users_by_service, +): + client_request.get( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=USER_ONE_ID, + _expected_status=404, + ) + + @pytest.mark.parametrize('submitted_permissions, permissions_sent_to_api', [ ( { @@ -398,6 +416,7 @@ def test_should_show_page_for_one_user( def test_edit_user_permissions( client_request, mocker, + mock_get_users_by_service, mock_get_invites_for_service, mock_set_user_permissions, fake_uuid, @@ -426,11 +445,31 @@ def test_edit_user_permissions( ) +def test_cant_edit_non_member_user_permissions( + client_request, + mocker, + mock_get_users_by_service, + mock_set_user_permissions, +): + client_request.post( + 'main.edit_user_permissions', + service_id=SERVICE_ONE_ID, + user_id=USER_ONE_ID, + _data={ + 'email_address': 'test@example.com', + 'manage_service': 'y', + }, + _expected_status=404, + ) + assert mock_set_user_permissions.called is False + + @pytest.mark.parametrize('auth_type', ['email_auth', 'sms_auth']) def test_edit_user_permissions_including_authentication_with_email_auth_service( logged_in_client, active_user_with_permissions, mocker, + mock_get_users_by_service, mock_get_invites_for_service, mock_set_user_permissions, mock_update_user_attribute, @@ -588,19 +627,40 @@ def test_invite_user_with_email_auth_service( def test_cancel_invited_user_cancels_user_invitations( - logged_in_client, + client_request, + mock_get_invites_for_service, active_user_with_permissions, mocker, ): - mocker.patch('app.invite_api_client.cancel_invited_user') - import uuid - invited_user_id = uuid.uuid4() - service = create_sample_service(active_user_with_permissions) - response = logged_in_client.get(url_for('main.cancel_invited_user', service_id=service['id'], - invited_user_id=invited_user_id)) + mock_cancel = mocker.patch('app.invite_api_client.cancel_invited_user') + client_request.get( + 'main.cancel_invited_user', + service_id=SERVICE_ONE_ID, + invited_user_id=sample_uuid(), + _expected_status=302, + _expected_redirect=url_for( + 'main.manage_users', service_id=SERVICE_ONE_ID, _external=True + ), + ) + mock_cancel.assert_called_once_with( + service_id=SERVICE_ONE_ID, + invited_user_id=sample_uuid(), + ) - assert response.status_code == 302 - assert response.location == url_for('main.manage_users', service_id=service['id'], _external=True) + +def test_cancel_invited_user_doesnt_work_if_user_not_invited_to_this_service( + client_request, + mock_get_invites_for_service, + mocker, +): + mock_cancel = mocker.patch('app.invite_api_client.cancel_invited_user') + client_request.get( + 'main.cancel_invited_user', + service_id=SERVICE_ONE_ID, + invited_user_id=USER_ONE_ID, + _expected_status=404, + ) + assert mock_cancel.called is False @pytest.mark.parametrize('invite_status, expected_text', [ @@ -702,6 +762,7 @@ def test_no_permission_manage_users_page( def test_get_remove_user_from_service( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, ): @@ -741,6 +802,7 @@ def test_can_remove_user_from_service_as_platform_admin( service_one, platform_admin_user, active_user_with_permissions, + mock_get_users_by_service, mock_remove_user_from_service, mocker, ): @@ -775,15 +837,16 @@ def test_edit_user_email_page( client_request, active_user_with_permissions, service_one, + mock_get_users_by_service, mocker ): user = active_user_with_permissions - test_user = mocker.patch('app.user_api_client.get_user', return_value=user) + mocker.patch('app.user_api_client.get_user', return_value=user) page = client_request.get( 'main.edit_user_email', service_id=service_one['id'], - user_id=test_user.id + user_id=sample_uuid() ) assert page.find('h1').text == "Change team member’s email address" @@ -792,9 +855,22 @@ def test_edit_user_email_page( assert page.select('button[type=submit]')[0].text == "Save" +def test_edit_user_email_page_404_for_non_team_member( + client_request, + mock_get_users_by_service, +): + client_request.get( + 'main.edit_user_email', + service_id=SERVICE_ONE_ID, + user_id=USER_ONE_ID, + _expected_status=404, + ) + + def test_edit_user_email_redirects_to_confirmation( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -817,6 +893,7 @@ def test_edit_user_email_without_changing_goes_back_to_team_members( client_request, active_user_with_permissions, mock_get_user, + mock_get_users_by_service, mock_update_user_attribute, ): client_request.post( @@ -839,6 +916,7 @@ def test_edit_user_email_without_changing_goes_back_to_team_members( def test_confirm_edit_user_email_page( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -866,6 +944,7 @@ def test_confirm_edit_user_email_page( def test_confirm_edit_user_email_page_redirects_if_session_empty( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -879,9 +958,22 @@ def test_confirm_edit_user_email_page_redirects_if_session_empty( assert 'Confirm change of email address' not in response.get_data(as_text=True) +def test_confirm_edit_user_email_page_404s_for_non_team_member( + client_request, + mock_get_users_by_service, +): + client_request.get( + 'main.confirm_edit_user_email', + service_id=SERVICE_ONE_ID, + user_id=USER_ONE_ID, + _expected_status=404, + ) + + def test_confirm_edit_user_email_changes_user_email( logged_in_client, active_user_with_permissions, + mock_get_users_by_service, service_one, mocker, mock_get_user, @@ -899,3 +991,17 @@ def test_confirm_edit_user_email_changes_user_email( 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, email_address=new_email) + + +def test_confirm_edit_user_email_doesnt_change_user_email_for_non_team_member( + client_request, + mock_get_users_by_service, +): + with client_request.session_transaction() as session: + session['team_member_email_change'] = 'new_email@gov.uk' + client_request.post( + 'main.confirm_edit_user_email', + service_id=SERVICE_ONE_ID, + user_id=USER_ONE_ID, + _expected_status=404, + ) diff --git a/tests/conftest.py b/tests/conftest.py index f96cba090..de4566c59 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -696,6 +696,7 @@ SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb" SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52" ORGANISATION_ID = "c011fa40-4cbe-4524-b415-dde2f421bd9c" TEMPLATE_ONE_ID = "b22d7d94-2197-4a7d-a8e7-fd5f9770bf48" +USER_ONE_ID = "7b395b52-c6c1-469c-9d61-54166461c1ab" @pytest.fixture(scope='function') @@ -1554,6 +1555,18 @@ def mock_get_user_by_email(mocker, user=None): return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) +@pytest.fixture(scope='function') +def mock_get_unknown_user_by_email(mocker, user=None): + if user is None: + user = api_user_active(USER_ONE_ID) + + def _get_user(email_address): + user.email_address = email_address + return user + + return mocker.patch('app.user_api_client.get_user_by_email', side_effect=_get_user) + + @pytest.fixture(scope='function') def mock_get_locked_user_by_email(mocker, api_user_locked): return mock_get_user_by_email(mocker, user=api_user_locked) @@ -2139,7 +2152,7 @@ def mock_has_permissions(mocker): @pytest.fixture(scope='function') def mock_get_users_by_service(mocker): def _get_users_for_service(service_id): - data = [{'id': 1, + data = [{'id': sample_uuid(), 'logged_in_at': None, 'mobile_number': '+447700900986', 'permissions': {SERVICE_ONE_ID: ['send_texts', @@ -2191,7 +2204,7 @@ def mock_s3_set_metadata(mocker, content=None): @pytest.fixture(scope='function') def sample_invite(mocker, service_one, status='pending'): - id_ = str(generate_uuid()) + id_ = str(sample_uuid()) from_user = service_one['users'][0] email_address = 'invited_user@test.gov.uk' service_id = service_one['id']