Bug 37392 - Edit item permission by library group is broken
Summary: Edit item permission by library group is broken
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Nick Clemens (kidclamp)
QA Contact: Pedro Amorim
URL:
Keywords:
Depends on: 37378 37383 29523
Blocks:
  Show dependency treegraph
 
Reported: 2024-07-17 11:54 UTC by Katrin Fischer
Modified: 2024-10-16 16:20 UTC (History)
11 users (show)

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


Attachments
Bug 37392: Unit test (5.97 KB, patch)
2024-07-23 15:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37392: can_see_things_from is always checking patron visibility (2.47 KB, patch)
2024-07-23 15:42 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37392: Unit test (5.97 KB, patch)
2024-07-24 19:21 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37392: can_see_things_from is always checking patron visibility (2.47 KB, patch)
2024-07-24 19:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37392: Adjust routines (5.68 KB, patch)
2024-07-24 19:22 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37392: Unit test (6.03 KB, patch)
2024-08-15 14:41 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 37392: can_see_things_from is always checking patron visibility (2.52 KB, patch)
2024-08-15 14:41 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 37392: Adjust routines (5.73 KB, patch)
2024-08-15 14:41 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 37392: Unit test (6.08 KB, patch)
2024-10-11 14:58 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37392: can_see_things_from is always checking patron visibility (2.57 KB, patch)
2024-10-11 14:58 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 37392: Adjust routines (5.78 KB, patch)
2024-10-11 14:58 UTC, Brendan Lawlor
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2024-07-17 11:54:27 UTC
The library groups allow to set up library groups within the staff users can edit each others items. This was introduced in 23.05, but is now broken in 23.11.

It's not possible for a staff user to edit another libraries items now in some circumstances.

Test plan:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A 
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.   
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.


-----
After some digging to understand the change of behavior noticed between 23.05 and 23.11:

I believe commit 2c502d32 to be the culprit.
item.can_be_edit uses Patron::can_edit_item_from which calls
can_see_things_from.

The commit removed the library group specific code and introduced a call to libraries_where_can_see_patrons instead, probably assuming the thing was always a patron record. But here it is an item record.

Thinking we should not use "thing" in method names without at least explaining what thing can be...
Comment 1 Lucas Gass (lukeg) 2024-07-22 23:48:36 UTC
Have a look at my patch on Bug 37383, I hope that it solves both this problem and the one reported there.
Comment 2 Nick Clemens (kidclamp) 2024-07-23 15:42:06 UTC
Created attachment 169433 [details] [review]
Bug 37392: Unit test
Comment 3 Nick Clemens (kidclamp) 2024-07-23 15:42:09 UTC
Created attachment 169434 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.
Comment 4 Nick Clemens (kidclamp) 2024-07-23 15:43:34 UTC
(In reply to Lucas Gass from comment #1)
> Have a look at my patch on Bug 37383, I hope that it solves both this
> problem and the one reported there.

This does solve the problem and we should move that forward.

We should also address here the incorrect call in "can_see_things_from"

I have tried ot test and fix this, but I don't understand the groups well enough to write a test that fails befroe and passes after, though I am fairley certain the actual code change is right.
Comment 5 Kyle M Hall (khall) 2024-07-23 17:03:37 UTC
libraries_where_can_see_things has the line

my $library_groups = $self->library->library_groups({ $group_feature => 1 });

but library_groups() does not take a parameter, it always returns all related library groups, not filtered by feature. All attempts to fix this issue result in different unit test failures.

It seems like replacing all uses of libraries_where_can_see_things with the more specific counterpart is the best solution. Then we can remove that bad code altogether and have more DRY code.
Comment 6 Nick Clemens (kidclamp) 2024-07-24 19:21:58 UTC
Created attachment 169515 [details] [review]
Bug 37392: Unit test
Comment 7 Nick Clemens (kidclamp) 2024-07-24 19:22:00 UTC
Created attachment 169516 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.
Comment 8 Nick Clemens (kidclamp) 2024-07-24 19:22:03 UTC
Created attachment 169517 [details] [review]
Bug 37392: Adjust routines

The current code only handled a single layer of groups - top level setting the features, and libraries directly
underneath.

The code, however, was not correctly checking the features, and was limiting to single like when no restrictions found.

This patch gets the root ancestor for a group, checks the desired feature against than group, then fetches all children
of the current group and makes them allowed - i.e. when a library is in a group, all siblings and descendants in that group
or subgroups can be accessed

I adjust some typos in the tests too, this needs mroe cleanup, but am submitting for any discussion
Comment 9 Katrin Fischer 2024-07-29 11:17:49 UTC
Hi Nick, how can we help to move this forward?
Comment 10 Nick Clemens (kidclamp) 2024-07-30 12:57:46 UTC
(In reply to Katrin Fischer from comment #9)
> Hi Nick, how can we help to move this forward?

Test and confirm that it works as described, and that you agree with the functionality. If so, I can write the additional tests needed
Comment 11 Katrin Fischer 2024-07-31 11:52:24 UTC
Nick, can you explain a bit how this relates to bug 37383?
Comment 12 Nick Clemens (kidclamp) 2024-07-31 12:50:27 UTC
(In reply to Katrin Fischer from comment #11)
> Nick, can you explain a bit how this relates to bug 37383?

That moves from using the "can_edit_items_from" routine to "libraries_where_can_edit_items"

It makes it closer to correct, but here I found the underlying routines still had inaccuracies.

If you have only groups that limit editing it would all work, which is why the test plan is correct, but as Kyle notes here - the parameter to limit by group type isn't actually effective - that patches here aim to fix that
Comment 13 Katrin Fischer 2024-07-31 13:37:14 UTC
So we would want multiple overlapping groups for testing with a mix of edit items and patron permissions?
Comment 14 Nick Clemens (kidclamp) 2024-07-31 13:48:04 UTC
(In reply to Katrin Fischer from comment #13)
> So we would want multiple overlapping groups for testing with a mix of edit
> items and patron permissions?

Yes, sorry, I thought I had posted more here, but it seems I did not :-)

I think ideally you would setup

Top level group
Consortia - set to limit both patrons and item edition

This would have a subgroup:
East libraries - With Centerville and Fairfield
East Hill libraries - With Midway and IPT
East Lake Libraries - With Fairview and Centervile

Each library should be limited to their siblings and descendants, so:
Centerville and Fairfield can see all the libraries in the group
Midway and IPT can only see each other
Fairview can see themselves and Centerville
Comment 15 Nick Clemens (kidclamp) 2024-07-31 13:48:51 UTC
(In reply to Nick Clemens (kidclamp) from comment #14)
> (In reply to Katrin Fischer from comment #13)
> > So we would want multiple overlapping groups for testing with a mix of edit
> > items and patron permissions?
> 
> Yes, sorry, I thought I had posted more here, but it seems I did not :-)
> 
> I think ideally you would setup
> 
> Top level group
> Consortia - set to limit both patrons and item edition
> 
> This would have a subgroup:
> East libraries - With Centerville and Fairfield
> East Hill libraries - With Midway and IPT
> East Lake Libraries - With Fairview and Centervile
> 
> Each library should be limited to their siblings and descendants, so:
> Centerville and Fairfield can see all the libraries in the group
> Midway and IPT can only see each other
> Fairview can see themselves and Centerville

Then test with only one limit enabled
Then make a second group with the other limit enabled and choose different libraries
Comment 16 Katrin Fischer 2024-08-15 09:47:49 UTC
Thanks Nick, I think that cleared up things. This is still on my list, but I am not sure when I will make time to help with testing. I'd love if someone else could help here. Maybe you are aware of someone else using this feature?
Comment 17 Katrin Fischer 2024-08-15 09:48:01 UTC
Should we switch to NSO now?
Comment 18 Lucas Gass (lukeg) 2024-08-15 13:46:24 UTC
(In reply to Katrin Fischer from comment #17)
> Should we switch to NSO now?

I think, yes.
Comment 19 Biblibre Sandboxes 2024-08-15 14:41:51 UTC
Created attachment 170392 [details] [review]
Bug 37392: Unit test

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 20 Biblibre Sandboxes 2024-08-15 14:41:54 UTC
Created attachment 170393 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 21 Biblibre Sandboxes 2024-08-15 14:41:56 UTC
Created attachment 170394 [details] [review]
Bug 37392: Adjust routines

The current code only handled a single layer of groups - top level setting the features, and libraries directly
underneath.

The code, however, was not correctly checking the features, and was limiting to single like when no restrictions found.

This patch gets the root ancestor for a group, checks the desired feature against than group, then fetches all children
of the current group and makes them allowed - i.e. when a library is in a group, all siblings and descendants in that group
or subgroups can be accessed

I adjust some typos in the tests too, this needs mroe cleanup, but am submitting for any discussion

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Comment 22 Marcel de Rooy 2024-08-16 09:12:53 UTC
 WARN   Koha/Patron.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 294, now: 296)

 WARN   t/db_dependent/Koha/Patrons.t
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 675, now: 691)
Comment 23 Lucas Gass (lukeg) 2024-08-16 13:42:59 UTC
(In reply to Marcel de Rooy from comment #22)
>  WARN   Koha/Patron.pm
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 294, now: 296)
> 
>  WARN   t/db_dependent/Koha/Patrons.t
>    WARN   tidiness
>                 The file is less tidy than before (bad/messy lines before:
> 675, now: 691)

Nick's patch does not touch these lines. Should we be tidying unrelated code? In my opinion, no.
Comment 24 Pedro Amorim 2024-08-16 16:15:28 UTC
Taking this bug for QA, assigning it to me to prevent others from also looking and possibly waste mine or their time.
I've taken a look at the code and I'm having a hard time following what's happening here.
I've left Nick a message to discuss this with him and hopefully bring me up to speed so that I can properly QA this or ultimately pass it to someone else if I'm unable to.
Comment 25 Marcel de Rooy 2024-09-20 09:16:34 UTC
+subtest
+    'libraries_where_can_see_patrons + libraries_where_can_see_things + can_see_patron_infos + search_limited+ can_see_patrons_from + can_edit_items_from'
+    => sub {

Hmm
Comment 26 Marcel de Rooy 2024-09-20 09:18:16 UTC
(In reply to Lucas Gass from comment #23)
> (In reply to Marcel de Rooy from comment #22)
> >  WARN   Koha/Patron.pm
> >    WARN   tidiness
> >                 The file is less tidy than before (bad/messy lines before:
> > 294, now: 296)
> > 
> >  WARN   t/db_dependent/Koha/Patrons.t
> >    WARN   tidiness
> >                 The file is less tidy than before (bad/messy lines before:
> > 675, now: 691)
> 
> Nick's patch does not touch these lines. Should we be tidying unrelated
> code? In my opinion, no.

The changed lines somehow cause an different outcome in untidy lines. So yes, it needs attention. Sorry.
Comment 27 Marcel de Rooy 2024-09-20 09:21:17 UTC
(In reply to Pedro Amorim from comment #24)
> Taking this bug for QA, assigning it to me to prevent others from also
> looking and possibly waste mine or their time.
> I've taken a look at the code and I'm having a hard time following what's
> happening here.
> I've left Nick a message to discuss this with him and hopefully bring me up
> to speed so that I can properly QA this or ultimately pass it to someone
> else if I'm unable to.

Sure, changing the status for now.
Comment 28 Pedro Amorim 2024-09-20 09:30:37 UTC
@Marcel I have not yet had the chance to pick this up again, and I failed to understand it well enough to do proper QA the first time. You are certainly more capable than me. If you're available here, I'm happy to swap places with you as QA Contact.
Comment 29 Katrin Fischer 2024-09-25 15:36:43 UTC
(In reply to Marcel de Rooy from comment #25)
> +subtest
> +    'libraries_where_can_see_patrons + libraries_where_can_see_things +
> can_see_patron_infos + search_limited+ can_see_patrons_from +
> can_edit_items_from'
> +    => sub {
> 
> Hmm

Can you explain or was this just a personal note for later?

Is this just failed for tidiness or is there another issue?
Comment 30 Marcel de Rooy 2024-09-26 05:38:42 UTC
(In reply to Katrin Fischer from comment #29)
> (In reply to Marcel de Rooy from comment #25)
> > +subtest
> > +    'libraries_where_can_see_patrons + libraries_where_can_see_things +
> > can_see_patron_infos + search_limited+ can_see_patrons_from +
> > can_edit_items_from'
> > +    => sub {
> > 
> > Hmm
> 
> Can you explain or was this just a personal note for later?
This is about subtest naming :) Thats just crazy.

> Is this just failed for tidiness or is there another issue?
Comment27/28 explains that. I will change to SO again in view of comment28.
Comment 31 Brendan Lawlor 2024-10-11 14:58:52 UTC
Created attachment 172698 [details] [review]
Bug 37392: Unit test

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 32 Brendan Lawlor 2024-10-11 14:58:54 UTC
Created attachment 172699 [details] [review]
Bug 37392: can_see_things_from is always checking patron visibility

When trying to edit items, we are seeing the button visibility affected by a patron's
permission to view patrons form any library.

This is because can_edit_items_from is calling can_see_things_from - which is ultimately calling
libraries_where_can_see_patrons

That last call should be to libraries_where_can_see_things. This patch corrects that, and passes forward the group feature to check against

To test:

Set up library group:
* Create a library group for library A + B
* Action: Limit item editing by group

Set up test user:
* Create a staff patron with these permissions:
  * catalogue
  * fast_cataloguing
  * edit_items
  * view_borrower_infos_from_any_libraries
  * edit borrowers
* Home library: library A

Set up test items:
* Create a record with 3 items with different home libraries:
  * A
  * B
  * C

We expect the user will be allowed to edit A and B, but not C.

Test:
* Test editing the items with the test user, only A is allowed to be edited.
* Remove the view_borrower_infos_from_any_libraries permission from test user.
* Test editing items now behaves as expected: A + B are allowed, C is not.

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 33 Brendan Lawlor 2024-10-11 14:58:56 UTC
Created attachment 172700 [details] [review]
Bug 37392: Adjust routines

The current code only handled a single layer of groups - top level setting the features, and libraries directly
underneath.

The code, however, was not correctly checking the features, and was limiting to single like when no restrictions found.

This patch gets the root ancestor for a group, checks the desired feature against than group, then fetches all children
of the current group and makes them allowed - i.e. when a library is in a group, all siblings and descendants in that group
or subgroups can be accessed

I adjust some typos in the tests too, this needs mroe cleanup, but am submitting for any discussion

Signed-off-by: Michaela Sieber <michaela.sieber@kit.edu>
Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 34 Brendan Lawlor 2024-10-11 15:05:45 UTC
I tested this for our consortium's use case where libraries with branches need to be able to edit their fellow branches items.

Thanks to everyone working on this bug. It is a really important permission for consortia. Hopefully it can be backported to 24.05.