Bug 30178 - Every librarian can edit every item with IndependentBranches on
Summary: Every librarian can edit every item with IndependentBranches on
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P2 major (vote)
Assignee: Janusz Kaczmarek
QA Contact: Testopia
URL:
Keywords:
Depends on: 27526 28445
Blocks:
  Show dependency treegraph
 
Reported: 2022-02-24 20:42 UTC by Janusz Kaczmarek
Modified: 2022-03-20 15:41 UTC (History)
5 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:
22.05.00,21.11.04


Attachments
[SOLVED] Every librarian can edit every item with IndependentBranches on (1.98 KB, patch)
2022-02-24 20:55 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on (4.95 KB, patch)
2022-02-25 14:37 UTC, Janusz Kaczmarek
Details | Diff | Splinter Review
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on (5.00 KB, patch)
2022-02-26 16:14 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on (5.06 KB, patch)
2022-03-01 14:55 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 Janusz Kaczmarek 2022-02-24 20:42:00 UTC
Problem arises after applying bugfix #27526: with IndependentBranches on, a librarian without superlibrarian privileges, can edit every item (i.e. also from a foreign branch).  This is because can_be_edited calculation is buggy.  A patch will follow.
Comment 1 Janusz Kaczmarek 2022-02-24 20:55:11 UTC
Created attachment 131090 [details] [review]
[SOLVED] Every librarian can edit every item with  IndependentBranches on

Test plan:
=========
1. Have (at least) two branches defined.
2. Have IndependentBranches set.
3. Have a biblio record with items belonging to different branches.
4. Be a librarian without superlibrarian rights.
5. Go to Edit -> Edit items view (cataloguing/additems.pl).
6. You will be able to edit every item, also ones not from the branch
   you are from.
7. Apply the patch.
8. Repeat 5.
9. You should be able to edit only the items from your branch.
Comment 2 Fridolin Somers 2022-02-25 00:58:36 UTC
Thanks for this patch.

You also need to fix :
koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc

Please have a look at how we write commit/patch messages :
https://wiki.koha-community.org/wiki/Commit_messages
Comment 3 Janusz Kaczmarek 2022-02-25 14:37:00 UTC
Created attachment 131123 [details] [review]
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on

Additionally, problems caused by koha-tmpl/intranet-tmpl/prog/en/includes/html_helpers.inc are resolved.  

Fridolin, thank you for pointing that out.
Comment 4 Katrin Fischer 2022-02-26 15:54:54 UTC
This changes sub GetLoggedInBranchcode to GetLoggedInBranchname, but doesn't this mean that GetLoggedInBranchcode is wrong?

But this doesn't feel right as we compare to item.homebranch, which should be the code? Something here is for sure confusing.
Comment 5 Janusz Kaczmarek 2022-02-26 16:06:14 UTC
Katrin, you are right, it is confusing.  I wish it was a branch code, but it is not--it is the name.  Have a look at columns_to_str here (cataloguing/additem.pl):

my @items;
for my $item ( $biblio->items->as_list, $biblio->host_items->as_list ) {
    push @items, $item->columns_to_str;
}

It took me some time to figure this out.  The values of item.homebranch are then put directly into the table for users, so--I guess--it has been decided to pass this like this...
Comment 6 Katrin Fischer 2022-02-26 16:09:03 UTC
(In reply to Janusz Kaczmarek from comment #5)
> Katrin, you are right, it is confusing.  I wish it was a branch code, but it
> is not--it is the name.  Have a look at columns_to_str here
> (cataloguing/additem.pl):
> 
> my @items;
> for my $item ( $biblio->items->as_list, $biblio->host_items->as_list ) {
>     push @items, $item->columns_to_str;
> }
> 
> It took me some time to figure this out.  The values of item.homebranch are
> then put directly into the table for users, so--I guess--it has been decided
> to pass this like this...

We just arrived at the same spot :) I first confirmed it by adding the line to the templates:

[% item.homebranch %] | [% Branches.GetLoggedInBranchname() %]

So the fix is correct, but I feel we might want to revisit this confusing variable at some point. Changing code to name is an easy template task, so passing it like this appears not necessary.
Comment 7 Katrin Fischer 2022-02-26 16:09:37 UTC
And great commit message now btw!
Comment 8 Janusz Kaczmarek 2022-02-26 16:13:39 UTC
One could add for testing a line like this:

[% Koha.Preference('IndependentBranches') %] // [% logged_in_user.is_superlibrarian %] // [% item.homebranch %] // [% Branches.GetLoggedInBranchname() ) %] // [% Branches.GetLoggedInBranchcode() ) %] </br>

next to "SET can_be_edited" to see the actual values on the web page...
Comment 9 Katrin Fischer 2022-02-26 16:14:21 UTC
Created attachment 131137 [details] [review]
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on

Problem arises after applying bugfix 27526 and 28445: with
IndependentBranches on, a librarian without superlibrarian privileges,
can edit (and potentially delete) every item (i.e. also from a foreign
branch).  This is because can_be_edited calculation is buggy (in two spots).

Test plan:
1. Have (at least) two branches defined.
2. Have IndependentBranches set.
3. Have a biblio record with items belonging to different branches.
4. Be a librarian without superlibrarian rights, with editcatalogue
   and tool permissions set.

Scenario A (Edit items):
1. Go to Edit -> Edit items view (cataloguing/additems.pl).
2. You will be able to edit every item, also ones not from the branch
   you are from (cf. the button 'Actions').
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch.

Scenario B (Edit items in batch):
1. From Normal view go to Edit -> Edit items in batch.
2. You will be able to batch edit every item, also ones not from the branch
   you are from.
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch (and
   see 'Cannot edit' for others.

Scenario C (Delete items in batch):
1. From Normal view go to Edit -> Delete items in batch.
2. You will not see the string 'Cannot delete' and only by chance
   will not be able to activate the checkboxes next to foreign items.
3. Apply the patch.
4. Repeat 1.
5. You should be able to delete only the items from your branch (and
   see 'Cannot delete' for others.

Scenario D and E:
Analogous steps can be executed from Tools -> Batch item modification
and Tools -> Batch item deletion

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 10 Janusz Kaczmarek 2022-02-26 17:40:28 UTC
> We just arrived at the same spot :) I first confirmed it by adding the line
> to the templates:
> 
> [% item.homebranch %] | [% Branches.GetLoggedInBranchname() %]
> 
> So the fix is correct, but I feel we might want to revisit this confusing
> variable at some point. Changing code to name is an easy template task, so
> passing it like this appears not necessary.

The more that the use of GetLoggedInBranchname creates a possible security issue in a malignant case of two different branches with the same name.
Comment 11 Fridolin Somers 2022-02-28 06:30:17 UTC
Maybe we should create a Koha::Patron method can_edit_item(), like can_log_into()
Comment 12 Katrin Fischer 2022-02-28 11:08:51 UTC
(In reply to Fridolin Somers from comment #11)
> Maybe we should create a Koha::Patron method can_edit_item(), like
> can_log_into()

I had similar thoughts - but I think we could treat this as an urgent bug fix maybe and I'll file another bug for centralizing the code? I believe it would make sense, but might create a bigger/more conflict-prone patch set if done right.
Comment 13 Fridolin Somers 2022-03-01 09:40:35 UTC
Sure, another bug report for long-term fix is good.
Comment 14 Martin Renvoize 2022-03-01 14:55:50 UTC
Created attachment 131182 [details] [review]
Bug 30178: (bugs 27526 and 28445 follow-up) Every librarian can edit every item with IndependentBranches on

Problem arises after applying bugfix 27526 and 28445: with
IndependentBranches on, a librarian without superlibrarian privileges,
can edit (and potentially delete) every item (i.e. also from a foreign
branch).  This is because can_be_edited calculation is buggy (in two spots).

Test plan:
1. Have (at least) two branches defined.
2. Have IndependentBranches set.
3. Have a biblio record with items belonging to different branches.
4. Be a librarian without superlibrarian rights, with editcatalogue
   and tool permissions set.

Scenario A (Edit items):
1. Go to Edit -> Edit items view (cataloguing/additems.pl).
2. You will be able to edit every item, also ones not from the branch
   you are from (cf. the button 'Actions').
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch.

Scenario B (Edit items in batch):
1. From Normal view go to Edit -> Edit items in batch.
2. You will be able to batch edit every item, also ones not from the branch
   you are from.
3. Apply the patch.
4. Repeat 1.
5. You should be able to edit only the items from your branch (and
   see 'Cannot edit' for others.

Scenario C (Delete items in batch):
1. From Normal view go to Edit -> Delete items in batch.
2. You will not see the string 'Cannot delete' and only by chance
   will not be able to activate the checkboxes next to foreign items.
3. Apply the patch.
4. Repeat 1.
5. You should be able to delete only the items from your branch (and
   see 'Cannot delete' for others.

Scenario D and E:
Analogous steps can be executed from Tools -> Batch item modification
and Tools -> Batch item deletion

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2022-03-01 14:57:20 UTC
Well sleuthed, this was hard to trace.

I've confirmed the fix works and I can't find any regressions. Lets get this one pushed as a matter of urgency and continue tidying up in a further bug.. I'll leave Katrin and Fridolin to report that new bug.

So.. Passing QA, thanks for the patch Janusz
Comment 16 Fridolin Somers 2022-03-04 02:12:41 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 17 Andrew Fuerste-Henry 2022-03-20 15:41:21 UTC
I see this is in 21.11.
Missing dependency, not backported to 21.05