Description
Nicole C. Engard
2015-06-15 21:07:27 UTC
I agree. If you give a permission to marc_modification_templates, a librarian can't do it because she has no admin permission! 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. Created attachment 77461 [details] [review] Bug 14291: 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 Created attachment 77538 [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> QA: Looking here 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? (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 :) Created attachment 78109 [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> > > 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 :) Created attachment 79556 [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> Created attachment 79557 [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> Created attachment 79558 [details] [review] Bug 14391: (QA follow-up) Fix some typos > 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. Created attachment 80004 [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> Created attachment 80005 [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> Created attachment 80006 [details] [review] Bug 14391: (QA follow-up) Fix some typos Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/includes/permissions.inc CONFLICT (content): Merge conflict in installer/data/mysql/userpermissions.sql 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> 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> Created attachment 80543 [details] [review] Bug 14391: (QA follow-up) Fix some typos Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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. Rebased and fixed for latest changes on master. Created attachment 81362 [details] [review] Bug 14391: Move udpate to perl 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 Created attachment 81451 [details] [review] Bug 14391: (QA follo-up) Correct database update for manage_item_search_fields permission Created attachment 81452 [details] [review] Bug 14391: (QA follow-up) Correct database update for manage_item_search_fields permission Hi Nick, thx for catching this! I have corrected and tested the update. Created attachment 82022 [details] [review] Bug 14391: (RM follow-up) Adjust a few template conditions Awesome work all! Pushed to master for 18.11 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 Enhancement, will not be backported to 18.05.x series. |