1696 update rest api for access control #1704

aphelionz · 2016-12-09T19:58:41Z

Starting the PR process for this so I can keep an eye on Jenkins while I do the next parts. - will update with docs and tests over the next day or so.

This exposes a new endpoint at /hsapi/resource/[id]/access/.

A GET request by a resource owner will show all users and groups associated
A GET request by a non-owners will only show their access and group access
A PUT request by an owner with a 'group_id' or 'user_id', along with a 'privilege' int will add access
A DELETE request by an owner with a group_id or user_id will remove access.

…PI-for-access-control

pkdash · 2016-12-20T06:16:07Z

@aphelionz This PR needs some update to resolve conflicts as well failing test.

aphelionz · 2016-12-20T17:31:52Z

@pkdash Thanks for the nudge :) All set.

pkdash · 2016-12-21T06:02:21Z

hs_core/views/resource_access_api.py

+
+class ResourceAccessUpdateDelete(generics.RetrieveUpdateDestroyAPIView):
+    """
+    Read, update, or delete a resource


Probably this comment be "Read, update or delete access permission for a resource"

pkdash · 2016-12-21T06:30:16Z

hs_core/views/resource_access_api.py

+    :return: (on success): Success or Error JSON object
+
+    :type str
+    :param user_id: user ID to remove


Parameter description seems not correct.

pkdash · 2016-12-21T06:34:13Z

hs_core/views/resource_access_api.py

+        message = "Request must contain a 'resource' ID as well as a 'user_id' or 'group_id'"
+        return Response(
+            data={'error': message},
+            status=status.HTTP_200_OK


Should the status code be HTTP_400_BAD_REQUEST?

pkdash · 2016-12-21T06:43:20Z

hs_core/views/resource_access_api.py

+        message = "Request must contain a 'resource' ID as well as a 'user_id' or 'group_id'"
+        return Response(
+            data={'error': message},
+            status=status.HTTP_200_OK


status code BAD_REQUEST?

pkdash · 2016-12-21T07:00:21Z

hs_core/views/resource_access_api.py

+    def get_queryset(self, pk, user):
+        resource = hydroshare.get_resource_by_shortkey(shortkey=pk)
+
+        if resource.user_id == user.id:


Not sure how this if statement works. I don't think resource object has attribute/property user_id. user can be an anonymous user.

This works. It allows resource owners to see all resource access but non-owners to only see their own access.

That being said i'm perfectly amenable to a different way of doing this

There is nothing wrong about the logic. I was just wondering how you are able to access the user_id of the resource object. Does the resource object has such an attribute?

Indeed it does! @pkdash

@aphelionz I assume we are able to access an instance of user from an instance of resource due to the inheritance of Resource class from Ownable (mezzanine code) class.

@aphelionz It seems the user_id is the id of the user who originally created the resource. So we can't use user_id to check if the requesting user is the owner of the resource. I think the following check should work:
if user in resource.raccess.owners:

pkdash · 2016-12-21T07:04:15Z

hs_core/views/utils.py

+        elif needed_permission == ACTION_TO_AUTHORIZE.VIEW_RESOURCE_ACCESS:
+            authorized = user.uaccess.can_view_resource(res)
+        elif needed_permission == ACTION_TO_AUTHORIZE.EDIT_RESOURCE_ACCESS:
+            authorized = user.uaccess.owns_resource(res)


Should this be authorized = user.uaccess.can_edit_resource(res)?

I guess that's how the UI works, but we should definitely clarify if we want any editor to edit resource access, or only the owner.

@alvacouch can you clarify this?

@alvacouch ping

@aphelionz @pkdash This does not conform to the engine. According to the engine, anyone can share something with someone else, but only at the privilege that person holds. So, being able to edit permissions requires VIEW access..... but that isn't enough..... what do you mean by "edit"?

user.uaccess.can_share_resource is appropriate.

pkdash · 2016-12-21T07:11:39Z

hs_core/tests/api/rest/test_set_access_rules.py

+
+    def tearDown(self):
+        super(TestSetAccessRules, self).tearDown()
+        self.secondUser.delete()


Do we need these deletes?

I think yes, based on the tearDown method in /hs_core/tests/api/rest/base.py. Am I wrong?

I am not sure if the implementation of teardown() in base.py is correct. Shouldn't it be calling the super()?. I assume the APITestCase is an extension of the Django's TestCase class. When subclassing the TestCase I believe there is no need to override the teardown() for database cleanup as that's the default behavior. I may be wrong.

I'm not sure either. Who can answer this? @alvacouch ?

@aphelionz @pkdash I have not looked this over much.... I didn't write this suite of tests. Check hs_access_control/tests for mine.....

@alvacouch In you tests it seems you are also explicitly deleting the database objects using the function global_reset(). Also, it seems all access control tests pass without the use of global_reset().

In general, the tests don't reset, and using --keepdb leads to different results every time. I have made a practice of resetting and cleaning up after myself, which at least insures that --keepdb works for me. If you repeat test --keepdb several times, you will see the number of errors grow as state accumulates, and only the tests for which there is local cleanup work properly in --keepdb. The initial global_reset is redundant, I admit, but it is without cost.

There are some methods in the super that might be useful on occasion. In this case, they don't seem to be relevant.

I then am OK with leaving the deletes in trearDown().

pkdash · 2016-12-21T07:14:50Z

hs_core/tests/api/rest/test_set_access_rules.py

+        self.assertEqual(
+            "Request cannot contain both a 'user_id' and a 'group_id' parameter.",
+            put_response.data['error'])
+


Another error case you may consider where a user who does not have sufficient permission on the resource tries to grant/revoke permission.

Yes, I will add negative cases.

Negative cases added :)

…PI-for-access-control

pkdash · 2017-01-10T05:16:36Z

hs_core/views/resource_access_api.py

+        else:
+            querysets = (
+                UserResourcePrivilege.objects.filter(resource=resource, user=user),
+                GroupResourcePrivilege.objects.filter(resource=resource, user=user)


GroupResourcePrivelege class doesn't have user field.

pkdash · 2017-01-10T05:31:34Z

hs_core/views/resource_access_api.py

+
+        if "user_id" in keys and "privilege" in keys:
+            user_to_add = utils.user_from_id(request.data['user_id'])
+            user_access.share_resource_with_user(resource, user_to_add, request.data['privilege'])


Should this also be within try ... except?

Also should we be validating the value in request.data['privilege'] before using it?

pkdash · 2017-01-10T05:34:22Z

@aphelionz I have left few additional comments. It would be good to close this PR soon.

…PI-for-access-control

aphelionz

@pkdash All current feedback is implemented or otherwise handled. Let me know!

aphelionz · 2017-01-10T17:23:05Z

hs_core/views/utils.py

+        elif needed_permission == ACTION_TO_AUTHORIZE.VIEW_RESOURCE_ACCESS:
+            authorized = user.uaccess.can_view_resource(res)
+        elif needed_permission == ACTION_TO_AUTHORIZE.EDIT_RESOURCE_ACCESS:
+            authorized = user.uaccess.owns_resource(res)


pkdash · 2017-01-12T04:44:41Z

@aphelionz Looks good. +1

aphelionz added 3 commits December 8, 2016 16:02

Initial commit for resource ACL GET and DELETE

b8fba5e

Merge remote-tracking branch 'origin/develop' into 1696-update-REST-A…

ab9c01a

…PI-for-access-control

Adding PUT method for resources

dbcfccb

mjstealey added the in progress label Dec 9, 2016

pkdash self-assigned this Dec 11, 2016

aphelionz added 6 commits December 13, 2016 12:24

Tests

7cae6ea

Docs for access rules API

1d86bd1

PEP8

64a01bd

More PEP8

c8076c3

Fixing failed test

a0cd1ae

Merge remote-tracking branch 'origin/develop' into 1696-update-REST-A…

358577e

…PI-for-access-control

pkdash requested review from alvacouch and pkdash December 17, 2016 06:20

pkdash assigned alvacouch Dec 17, 2016

aphelionz added 3 commits December 20, 2016 11:22

Merging in develop

095db35

Submodule update

4ea0e73

Updating tests

d6a2263

pkdash reviewed Dec 21, 2016

View reviewed changes

aphelionz added 3 commits December 21, 2016 14:36

Merge remote-tracking branch 'origin/develop' into 1696-update-REST-A…

bb4d00a

…PI-for-access-control

Quick fixes based on code review

bf985d6

Negative test case

7b0d5f7

pkdash reviewed Jan 10, 2017

View reviewed changes

Mark Henderson added 5 commits January 10, 2017 11:15

Merge remote-tracking branch 'origin/develop' into 1696-update-REST-A…

b44f553

…PI-for-access-control

Switching to raccess.owners

d89776f

Fixing group query

a663c42

Fixing resource permissions in utils.py

388cc64

Fixing tests and validating privilege

cb4365a

aphelionz commented Jan 11, 2017

View reviewed changes

PEP8

f1405e2

aphelionz merged commit 09878d5 into develop Jan 12, 2017

mjstealey removed the in progress label Jan 12, 2017

mjstealey deleted the 1696-update-REST-API-for-access-control branch February 16, 2017 16:51

1696 update rest api for access control #1704

1696 update rest api for access control #1704

Conversation

aphelionz commented Dec 9, 2016

pkdash commented Dec 20, 2016

aphelionz commented Dec 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkdash commented Jan 10, 2017

aphelionz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkdash commented Jan 12, 2017