deposits: update 'permissions' action methods

* refactors the code so that when an action is not possible,
  it fails silently, and the flow doesn't stop
* closes #2023

Signed-off-by: Ilias Koutsakis <ilias.koutsakis@cern.ch>

Author: Lilykos

cap/modules/deposit/api.py

@@ -431,55 +431,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)
@@ -508,45 +489,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."""

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,13 @@ 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
+
+    resp = client.get('/deposits/', headers=auth_headers_for_superuser)
+    assert owner.email in resp.json['hits']['hits'][0]['access']['deposit-read']['users']
 
 
-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,10 +221,50 @@ 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
+    time.sleep(1)
+
+    resp = client.get('/deposits/', headers=auth_headers_for_superuser)
+    read_users = resp.json['hits']['hits'][0]['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(
     client, users, auth_headers_for_user, create_deposit, json_headers):
@@ -242,10 +285,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 +522,10 @@ 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
+
+    resp = client.get('/deposits/', headers=auth_headers_for_user(owner))
+    assert owner.email in resp.json['hits']['hits'][0]['access']['deposit-read']['users']
 
     resp = client.post(
         f'/deposits/{pid}/actions/permissions',
@@ -535,8 +578,10 @@ 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
+
+    resp = client.get('/deposits/', headers=auth_headers_for_user(owner))
+    assert owner.email in resp.json['hits']['hits'][0]['access']['deposit-read']['users']
 
     resp = client.post(
         f'/deposits/{pid}/actions/permissions',