Bug 29509 - GET /patrons* routes permissions excessive
Summary: GET /patrons* routes permissions excessive
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords: Sandbox
Depends on: 29510 30230 29506 30055 35773
Blocks:
  Show dependency treegraph
 
Reported: 2021-11-17 19:42 UTC by Tomás Cohen Arazi
Modified: 2024-04-05 09:29 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 29509: WIP - Start of DB and specification update (2.54 KB, patch)
2024-01-22 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29509: Update swagger specification and add permissions to users (4.89 KB, patch)
2024-01-23 09:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29509: Update swagger specification and add permissions to users (4.88 KB, patch)
2024-01-23 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29509: Update swagger specification and add permissions to users (5.05 KB, patch)
2024-01-23 10:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29509: Update swagger specification and add permissions to users (5.33 KB, patch)
2024-01-23 10:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29509: Update swagger specification and add permissions to users (5.33 KB, patch)
2024-02-29 10:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29509: Correction to bitwise check (1.06 KB, patch)
2024-02-29 10:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29509: Update swagger specification and add permissions to users (5.38 KB, patch)
2024-03-01 19:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 29509: Correction to bitwise check (1.12 KB, patch)
2024-03-01 19:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 29509: Update swagger specification and add permissions to users (5.54 KB, patch)
2024-04-05 09:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-11-17 19:42:14 UTC
The routes require full borrowers permissions, and that's not the case for Koha, generally.
If required permissions were to be lowered, bug 29503 should be pushed first.
Comment 1 Tomás Cohen Arazi 2021-11-17 21:16:35 UTC
Based on moremember.pl we could do { borrowers => 'edit_borrowers' }.
Comment 2 Martin Renvoize 2021-11-18 07:57:54 UTC
I wondering how our changes to objects.search and objects.find might affect public routes and seeing ones owned data?
Comment 3 Tomás Cohen Arazi 2021-11-18 10:55:31 UTC
(In reply to Martin Renvoize from comment #2)
> I wondering how our changes to objects.search and objects.find might affect
> public routes and seeing ones owned data?

That's a good question. It probably highlights the search_limited methods are too staff side oriented.

I did this:
$ git grep 'sub search_limited'
Koha/ArticleRequests.pm:sub search_limited {
Koha/Patron/Discharge.pm:sub search_limited {
Koha/Patrons.pm:sub search_limited {
Koha/Reviews.pm:sub search_limited {

It feels like safe for now.
Comment 4 Tomás Cohen Arazi 2022-02-09 11:26:02 UTC
We need a new 'list_borrowers' subpermission.
Comment 5 Jonathan Druart 2022-02-09 12:03:45 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> We need a new 'list_borrowers' subpermission.

On top of bug 30055 please.
Comment 6 David Cook 2023-08-31 23:18:16 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> We need a new 'list_borrowers' subpermission.

That's my conclusion with bug 30230 as well. I was thinking "view_borrowers" but "list_borrowers" might be better. We'd need to add it to "member.pl" as well as that provides a page which calls the /patrons* routes to populate the data table.

I think bug 29523 will become more and more relevant there as well...
Comment 7 David Cook 2023-11-21 23:51:40 UTC
(In reply to Tomás Cohen Arazi from comment #4)
> We need a new 'list_borrowers' subpermission.

I think we do, and we could retrospectively add it to any patron account that has "edit_borrowers" during the upgrade, so really there's no reason not to. I don't know why I didn't think to do this earlier in the release cycle...
Comment 8 Katrin Fischer 2023-11-23 22:07:57 UTC
I am not sure I understand the difference between view and list, can you explain?
Comment 9 David Cook 2023-11-23 23:35:07 UTC
(In reply to Katrin Fischer from comment #8)
> I am not sure I understand the difference between view and list, can you
> explain?

To me, I think "list" would align better with the REST API. It would be a permission that lets you GET a list of borrowers. 

For the patron detail page, I suppose that might be a list of 1 borrower conceptually...

I suggested "view" since it implies a read-only permission but it doesn't really describe much beyond that?
Comment 10 Martin Renvoize 2024-01-11 15:29:45 UTC
Can we be even clearer here somehow?

i.e. should it be 'list_`something`_borrowers' (and whilst we're here can we swap out 'borrowers' for 'users' as it affect both borrowers and staff 'users'.

The reason I add the 'something' in the middle is that I want it made clear this permission only allows the end api consumer to see the users they should be able to see (i.e. limited by library or library group depending on settings, vs the 'view_borrower_infos_from_any_library' option that expands that list significantly.. in theory at least)
Comment 11 Martin Renvoize 2024-01-22 12:51:15 UTC
David goes ahead and adds the list_borrowers permission in bug 30230
Comment 12 Martin Renvoize 2024-01-22 12:52:19 UTC
We should use this bug to clean up the old permissions on the endpoint.
Comment 13 Martin Renvoize 2024-01-22 16:50:34 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2024-01-22 16:50:54 UTC
Just some scaffolding to show what I'm planning here.
Comment 15 Martin Renvoize 2024-01-23 09:19:12 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2024-01-23 09:21:38 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2024-01-23 10:08:26 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2024-01-23 10:10:45 UTC
Created attachment 161271 [details] [review]
Bug 29509: Update swagger specification and add permissions to users

This patch removes the 'edit_borrowers', 'manage_bookings',
'lable_creator', 'routing' and 'order_manage' permissions from the list
of options in the patrons list endpoint.

We then assign the new 'list_borrowers' permission to any users who have
those removed permissions

Test plan
1) Apply patch and run the database update
2) Users with any of the permissions listed above should now also have
   the 'list_borrowers' permission too.
3) Check that patron searching continues to work from the various
   locations in the UI for the above affected users
Comment 19 Martin AUBEUT 2024-01-23 11:24:57 UTC
(In reply to Martin Renvoize from comment #18)
> Created attachment 161271 [details] [review] [review]
> Bug 29509: Update swagger specification and add permissions to users
> 
> This patch removes the 'edit_borrowers', 'manage_bookings',
> 'lable_creator', 'routing' and 'order_manage' permissions from the list
> of options in the patrons list endpoint.
> 
> We then assign the new 'list_borrowers' permission to any users who have
> those removed permissions
> 
> Test plan
> 1) Apply patch and run the database update
> 2) Users with any of the permissions listed above should now also have
>    the 'list_borrowers' permission too.
> 3) Check that patron searching continues to work from the various
>    locations in the UI for the above affected users

For your information, this is the routes {staff_url}/api/v1/patrons
During the test plan, we needed more details about the locations related to : manage_bookings, lable_creator, routing, order_manage.
Comment 20 David Cook 2024-01-23 23:17:26 UTC
I think I like this idea. I don't love the idea of "implicit" permissions for list_borrowers for things like "circulation > manage_bookings", so good to be explicit. (And to add the permissions for current users so it's not a breaking change.)
Comment 21 Martin Renvoize 2024-01-24 07:06:57 UTC
I'm keen to use hackfest to try to progress a bit on role based access control. Our current 0ermissions are frankly a bit of a mess with the split really not working well for the API in many cases. I'm envisaging a bit of a move to having a single list of crud based permissions for each endpoint and then using roles to group those permissions around functional requirements allowing permissions to be added as required to multiple roles and then roles assigned to end users, sometimes multiple roles.
Comment 22 Victor Grousset/tuxayo 2024-02-22 02:46:35 UTC
> 2) Users with any of the permissions listed above should now also have
>   the 'list_borrowers' permission too.

I took a borrower, gave them
- catalogue
- manage_bookings
- edit_borrowers
- order_manage
- label_creator
- routing
to turn them info a librarian with the minimal permission for the tasks that relate to list_borrowers

then
- apply patch & dep
- updatedatabase (dbrev printed as expected)
- restart_all
- opened the permission page
they still don't have list_borrowers permission! :o

Any idea about what could be missing?
No reason for them to be caught in @exclusions so I don't know why they don't have the permission.

---

> 3) Check that patron searching continues to work from the various
   locations in the UI for the above affected users

For the next step when I'll be able to reach it, should I remove
- manage_bookings
- edit_borrowers
- order_manage
- label_creator
- routing
otherwise, that doesn't test at all that list_borrowers is really there. Vs just look at the permission page.

Or maybe all the related features don't yet use list_borrowers?
If they do use, then it's supposed to be already tested. But I don't mind double-checking if that looks relevant here.
Comment 23 Martin Renvoize 2024-02-29 10:19:00 UTC
Created attachment 162579 [details] [review]
Bug 29509: Update swagger specification and add permissions to users

This patch removes the 'edit_borrowers', 'manage_bookings',
'lable_creator', 'routing' and 'order_manage' permissions from the list
of options in the patrons list endpoint.

We then assign the new 'list_borrowers' permission to any users who have
those removed permissions

Test plan
1) Apply patch and run the database update
2) Users with any of the permissions listed above should now also have
   the 'list_borrowers' permission too.
3) Check that patron searching continues to work from the various
   locations in the UI for the above affected users
Comment 24 Martin Renvoize 2024-02-29 10:19:03 UTC
Created attachment 162580 [details] [review]
Bug 29509: Correction to bitwise check

I was using it wrong :(
Comment 25 Martin Renvoize 2024-02-29 10:22:12 UTC
Thanks for testing Victor.. you did indeed highlight an issue with my database update... (I always struggle with these bitwise ops)

The follow-up corrects the DB update.

No, don't remove those permissions from the users, they need to be left in place else the features are disabled entirely for the user.  The permissions are removed from the api schema.. so the requirement is to perform a patron search from each of those features and confirm the patron search still works.. if it doesn't then the permission wasn't added correctly.
Comment 26 Victor Grousset/tuxayo 2024-03-01 19:49:29 UTC
Created attachment 162690 [details] [review]
Bug 29509: Update swagger specification and add permissions to users

This patch removes the 'edit_borrowers', 'manage_bookings',
'lable_creator', 'routing' and 'order_manage' permissions from the list
of options in the patrons list endpoint.

We then assign the new 'list_borrowers' permission to any users who have
those removed permissions

Test plan
1) Apply patch and run the database update
2) Users with any of the permissions listed above should now also have
   the 'list_borrowers' permission too.
3) Check that patron searching continues to work from the various
   locations in the UI for the above affected users

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 27 Victor Grousset/tuxayo 2024-03-01 19:49:32 UTC
Created attachment 162691 [details] [review]
Bug 29509: Correction to bitwise check

I was using it wrong :(

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 28 Victor Grousset/tuxayo 2024-03-01 19:52:44 UTC
(In reply to Martin Renvoize from comment #25)
> Thanks for testing Victor.. you did indeed highlight an issue with my
> database update... (I always struggle with these bitwise ops)
> 
> The follow-up corrects the DB update.
> 
> No, don't remove those permissions from the users, they need to be left in
> place else the features are disabled entirely for the user.  

Oh right, I won't be able to get to the view that uses the search.

---

It works! :)

Testing notes:
- manage_bookings: search a patron to make a booking
- edit_borrowers: search a guarantor when editing a patron
- order_manage: search a patron in the form for new order
- label_creator: search a patron when creating a new batch
- routing: search a patron as a recipient in the "create routing list" form
Comment 29 Marcel de Rooy 2024-04-05 09:26:31 UTC
Created attachment 164461 [details] [review]
Bug 29509: Update swagger specification and add permissions to users

This patch removes the 'edit_borrowers', 'manage_bookings',
'lable_creator', 'routing' and 'order_manage' permissions from the list
of options in the patrons list endpoint.

We then assign the new 'list_borrowers' permission to any users who have
those removed permissions

Test plan
1) Apply patch and run the database update
2) Users with any of the permissions listed above should now also have
   the 'list_borrowers' permission too.
3) Check that patron searching continues to work from the various
   locations in the UI for the above affected users

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Incorporated second patch and removed 1<<4. 16 reads much better :)
Comment 30 Marcel de Rooy 2024-04-05 09:27:37 UTC
This still needs a bit of attention.

If I add a user with Tools permission, he wont get the list_borrowers since you only check user_permissions. But you should also check flags.
Comment 31 Marcel de Rooy 2024-04-05 09:28:35 UTC
Looking at the dbrev, I am wondering if we should do just one query instead of the whole bunch one at a time.

Could be useful to print the number of added permissions?
Comment 32 Marcel de Rooy 2024-04-05 09:29:40 UTC
(In reply to Marcel de Rooy from comment #30)
> This still needs a bit of attention.
> 
> If I add a user with Tools permission, he wont get the list_borrowers since
> you only check user_permissions. But you should also check flags.

Same for the others obviously.