Bug 29503 - GET /patrons should use Koha::Patrons->search_limited
Summary: GET /patrons should use Koha::Patrons->search_limited
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Keywords:
Depends on:
Blocks: 29506
  Show dependency treegraph
 
Reported: 2021-11-17 14:22 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:25 UTC (History)
11 users (show)

See Also:
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


Attachments
Bug 29503: Regression tests (3.12 KB, patch)
2021-11-17 16:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited (1.30 KB, patch)
2021-11-17 16:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29503: Regression tests (3.18 KB, patch)
2021-11-17 16:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited (1.37 KB, patch)
2021-11-17 16:28 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29503: Regression tests (3.24 KB, patch)
2021-12-17 14:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 29503: Make GET /patrons use Koha::Patrons->search_limited (1.42 KB, patch)
2021-12-17 14:17 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.