Bug 28882 - Incorrect permissions check client-side
Summary: Incorrect permissions check client-side
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 20100 22150
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-19 13:03 UTC by Jonathan Druart
Modified: 2021-09-16 07:46 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.04


Attachments
Bug 28882: permission UI allows selection of superlibrarian permission (4.36 KB, patch)
2021-08-19 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28882: Disable select/hide all links (3.43 KB, patch)
2021-08-25 09:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 28882: permission UI allows selection of superlibrarian permission (4.36 KB, patch)
2021-09-03 01:28 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 28882: permission UI allows selection of superlibrarian permission (4.42 KB, patch)
2021-09-03 01:30 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 28882: Disable select/hide all links (3.50 KB, patch)
2021-09-03 01:31 UTC, Hayley Pelham
Details | Diff | Splinter Review
Bug 28882: permission UI allows selection of superlibrarian permission (4.51 KB, patch)
2021-09-06 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28882: Disable select/hide all links (3.59 KB, patch)
2021-09-06 08:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-08-19 13:03:20 UTC

    
Comment 1 Jonathan Druart 2021-08-19 13:17:00 UTC
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?
Comment 2 Jonathan Druart 2021-08-19 13:20:00 UTC
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?
Comment 3 Marcel de Rooy 2021-08-19 14:15:55 UTC
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.
Comment 4 Jonathan Druart 2021-08-25 09:50:12 UTC
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.
Comment 5 Hayley Pelham 2021-09-03 01:28:50 UTC
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?
Comment 6 Hayley Pelham 2021-09-03 01:30:38 UTC
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>
Comment 7 Hayley Pelham 2021-09-03 01:31:15 UTC
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>
Comment 8 Marcel de Rooy 2021-09-06 08:04:54 UTC
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>
Comment 9 Marcel de Rooy 2021-09-06 08:04:57 UTC
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>
Comment 10 Marcel de Rooy 2021-09-06 08:13:04 UTC
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?
Comment 11 Jonathan Druart 2021-09-09 09:00:54 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 12 Katrin Fischer 2021-09-10 06:04:50 UTC
(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.
Comment 13 Marcel de Rooy 2021-09-10 06:22:42 UTC
(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.
Comment 14 Kyle M Hall 2021-09-10 12:05:49 UTC
Pushed to 21.05.x for 21.05.04
Comment 15 Fridolin Somers 2021-09-16 07:46:08 UTC
Depends on Bug B22150 not in 20.11.x