Bug 15520

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" />

Never mind, my db update didn't run correctly. This looks like it's (still) working correctly.

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.

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>

Created attachment 72241 [details] [review]
Bug 15520: (QA follow-up) Add tests, missing installer data

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!

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?

(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

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

it's NOT a problem we should solve here... (sorry, typo)

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

(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

(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

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

Created attachment 77203 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

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.

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

Created attachment 77220 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

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 .

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

Created attachment 78276 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

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

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

Created attachment 78723 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

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

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

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

Created attachment 78736 [details] [review]
Bug 15520: Rename permission to manage_circ_rules_from_any_libraries

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

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>

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>

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>

Awesome work all!

Pushed to master for 18.11

Enhancement, will not be backported to 18.05.x series.