Skip to content

Commit

Permalink
deposits: update 'permissions' action method to better handle exception
Browse files Browse the repository at this point in the history 
* closes cernanalysispreservation#2023

Signed-off-by: Ilias Koutsakis <[email protected]>
  • Loading branch information
@Lilykos
Lilykos committed Feb 1, 2021
1 parent 9a50258 commit c94d4de
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 82 deletions.
145 changes: 73 additions & 72 deletions cap/modules/deposit/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -428,55 +428,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)
Expand Down Expand Up @@ -505,45 +486,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."""
Expand Down
25 changes: 15 additions & 10 deletions tests/integration/test_permissions_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,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']
Expand All @@ -196,11 +196,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']
Expand All @@ -218,8 +220,7 @@ 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


@mark.skip('to discuss if this should be done this way')
Expand Down Expand Up @@ -481,8 +482,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',
Expand Down Expand Up @@ -535,8 +538,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',
Expand Down

0 comments on commit c94d4de

Please sign in to comment.