Bug 36085 - Setting and unsetting the protected flag should be limited to superlibrarian accounts
Summary: Setting and unsetting the protected flag should be limited to superlibrarian ...
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Jake Deery
QA Contact: Paul Derscheid
URL:
Keywords:
Depends on: 26170
Blocks: 37360 37532 37815
  Show dependency treegraph
 
Reported: 2024-02-13 15:04 UTC by Kelly McElligott
Modified: 2024-11-15 20:35 UTC (History)
12 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Only patrons with superlibrarian permissions will be able to set or remove the "Protected" flag on patron accounts
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 36085: Only superlibrarians can protect patrons (3.37 KB, patch)
2024-07-18 11:14 UTC, Jake Deery
Details | Diff | Splinter Review
Bug 36085: Only superlibrarians can protect patrons (3.40 KB, patch)
2024-07-18 11:39 UTC, Jan Kissig
Details | Diff | Splinter Review
Bug 36085: Only superlibrarians can protect patrons (3.46 KB, patch)
2024-08-27 14:02 UTC, Paul Derscheid
Details | Diff | Splinter Review
Bug 36085: (QA follow-up) Tidy members/memberentry.pl (2.74 KB, patch)
2024-08-27 14:15 UTC, Paul Derscheid
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kelly McElligott 2024-02-13 15:04:17 UTC
With the new enhancement, 26170, add protected status to patrons, this allows any patron to be updated/edited to be a 'protected patron'.  Staff with 'edit borrowers' permission will be able to update this status. I believe there should be a permission to allow for this field to be updated/edited within Koha.
Comment 1 Emily Lamancusa (emlam) 2024-05-16 18:03:59 UTC
+1 for adding a permission!
Comment 2 David Cook 2024-05-16 23:53:13 UTC
Mmm that's an interesting one. 

My original vision for this was to have it manageable only be superlibrarians (or no web users at all even).

It looks like Magnus thought about a permission for changing the protected flag in comment 14 of bug 26170. 

The hard part about adding a permission is then who is allowed to set that permission? (Sort of a "who watches the watcher" scenario.)
Comment 3 Katrin Fischer 2024-05-17 07:14:42 UTC
At the hackfest in Marseille we had a long discussion about a potential new permission system based on CRUD and objects. Having a permission for a single field goes a bit beyond that. I'd be interested to hear about the use case for needing the extra protection here. Are you worried that staff will remove the protected status willingly?
Comment 4 Kelly McElligott 2024-07-02 22:55:08 UTC
Yes, having that extra layer of protection with a permission.
Comment 5 Jake Deery 2024-07-17 14:44:53 UTC
See: https://chat.koha-community.org/koha-community/pl/bnfop47u8ifauf4gpnby4rdd8y 

It would be nice to have a discussion on some use-cases before deciding which direction to take on this bug. Is it because we are worried librarians might erroneously pick the wrong option for their needs, or that the labelling of the options in memberentry.pl are too vague?

Perhaps rather than a permission, we just need better documentation and in-line information?
Comment 6 Jan Kissig 2024-07-18 07:59:05 UTC
As I understand the protected status is preferred to administrative system users like sip2, api, selfcheck etc so that my colleagues at the counter cannot delete them accidentally. 
For standard patrons (which should not be protected in my opinion) a deletion cannot be done by accident, as there is always an inquiry like 'Are you sure ...'
Therefore I would suggest that the protect flag should have permissions bound to it or even just be allowed to be set by superlibrarians.
Comment 7 Jake Deery 2024-07-18 09:26:49 UTC
If I understand, then, the use case for this is really centred around superlibrarians protecting borrowers that are necessary for concurrent integrations to work?

If this is so, I think cait's suggestion is probably optimal. It's what I like as well, so unless anyone objects, that's the route I will take.

Thanks all for your input!
Comment 8 Katrin Fischer 2024-07-18 10:00:56 UTC
I am not sure I made one :D 
But do you mean tying to superlibrarian?
Comment 9 Jake Deery 2024-07-18 10:20:41 UTC
That's correct cait, and I meant David Cook's comment rather, about using superlibrarian..! Apologies :-)

I'll write a patch this afternoon
Comment 10 Katrin Fischer 2024-07-18 10:32:19 UTC
All good!
Comment 11 Jake Deery 2024-07-18 11:14:39 UTC
Created attachment 169127 [details] [review]
Bug 36085: Only superlibrarians can protect patrons

A drive-by patch which hopes to resolve bug 36085 by only allowing superlibrarians
to protect or unprotect patrons.

Test plan:
a)  prepare two koha staff users:
        1)  a superlibrarian
        2)  a user that only has permission to edit patrons
b)  when logged in as the user prepared in step a2 (non-superlibrarian),
    then go to edit any patron
        *)  note how you can set the protected yes/no radios
c)  apply the patch
d)  repeat steps a-b as this same user
        *)  note how you can now no longer see the protected yes/no radios
e)  log in as the user prepared in step a1 (superlibrarian), then repeat
    steps a-b
f)  note how the protected yes/no radios are back
Comment 12 Jake Deery 2024-07-18 11:15:30 UTC
A quick fix to this solution, until we have had time to think about how we want to handle these kinds of permissions in future..! :-)
Comment 13 Jan Kissig 2024-07-18 11:39:13 UTC
Created attachment 169129 [details] [review]
Bug 36085: Only superlibrarians can protect patrons

A drive-by patch which hopes to resolve bug 36085 by only allowing superlibrarians
to protect or unprotect patrons.

Test plan:
a)  prepare two koha staff users:
        1)  a superlibrarian
        2)  a user that only has permission to edit patrons
b)  when logged in as the user prepared in step a2 (non-superlibrarian),
    then go to edit any patron
        *)  note how you can set the protected yes/no radios
c)  apply the patch
d)  repeat steps a-b as this same user
        *)  note how you can now no longer see the protected yes/no radios
e)  log in as the user prepared in step a1 (superlibrarian), then repeat
    steps a-b
f)  note how the protected yes/no radios are back

Signed-off-by: Jan Kissig <jkissig@th-wildau.de>
Comment 14 Paul Derscheid 2024-08-27 14:02:18 UTC
Created attachment 170773 [details] [review]
Bug 36085: Only superlibrarians can protect patrons

A drive-by patch which hopes to resolve bug 36085 by only allowing superlibrarians
to protect or unprotect patrons.

Test plan:
a)  prepare two koha staff users:
        1)  a superlibrarian
        2)  a user that only has permission to edit patrons
b)  when logged in as the user prepared in step a2 (non-superlibrarian),
    then go to edit any patron
        *)  note how you can set the protected yes/no radios
c)  apply the patch
d)  repeat steps a-b as this same user
        *)  note how you can now no longer see the protected yes/no radios
e)  log in as the user prepared in step a1 (superlibrarian), then repeat
    steps a-b
f)  note how the protected yes/no radios are back

Signed-off-by: Jan Kissig <jkissig@th-wildau.de>
Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
Comment 15 Paul Derscheid 2024-08-27 14:15:41 UTC
Created attachment 170774 [details] [review]
Bug 36085: (QA follow-up) Tidy members/memberentry.pl
Comment 16 Katrin Fischer 2024-08-30 13:46:07 UTC
Are we OK that this change only applies to the GUI, but not to the REST API?
What about the patron import?
Comment 17 Katrin Fischer 2024-08-30 14:42:40 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 18 Jake Deery 2024-09-03 08:15:03 UTC
Yes, we could probably do with a follow-up bug for this. I'll create one.
Comment 19 Jake Deery 2024-09-03 11:55:31 UTC
See bug 37815
Comment 20 Lucas Gass (lukeg) 2024-11-15 20:35:06 UTC
Enhancement wont be backpoted to 24.05.x