Bug 34610 - ProtectSuperlibrarianPrivileges, not ProtectSuperlibrarian
Summary: ProtectSuperlibrarianPrivileges, not ProtectSuperlibrarian
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal
Assignee: Noémie Ariste
QA Contact: Emily Lamancusa
URL:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2023-08-24 15:27 UTC by Magnus Enger
Modified: 2024-08-16 15:37 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: String patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:


Attachments
Bug 34610: Udate deprecated ProtectSuperlibrarian string (1.66 KB, patch)
2024-07-26 00:00 UTC, Koha Training Catalyst
Details | Diff | Splinter Review
Bug 34610: Update deprecated ProtectSuperlibrarian string (1.74 KB, patch)
2024-08-09 15:12 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 34610: Update deprecated ProtectSuperlibrarian string (1.81 KB, patch)
2024-08-14 15:36 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34610: Update deprecated ProtectSuperlibrarian string (1.80 KB, patch)
2024-08-14 15:38 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 34610: Update deprecated ProtectSuperlibrarian string (1.81 KB, patch)
2024-08-16 15:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Enger 2023-08-24 15:27:38 UTC
We have a syspref called ProtectSuperlibrarianPrivileges, but a couple of places in the code refer to ProtectSuperlibrarian:

$ grep -rn --exclude=*.po "ProtectSuperlibrarian " *
koha-tmpl/intranet-tmpl/prog/en/modules/members/member-flags.tt:132:                    .attr("title", _("The system preference ProtectSuperlibrarian is enabled"))
koha-tmpl/intranet-tmpl/prog/nb-NO/modules/members/member-flags.tt:132:                    .attr("title", _("The system preference ProtectSuperlibrarian is enabled"))
Comment 1 Koha Training Catalyst 2024-07-26 00:00:01 UTC
Created attachment 169633 [details] [review]
Bug 34610: Udate deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers and permissions permissions to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the superlibrarian permission checkbox.
6. Check the message that appears contains ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian
Comment 2 David Nind 2024-07-26 05:25:25 UTC
I tried to get the hover message with the incorrect text ("The system preference ProtectSuperlibrarian is enabled") to display before applying the patch, but I wasn't able to. (I normally do this so that I can show that the patch fixed the issue.)

I think something else is going on here, and I don't think the ProtectSuperlibrarianPrivileges system preference is working as it should.

So I will leave for someone else to test that has a better understanding of the code and how permissions work.

A couple of other notes:
1. The commit message title needs updating "..Udate.." to "...Update...".
2. To get credit on the dashboard (https://dashboard.koha-community.org/), and to be listed as a Koha developer on the Koha timeline page, please update the assignee for the bug.

How I tested
============

This is what I did, with and without the patch applied, as per the test plan (using koha-testing docker (KTD) and the sample data already set up):
1. Set the ProtectSuperlibrarianPrivileges system preference to "Allow only superlibrarians",
2. Set up permissions to test: [1]
   - User 1 = Henry Acevedo (permissions: superlibrarian permission)
   - User 2 = Mary Burton (permissions: catalogue, borrowers, and permissions), change password so you can log in as Mary
   - User 3 = Edna Acosta (permissions: catalogue), so can test anotehr staff patron with minimal access
3. If I login as User 2 (Mary) and go to edit the permissions for User 1 (Henry), I get the login form with the message "Error: You do not have permission to access this page."
  . This seems correct, Mary has the permission permission, but is not a superlibrarian. As ProtectSuperlibrarianPrivileges is set to "[Allow only superlibrarians] to access/change superlibrarian privileges." (step 1 of the test plan), she should not be able to change the permissions for superlibrarians. So she can't view the permissions page (so can't change them).
  . Maybe another bug is preventing User 2 (Mary) from viewing User 1's (Henry's) permissions?
  . Maybe it should be showing the permissions, but not them be changed and display the incorrect hover message?
4. If I repeat step 3, but try to add super librarian permissions to a staff patron who is not a a super librarian (User 3, Edna Acosta), I get the same error as I got for Henery in step 3 above.
5. If I try to set the permissions for another non-staff patron (I chose Lisa Charles, ), I get the hover message but it has the correct system preference name.
6. If I repeat steps 3 to 5, with ProtectSuperlibrarianPrivileges set to "Allow all permitted users", Mary should be able to change the permissions for superlibrarians and add superlibrarian privileges to other patrons:
   . Mary trying to change Henry's permissions: can't change, get "Error: You do not have permission to access this page."
   . Mary trying to change Edna's permissions: can't change get "Error: You do not have permission to access this page."
   . Mary trying to change Lisa's permissions (K - Kid patron category): I can change to a superlibrarian!
7. I applied the patch and repeated the steps - I got the same results.

NOTES: 
1. I logged Mary out each time I changed any permissions or system preference values.
2. If I applied the patch, and did a restart_all, for step 4 above, I get the login form and "Error: You do not have permission to access this page.".

[1] Permission names and codes
------------------------------

- Access to all librarian functions (superlibrarian)
- Staff access, allows viewing of catalogue in staff interface (catalogue)
- Add, modify and view patron information (borrowers)
- Set user permissions (permissions)
Comment 3 Owen Leonard 2024-08-09 15:12:41 UTC
Created attachment 170201 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the superlibrarian permission checkbox.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 4 Owen Leonard 2024-08-09 15:14:39 UTC
(In reply to David Nind from comment #2)
>    - User 2 = Mary Burton (permissions: catalogue, borrowers, and
> permissions)

I think the test plan was incorrect and I have modified it: I think the testing process assumes that the user also has the 'staffaccess' permission: "Allow staff members to modify permissions, usernames, and passwords for other staff members."

In any case the patch is correct and if there is another issue with this permission we can file a new bug.
Comment 5 Emily Lamancusa 2024-08-14 15:36:48 UTC
Created attachment 170321 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the superlibrarian permission checkbox.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 6 Emily Lamancusa 2024-08-14 15:38:30 UTC
Created attachment 170322 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the Select all and Clear all links.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 7 Emily Lamancusa 2024-08-14 15:41:02 UTC
I made one more edit to the test plan - the deprecated string was in the tooltip message for the Select all / Clear all buttons, not the superlibrarian permissions checkbox. Otherwise, everything looks good! Passing QA
Comment 8 Katrin Fischer 2024-08-16 15:15:30 UTC
I've updated the author line in the patch to:
Author: Noémie Ariste <noemie.ariste@catalyst.net.nz>

Thanks Noémie and welcome as a new Koha developer!
Comment 9 Katrin Fischer 2024-08-16 15:16:21 UTC
Created attachment 170455 [details] [review]
Bug 34610: Update deprecated ProtectSuperlibrarian string

Test:
1. Set ProtectSuperlibrarianPrivileges system preference to Allow only
   superlibrarians
2. Give superlibrarian permissions to a user (User 1)
3. Give catalogue, borrowers, permissions, and staffaccess  permissions
   to a second user (User 2)
3. Login as User 2
4. Go to User 1 permission settings page.
5. Hover over the Select all and Clear all links.
6. Check the message that appears contains
   ProtectSuperlibrarianPrivileges and not ProtectSuperlibrarian

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 10 Katrin Fischer 2024-08-16 15:37:00 UTC
Pushed for 24.11!

Well done everyone, thank you!