From dff4f0ecbec4a88e24a9b8a216e758cbcee9c5e8 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 25 Mar 2020 11:21:15 +0100 Subject: [PATCH] Bug 24964: Do not filter patrons after they have been fetched The svc/members/search script is called in different places. In some places (Set owner for a fund, add users to a fund, or set a manager to a suggestion), we need patrons to be filtered depending on the permissions they have. For instance you can only set a fund's owner with a patron that has acquisition.order_manage. Currently we have fetching X (default 20) patrons, then filter them depending on their permission. Says you have 3 patrons that have the correct permissions but are not in the 20 first patrons, if you do not define a search term, the search result will be empty. This is not ideal and we should filter when requesting the DB. Test plan: - Have more than 20 patrons, remove them their permissions - Create 3 more: 1 superlibrarian 1 with the full acq permission 1 with acquisition.order_manage - Create a fund and set a owner - Search for patrons, without specifying a search term (to get them all) => Without this patch the new patrons you created are not displayed => With this patch they are! Same test plan apply to set a manager to a suggestion (freshly pushed, see bug 23590), with suggestions and suggestions.suggestions_manage Note: The code has been written that way to rely on C4::Auth::haspermission, but the SQL query is quite trivial and the gain is important. Signed-off-by: Fridolin Somers Signed-off-by: Kyle M Hall --- C4/Utils/DataTables/Members.pm | 43 ++++++++++++++++++++++++++++------ svc/members/search | 19 +++++---------- 2 files changed, 42 insertions(+), 20 deletions(-) diff --git a/C4/Utils/DataTables/Members.pm b/C4/Utils/DataTables/Members.pm index b7887a858e..d2b4089dc4 100644 --- a/C4/Utils/DataTables/Members.pm +++ b/C4/Utils/DataTables/Members.pm @@ -13,6 +13,7 @@ sub search { my $branchcode = $params->{branchcode}; my $searchtype = $params->{searchtype} || 'contain'; my $searchfieldstype = $params->{searchfieldstype} || 'standard'; + my $has_permission = $params->{has_permission}; my $dt_params = $params->{dt_params}; unless ( $searchmember ) { @@ -27,12 +28,31 @@ sub search { my ($sth, $query, $iTotalQuery, $iTotalRecords, $iTotalDisplayRecords); my $dbh = C4::Context->dbh; + + # Get the module_bit from a given permission code + if ( $has_permission ) { + ($has_permission->{module_bit}) = $dbh->selectrow_array(q| + SELECT bit FROM userflags WHERE flag=? + |, undef, $has_permission->{permission}); + } + # Get the iTotalRecords DataTable variable - $query = $iTotalQuery = "SELECT COUNT(borrowers.borrowernumber) FROM borrowers"; + $iTotalQuery = "SELECT COUNT(borrowers.borrowernumber) FROM borrowers"; + if ( $has_permission ) { + $iTotalQuery .= ' LEFT JOIN user_permissions on borrowers.borrowernumber=user_permissions.borrowernumber'; + } + + my (@where, @conditions); if ( @restricted_branchcodes ) { - $iTotalQuery .= " WHERE borrowers.branchcode IN (" . join( ',', ('?') x @restricted_branchcodes ) . ")"; + push @where, "borrowers.branchcode IN (" . join( ',', ('?') x @restricted_branchcodes ) . ")"; + push @conditions, @restricted_branchcodes; } - ($iTotalRecords) = $dbh->selectrow_array( $iTotalQuery, undef, @restricted_branchcodes ); + if ( $has_permission ) { + push @where, '( borrowers.flags = 1 OR borrowers.flags & (1 << ?) OR module_bit=? AND code=? )'; + push @conditions, ($has_permission->{module_bit}) x 2, $has_permission->{subpermission}; + } + $iTotalQuery .= ' WHERE ' . join ' AND ', @where if @where; + ($iTotalRecords) = $dbh->selectrow_array( $iTotalQuery, undef, @conditions ); # Do that after iTotalQuery! if ( defined $branchcode and $branchcode ) { @@ -57,6 +77,7 @@ sub search { my $select = "SELECT borrowers.borrowernumber, borrowers.surname, borrowers.firstname, + borrowers.flags, borrowers.streetnumber, borrowers.streettype, borrowers.address, borrowers.address2, borrowers.city, borrowers.state, borrowers.zipcode, borrowers.country, cardnumber, borrowers.dateexpiry, @@ -65,8 +86,12 @@ sub search { categories.description AS category_description, categories.category_type, branches.branchname, borrowers.phone"; my $from = "FROM borrowers - LEFT JOIN branches ON borrowers.branchcode = branches.branchcode - LEFT JOIN categories ON borrowers.categorycode = categories.categorycode"; + LEFT JOIN branches ON borrowers.branchcode = branches.branchcode + LEFT JOIN categories ON borrowers.categorycode = categories.categorycode"; + if ( $has_permission ) { + $from .= ' + LEFT JOIN user_permissions on borrowers.borrowernumber=user_permissions.borrowernumber'; + } my @where_args; my @where_strs; if(defined $firstletter and $firstletter ne '') { @@ -142,8 +167,12 @@ sub search { if @where_strs_or; } - my $where; - $where = " WHERE " . join (" AND ", @where_strs) if @where_strs; + if ( $has_permission ) { + push @where_strs, '( borrowers.flags = 1 OR borrowers.flags & (1 << ?) OR module_bit=? AND code=? )'; + push @where_args, ($has_permission->{module_bit}) x 2, $has_permission->{subpermission}; + } + + my $where = " WHERE " . join (" AND ", @where_strs) if @where_strs; my $orderby = dt_build_orderby($dt_params); my $limit; diff --git a/svc/members/search b/svc/members/search index 2d739869ee..76c5749f22 100755 --- a/svc/members/search +++ b/svc/members/search @@ -72,6 +72,11 @@ if ( $searchmember } if $member; } +if ($has_permission) { + my ( $permission, $subpermission ) = split /\./, $has_permission; + $has_permission = {permission => $permission, subpermission => $subpermission}; +} + # Perform the patrons search $results = C4::Utils::DataTables::Members::search( { @@ -82,22 +87,10 @@ $results = C4::Utils::DataTables::Members::search( searchtype => $searchtype, searchfieldstype => $searchfieldstype, dt_params => \%dt_params, + ( $has_permission ? ( has_permission => $has_permission ) : () ), } ) unless $results; -# It is not recommanded to use the has_permission param if you use the pagination -# The filter is done AFTER requested the data -if ($has_permission) { - my ( $permission, $subpermission ) = split /\./, $has_permission; - my @patrons_with_permission; - for my $patron ( @{ $results->{patrons} } ) { - push @patrons_with_permission, $patron - if haspermission( $patron->{userid}, { $permission => $subpermission } ); - } - $results->{patrons} = \@patrons_with_permission; - $results->{iTotalDisplayRecords} = scalar( @patrons_with_permission ); -} - $template->param( sEcho => $sEcho, iTotalRecords => $results->{iTotalRecords}, -- 2.21.1 (Apple Git-122.3)