Bug 25142 - Staff can access patrons' infos from outside of their group
Summary: Staff can access patrons' infos from outside of their group
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on: 18403
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-14 11:24 UTC by Jonathan Druart
Modified: 2020-11-30 21:48 UTC (History)
9 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:
20.05.00, 19.11.05, 19.05.10, 18.11.16


Attachments
Bug 25142: Fix wrong grep logical test (1.71 KB, patch)
2020-04-14 11:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25142: Fix wrong grep logical test (1.94 KB, patch)
2020-04-14 12:38 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 25142: Fix wrong grep logical test (2.04 KB, patch)
2020-04-14 22:12 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25142: Fix wrong grep logical test (2.04 KB, patch)
2020-04-14 22:23 UTC, Katrin Fischer
Details | Diff | Splinter Review

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