Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deposits: update 'permissions' action method to better handle exception #2027

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

deposits: update 'permissions' action method to better handle exception #2027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 73 additions & 72 deletions cap/modules/deposit/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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."""
Expand Down
78 changes: 64 additions & 14 deletions tests/integration/test_permissions_api.py
View file
Open in desktop
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,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(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a test with submitting multiple permissions with one failing to check .rollback

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,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(
Expand All @@ -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']

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