From dc082b2ce01863a55460f9544efe99a92768d384 Mon Sep 17 00:00:00 2001 From: David K Jackson Date: Mon, 4 Dec 2023 12:15:48 +0000 Subject: [PATCH 01/11] expose ensure_group_exists method from iRODS::GroupAdmin --- lib/WTSI/NPG/iRODS/GroupAdmin.pm | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/lib/WTSI/NPG/iRODS/GroupAdmin.pm b/lib/WTSI/NPG/iRODS/GroupAdmin.pm index e5522d45..640fa282 100644 --- a/lib/WTSI/NPG/iRODS/GroupAdmin.pm +++ b/lib/WTSI/NPG/iRODS/GroupAdmin.pm @@ -191,15 +191,22 @@ sub _op_g_u { return; } -sub _ensure_existence_of_group { +=head2 ensure_group_exists + +Given a group ensure that it exists in iRODS by making it if it does not, and adding this admin user to it to ensure admin rights on it are retained. Return true if a group is created. + +=cut + +sub ensure_group_exists { my($self,$group)=@_; $self->__croak_on_bad_group_name($group); if ( any {$group eq $_} $self->lg){ return;} if ($self->dry_run) { - $self->info("Dry run: mkgroup '$group'"); + $self->info("Dry run: mkgroup '$group' and then atg..."); } else { $self->_push_pump_trim_split(qq(mkgroup "$group"\n)); + $self->_op_g_u('atg',$group, $self->_user); #add this user to empty group (first) so admin rights to operate on it are retained } return 1; #return true if we make a group } @@ -212,7 +219,7 @@ Given a group and list of members will ensure that the group exists and contains sub set_group_membership { my($self,$group,@members)=@_; - my $altered = $self->_ensure_existence_of_group($group); + my $altered = $self->ensure_group_exists($group); my @orig_members = $self->lg($group); $self->debug("Members of $group: ", join q(, ), @orig_members); if (@orig_members){ From 6a37999653865cd512f50c376b12014e2b612940 Mon Sep 17 00:00:00 2001 From: David K Jackson Date: Mon, 4 Dec 2023 14:19:51 +0000 Subject: [PATCH 02/11] tests for ensure_group_exists method in iRODS::GroupAdmin --- t/lib/WTSI/NPG/iRODS/GroupAdminTest.pm | 41 +++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/t/lib/WTSI/NPG/iRODS/GroupAdminTest.pm b/t/lib/WTSI/NPG/iRODS/GroupAdminTest.pm index 6f2b61a8..e560c4d0 100644 --- a/t/lib/WTSI/NPG/iRODS/GroupAdminTest.pm +++ b/t/lib/WTSI/NPG/iRODS/GroupAdminTest.pm @@ -44,7 +44,7 @@ sub setup_test : Test(setup) { sub teardown_test : Test(teardown) { my ($self) = @_; - $self->remove_irods_groups($test_irods, @irods_groups); + $self->remove_irods_groups($test_irods, @irods_groups, q(ss_newempty)); if ($self->have_admin_rights) { foreach my $user (@irods_users) { @@ -93,6 +93,45 @@ sub lg : Test(5) { 'Empty string group throw'; } +sub ensure_group_exists: Test(6) { + my ($self) = @_; + + my $irods = WTSI::NPG::iRODS->new(environment => \%ENV, + strict_baton_version => 0); + SKIP: { + if (not $self->have_admin_rights) { + skip 'No admin rights to create test groups', 5; + } + + my $zone = $irods->find_zone_name($irods->working_collection); + my $user = $irods->get_irods_user; + + my $iga = WTSI::NPG::iRODS::GroupAdmin->new; + + throws_ok { $iga->lg('ss_newempty') } qr/does not exist/sm, + 'Non-existent group throw for ss_newempty'; + + my$created=0; + lives_ok { + $created=$iga->ensure_group_exists('ss_newempty'); + } 'Create new group ss_newempty'; + ok($created, "Reports that group has been created"); + + my @observed_members = sort $iga->lg('ss_newempty'); + my @expected_members = ("$user#$zone"); + is_deeply(\@observed_members, \@expected_members, + 'Has expected admin user automatically added') or + diag explain \@observed_members; + + $created=1; + lives_ok { + $created=$iga->ensure_group_exists('ss_newempty'); + } 'Safe to run on existing group'; + + ok(!$created, "Reports that group has not been created"); + } +} + sub set_group_membership : Test(5) { my ($self) = @_; From 7c357b1be79e1340337cfd72711daa08705cb727 Mon Sep 17 00:00:00 2001 From: David K Jackson Date: Tue, 5 Dec 2023 09:53:59 +0000 Subject: [PATCH 03/11] populate_wtsi_irods_groups creates _human iRODS groups --- bin/populate_wtsi_irods_groups.pl | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/bin/populate_wtsi_irods_groups.pl b/bin/populate_wtsi_irods_groups.pl index ca1c0d32..7158558d 100755 --- a/bin/populate_wtsi_irods_groups.pl +++ b/bin/populate_wtsi_irods_groups.pl @@ -44,6 +44,11 @@ sequencing tracked in the ML warehouse, the iRODS group will be left empty (except for the iRODS groupadmin user). +Studies which are marked as have samples contaminated with human which +should be removed will have an ss__human iRODS group created +when they do not exist - population of this group is performed outside +this process and should be tracked in an auditable manner by a ticket. + Script runs to perform such updates when no arguments are given. Options: @@ -72,7 +77,7 @@ exit 0; }, 'logconf=s' => \$log4perl_config, - 'study=i' => \@study_ids, + 'study_id=i' => \@study_ids, 'verbose' => \$verbose) or die "\n$what_on_earth\n"; if ($log4perl_config) { @@ -141,7 +146,7 @@ sub _uid_to_irods_uid { my $studies = $mlwh->resultset('Study')->search($query, {order_by => 'id_study_lims'}); -my ($group_count, $altered_count) = (0, 0); +my ($group_count, $altered_count, $altered_human_count) = (0, 0, 0); while (my $study = $studies->next){ my $study_id = $study->id_study_lims; my $dag_str = $study->data_access_group || q(); @@ -172,14 +177,19 @@ sub _uid_to_irods_uid { $altered_count++; } + $altered_human_count += $iga->ensure_group_exists("ss_$study_id".'_human') if $study->contaminated_human_dna; + $group_count++; } $log->debug("Altered $altered_count groups"); +$log->debug("Created $altered_human_count _human groups"); $log->info("When considering $group_count Sequencescape studies, ", - "$altered_count iRODS groups were created or their ", - 'membership altered (by ', $iga->_user, ')'); + $altered_count.' iRODS "ss_*" groups were created or their ', + 'membership altered, and '.$altered_human_count, + ' "ss_?????_human" groups were created (by ', + $iga->_user, ')'); # Find both gid and member uids for each group sub find_group_ids { From 11a7a829ae7f0f322f86c2c14699e15af3113f1d Mon Sep 17 00:00:00 2001 From: Marina Gourtovaia Date: Mon, 4 Dec 2023 18:13:07 +0000 Subject: [PATCH 04/11] Added to fixtures an extra study-specific iRODS group. --- t/lib/WTSI/NPG/DriRODSTest.pm | 3 ++- t/lib/WTSI/NPG/iRODS/CollectionTest.pm | 3 ++- t/lib/WTSI/NPG/iRODS/DataObjectTest.pm | 3 ++- t/lib/WTSI/NPG/iRODSTest.pm | 3 ++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/t/lib/WTSI/NPG/DriRODSTest.pm b/t/lib/WTSI/NPG/DriRODSTest.pm index ee780f93..6312ef8b 100644 --- a/t/lib/WTSI/NPG/DriRODSTest.pm +++ b/t/lib/WTSI/NPG/DriRODSTest.pm @@ -31,7 +31,8 @@ my $have_admin_rights = # Prefix for test iRODS data access groups my $group_prefix = 'ss_'; # Groups to be added to the test iRODS -my @irods_groups = map { $group_prefix . $_ } (0); +my @irods_groups = map { $group_prefix . $_, $group_prefix . $_ . '_human' } + (0); # Groups added to the test iRODS in fixture setup my @groups_added; # Enable group tests diff --git a/t/lib/WTSI/NPG/iRODS/CollectionTest.pm b/t/lib/WTSI/NPG/iRODS/CollectionTest.pm index e11cb05b..e5eb1cb0 100644 --- a/t/lib/WTSI/NPG/iRODS/CollectionTest.pm +++ b/t/lib/WTSI/NPG/iRODS/CollectionTest.pm @@ -27,7 +27,8 @@ my $have_admin_rights = # Prefix for test iRODS data access groups my $group_prefix = 'ss_'; # Groups to be added to the test iRODS -my @irods_groups = map { $group_prefix . $_ } (10, 100); +my @irods_groups = map { $group_prefix . $_, $group_prefix . $_ . '_human' } + (10, 100); # Groups added to the test iRODS in fixture setup my @groups_added; diff --git a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm index fcc8eac1..4a044ab9 100644 --- a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm +++ b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm @@ -34,7 +34,8 @@ my $have_admin_rights = # Prefix for test iRODS data access groups my $group_prefix = 'ss_'; # Groups to be added to the test iRODS -my @irods_groups = map { $group_prefix . $_ } (10, 100); +my @irods_groups = map { $group_prefix . $_, $group_prefix . $_ . '_human' } + (10, 100); # Groups added to the test iRODS in fixture setup my @groups_added; diff --git a/t/lib/WTSI/NPG/iRODSTest.pm b/t/lib/WTSI/NPG/iRODSTest.pm index d638f258..dee53935 100644 --- a/t/lib/WTSI/NPG/iRODSTest.pm +++ b/t/lib/WTSI/NPG/iRODSTest.pm @@ -33,7 +33,8 @@ my $irods_tmp_coll; # Prefix for test iRODS data access groups my $group_prefix = 'ss_'; # Groups to be added to the test iRODS -my @irods_groups = map { $group_prefix . $_ } (0, 10, 100); +my @irods_groups = map { $group_prefix . $_, $group_prefix . $_ . '_human' } + (0, 10, 100); # Groups added to the test iRODS in fixture setup my @groups_added; # Enable group tests From cd95d4f92ee5064ba71b37bc60ed7b66ea66c3bb Mon Sep 17 00:00:00 2001 From: Marina Gourtovaia Date: Mon, 4 Dec 2023 18:31:01 +0000 Subject: [PATCH 05/11] Dropped superfluous dependency from Build.PL --- Build.PL | 1 - 1 file changed, 1 deletion(-) diff --git a/Build.PL b/Build.PL index db90e0ec..71d49a0f 100644 --- a/Build.PL +++ b/Build.PL @@ -52,7 +52,6 @@ my $build = WTSI::DNAP::Utilities::Build->new }, recommends => { 'Net::LDAP' => '0', - 'WTSI::DNAP::Warehouse::Schema' => '0', }); $build->create_build_script; From 617cba141bc79594bf040c4ab4ade7e049745cd7 Mon Sep 17 00:00:00 2001 From: David K Jackson Date: Wed, 6 Dec 2023 13:51:46 +0000 Subject: [PATCH 06/11] populate_wtsi_irods_groups - remove postifx if for perlcritic --- bin/populate_wtsi_irods_groups.pl | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/bin/populate_wtsi_irods_groups.pl b/bin/populate_wtsi_irods_groups.pl index 7158558d..f76648fa 100755 --- a/bin/populate_wtsi_irods_groups.pl +++ b/bin/populate_wtsi_irods_groups.pl @@ -177,7 +177,9 @@ sub _uid_to_irods_uid { $altered_count++; } - $altered_human_count += $iga->ensure_group_exists("ss_$study_id".'_human') if $study->contaminated_human_dna; + if ($study->contaminated_human_dna) { + $altered_human_count += $iga->ensure_group_exists("ss_$study_id".'_human'); + } $group_count++; } From 977b7bd4f51e18eecee4ad065f5a15ee53770a0b Mon Sep 17 00:00:00 2001 From: Marina Gourtovaia Date: Tue, 12 Dec 2023 09:25:47 +0000 Subject: [PATCH 07/11] Fixed incorrect example in POD --- lib/WTSI/NPG/iRODS/DataObject.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/WTSI/NPG/iRODS/DataObject.pm b/lib/WTSI/NPG/iRODS/DataObject.pm index b01fea80..3e6c2231 100644 --- a/lib/WTSI/NPG/iRODS/DataObject.pm +++ b/lib/WTSI/NPG/iRODS/DataObject.pm @@ -426,7 +426,7 @@ sub set_permissions { $WTSI::NPG::iRODS::OWN_PERMISSION or $WTSI::NPG::iRODS::NULL_PERMISSION. Optional. - Example : $obj->get_object_groups($WTSI::NPG::iRODS::READ_PERMISSION) + Example : $obj->get_groups($WTSI::NPG::iRODS::READ_PERMISSION) Description: Return a list of the data access groups in the object's ACL. If a permission level argument is supplied, only groups with that level of access will be returned. Only groups having a From d99ebaade23b36134dd158761e864323ce0eb3b5 Mon Sep 17 00:00:00 2001 From: Marina Gourtovaia Date: Thu, 14 Dec 2023 12:38:25 +0000 Subject: [PATCH 08/11] Provided access to human split-out data. Previously no end-user access was provided to unconcented human split-out data. For each study a new iRODS group will be created. This pull request enables access to the nonconcented human data via this new group. --- lib/WTSI/NPG/iRODS/Path.pm | 18 ++- t/lib/WTSI/NPG/iRODS/DataObjectTest.pm | 197 ++++++++++++++++++++++++- 2 files changed, 212 insertions(+), 3 deletions(-) diff --git a/lib/WTSI/NPG/iRODS/Path.pm b/lib/WTSI/NPG/iRODS/Path.pm index f374aa8a..9b20e34d 100644 --- a/lib/WTSI/NPG/iRODS/Path.pm +++ b/lib/WTSI/NPG/iRODS/Path.pm @@ -6,7 +6,7 @@ use List::MoreUtils qw(any notall uniq); use Moose::Role; use WTSI::NPG::iRODS; -use WTSI::NPG::iRODS::Metadata qw($STUDY_ID); +use WTSI::NPG::iRODS::Metadata qw($STUDY_ID $ALIGNMENT_FILTER); our $VERSION = ''; @@ -294,6 +294,22 @@ sub expected_groups { push @groups, $group; } + # For nonconsented split-out human data add a special group. + # Give preference to the 'alignment_filter' metadata attribute. + # If this attrbute is not defined, examine the path of the object. + if (@groups == 1) { # Do not give access to nc human data to multiple groups. + my $give_human_subset_access = 0; + my @af_avus = $self->find_in_metadata($ALIGNMENT_FILTER); + if (not @af_avus and $self->str =~ /_human[.]/xms) { + $give_human_subset_access = 1; + } elsif (@af_avus and any { $_->{value} eq 'human' } @af_avus) { + $give_human_subset_access = 1; + } + if ($give_human_subset_access) { + push @groups, $groups[0] . '_human'; + } + } + return @groups; } diff --git a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm index 4a044ab9..c49ead46 100644 --- a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm +++ b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm @@ -15,7 +15,7 @@ use Test::Exception; Log::Log4perl::init('./etc/log4perl_tests.conf'); use WTSI::NPG::iRODS::DataObject; -use WTSI::NPG::iRODS::Metadata qw($STUDY_ID); +use WTSI::NPG::iRODS::Metadata qw($STUDY_ID $ALIGNMENT_FILTER); my $fixture_counter = 0; my $data_path = './t/data/path'; @@ -676,7 +676,7 @@ sub update_group_permissions : Test(12) { ok($r2, 'Removed ss_0 read access'); # Add a study 0 AVU and use it to update (add) permissions - # in the presence of anAVU that will infer a non-existent group + # in the presence of an AVU that will infer a non-existent group ok($obj->add_avu($STUDY_ID, '0')); ok($obj->add_avu($STUDY_ID, 'no_such_study')); ok($obj->update_group_permissions); @@ -695,4 +695,197 @@ sub update_group_permissions : Test(12) { } } +sub update_group_permissions_for_nc_human_data : Test(49) { + + my $irods = WTSI::NPG::iRODS->new(environment => \%ENV, + strict_baton_version => 0); + my ($fh, $empty_file) = tempfile(UNLINK => 1); + + ##### + # Object with a name that should not trigger _human group assignment + my $obj_path = "$irods_tmp_coll/test_file_human_no.txt"; + $irods->add_object($empty_file, $obj_path) or fail; + my $obj = WTSI::NPG::iRODS::DataObject->new($irods, $obj_path); + ok($obj->add_avu($STUDY_ID, '10')); + ok($obj->update_group_permissions); + my @permissions = $obj->get_permissions; + my $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_10 read access'); + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_10_human' } + @permissions; + ok($outcome, 'ss_10_human access is not added'); + + ##### + # Object with a name that should not trigger _human group assignment + $obj_path = "$irods_tmp_coll/test_file_nohuman.txt"; + $irods->add_object($empty_file, $obj_path) or fail; + $obj = WTSI::NPG::iRODS::DataObject->new($irods, $obj_path); + + # No alignment filter metadata + ok($obj->add_avu($STUDY_ID, '10')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_10 read access'); + $outcome = none { + exists $_->{owner} && $_->{owner} eq 'ss_10_human' + } @permissions; + ok($outcome, 'ss_10_human access is not added'); + + # phix alignment filter metadata + ok($obj->add_avu($ALIGNMENT_FILTER, 'phix')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Retained ss_10 read access'); + $outcome = none { + exists $_->{owner} && $_->{owner} eq 'ss_10_human' + } @permissions; + ok($outcome, 'ss_10_human access is not retained'); + + # yhuman alignment filter metadata + ok($obj->remove_avu($ALIGNMENT_FILTER, 'phix')); + ok($obj->add_avu($ALIGNMENT_FILTER, 'yhuman')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Retained ss_10 read access'); + $outcome = none { + exists $_->{owner} && $_->{owner} eq 'ss_10_human' + } @permissions; + ok($outcome, 'ss_10_human access is not added'); + + # human alignment filter metadata + ok($obj->remove_avu($ALIGNMENT_FILTER, 'yhuman')); + ok($obj->add_avu($ALIGNMENT_FILTER, 'human')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Retained ss_10 read access'); + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10_human' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_10_human read access'); + + # Multiple alignment filter metadata + ok($obj->add_avu($ALIGNMENT_FILTER, 'phix')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Retained ss_10 read access'); + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10_human' + } @permissions; + ok($outcome, 'ss_10_human access is retained'); + + # Metadata for two studies + ok($obj->add_avu($STUDY_ID, '100')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Retained ss_10 read access'); + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_100' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_100 read access'); + $outcome = none { + exists $_->{owner} && $_->{owner} eq 'ss_10_human' + } @permissions; + ok($outcome, 'ss_10_human access is not retained'); + $outcome = none { + exists $_->{owner} && $_->{owner} eq 'ss_100_human' + } @permissions; + ok($outcome, 'ss_100_human access is not added'); + + ##### + # Object with a name that should trigger _human group assignment + $obj_path = "$irods_tmp_coll/test_file_human.txt"; + $obj = WTSI::NPG::iRODS::DataObject->new($irods, $obj_path); + $irods->add_object($empty_file, $obj_path) or fail; + + # No study metadata + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = none { + exists $_->{owner} && $_->{owner} =~ /_human\Z/xms + } @permissions; + ok($outcome, 'None of _human access groups is added'); + + # Single study metadata + ok($obj->add_avu($STUDY_ID, '100')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_100' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_100 read access'); + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_100_human' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_100_human read access'); + + # Single study metadata and conflicting alignment filter + ok($obj->add_avu($ALIGNMENT_FILTER, 'phix')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = none { + exists $_->{owner} && $_->{owner} eq 'ss_100_human' + } @permissions; + ok($outcome, 'ss_100_human access is not present'); + + # Drop conflicting alignment filter metadata + ok($obj->remove_avu($ALIGNMENT_FILTER, 'phix')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_100_human' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_100_human read access'); + + # Metadata for two studies + ok($obj->add_avu($STUDY_ID, '10')); + ok($obj->update_group_permissions); + @permissions = $obj->get_permissions; + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_100' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Retained ss_100 read access'); + $outcome = any { + exists $_->{owner} && $_->{owner} eq 'ss_10' && + exists $_->{level} && $_->{level} eq 'read' + } @permissions; + ok($outcome, 'Added ss_10 read access'); + $outcome = none { + exists $_->{owner} && $_->{owner} =~ /_human\Z/xms + } @permissions; + ok($outcome, 'None of _human access groups is added'); +} + 1; From f7111c7377409c8f9b6481fac37cc7fac85583ba Mon Sep 17 00:00:00 2001 From: Marina Gourtovaia Date: Thu, 14 Dec 2023 17:52:00 +0000 Subject: [PATCH 09/11] Consolidated all logic about study-related ACLs. Consolidated all logic about study-related ACLs in the expected_groups method of WTSI::NPG::iRODS::Path thus making this part of logic independent of the sequencing platform. In future this logic can be applied, if necessary, to iRODS collections. Elements of this logic are moved from WTSI::NPG::iRODS::DataObject. The logic related to xahuman is currently inplemented in npg_irods GitHub package. --- lib/WTSI/NPG/iRODS/DataObject.pm | 21 +------- lib/WTSI/NPG/iRODS/Path.pm | 67 +++++++++++++++++--------- t/lib/WTSI/NPG/iRODS/DataObjectTest.pm | 49 ++++++------------- 3 files changed, 61 insertions(+), 76 deletions(-) diff --git a/lib/WTSI/NPG/iRODS/DataObject.pm b/lib/WTSI/NPG/iRODS/DataObject.pm index b01fea80..0efc638f 100644 --- a/lib/WTSI/NPG/iRODS/DataObject.pm +++ b/lib/WTSI/NPG/iRODS/DataObject.pm @@ -9,8 +9,6 @@ use Set::Scalar; use Try::Tiny; use WTSI::NPG::iRODS; -use WTSI::NPG::iRODS::Metadata qw($SAMPLE_CONSENT - $SAMPLE_CONSENT_WITHDRAWN); use WTSI::NPG::iRODS::Replicate; use WTSI::NPG::iRODS::Types qw(ArrayRefOfReplicate); @@ -452,9 +450,7 @@ sub get_groups { logged only. Example : $obj->update_group_permissions - Description: Modify a data objects ACL with respect to its study_id and - sample_consent / consent_withdrawn metadata and return the - data object. + Description: Modify a data objects ACL. The target group membership is determined by the result of calling $self->expected_groups. The current group membership @@ -462,9 +458,6 @@ sub get_groups { group memberships are pruned, then missing group memberships are added. - If there are sample_consent or consent_withdrawn metadata, - access for all groups is removed. - This method does not add or remove access for the 'public' group. Returntype : WTSI::NPG::iRODS::DataObject @@ -488,16 +481,6 @@ sub update_group_permissions { $self->debug('Updated annotations: [', join(', ', @groups_annotated), ']'); my $path = $self->str; - - my $true = 1; - my $false = 0; - if ($self->get_avu($SAMPLE_CONSENT, $false) or - $self->get_avu($SAMPLE_CONSENT_WITHDRAWN, $true)) { - $self->info('Data is marked as CONSENT WITHDRAWN; ', - 'all permissions will be withdrawn'); - @groups_annotated = (); # Emptying this means all will be removed - } - my $perms = Set::Scalar->new(@groups_permissions); my $annot = Set::Scalar->new(@groups_annotated); my @to_remove = $perms->difference($annot)->members; @@ -638,7 +621,7 @@ Keith James =head1 COPYRIGHT AND DISCLAIMER -Copyright (C) 2013, 2014, 2015, 2016, 2021 Genome Research Limited. All +Copyright (C) 2013, 2014, 2015, 2016, 2021, 2023 Genome Research Limited. All Rights Reserved. This program is free software: you can redistribute it and/or modify diff --git a/lib/WTSI/NPG/iRODS/Path.pm b/lib/WTSI/NPG/iRODS/Path.pm index 9b20e34d..08f97703 100644 --- a/lib/WTSI/NPG/iRODS/Path.pm +++ b/lib/WTSI/NPG/iRODS/Path.pm @@ -6,7 +6,10 @@ use List::MoreUtils qw(any notall uniq); use Moose::Role; use WTSI::NPG::iRODS; -use WTSI::NPG::iRODS::Metadata qw($STUDY_ID $ALIGNMENT_FILTER); +use WTSI::NPG::iRODS::Metadata qw($STUDY_ID + $ALIGNMENT_FILTER + $SAMPLE_CONSENT + $SAMPLE_CONSENT_WITHDRAWN); our $VERSION = ''; @@ -276,38 +279,56 @@ sub supersede_multivalue_avus { Arg [1] : None Example : @groups = $path->expected_groups - Description: Return an array of iRODS group names given metadata containing - >=1 study_id. - Returntype : Array + Description: Return a list of iRODS group names given metadata containing + >=1 study_id. The list might be empty. + + An empty list is returned if the concent has been withdrawn or + for split-out xa-human data, for which the consent does not + exist by definition, or for split-out human data that is + associated with multiple studies. + + Special study-related 'human' group ss__human is + returned for split-out human data associated with a single study. + + In all other cases if data is associated with a list of studies, + a list of groups is returned, a group per study, the group name + pattern being ss_. + + Returntype : List =cut sub expected_groups { my ($self) = @_; + my @af_avus = $self->find_in_metadata($ALIGNMENT_FILTER); my @ss_study_avus = $self->find_in_metadata($STUDY_ID); + my $human_subset = (any { $_->{value} eq 'human' } @af_avus) || + ($self->str =~ /_human[.]/xms); + my $info; + my $true = 1; + my $false = 0; my @groups; - foreach my $avu (@ss_study_avus) { - my $study_id = $avu->{value}; - my $group = $self->irods->make_group_name($study_id); - push @groups, $group; + if ($self->get_avu($SAMPLE_CONSENT, $false) || + $self->get_avu($SAMPLE_CONSENT_WITHDRAWN, $true)) { + $info = 'Data is marked as CONSENT WITHDRAWN'; + } elsif (any { $_->{value} eq 'xahuman' } @af_avus) { + $info = 'Data belongs to xahuman subset'; + } elsif ($human_subset && (@ss_study_avus > 1)) { + $info = 'Data belongs to human subset and multiple studies'; + } else { + @groups = map { $self->irods->make_group_name($_) } + map { $_->{value} } + @ss_study_avus; + if (@groups == 1 and $human_subset) { + $self->info('Data belongs to human subset'); + @groups = ($groups[0] . '_human'); # Reset the list + } } - # For nonconsented split-out human data add a special group. - # Give preference to the 'alignment_filter' metadata attribute. - # If this attrbute is not defined, examine the path of the object. - if (@groups == 1) { # Do not give access to nc human data to multiple groups. - my $give_human_subset_access = 0; - my @af_avus = $self->find_in_metadata($ALIGNMENT_FILTER); - if (not @af_avus and $self->str =~ /_human[.]/xms) { - $give_human_subset_access = 1; - } elsif (@af_avus and any { $_->{value} eq 'human' } @af_avus) { - $give_human_subset_access = 1; - } - if ($give_human_subset_access) { - push @groups, $groups[0] . '_human'; - } + if ($info) { + $self->info("${info}:\n no study-associated iRODS groups are applicable"); } return @groups; @@ -464,7 +485,7 @@ Keith James =head1 COPYRIGHT AND DISCLAIMER -Copyright (C) 2013, 2014, 2015, 2016 Genome Research Limited. All +Copyright (C) 2013, 2014, 2015, 2016, 2023 Genome Research Limited. All Rights Reserved. This program is free software: you can redistribute it and/or modify diff --git a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm index c49ead46..0a0a32bd 100644 --- a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm +++ b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm @@ -695,7 +695,7 @@ sub update_group_permissions : Test(12) { } } -sub update_group_permissions_for_nc_human_data : Test(49) { +sub update_group_permissions_for_nc_human_data : Test(43) { my $irods = WTSI::NPG::iRODS->new(environment => \%ENV, strict_baton_version => 0); @@ -772,11 +772,11 @@ sub update_group_permissions_for_nc_human_data : Test(49) { ok($obj->add_avu($ALIGNMENT_FILTER, 'human')); ok($obj->update_group_permissions); @permissions = $obj->get_permissions; - $outcome = any { + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_10' && exists $_->{level} && $_->{level} eq 'read' } @permissions; - ok($outcome, 'Retained ss_10 read access'); + ok($outcome, 'Lost ss_10 read access'); $outcome = any { exists $_->{owner} && $_->{owner} eq 'ss_10_human' && exists $_->{level} && $_->{level} eq 'read' @@ -787,11 +787,11 @@ sub update_group_permissions_for_nc_human_data : Test(49) { ok($obj->add_avu($ALIGNMENT_FILTER, 'phix')); ok($obj->update_group_permissions); @permissions = $obj->get_permissions; - $outcome = any { + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_10' && exists $_->{level} && $_->{level} eq 'read' } @permissions; - ok($outcome, 'Retained ss_10 read access'); + ok($outcome, 'Lost ss_10 read access'); $outcome = any { exists $_->{owner} && $_->{owner} eq 'ss_10_human' } @permissions; @@ -801,16 +801,16 @@ sub update_group_permissions_for_nc_human_data : Test(49) { ok($obj->add_avu($STUDY_ID, '100')); ok($obj->update_group_permissions); @permissions = $obj->get_permissions; - $outcome = any { + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_10' && exists $_->{level} && $_->{level} eq 'read' } @permissions; - ok($outcome, 'Retained ss_10 read access'); - $outcome = any { + ok($outcome, 'No ss_10 read access'); + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_100' && exists $_->{level} && $_->{level} eq 'read' } @permissions; - ok($outcome, 'Added ss_100 read access'); + ok($outcome, 'No ss_100 read access'); $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_10_human' } @permissions; @@ -838,30 +838,11 @@ sub update_group_permissions_for_nc_human_data : Test(49) { ok($obj->add_avu($STUDY_ID, '100')); ok($obj->update_group_permissions); @permissions = $obj->get_permissions; - $outcome = any { + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_100' && exists $_->{level} && $_->{level} eq 'read' } @permissions; - ok($outcome, 'Added ss_100 read access'); - $outcome = any { - exists $_->{owner} && $_->{owner} eq 'ss_100_human' && - exists $_->{level} && $_->{level} eq 'read' - } @permissions; - ok($outcome, 'Added ss_100_human read access'); - - # Single study metadata and conflicting alignment filter - ok($obj->add_avu($ALIGNMENT_FILTER, 'phix')); - ok($obj->update_group_permissions); - @permissions = $obj->get_permissions; - $outcome = none { - exists $_->{owner} && $_->{owner} eq 'ss_100_human' - } @permissions; - ok($outcome, 'ss_100_human access is not present'); - - # Drop conflicting alignment filter metadata - ok($obj->remove_avu($ALIGNMENT_FILTER, 'phix')); - ok($obj->update_group_permissions); - @permissions = $obj->get_permissions; + ok($outcome, 'ss_100 read access is not added'); $outcome = any { exists $_->{owner} && $_->{owner} eq 'ss_100_human' && exists $_->{level} && $_->{level} eq 'read' @@ -872,16 +853,16 @@ sub update_group_permissions_for_nc_human_data : Test(49) { ok($obj->add_avu($STUDY_ID, '10')); ok($obj->update_group_permissions); @permissions = $obj->get_permissions; - $outcome = any { + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_100' && exists $_->{level} && $_->{level} eq 'read' } @permissions; - ok($outcome, 'Retained ss_100 read access'); - $outcome = any { + ok($outcome, 'ss_100 read access is not added'); + $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_10' && exists $_->{level} && $_->{level} eq 'read' } @permissions; - ok($outcome, 'Added ss_10 read access'); + ok($outcome, 'ss_10 read access is not added'); $outcome = none { exists $_->{owner} && $_->{owner} =~ /_human\Z/xms } @permissions; From ddaf70a15dc66cffba607e797952c2750e2723ff Mon Sep 17 00:00:00 2001 From: Marina Gourtovaia Date: Wed, 10 Jan 2024 13:41:39 +0000 Subject: [PATCH 10/11] Base assignment of iRODS groups on metadata only. Disregard file names when assessing what study-related iRODS groups should be assigned to the entity. --- lib/WTSI/NPG/iRODS/Path.pm | 19 ++++--- t/lib/WTSI/NPG/iRODS/DataObjectTest.pm | 74 ++------------------------ 2 files changed, 15 insertions(+), 78 deletions(-) diff --git a/lib/WTSI/NPG/iRODS/Path.pm b/lib/WTSI/NPG/iRODS/Path.pm index 08f97703..f52ec638 100644 --- a/lib/WTSI/NPG/iRODS/Path.pm +++ b/lib/WTSI/NPG/iRODS/Path.pm @@ -279,9 +279,9 @@ sub supersede_multivalue_avus { Arg [1] : None Example : @groups = $path->expected_groups - Description: Return a list of iRODS group names given metadata containing - >=1 study_id. The list might be empty. - + Description: Return a list of iRODS group names. The list might be empty or + contain either one or multiple iRODS group names. + An empty list is returned if the concent has been withdrawn or for split-out xa-human data, for which the consent does not exist by definition, or for split-out human data that is @@ -293,6 +293,11 @@ sub supersede_multivalue_avus { In all other cases if data is associated with a list of studies, a list of groups is returned, a group per study, the group name pattern being ss_. + + The logic is based on examining such iRODS metadata as + 'study_id', 'alignment_filter', 'sample_consent', + 'sample_consent_withdrawn'. Neither object's path nor file name + is considered. Returntype : List @@ -303,8 +308,8 @@ sub expected_groups { my @af_avus = $self->find_in_metadata($ALIGNMENT_FILTER); my @ss_study_avus = $self->find_in_metadata($STUDY_ID); - my $human_subset = (any { $_->{value} eq 'human' } @af_avus) || - ($self->str =~ /_human[.]/xms); + my $human_subset = any { $_->{value} eq 'human' } @af_avus; + my $xahuman_subset = any { $_->{value} eq 'xahuman' } @af_avus; my $info; my $true = 1; @@ -313,7 +318,7 @@ sub expected_groups { if ($self->get_avu($SAMPLE_CONSENT, $false) || $self->get_avu($SAMPLE_CONSENT_WITHDRAWN, $true)) { $info = 'Data is marked as CONSENT WITHDRAWN'; - } elsif (any { $_->{value} eq 'xahuman' } @af_avus) { + } elsif ($xahuman_subset) { $info = 'Data belongs to xahuman subset'; } elsif ($human_subset && (@ss_study_avus > 1)) { $info = 'Data belongs to human subset and multiple studies'; @@ -485,7 +490,7 @@ Keith James =head1 COPYRIGHT AND DISCLAIMER -Copyright (C) 2013, 2014, 2015, 2016, 2023 Genome Research Limited. All +Copyright (C) 2013, 2014, 2015, 2016, 2023, 2024 Genome Research Limited. All Rights Reserved. This program is free software: you can redistribute it and/or modify diff --git a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm index 0a0a32bd..b5612e7e 100644 --- a/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm +++ b/t/lib/WTSI/NPG/iRODS/DataObjectTest.pm @@ -695,14 +695,14 @@ sub update_group_permissions : Test(12) { } } -sub update_group_permissions_for_nc_human_data : Test(43) { +sub update_group_permissions_for_nc_human_data : Test(28) { my $irods = WTSI::NPG::iRODS->new(environment => \%ENV, strict_baton_version => 0); my ($fh, $empty_file) = tempfile(UNLINK => 1); ##### - # Object with a name that should not trigger _human group assignment + # Object that should not trigger _human group assignment my $obj_path = "$irods_tmp_coll/test_file_human_no.txt"; $irods->add_object($empty_file, $obj_path) or fail; my $obj = WTSI::NPG::iRODS::DataObject->new($irods, $obj_path); @@ -718,26 +718,6 @@ sub update_group_permissions_for_nc_human_data : Test(43) { @permissions; ok($outcome, 'ss_10_human access is not added'); - ##### - # Object with a name that should not trigger _human group assignment - $obj_path = "$irods_tmp_coll/test_file_nohuman.txt"; - $irods->add_object($empty_file, $obj_path) or fail; - $obj = WTSI::NPG::iRODS::DataObject->new($irods, $obj_path); - - # No alignment filter metadata - ok($obj->add_avu($STUDY_ID, '10')); - ok($obj->update_group_permissions); - @permissions = $obj->get_permissions; - $outcome = any { - exists $_->{owner} && $_->{owner} eq 'ss_10' && - exists $_->{level} && $_->{level} eq 'read' - } @permissions; - ok($outcome, 'Added ss_10 read access'); - $outcome = none { - exists $_->{owner} && $_->{owner} eq 'ss_10_human' - } @permissions; - ok($outcome, 'ss_10_human access is not added'); - # phix alignment filter metadata ok($obj->add_avu($ALIGNMENT_FILTER, 'phix')); ok($obj->update_group_permissions); @@ -818,55 +798,7 @@ sub update_group_permissions_for_nc_human_data : Test(43) { $outcome = none { exists $_->{owner} && $_->{owner} eq 'ss_100_human' } @permissions; - ok($outcome, 'ss_100_human access is not added'); - - ##### - # Object with a name that should trigger _human group assignment - $obj_path = "$irods_tmp_coll/test_file_human.txt"; - $obj = WTSI::NPG::iRODS::DataObject->new($irods, $obj_path); - $irods->add_object($empty_file, $obj_path) or fail; - - # No study metadata - ok($obj->update_group_permissions); - @permissions = $obj->get_permissions; - $outcome = none { - exists $_->{owner} && $_->{owner} =~ /_human\Z/xms - } @permissions; - ok($outcome, 'None of _human access groups is added'); - - # Single study metadata - ok($obj->add_avu($STUDY_ID, '100')); - ok($obj->update_group_permissions); - @permissions = $obj->get_permissions; - $outcome = none { - exists $_->{owner} && $_->{owner} eq 'ss_100' && - exists $_->{level} && $_->{level} eq 'read' - } @permissions; - ok($outcome, 'ss_100 read access is not added'); - $outcome = any { - exists $_->{owner} && $_->{owner} eq 'ss_100_human' && - exists $_->{level} && $_->{level} eq 'read' - } @permissions; - ok($outcome, 'Added ss_100_human read access'); - - # Metadata for two studies - ok($obj->add_avu($STUDY_ID, '10')); - ok($obj->update_group_permissions); - @permissions = $obj->get_permissions; - $outcome = none { - exists $_->{owner} && $_->{owner} eq 'ss_100' && - exists $_->{level} && $_->{level} eq 'read' - } @permissions; - ok($outcome, 'ss_100 read access is not added'); - $outcome = none { - exists $_->{owner} && $_->{owner} eq 'ss_10' && - exists $_->{level} && $_->{level} eq 'read' - } @permissions; - ok($outcome, 'ss_10 read access is not added'); - $outcome = none { - exists $_->{owner} && $_->{owner} =~ /_human\Z/xms - } @permissions; - ok($outcome, 'None of _human access groups is added'); + ok($outcome, 'ss_100_human access is not added'); } 1; From ec9fa3282bee658771507644b278aea45f777c6b Mon Sep 17 00:00:00 2001 From: jmtcsngr Date: Thu, 25 Jan 2024 18:36:21 +0000 Subject: [PATCH 11/11] prep release 3.22.0 --- Changes | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Changes b/Changes index ad1711f3..968ed846 100644 --- a/Changes +++ b/Changes @@ -1,3 +1,17 @@ +Release 3.22.0 + + - Introduced ss__human iRODS group to provide limited + access to split-out human data. Specific changes: + - exposed ensure_group_exists method from iRODS::GroupAdmin + - populate_wtsi_irods_groups creates _human iRODS groups + - tests fixtures extended to include these new iRODS groups + - reimplemented expected_groups from WTSI::NPG::irods::Path + to return all groups that are appropriate for an object or + a collection; this method consolidates all logic about + study-related access to data, this logic was previously split + between WTSI::NPG::irods::Path and WTSI::NPG::irods::DataObject + - Fixed incorrect example in POD + Release 3.21.0 - Add iRODS 4.3.1 Ubuntu 22.04 as a required test target