Created attachment 123972 [details] [review] Bug 28882: permission UI allows selection of superlibrarian permission With bug 20100 and 22150 we allow to select all the permissions but the superlibrarian permission, to ease the selection. And we also forbid a non-superlibrarian user to add superlibrarian permission. However there is something wrong in the JS code and it's possible to add the superlibrarian permission. The user is getting an ugly 500 and so the permission change is not done, but the UI checks must be fixed. To recreate: Login with a non-superlibrarian user Edit permission Clear all => You can select the "superlibrarian" permission Test plan: Login with a non-superlibrarian user Try to set the superlibrarian permissions to a user => not possible Try the select all/clear all => still cannot set the superlibrarian permission Work to be done: Login with a non-superlibrarian user Edit permissions for a superlibrarian user => You can remove it, then cannot add it back Should we allow removal of superlibrarian permission by non-superlibrarian user?
Can you have a look at the "work to be done" in the last patch please? What's your opinion? Did we discuss it already?
Work to be done: Login with a non-superlibrarian user Edit permissions for a superlibrarian user => You can remove it, then cannot add it back Should we allow removal of superlibrarian permission by non-superlibrarian user? No, I definitely think that a non-superlib should not be able to remove perms from a superlib. That would be kind of a security hole imo.
Created attachment 124082 [details] [review] Bug 28882: Disable select/hide all links We want to prevent non-superlibrarian users to remove superlibrarian flag. It's handled already in the controller, but needs to be done client-side. Here we are disabling the select/hide all links to remove the remaining possibility to remove the superlibrarian flag from non-superlibrarian users.
Created attachment 124443 [details] [review] Bug 28882: permission UI allows selection of superlibrarian permission With bug 20100 and 22150 we allow to select all the permissions but the superlibrarian permission, to ease the selection. And we also forbid a non-superlibrarian user to add superlibrarian permission. However there is something wrong in the JS code and it's possible to add the superlibrarian permission. The user is getting an ugly 500 and so the permission change is not done, but the UI checks must be fixed. To recreate: Login with a non-superlibrarian user Edit permission Clear all => You can select the "superlibrarian" permission Test plan: Login with a non-superlibrarian user Try to set the superlibrarian permissions to a user => not possible Try the select all/clear all => still cannot set the superlibrarian permission Work to be done: Login with a non-superlibrarian user Edit permissions for a superlibrarian user => You can remove it, then cannot add it back Should we allow removal of superlibrarian permission by non-superlibrarian user?
Created attachment 124444 [details] [review] Bug 28882: permission UI allows selection of superlibrarian permission With bug 20100 and 22150 we allow to select all the permissions but the superlibrarian permission, to ease the selection. And we also forbid a non-superlibrarian user to add superlibrarian permission. However there is something wrong in the JS code and it's possible to add the superlibrarian permission. The user is getting an ugly 500 and so the permission change is not done, but the UI checks must be fixed. To recreate: Login with a non-superlibrarian user Edit permission Clear all => You can select the "superlibrarian" permission Test plan: Login with a non-superlibrarian user Try to set the superlibrarian permissions to a user => not possible Try the select all/clear all => still cannot set the superlibrarian permission Work to be done: Login with a non-superlibrarian user Edit permissions for a superlibrarian user => You can remove it, then cannot add it back Should we allow removal of superlibrarian permission by non-superlibrarian user? Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Created attachment 124445 [details] [review] Bug 28882: Disable select/hide all links We want to prevent non-superlibrarian users to remove superlibrarian flag. It's handled already in the controller, but needs to be done client-side. Here we are disabling the select/hide all links to remove the remaining possibility to remove the superlibrarian flag from non-superlibrarian users. Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz>
Created attachment 124558 [details] [review] Bug 28882: permission UI allows selection of superlibrarian permission With bug 20100 and 22150 we allow to select all the permissions but the superlibrarian permission, to ease the selection. And we also forbid a non-superlibrarian user to add superlibrarian permission. However there is something wrong in the JS code and it's possible to add the superlibrarian permission. The user is getting an ugly 500 and so the permission change is not done, but the UI checks must be fixed. To recreate: Login with a non-superlibrarian user Edit permission Clear all => You can select the "superlibrarian" permission Test plan: Login with a non-superlibrarian user Try to set the superlibrarian permissions to a user => not possible Try the select all/clear all => still cannot set the superlibrarian permission Work to be done: Login with a non-superlibrarian user Edit permissions for a superlibrarian user => You can remove it, then cannot add it back Should we allow removal of superlibrarian permission by non-superlibrarian user? Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 124559 [details] [review] Bug 28882: Disable select/hide all links We want to prevent non-superlibrarian users to remove superlibrarian flag. It's handled already in the controller, but needs to be done client-side. Here we are disabling the select/hide all links to remove the remaining possibility to remove the superlibrarian flag from non-superlibrarian users. Signed-off-by: Hayley Pelham <hayleypelham@catalyst.net.nz> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
What remains for me is the question: Should we allow a non-superlibrarian to give him or herself the privilege to change systemprefs? Because you can disable ProtectSuperlib etc.. But this should be solved on another report imo. Should we only protect that pref, or a specific set of prefs, or actually not allow changing sysprefs at all by non superlibs? I tend to choose the latter. Even a few other options on the Admin menu like Koha to MARC mapping (among others) might be candidates too for restricting them to superlibrarians only. What do you think?
Pushed to master for 21.11, thanks to everybody involved!
(In reply to Marcel de Rooy from comment #10) > What remains for me is the question: Should we allow a non-superlibrarian to > give him or herself the privilege to change systemprefs? > Because you can disable ProtectSuperlib etc.. > But this should be solved on another report imo. > > Should we only protect that pref, or a specific set of prefs, or actually > not allow changing sysprefs at all by non superlibs? I tend to choose the > latter. I don't think shutting out non-superlibrarians from the admin tools/system preferences is a feasible solution. That forces you to give out superlibrarian permissions to more people instead of making it a protected circle. We just added granular permissions for administration areas - that would make them practically useless. Maybe the ProtectSuperLib should not be a pref in the first place? > Even a few other options on the Admin menu like Koha to MARC mapping (among > others) might be candidates too for restricting them to superlibrarians only. > What do you think? We do already have separate permissions for all those areas, I believe adding dependencies like that, will only make the system more intransparent.
(In reply to Katrin Fischer from comment #12) Thx for your feedback. > I don't think shutting out non-superlibrarians from the admin tools/system > preferences is a feasible solution. That forces you to give out > superlibrarian permissions to more people instead of making it a protected > circle. We just added granular permissions for administration areas - that > would make them practically useless. It might be helpful if we could distinguish between viewing sysprefs and changing them. And along those lines, a small set of sysprefs could somehow be set apart for changes only by superlibrarian. Perhaps even configurable by sysadmin. The gap in the changing permissions form is that we allow a staff user to set and clear permissions that he does not have himself. Which is a bit awkward, especially for Admininstration. I think that we should give that a bit more thought. > Maybe the ProtectSuperLib should not be a pref in the first place? Currently, a lot of devs need a preference to come in since we are too afraid to change 'current behavior'. This pref could be marked as privileged. See before. I will open a few reports for these things.
Pushed to 21.05.x for 21.05.04
Depends on Bug B22150 not in 20.11.x