Bug 8083 - Granular parameters permissions
Summary: Granular parameters permissions
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-14 06:38 UTC by Srdjan Jankovic
Modified: 2013-05-23 06:24 UTC (History)
5 users (show)

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


Attachments
patch (20.56 KB, patch)
2012-05-15 05:06 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (27.98 KB, patch)
2012-05-22 05:58 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Attachment to Bug 8083 - Granular parameters permissions (28.04 KB, patch)
2012-07-05 12:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] bug_8083: parameters subpermissions: manage_circ_rules and parameters_remaining_permissions (28.04 KB, patch)
2012-07-05 12:10 UTC, Kyle M Hall
Details | Diff | Splinter Review
bug_8083: Explanation of update query (1.09 KB, patch)
2012-07-18 05:55 UTC, Srdjan Jankovic
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2012-05-14 06:38:36 UTC
Introduce 'manage_circ_rules' and 'parameters_remaining_permissions' parameters subpermissions.

Add "GranularParametersPermissions" boolean syspref, No default. If set to yes use aforementioned subpermissions in admin/* functions, otherwise just use parameters flag
Comment 1 Katrin Fischer 2012-05-14 06:46:38 UTC
Hi Srdjan, 
I think adding granular permissions for administration is a good idea and I can think of more to be added later :)

But I am not sure the system preference is needed here. We already have granular permissions for other modules. All granular permissions used to be controlled by a system preference that got removed after some time, because granular got to be standard over permissions on module level only.

I think it should also be ok on update if both granular permissions are checked by default.
Comment 2 Nicole C. Engard 2012-05-14 12:26:35 UTC
I agree with Katrin :)
Comment 3 Srdjan Jankovic 2012-05-15 00:06:10 UTC
I totally agree. The suspref was suggested in order not to disrupt other people, but when I looked at the workflow it did not make any sense. I've already removed it, just did not update the bug.

Assigning both permissions goes without saying :)
Comment 4 Srdjan Jankovic 2012-05-15 05:06:17 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2012-05-15 05:10:09 UTC
I have blocked "Circ and Fines rules" link if manage_circ_rules permission is missing, but did not do similar for all other links in case parameters_remaining_permissions is missing - I did not know how to do it and still have a decent looking screen.
Any ideas welcome.
Comment 6 Katrin Fischer 2012-05-15 06:42:33 UTC
Hi Srdjan, 
the userpermissions.sql is one of the files that gets translated to provide a web installer in different languages. Can you please include the other languages too? Leaving the description in English should be ok. There is also a test that got updated very recently (bug 8022): xt/permissions.t
Comment 7 Srdjan Jankovic 2012-05-22 05:58:58 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-07-05 12:09:53 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2012-07-05 12:10:16 UTC
Created attachment 10639 [details] [review]
[SIGNED-OFF] bug_8083: parameters subpermissions: manage_circ_rules and parameters_remaining_permissions

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Paul Poulain 2012-07-13 13:37:11 UTC
QA comment,
I was about passing QA this patch, when I saw in the updatedatabase:
+    $dbh->do("INSERT INTO user_permissions (borrowernumber, module_bit, code)
+        SELECT borrowernumber, 3, 'parameters_remaining_permissions'
+        FROM borrowers WHERE flags & (1 << 3)");

I think it gives the permission to all patrons that already had the 'admin' permission. But:
1- please confirm
2- a comment just before the request would be usefull for a future reader.

Site comment: we have to find a way to handle translated user permission : our libraries are complaining that new ones are not translated. That's because the updatedatabase is in english & in english only. But that's another topic...
Comment 11 Srdjan Jankovic 2012-07-18 05:55:24 UTC
Created attachment 10917 [details] [review]
bug_8083: Explanation of update query
Comment 12 Srdjan Jankovic 2012-07-18 06:00:51 UTC
> I think it gives the permission to all patrons that already had the 'admin'
> permission. But:
> 1- please confirm

Confirmed, except it is 'parameters' permission

> 2- a comment just before the request would be usefull for a future reader.

Done
Comment 13 Paul Poulain 2012-07-23 08:43:13 UTC
QA comments: previous comments fixed, no more reason not to pass QA
Comment 14 Paul Poulain 2012-07-23 08:50:32 UTC
I tried to access all admin pages with a superlibrarian account after applying the patch, no problem displaying those pages.

Good job srdjan.

@all = follow-ups welcomed (in separate bugs) for introducing other admin granular permissions !