The 'search_limited' method filters the resultset on what the patron can actually see. If not used, some patron information is leaked.
Created attachment 127735 [details] [review] Bug 29503: Regression tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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')).
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>
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>
Great work, works as described and contains unit tests. Signing off.
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.
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>
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>
(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
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Pushed to 21.11.x for 21.11.02
Pushed to 21.05.x for 21.05.08
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.