Bug 18403 - Hide patron information if not part of the logged in user library group
Summary: Hide patron information if not part of the logged in user library group
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low new feature (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL: https://github.com/joubu/Koha/commits...
Whiteboard:
Keywords:
Depends on: 16735 17829 18402
Blocks: 18789
  Show dependency treegraph
 
Reported: 2017-04-07 21:40 UTC by Jonathan Druart
Modified: 2017-06-12 16:40 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18403: Hide patron information if not part of the logged in user library group (6.20 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Library->library_group (3.40 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Library::Group->has_child (4.14 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Send logged_in_user to template from C4::Auth (2.77 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Patron->can_see_patron_info (1.80 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers' (45.71 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos (50.19 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Adapt patron search (4.50 KB, patch)
2017-04-10 21:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Only display libraries from group in dropdown lists (12.89 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Use patron-title.inc when hidepatronname is used (14.98 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory] (6.16 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue] (2.44 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Batch patron modification tool (4.92 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: output_and_exit_if_error for circulation.pl (2.66 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed (3.96 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons (7.00 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Add tests for Koha::Patrons (8.15 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Patron->can_see_patrons_from (1.95 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Patron modification requests (8.18 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Patron reviews (6.97 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Patron discharges (11.70 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Article requests (5.89 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Guarantors (2.70 KB, patch)
2017-04-10 21:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: REST API - patrons endpoint (2.41 KB, patch)
2017-04-10 21:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Deal with the DB user (7.74 KB, patch)
2017-04-18 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Fix patron creation (1.17 KB, patch)
2017-04-18 16:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18403: Hide patron information if not part of the logged in user library group (6.25 KB, patch)
2017-04-28 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Library->library_group (3.46 KB, patch)
2017-04-28 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Library::Group->has_child (4.20 KB, patch)
2017-04-28 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Send logged_in_user to template from C4::Auth (2.83 KB, patch)
2017-04-28 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Patron->can_see_patron_info (1.86 KB, patch)
2017-04-28 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers' (45.78 KB, patch)
2017-04-28 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos (50.26 KB, patch)
2017-04-28 15:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Adapt patron search (4.56 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Only display libraries from group in dropdown lists (12.95 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Use patron-title.inc when hidepatronname is used (15.03 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory] (6.21 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue] (2.50 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Batch patron modification tool (4.98 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: output_and_exit_if_error for circulation.pl (2.72 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed (4.02 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons (7.05 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Add tests for Koha::Patrons (8.21 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Add new method Koha::Patron->can_see_patrons_from (2.00 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Patron modification requests (8.24 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Patron reviews (7.03 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Patron discharges (11.76 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Article requests (6.96 KB, patch)
2017-04-28 15:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Guarantors (4.48 KB, patch)
2017-04-28 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: REST API - patrons endpoint (2.41 KB, patch)
2017-04-28 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Deal with the DB user (7.79 KB, patch)
2017-04-28 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 18403: Fix patron creation (1.23 KB, patch)
2017-04-28 15:31 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2017-04-07 21:40:52 UTC
Add the ability to hide patron record information from any other library (outside the group).
Comment 1 Jonathan Druart 2017-04-10 21:32:24 UTC
Created attachment 62010 [details] [review]
Bug 18403: Hide patron information if not part of the logged in user library group

This patchset adds a new feature that will allow libraries inside a
single Koha installation to restrict access to information of patrons
that

The group of libraries feature is introduced by bug 15707, see this bug for more
information.

Let's imagine that 2 groups G1 and G2 are defined and that they include 2 libraries
each G1a, G1b and G2c, G2d: logged in users attached to G1a will only see patron's
information from G1a and G1b.
To add more flexibility, a new user permission named 'view_borrower_infos_from_any_libraries'
will drive this behavior. If set, the patron will be able to see patron's information
of any libraries.

If the restriction is set, the logged in user will not be able to search, show, edit,
delete patron's information of patrons attached to groups of libraries outside his
own group.
In situations we need to refer to a patron, for holds and checkouts for instance,
and his information cannot be viewed, a text "A patron from library G1A" will be
displayed.

Considered unecessary or outside the scope of this bug report:
* The report module is not affected by this feature for obvious reasons
* The firstname and surname of guarantors, basket (acq) managers, patrons linked
to orders are still displayed.
* Log viewer: Can only be staff
* patron list: you cannot add patrons from another group of librairies, but can
see/delete from list (too much rewrite, or we can test for patron one by one?).
* "Patron card creator" tool is not impacted by this feature.
* Upload patron images is not impacted by this patch, should it be?
* Tools:
  - Upload patrons
  - Clean borrowers tool (This can can done easily updating Koha::Patrons->search
with Koha::Patrons->search_limited in search_upcoming_membership_expires and
search_patrons_to_anonymise but we will need to move GetBorrowersToExpunge to
Koha::Patrons first)
We can discuss these different points but will be other bug reports not to add
more complexity to this first patchset.

Test plan:
You will find a test plan in the following commit messages.
Start by creating different group of libraries and patrons with and without the
new permission. Open different browser sessions to ease the tests.
Note that all patches have to be applied to test the different test plans.

Technical notes:
For QAers (and others) a techical note will be added to the commit messages of this
patchset. I would recommend you to read them one by one to understand the different
steps of this development.

+ Special attention should be payed to the REST api changes
+ Should we restrict the logged in user to libraries from his group when
he wants to set his library (Home › Circulation › Set library)?
Comment 2 Jonathan Druart 2017-04-10 21:32:28 UTC
Created attachment 62011 [details] [review]
Bug 18403: Add new method Koha::Library->library_group

This is more a follow-up for bug 15707. It could be moved on its own bug report
if necessary.

Test plan:
  prove t/db_dependent/LibraryGroups.t
should return green
Comment 3 Jonathan Druart 2017-04-10 21:32:32 UTC
Created attachment 62012 [details] [review]
Bug 18403: Add new method Koha::Library::Group->has_child

This is more a follow-up for bug 15707. It could be moved on its own bug report
if necessary.

IMPORTANT NOTE: At the moment the feature only works for 1 level depth, see
bug 15707 comment 166+ for the discussion

It means that if we have:
 root_group
     + groupA
         + groupA1
             + groupA1_library2
         + groupA_library1
         + groupA2
     + groupB
         + groupB_library1
groupA1_library2 is not considered a child of groupA1.
Note that this can change.

Test plan:
  prove t/db_dependent/LibraryGroups.t
should return green
Comment 4 Jonathan Druart 2017-04-10 21:32:35 UTC
Created attachment 62013 [details] [review]
Bug 18403: Send logged_in_user to template from C4::Auth

Technical note:
To ease future changes we are passing a logged_in_user variable to templates.
It contains the Koha::Patron object representing the logged in patron.
This will be very useful for this patch and even after (for instance we will be
able to replace easily loggedinusername and loggedinusernumber).
Comment 5 Jonathan Druart 2017-04-10 21:32:39 UTC
Created attachment 62014 [details] [review]
Bug 18403: Add new method Koha::Patron->can_see_patron_info

Technical note:
This is the method that will be called on the logged_in_user variable sent to
the template. Moreover we will check that the logged in user can access patron'
information when access to members/* and some circulation scripts will be done.
Comment 6 Jonathan Druart 2017-04-10 21:32:44 UTC
Created attachment 62015 [details] [review]
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers'

Test plan:
Login with a patron that only have the 'edit_borrowers' permission.
You should be able to access patron's information of patrons inside of your group.

Technical note:
Before this patchset the borrowers permission module contains only 1 permission 'edit_borrowers'.
That meant
  borrowers => 1
and
  borrowers => '*'
had the same behavior.
Moreover, now that we have 2 permissions, 'CAN_user_borrowers' is set when all
permissions of 'borrowers' are set.
We need to update the different occurrences of these tests.
Comment 7 Jonathan Druart 2017-04-10 21:32:50 UTC
Created attachment 62016 [details] [review]
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos

Test plan:
Login with a patron that is not allowed to see patron's information for patrons
outside of his group. Try to access patron's information from scripts of the patron
module (members/*) and circ/circulation.pl.
You should be able to access patron's information of patrons outside of your group
and get "You are not allowed to see the information of this patron."
If you try and access a patron page with a borrowernumber that does not exist, you
should get "This patron does not exist"

Technical note:
A new C4::Output subroutine is created in this patch: "output_and_exit_if_error"
Executed at the beginning of the script it will permit not to copy/paste all the
different checks to know if the logged in user is authorised to see patron's information.
The design here can be discussed, but I did not find an alternative with as less changes.
On the way I refactor what we did with 'unknowuser' previously: it will now work with all
patron pages, not only the few that used it.
Note that the 'or die "Not logged in";' part should not be needed, but... who trusts
C4::Auth?
I think it could be used as a safeguard later. I am willing to sed and remove them
if required.

Changes in discharge.pl are mainly indentation changes.

With this patch we should now have a $patron variable that refer to the patron we
want to access. That will be very useful to remove plenty of code in members/* and
only pass this variable to the template (instead of 1 variable per patron's attribute).
Comment 8 Jonathan Druart 2017-04-10 21:32:54 UTC
Created attachment 62017 [details] [review]
Bug 18403: Adapt patron search

This patch modifies the patron search code to limit the libraries to the
ones
the logged in user is allowed to access

Test plan:
Search for patrons
You should not see patrons you are not allowed to see.

Technical note:
I am really glad to have refactored all the patron searches before
having to
write this patch. It tooks me ~40 l to acchieve this job and affect all
patron searches.
Thanks refactoring!
Comment 9 Jonathan Druart 2017-04-10 21:33:03 UTC
Created attachment 62018 [details] [review]
Bug 18403: Only display libraries from group in dropdown lists

From where patrons it's about patrons, we do not want to display the libraries
from all the system, but only the ones from the group.

Test plan:
- See the overdues (circ/overdue.pl) and make sure you can only see overdues from
patrons part of your group (do not forget to test the CSV export).
- Search for patrons, the 'library' filters (headers and left side) should only
display libraries from your group
- Search for article request by patron's library: only the libraries from your
group should be displayed
Comment 10 Jonathan Druart 2017-04-10 21:33:08 UTC
Created attachment 62019 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used

There is already a HidePatronName syspref to hide patron's information
on bibliographic
record detail pages and the hold list.

Test plan:
With the HidePatronName enabled, make sure the patron's information are
hidden from
the catalogue and hold list pages. If the logged in user is not allowed
to see the
patron's info, no link and no cardnumber will be displayed
With he HidePatronName disabled, make sure the patron's information are
displayed
if the logged in user is allowed to see the patron's info.

Technical note:
This patch improves the existing patron-title.inc include file to
display patron's
information. Using it everywhere patron's details are displayed will
permit to
homogenise the way they are displayed. The file takes now a patron
object (what
should be, in the future, the only way to use it), that way we can call
the new
method on it to know if patron's information can be shown by the logged
in used.

NOTE: I am not sure this syspref makes sense anymore. Should not we
remove it?
Comment 11 Jonathan Druart 2017-04-10 21:33:13 UTC
Created attachment 62020 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory]

On this page we do not have the patron object sent to the template,
let's pass it!

Test plan:
Go on the checkout history of a bibliographic record
(catalogue/issuehistory.pl)
You should not see patron's information that are not part of your group
if you
are not allowed to see them.
Comment 12 Jonathan Druart 2017-04-10 21:33:17 UTC
Created attachment 62021 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue]

Same that the previous patch but for the holds queue
Comment 13 Jonathan Druart 2017-04-10 21:33:20 UTC
Created attachment 62022 [details] [review]
Bug 18403: Batch patron modification tool

Do not allow a logged in staff user to modify patrons that are not part of his
group if he is not allowed.

Test plan:
Make sure you are not allowed to modify patrons that are not part of your group
from the batch patron modification tool
Comment 14 Jonathan Druart 2017-04-10 21:33:24 UTC
Created attachment 62023 [details] [review]
Bug 18403: output_and_exit_if_error for circulation.pl

This is a follow-up for a previous patch, changes have been tested
already
Comment 15 Jonathan Druart 2017-04-10 21:33:27 UTC
Created attachment 62024 [details] [review]
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed

Most of the time when we search for patrons we do not want to search for all patrons,
but just the ones the logged in user is allowed to see the information.
This patch takes care of that by adding a new search_limited method to Koha::Patrons.
When called this method only search for patrons that the logged in user is allowed
to see.

Test plan:
Patron autocomplete search should be limited
Comment 16 Jonathan Druart 2017-04-10 21:33:31 UTC
Created attachment 62025 [details] [review]
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons

Technical note:
Here we are just refactoring a code that have been copied into 3 different places.
libraries_where_can_see_patrons is a terrible method's name, feel free to suggest
something better. The method return a list of branchcodes to be more efficient,
instead of Koha::Libraries
Comment 17 Jonathan Druart 2017-04-10 21:33:34 UTC
Created attachment 62026 [details] [review]
Bug 18403: Add tests for Koha::Patrons

A bit late but here are the tests for
 Koha::Patron->libraries_where_can_see_patrons
 Koha::Patron->can_see_patron_infos
 Koha::Patron->search_limited

Test plan:
  prove t/db_dependent/Koha/Patrons.t
should return green
Comment 18 Jonathan Druart 2017-04-10 21:33:38 UTC
Created attachment 62027 [details] [review]
Bug 18403: Add new method Koha::Patron->can_see_patrons_from

Technical note:
Sometimes we do not have the patron object, for instance for the patron modifications
we will need to know if the logged in user can modify patron's from a given library.
This new subroutine 'can_see_patrons_from' will then be useful
Comment 19 Jonathan Druart 2017-04-10 21:33:42 UTC
Created attachment 62028 [details] [review]
Bug 18403: Patron modification requests

Limit patron's modifications based on logged in patron permissions.

Test plan:
Create some patron's modification requests at the OPAC
Make sure the logged in staff user see (or not) the modification depending his
permissions.
The number of modification displayed on the mainpage should be correct as well.
Comment 20 Jonathan Druart 2017-04-10 21:33:46 UTC
Created attachment 62029 [details] [review]
Bug 18403: Patron reviews

This patch adds a new method Koha::Reviews->search_limited to return the
reviews
a logged in user is allowed to see depending his permissions.

Test plan:
Create some reviews at the OPAC and make sure a staff user is limited
(or not) to approve
or decline it.
The number of reviews displayed on the mainpage should be correct as
well.
Comment 21 Jonathan Druart 2017-04-10 21:33:50 UTC
Created attachment 62030 [details] [review]
Bug 18403: Patron discharges

This patch deals with patron's discharges.

Test plan:
Same as previously you will need to request dischages at the OPAC.
On the staff interface the logged in user should not be allowed to see
discharge
from patrons outside his library group.
The number of discharges waiting displayed on the mainpage should be
correct as well.
Comment 22 Jonathan Druart 2017-04-10 21:33:54 UTC
Created attachment 62031 [details] [review]
Bug 18403: Article requests

Same as previously but for article requests.

Test plan:
Test article requests and make sure you do not need the requests for
patrons that
are attached to a group that is not part of your library's group
Comment 23 Jonathan Druart 2017-04-10 21:33:58 UTC
Created attachment 62032 [details] [review]
Bug 18403: Guarantors

Technically a kid from your library group could have a guarantor
attached to another
group of library, let's deal with this case.

Test plan:
- Create a kid from your library group
- With a superlibrarian staff user create a guarantor that is outside of
the group of
libraries of the kid
- Login with a limited staff user and confirm that on the patron detail
page you do not
see the link to the guarantor detail page.

Note that you see the firstname and surname of the guarantor
Q. should it be hidden?
Comment 24 Jonathan Druart 2017-04-10 21:34:02 UTC
Created attachment 62033 [details] [review]
Bug 18403: REST API - patrons endpoint

There is something wrond here, the userenv is no set and so we cannot
user search_limited.
Should we set the userenv or filter on the libraries using
libraries_where_can_see_patrons?
WAITING FOR FEEDBACK HERE.
Comment 25 Jonathan Druart 2017-04-10 21:37:00 UTC
There are too many dependencies on this one, please see the following remote branch for the whole stack:
https://github.com/joubu/Koha/commits/bug_18403
Comment 26 Jonathan Druart 2017-04-18 16:15:02 UTC
Created attachment 62302 [details] [review]
Bug 18403: Deal with the DB user

On first login, Koha explodes before the logged in user does not exist
in DB.
This patch deals with that by adding several checks when it's needed.

Test plan:
Use the DB user to create a superlibrarian user.
The DB user should no be allowed to do anything else.
Comment 27 Jonathan Druart 2017-04-18 16:15:08 UTC
Created attachment 62303 [details] [review]
Bug 18403: Fix patron creation

memberentry.pl can be called to create a new patron, in that case the
patron does not exist yet.
Comment 28 Martin Renvoize 2017-04-26 10:27:37 UTC
We're walking through testing this... as it's such a big piece we've split out the testing results into a shared spreadsheet available here:

https://docs.google.com/spreadsheets/d/11NVVHFDJavGFhnARYozmdR-FVdKWxDEjavJAZRtB0Ww/edit?usp=sharing
Comment 29 Martin Renvoize 2017-04-28 15:29:35 UTC
Created attachment 62830 [details] [review]
Bug 18403: Hide patron information if not part of the logged in user library group

This patchset adds a new feature that will allow libraries inside a
single Koha installation to restrict access to information of patrons
that

The group of libraries feature is introduced by bug 15707, see this bug for more
information.

Let's imagine that 2 groups G1 and G2 are defined and that they include 2 libraries
each G1a, G1b and G2c, G2d: logged in users attached to G1a will only see patron's
information from G1a and G1b.
To add more flexibility, a new user permission named 'view_borrower_infos_from_any_libraries'
will drive this behavior. If set, the patron will be able to see patron's information
of any libraries.

If the restriction is set, the logged in user will not be able to search, show, edit,
delete patron's information of patrons attached to groups of libraries outside his
own group.
In situations we need to refer to a patron, for holds and checkouts for instance,
and his information cannot be viewed, a text "A patron from library G1A" will be
displayed.

Considered unecessary or outside the scope of this bug report:
* The report module is not affected by this feature for obvious reasons
* The firstname and surname of guarantors, basket (acq) managers, patrons linked
to orders are still displayed.
* Log viewer: Can only be staff
* patron list: you cannot add patrons from another group of librairies, but can
see/delete from list (too much rewrite, or we can test for patron one by one?).
* "Patron card creator" tool is not impacted by this feature.
* Upload patron images is not impacted by this patch, should it be?
* Tools:
  - Upload patrons
  - Clean borrowers tool (This can can done easily updating Koha::Patrons->search
with Koha::Patrons->search_limited in search_upcoming_membership_expires and
search_patrons_to_anonymise but we will need to move GetBorrowersToExpunge to
Koha::Patrons first)
We can discuss these different points but will be other bug reports not to add
more complexity to this first patchset.

Test plan:
You will find a test plan in the following commit messages.
Start by creating different group of libraries and patrons with and without the
new permission. Open different browser sessions to ease the tests.
Note that all patches have to be applied to test the different test plans.

Technical notes:
For QAers (and others) a techical note will be added to the commit messages of this
patchset. I would recommend you to read them one by one to understand the different
steps of this development.

+ Special attention should be payed to the REST api changes
+ Should we restrict the logged in user to libraries from his group when
he wants to set his library (Home › Circulation › Set library)?

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 30 Martin Renvoize 2017-04-28 15:29:39 UTC
Created attachment 62831 [details] [review]
Bug 18403: Add new method Koha::Library->library_group

This is more a follow-up for bug 15707. It could be moved on its own bug report
if necessary.

Test plan:
  prove t/db_dependent/LibraryGroups.t
should return green

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 31 Martin Renvoize 2017-04-28 15:29:43 UTC
Created attachment 62832 [details] [review]
Bug 18403: Add new method Koha::Library::Group->has_child

This is more a follow-up for bug 15707. It could be moved on its own bug report
if necessary.

IMPORTANT NOTE: At the moment the feature only works for 1 level depth, see
bug 15707 comment 166+ for the discussion

It means that if we have:
 root_group
     + groupA
         + groupA1
             + groupA1_library2
         + groupA_library1
         + groupA2
     + groupB
         + groupB_library1
groupA1_library2 is not considered a child of groupA1.
Note that this can change.

Test plan:
  prove t/db_dependent/LibraryGroups.t
should return green

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 32 Martin Renvoize 2017-04-28 15:29:46 UTC
Created attachment 62833 [details] [review]
Bug 18403: Send logged_in_user to template from C4::Auth

Technical note:
To ease future changes we are passing a logged_in_user variable to templates.
It contains the Koha::Patron object representing the logged in patron.
This will be very useful for this patch and even after (for instance we will be
able to replace easily loggedinusername and loggedinusernumber).

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 33 Martin Renvoize 2017-04-28 15:29:50 UTC
Created attachment 62834 [details] [review]
Bug 18403: Add new method Koha::Patron->can_see_patron_info

Technical note:
This is the method that will be called on the logged_in_user variable sent to
the template. Moreover we will check that the logged in user can access patron'
information when access to members/* and some circulation scripts will be done.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 34 Martin Renvoize 2017-04-28 15:29:54 UTC
Created attachment 62835 [details] [review]
Bug 18403: Update permissions - borrowers => 1|* becomes borrowers => 'edit_borrowers'

Test plan:
Login with a patron that only have the 'edit_borrowers' permission.
You should be able to access patron's information of patrons inside of your group.

Technical note:
Before this patchset the borrowers permission module contains only 1 permission 'edit_borrowers'.
That meant
  borrowers => 1
and
  borrowers => '*'
had the same behavior.
Moreover, now that we have 2 permissions, 'CAN_user_borrowers' is set when all
permissions of 'borrowers' are set.
We need to update the different occurrences of these tests.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 35 Martin Renvoize 2017-04-28 15:29:58 UTC
Created attachment 62836 [details] [review]
Bug 18403: Add sub output_and_exit_if_error - unknown_patron & cannot_see_patron_infos

Test plan:
Login with a patron that is not allowed to see patron's information for patrons
outside of his group. Try to access patron's information from scripts of the patron
module (members/*) and circ/circulation.pl.
You should be able to access patron's information of patrons outside of your group
and get "You are not allowed to see the information of this patron."
If you try and access a patron page with a borrowernumber that does not exist, you
should get "This patron does not exist"

Technical note:
A new C4::Output subroutine is created in this patch: "output_and_exit_if_error"
Executed at the beginning of the script it will permit not to copy/paste all the
different checks to know if the logged in user is authorised to see patron's information.
The design here can be discussed, but I did not find an alternative with as less changes.
On the way I refactor what we did with 'unknowuser' previously: it will now work with all
patron pages, not only the few that used it.
Note that the 'or die "Not logged in";' part should not be needed, but... who trusts
C4::Auth?
I think it could be used as a safeguard later. I am willing to sed and remove them
if required.

Changes in discharge.pl are mainly indentation changes.

With this patch we should now have a $patron variable that refer to the patron we
want to access. That will be very useful to remove plenty of code in members/* and
only pass this variable to the template (instead of 1 variable per patron's attribute).

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 36 Martin Renvoize 2017-04-28 15:30:02 UTC
Created attachment 62837 [details] [review]
Bug 18403: Adapt patron search

This patch modifies the patron search code to limit the libraries to the
ones
the logged in user is allowed to access

Test plan:
Search for patrons
You should not see patrons you are not allowed to see.

Technical note:
I am really glad to have refactored all the patron searches before
having to
write this patch. It tooks me ~40 l to acchieve this job and affect all
patron searches.
Thanks refactoring!

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 37 Martin Renvoize 2017-04-28 15:30:07 UTC
Created attachment 62838 [details] [review]
Bug 18403: Only display libraries from group in dropdown lists

From where patrons it's about patrons, we do not want to display the libraries
from all the system, but only the ones from the group.

Test plan:
- See the overdues (circ/overdue.pl) and make sure you can only see overdues from
patrons part of your group (do not forget to test the CSV export).
- Search for patrons, the 'library' filters (headers and left side) should only
display libraries from your group
- Search for article request by patron's library: only the libraries from your
group should be displayed

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 38 Martin Renvoize 2017-04-28 15:30:11 UTC
Created attachment 62839 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used

There is already a HidePatronName syspref to hide patron's information
on bibliographic
record detail pages and the hold list.

Test plan:
With the HidePatronName enabled, make sure the patron's information are
hidden from
the catalogue and hold list pages. If the logged in user is not allowed
to see the
patron's info, no link and no cardnumber will be displayed
With he HidePatronName disabled, make sure the patron's information are
displayed
if the logged in user is allowed to see the patron's info.

Technical note:
This patch improves the existing patron-title.inc include file to
display patron's
information. Using it everywhere patron's details are displayed will
permit to
homogenise the way they are displayed. The file takes now a patron
object (what
should be, in the future, the only way to use it), that way we can call
the new
method on it to know if patron's information can be shown by the logged
in used.

NOTE: I am not sure this syspref makes sense anymore. Should not we
remove it?

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 39 Martin Renvoize 2017-04-28 15:30:15 UTC
Created attachment 62840 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for issuehistory]

On this page we do not have the patron object sent to the template,
let's pass it!

Test plan:
Go on the checkout history of a bibliographic record
(catalogue/issuehistory.pl)
You should not see patron's information that are not part of your group
if you
are not allowed to see them.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 40 Martin Renvoize 2017-04-28 15:30:19 UTC
Created attachment 62841 [details] [review]
Bug 18403: Use patron-title.inc when hidepatronname is used [SPECIFIC for view_holdsqueue]

Same that the previous patch but for the holds queue

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 41 Martin Renvoize 2017-04-28 15:30:23 UTC
Created attachment 62842 [details] [review]
Bug 18403: Batch patron modification tool

Do not allow a logged in staff user to modify patrons that are not part of his
group if he is not allowed.

Test plan:
Make sure you are not allowed to modify patrons that are not part of your group
from the batch patron modification tool

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 42 Martin Renvoize 2017-04-28 15:30:27 UTC
Created attachment 62843 [details] [review]
Bug 18403: output_and_exit_if_error for circulation.pl

This is a follow-up for a previous patch, changes have been tested
already

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 43 Martin Renvoize 2017-04-28 15:30:31 UTC
Created attachment 62844 [details] [review]
Bug 18403: Add new methods Koha::Patrons->search_limited and use it where needed

Most of the time when we search for patrons we do not want to search for all patrons,
but just the ones the logged in user is allowed to see the information.
This patch takes care of that by adding a new search_limited method to Koha::Patrons.
When called this method only search for patrons that the logged in user is allowed
to see.

Test plan:
Patron autocomplete search should be limited

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 44 Martin Renvoize 2017-04-28 15:30:35 UTC
Created attachment 62845 [details] [review]
Bug 18403: Refactor and add Koha::Patron->libraries_where_can_see_patrons

Technical note:
Here we are just refactoring a code that have been copied into 3 different places.
libraries_where_can_see_patrons is a terrible method's name, feel free to suggest
something better. The method return a list of branchcodes to be more efficient,
instead of Koha::Libraries

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 45 Martin Renvoize 2017-04-28 15:30:39 UTC
Created attachment 62846 [details] [review]
Bug 18403: Add tests for Koha::Patrons

A bit late but here are the tests for
 Koha::Patron->libraries_where_can_see_patrons
 Koha::Patron->can_see_patron_infos
 Koha::Patron->search_limited

Test plan:
  prove t/db_dependent/Koha/Patrons.t
should return green

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 46 Martin Renvoize 2017-04-28 15:30:43 UTC
Created attachment 62847 [details] [review]
Bug 18403: Add new method Koha::Patron->can_see_patrons_from

Technical note:
Sometimes we do not have the patron object, for instance for the patron modifications
we will need to know if the logged in user can modify patron's from a given library.
This new subroutine 'can_see_patrons_from' will then be useful

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 47 Martin Renvoize 2017-04-28 15:30:47 UTC
Created attachment 62848 [details] [review]
Bug 18403: Patron modification requests

Limit patron's modifications based on logged in patron permissions.

Test plan:
Create some patron's modification requests at the OPAC
Make sure the logged in staff user see (or not) the modification depending his
permissions.
The number of modification displayed on the mainpage should be correct as well.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 48 Martin Renvoize 2017-04-28 15:30:51 UTC
Created attachment 62849 [details] [review]
Bug 18403: Patron reviews

This patch adds a new method Koha::Reviews->search_limited to return the
reviews
a logged in user is allowed to see depending his permissions.

Test plan:
Create some reviews at the OPAC and make sure a staff user is limited
(or not) to approve
or decline it.
The number of reviews displayed on the mainpage should be correct as
well.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 49 Martin Renvoize 2017-04-28 15:30:55 UTC
Created attachment 62850 [details] [review]
Bug 18403: Patron discharges

This patch deals with patron's discharges.

Test plan:
Same as previously you will need to request dischages at the OPAC.
On the staff interface the logged in user should not be allowed to see
discharge
from patrons outside his library group.
The number of discharges waiting displayed on the mainpage should be
correct as well.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 50 Martin Renvoize 2017-04-28 15:30:59 UTC
Created attachment 62851 [details] [review]
Bug 18403: Article requests

Same as previously but for article requests.

Test plan:
Test article requests and make sure you do not need the requests for
patrons that
are attached to a group that is not part of your library's group

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 51 Martin Renvoize 2017-04-28 15:31:03 UTC
Created attachment 62852 [details] [review]
Bug 18403: Guarantors

Technically a kid from your library group could have a guarantor
attached to another
group of library, let's deal with this case.

Test plan:
- Create a kid from your library group
- With a superlibrarian staff user create a guarantor that is outside of
the group of
libraries of the kid
- Login with a limited staff user and confirm that on the patron detail
page you do not
see the link to the guarantor detail page.

Note that you see the firstname and surname of the guarantor
Q. should it be hidden?

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 52 Martin Renvoize 2017-04-28 15:31:08 UTC
Created attachment 62853 [details] [review]
Bug 18403: REST API - patrons endpoint

There is something wrond here, the userenv is no set and so we cannot
user search_limited.
Should we set the userenv or filter on the libraries using
libraries_where_can_see_patrons?
WAITING FOR FEEDBACK HERE.
Comment 53 Martin Renvoize 2017-04-28 15:31:12 UTC
Created attachment 62854 [details] [review]
Bug 18403: Deal with the DB user

On first login, Koha explodes before the logged in user does not exist
in DB.
This patch deals with that by adding several checks when it's needed.

Test plan:
Use the DB user to create a superlibrarian user.
The DB user should no be allowed to do anything else.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 54 Martin Renvoize 2017-04-28 15:31:16 UTC
Created attachment 62855 [details] [review]
Bug 18403: Fix patron creation

memberentry.pl can be called to create a new patron, in that case the
patron does not exist yet.

Signed-off-by: Jon McGowan <jon.mcgowan@ptfs-europe.com>
Comment 55 Martin Renvoize 2017-04-28 15:33:30 UTC
It's your friendly git gopher again.. 

Uploaded signed off patches on behalf of Jon McGowan at PTFS Europe.  He and Jonathan Field have both rigorously tested this and you can see the feedback and testing results in the spreadsheet linked to above.

Signed Off.
Comment 56 Jonathan Druart 2017-06-02 16:02:19 UTC
I have rebased the remote branch and added the signed-off-by line.
Comment 57 Jonathan Druart 2017-06-02 16:06:24 UTC
Obsoleted the patches here to avoid confusions. The remote branch must be used.