Bug 25142

Summary: Staff can access patrons' infos from outside of their group
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart+koha>
Status: CLOSED FIXED QA Contact:
Severity: major    
Priority: P5 - low CC: bgkriegel, fridolin.somers, hayleypelham, jonathan.druart+koha, joy, katrin.fischer, lucas, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.05, 19.05.10, 18.11.16
Bug Depends on: 18403    
Bug Blocks:    
Attachments: Bug 25142: Fix wrong grep logical test
Bug 25142: Fix wrong grep logical test
Bug 25142: Fix wrong grep logical test
Bug 25142: Fix wrong grep logical test

Description Jonathan Druart 2020-04-14 11:24:53 UTC

    
Comment 1 Jonathan Druart 2020-04-14 11:29:40 UTC
Created attachment 102923 [details] [review]
Bug 25142: Fix wrong grep logical test

To test and understand what's going on, you can try that bit of code:
my @a = qw( a b c a);
my @b = qw( b c d );
my @c;
@c = grep { 'a' eq $_ } @a ? 'ok' : ();
say @c;
@c = ( grep { 'a' eq $_ } @a ) ? 'ok' : ();
say @c;
@c = grep { 'a' eq $_ } @a ? ('ok') : (undef);
say @c;

The problem here:
Have patrons in 3 branches CPL, MPL, SPL
Have a non superlibrarian with edit_borrowers permission but view_borrower_infos_from_any_libraries, from CPL
Create a library group with CPL, MPL
Use that non superlibrarian to search for patrons
You can search for patrons fro CPL and MPL
BUT, edit the value for CPL, use SPL (edit the DOM)
Search and... oops

Apply this patch, try again

Also use a superlibrarian patron (and/or with view_borrower_infos_from_any_libraries)
and confirm that they can see all patrons
Comment 2 Jonathan Druart 2020-04-14 11:30:15 UTC
This is NOT coming from bug 23084 and must be backported.
Comment 3 Bernardo Gonzalez Kriegel 2020-04-14 11:54:32 UTC
Mmmm,

grep ( { $_ eq $b } @r ) != ( grep { $_ eq $b } @r )

Is this Ok?
Comment 4 Jonathan Druart 2020-04-14 12:07:00 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #3)
> Mmmm,
> 
> grep ( { $_ eq $b } @r ) != ( grep { $_ eq $b } @r )
> 
> Is this Ok?

Yes, that's what I wanted to write.
Comment 5 Bernardo Gonzalez Kriegel 2020-04-14 12:38:20 UTC
Created attachment 102928 [details] [review]
Bug 25142: Fix wrong grep logical test

To test and understand what's going on, you can try that bit of code:
my @a = qw( a b c a);
my @b = qw( b c d );
my @c;
@c = grep { 'a' eq $_ } @a ? 'ok' : ();
say @c;
@c = ( grep { 'a' eq $_ } @a ) ? 'ok' : ();
say @c;
@c = grep { 'a' eq $_ } @a ? ('ok') : (undef);
say @c;

The problem here:
Have patrons in 3 branches CPL, MPL, SPL
Have a non superlibrarian with edit_borrowers permission but view_borrower_infos_from_any_libraries, from CPL
Create a library group with CPL, MPL
Use that non superlibrarian to search for patrons
You can search for patrons fro CPL and MPL
BUT, edit the value for CPL, use SPL (edit the DOM)
Search and... oops

Apply this patch, try again

Also use a superlibrarian patron (and/or with view_borrower_infos_from_any_libraries)
and confirm that they can see all patrons

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested with edit_borrowers permission but NOT view_borrower_infos_from_any_libraries
Editing the DOM I can search (not edit) from any branch
With patch no entries found :)
Comment 6 Katrin Fischer 2020-04-14 22:12:31 UTC
Created attachment 102952 [details] [review]
Bug 25142: Fix wrong grep logical test

To test and understand what's going on, you can try that bit of code:
my @a = qw( a b c a);
my @b = qw( b c d );
my @c;
@c = grep { 'a' eq $_ } @a ? 'ok' : ();
say @c;
@c = ( grep { 'a' eq $_ } @a ) ? 'ok' : ();
say @c;
@c = grep { 'a' eq $_ } @a ? ('ok') : (undef);
say @c;

The problem here:
Have patrons in 3 branches CPL, MPL, SPL
Have a non superlibrarian with edit_borrowers permission but
without view_borrower_infos_from_any_libraries, from CPL
Create a library group with CPL, MPL
Use that non superlibrarian to search for patrons
You can search for patrons fro CPL and MPL
BUT, edit the value for CPL, use SPL (edit the DOM)
Search and... oops

Apply this patch, try again

Also use a superlibrarian patron (and/or with view_borrower_infos_from_any_libraries)
and confirm that they can see all patrons

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested with edit_borrowers permission but NOT view_borrower_infos_from_any_libraries
Editing the DOM I can search (not edit) from any branch
With patch no entries found :)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Amended test plan slightly (+ without)
Comment 7 Katrin Fischer 2020-04-14 22:23:38 UTC
Created attachment 102953 [details] [review]
Bug 25142: Fix wrong grep logical test

To test and understand what's going on, you can try that bit of code:
my @a = qw( a b c a);
my @b = qw( b c d );
my @c;
@c = grep { 'a' eq $_ } @a ? 'ok' : ();
say @c;
@c = ( grep { 'a' eq $_ } @a ) ? 'ok' : ();
say @c;
@c = grep { 'a' eq $_ } @a ? ('ok') : (undef);
say @c;

The problem here:
Have patrons in 3 branches CPL, MPL, SPL
Have a non superlibrarian with edit_borrowers permission but
without view_borrower_infos_from_any_libraries, from CPL
Create a library group with CPL, MPL
Use that non superlibrarian to search for patrons
You can search for patrons fro CPL and MPL
BUT, edit the value for CPL, use SPL (edit the DOM)
Search and... oops

Apply this patch, try again

Also use a superlibrarian patron (and/or with view_borrower_infos_from_any_libraries)
and confirm that they can see all patrons

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Tested with edit_borrowers permission but NOT view_borrower_infos_from_any_libraries
Editing the DOM I can search (not edit) from any branch
With patch no entries found :)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Amended test plan slightly (+ without)
Comment 8 Joy Nelson 2020-04-17 19:06:38 UTC
This does not apply to 19.11.x branch.  Please rebase.
Comment 9 Bernardo Gonzalez Kriegel 2020-04-18 11:31:04 UTC
(In reply to Joy Nelson from comment #8)
> This does not apply to 19.11.x branch.  Please rebase.

Hi Joy, this can be 'backported' modifying line 41 of C4/Utils/DataTables/Members.pm, and it's needed because the bug is present in 19.11 (and previous I suppose)

-            ? grep { /^$branchcode$/ } @restricted_branchcodes
+            ? grep ( { /^$branchcode$/ } @restricted_branchcodes )

That's the equivalent code of the patch for 19.11
I would write something a little bit different

-            ? grep { /^$branchcode$/ } @restricted_branchcodes
+            ? ( grep { /^$branchcode$/ } @restricted_branchcodes )

Both works, can't understand why the first does.
Comment 10 Joy Nelson 2020-04-21 18:10:57 UTC
Pushed to 19.11.x branch for 19.11.05
Comment 11 Hayley Pelham 2020-04-21 21:52:22 UTC
Backported to 18.11.x for 18.11.16
Comment 12 Lucas Gass 2020-04-22 19:52:29 UTC
backported to 19.05.x for 19.05.10
Comment 13 Martin Renvoize 2020-04-27 09:46:04 UTC
Nice work everyone!

Pushed to master for 20.05