Bug 15520 - Add more granular permission for only editing own library's circ rules
Summary: Add more granular permission for only editing own library's circ rules
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 15522
  Show dependency treegraph
 
Reported: 2016-01-07 21:39 UTC by Jesse Weaver
Modified: 2018-10-15 13:42 UTC (History)
14 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 15520: Add permission to restrict circ rules editing to own library (8.15 KB, patch)
2017-09-01 03:03 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (8.22 KB, patch)
2017-09-17 04:34 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (8.25 KB, patch)
2018-01-09 18:57 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (8.41 KB, patch)
2018-02-26 19:33 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15520: (QA follow-up) Add tests, missing installer data (2.69 KB, patch)
2018-02-26 19:33 UTC, Jesse Weaver
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (7.73 KB, patch)
2018-07-23 22:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries (4.49 KB, patch)
2018-07-23 22:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (7.45 KB, patch)
2018-07-24 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries (4.45 KB, patch)
2018-07-24 14:26 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (7.46 KB, patch)
2018-08-29 17:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries (4.45 KB, patch)
2018-08-29 17:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (7.45 KB, patch)
2018-09-14 19:32 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries (4.45 KB, patch)
2018-09-14 19:32 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (7.42 KB, patch)
2018-09-14 21:24 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries (4.48 KB, patch)
2018-09-14 21:24 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 15520: Add permission to restrict circ rules editing to own library (7.47 KB, patch)
2018-10-01 20:17 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries (4.53 KB, patch)
2018-10-01 20:17 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 15520: (QA follow-up) Fix renaming of circ rules permission (1.28 KB, patch)
2018-10-01 20:17 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jesse Weaver 2016-01-07 21:39:45 UTC
In large library systems, it can be inconvenient to allow a staff member to edit circulation rules for all libraries. A permission that allows editing only circ rules for the staff member's home library would help.
Comment 1 Jesse Weaver 2017-09-01 03:03:25 UTC Comment hidden (obsolete)
Comment 2 Alex Buckley 2017-09-17 04:34:58 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2018-01-09 18:57:50 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2018-01-09 18:58:46 UTC
I have fixed an easy conflict with commit 971591d4ee8576d6782d32610bbae7cfe4674231
    Bug 19029: Add JavaScript security question for cloning circ rules

-                <input type="submit" value="Clone" />
+                <input type="submit" id="clone_rules" value="Clone" />
Comment 5 Owen Leonard 2018-02-01 15:34:10 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2018-02-01 15:39:41 UTC
Never mind, my db update didn't run correctly. This looks like it's (still) working correctly.
Comment 7 Josef Moravec 2018-02-05 09:12:17 UTC
Jesse, this is working good, but I have found some small issues though:

The new permission option is added only for existing installation, we need this in installer data too.

Would be good to have test for the new functionality of haspermission sub.
Comment 8 Jesse Weaver 2018-02-26 19:33:38 UTC
Created attachment 72240 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

Test plan:

1) Ensure that you have four users:
 a) A superlibrarian
 b) A user with all `parameters` permissions (the toplevel `parameters`
    box is checked).
 c) A user with the `manage_circ_rules` permission (and, of course,
    `catalogue`).
 d) A user with the `manage_circ_rules`, `manage_circ_rules_restricted`
    and `catalogue` permissions.
2) As all four users, load the "Circulation and fine rules"
   administration page (admin/smart-rules.pl).
3) The page should be unchanged for the first three users. It should be
   possible to view and edit the circ rules for all libraries.
4) The last (restricted) user should only be able to view and edit the
   circ rules for their own library.

Followed test plan, patch worked as described
Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 9 Jesse Weaver 2018-02-26 19:33:41 UTC
Created attachment 72241 [details] [review]
Bug 15520: (QA follow-up) Add tests, missing installer data
Comment 10 Josef Moravec 2018-04-04 13:07:10 UTC
Comment on attachment 72240 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

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

::: admin/smart-rules.pl
@@ +63,5 @@
> +
> +my $uid = Koha::Patrons->find( $loggedinuser )->userid;
> +my $restricted_to_own_library = $uid && haspermission( $uid, { parameters => 'manage_circ_rules_restricted' }, { no_inherit => 1 } );
> +$template->param( restricted_to_own_library => $restricted_to_own_library );
> +$branch = C4::Context::mybranch() if $restricted_to_own_library;

This is not the logged in user's home library, it is the library where the user is logged in now - so when user change library, he/she could change another's library rules!
Comment 11 Katrin Fischer 2018-04-04 15:17:41 UTC
But couldn't they change the home library in their records too? 
I think in general we got a problem there, that you can get around the limits, maybe the logged-in library is more helpful?
Comment 12 Nick Clemens 2018-05-22 13:21:20 UTC
(In reply to Josef Moravec from comment #10)
> This is not the logged in user's home library, it is the library where the
> user is logged in now - so when user change library, he/she could change
> another's library rules!

I (In reply to Katrin Fischer from comment #11)
> But couldn't they change the home library in their records too? 
> I think in general we got a problem there, that you can get around the
> limits, maybe the logged-in library is more helpful?

I think this should be the user's logged in library - for extra protection I think the library would implement AutoLocation and limit logged in site by ip so this would require some extra steps but would also be a first level guard against accidentally altering other rules
Comment 13 Katrin Fischer 2018-05-22 18:01:04 UTC
> I (In reply to Katrin Fischer from comment #11)
> > But couldn't they change the home library in their records too? 
> > I think in general we got a problem there, that you can get around the
> > limits, maybe the logged-in library is more helpful?
> 
> I think this should be the user's logged in library - for extra protection I
> think the library would implement AutoLocation and limit logged in site by
> ip so this would require some extra steps but would also be a first level
> guard against accidentally altering other rules

A lot of the public and smaller libraries here don't have static IP addresses, so AutoLocation is not good. But it's a problem we should solve on this bug report! I think the new permission adds an extra barrier as a good first step and it will prevent people from accidentally editing the wrong library's rules.
Comment 14 Katrin Fischer 2018-05-23 12:41:24 UTC
it's NOT a problem we should solve here... (sorry, typo)
Comment 15 Jonathan Druart 2018-05-28 20:19:31 UTC
Yes, that would work, but it smells a bit :)

For instance a superlibrarian user will have CAN_manage_circ_rules_restricted set in templates.

Why not simply do the reverse, like view_borrower_infos_from_any_libraries, and name it manage_circ_rules_from_any_libraries (will need to be set on update for patrons with manage_circ_rules).
It would remove the new parameter and avoid a permission to restrict, which could add side-effects (maybe not now, but certainly later).
Comment 16 Josef Moravec 2018-05-30 13:20:48 UTC
(In reply to Jonathan Druart from comment #15)
> Yes, that would work, but it smells a bit :)
> 
> For instance a superlibrarian user will have
> CAN_manage_circ_rules_restricted set in templates.
> 
> Why not simply do the reverse, like view_borrower_infos_from_any_libraries,
> and name it manage_circ_rules_from_any_libraries (will need to be set on
> update for patrons with manage_circ_rules).
> It would remove the new parameter and avoid a permission to restrict, which
> could add side-effects (maybe not now, but certainly later).

It does make more sense to me
Comment 17 Tomás Cohen Arazi 2018-05-30 16:14:36 UTC
(In reply to Jonathan Druart from comment #15)
> Yes, that would work, but it smells a bit :)
> 
> For instance a superlibrarian user will have
> CAN_manage_circ_rules_restricted set in templates.
> 
> Why not simply do the reverse, like view_borrower_infos_from_any_libraries,
> and name it manage_circ_rules_from_any_libraries (will need to be set on
> update for patrons with manage_circ_rules).
> It would remove the new parameter and avoid a permission to restrict, which
> could add side-effects (maybe not now, but certainly later).

I agree with Jonathan's proposal. We can remove this later once the permissions system is based on the library hierarchies :-D
Comment 18 Jonathan Druart 2018-07-23 22:15:35 UTC
Created attachment 77202 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

Test plan:

1) Ensure that you have four users:
 a) A superlibrarian
 b) A user with all `parameters` permissions (the toplevel `parameters`
    box is checked).
 c) A user with the `manage_circ_rules` permission (and, of course,
    `catalogue`).
 d) A user with the `manage_circ_rules`, `manage_circ_rules_restricted`
    and `catalogue` permissions.
2) As all four users, load the "Circulation and fine rules"
   administration page (admin/smart-rules.pl).
3) The page should be unchanged for the first three users. It should be
   possible to view and edit the circ rules for all libraries.
4) The last (restricted) user should only be able to view and edit the
   circ rules for their own library.

Amended by JD: In a second version of this patch
manage_circ_rules_restricted has been replaced by
manage_circ_rules_from_any_libraries and 'no_inherit' related code has
been removed
Comment 19 Jonathan Druart 2018-07-23 22:15:40 UTC
Created attachment 77203 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries
Comment 20 Jonathan Druart 2018-07-23 22:17:21 UTC
I have removed unnecessary code from the first version and provide a second patch to rename the permission as well as adjust the code. Ready for testing.
Comment 21 Jonathan Druart 2018-07-24 14:26:39 UTC
Created attachment 77219 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

Test plan:

1) Ensure that you have four users:
 a) A superlibrarian
 b) A user with all `parameters` permissions (the toplevel `parameters`
    box is checked).
 c) A user with the `manage_circ_rules` permission (and, of course,
    `catalogue`).
 d) A user with the `manage_circ_rules`, `manage_circ_rules_restricted`
    and `catalogue` permissions.
2) As all four users, load the "Circulation and fine rules"
   administration page (admin/smart-rules.pl).
3) The page should be unchanged for the first three users. It should be
   possible to view and edit the circ rules for all libraries.
4) The last (restricted) user should only be able to view and edit the
   circ rules for their own library.

Amended by JD: In a second version of this patch
manage_circ_rules_restricted has been replaced by
manage_circ_rules_from_any_libraries and 'no_inherit' related code has
been removed
Comment 22 Jonathan Druart 2018-07-24 14:26:44 UTC
Created attachment 77220 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries
Comment 23 Cab Vinton 2018-08-29 00:31:00 UTC
Issues in Biblibre Sandbox 1:

Some problems occurred applying patches from bug 15520:
<h1>Something went wrong !</h1>Applying: Bug 15520: Add permission to restrict circ rules editing to own library
Using index info to reconstruct a base tree...
M       admin/smart-rules.pl
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging admin/smart-rules.pl
error: Failed to merge in the changes.
Patch failed at 0001 Bug 15520: Add permission to restrict circ rules editing to own library
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 15520 - Add more granular permission for only editing own library's circ rules

77219 - Bug 15520: Add permission to restrict circ rules editing to own library
77220 - Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-15520-Add-permission-to-restrict-circ-rules-ed-GgcAa1.patch .
Comment 24 Jonathan Druart 2018-08-29 17:49:28 UTC
Created attachment 78275 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

Test plan:

1) Ensure that you have four users:
 a) A superlibrarian
 b) A user with all `parameters` permissions (the toplevel `parameters`
    box is checked).
 c) A user with the `manage_circ_rules` permission (and, of course,
    `catalogue`).
 d) A user with the `manage_circ_rules`, `manage_circ_rules_restricted`
    and `catalogue` permissions.
2) As all four users, load the "Circulation and fine rules"
   administration page (admin/smart-rules.pl).
3) The page should be unchanged for the first three users. It should be
   possible to view and edit the circ rules for all libraries.
4) The last (restricted) user should only be able to view and edit the
   circ rules for their own library.

Amended by JD: In a second version of this patch
manage_circ_rules_restricted has been replaced by
manage_circ_rules_from_any_libraries and 'no_inherit' related code has
been removed
Comment 25 Jonathan Druart 2018-08-29 17:49:33 UTC
Created attachment 78276 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries
Comment 26 Lisette Scheer 2018-09-14 19:26:20 UTC
Applying: Bug 15520: Add permission to restrict circ rules editing to own library
Using index info to reconstruct a base tree...
M       admin/smart-rules.pl
M       koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
Auto-merging admin/smart-rules.pl
CONFLICT (content): Merge conflict in admin/smart-rules.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 15520: Add permission to restrict circ rules editing to own library
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-15520-Add-permission-to-restrict-circ-rules-ed-IQNrNs.patch


Lisette
Comment 27 Chris Cormack 2018-09-14 19:32:06 UTC
Created attachment 78722 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

Test plan:

1) Ensure that you have four users:
 a) A superlibrarian
 b) A user with all `parameters` permissions (the toplevel `parameters`
    box is checked).
 c) A user with the `manage_circ_rules` permission (and, of course,
    `catalogue`).
 d) A user with the `manage_circ_rules`, `manage_circ_rules_restricted`
    and `catalogue` permissions.
2) As all four users, load the "Circulation and fine rules"
   administration page (admin/smart-rules.pl).
3) The page should be unchanged for the first three users. It should be
   possible to view and edit the circ rules for all libraries.
4) The last (restricted) user should only be able to view and edit the
   circ rules for their own library.

Amended by JD: In a second version of this patch
manage_circ_rules_restricted has been replaced by
manage_circ_rules_from_any_libraries and 'no_inherit' related code has
been removed
Comment 28 Chris Cormack 2018-09-14 19:32:11 UTC
Created attachment 78723 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries
Comment 29 Lisette Scheer 2018-09-14 19:47:53 UTC
Restricted user was able to see rules for all libraries and when they tried to edit it, the following error occurred:
No property note for Koha::IssuingRule at /usr/share/perl5/Exception/Class/Base.pm line 73

Lisette
Comment 30 Jonathan Druart 2018-09-14 20:49:21 UTC
(In reply to Lisette Scheer from comment #29)
> Restricted user was able to see rules for all libraries and when they tried
> to edit it, the following error occurred:
> No property note for Koha::IssuingRule at
> /usr/share/perl5/Exception/Class/Base.pm line 73
> 
> Lisette

I think comment was for bug 12365.
Comment 31 Lisette Scheer 2018-09-14 21:24:46 UTC
Created attachment 78735 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

Test plan:

1) Ensure that you have four users:
 a) A superlibrarian
 b) A user with all `parameters` permissions (the toplevel `parameters`
    box is checked).
 c) A user with the `manage_circ_rules` permission (and, of course,
    `catalogue`).
 d) A user with the `manage_circ_rules`, `manage_circ_rules_restricted`
    and `catalogue` permissions.
2) As all four users, load the "Circulation and fine rules"
   administration page (admin/smart-rules.pl).
3) The page should be unchanged for the first three users. It should be
   possible to view and edit the circ rules for all libraries.
4) The last (restricted) user should only be able to view and edit the
   circ rules for their own library.

Amended by JD: In a second version of this patch
manage_circ_rules_restricted has been replaced by
manage_circ_rules_from_any_libraries and 'no_inherit' related code has
been removed
Comment 32 Lisette Scheer 2018-09-14 21:24:50 UTC
Created attachment 78736 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>
Comment 33 Josef Moravec 2018-10-01 20:17:00 UTC
Created attachment 79780 [details] [review]
Bug 15520: Add permission to restrict circ rules editing to own library

Test plan:

1) Ensure that you have four users:
 a) A superlibrarian
 b) A user with all `parameters` permissions (the toplevel `parameters`
    box is checked).
 c) A user with the `manage_circ_rules` permission (and, of course,
    `catalogue`).
 d) A user with the `manage_circ_rules`, `manage_circ_rules_restricted`
    and `catalogue` permissions.
2) As all four users, load the "Circulation and fine rules"
   administration page (admin/smart-rules.pl).
3) The page should be unchanged for the first three users. It should be
   possible to view and edit the circ rules for all libraries.
4) The last (restricted) user should only be able to view and edit the
   circ rules for their own library.

Amended by JD: In a second version of this patch
manage_circ_rules_restricted has been replaced by
manage_circ_rules_from_any_libraries and 'no_inherit' related code has
been removed

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 34 Josef Moravec 2018-10-01 20:17:06 UTC
Created attachment 79781 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

Signed-off-by: Lisette Scheer <lisetteslatah@gmail.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 35 Josef Moravec 2018-10-01 20:17:12 UTC
Created attachment 79782 [details] [review]
Bug 15520: (QA follow-up) Fix renaming of circ rules permission

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 36 Nick Clemens 2018-10-09 16:27:48 UTC
Awesome work all!

Pushed to master for 18.11
Comment 37 Martin Renvoize 2018-10-15 13:42:10 UTC
Enhancement, will not be backported to 18.05.x series.