diff --git a/cap/modules/deposit/api.py b/cap/modules/deposit/api.py index 4962f3ecd7..07aaabf178 100644 --- a/cap/modules/deposit/api.py +++ b/cap/modules/deposit/api.py @@ -423,55 +423,36 @@ def edit_permissions(self, data): }] """ - with db.session.begin_nested(): - for obj in data: - if obj['type'] == 'user': - try: - user = get_existing_or_register_user(obj['email']) - except DoesNotExistInLDAP: - raise UpdateDepositPermissionsError( - 'User with this mail does not exist in LDAP.') - - if obj['op'] == 'add': - try: - self._add_user_permissions(user, [obj['action']], - db.session) - except IntegrityError: - raise UpdateDepositPermissionsError( - 'Permission already exist.') - - elif obj['op'] == 'remove': - try: - self._remove_user_permissions( - user, [obj['action']], db.session) - except NoResultFound: - raise UpdateDepositPermissionsError( - 'Permission does not exist.') - - elif obj['type'] == 'egroup': - try: - role = get_existing_or_register_role(obj['email']) - except DoesNotExistInLDAP: - raise UpdateDepositPermissionsError( - 'Egroup with this mail does not exist in LDAP.') - - if obj['op'] == 'add': - try: - self._add_egroup_permissions( - role, [obj['action']], db.session) - except IntegrityError: - raise UpdateDepositPermissionsError( - 'Permission already exist.') - elif obj['op'] == 'remove': - try: - self._remove_egroup_permissions( - role, [obj['action']], db.session) - except NoResultFound: - raise UpdateDepositPermissionsError( - 'Permission does not exist.') + for obj in data: + if obj['type'] == 'user': + try: + user = get_existing_or_register_user(obj['email']) + except DoesNotExistInLDAP: + raise UpdateDepositPermissionsError( + 'User with this mail does not exist in LDAP.') + + if obj['op'] == 'add': + self._add_user_permissions( + user, [obj['action']], db.session) + elif obj['op'] == 'remove': + self._remove_user_permissions( + user, [obj['action']], db.session) + + elif obj['type'] == 'egroup': + try: + role = get_existing_or_register_role(obj['email']) + except DoesNotExistInLDAP: + raise UpdateDepositPermissionsError( + 'Egroup with this mail does not exist in LDAP.') + + if obj['op'] == 'add': + self._add_egroup_permissions( + role, [obj['action']], db.session) + elif obj['op'] == 'remove': + self._remove_egroup_permissions( + role, [obj['action']], db.session) self.commit() - return self @preserve(result=False, fields=PRESERVE_FIELDS) @@ -500,45 +481,65 @@ def commit(self, *args, **kwargs): def _add_user_permissions(self, user, permissions, session): """Adds permissions for user for this deposit.""" for permission in permissions: - session.add( - ActionUsers.allow(DEPOSIT_ACTIONS_NEEDS(self.id)[permission], - user=user)) - - session.flush() - - self['_access'][permission]['users'].append(user.id) + try: + session.add( + ActionUsers.allow( + DEPOSIT_ACTIONS_NEEDS(self.id)[permission], + user=user) + ) + session.flush() + except IntegrityError: + session.rollback() + + if user.id not in self['_access'][permission]['users']: + self['_access'][permission]['users'].append(user.id) def _remove_user_permissions(self, user, permissions, session): """Remove permissions for user for this deposit.""" for permission in permissions: - session.delete( - ActionUsers.query.filter(ActionUsers.action == permission, - ActionUsers.argument == str(self.id), - ActionUsers.user_id == user.id).one()) - session.flush() + try: + session.delete( + ActionUsers.query.filter( + ActionUsers.action == permission, + ActionUsers.argument == str(self.id), + ActionUsers.user_id == user.id).one() + ) + session.flush() + except NoResultFound: + session.rollback() - self['_access'][permission]['users'].remove(user.id) + if user.id in self['_access'][permission]['users']: + self['_access'][permission]['users'].remove(user.id) def _add_egroup_permissions(self, egroup, permissions, session): for permission in permissions: - session.add( - ActionRoles.allow(DEPOSIT_ACTIONS_NEEDS(self.id)[permission], - role=egroup)) - session.flush() + try: + session.add( + ActionRoles.allow( + DEPOSIT_ACTIONS_NEEDS(self.id)[permission], + role=egroup) + ) + session.flush() + except IntegrityError: + session.rollback() if egroup.id not in self['_access'][permission]['roles']: self['_access'][permission]['roles'].append(egroup.id) def _remove_egroup_permissions(self, egroup, permissions, session): for permission in permissions: - session.delete( - ActionRoles.query.filter( - ActionRoles.action == permission, - ActionRoles.argument == str(self.id), - ActionRoles.role_id == egroup.id).one()) - session.flush() - - self['_access'][permission]['roles'].remove(egroup.id) + try: + session.delete( + ActionRoles.query.filter( + ActionRoles.action == permission, + ActionRoles.argument == str(self.id), + ActionRoles.role_id == egroup.id).one()) + session.flush() + except NoResultFound: + session.rollback() + + if egroup.id in self['_access'][permission]['roles']: + self['_access'][permission]['roles'].remove(egroup.id) def _add_experiment_permissions(self, experiment, permissions): """Add read permissions to everybody assigned to experiment.""" diff --git a/tests/integration/test_permissions_api.py b/tests/integration/test_permissions_api.py index fa6f72de9b..4ffe2e2df9 100644 --- a/tests/integration/test_permissions_api.py +++ b/tests/integration/test_permissions_api.py @@ -27,6 +27,7 @@ from __future__ import absolute_import, print_function import json +import time from flask import current_app from werkzeug.local import LocalProxy @@ -178,7 +179,7 @@ def test_add_permissions_for_user_that_doesnt_exist_in_ldap_returns_400( 'message'] == 'User with this mail does not exist in LDAP.' -def test_add_permissions_when_permission_already_exist_returns_400( +def test_add_permissions_when_permission_already_exist_returns_201( client, users, auth_headers_for_superuser, create_deposit, json_headers): owner = users['cms_user'] pid = create_deposit(owner, 'test-v1.0.0')['_deposit']['id'] @@ -196,11 +197,14 @@ def test_add_permissions_when_permission_already_exist_returns_400( ]), ) - assert resp.status_code == 400 - assert resp.json['message'] == 'Permission already exist.' + assert resp.status_code == 201 + depid = resp.json['id'] + + resp = client.get(f'/deposits/{depid}', headers=auth_headers_for_superuser) + assert owner.email == resp.json['access']['deposit-read']['users'][0]['email'] -def test_add_permissions_when_permission_doesnt_exist_returns_400( +def test_add_permissions_when_permission_doesnt_exist_returns_201( client, users, auth_headers_for_superuser, create_deposit, json_headers): owner, other_user = users['cms_user'], users['cms_user2'] pid = create_deposit(owner, 'test-v1.0.0')['_deposit']['id'] @@ -218,9 +222,51 @@ def test_add_permissions_when_permission_doesnt_exist_returns_400( ]), ) - assert resp.status_code == 400 - assert resp.json['message'] == 'Permission does not exist.' + assert resp.status_code == 201 + + +def test_add_multiple_permissions_with_one_failing_adds_the_others( + client, users, auth_headers_for_superuser, create_deposit, json_headers): + owner, other_user = users['cms_user'], users['cms_user2'] + third_user = users['lhcb_user'] + pid = create_deposit(owner, 'test-v1.0.0')['_deposit']['id'] + + resp = client.post( + f'/deposits/{pid}/actions/permissions', + headers=auth_headers_for_superuser + json_headers, + data=json.dumps([ + { + 'email': owner.email, # this should fail, the rest should work + 'type': 'user', + 'op': 'add', + 'action': 'deposit-read' + }, + { + 'email': other_user.email, + 'type': 'user', + 'op': 'add', + 'action': 'deposit-read' + }, + { + 'email': third_user.email, + 'type': 'user', + 'op': 'add', + 'action': 'deposit-read' + } + ]), + ) + assert resp.status_code == 201 + depid = resp.json['id'] + + time.sleep(1) + + resp = client.get(f'/deposits/{depid}', headers=auth_headers_for_superuser) + read_users = [user['email'] for user in resp.json['access']['deposit-read']['users']] + + assert owner.email in read_users + assert other_user.email in read_users + assert third_user.email in read_users @mark.skip('to discuss if this should be done this way') def test_add_permissions_when_add_admin_permissions_add_all_the_others_as_well( @@ -242,10 +288,8 @@ def test_add_permissions_when_add_admin_permissions_add_all_the_others_as_well( ) -def test_add_ermissions_gives_permissions_for_user(client, users, - auth_headers_for_user, - create_deposit, - json_headers): +def test_add_permissions_gives_permissions_for_user( + client, users, auth_headers_for_user, create_deposit, json_headers): owner, other_user = users['cms_user'], users['cms_user2'] pid = create_deposit(owner, 'test-v1.0.0')['_deposit']['id'] @@ -481,8 +525,11 @@ def test_change_permissions_for_egroup_is_not_case_sensitive( ]), ) - assert resp.status_code == 400 - assert resp.json['message'] == 'Permission already exist.' + assert resp.status_code == 201 + depid = resp.json['id'] + + resp = client.get(f'/deposits/{depid}', headers=auth_headers_for_user(owner)) + assert owner.email == resp.json['access']['deposit-read']['users'][0]['email'] resp = client.post( f'/deposits/{pid}/actions/permissions', @@ -535,8 +582,11 @@ def test_change_permissions_for_user_is_not_case_sensitive( ]), ) - assert resp.status_code == 400 - assert resp.json['message'] == 'Permission already exist.' + assert resp.status_code == 201 + depid = resp.json['id'] + + resp = client.get(f'/deposits/{depid}', headers=auth_headers_for_user(owner)) + assert owner.email == resp.json['access']['deposit-read']['users'][0]['email'] resp = client.post( f'/deposits/{pid}/actions/permissions',