Description
Marcel de Rooy
2018-01-30 15:59:55 UTC
We have a "permissions" permission which does what is advertised :) (In reply to Jonathan Druart from comment #1) > We have a "permissions" permission which does what is advertised :) It does too much and I wonder if we should want to advertize that ;) But you can also give all other permissions with permissions... which is almost superlibrarian. I don't see a big issue with it. If we want to change behaviour, I think we'd need a good concept first, because making it impossible to misuse is probably impossible. :) Thx for responding, Jonathan and Katrin. Your response tell me at least that there is no need to keep this report under Security. I am thinking of an approach to hide superlib privs from non-superlibs based on a preference or so. Hm, so you could only add one using the onboarding tool then or by deactivating the pref? (In reply to Katrin Fischer from comment #5) > Hm, so you could only add one using the onboarding tool then or by > deactivating the pref? A superlib can always add another one. The point is that a non-superlib should not be able to do so (imo). Created attachment 71091 [details] [review] Bug 20100: Introduce preference ProtectSuperlibPrivs This pref allows you to block changes to superlibrarian privileges at client and server side for non-superlibrarians. This patch only contains the db rev and the preference description. Test plan: Run dbrev or new install. Check pref value. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 71092 [details] [review] Bug 20100: Disallow access to superlib privileges at server side Depends on pref ProtectSuperlibPrivs. If enabled, script member-flags.pl will not allow you to add or remove superlib privs when you are no superlibrarian. The follow-up patch will enable the check at client side. Test plan: [1] Enable the pref. Do not apply the third patch (client side). [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you have an internal server error when you add or remove superlib privs. The log contains a warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 71093 [details] [review] Bug 20100: Disallow access to superlibrarian privileges at client side This last patch activates the check at client side. If the pref ProtectSuperlibPrivs is enabled, non-superlibs should not be able to change superlibrarian privileges via the interface. Test plan: [1] Enable the pref. [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you cannot add or remove superlib privs. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> This is the idea. Will finish up tomorrow. No refactoring of the complete script here. Created attachment 71918 [details] [review] Bug 20100: Disallow access to superlibrarian privileges at client side This last patch activates the check at client side. If the pref ProtectSuperlibPrivs is enabled, non-superlibs should not be able to change superlibrarian privileges via the interface. Test plan: [1] Enable the pref. [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you cannot add or remove superlib privs. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebased for move of javascript to footer. Created attachment 72260 [details] [review] Bug 20100: Introduce preference ProtectSuperlibPrivs This pref allows you to block changes to superlibrarian privileges at client and server side for non-superlibrarians. This patch only contains the db rev and the preference description. Test plan: Run dbrev or new install. Check pref value. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 72261 [details] [review] Bug 20100: Disallow access to superlib privileges at server side Depends on pref ProtectSuperlibPrivs. If enabled, script member-flags.pl will not allow you to add or remove superlib privs when you are no superlibrarian. The follow-up patch will enable the check at client side. Test plan: [1] Enable the pref. Do not apply the third patch (client side). [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you have an internal server error when you add or remove superlib privs. The log contains a warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 72262 [details] [review] Bug 20100: Disallow access to superlibrarian privileges at client side This last patch activates the check at client side. If the pref ProtectSuperlibPrivs is enabled, non-superlibs should not be able to change superlibrarian privileges via the interface. Test plan: [1] Enable the pref. [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you cannot add or remove superlib privs. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Rebase on sysprefs.sql Patch tested with a sandbox, by JM Broust <jean-manuel.broust@univ-lyon2.fr> Created attachment 72747 [details] [review] Bug 20100: Introduce preference ProtectSuperlibPrivs This pref allows you to block changes to superlibrarian privileges at client and server side for non-superlibrarians. This patch only contains the db rev and the preference description. Test plan: Run dbrev or new install. Check pref value. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr> Created attachment 72749 [details] [review] Bug 20100: Disallow access to superlib privileges at server side Depends on pref ProtectSuperlibPrivs. If enabled, script member-flags.pl will not allow you to add or remove superlib privs when you are no superlibrarian. The follow-up patch will enable the check at client side. Test plan: [1] Enable the pref. Do not apply the third patch (client side). [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you have an internal server error when you add or remove superlib privs. The log contains a warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr> Created attachment 72750 [details] [review] Bug 20100: Disallow access to superlibrarian privileges at client side This last patch activates the check at client side. If the pref ProtectSuperlibPrivs is enabled, non-superlibs should not be able to change superlibrarian privileges via the interface. Test plan: [1] Enable the pref. [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you cannot add or remove superlib privs. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr> Created attachment 74618 [details] [review] Bug 20100: Introduce preference ProtectSuperlibPrivs This pref allows you to block changes to superlibrarian privileges at client and server side for non-superlibrarians. This patch only contains the db rev and the preference description. Test plan: Run dbrev or new install. Check pref value. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 74619 [details] [review] Bug 20100: Disallow access to superlib privileges at server side Depends on pref ProtectSuperlibPrivs. If enabled, script member-flags.pl will not allow you to add or remove superlib privs when you are no superlibrarian. The follow-up patch will enable the check at client side. Test plan: [1] Enable the pref. Do not apply the third patch (client side). [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you have an internal server error when you add or remove superlib privs. The log contains a warning. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 74620 [details] [review] Bug 20100: Disallow access to superlibrarian privileges at client side This last patch activates the check at client side. If the pref ProtectSuperlibPrivs is enabled, non-superlibs should not be able to change superlibrarian privileges via the interface. Test plan: [1] Enable the pref. [2] Login as superlib and add/remove superlib privs to a staff user. [3] Login as another user (no superlib, but having borrowers, permissions and staff_access). Verify that you cannot add or remove superlib privs. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: JM Broust <jean-manuel.broust@univ-lyon2.fr> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 74621 [details] [review] Bug 20100: (Alternate) Use template logic instead of js to disable superlib box All looks good to me, the trick of enabling the button on submit didn't feel right though. I attached an alternate patch that changes the form, adding a hidden input for when the button is disabled. It forced a few more js changes, but I like the concept better. Can you weigh in Marcel and either SO or obsolete my patch? you can then mark as PQA Created attachment 74721 [details] [review] Bug 20100: (QA follow-up) Use template logic instead of js to disable superlib box Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> (In reply to Nick Clemens from comment #25) > All looks good to me, the trick of enabling the button on submit didn't feel > right though. > > I attached an alternate patch that changes the form, adding a hidden input > for when the button is disabled. It forced a few more js changes, but I like > the concept better. > > Can you weigh in Marcel and either SO or obsolete my patch? you can then > mark as PQA Thx for QA, Nick. The 'trick' is needed to include the field in the submit. Could have added a hidden input too. I do not have a strong opinion about it. Your patch works. So fine with me. Should not we enable this by default for new installations? 1. The block reads weird, I would have written: IF we are going to add or remove the librarian flag THEN we check if the loggeed in patron is allowed to But not blocker. 2. I do not think we should shorten syspref's name. Not blocker 3. The descriptions of the 2 options are: - "Do not block permitted non-superlibrarians" - "Allow only superlibrarians" It is negative for "yes" and positive for "no". Native English speakers, sounds ok for you? (In reply to Jonathan Druart from comment #29) > 3. The descriptions of the 2 options are: > - "Do not block permitted non-superlibrarians" > - "Allow only superlibrarians" > It is negative for "yes" and positive for "no". > > Native English speakers, sounds ok for you? I think it would be clearer to either say: "Do not block permitted non-superlibrarians" "Block permitted non-superlibrarians" OR "Allow permitted non-superlibrarians" "Allow only superlibrarians" The other way just seems mixed up to me. Lisette (In reply to Jonathan Druart from comment #28) > Should not we enable this by default for new installations? My initial thought was to do this without a preference. But strong opposition forced me to do otherwise ;) So I would not dare to make it the default yet.. Created attachment 74840 [details] [review] Bug 20100: (QA follow-up) Change syspref name to full version As per RM request, changing the shorter name to its full form. Result of a git grep | sed. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> No occurrences to be found anymore with git grep -i. Created attachment 74841 [details] [review] Bug 20100: (QA follow-up) Pref description and improve code in member-flags Two points as mentioned on Bugzilla comment29. [1] Improve pref description. Feedback from comment30 incorporated. [2] Improve code in member-flags. Check if we change librarian flag first. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested the die with "disable_superlibrarian_privs => 0" in member-flags. (In reply to Jonathan Druart from comment #29) > 1. The block reads weird, I would have written: > IF we are going to add or remove the librarian flag > THEN we check if the loggeed in patron is allowed to > > But not blocker. > > 2. I do not think we should shorten syspref's name. Not blocker > > 3. The descriptions of the 2 options are: > - "Do not block permitted non-superlibrarians" > - "Allow only superlibrarians" > It is negative for "yes" and positive for "no". > > Native English speakers, sounds ok for you? Point 2 fixed in patch 5. Points 1 and 3 are addressed in patch 6. (In reply to Lisette from comment #30) > I think it would be clearer to either say: > "Do not block permitted non-superlibrarians" > "Block permitted non-superlibrarians" > > OR > "Allow permitted non-superlibrarians" > "Allow only superlibrarians" > > The other way just seems mixed up to me. > Lisette Thx Lisette. Incorporated the two allows in the pref description on patch 6. Created attachment 74844 [details] [review] Bug 20100: Explain why the checkbox is disabled Created attachment 74846 [details] [review] Bug 20100: Set ProtectSuperlibrarianPrivileges by default Pushed to master for 18.05, thanks to everybody involved! *** Bug 7648 has been marked as a duplicate of this bug. *** *** Bug 10537 has been marked as a duplicate of this bug. *** Big enhancement not pushed to 17.11.x |