diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index 2d5b88618b..76f5dfc4d6 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -67,5 +67,14 @@ def get_permissions_by_user_id_and_service_id(self, user_id, service_id): self.Meta.model.query.filter_by(user_id=user_id).join(Permission.service).filter_by(active=True, id=service_id).all() ) + def get_team_members_with_permission(self, service_id, permission): + permission_objs = ( + self.Meta.model.query.filter_by(service_id=service_id, permission=permission) + .join(Permission.user) + .filter_by(state="active") + .all() + ) + return [p.user for p in permission_objs] + permission_dao = PermissionDAO() diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 21f469d5d1..a17e5ec7d0 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -104,7 +104,7 @@ def verify_within_time(user, age=timedelta(seconds=30)): return query.count() -def get_user_by_id(user_id=None): +def get_user_by_id(user_id=None) -> User: if user_id: return User.query.filter_by(id=user_id).one() return User.query.filter_by().all() diff --git a/app/errors.py b/app/errors.py index 0bc19c0b2d..39f9e617bb 100644 --- a/app/errors.py +++ b/app/errors.py @@ -41,7 +41,23 @@ def __str__(self): return str(self.to_dict()) +class CannotRemoveUserError(InvalidRequest): + message = "Cannot remove user from team" + + def __init__(self, fields=[], message=None, status_code=400): + # Call parent class __init__ with message and status_code + super().__init__(message=message if message else self.message, status_code=status_code) + self.fields = fields + + def register_errors(blueprint): + @blueprint.errorhandler(CannotRemoveUserError) + def cannot_remove_user_error(error): + response = jsonify(error.to_dict()) + response.status_code = error.status_code + current_app.logger.info(error.message) + return response + @blueprint.errorhandler(InvalidEmailError) def invalid_format(error): # Please not that InvalidEmailError is re-raised for InvalidEmail or InvalidPhone, diff --git a/app/models.py b/app/models.py index ec6fb01e3e..02efdd80b6 100644 --- a/app/models.py +++ b/app/models.py @@ -645,6 +645,13 @@ def serialize_for_org_dashboard(self) -> dict: "research_mode": self.research_mode, } + def get_users_with_permission(self, permission): + from app.dao.permissions_dao import permission_dao + + if permission: + return permission_dao.get_team_members_with_permission(self.id, permission) + return [] + class AnnualBilling(BaseModel): __tablename__ = "annual_billing" diff --git a/app/service/rest.py b/app/service/rest.py index 1934dabb7d..f25633c10e 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -85,11 +85,12 @@ ) from app.dao.templates_dao import dao_get_template_by_id from app.dao.users_dao import get_user_by_id -from app.errors import InvalidRequest, register_errors +from app.errors import CannotRemoveUserError, InvalidRequest, register_errors from app.models import ( EMAIL_TYPE, KEY_TYPE_NORMAL, LETTER_TYPE, + MANAGE_SETTINGS, NOTIFICATION_CANCELLED, SMS_TYPE, EmailBranding, @@ -490,13 +491,23 @@ def add_user_to_service(service_id, user_id): def remove_user_from_service(service_id, user_id): service = dao_fetch_service_by_id(service_id) user = get_user_by_id(user_id=user_id) + users_with_manage_settings_perm = service.get_users_with_permission(MANAGE_SETTINGS) + if user not in service.users: error = "User not found" raise InvalidRequest(error, status_code=404) elif len(service.users) == 1: error = "You cannot remove the only user for a service" - raise InvalidRequest(error, status_code=400) + raise CannotRemoveUserError(message=error) + + elif len(service.users) == 2: + error = "SERVICE_CANNOT_HAVE_LT_2_MEMBERS" + raise CannotRemoveUserError(message=error) + + elif user in users_with_manage_settings_perm and len(users_with_manage_settings_perm) <= 1: + error = "SERVICE_NEEDS_USER_W_MANAGE_SETTINGS_PERM" + raise CannotRemoveUserError(message=error) dao_remove_user_from_service(service, user) diff --git a/tests/app/dao/test_permissions_dao.py b/tests/app/dao/test_permissions_dao.py index 02bbfbf567..2e1d8dbcf6 100644 --- a/tests/app/dao/test_permissions_dao.py +++ b/tests/app/dao/test_permissions_dao.py @@ -1,4 +1,5 @@ from app.dao.permissions_dao import permission_dao +from app.models import MANAGE_SETTINGS from tests.app.conftest import create_sample_service @@ -27,3 +28,17 @@ def test_get_permissions_by_user_id_returns_only_active_service(notify_db, notif assert len(permissions) == 8 assert active_service in [i.service for i in permissions] assert inactive_service not in [i.service for i in permissions] + + +def test_get_team_members_with_permission(notify_db, notify_db_session, sample_user): + active_service = create_sample_service(notify_db, notify_db_session, service_name="Active service", user=sample_user) + + users_w_permission_1 = permission_dao.get_team_members_with_permission(active_service.id, MANAGE_SETTINGS) + assert users_w_permission_1 == [sample_user] + + permission_dao.remove_user_service_permissions(user=sample_user, service=active_service) + users_w_permission_2 = permission_dao.get_team_members_with_permission(active_service.id, MANAGE_SETTINGS) + assert users_w_permission_2 == [] + + users_w_permission_3 = permission_dao.get_team_members_with_permission(active_service.id, None) + assert users_w_permission_3 == [] diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 80a061cbe9..73c027ac46 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1482,18 +1482,42 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db def test_remove_user_from_service(notify_db, notify_db_session, client, sample_user_service_permission, mocker): mocked_salesforce_client = mocker.patch("app.service.rest.salesforce_client") - second_user = create_user(email="new@digital.cabinet-office.gov.uk") + third_user = create_user(email="new@digital.cabinet-office.gov.uk") + fourth_user = create_user(email="new@cds-snc.ca") # Simulates successfully adding a user to the service - second_permission = create_sample_user_service_permission(notify_db, notify_db_session, user=second_user) + third_permission = create_sample_user_service_permission(notify_db, notify_db_session, user=third_user) + create_sample_user_service_permission(notify_db, notify_db_session, user=fourth_user) endpoint = url_for( "service.remove_user_from_service", - service_id=str(second_permission.service.id), - user_id=str(second_permission.user.id), + service_id=str(third_permission.service.id), + user_id=str(third_permission.user.id), ) auth_header = create_authorization_header() resp = client.delete(endpoint, headers=[("Content-Type", "application/json"), auth_header]) assert resp.status_code == 204 - mocked_salesforce_client.engagement_delete_contact_role.assert_called_with(second_permission.service, second_permission.user) + mocked_salesforce_client.engagement_delete_contact_role.assert_called_with(third_permission.service, third_permission.user) + + +def test_remove_user_from_service_only_user_with_manage_perm( + notify_api, notify_db, notify_db_session, client, sample_user_service_permission, mocker +): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + manage_settings_user = sample_user_service_permission.user + second_user = create_user(email="new2@cds-snc.ca") + third_user = create_user(email="new3@cds-snc.ca") + create_sample_user_service_permission(notify_db, notify_db_session, user=second_user, permission="view_activity") + create_sample_user_service_permission(notify_db, notify_db_session, user=third_user, permission="view_activity") + endpoint = url_for( + "service.remove_user_from_service", + service_id=str(sample_user_service_permission.service.id), + user_id=str(manage_settings_user.id), + ) + auth_header = create_authorization_header() + resp = client.delete(endpoint, headers=[("Content-Type", "application/json"), auth_header]) + assert resp.status_code == 400 + result = resp.json + assert result["message"] == "SERVICE_NEEDS_USER_W_MANAGE_SETTINGS_PERM" def test_remove_non_existant_user_from_service(client, sample_user_service_permission): @@ -1523,6 +1547,25 @@ def test_cannot_remove_only_user_from_service(notify_api, notify_db, notify_db_s assert result["message"] == "You cannot remove the only user for a service" +def test_remove_user_from_service_with_2_users( + notify_api, notify_db, notify_db_session, client, sample_user_service_permission, mocker +): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + second_user = create_user(email="new@cds-snc.ca") + create_sample_user_service_permission(notify_db, notify_db_session, user=second_user, permission="view_activity") + endpoint = url_for( + "service.remove_user_from_service", + service_id=str(sample_user_service_permission.service.id), + user_id=str(sample_user_service_permission.user.id), + ) + auth_header = create_authorization_header() + resp = client.delete(endpoint, headers=[("Content-Type", "application/json"), auth_header]) + assert resp.status_code == 400 + result = resp.json + assert result["message"] == "SERVICE_CANNOT_HAVE_LT_2_MEMBERS" + + # This test is just here verify get_service_and_api_key_history that is a temp solution # until proper ui is sorted out on admin app def test_get_service_and_api_key_history(notify_api, notify_db, notify_db_session, sample_service):