From 420e122ce3d81c09b555884457eddc64eab6746a Mon Sep 17 00:00:00 2001 From: Keith James Date: Wed, 3 Jan 2024 09:32:06 +0000 Subject: [PATCH] Fix handling of permission for multiple iRODS groups When multiple iRODS groups have permission to read a data object, they should all be permitted that access. The system should not default to revoking those permissions as "inconsistent". --- src/npg_irods/common.py | 9 +++------ tests/test_illumina.py | 6 +++--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/npg_irods/common.py b/src/npg_irods/common.py index 26b6a1fa..43564077 100644 --- a/src/npg_irods/common.py +++ b/src/npg_irods/common.py @@ -230,8 +230,8 @@ def update_permissions( item: Collection | DataObject, acl: list[AC], recurse=False ) -> bool: """Update permissions on an iRODS path, removing existing permissions and replacing - with the given ACL. If the ACL contains multiple, conflicting, managed permissions - then it will issue a warning and revoke access. + with the given ACL. If the ACL contains managed permissions for multiple groups + then it will grant access to all those groups. Args: item: iRODS path to update. @@ -252,10 +252,7 @@ def update_permissions( acl = sorted(set(acl)) # Ensure no duplicates, sort for reproducibility if has_mixed_ownership(acl): - log.warn("Mixed-study data", path=item, acl=acl) - for ac in acl: - if is_managed_access(ac): - ac.perm = Permission.NULL + log.info("Granting access for multiple groups", path=item, acl=acl) # Gather some of the current permissions that we want to keep, while we supersede # all the rest with our new ACL: diff --git a/tests/test_illumina.py b/tests/test_illumina.py index dd51b9b5..6e9d2a61 100644 --- a/tests/test_illumina.py +++ b/tests/test_illumina.py @@ -460,7 +460,7 @@ def test_updates_xahuman_permissions_mx( @m.context("When data are multiplexed") @m.context("When data are from multiple studies") - @m.it("Removes managed access permissions") + @m.it("Retains managed access permissions") def test_multiple_study_permissions_mx( self, illumina_synthetic_irods, illumina_synthetic_mlwh ): @@ -468,17 +468,17 @@ def test_multiple_study_permissions_mx( path = illumina_synthetic_irods / "12345/12345#2.cram" qc_path = illumina_synthetic_irods / "12345" / "qc" / "12345#2.genotype.json" old_permissions = [ + AC("irods", perm=Permission.OWN, zone=zone), AC("ss_4000", Permission.READ, zone=zone), AC("ss_5000", Permission.READ, zone=zone), ] - new_permissions = [AC("irods", perm=Permission.OWN, zone=zone)] for obj in [DataObject(path), DataObject(qc_path)]: obj.add_permissions(*old_permissions) assert ensure_secondary_metadata_updated( obj, mlwh_session=illumina_synthetic_mlwh ) - assert obj.permissions() == new_permissions + assert obj.permissions() == old_permissions @m.context("When data are not multiplexed") @m.context("When data have had consent withdrawn")