Fixed bug w/ not checking permissions in copy #174

kelockhart · 2023-12-05T15:55:57Z

Bug fix: users were erroneously able to copy their own library into a library they did not have permissions to edit

kelockhart · 2023-12-05T17:17:25Z

biblib/views/operations_view.py

@@ -207,6 +207,10 @@ def post(self, library):
        if data['action'] in ['union', 'intersection', 'difference']:
            if 'libraries' not in data:
                return err(NO_LIBRARY_SPECIFIED_ERROR)
+            for lib in data['libraries']:
+                if not self.read_access(service_uid=user_editing_uid,


This should be a check on if the user has read access, or if the library is public

Just added the access check when library is public. A slug is passed for each library, so we have to convert it to UUID and then get the library to check if it's public. I noticed there was code right below that sort of did that, so I moved some things around.

kelockhart · 2023-12-05T22:22:22Z

biblib/views/operations_view.py

+                    return err(NO_LIBRARY_SPECIFIED_ERROR)
+                for lib in check_access:
+                    lib = session.merge(lib)
+                    if lib.public or not self.read_access(service_uid=user_editing_uid,


Whoops my comment wasn't very clear - this should be if not lib.public or not self.read_access...

kelockhart · 2023-12-06T15:32:27Z

biblib/views/operations_view.py

+                if 'libraries' not in data:
+                    return err(NO_LIBRARY_SPECIFIED_ERROR)
+                for lib in check_access:
+                    lib = session.merge(lib)


I haven't used merge before - lib is the same as secondary, as returned by the query in line 217. Do you need to do the merge into the session, even though this should already be a sqlalchemy object?

Totally get it! The reason I used merge was because the libraries were getting detached from the session. I'm sure there's some other way around it, but I couldn't think of any off the top of my head. If you're okay with holding off on this until tomorrow, I can try to come up with something else.

tjacovich

Overall I think we are nearly there. One quick question on a test and a modification of the logic for checking if we have permissions for the secondary libraries.

tjacovich · 2023-12-13T17:15:49Z

biblib/tests/unit_tests/test_webservices.py

@@ -718,7 +718,7 @@ def test_operations_view_post_types(self):
        self.assertEqual(response.json['error'], NO_LIBRARY_SPECIFIED_ERROR['body'])

        post_data['action'] = 'copy'
-        post_data['libraries'] = ['lib1', 'lib2']
+        post_data['libraries'] = [library_id_2, library_id_2]


I don't think it matters for the test, but did you mean for these to both be the same library?

Yeah, it wasn't relevant so I added lib2. But I changed it now to lib1, just because that's more consistent with how it was before.

tjacovich · 2023-12-13T21:01:21Z

biblib/views/operations_view.py

+                                        library_id=library_uuid):
+                has_read_access_primary = True 
+
+            for lib in data.get('libraries', []):


I think this might miss a corner case where if someone specifies a mix of libraries with various access types. This isn't an issue for the copy method because we limit the number of secondary libraries to 1, but for other methods, they could potentially acquire access to a secondary library they are not supposed to have (ie. a user could compute the union with three libraries where they only have access to two of them and gain access to the documents stored in the third library.) I have to double-check all the methods but a simple first past fix might be to raise an exception immediately if a user has no permissions on a private secondary library.

Thank you for catching that. I had changed the logic a little bit and forgot to account for the implications here. Good one. I'll fix it right away.

tjacovich

I think this PR is basically there. The logic is all sound, I just had one comment on the handling of the primary library in the permissions check.

tjacovich · 2023-12-15T19:10:02Z

biblib/views/operations_view.py

+                )
+
+                if action in ["union", "intersection", "difference"]:
+                    if not has_read_access_primary or not (


Looks good! The only comment I have, and I might be misunderstanding this, is that we can also perform union, intersection, and difference actions with a public primary library. In that case, you might be able to move the logical check for the primary out of the loop and immediately exit if the primary permissions aren't set and then have checks only on the secondary in the loop. Would potentially save some postgres queries.

For the union, intersection, and difference we need a public secondary library, so I think it makes sense as it is right now. Feel free to correct me.

I think you are entirely correct on that point. I'm wondering if has_read_access_primary should be (primary_is_public or has_read_access_primary) as it is in the check for copy. If that is the case we may be able to keep the checks in the loop to only looking at the secondary because the requirements for the primary are the same.

That's the thing. For copy we need (primary_is_public or has_read_access_primary) but for union, intersection, difference we need has_read_access_primary.
At least that is my understanding.

Here's my notes:
copy:

primary: has_read_access or is_public

secondary: has_write_access

union, intersection, difference:

primary: has_read_access

secondary: has_read_access or is_public

Does that make sense to you?
I talked to Kelly and checked a couple of times, but we might have missed something.

kelockhart

Looks good! I think this is ready.

tjacovich

I agree. Looks ready to me.

tjacovich · 2023-12-21T20:44:54Z

biblib/views/operations_view.py

-
-        if data['action'] == 'union':
+
+            if action == "empty":


I really like this change to use permission_check_<>

Thanks. It was pretty readable in the end.

kelockhart added 2 commits December 5, 2023 10:55

Fixed bug w/ not checking permissions in copy

c4e22f6

Check read access for secondary libraries

585af53

kelockhart commented Dec 5, 2023

View reviewed changes

added access check for public libraries and fixed unittests

dbf1327

kelockhart commented Dec 5, 2023

View reviewed changes

femalves added 2 commits December 5, 2023 22:28

adding a not

8b75c7a

fixing tests

accbeeb

kelockhart commented Dec 6, 2023

View reviewed changes

femalves added 3 commits December 7, 2023 15:17

changed it to stop using merge

135f3e8

created variables and changed logic

f89a312

added tests and removed write access check

b0b855c

femalves requested a review from tjacovich December 12, 2023 00:18

tjacovich requested changes Dec 13, 2023

View reviewed changes

fixed bug

77e7d79

femalves requested a review from tjacovich December 14, 2023 21:04

tjacovich reviewed Dec 15, 2023

View reviewed changes

changing accesses hoping it's final

030e424

femalves requested a review from tjacovich December 21, 2023 19:13

kelockhart commented Dec 21, 2023

View reviewed changes

tjacovich approved these changes Dec 21, 2023

View reviewed changes

femalves merged commit 34ba5bd into adsabs:master Dec 21, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed bug w/ not checking permissions in copy #174

Fixed bug w/ not checking permissions in copy #174

kelockhart commented Dec 5, 2023

kelockhart Dec 5, 2023

femalves Dec 5, 2023

kelockhart Dec 5, 2023

kelockhart Dec 6, 2023

femalves Dec 6, 2023

tjacovich left a comment

tjacovich Dec 13, 2023

femalves Dec 14, 2023 •

edited

Loading

tjacovich Dec 13, 2023

femalves Dec 14, 2023

tjacovich left a comment

tjacovich Dec 15, 2023

femalves Dec 16, 2023

tjacovich Dec 18, 2023

femalves Dec 18, 2023

kelockhart left a comment

tjacovich left a comment

tjacovich Dec 21, 2023

femalves Dec 21, 2023

Fixed bug w/ not checking permissions in copy #174

Fixed bug w/ not checking permissions in copy #174

Conversation

kelockhart commented Dec 5, 2023

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

tjacovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

femalves Dec 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjacovich left a 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

kelockhart left a comment

Choose a reason for hiding this comment

tjacovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

femalves Dec 14, 2023 •

edited

Loading