From 1b91d371c8abb9e37247c400bef79ad1e803dd60 Mon Sep 17 00:00:00 2001 From: Keith James Date: Fri, 2 Jun 2023 15:21:08 +0100 Subject: [PATCH 1/2] Fix incorrect recursive behaviour of add/remove/supersede_permissions For collections, ensure that full recursion is carried out when changing permisssions, rather than just inspected/updating the root collection path. Avoid modifiying the arguments passed to the permissions methods, which had the effect of mutating the object's ACL. These changes were made by overriding the relevent RodsItem methods specifically for Collections. The RodsItem methods now deal with the non-recursive cases (for both Collections and DataObjects). --- src/partisan/irods.py | 112 +++++++++-- tests/conftest.py | 18 ++ tests/data/recursive/level1/level2/leaf1.txt | 1 + tests/data/recursive/level1/level2/leaf2.txt | 1 + tests/test_irods.py | 196 +++++++++++++++++-- 5 files changed, 288 insertions(+), 40 deletions(-) create mode 100644 tests/data/recursive/level1/level2/leaf1.txt create mode 100644 tests/data/recursive/level1/level2/leaf2.txt diff --git a/src/partisan/irods.py b/src/partisan/irods.py index d457903..a5c0640 100644 --- a/src/partisan/irods.py +++ b/src/partisan/irods.py @@ -1481,14 +1481,13 @@ def supersede_metadata( return len(to_remove), len(to_add) @rods_type_check - def add_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int: + def add_permissions(self, *acs: AC, timeout=None, tries=1) -> int: """Add access controls to the item. Return the number of access controls added. If some argument access controls are already present, those arguments will be ignored. Args: *acs: Access controls. - recurse: Recursively add access control. timeout: Operation timeout in seconds. tries: Number of times to try the operation. @@ -1496,25 +1495,30 @@ def add_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int """ current = self.acl() to_add = sorted(set(acs).difference(current)) - log.debug("Adding to ACL", path=self, curr=current, arg=acs, add=to_add) + log.debug( + "Adding to ACL", + path=self, + curr=current, + arg=acs, + add=to_add, + ) if to_add: item = self._to_dict() item[Baton.ACCESS] = to_add with client(self.pool) as c: - c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries) + c.set_permission(item, timeout=timeout, tries=tries) return len(to_add) @rods_type_check - def remove_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int: + def remove_permissions(self, *acs: AC, timeout=None, tries=1) -> int: """Remove access controls from the item. Return the number of access controls removed. If some argument access controls are not present, those arguments will be ignored. Args: *acs: Access controls. - recurse: Recursively add access control. timeout: Operation timeout in seconds. tries: Number of times to try the operation. @@ -1522,31 +1526,33 @@ def remove_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> """ current = self.acl() to_remove = sorted(set(current).intersection(acs)) - log.debug("Removing from ACL", path=self, curr=current, arg=acs, rem=to_remove) + log.debug( + "Removing from ACL", + path=self, + curr=current, + arg=acs, + rem=to_remove, + ) if to_remove: # In iRODS we "remove" permissions by setting them to NULL - for ac in to_remove: - ac.perm = Permission.NULL + to_null = [AC(ac.user, Permission.NULL, zone=ac.zone) for ac in to_remove] item = self._to_dict() - item[Baton.ACCESS] = to_remove + item[Baton.ACCESS] = to_null with client(self.pool) as c: - c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries) + c.set_permission(item, timeout=timeout, tries=tries) return len(to_remove) @rods_type_check - def supersede_permissions( - self, *acs: AC, recurse=False, timeout=None, tries=1 - ) -> Tuple[int, int]: + def supersede_permissions(self, *acs: AC, timeout=None, tries=1) -> Tuple[int, int]: """Remove all access controls from the item, replacing them with the specified access controls. Return the numbers of access controls removed and added. Args: *acs: Access controls. - recurse: Recursively supersede access controls. timeout: Operation timeout in seconds. tries: Number of times to try the operation. @@ -1557,16 +1563,17 @@ def supersede_permissions( to_remove = sorted(set(current).difference(acs)) if to_remove: + # If recurse is true, we want to do this update, even if the target path + # has the required ACL because child paths may not have the ACL. log.debug("Removing from ACL", path=self, ac=to_remove) # In iRODS we "remove" permissions by setting them to NULL - for ac in to_remove: - ac.perm = Permission.NULL + to_null = [AC(ac.user, Permission.NULL, zone=ac.zone) for ac in to_remove] item = self._to_dict() - item[Baton.ACCESS] = to_remove + item[Baton.ACCESS] = to_null with client(self.pool) as c: - c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries) + c.set_permission(item, timeout=timeout, tries=tries) to_add = sorted(set(acs).difference(current)) if to_add: @@ -1574,7 +1581,7 @@ def supersede_permissions( item = self._to_dict() item[Baton.ACCESS] = to_add with client(self.pool) as c: - c.set_permission(item, recurse=recurse, timeout=timeout, tries=tries) + c.set_permission(item, timeout=timeout, tries=tries) return len(to_remove), len(to_add) @@ -2303,6 +2310,71 @@ def put(self, local_path: Union[Path, str], recurse=True, timeout=None, tries=1) """ raise NotImplementedError() + def add_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int: + """Add access controls to the collection. Return the number of access + controls added. If some argument access controls are already present, + those arguments will be ignored. + + Args: + *acs: Access controls. + recurse: Recursively add access controls. + timeout: Operation timeout in seconds. + tries: Number of times to try the operation. + + Returns: int + """ + num_added = super().add_permissions(*acs, timeout=timeout, tries=tries) + if recurse: + for item in self.iter_contents(recurse=recurse): + num_added += item.add_permissions(*acs, timeout=timeout, tries=tries) + return num_added + + def remove_permissions(self, *acs: AC, recurse=False, timeout=None, tries=1) -> int: + """Remove access controls from the collection. Return the number of access + controls removed. If some argument access controls are not present, those + arguments will be ignored. + + Args: + *acs: Access controls. + recurse: Recursively remove access controls. + timeout: Operation timeout in seconds. + tries: Number of times to try the operation. + + Returns: int + """ + num_removed = super().remove_permissions(*acs, timeout=timeout, tries=tries) + if recurse: + for item in self.iter_contents(recurse=recurse): + num_removed += item.remove_permissions( + *acs, timeout=timeout, tries=tries + ) + return num_removed + + def supersede_permissions( + self, *acs: AC, recurse=False, timeout=None, tries=1 + ) -> Tuple[int, int]: + """Remove all access controls from the collection, replacing them with the + specified access controls. Return the numbers of access controls + removed and added. + + Args: + *acs: Access controls. + recurse: Recursively supersede access controls. + timeout: Operation timeout in seconds. + tries: Number of times to try the operation. + + Returns: Tuple[int, int] + """ + num_removed, num_added = super().supersede_permissions( + *acs, timeout=timeout, tries=tries + ) + if recurse: + for item in self.iter_contents(recurse=recurse): + nr, na = item.supersede_permissions(*acs, timeout=timeout, tries=tries) + num_removed += nr + num_added += na + return num_removed, num_added + def _list(self, **kwargs) -> List[dict]: with client(self.pool) as c: return c.list({Baton.COLL: self.path}, **kwargs) diff --git a/tests/conftest.py b/tests/conftest.py index 8f38420..625a27f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -153,6 +153,24 @@ def annotated_collection(simple_collection): irm(simple_collection, force=True, recurse=True) +@pytest.fixture(scope="function") +def full_collection(tmp_path): + """A fixture providing a collection with some contents""" + root_path = PurePath("/testZone/home/irods/test") + rods_path = add_rods_path(root_path, tmp_path) + + iput("./tests/data/recursive/", rods_path, recurse=True) + coll_path = rods_path / "recursive" + + try: + add_test_groups() + + yield coll_path + finally: + remove_test_groups() + irm(root_path, force=True, recurse=True) + + @pytest.fixture(scope="function") def simple_data_object(tmp_path): """A fixture providing a collection containing a single data object containing diff --git a/tests/data/recursive/level1/level2/leaf1.txt b/tests/data/recursive/level1/level2/leaf1.txt new file mode 100644 index 0000000..c274d3e --- /dev/null +++ b/tests/data/recursive/level1/level2/leaf1.txt @@ -0,0 +1 @@ +e276ebe6-012e-11ee-b52e-c3459e3defec diff --git a/tests/data/recursive/level1/level2/leaf2.txt b/tests/data/recursive/level1/level2/leaf2.txt new file mode 100644 index 0000000..0dd1d67 --- /dev/null +++ b/tests/data/recursive/level1/level2/leaf2.txt @@ -0,0 +1 @@ +d1dc40b0-012e-11ee-8774-af1e5dcfdab1 diff --git a/tests/test_irods.py b/tests/test_irods.py index dd03d1c..1265f3e 100644 --- a/tests/test_irods.py +++ b/tests/test_irods.py @@ -473,6 +473,164 @@ def test_create_collection_existing(self, simple_collection): with pytest.raises(RodsError, match="create collection"): coll.create(parents=False, exist_ok=False) + @m.it("Can have access controls added, non-recursively") + def test_add_ac_collection(self, full_collection): + zone = "testZone" + coll = Collection(full_collection) + irods_own = AC("irods", Permission.OWN, zone=zone) + public_read = AC("public", Permission.READ, zone=zone) + + assert coll.acl() == [irods_own] + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own] + + assert ( + coll.add_permissions(irods_own) == 0 + ), "Nothing is added when new ACL == all old ACL" + assert coll.acl() == [irods_own] + + assert coll.add_permissions(public_read) == 1 + assert coll.acl() == [ + irods_own, + public_read, + ], "Access control added non-recursively" + + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own], "Collection content ACL unchanged" + + @m.it("Can have access controls added, recursively") + def test_add_ac_collection_recurse(self, full_collection): + zone = "testZone" + coll = Collection(full_collection) + irods_own = AC("irods", Permission.OWN, zone=zone) + public_read = AC("public", Permission.READ, zone=zone) + + assert coll.acl() == [irods_own] + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own] + + assert ( + coll.add_permissions(irods_own, recurse=True) == 0 + ), "Nothing is added when new ACL == all old ACL, recursively" + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own] + + tree = [ + "recurse", + "level1/", + "level1/level2/", + "level1/level2/leaf1.txt", + "level1/level2/leaf2.txt", + ] + assert coll.add_permissions(public_read, recurse=True) == len( + tree + ), "Access control added recursively" + for item in coll.contents(recurse=True): + assert item.acl() == [ + irods_own, + public_read, + ], "Collection content ACL updated" + + @m.it("Can have access controls removed, non-recursively") + def test_rem_ac_collection(self, full_collection): + zone = "testZone" + coll = Collection(full_collection) + irods_own = AC("irods", Permission.OWN, zone=zone) + public_read = AC("public", Permission.READ, zone=zone) + + assert coll.acl() == [irods_own] + assert ( + coll.remove_permissions(public_read) == 0 + ), "Nothing is removed when the access control does not exist" + + coll.add_permissions(public_read, recurse=True) + assert coll.acl() == [irods_own, public_read] + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own, public_read] + + assert coll.remove_permissions(public_read) == 1 + assert coll.acl() == [irods_own], "Access control removed non-recursively" + for item in coll.contents(recurse=True): + assert item.acl() == [ + irods_own, + public_read, + ], "Collection content ACL unchanged" + + @m.it("Can have access controls removed, recursively") + def test_rem_ac_collection_recurse(self, full_collection): + zone = "testZone" + coll = Collection(full_collection) + irods_own = AC("irods", Permission.OWN, zone=zone) + public_read = AC("public", Permission.READ, zone=zone) + + coll.add_permissions(public_read, recurse=True) + assert coll.acl() == [irods_own, public_read] + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own, public_read] + + tree = [ + "recurse", + "level1/", + "level1/level2/", + "level1/level2/leaf1.txt", + "level1/level2/leaf2.txt", + ] + assert coll.remove_permissions(public_read, recurse=True) == len( + tree + ), "Access control removed recursively" + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own], "Collection content ACL updated" + + @m.it("Can have access controls superseded, non-recursively") + def test_super_ac_collection(self, full_collection): + zone = "testZone" + coll = Collection(full_collection) + irods_own = AC("irods", Permission.OWN, zone=zone) + public_read = AC("public", Permission.READ, zone=zone) + study_01_read = AC("ss_study_01", Permission.READ, zone=zone) + study_02_read = AC("ss_study_02", Permission.READ, zone=zone) + + coll.add_permissions(study_01_read, study_02_read, recurse=True) + assert coll.acl() == [irods_own, study_01_read, study_02_read] + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own, study_01_read, study_02_read] + + num_removed, num_added = coll.supersede_permissions(irods_own, public_read) + assert num_removed == 2 # study access + assert num_added == 1 # public access + assert coll.acl() == [ + irods_own, + public_read, + ], "Access superseded removed non-recursively" + for item in coll.contents(recurse=True): + assert item.acl() == [ + irods_own, + study_01_read, + study_02_read, + ], "Collection content ACL unchanged" + + @m.it("Can have access controls superseded, recursively") + def test_super_ac_collection_recur(self, full_collection): + zone = "testZone" + coll = Collection(full_collection) + irods_own = AC("irods", Permission.OWN, zone=zone) + public_read = AC("public", Permission.READ, zone=zone) + study_01_read = AC("ss_study_01", Permission.READ, zone=zone) + study_02_read = AC("ss_study_02", Permission.READ, zone=zone) + + coll.add_permissions(study_01_read, study_02_read, recurse=True) + assert coll.acl() == [irods_own, study_01_read, study_02_read] + for item in coll.contents(recurse=True): + assert item.acl() == [irods_own, study_01_read, study_02_read] + + new_acl = [irods_own, public_read] + num_removed, num_added = coll.supersede_permissions(*new_acl, recurse=True) + assert num_removed == 2 * 5 # study access + assert num_added == 1 * 5 # public access + assert coll.acl() == new_acl, "Access superseded removed recursively" + for item in coll.contents(recurse=True): + assert item.acl() == new_acl, "Collection content ACL updated" + @m.describe("DataObject") class TestDataObject: @@ -700,8 +858,8 @@ def test_avu_collection(self, simple_data_object): ): obj.avu("abcde") - @m.it("Can have metadata replaced") - def test_repl_meta_data_object(self, simple_data_object): + @m.it("Can have metadata superseded") + def test_supersede_meta_data_object(self, simple_data_object): obj = DataObject(simple_data_object) assert obj.metadata() == [] @@ -755,39 +913,37 @@ def test_add_ac_data_object(self, simple_data_object): zone = "testZone" user = "irods" obj = DataObject(simple_data_object) - assert obj.acl() == [AC(user, Permission.OWN, zone=zone)] + irods_own = AC(user, Permission.OWN, zone=zone) + assert obj.acl() == [irods_own] assert ( - obj.add_permissions(AC(user, Permission.OWN, zone=zone)) == 0 + obj.add_permissions(irods_own) == 0 ), "nothing is replaced when new ACL == all old ACL" - assert obj.acl() == [AC(user, Permission.OWN, zone=zone)] + assert obj.acl() == [irods_own] - assert obj.add_permissions(AC("public", Permission.READ, zone=zone)) == 1 - assert obj.acl() == [ - AC(user, Permission.OWN, zone=zone), - AC("public", Permission.READ, zone=zone), - ] + public_read = AC("public", Permission.READ, zone=zone) + assert obj.add_permissions(public_read) == 1 + assert obj.acl() == [irods_own, public_read] @m.it("Can have access controls removed") def test_rem_ac_data_object(self, simple_data_object): zone = "testZone" user = "irods" obj = DataObject(simple_data_object) - assert obj.acl() == [AC(user, Permission.OWN, zone=zone)] + irods_own = AC(user, Permission.OWN, zone=zone) + assert obj.acl() == [irods_own] + public_read = AC("public", Permission.READ, zone=zone) assert ( - obj.remove_permissions(AC("public", Permission.READ, zone=zone)) == 0 + obj.remove_permissions(public_read) == 0 ), "nothing is removed when the access control does not exist" - assert obj.acl() == [AC(user, Permission.OWN, zone=zone)] + assert obj.acl() == [irods_own] - assert obj.add_permissions(AC("public", Permission.READ, zone=zone)) == 1 - assert obj.acl() == [ - AC(user, Permission.OWN, zone=zone), - AC("public", Permission.READ, zone=zone), - ] + assert obj.add_permissions(public_read) == 1 + assert obj.acl() == [irods_own, public_read] - assert obj.remove_permissions(AC("public", Permission.READ, zone=zone)) == 1 - assert obj.acl() == [AC(user, Permission.OWN, zone=zone)] + assert obj.remove_permissions(public_read) == 1 + assert obj.acl() == [irods_own] @m.describe("Replica management") From 87234ab7504c60f0fffbeb0ea87fbee15c33de5a Mon Sep 17 00:00:00 2001 From: Keith James Date: Tue, 6 Jun 2023 09:37:56 +0100 Subject: [PATCH 2/2] Improve docstrings for recursive operations on permissions --- src/partisan/irods.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/partisan/irods.py b/src/partisan/irods.py index a5c0640..3b3339e 100644 --- a/src/partisan/irods.py +++ b/src/partisan/irods.py @@ -1486,6 +1486,9 @@ def add_permissions(self, *acs: AC, timeout=None, tries=1) -> int: controls added. If some argument access controls are already present, those arguments will be ignored. + This method handles only the permissions of this RodsItem. See the Collection + class for methods handling recursive operations. + Args: *acs: Access controls. timeout: Operation timeout in seconds. @@ -1517,6 +1520,9 @@ def remove_permissions(self, *acs: AC, timeout=None, tries=1) -> int: controls removed. If some argument access controls are not present, those arguments will be ignored. + This method handles only the permissions of this RodsItem. See the Collection + class for methods handling recursive operations. + Args: *acs: Access controls. timeout: Operation timeout in seconds. @@ -1551,6 +1557,9 @@ def supersede_permissions(self, *acs: AC, timeout=None, tries=1) -> Tuple[int, i specified access controls. Return the numbers of access controls removed and added. + This method handles only the permissions of this RodsItem. See the Collection + class for methods handling recursive operations. + Args: *acs: Access controls. timeout: Operation timeout in seconds. @@ -1563,8 +1572,6 @@ def supersede_permissions(self, *acs: AC, timeout=None, tries=1) -> Tuple[int, i to_remove = sorted(set(current).difference(acs)) if to_remove: - # If recurse is true, we want to do this update, even if the target path - # has the required ACL because child paths may not have the ACL. log.debug("Removing from ACL", path=self, ac=to_remove) # In iRODS we "remove" permissions by setting them to NULL