Skip to content

Commit

Permalink
Jkmarx/fix perms api error (#2959)
Browse files Browse the repository at this point in the history 
* Fix bug related to incorrect perms usage.

* Add unit tests to cover perms filter in api.

* Remove inaccurate comment.
  • Loading branch information
@jkmarx
jkmarx committed Aug 29, 2018
1 parent f6b5665 commit d63e82d
Show file tree
Hide file tree
Showing 3 changed files with 98 additions and 16 deletions.
5 changes: 2 additions & 3 deletions refinery/core/api.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ def transform_res_list(self, user, res_list, request, **kwargs):
owned_res_set = Set(
get_objects_for_user(
user,
'core.add_%s' %
'core.share_%s' %
self.res_type._meta.verbose_name,
use_groups=False
).values_list("id", flat=True))
Expand Down Expand Up @@ -698,8 +698,7 @@ def get_by_db_id(self, request, **kwargs):
if group.group == ExtendedGroup.objects.public_group():
is_public = True

# get_owner in core models uses add to distinguish owner
is_owner = request.user.has_perm('core.add_dataset', ds)
is_owner = request.user.has_perm('core.share_dataset', ds)

try:
user_uuid = request.user.profile.uuid
Expand Down
107 changes: 95 additions & 12 deletions refinery/core/test_views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,24 @@ def setUp(self):
format="json"
)
self.options_response = self.view(self.options_request)
self.user_2 = User.objects.create_user('jane_lab',
'jane@fake.com',
'coffeecoffee')
self.user_2_data_set = create_dataset_with_necessary_models(
user=self.user_2
)
self.user_3 = User.objects.create_user('john_lab',
'john@fake.com',
'coffeecoffee')
self.user_3_data_set = create_dataset_with_necessary_models(
user=self.user_3
)
self.user_2_data_set.share(ExtendedGroup.objects.public_group())
self.user_3_data_set.share(ExtendedGroup.objects.public_group())
self.group = ExtendedGroup.objects.create(name="Test Group")
self.user_2_data_set.share(self.group)
self.group.user_set.add(self.user_2)
self.group.user_set.add(self.user_3)

def test_unallowed_http_verbs(self):
self.assertEqual(
Expand All @@ -116,9 +134,9 @@ def test_get_data_set_pagination_offset(self):
get_request = self.factory.get(self.url_root, params)
get_request.user = self.user
get_response = self.view(get_request)
self.assertEqual(len(get_response.data.get('data_sets')), 2)
self.assertEqual(get_response.data.get('total_data_sets'), 5)
self.assertEqual(get_response.data.get('data_sets')[0].get('uuid'),
self.data_set_2.uuid)
self.user_3_data_set.uuid)

def test_get_data_set_pagination_limit(self):
data_set_3 = create_dataset_with_necessary_models(user=self.user)
Expand All @@ -130,15 +148,80 @@ def test_get_data_set_pagination_limit(self):
self.assertEqual(get_response.data.get('data_sets')[0].get('uuid'),
data_set_3.uuid)

def test_get_returns_only_owned(self):
get_request = self.factory.get(self.url_root, {'is_owner': True})
get_request.user = self.user_2
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 1)
self.assertEqual(get_response.data.get('data_sets')[0]['uuid'],
self.user_2_data_set.uuid)

def test_get_returns_only_public(self):
get_request = self.factory.get(self.url_root, {'is_public': True})
get_request.user = self.user_2
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 2)

def test_get_returns_public_owned(self):
get_request = self.factory.get(self.url_root,
{'is_public': True,
'is_owner': True})
get_request.user = self.user_2
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 1)
self.assertEqual(get_response.data.get('data_sets')[0]['uuid'],
self.user_2_data_set.uuid)

def test_get_returns_public_group(self):
get_request = self.factory.get(self.url_root,
{'is_public': True,
'group': self.group.id})
get_request.user = self.user_3
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 1)
self.assertEqual(get_response.data.get('data_sets')[0]['uuid'],
self.user_2_data_set.uuid)

def test_get_returns_owned_group(self):
get_request = self.factory.get(self.url_root,
{'is_owner': True,
'group': self.group.id})
create_dataset_with_necessary_models(user=self.user_2) # not shared
get_request.user = self.user_2
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 1)
self.assertEqual(get_response.data.get('data_sets')[0]['uuid'],
self.user_2_data_set.uuid)

def test_get_returns_owned_group_public(self):
get_request = self.factory.get(self.url_root,
{'is_public': True,
'is_owner': True,
'group': self.group.id})
get_request.user = self.user_2
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 1)
self.assertEqual(get_response.data.get('data_sets')[0]['uuid'],
self.user_2_data_set.uuid)
get_request.user = self.user_3 # user_3 does not share data_set
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 0)

def test_get_returns_only_grouped(self):
get_request = self.factory.get(self.url_root, {'group': self.group.id})
get_request.user = self.user_2
get_response = self.view(get_request)
self.assertEqual(get_response.data.get('total_data_sets'), 1)

def test_get_data_set_pagination_limit_and_offset(self):
create_dataset_with_necessary_models(user=self.user)
params = {'limit': 1, 'offset': 2}
get_request = self.factory.get(self.url_root, params)
get_request.user = self.user
get_response = self.view(get_request)
self.assertEqual(len(get_response.data.get('data_sets')), 1)
self.assertEqual(get_response.data.get('total_data_sets'), 5)
self.assertEqual(get_response.data.get('data_sets')[0].get('uuid'),
self.data_set.uuid)
self.user_2_data_set.uuid)

def test_total_data_sets_returned_correctly(self):
create_dataset_with_necessary_models(user=self.user)
Expand All @@ -150,7 +233,7 @@ def test_total_data_sets_returned_correctly(self):

def test_dataset_delete_successful(self):

self.assertEqual(DataSet.objects.all().count(), 2)
self.assertEqual(DataSet.objects.all().count(), 4)

self.delete_request1 = self.factory.delete(
urljoin(self.url_root, self.data_set.uuid)
Expand All @@ -163,7 +246,7 @@ def test_dataset_delete_successful(self):

self.assertEqual(self.delete_response.status_code, 200)

self.assertEqual(DataSet.objects.all().count(), 1)
self.assertEqual(DataSet.objects.all().count(), 3)

self.delete_request2 = self.factory.delete(
urljoin(self.url_root, self.data_set_2.uuid)
Expand All @@ -175,10 +258,10 @@ def test_dataset_delete_successful(self):
self.data_set_2.uuid)
self.assertEqual(self.delete_response.status_code, 200)

self.assertEqual(DataSet.objects.all().count(), 0)
self.assertEqual(DataSet.objects.all().count(), 2)

def test_dataset_delete_no_auth(self):
self.assertEqual(DataSet.objects.all().count(), 2)
self.assertEqual(DataSet.objects.all().count(), 4)

self.delete_request = self.factory.delete(
urljoin(self.url_root, self.data_set.uuid)
Expand All @@ -189,16 +272,16 @@ def test_dataset_delete_no_auth(self):

self.assertEqual(self.delete_response.status_code, 403)

self.assertEqual(DataSet.objects.all().count(), 2)
self.assertEqual(DataSet.objects.all().count(), 4)

def test_dataset_delete_not_found(self):
self.assertEqual(DataSet.objects.all().count(), 2)
self.assertEqual(DataSet.objects.all().count(), 4)

uuid = self.data_set.uuid

self.data_set.delete()

self.assertEqual(DataSet.objects.all().count(), 1)
self.assertEqual(DataSet.objects.all().count(), 3)

self.delete_request = self.factory.delete(
urljoin(self.url_root, uuid)
Expand All @@ -210,7 +293,7 @@ def test_dataset_delete_not_found(self):

self.assertEqual(self.delete_response.status_code, 404)

self.assertEqual(DataSet.objects.all().count(), 1)
self.assertEqual(DataSet.objects.all().count(), 3)

@mock.patch('core.views.DataSetsViewSet.update_group_perms')
@mock.patch('core.views.DataSetsViewSet.send_transfer_notification_email')
Expand Down
2 changes: 1 addition & 1 deletion refinery/core/views.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -690,7 +690,7 @@ def get(self, request):
for data_set in user_data_sets:
is_public = all_public_perms.has_perm('read_meta_dataset',
data_set)
is_owner = all_owner_perms.has_perm('add_dataset', data_set)
is_owner = all_owner_perms.has_perm('share_dataset', data_set)
setattr(data_set, 'public', is_public)
setattr(data_set, 'is_owner', is_owner)

Expand Down

0 comments on commit d63e82d

Please sign in to comment.