Bug 20100 - A non-superlibrarian should not be able to add superlibrarian privileges
Summary: A non-superlibrarian should not be able to add superlibrarian privileges
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
: 7648 10537 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-01-30 15:59 UTC by Marcel de Rooy
Modified: 2018-05-16 23:37 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
This report adds pref ProtectSuperlibrarianPrivileges in order to block users without superlibrarian privileges to modify the superlibrarian flag for themselves or other users, if the pref is enabled. For existing installs the pref will not be set, so behavior does not change. For new installs the pref will be enabled.


Attachments
Bug 20100: Introduce preference ProtectSuperlibPrivs (3.56 KB, patch)
2018-01-31 16:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlib privileges at server side (2.37 KB, patch)
2018-01-31 16:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlibrarian privileges at client side (1.54 KB, patch)
2018-01-31 16:37 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlibrarian privileges at client side (1.58 KB, patch)
2018-02-19 08:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Introduce preference ProtectSuperlibPrivs (3.60 KB, patch)
2018-02-27 13:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlib privileges at server side (2.35 KB, patch)
2018-02-27 13:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlibrarian privileges at client side (1.58 KB, patch)
2018-02-27 13:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Introduce preference ProtectSuperlibPrivs (3.62 KB, patch)
2018-03-13 09:37 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlib privileges at server side (2.37 KB, patch)
2018-03-13 09:37 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlibrarian privileges at client side (1.60 KB, patch)
2018-03-13 09:37 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 20100: Introduce preference ProtectSuperlibPrivs (3.67 KB, patch)
2018-04-20 11:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlib privileges at server side (2.42 KB, patch)
2018-04-20 11:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20100: Disallow access to superlibrarian privileges at client side (1.66 KB, patch)
2018-04-20 11:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20100: (Alternate) Use template logic instead of js to disable superlib box (4.07 KB, patch)
2018-04-20 11:54 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 20100: (QA follow-up) Use template logic instead of js to disable superlib box (4.16 KB, patch)
2018-04-23 13:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: (QA follow-up) Change syspref name to full version (4.76 KB, patch)
2018-04-25 11:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: (QA follow-up) Pref description and improve code in member-flags (3.00 KB, patch)
2018-04-25 11:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20100: Explain why the checkbox is disabled (2.10 KB, patch)
2018-04-25 13:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20100: Set ProtectSuperlibrarianPrivileges by default (1.70 KB, patch)
2018-04-25 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-01-30 15:59:55 UTC
If you have permissions and staff access, you can give yourself or others superlibrarian privileges. Which is rather odd imo (at least). And a potential risk?
Comment 1 Jonathan Druart 2018-01-30 16:19:38 UTC
We have a "permissions" permission which does what is advertised :)
Comment 2 Marcel de Rooy 2018-01-31 12:21:29 UTC
(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 ;)
Comment 3 Katrin Fischer 2018-01-31 13:03:04 UTC
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. :)
Comment 4 Marcel de Rooy 2018-01-31 13:20:25 UTC
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.
Comment 5 Katrin Fischer 2018-01-31 13:38:18 UTC
Hm, so you could only add one using the onboarding tool then or by deactivating the pref?
Comment 6 Marcel de Rooy 2018-01-31 13:48:17 UTC
(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).
Comment 7 Marcel de Rooy 2018-01-31 16:37:13 UTC
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>
Comment 8 Marcel de Rooy 2018-01-31 16:37:17 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2018-01-31 16:37:22 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2018-01-31 16:38:42 UTC
This is the idea. Will finish up tomorrow. No refactoring of the complete script here.
Comment 11 Marcel de Rooy 2018-02-19 08:05:25 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2018-02-19 08:06:10 UTC
Rebased for move of javascript to footer.
Comment 13 Marcel de Rooy 2018-02-27 13:48:29 UTC
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>
Comment 14 Marcel de Rooy 2018-02-27 13:48:34 UTC
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>
Comment 15 Marcel de Rooy 2018-02-27 13:48:38 UTC
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>
Comment 16 Marcel de Rooy 2018-02-27 13:49:00 UTC
Rebase on sysprefs.sql
Comment 17 sandboxes@biblibre.com 2018-03-13 09:37:11 UTC
Patch tested with a sandbox, by JM Broust <jean-manuel.broust@univ-lyon2.fr>
Comment 18 sandboxes@biblibre.com 2018-03-13 09:37:51 UTC
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>
Comment 19 sandboxes@biblibre.com 2018-03-13 09:37:54 UTC
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>
Comment 20 sandboxes@biblibre.com 2018-03-13 09:37:57 UTC
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>
Comment 21 Nick Clemens 2018-04-20 11:54:40 UTC
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>
Comment 22 Nick Clemens 2018-04-20 11:54:43 UTC
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>
Comment 23 Nick Clemens 2018-04-20 11:54:46 UTC
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>
Comment 24 Nick Clemens 2018-04-20 11:54:49 UTC
Created attachment 74621 [details] [review]
Bug 20100: (Alternate) Use template logic instead of js to disable superlib box
Comment 25 Nick Clemens 2018-04-20 11:57:01 UTC
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
Comment 26 Marcel de Rooy 2018-04-23 13:03:45 UTC
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>
Comment 27 Marcel de Rooy 2018-04-23 13:04:05 UTC
(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.
Comment 28 Jonathan Druart 2018-04-23 16:14:12 UTC
Should not we enable this by default for new installations?
Comment 29 Jonathan Druart 2018-04-23 16:30:37 UTC
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?
Comment 30 Lisette 2018-04-24 16:38:31 UTC
(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
Comment 31 Marcel de Rooy 2018-04-25 11:03:31 UTC
(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..
Comment 32 Marcel de Rooy 2018-04-25 11:57:22 UTC
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.
Comment 33 Marcel de Rooy 2018-04-25 11:57:27 UTC
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.
Comment 34 Marcel de Rooy 2018-04-25 11:58:20 UTC
(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.
Comment 35 Marcel de Rooy 2018-04-25 11:59:10 UTC
(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.
Comment 36 Jonathan Druart 2018-04-25 13:38:10 UTC
Created attachment 74844 [details] [review]
Bug 20100: Explain why the checkbox is disabled
Comment 37 Jonathan Druart 2018-04-25 13:49:05 UTC
Created attachment 74846 [details] [review]
Bug 20100: Set ProtectSuperlibrarianPrivileges by default
Comment 38 Jonathan Druart 2018-04-25 14:50:58 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 39 Katrin Fischer 2018-05-16 23:30:03 UTC
*** Bug 7648 has been marked as a duplicate of this bug. ***
Comment 40 Katrin Fischer 2018-05-16 23:37:10 UTC
*** Bug 10537 has been marked as a duplicate of this bug. ***