Bug 29541 - Patron images can be accessed with just 'catalogue' permission
Summary: Patron images can be accessed with just 'catalogue' permission
Status: Pushed to oldoldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Jonathan Druart
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 17423
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-20 19:37 UTC by Joonas Kylmälä
Modified: 2022-02-08 00:35 UTC (History)
15 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.05.00,21.11.02,21.05.09,20.11.14,19.11.25


Attachments
Bug 29541: Restrict access to patron's image to borrowers => * and circulate => * (1010 bytes, patch)
2021-11-22 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29541: Prevent users from another group to access patron's images (1.78 KB, patch)
2021-12-06 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29541: Restrict access to patron's image to borrowers => * and circulate => * (1.04 KB, patch)
2021-12-20 21:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29541: Prevent users from another group to access patron's images (1.84 KB, patch)
2021-12-20 21:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 29541: Restrict access to patron's image to borrowers => * and circulate => * (1.09 KB, patch)
2022-01-12 12:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29541: Prevent users from another group to access patron's images (1.89 KB, patch)
2022-01-12 12:08 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joonas Kylmälä 2021-11-20 19:37:12 UTC
If you login to staff interface with a user that has just 'catalogue' permission and nothing else you can view all the patrons' images, this should not be possible you should need to have the 'edit_borrowers' permission to do that.

To reproduce:

1. Enable "patronimages" syspref 
2. Upload first a photo to patron with borrower number XXX
3. With the user with just 'catalogue' permission go to http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=XXX
Comment 1 Jonathan Druart 2021-11-22 14:01:23 UTC
I am getting "Error: You do not have permission to access this page."

members/moremember.pl requires { borrowers => 'edit_borrowers' }
Comment 2 Jonathan Druart 2021-11-22 14:02:50 UTC
And I am getting a 403 on /cgi-bin/koha/members/patronimage.pl?borrowernumber=51
Comment 3 Jonathan Druart 2021-11-22 14:03:58 UTC
(In reply to Jonathan Druart from comment #2)
> And I am getting a 403 on
> /cgi-bin/koha/members/patronimage.pl?borrowernumber=51

Wrong, I am seeing the image.
Comment 4 Jonathan Druart 2021-11-22 14:22:55 UTC
I was expecting the following change to work, but it does not.

diff --git a/members/patronimage.pl b/members/patronimage.pl
index da62b875536..c4a6e5eb401 100755
--- a/members/patronimage.pl
+++ b/members/patronimage.pl
@@ -47,7 +47,7 @@ This script, when called from within HTML and passed a valid patron borrowernumb
 
 =cut
 
-my ($status, $cookie, $sessionID) = check_api_auth($query, { catalogue => 1 } );
+my ($status, $cookie, $sessionID) = check_api_auth($query, [ { borrowers => '*' }, { circulate => '*' } ] );
 
 unless ( $status eq 'ok' ) {
     print $query->header(-type => 'text/plain', -status => '403 Forbidden');
Comment 5 Jonathan Druart 2021-11-22 14:31:14 UTC
Created attachment 127932 [details] [review]
Bug 29541: Restrict access to patron's image to borrowers => * and circulate => *

The patron images is displayed on the 'circulation' and 'members'
modules.
Comment 6 Jonathan Druart 2021-11-22 14:31:52 UTC
(In reply to Jonathan Druart from comment #4)
> I was expecting the following change to work, but it does not.
> 
> diff --git a/members/patronimage.pl b/members/patronimage.pl
> index da62b875536..c4a6e5eb401 100755
> --- a/members/patronimage.pl
> +++ b/members/patronimage.pl
> @@ -47,7 +47,7 @@ This script, when called from within HTML and passed a
> valid patron borrowernumb
>  
>  =cut
>  
> -my ($status, $cookie, $sessionID) = check_api_auth($query, { catalogue => 1
> } );
> +my ($status, $cookie, $sessionID) = check_api_auth($query, [ { borrowers =>
> '*' }, { circulate => '*' } ] );
>  
>  unless ( $status eq 'ok' ) {
>      print $query->header(-type => 'text/plain', -status => '403 Forbidden');

It actually works. I have been logged out at some point and haven't noticed...
Comment 7 Jonathan Druart 2021-11-22 14:32:09 UTC
borrowers => * and circulate => *

would that works?
Comment 8 Katrin Fischer 2021-12-06 12:01:06 UTC
I am not sure if borrowers * is correct. I think if you are not allowed to see another library's patron data, you should also not be able to access the patron images for these patrons - so we probably need to add library groups into the equation here?
Comment 9 Jonathan Druart 2021-12-06 13:00:37 UTC
Created attachment 128276 [details] [review]
Bug 29541: Prevent users from another group to access patron's images

We should respect group restrictions here.

Test plan:
Create a patron from another group of libraries and don't let them
access info from patrons outside of this group.
Access the following link and confirm that you can see the image only
for patrons from their group
  /cgi-bin/koha/members/patronimage.pl?borrowernumber=XX
Comment 10 Katrin Fischer 2021-12-20 21:57:25 UTC
Created attachment 128801 [details] [review]
Bug 29541: Restrict access to patron's image to borrowers => * and circulate => *

The patron images is displayed on the 'circulation' and 'members'
modules.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2021-12-20 21:57:29 UTC
Created attachment 128802 [details] [review]
Bug 29541: Prevent users from another group to access patron's images

We should respect group restrictions here.

Test plan:
Create a patron from another group of libraries and don't let them
access info from patrons outside of this group.
Access the following link and confirm that you can see the image only
for patrons from their group
  /cgi-bin/koha/members/patronimage.pl?borrowernumber=XX

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Tomás Cohen Arazi 2022-01-12 12:08:33 UTC
Created attachment 129333 [details] [review]
Bug 29541: Restrict access to patron's image to borrowers => * and circulate => *

The patron images is displayed on the 'circulation' and 'members'
modules.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Tomás Cohen Arazi 2022-01-12 12:08:37 UTC
Created attachment 129334 [details] [review]
Bug 29541: Prevent users from another group to access patron's images

We should respect group restrictions here.

Test plan:
Create a patron from another group of libraries and don't let them
access info from patrons outside of this group.
Access the following link and confirm that you can see the image only
for patrons from their group
  /cgi-bin/koha/members/patronimage.pl?borrowernumber=XX

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Victor Grousset/tuxayo 2022-01-24 04:45:11 UTC
Backported: Pushed to 20.11.x branch for 20.11.14

Testing notes: for the 2nd patch the permission for the observing patron is the subpermission "Add, modify and view patron information (edit_borrowers)"
and without the next one (view_borrower_infos_from_any_libraries)

Then create a group with 2 libraries, confirm that it's possible to see another patron's (from another library) page and image URL. But when removing the library from the group, it's not possible to see the patron's page anymore.
Without the patch, the image URL still works! And the patch fixes that.
Comment 15 Victor Grousset/tuxayo 2022-01-24 04:45:45 UTC
*20.11.x-security branch
Comment 16 wainuiwitikapark 2022-01-26 04:43:46 UTC
Backported to 19.11.x-security branch on the Security repo for 19.11.25 security release.
Comment 17 Andrew Fuerste-Henry 2022-01-26 20:09:07 UTC
Pushed to security 21.05.x for 21.05.09