diff --git a/app/dao/permissions_dao.py b/app/dao/permissions_dao.py index 07e631043..508075932 100644 --- a/app/dao/permissions_dao.py +++ b/app/dao/permissions_dao.py @@ -57,6 +57,10 @@ class PermissionDAO(DAOClass): permission = Permission(permission=name, user=user, service=service) self.create_instance(permission, _commit=False) + def remove_user_service_permissions(self, user, service): + query = self.get_query(filter_by_dict={'user': user.id, 'service': service.id}) + query.delete() + def set_user_permission(self, user, permissions): try: query = self.get_query(filter_by_dict={'user': user.id}) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 9c66e30bc..7c6a056a0 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -45,6 +45,14 @@ def dao_add_user_to_service(service, user): def dao_remove_user_from_service(service, user): - service.users.remove(user) - db.session.add(service) - db.session.commit() + try: + from app.dao.permissions_dao import permission_dao + permission_dao.remove_user_service_permissions(user, service) + service.users.remove(user) + db.session.add(service) + except Exception as e: + # Proper clean up + db.session.rollback() + raise e + else: + db.session.commit() diff --git a/app/errors.py b/app/errors.py index 7617e0add..ac0f7bf60 100644 --- a/app/errors.py +++ b/app/errors.py @@ -40,6 +40,8 @@ def register_errors(blueprint): @blueprint.app_errorhandler(500) def internal_server_error(e): + if current_app.config.get('DEBUG'): + raise e if isinstance(e, str): current_app.logger.exception(e) elif isinstance(e, Exception): @@ -58,5 +60,7 @@ def register_errors(blueprint): @blueprint.app_errorhandler(SQLAlchemyError) def db_error(e): + if current_app.config.get('DEBUG'): + raise e current_app.logger.exception(e) return jsonify(result='error', message=str(e)), 500 diff --git a/app/service/rest.py b/app/service/rest.py index 0bead75a2..6fb76a032 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -20,7 +20,8 @@ from app.dao.services_dao import ( dao_create_service, dao_update_service, dao_fetch_all_services_by_user, - dao_add_user_to_service + dao_add_user_to_service, + dao_remove_user_from_service ) from app.dao.users_dao import get_model_users @@ -159,6 +160,22 @@ def add_user_to_service(service_id, user_id): return jsonify(data=data), 201 +@service.route('//users/', methods=['DELETE']) +def remove_user_from_service(service_id, user_id): + service = dao_fetch_service_by_id(service_id) + user = get_model_users(user_id=user_id) + if user not in service.users: + return jsonify( + result='error', + message='User not found'), 404 + elif len(service.users) == 1: + return jsonify( + result='error', + message='You cannot remove the only user for a service'), 400 + dao_remove_user_from_service(service, user) + return jsonify({}), 204 + + def _process_permissions(user, service, permission_groups): from app.permissions_utils import get_permissions_by_group from app.dao.permissions_dao import permission_dao diff --git a/config.py b/config.py index 0bf45618c..dde70a44e 100644 --- a/config.py +++ b/config.py @@ -84,7 +84,7 @@ class Development(Config): class Test(Development): - pass + DEBUG = True configs = { 'live': 'config_live.Live', diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b4eddbb78..e79b5fe76 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -4,7 +4,7 @@ from app import (email_safe, db) from app.models import ( User, Service, Template, ApiKey, Job, Notification, InvitedUser, Permission) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) -from app.dao.services_dao import dao_create_service +from app.dao.services_dao import (dao_create_service, dao_add_user_to_service) from app.dao.templates_dao import dao_create_template from app.dao.api_key_dao import save_model_api_key from app.dao.jobs_dao import dao_create_job @@ -116,6 +116,9 @@ def sample_service(notify_db, if not service: service = Service(**data) dao_create_service(service, user) + else: + if user not in service.users: + dao_add_user_to_service(service, user) return service @@ -394,7 +397,7 @@ def sample_service_permission(notify_db, if user is None: user = sample_user(notify_db, notify_db_session) if service is None: - service = sample_service(notify_db, notify_db_session) + service = sample_service(notify_db, notify_db_session, user=user) data = { 'user': user, 'service': service, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index cc62b0e70..960d66543 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -5,7 +5,9 @@ from app.dao.users_dao import save_model_user from app.dao.services_dao import dao_remove_user_from_service from app.models import User from tests import create_authorization_header -from tests.app.conftest import sample_service as create_service +from tests.app.conftest import sample_service as create_sample_service +from tests.app.conftest import sample_service_permission as create_sample_service_permission +from tests.app.conftest import sample_user as create_sample_user def test_get_service_list(notify_api, service_factory): @@ -358,7 +360,7 @@ def test_should_not_update_service_with_duplicate_name(notify_api, with notify_api.test_request_context(): with notify_api.test_client() as client: service_name = "another name" - another_service = create_service( + another_service = create_sample_service( notify_db, notify_db_session, service_name=service_name, @@ -837,3 +839,72 @@ def test_add_unknown_user_to_service_returns404(notify_api, notify_db, notify_db assert resp.status_code == 404 assert result['result'] == 'error' assert result['message'] == expected_message + + +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + second_user = create_sample_user( + notify_db, + notify_db_session, + email="new@digital.cabinet-office.gov.uk") + # Simulates successfully adding a user to the service + second_permission = create_sample_service_permission( + notify_db, + notify_db_session, + user=second_user) + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(second_permission.service.id), + user_id=str(second_permission.user.id)) + auth_header = create_authorization_header( + path=endpoint, + method='DELETE' + ) + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 204 + + +def test_remove_user_from_service(notify_api, notify_db, notify_db_session, sample_service_permission): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + second_user = create_sample_user( + notify_db, + notify_db_session, + email="new@digital.cabinet-office.gov.uk") + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(sample_service_permission.service.id), + user_id=str(second_user.id)) + auth_header = create_authorization_header( + path=endpoint, + method='DELETE' + ) + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 404 + + +def test_cannot_remove_only_user_from_service(notify_api, + notify_db, + notify_db_session, + sample_service_permission): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + endpoint = url_for( + 'service.remove_user_from_service', + service_id=str(sample_service_permission.service.id), + user_id=str(sample_service_permission.user.id)) + auth_header = create_authorization_header( + path=endpoint, + method='DELETE' + ) + resp = client.delete( + endpoint, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 400 + result = json.loads(resp.get_data(as_text=True)) + assert result['message'] == 'You cannot remove the only user for a service'