Bug 16231 - Correct permission handling in subscription edit menu
Summary: Correct permission handling in subscription edit menu
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Serials (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Owen Leonard
QA Contact: Testopia
URL: /cgi-bin/koha/serials/subscription-de...
Keywords:
Depends on:
Blocks:
 
Reported: 2016-04-08 18:22 UTC by Owen Leonard
Modified: 2019-06-06 17:53 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16231 - Correct permission handling in subscription edit menu (6.03 KB, patch)
2016-04-08 18:47 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16231 - Correct permission handling in subscription edit menu (6.16 KB, patch)
2016-04-13 13:33 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 16231 [Follow-up] Correct permission handling in subscription edit menu (2.06 KB, patch)
2016-04-25 14:31 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16231 [Follow-up] Correct permission handling in subscription edit menu (2.07 KB, patch)
2016-04-26 08:58 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 16231: Correct permission handling in subscription edit (4.84 KB, patch)
2018-10-03 16:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16231: Correct permission handling in subscription edit (4.83 KB, patch)
2018-11-14 13:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 16231: Correct permission handling in subscription edit (4.89 KB, patch)
2019-01-11 22:47 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 16231: Correct permission handling in subscription edit (4.96 KB, patch)
2019-02-03 10:44 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2016-04-08 18:22:48 UTC
The "Edit" toolbar menu for serials contains three items, "Edit subscription," "Edit as new (duplicate)" and "Delete subscription." Each item should be wrapped in its own check for the "CAN_user_serials_*" permission.
Comment 1 Owen Leonard 2016-04-08 18:47:26 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-04-13 13:33:00 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2016-04-13 22:53:43 UTC
Comment on attachment 50195 [details] [review]
Bug 16231 - Correct permission handling in subscription edit menu

Review of attachment 50195 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/includes/serials-toolbar.inc
@@ -50,4 @@
>                  <div class="btn-group">
>                      <button class="btn btn-small dropdown-toggle" data-toggle="dropdown"><i class="fa fa-pencil"></i> Edit <span class="caret"></span></button>
>                          <ul class="dropdown-menu">
> -                            [% IF ( cannotedit ) %] <li class="disabled"> [% ELSE %]

If you follow 'cannotedit' through the code, you can see that the permission check is quite a complicated one here:
sub _can_do_on_subscription 

It also checks superserials and IndependentBranches.
Comment 4 Owen Leonard 2016-04-14 18:33:07 UTC
(In reply to Katrin Fischer from comment #3)

> If you follow 'cannotedit' through the code

That whole menu is wrapped in "[% UNLESS ( cannotedit ) %]," so I concluded that the checks on "cannotedit" inside the menu were redundant. Does that sound correct?
Comment 5 Owen Leonard 2016-04-20 14:43:17 UTC
I'm returning this to "signed off" because I think the QA issue is actually okay. I hope someone can take another look and let me know.
Comment 6 Katrin Fischer 2016-04-20 14:44:09 UTC
Thx Owen, sorry I didn't come back to this yet.
Comment 7 Katrin Fischer 2016-04-25 11:08:26 UTC
Hi Owen, started to test the permissions - especially looking at:

- edit_subscription
- delete_subscription
- create_subscription

When the user has only delete_subscription out of those three it appears that the option is not visible. Could you take a look please?
Comment 8 Owen Leonard 2016-04-25 14:31:01 UTC Comment hidden (obsolete)
Comment 9 Chris Cormack 2016-04-26 08:58:29 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-04-26 18:08:37 UTC
(In reply to Owen Leonard from comment #8)
> This follow-up allows for the case where the user has only
> "delete_subscription" permission. The delete option might be
> available even if the template's "cannotedit" variable is true.

I don't understand why, could you please detail? I have not tested the patch, but looking at the diff it seems that it's a behavior change.
Comment 11 Katrin Fischer 2016-05-08 12:01:39 UTC
Maybe my first test here was flawed. This is something I see retesting this:

- Set independentbranches to prevent
- Make sure staff patron doesn't have superserials permission
= I should not be allowed to change serials from other branches
- Go to another branches subscription (by URL manipulation)
- With the patches I can: renew (opens wrong window?), receive (ew!) and DELETE
- Without the patch on master, I can't delete (better)

I'd say this is a change of the behaviour that we don't want - allowing deletions. But overall... permission handling in serials could still be improved a lot.

Can we find a good fix for delete here and then see about the other problems later?
Comment 12 Katrin Fischer 2016-05-08 12:04:27 UTC
I think if you don't have edit_subscription, but delete an 'cannotedit = 0 (can edit)' - then maybe show a single 'delete' button?
Comment 13 Owen Leonard 2018-10-03 16:12:56 UTC Comment hidden (obsolete)
Comment 14 Owen Leonard 2018-10-03 16:14:24 UTC
As far as I can tell you cannot delete a subscription without "edit" permission, despite the separate permissions. I think it has to do with out the "cannotedit" variable is defined.
Comment 15 Owen Leonard 2018-11-14 13:53:56 UTC Comment hidden (obsolete)
Comment 16 Charles Farmer 2019-01-11 22:47:42 UTC
Created attachment 83833 [details] [review]
Bug 16231: Correct permission handling in subscription edit

This patch corrects the way subscription edit menu items are displayed
according to the user's permissions settings.

To test, apply the patch and log in to the staff client as a user who
has permission to create, edit, and delete subscriptions.

- Locate an subscription and view the details for it.
- Confirm that each "Edit" menu item works correctly.
- Repeat the process when logged in as a user with varying combinations
  of create, edit, and delete permissions.
- Test as a user limited by IndependentBranches.

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 17 Katrin Fischer 2019-02-03 10:44:08 UTC
Created attachment 84680 [details] [review]
Bug 16231: Correct permission handling in subscription edit

This patch corrects the way subscription edit menu items are displayed
according to the user's permissions settings.

To test, apply the patch and log in to the staff client as a user who
has permission to create, edit, and delete subscriptions.

- Locate an subscription and view the details for it.
- Confirm that each "Edit" menu item works correctly.
- Repeat the process when logged in as a user with varying combinations
  of create, edit, and delete permissions.
- Test as a user limited by IndependentBranches.

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 18 Nick Clemens 2019-02-04 17:42:16 UTC
Awesome work all!

Pushed to master for 19.05
Comment 19 Martin Renvoize 2019-02-11 13:41:51 UTC
Pushed to 18.11.x for 18.11.03
Comment 20 Lucas Gass 2019-02-12 20:38:48 UTC
backported to 18.05.x for 18.05.09
Comment 21 Jonathan Druart 2019-02-16 19:24:45 UTC
(In reply to Owen Leonard from comment #14)
> As far as I can tell you cannot delete a subscription without "edit"
> permission, despite the separate permissions. I think it has to do with out
> the "cannotedit" variable is defined.

Not sure this is correct, in subscription-detail.pl:
 52 # Permission needed if it is a deletion (del) : delete_subscription
 53 # Permission needed otherwise : *
 54 my $permission = ($op eq "del") ? "delete_subscription" : "*";

Not a big deal I'd say, but maybe a minor bug (?)