Skip to content

Commit

Permalink
Fix handling of permission for multiple iRODS groups
Browse files Browse the repository at this point in the history
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".
  • Loading branch information
kjsanger committed Jan 3, 2024
1 parent 92d3d93 commit 420e122
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 9 deletions.
9 changes: 3 additions & 6 deletions src/npg_irods/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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:
Expand Down
6 changes: 3 additions & 3 deletions tests/test_illumina.py
Original file line number Diff line number Diff line change
Expand Up @@ -460,25 +460,25 @@ 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
):
zone = "testZone"
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")
Expand Down

0 comments on commit 420e122

Please sign in to comment.