Bug 37378 - Patron searches can fail when library groups are set to 'Limit patron data access by group'
Summary: Patron searches can fail when library groups are set to 'Limit patron data ac...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Nick Clemens (kidclamp)
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on: 29523
Blocks: 37392
  Show dependency treegraph
 
Reported: 2024-07-16 18:08 UTC by Lucas Gass (lukeg)
Modified: 2024-09-18 06:34 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00,24.05.04,23.11.09
Circulation function:


Attachments
Bug 37378: Unit test (2.11 KB, patch)
2024-07-23 12:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37378: libraries_where_can_see_things should always return an array (2.47 KB, patch)
2024-07-23 12:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37378: Tidy (11.29 KB, patch)
2024-07-23 12:27 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37378: Unit test (2.16 KB, patch)
2024-07-23 15:23 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37378: libraries_where_can_see_things should always return an array (2.52 KB, patch)
2024-07-23 15:23 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37378: Tidy (11.34 KB, patch)
2024-07-23 15:23 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37378: Unit test (2.23 KB, patch)
2024-07-26 13:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37378: libraries_where_can_see_things should always return an array (2.59 KB, patch)
2024-07-26 13:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 37378: Tidy (11.40 KB, patch)
2024-07-26 13:32 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass (lukeg) 2024-07-16 18:08:46 UTC
To recreate:

1. Setup a library group and check the "Limit patron data access by group ." option. 
2. Add some libraries to the group. ( IN k-t-d I added CPL and MPL )
3. Create a staff account who has staff access permissions and all of the borrower permissions except "view_borrower_infos_from_any_libraries"
4. Set the home library of that staff member to one of the branches in step 2. ( In my test I choose MPL )
5. Log in as that patron and attempt a patron search that would include users from either library in step 2. 
6. See the error:

Something went wrong when loading the table.
500: Internal Server Error. 
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.
Expected string - got null.
Expected string - got null.
Expected integer - got null.
Expected integer - got null.
Expected integer - got null.
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.
Comment 1 AspenCat Team 2024-07-18 16:29:27 UTC
We can't operate in our consortial setting the way we need to until this fixed.  We need the ability to not view patrons from other branches. - Bob Bennhoff
Comment 2 Katrin Fischer 2024-07-18 16:44:07 UTC
Bug 37368 has a note that makes me think it might be related - even a fix maybe?

This patch adds the missing library embed from the patron search forms.
This embed is required for the case where a patron is missing both first and last name or is from a different library group and thus falls into the 'Patron from library X' block.
Comment 3 Nick Clemens (kidclamp) 2024-07-23 12:27:42 UTC
Created attachment 169411 [details] [review]
Bug 37378: Unit test
Comment 4 Nick Clemens (kidclamp) 2024-07-23 12:27:47 UTC
Created attachment 169412 [details] [review]
Bug 37378: libraries_where_can_see_things should always return an array

The subroutine libraries_where_can_see_things stores the list of libraries that things
can be viewed from in an internal variable, so we can return this directly if we have already calculated.

When returning if not cached, we dereference the list and return an array. If cached, we are returning
an arrayref. This patch simply ensures we dereference the array even if already cached.

Before this patch, we were fetching the patrons, then redacting all info as their branches didn't match against
an arrayref, rather than checking against each branch we are allowed to view.

To test:
1. Setup a library group and check the "Limit patron data access by group ." option.
2. Add some libraries to the group. ( IN k-t-d I added CPL and MPL )
3. Create a staff account who has staff access permissions and all of the borrower permissions except "view_borrower_infos_from_any_libraries"
4. Set the home library of that staff member to one of the branches in step 2. ( In my test I choose MPL )
5. Log in as that patron and attempt a patron search that would include users from either library in step 2.
6. See the error:

Something went wrong when loading the table.
500: Internal Server Error.
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.
Expected string - got null.
Expected string - got null.
Expected integer - got null.
Expected integer - got null.
Expected integer - got null.
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.

7. Apply patch, restart all
8. Search again, you can see the expected patrons
Comment 5 Nick Clemens (kidclamp) 2024-07-23 12:27:52 UTC
Created attachment 169413 [details] [review]
Bug 37378: Tidy
Comment 6 Nick Clemens (kidclamp) 2024-07-23 12:29:40 UTC
(In reply to Katrin Fischer from comment #2)
> Bug 37368 has a note that makes me think it might be related - even a fix
> maybe?
> 
> This patch adds the missing library embed from the patron search forms.
> This embed is required for the case where a patron is missing both first and
> last name or is from a different library group and thus falls into the
> 'Patron from library X' block.

Nope, this is unrelated. We limit the search by libraries that can be seen, then redact any patrons we can't see - that redaction breaks the api response as it blanks fields that should not be null

We don't need to redact in this case, because we already limited to visible libraries, we just didn't get the list correctly
Comment 7 Brendan Lawlor 2024-07-23 15:23:50 UTC
Created attachment 169430 [details] [review]
Bug 37378: Unit test

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 8 Brendan Lawlor 2024-07-23 15:23:52 UTC
Created attachment 169431 [details] [review]
Bug 37378: libraries_where_can_see_things should always return an array

The subroutine libraries_where_can_see_things stores the list of libraries that things
can be viewed from in an internal variable, so we can return this directly if we have already calculated.

When returning if not cached, we dereference the list and return an array. If cached, we are returning
an arrayref. This patch simply ensures we dereference the array even if already cached.

Before this patch, we were fetching the patrons, then redacting all info as their branches didn't match against
an arrayref, rather than checking against each branch we are allowed to view.

To test:
1. Setup a library group and check the "Limit patron data access by group ." option.
2. Add some libraries to the group. ( IN k-t-d I added CPL and MPL )
3. Create a staff account who has staff access permissions and all of the borrower permissions except "view_borrower_infos_from_any_libraries"
4. Set the home library of that staff member to one of the branches in step 2. ( In my test I choose MPL )
5. Log in as that patron and attempt a patron search that would include users from either library in step 2.
6. See the error:

Something went wrong when loading the table.
500: Internal Server Error.
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.
Expected string - got null.
Expected string - got null.
Expected integer - got null.
Expected integer - got null.
Expected integer - got null.
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.

7. Apply patch, restart all
8. Search again, you can see the expected patrons

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 9 Brendan Lawlor 2024-07-23 15:23:55 UTC
Created attachment 169432 [details] [review]
Bug 37378: Tidy

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 10 Martin Renvoize (ashimema) 2024-07-26 13:32:52 UTC
Created attachment 169711 [details] [review]
Bug 37378: Unit test

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize (ashimema) 2024-07-26 13:32:55 UTC
Created attachment 169713 [details] [review]
Bug 37378: libraries_where_can_see_things should always return an array

The subroutine libraries_where_can_see_things stores the list of libraries that things
can be viewed from in an internal variable, so we can return this directly if we have already calculated.

When returning if not cached, we dereference the list and return an array. If cached, we are returning
an arrayref. This patch simply ensures we dereference the array even if already cached.

Before this patch, we were fetching the patrons, then redacting all info as their branches didn't match against
an arrayref, rather than checking against each branch we are allowed to view.

To test:
1. Setup a library group and check the "Limit patron data access by group ." option.
2. Add some libraries to the group. ( IN k-t-d I added CPL and MPL )
3. Create a staff account who has staff access permissions and all of the borrower permissions except "view_borrower_infos_from_any_libraries"
4. Set the home library of that staff member to one of the branches in step 2. ( In my test I choose MPL )
5. Log in as that patron and attempt a patron search that would include users from either library in step 2.
6. See the error:

Something went wrong when loading the table.
500: Internal Server Error.
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.
Expected string - got null.
Expected string - got null.
Expected integer - got null.
Expected integer - got null.
Expected integer - got null.
Expected boolean - got null.
Expected boolean - got null.
Expected string - got null.

7. Apply patch, restart all
8. Search again, you can see the expected patrons

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize (ashimema) 2024-07-26 13:32:58 UTC
Created attachment 169714 [details] [review]
Bug 37378: Tidy

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize (ashimema) 2024-07-26 13:33:12 UTC
Thanks Nick.. Passing QA
Comment 14 Martin Renvoize (ashimema) 2024-07-26 13:51:13 UTC
Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant
Comment 15 Lucas Gass (lukeg) 2024-08-29 15:10:00 UTC
Backported to 24.05.x for upcoming 24.05.04
Comment 16 Fridolin Somers 2024-09-10 14:56:23 UTC
Pushed to 23.11.x for 23.11.09
Comment 17 Wainui Witika-Park 2024-09-18 06:34:24 UTC
Not backporting to 23.05.x unless requested