Bug 29503

Summary: GET /patrons should use Koha::Patrons->search_limited
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: REST APIAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: 1joynelson, andrewfh, fridolin.somers, jonathan.druart, katrin.fischer, kyle, lucas, m.de.rooy, martin.renvoize, nick, victor
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29523
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02,21.05.08
Bug Depends on:    
Bug Blocks: 29506    
Attachments: Bug 29503: Regression tests
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited
Bug 29503: Regression tests
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited
Bug 29503: Regression tests
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited

Description Tomás Cohen Arazi 2021-11-17 14:22:00 UTC
The 'search_limited' method filters the resultset on what the patron can actually see. If not used, some patron information is leaked.
Comment 1 Tomás Cohen Arazi 2021-11-17 16:20:20 UTC
Created attachment 127735 [details] [review]
Bug 29503: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-11-17 16:20:24 UTC
Created attachment 127736 [details] [review]
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited

This patch makes the controller method use Koha::Patrons->search_limited
so filters based on configuration and permissions apply when fetching
patrons.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: Boo, you get more patrons than you should
3. Apply the patch
4. Repeat 2
=> SUCCESS: Yay! Things are filtered as expected (i.e. using
Koha::Patron->libraries_where_can_see_patrons)
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2021-11-17 16:21:41 UTC
I made it like this, mainly because of backportability. But I will file a separate bug to update objects.search to apply ->search_filtered when appropriate (i.e. when $result_set->can('search_filtered')).
Comment 4 Martin Renvoize 2021-11-17 16:28:30 UTC
Created attachment 127737 [details] [review]
Bug 29503: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-11-17 16:28:34 UTC
Created attachment 127738 [details] [review]
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited

This patch makes the controller method use Koha::Patrons->search_limited
so filters based on configuration and permissions apply when fetching
patrons.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: Boo, you get more patrons than you should
3. Apply the patch
4. Repeat 2
=> SUCCESS: Yay! Things are filtered as expected (i.e. using
Koha::Patron->libraries_where_can_see_patrons)
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 6 Martin Renvoize 2021-11-17 16:29:02 UTC
Great work, works as described and contains unit tests.

Signing off.
Comment 7 Tomás Cohen Arazi 2021-11-17 19:51:51 UTC
I'm lowering the severity because, as bug 29509 highlights, we actually require all _borrowers_ permissions to use the route (iincluding view_borrower_infos_from_any_libraries).

But those permissions are too high for using the routes on the API, and this bug (and bug 29506) will prevent leakages when time comes.
Comment 8 Nick Clemens 2021-12-17 14:17:08 UTC
Created attachment 128666 [details] [review]
Bug 29503: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 9 Nick Clemens 2021-12-17 14:17:12 UTC
Created attachment 128667 [details] [review]
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited

This patch makes the controller method use Koha::Patrons->search_limited
so filters based on configuration and permissions apply when fetching
patrons.

To test:
1. Apply the regression tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/api/v1/patrons.t
=> FAIL: Boo, you get more patrons than you should
3. Apply the patch
4. Repeat 2
=> SUCCESS: Yay! Things are filtered as expected (i.e. using
Koha::Patron->libraries_where_can_see_patrons)
5. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 10 Tomás Cohen Arazi 2022-01-05 23:53:27 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> I'm lowering the severity because, as bug 29509 highlights, we actually
> require all _borrowers_ permissions to use the route (iincluding
> view_borrower_infos_from_any_libraries).
> 
> But those permissions are too high for using the routes on the API, and this
> bug (and bug 29506) will prevent leakages when time comes.

I wrote this and forgot to change the 'Product'. Sorry for that
Comment 11 Fridolin Somers 2022-01-06 00:03:05 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 12 Kyle M Hall 2022-01-07 11:34:15 UTC
Pushed to 21.11.x for 21.11.02
Comment 13 Andrew Fuerste-Henry 2022-01-11 14:29:46 UTC
Pushed to 21.05.x for 21.05.08
Comment 14 Victor Grousset/tuxayo 2022-02-07 18:52:43 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.