diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index b57d79a2e..1d43ba56a 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -49,8 +49,9 @@ def old_service_dashboard(service_id): @user_has_permissions() def service_dashboard(service_id): - if session.get('invited_user'): + if session.get('invited_user_id') or session.get('invited_user'): session.pop('invited_user', None) + session.pop('invited_user_id', None) session['service_id'] = service_id if current_service.has_permission('broadcast'): diff --git a/app/main/views/invites.py b/app/main/views/invites.py index 329f1d722..02f9c644f 100644 --- a/app/main/views/invites.py +++ b/app/main/views/invites.py @@ -40,12 +40,14 @@ def accept_invite(token): if invited_user.status == 'accepted': session.pop('invited_user', None) + session.pop('invited_user_id', None) service = Service.from_id(invited_user.service) if service.has_permission('broadcast'): return redirect(url_for('main.broadcast_tour', service_id=service.id, step_index=1)) return redirect(url_for('main.service_dashboard', service_id=invited_user.service)) session['invited_user'] = invited_user.serialize() + session['invited_user_id'] = invited_user.id existing_user = User.from_email_address_or_none(invited_user.email_address) @@ -103,9 +105,11 @@ def accept_org_invite(token): if invited_org_user.status == 'accepted': session.pop('invited_org_user', None) + session.pop('invited_org_user_id', None) return redirect(url_for('main.organisation_dashboard', org_id=invited_org_user.organisation)) session['invited_org_user'] = invited_org_user.serialize() + session['invited_org_user_id'] = invited_org_user.id existing_user = User.from_email_address_or_none(invited_org_user.email_address) organisation_users = OrganisationUsers(invited_org_user.organisation) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 7ce9225b4..f6e1a3598 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -36,11 +36,12 @@ def sign_in(): if user and user.state == 'pending': return redirect(url_for('main.resend_email_verification', next=redirect_url)) - if user and session.get('invited_user'): + if user and (session.get('invited_user') or session.get('invited_user_id')): invited_user = InvitedUser.from_session() if user.email_address.lower() != invited_user.email_address.lower(): flash("You cannot accept an invite for another person.") session.pop('invited_user', None) + session.pop('invited_user_id', None) abort(403) else: invited_user.accept_invite() diff --git a/app/main/views/verify.py b/app/main/views/verify.py index 61c9b64f2..b2fd145fc 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -16,7 +16,7 @@ from app import user_api_client from app.main import main from app.main.forms import TwoFactorForm from app.models.service import Service -from app.models.user import InvitedUser, User +from app.models.user import InvitedOrgUser, InvitedUser, User from app.utils import redirect_to_sign_in @@ -75,7 +75,7 @@ def activate_user(user_id): activated_user = user.activate() activated_user.login() - invited_user = session.get('invited_user') + invited_user = InvitedUser.from_session() if invited_user: service_id = _add_invited_user_to_service(invited_user) service = Service.from_id(service_id) @@ -83,9 +83,9 @@ def activate_user(user_id): return redirect(url_for('main.broadcast_tour', service_id=service.id, step_index=1)) return redirect(url_for('main.service_dashboard', service_id=service_id)) - invited_org_user = session.get('invited_org_user') + invited_org_user = InvitedOrgUser.from_session() if invited_org_user: - user_api_client.add_user_to_organisation(invited_org_user['organisation'], session['user_details']['id']) + user_api_client.add_user_to_organisation(invited_org_user.organisation, session['user_details']['id']) if organisation_id: return redirect(url_for('main.organisation_dashboard', org_id=organisation_id)) @@ -93,10 +93,9 @@ def activate_user(user_id): return redirect(url_for('main.add_service', first='first')) -def _add_invited_user_to_service(invited_user): - invitation = InvitedUser(invited_user) +def _add_invited_user_to_service(invitation): user = User.from_id(session['user_id']) - service_id = invited_user['service'] + service_id = invitation.service user.add_to_service( service_id, invitation.permissions, diff --git a/app/models/user.py b/app/models/user.py index ff93e1eb9..bce03d7a9 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -473,7 +473,13 @@ class InvitedUser(JSONModel): @classmethod def by_id_and_service_id(cls, service_id, invited_user_id): return cls( - invite_api_client.get_invited_user(service_id, invited_user_id) + invite_api_client.get_invited_user_for_service(service_id, invited_user_id) + ) + + @classmethod + def by_id(cls, invited_user_id): + return cls( + invite_api_client.get_invited_user(invited_user_id) ) def accept_invite(self): @@ -515,6 +521,10 @@ class InvitedUser(JSONModel): @classmethod def from_session(cls): + invited_user_id = session.get('invited_user_id') + if invited_user_id: + return cls.by_id(invited_user_id) + invited_user = session.get('invited_user') return cls(invited_user) if invited_user else None @@ -595,13 +605,23 @@ class InvitedOrgUser(JSONModel): @classmethod def from_session(cls): + invited_org_user_id = session.get('invited_org_user_id') + if invited_org_user_id: + return cls.by_id(invited_org_user_id) + invited_org_user = session.get('invited_org_user') return cls(invited_org_user) if invited_org_user else None @classmethod def by_id_and_org_id(cls, org_id, invited_user_id): return cls( - org_invite_api_client.get_invited_user(org_id, invited_user_id) + org_invite_api_client.get_invited_user_for_org(org_id, invited_user_id) + ) + + @classmethod + def by_id(cls, invited_user_id): + return cls( + org_invite_api_client.get_invited_user(invited_user_id) ) def serialize(self, permissions_as_string=False): diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 8954104ea..f77b9f5c1 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -37,7 +37,12 @@ class InviteApiClient(NotifyAdminAPIClient): '/service/{}/invite'.format(service_id) )['data'] - def get_invited_user(self, service_id, invited_user_id): + def get_invited_user(self, invited_user_id): + return self.get( + f'/invite/service/{invited_user_id}' + )['data'] + + def get_invited_user_for_service(self, service_id, invited_user_id): return self.get( f'/service/{service_id}/invite/{invited_user_id}' )['data'] @@ -51,7 +56,7 @@ class InviteApiClient(NotifyAdminAPIClient): ]) def check_token(self, token): - return self.get(url='/invite/service/{}'.format(token))['data'] + return self.get(url='/invite/service/check/{}'.format(token))['data'] def cancel_invited_user(self, service_id, invited_user_id): data = {'status': 'cancelled'} diff --git a/app/notify_client/org_invite_api_client.py b/app/notify_client/org_invite_api_client.py index c5d333d18..23c1d7b99 100644 --- a/app/notify_client/org_invite_api_client.py +++ b/app/notify_client/org_invite_api_client.py @@ -23,13 +23,18 @@ class OrgInviteApiClient(NotifyAdminAPIClient): resp = self.get(endpoint) return resp['data'] - def get_invited_user(self, org_id, invited_org_user_id): + def get_invited_user_for_org(self, org_id, invited_org_user_id): return self.get( f'/organisation/{org_id}/invite/{invited_org_user_id}' )['data'] + def get_invited_user(self, invited_user_id): + return self.get( + f'/invite/organisation/{invited_user_id}' + )['data'] + def check_token(self, token): - resp = self.get(url='/invite/organisation/{}'.format(token)) + resp = self.get(url='/invite/organisation/check/{}'.format(token)) return resp['data'] def cancel_invited_user(self, org_id, invited_user_id): diff --git a/tests/app/main/views/organisations/test_organisation_invites.py b/tests/app/main/views/organisations/test_organisation_invites.py index 7ac2eebb0..9de398170 100644 --- a/tests/app/main/views/organisations/test_organisation_invites.py +++ b/tests/app/main/views/organisations/test_organisation_invites.py @@ -341,6 +341,48 @@ def test_org_user_registration( ) +def test_org_user_registration_when_org_user_id_in_session( + client, + sample_org_invite, + mock_email_is_not_already_in_use, + mock_register_user, + mock_send_verify_code, + mock_get_user_by_email, + mock_send_verify_email, + mock_accept_org_invite, + mock_add_user_to_organisation, + mock_get_invited_org_user_by_id, +): + with client.session_transaction() as session: + session['invited_org_user_id'] = sample_org_invite['id'] + + response = client.post(url_for('main.register_from_org_invite'), data={ + 'name': 'Test User', + 'email_address': sample_org_invite['email_address'], + 'mobile_number': '+4407700900460', + 'password': 'validPassword!', + 'organisation': sample_org_invite['organisation'] + }) + + assert response.status_code == 302 + assert response.location == url_for('main.verify', _external=True) + + assert mock_get_user_by_email.called is False + mock_get_invited_org_user_by_id.assert_called_once_with(sample_org_invite['id']) + mock_register_user.assert_called_once_with( + 'Test User', + sample_org_invite['email_address'], + '+4407700900460', + 'validPassword!', + 'sms_auth' + ) + mock_send_verify_code.assert_called_once_with( + '6ce466d0-fd6a-11e5-82f5-e0accb9d11a6', + 'sms', + '+4407700900460', + ) + + def test_verified_org_user_redirects_to_dashboard( client, sample_org_invite, diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py index 7633649c3..c690d5301 100644 --- a/tests/app/main/views/organisations/test_organisations.py +++ b/tests/app/main/views/organisations/test_organisations.py @@ -676,7 +676,7 @@ def test_cancel_invited_org_user_cancels_user_invitations( mocker, ): mock_cancel = mocker.patch('app.org_invite_api_client.cancel_invited_user') - mocker.patch('app.org_invite_api_client.get_invited_user', return_value=sample_org_invite) + mocker.patch('app.org_invite_api_client.get_invited_user_for_org', return_value=sample_org_invite) page = client_request.get( 'main.cancel_invited_org_user', diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 7fe84def9..cb36cf8d0 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -306,6 +306,7 @@ def test_new_user_accept_invite_calls_api_and_views_registration_page( service_one, mock_check_invite_token, mock_dont_get_user_by_email, + mock_get_invited_user_by_id, mock_add_user_to_service, mock_get_users_by_service, mock_get_service, @@ -315,6 +316,7 @@ def test_new_user_accept_invite_calls_api_and_views_registration_page( mock_check_invite_token.assert_called_with('thisisnotarealtoken') mock_dont_get_user_by_email.assert_called_with('invited_user@test.gov.uk') + mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -401,6 +403,7 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( mock_email_is_not_already_in_use, mock_register_user, mock_send_verify_code, + mock_get_invited_user_by_id, mock_accept_invite, mock_get_users_by_service, mock_add_user_to_service, @@ -418,6 +421,7 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( assert response.location == expected_redirect_location invited_user = session.get('invited_user') assert invited_user + assert session.get('invited_user_id') == USER_ONE_ID assert expected_service == invited_user['service'] assert expected_email == invited_user['email_address'] assert expected_from_user == invited_user['from_user'] @@ -437,6 +441,7 @@ def test_new_user_accept_invite_completes_new_registration_redirects_to_verify( assert response.location == expected_redirect_location mock_send_verify_code.assert_called_once_with(ANY, 'sms', data['mobile_number']) + mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID) mock_register_user.assert_called_with(data['name'], data['email_address'], @@ -516,6 +521,7 @@ def test_new_invited_user_verifies_and_added_to_service( mock_add_user_to_service, mock_accept_invite, mock_get_service, + mock_get_invited_user_by_id, mock_get_service_templates, mock_get_template_statistics, mock_has_no_jobs, @@ -558,6 +564,7 @@ def test_new_invited_user_verifies_and_added_to_service( expected_permissions = {'view_activity', 'send_messages', 'manage_service', 'manage_api_keys'} with client.session_transaction() as session: + assert 'invited_user_id' not in session new_user_id = session['user_id'] mock_add_user_to_service.assert_called_with(data['service'], new_user_id, expected_permissions, []) mock_accept_invite.assert_called_with(data['service'], sample_invite['id']) diff --git a/tests/app/main/views/test_manage_users.py b/tests/app/main/views/test_manage_users.py index f54473809..186eac9d6 100644 --- a/tests/app/main/views/test_manage_users.py +++ b/tests/app/main/views/test_manage_users.py @@ -1242,7 +1242,7 @@ def test_cancel_invited_user_cancels_user_invitations( mocker, ): mock_cancel = mocker.patch('app.invite_api_client.cancel_invited_user') - mocker.patch('app.invite_api_client.get_invited_user', return_value=sample_invite) + mocker.patch('app.invite_api_client.get_invited_user_for_service', return_value=sample_invite) page = client_request.get( 'main.cancel_invited_user', diff --git a/tests/app/models/test_user.py b/tests/app/models/test_user.py index c74149904..8bc2d24ea 100644 --- a/tests/app/models/test_user.py +++ b/tests/app/models/test_user.py @@ -1,6 +1,10 @@ +import uuid +from unittest.mock import Mock + import pytest -from app.models.user import AnonymousUser, User +from app.models.user import AnonymousUser, InvitedOrgUser, InvitedUser, User +from tests.conftest import USER_ONE_ID def test_anonymous_user(app_): @@ -106,3 +110,79 @@ def test_has_live_services_when_service_is_not_live( 'id': fake_uuid, 'platform_admin': False, }).live_services == [] + + +def test_invited_user_from_session_uses_id(client, mocker, mock_get_invited_user_by_id): + session_dict = {'invited_user_id': USER_ONE_ID} + mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) + + assert InvitedUser.from_session().id == USER_ONE_ID + + mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID) + + +def test_invited_user_from_session_uses_id_even_if_obj_in_session( + client, + mocker, + sample_invite, + mock_get_invited_user_by_id +): + mock_session_obj = Mock(spec=dict) + session_dict = {'invited_user_id': USER_ONE_ID, 'invited_user': mock_session_obj} + mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) + + assert InvitedUser.from_session().id == USER_ONE_ID + + assert mock_session_obj.mock_calls == [] + mock_get_invited_user_by_id.assert_called_once_with(USER_ONE_ID) + + +def test_invited_user_from_session_uses_obj_if_id_not_present(client, mocker, sample_invite): + session_dict = {'invited_user': sample_invite} + mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) + + assert InvitedUser.from_session().id == USER_ONE_ID + + +def test_invited_user_from_session_returns_none_if_nothing_present(client, mocker): + mocker.patch.dict('app.models.user.session', values={}, clear=True) + assert InvitedUser.from_session() is None + + +def test_invited_org_user_from_session_uses_id(client, mocker, mock_get_invited_org_user_by_id, sample_org_invite): + session_dict = {'invited_org_user_id': sample_org_invite['id']} + mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) + + assert InvitedOrgUser.from_session().id == sample_org_invite['id'] + + mock_get_invited_org_user_by_id.assert_called_once_with(sample_org_invite['id']) + + +def test_invited_org_user_from_session_uses_id_even_if_obj_in_session( + client, + mocker, + sample_org_invite, + mock_get_invited_org_user_by_id +): + fake_id = str(uuid.uuid4()) + mock_org_dict = Mock(spec=dict) + session_dict = {'invited_org_user_id': fake_id, 'invited_org_user': mock_org_dict} + mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) + + assert InvitedOrgUser.from_session().id == sample_org_invite['id'] + + # make sure we didn't access invited_org_user (as org_user_id takes precedence) + assert mock_org_dict.mock_calls == [] + mock_get_invited_org_user_by_id.assert_called_once_with(fake_id) + + +def test_invited_org_user_from_session_uses_obj_if_id_not_present(client, mocker, sample_org_invite): + session_dict = {'invited_org_user': sample_org_invite} + mocker.patch.dict('app.models.user.session', values=session_dict, clear=True) + + assert InvitedOrgUser.from_session().id == sample_org_invite['id'] + + +def test_invited_org_user_from_session_returns_none_if_nothing_present(client, mocker): + mocker.patch.dict('app.models.user.session', values={}, clear=True) + assert InvitedOrgUser.from_session() is None diff --git a/tests/conftest.py b/tests/conftest.py index ae7fb8ab3..1f6ea450e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4462,3 +4462,29 @@ def mock_update_broadcast_message_status( 'app.broadcast_message_api_client.update_broadcast_message_status', side_effect=_update, ) + + +@pytest.fixture +def mock_get_invited_user_by_id(mocker, sample_invite): + def _get( + invited_user_id + ): + return sample_invite + + return mocker.patch( + 'app.invite_api_client.get_invited_user', + side_effect=_get, + ) + + +@pytest.fixture +def mock_get_invited_org_user_by_id(mocker, sample_org_invite): + def _get( + invited_org_user_id + ): + return sample_org_invite + + return mocker.patch( + 'app.org_invite_api_client.get_invited_user', + side_effect=_get, + )