Bug 29509

Based on moremember.pl we could do { borrowers => 'edit_borrowers' }.

I wondering how our changes to objects.search and objects.find might affect public routes and seeing ones owned data?

(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.

We need a new 'list_borrowers' subpermission.

(In reply to Tomás Cohen Arazi from comment #4)
> We need a new 'list_borrowers' subpermission.

On top of bug 30055 please.

(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...

(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...

I am not sure I understand the difference between view and list, can you explain?

(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?

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)

David goes ahead and adds the list_borrowers permission in bug 30230

We should use this bug to clean up the old permissions on the endpoint.

Just some scaffolding to show what I'm planning here.

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

(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.

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.)

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.

> 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.

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

Created attachment 162580 [details] [review]
Bug 29509: Correction to bitwise check

I was using it wrong :(

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.

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>

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>

(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

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 :)

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.

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?

(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.

Created attachment 166280 [details] [review]
Bug 29509: (QA follow-up) Check top level permissions too

It's beyond my brain to work out how to accomplish this in fewer SQL queries.. but I have resolved the lack of top level group permissions checking.

Created attachment 169235 [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 :)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 169236 [details] [review]
Bug 29509: (QA follow-up) Check top level permissions too

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Created attachment 169237 [details] [review]
Bug 29509: (QA follow-up) Tidy atomic update

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Thanks for all the hard work!

Pushed to main for the next 24.11.00 release as RM Assistant

Backported to 24.05.x for upcoming 24.05.04

Pushed to 23.11.x for 23.11.09

Not backporting to 23.05.x unless requested