Skip to content

Commit

Permalink
deposits: update 'permissions' action methods
Browse files Browse the repository at this point in the history 
* refactors the code so that when an action is not possible,
  it fails silently, and the flow doesn't stop
* closes cernanalysispreservation#2023

Signed-off-by: Ilias Koutsakis <[email protected]>
  • Loading branch information
@Lilykos
Lilykos committed Feb 23, 2021
1 parent 47dbc8b commit db527f9
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 86 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
73 changes: 59 additions & 14 deletions tests/integration/test_permissions_api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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']
Expand All @@ -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']
Expand All @@ -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):
Expand All @@ -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']

Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down

0 comments on commit db527f9

Please sign in to comment.