Bug 14391 - Granular permissions for the administration module
Summary: Granular permissions for the administration module
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Target Milestone: ---
Assignee: Katrin Fischer
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords: release-notes-needed
Depends on: 7651
Blocks: 15836 21961
  Show dependency treegraph
 
Reported: 2015-06-15 21:07 UTC by Nicole C. Engard
Modified: 2018-12-06 11:50 UTC (History)
9 users (show)

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


Attachments
Bug 14291: Add granular permissions to the administration module (72.77 KB, patch)
2018-08-02 23:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: Add granular permissions to the administration module (72.76 KB, patch)
2018-08-07 15:04 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Resolve POD warn on svc..systempreferences (1.29 KB, patch)
2018-08-24 08:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14391: Add granular permissions to the administration module (69.85 KB, patch)
2018-09-28 06:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Resolve POD warn on svc..systempreferences (1.31 KB, patch)
2018-09-28 06:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Fix some typos (2.19 KB, patch)
2018-09-28 06:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: Add granular permissions to the administration module (69.95 KB, patch)
2018-10-05 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Resolve POD warn on svc..systempreferences (1.34 KB, patch)
2018-10-05 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Fix some typos (2.29 KB, patch)
2018-10-05 09:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14391: Add granular permissions to the administration module (70.01 KB, patch)
2018-10-12 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Resolve POD warn on svc..systempreferences (1.31 KB, patch)
2018-10-12 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Fix some typos (2.25 KB, patch)
2018-10-12 17:32 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: Keep parameters_remaining_permissions for now (2.37 KB, patch)
2018-10-12 18:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: Move udpate to perl (9.33 KB, patch)
2018-10-27 14:10 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 14391: (QA follo-up) Correct database update for manage_item_search_fields permission (9.22 KB, patch)
2018-10-29 06:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: (QA follow-up) Correct database update for manage_item_search_fields permission (9.22 KB, patch)
2018-10-29 06:59 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14391: (RM follow-up) Adjust a few template conditions (4.69 KB, patch)
2018-11-07 13:05 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2015-06-15 21:07:27 UTC
I think administration permissions should be like tools, each one should have its own permissions so you can allow catalogers access to some of the cataloging admin but not the system preferences for example.
Comment 1 knuuti 2016-06-07 09:37:49 UTC
I agree. If you give a permission to marc_modification_templates, a librarian can't do it because she has no admin permission!
Comment 2 Jeanne Heicher 2018-01-29 16:12:58 UTC
I, too, vote for granularity in the permissions for the Admin module in Koha. As a library director, I have people doing tasks where they need access to parts of the admin module, but giving them the keys to the kingdom is not an option. This week we are struggling with updating our MARC frameworks. I will not be able to have my cataloger do this by herself (because of the permissions), so I will have to spend a lot of my time working with her to get the job done. If I could just give her permissions to alter the MARC frameworks, I would have a couple of work days more in my schedule this week to do what I need to do apart from the catalog improvements.
Comment 3 Katrin Fischer 2018-08-02 23:53:53 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2018-08-07 15:04:50 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2018-08-24 07:03:03 UTC
QA: Looking here
Comment 6 Marcel de Rooy 2018-08-24 08:01:55 UTC
QA Comment:
Looks good! Nice addition. Some minor comments which should take much time:

manage_classifications No text on form? => typo CASE 'manage_classfications' Missing letter !
Spelling Manage column configuation / SRU server confiugration / search enginge

installer/data/mysql/atomicupdate/bug14391_granular_admin_permissions.sql
The atomic update queries could be further optimized. You add separate inserts for each permission with subqueries for borrowers. Easy to read, but more costly to run.
Do you have figures about how long it takes on a larger patron table ?
Why not something like:
INSERT INTO user_permissions (borrowernumber, module_bit, code)
SELECT borrowers.borrowernumber, permissions.module_bit, permissions.code
FROM borrowers 
LEFT JOIN user_permissions USING (borrowernumber)
LEFT JOIN permissions ON module_bit =3 AND code NOT IN ( * list existing permissions for admin * )
WHERE user_permissions.code='parameters_remaining_permissions'
This replaces most separate inserts and uses joins instead of subqueries.

admin/localization.pl, svc/localization
Need manage_itemtypes
Understand why you do it, but it seems not logical in the long run. Better add a specific one.

admin/printers.pl
parameters => '*'
Only case where we refer to *. Why not separate?
Comment 7 Marcel de Rooy 2018-08-24 08:02:23 UTC
(In reply to Marcel de Rooy from comment #6)
> QA Comment:
> Looks good! Nice addition. Some minor comments which should take much time:
> 
> manage_classifications No text on form? => typo CASE 'manage_classfications'
> Missing letter !
> Spelling Manage column configuation / SRU server confiugration / search
> enginge
> 
> installer/data/mysql/atomicupdate/bug14391_granular_admin_permissions.sql
> The atomic update queries could be further optimized. You add separate
> inserts for each permission with subqueries for borrowers. Easy to read, but
> more costly to run.
> Do you have figures about how long it takes on a larger patron table ?
> Why not something like:
> INSERT INTO user_permissions (borrowernumber, module_bit, code)
> SELECT borrowers.borrowernumber, permissions.module_bit, permissions.code
> FROM borrowers 
> LEFT JOIN user_permissions USING (borrowernumber)
> LEFT JOIN permissions ON module_bit =3 AND code NOT IN ( * list existing
> permissions for admin * )
> WHERE user_permissions.code='parameters_remaining_permissions'
> This replaces most separate inserts and uses joins instead of subqueries.
> 
> admin/localization.pl, svc/localization
> Need manage_itemtypes
> Understand why you do it, but it seems not logical in the long run. Better
> add a specific one.
> 
> admin/printers.pl
> parameters => '*'
> Only case where we refer to *. Why not separate?

much time => not much time :)
Comment 8 Marcel de Rooy 2018-08-24 08:05:45 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2018-09-16 22:23:19 UTC
Removing the printer related code in bug 17845.
Comment 10 Katrin Fischer 2018-09-28 06:28:54 UTC
> > manage_classifications No text on form? => typo CASE 'manage_classfications'
> > Missing letter !

Fixed!

> > Spelling Manage column configuation / SRU server confiugration / search
> > enginge

Fixed!

> > installer/data/mysql/atomicupdate/bug14391_granular_admin_permissions.sql
> > The atomic update queries could be further optimized. You add separate
> > inserts for each permission with subqueries for borrowers. Easy to read, but
> > more costly to run.
> > Do you have figures about how long it takes on a larger patron table ?

I think there will be a limited number of patrons to be updated even for big libraries as the number of staff should still be relatively small compared to the potential number of users.

> > Why not something like:
> > INSERT INTO user_permissions (borrowernumber, module_bit, code)
> > SELECT borrowers.borrowernumber, permissions.module_bit, permissions.code
> > FROM borrowers 
> > LEFT JOIN user_permissions USING (borrowernumber)
> > LEFT JOIN permissions ON module_bit =3 AND code NOT IN ( * list existing
> > permissions for admin * )
> > WHERE user_permissions.code='parameters_remaining_permissions'
> > This replaces most separate inserts and uses joins instead of subqueries.

Need to take another look here if you think it's really necessary.

> > 
> > admin/localization.pl, svc/localization
> > Need manage_itemtypes
> > Understand why you do it, but it seems not logical in the long run. Better
> > add a specific one.

I'll add a separate permission in another follow-up.

> > 
> > admin/printers.pl
> > parameters => '*'
> > Only case where we refer to *. Why not separate?

The code is dead and unused - I have proposed removing it instead on bug 7651. As it's currently not used and not visible in the GUI, I would not like to expose it with a new permission. As it's non-functional there is also a super low risk of people breaking something there :)
Comment 11 Katrin Fischer 2018-09-28 06:30:13 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2018-09-28 06:30:19 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2018-09-28 06:30:25 UTC
Created attachment 79558 [details] [review]
Bug 14391: (QA follow-up) Fix some typos
Comment 14 Marcel de Rooy 2018-10-05 09:39:29 UTC
> Need to take another look here if you think it's really necessary.

Will not insist.

> > > admin/localization.pl, svc/localization
> > > Need manage_itemtypes
> > > Understand why you do it, but it seems not logical in the long run. Better
> > > add a specific one.
> I'll add a separate permission in another follow-up.

Please add a see also or blocks for the new report.
Comment 15 Marcel de Rooy 2018-10-05 09:48:01 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2018-10-05 09:48:07 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2018-10-05 09:48:12 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2018-10-12 14:43:03 UTC
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc                                                       

CONFLICT (content): Merge conflict in installer/data/mysql/userpermissions.sql
Comment 19 Katrin Fischer 2018-10-12 17:32:24 UTC
Created attachment 80541 [details] [review]
Bug 14391: Add granular permissions to the administration module

So far the administration module only allowed for 2 permissions:
- circulation conditions (manage_circ_rules)
- everything else (parameters_remaining_permissions)

With this patch almost every section of the administration page
will have its own granular permission.

To test:
- Create different staff users:
  1) One with parameters_remaining_permissions
  2) One with parameters
  3) One with catalogue and no parameters
  4) One superlibrarian
- Apply the patch
- Run the database update
- Check the staff users:
  1) All subpermissions, but manage_circ_rules
     should be checked
  2) Nothing should have changed
  3) manage_item_serach_fields shoudl be checked
     (page had catalogue permission before)
  4) Nothing should have changed
- Try different settings of the permissions and
  verify that
  - Administration page behaves correctly
  - Administration menu behaves correctly
  ! You shoudl only see what you have permission for

https://bugs.koha-community.org/show_bug.cgi?id=14391

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Katrin Fischer 2018-10-12 17:32:29 UTC
Created attachment 80542 [details] [review]
Bug 14391: (QA follow-up) Resolve POD warn on svc..systempreferences

*** WARNING: No items in =over (at line 48) / =back list at line 54 in file svc/config/systempreferences
*** WARNING: No items in =over (at line 73) / =back list at line 79 in file svc/config/systempreferences

=> If you use 'over', you should add 'item'-s.

Test plan:
Run perldoc svc/config/systempreferences

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 21 Katrin Fischer 2018-10-12 17:32:35 UTC
Created attachment 80543 [details] [review]
Bug 14391: (QA follow-up) Fix some typos

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Katrin Fischer 2018-10-12 18:25:55 UTC
Created attachment 80547 [details] [review]
Bug 14391: Keep parameters_remaining_permissions for now

Since this patch has been written, new references to this
permission have been added to the code:

- t/db_dependent/api/v1/stockrotationstage.t
- t/db_dependent/api/v1/cities.t
- admin/overdrive.pl

Instead of removing the permission, let's keep it for now
and do more clean-up later in separate patches if required.
Comment 23 Katrin Fischer 2018-10-12 18:26:44 UTC
Rebased and fixed for latest changes on master.
Comment 24 Nick Clemens 2018-10-27 14:10:29 UTC
Created attachment 81362 [details] [review]
Bug 14391: Move udpate to perl
Comment 25 Nick Clemens 2018-10-27 14:12:23 UTC
The test plan didn't work for me at this step
 3) One with catalogue and no parameters

  3) manage_item_serach_fields shoudl be checked
     (page had catalogue permission before)

The user did not get the new permission.

I attached the updated database statement I put together, please double check it - I believe the catalogue permission is stored in 'flags' and not the 'user_permissions' table
Comment 26 Katrin Fischer 2018-10-29 06:57:42 UTC
Created attachment 81451 [details] [review]
Bug 14391: (QA follo-up) Correct database update for manage_item_search_fields permission
Comment 27 Katrin Fischer 2018-10-29 06:59:18 UTC
Created attachment 81452 [details] [review]
Bug 14391: (QA follow-up) Correct database update for manage_item_search_fields permission
Comment 28 Katrin Fischer 2018-10-29 07:06:50 UTC
Hi Nick,
thx for catching this! I have corrected and tested the update.
Comment 29 Nick Clemens 2018-11-07 13:05:43 UTC
Created attachment 82022 [details] [review]
Bug 14391: (RM follow-up) Adjust a few template conditions
Comment 30 Nick Clemens 2018-11-07 13:10:16 UTC
Awesome work all!

Pushed to master for 18.11
Comment 31 Jonathan Druart 2018-11-07 13:55:36 UTC
I think these patches caused the last Jenkins failure

koha_1       | An element could not be located on the page using the given search parameters: //div[@class="main container-fluid"]//a[contains(@href, "/admin/categories.pl")],xpath at 
               /kohadevbox/koha/t/lib/Selenium.pm line 160.

SCREENSHOT: https://framapic.org/2vzFPRMkHvyA/pSVycziXstLI

=> the link "Patron categories" is not displayed
Comment 32 Martin Renvoize 2018-11-16 12:50:22 UTC
Enhancement, will not be backported to 18.05.x series.