From c9d987647a96227486412f7b00a20f72f82490de Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Tue, 23 Jul 2024 13:55:13 +0000 Subject: [PATCH] Bug 37392: [24.05.x] Edit item permission by library group is broken Signed-off-by: Michaela Sieber Signed-off-by: Brendan Lawlor Signed-off-by: Martin Renvoize Bug 37392: can_see_things_from is always checking patron visibility When trying to edit items, we are seeing the button visibility affected by a patron's permission to view patrons form any library. This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling libraries_where_can_see_patrons That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against To test: Set up library group: * Create a library group for library A + B * Action: Limit item editing by group Set up test user: * Create a staff patron with these permissions: * catalogue * fast_cataloguing * edit_items * view_borrower_infos_from_any_libraries * edit borrowers * Home library: library A Set up test items: * Create a record with 3 items with different home libraries: * A * B * C We expect the user will be allowed to edit A and B, but not C. Test: * Test editing the items with the test user, only A is allowed to be edited. * Remove the view_borrower_infos_from_any_libraries permission from test user. * Test editing items now behaves as expected: A + B are allowed, C is not. Signed-off-by: Michaela Sieber Signed-off-by: Brendan Lawlor Signed-off-by: Martin Renvoize Bug 37392: Adjust routines The current code only handled a single layer of groups - top level setting the features, and libraries directly underneath. The code, however, was not correctly checking the features, and was limiting to single like when no restrictions found. This patch gets the root ancestor for a group, checks the desired feature against than group, then fetches all children of the current group and makes them allowed - i.e. when a library is in a group, all siblings and descendants in that group or subgroups can be accessed I adjust some typos in the tests too, this needs more cleanup in the future, but am submitting for any discussion Signed-off-by: Michaela Sieber Signed-off-by: Brendan Lawlor Signed-off-by: Martin Renvoize Bug 37392: (follow-up) Tidy and improve descriptions Tidy and improve the description of patron limits in staff interface Signed-off-by: Martin Renvoize Bug 37392: (QA follow-up) Attempt to clarify POD This aptch attemptes to tidy up and clarify the POD for various 'things' methods in the Koha::Patron class. Signed-off-by: Martin Renvoize Bug 37392: (follow-up) Limit a borrower not in a group and fix tests The previous patches took into account all the groups for a patron, but missed the case where a patron didn't have permission to see outside their library, and their library is not in a group. Code updated and a test added. Other tests adjusted to ensure the feature to limit patrons was set in those groups. Bug 37392: Fix API helpers testsa This patch acknowledges the fact this patchset changed the called method `libraries_where_can_see_patrons` for `libraries_where_can_see_things`. And as such the mock was not working. Signed-off-by: Tomas Cohen Arazi Bug 37392: (QA tool fixes) --- Koha/Patron.pm | 74 ++++++++---- .../prog/en/modules/admin/library_groups.tt | 4 +- t/db_dependent/ArticleRequests.t | 4 +- t/db_dependent/Koha/Patrons.t | 112 ++++++++++++++---- t/db_dependent/Koha/REST/Plugin/Objects.t | 2 +- t/db_dependent/Koha/Reviews.t | 4 +- t/db_dependent/Patron/Borrower_Discharge.t | 4 +- 7 files changed, 153 insertions(+), 51 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 0483b88e16e..d06ad8dd928 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1819,9 +1819,10 @@ sub can_see_patrons_from { return $self->can_see_things_from( { - branchcode => $branchcode, - permission => 'borrowers', + branchcode => $branchcode, + permission => 'borrowers', subpermission => 'view_borrower_infos_from_any_libraries', + group_feature => 'ft_hide_patron_info', } ); } @@ -1849,6 +1850,7 @@ sub can_edit_items_from { branchcode => $branchcode, permission => 'editcatalogue', subpermission => 'edit_any_item', + group_feature => 'ft_limit_item_editing', } ); } @@ -1859,7 +1861,8 @@ sub can_edit_items_from { Return the list of branchcodes(!) of libraries the patron is allowed to items for. The branchcodes are arbitrarily returned sorted. -We are supposing here that the object is related to the logged in patron (use of C4::Context::only_my_library) +We are supposing here that the object is related to the logged in patron (use of +C4::Context::only_my_library) An empty array means no restriction, the user can edit any item. @@ -1879,13 +1882,18 @@ sub libraries_where_can_edit_items { =head3 libraries_where_can_see_patrons -my $libraries = $patron->libraries_where_can_see_patrons; + my $libraries = $patron->libraries_where_can_see_patrons; + +Return the list of branchcodes(!) of libraries the patron is allowed to see other +patron's infos. -Return the list of branchcodes(!) of libraries the patron is allowed to see other patron's infos. The branchcodes are arbitrarily returned sorted. -We are supposing here that the object is related to the logged in patron (use of C4::Context::only_my_library) -An empty array means no restriction, the patron can see patron's infos from any libraries. +We are supposing here that the object is related to the logged in patron (use of +C4::Context::only_my_library) + +An empty array means no restriction, the patron can see patron's infos from any +libraries. =cut @@ -1903,9 +1911,17 @@ sub libraries_where_can_see_patrons { =head3 can_see_things_from -my $can_see = $patron->can_see_things_from( $branchcode ); + my $can_see = $patron->can_see_things_from( + { + branchcode => $branchcode, + permission => $permission, + subpermission => $subpermission, + group_feature => $group_feature + } + ); -Return true if the I can perform some action on the given thing +Return true if the I can perform some action, as described by a +permission, subpermission, group_feature combination, at the passed library. =cut @@ -1923,8 +1939,11 @@ sub can_see_things_from { $can = 1; } elsif ( $self->has_permission( { $permission => $subpermission } ) ) { $can = 1; - } elsif ( my @branches = $self->libraries_where_can_see_patrons ) { + } elsif ( my @branches = $self->libraries_where_can_see_things($params) ) { $can = ( any { $_ eq $branchcode } @branches ) ? 1 : 0; + } else { + # This should be the case of not finding any limits above, so we can + $can = 1; } return $can; } @@ -1958,14 +1977,25 @@ sub can_log_into { =head3 libraries_where_can_see_things - my $libraries = $patron->libraries_where_can_see_things; + my $libraries = $patron->libraries_where_can_see_things( + { + permission => $permission, + subpermission => $subpermission, + group_feature => $group_feature + } + ); + +Returns a list of libraries where this user is allowed to perform an action, as +defined by a permission, subpermission, group_feature combination. -Returns a list of libraries where an aribitarary action is allowed to be taken by the logged in librarian -against an object based on some branchcode related to the object ( patron branchcode, item homebranch, etc ). +We account for `IndependentBranches` and permission/subpermission assignments +before looking into library group allowances. -We are supposing here that the object is related to the logged in librarian (use of C4::Context::only_my_library) +We are assuming here that the object is related to the logged in librarian (use +of C4::Context::only_my_library) -An empty array means no restriction, the thing can see thing's infos from any libraries. +An empty array means no restriction, the thing can see thing's infos from any +libraries. =cut @@ -1993,18 +2023,22 @@ sub libraries_where_can_see_things { ) ) { - my $library_groups = $self->library->library_groups({ $group_feature => 1 }); + my $library_groups = $self->library->library_groups(); if ( $library_groups->count ) { while ( my $library_group = $library_groups->next ) { + my $root = Koha::Library::Groups->get_root_ancestor({ id => $library_group->id }); + next unless $root->$group_feature; my $parent = $library_group->parent; - if ( $parent->has_child( $self->branchcode ) ) { - push @restricted_branchcodes, $parent->children->get_column('branchcode'); + my @children = $parent->all_libraries; + foreach my $child (@children){ + push @restricted_branchcodes, $child->branchcode; + } } + } else { + push @restricted_branchcodes, $self->branchcode; } - - @restricted_branchcodes = ( $self->branchcode ) unless @restricted_branchcodes; } } diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/library_groups.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/library_groups.tt index 85e12cfb722..82f516e6b9f 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/library_groups.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/library_groups.tt @@ -437,10 +437,10 @@ [% UNLESS group.branchcode %]
    [% IF group.ft_hide_patron_info %] -
  • Hide patron's info for librarians outside of this group.
  • +
  • Limit patron visibility to libraries within this group for members.
  • [% END %] [% IF group.ft_limit_item_editing %] -
  • Limit item editing to librarians inside of this group.
  • +
  • Limit item editing to items owned inside of this group.
  • [% END %] [% IF group.ft_search_groups_opac %]
  • Use for OPAC search groups
  • diff --git a/t/db_dependent/ArticleRequests.t b/t/db_dependent/ArticleRequests.t index f3d5b317c9a..289015b4314 100755 --- a/t/db_dependent/ArticleRequests.t +++ b/t/db_dependent/ArticleRequests.t @@ -174,8 +174,8 @@ subtest 'search_limited' => sub { plan tests => 2; my $nb_article_requests = Koha::ArticleRequests->count; - my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1' } )->store; - my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2' } )->store; + my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1', ft_hide_patron_info => 1 } )->store; + my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2', ft_hide_patron_info => 1 } )->store; Koha::Library::Group->new({ parent_id => $group_1->id, branchcode => $patron->branchcode })->store(); Koha::Library::Group->new({ parent_id => $group_2->id, branchcode => $patron_2->branchcode })->store(); t::lib::Mocks::mock_userenv( { patron => $patron } ); # Is superlibrarian diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 38a322e4444..7e4cfe586b9 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1284,9 +1284,10 @@ subtest 'search_patrons_to_anonymise' => sub { t::lib::Mocks::mock_preference('IndependentBranches', 0); }; -subtest 'libraries_where_can_see_patrons + libraries_where_can_see_things + can_see_patron_infos + search_limited' => - sub { - plan tests => 4; +subtest + 'libraries_where_can_see_patrons + libraries_where_can_see_things + can_see_patron_infos + search_limited+ can_see_patrons_from + can_edit_items_from' + => sub { + plan tests => 6; # group1 # + library_11 @@ -1299,30 +1300,34 @@ subtest 'libraries_where_can_see_patrons + libraries_where_can_see_things + can_ my $library_11 = $builder->build( { source => 'Branch' } ); my $library_12 = $builder->build( { source => 'Branch' } ); my $library_21 = $builder->build( { source => 'Branch' } ); + my $library_31 = $builder->build( { source => 'Branch' } ); $library_11 = Koha::Libraries->find( $library_11->{branchcode} ); $library_12 = Koha::Libraries->find( $library_12->{branchcode} ); $library_21 = Koha::Libraries->find( $library_21->{branchcode} ); + $library_31 = Koha::Libraries->find( $library_31->{branchcode} ); Koha::Library::Group->new( { branchcode => $library_11->branchcode, parent_id => $group_1->id } )->store; Koha::Library::Group->new( { branchcode => $library_12->branchcode, parent_id => $group_1->id } )->store; Koha::Library::Group->new( { branchcode => $library_21->branchcode, parent_id => $group_2->id } )->store; + # Library 31, not in any group my $sth = - C4::Context->dbh->prepare(q|INSERT INTO user_permissions( borrowernumber, module_bit, code ) VALUES (?, 4, ?)|) - ; # 4 for borrowers - # 2 patrons from library_11 (group1) - # patron_11_1 see patron's infos from outside its group - # Setting flags => undef to not be considered as superlibrarian + C4::Context->dbh->prepare(q|INSERT INTO user_permissions( borrowernumber, module_bit, code ) VALUES (?, ?, ?)|); + + # 2 patrons from library_11 (group1) + # patron_11_1 see patron's infos from outside its group + # Setting flags => undef to not be considered as superlibrarian my $patron_11_1 = $builder->build( { source => 'Borrower', value => { branchcode => $library_11->branchcode, flags => undef, } } ); $patron_11_1 = Koha::Patrons->find( $patron_11_1->{borrowernumber} ); - $sth->execute( $patron_11_1->borrowernumber, 'edit_borrowers' ); - $sth->execute( $patron_11_1->borrowernumber, 'view_borrower_infos_from_any_libraries' ); + $sth->execute( $patron_11_1->borrowernumber, 4, 'edit_borrowers' ); + $sth->execute( $patron_11_1->borrowernumber, 4, 'view_borrower_infos_from_any_libraries' ); # patron_11_2 can only see patron's info from its group my $patron_11_2 = $builder->build( { source => 'Borrower', value => { branchcode => $library_11->branchcode, flags => undef, } } ); $patron_11_2 = Koha::Patrons->find( $patron_11_2->{borrowernumber} ); - $sth->execute( $patron_11_2->borrowernumber, 'edit_borrowers' ); + $sth->execute( $patron_11_2->borrowernumber, 4, 'edit_borrowers' ); + $sth->execute( $patron_11_2->borrowernumber, 9, 'edit_items' ); # 1 patron from library_12 (group1) my $patron_12 = $builder->build( @@ -1333,31 +1338,86 @@ subtest 'libraries_where_can_see_patrons + libraries_where_can_see_things + can_ my $patron_21 = $builder->build( { source => 'Borrower', value => { branchcode => $library_21->branchcode, flags => undef, } } ); $patron_21 = Koha::Patrons->find( $patron_21->{borrowernumber} ); - $sth->execute( $patron_21->borrowernumber, 'edit_borrowers' ); + $sth->execute( $patron_21->borrowernumber, 4, 'edit_borrowers' ); + + # 1 patron from library_31 (no group) can only see patron's info from its library + my $patron_31 = $builder->build( + { source => 'Borrower', value => { branchcode => $library_31->branchcode, flags => undef, } } ); + $patron_31 = Koha::Patrons->find( $patron_31->{borrowernumber} ); + $sth->execute( $patron_31->borrowernumber, 4, 'edit_borrowers' ); # Pfiou, we can start now! subtest 'libraries_where_can_see_things' => sub { - plan tests => 2; - t::lib::Mocks::mock_userenv( { patron => $patron_11_2 } ); + plan tests => 4; + t::lib::Mocks::mock_userenv( { patron => $patron_11_1 } ); my $params = { - permission => 'editcatalogue', - subpermission => 'edit_any_item', - group_feature => 'ft_limit_item_editing', + permission => 'borrowers', + subpermission => 'view_borrower_infos_from_any_libraries', + group_feature => 'ft_hide_patron_info', }; - my @branchcodes = $patron_11_2->libraries_where_can_see_things($params); + my @branchcodes = $patron_11_1->libraries_where_can_see_things($params); is_deeply( - \@branchcodes, [ sort ( $library_11->branchcode, $library_12->branchcode ) ], + \@branchcodes, [], q|patron_11_1 has view_borrower_infos_from_any_libraries => No restriction| ); + @branchcodes = $patron_11_1->libraries_where_can_see_things($params); + is_deeply( + \@branchcodes, [], + q|confirming second/cached request is the same patron_11_1 has view_borrower_infos_from_any_libraries => No restriction| + ); + @branchcodes = $patron_11_2->libraries_where_can_see_things($params); is_deeply( \@branchcodes, [ sort ( $library_11->branchcode, $library_12->branchcode ) ], - q|patron_11_1 has view_borrower_infos_from_any_libraries => No restriction| + q|patron_11_2 can only view from group| + ); + @branchcodes = $patron_11_2->libraries_where_can_see_things($params); + is_deeply( + \@branchcodes, [ sort ( $library_11->branchcode, $library_12->branchcode ) ], + q|confirming second/cached request is the same patron_11_2 can only view from group| ); }; + subtest 'can_see_patrons_from' => sub { + plan tests => 2; + ok( $patron_11_2->can_see_patrons_from( $library_11->branchcode ), "We can see a patron from in our group" ); + ok( + !$patron_11_2->can_see_patrons_from( $library_21->branchcode ), + "We cannot see a patron from outside our group without permissions" + ); + }; + + subtest 'can_edit_items_from' => sub { + plan tests => 4; + ok( $patron_11_2->can_edit_items_from( $library_11->branchcode ), "We can edit an item from in our group" ); + ok( + $patron_11_2->can_edit_items_from( $library_21->branchcode ), + "We can edit an item from outside our group as the group does not limit item editing" + ); + $group_1->ft_limit_item_editing(1)->store(); + + $patron_11_2 = Koha::Patrons->find( $patron_11_2->borrowernumber ); + + #FIXME We refetch the patron because library lists are cached in an extra hash key + # in libraries_where_can_see_things + + ok( + !$patron_11_2->can_edit_items_from( $library_21->branchcode ), + "We can not edit an item from outside our group as the group does limit item editing" + ); + + $sth->execute( $patron_11_2->borrowernumber, 9, 'edit_any_item' ); + $patron_11_2 = Koha::Patrons->find( $patron_11_2->borrowernumber ); + + ok( + $patron_11_2->can_edit_items_from( $library_21->branchcode ), + "We can edit an item from outside our group as we have permission" + ); + + }; + subtest 'libraries_where_can_see_patrons' => sub { - plan tests => 3; + plan tests => 4; my @branchcodes; @@ -1378,6 +1438,13 @@ subtest 'libraries_where_can_see_patrons + libraries_where_can_see_things + can_ \@branchcodes, [ $library_21->branchcode ], q|patron_21 has not view_borrower_infos_from_any_libraries => Can only see patron's from its group| ); + + t::lib::Mocks::mock_userenv( { patron => $patron_31 } ); + @branchcodes = $patron_31->libraries_where_can_see_patrons; + is_deeply( + \@branchcodes, [ $library_31->branchcode ], + q|patron_31 has not view_borrower_infos_from_any_libraries => Can only see patron's from its library that is not in a group| + ); }; subtest 'can_see_patron_infos' => sub { plan tests => 6; @@ -1402,7 +1469,7 @@ subtest 'libraries_where_can_see_patrons + libraries_where_can_see_things + can_ t::lib::Mocks::mock_userenv( { patron => $patron_11_1 } ); $patron_11_1 = Koha::Patrons->find( $patron_11_1->borrowernumber ); - my $total_number_of_patrons = $nb_of_patrons + 4; #we added four in these tests + my $total_number_of_patrons = $nb_of_patrons + 5; #we added five in these tests is( Koha::Patrons->search->count, $total_number_of_patrons, 'Non-limited search should return all patrons' ); is( Koha::Patrons->search_limited->count, $total_number_of_patrons, @@ -1429,6 +1496,7 @@ subtest 'libraries_where_can_see_patrons + libraries_where_can_see_things + can_ $patron_11_2->delete; $patron_12->delete; $patron_21->delete; + $patron_31->delete; }; subtest 'account_locked' => sub { diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t index b0eebbd492c..afe605c94a5 100755 --- a/t/db_dependent/Koha/REST/Plugin/Objects.t +++ b/t/db_dependent/Koha/REST/Plugin/Objects.t @@ -649,7 +649,7 @@ subtest 'objects.search helper, search_limited() tests' => sub { my $t = Test::Mojo->new; my $mocked_patron = Test::MockModule->new('Koha::Patron'); - $mocked_patron->mock( 'libraries_where_can_see_patrons', sub + $mocked_patron->mock( 'libraries_where_can_see_things', sub { return @libraries_where_can_see_patrons; } diff --git a/t/db_dependent/Koha/Reviews.t b/t/db_dependent/Koha/Reviews.t index 7e947a1bb86..1b50f4885b2 100755 --- a/t/db_dependent/Koha/Reviews.t +++ b/t/db_dependent/Koha/Reviews.t @@ -71,8 +71,8 @@ is( $retrieved_review_1_1->review, $new_review_1_1->review, 'Find a review by id subtest 'search_limited' => sub { plan tests => 2; C4::Context->_new_userenv('xxx'); - my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1' } )->store; - my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2' } )->store; + my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1', ft_hide_patron_info => 1 } )->store; + my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2', ft_hide_patron_info => 1 } )->store; Koha::Library::Group->new({ parent_id => $group_1->id, branchcode => $patron_1->branchcode })->store(); Koha::Library::Group->new({ parent_id => $group_2->id, branchcode => $patron_2->branchcode })->store(); t::lib::Mocks::mock_userenv( { patron => $patron_1 } ); diff --git a/t/db_dependent/Patron/Borrower_Discharge.t b/t/db_dependent/Patron/Borrower_Discharge.t index 062a61076e0..56b7b795161 100755 --- a/t/db_dependent/Patron/Borrower_Discharge.t +++ b/t/db_dependent/Patron/Borrower_Discharge.t @@ -155,8 +155,8 @@ is( ref(Koha::Patron::Discharge::request({ borrowernumber => $patron->borrowernu subtest 'search_limited' => sub { plan tests => 4; $dbh->do(q|DELETE FROM discharges|); - my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1' } )->store; - my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2' } )->store; + my $group_1 = Koha::Library::Group->new( { title => 'TEST Group 1', ft_hide_patron_info => 1 } )->store; + my $group_2 = Koha::Library::Group->new( { title => 'TEST Group 2', ft_hide_patron_info => 1 } )->store; # $patron and $patron2 are from the same library, $patron3 from another one # Logged in user is $patron, superlibrarian t::lib::Mocks::mock_userenv({ patron => $patron }); -- 2.39.5