Bug 30700 - Patrons who can log into staff interface should be able to change their own password
Summary: Patrons who can log into staff interface should be able to change their own p...
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-05-05 22:46 UTC by Aleisha Amohia
Modified: 2023-10-13 23:53 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 30700: Allow staff users to change their password via staff client (6.38 KB, patch)
2022-05-06 02:30 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: Allow staff users to change their password via staff client (6.41 KB, patch)
2022-05-09 04:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword (6.90 KB, patch)
2022-05-18 04:25 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: Allow staff users to change their password via staff client (6.44 KB, patch)
2022-05-18 21:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword (6.90 KB, patch)
2022-05-18 21:21 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword (6.90 KB, patch)
2022-05-19 01:49 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: Allow staff users to change their password via staff client (6.54 KB, patch)
2023-07-10 04:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword (6.86 KB, patch)
2023-07-10 04:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: (follow-up) Redirect to patron page after changing password (1.34 KB, patch)
2023-07-10 04:11 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 30700: Allow staff users to change their password via staff client (6.57 KB, patch)
2023-07-10 17:23 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword (6.89 KB, patch)
2023-07-10 17:23 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 30700: (follow-up) Redirect to patron page after changing password (1.38 KB, patch)
2023-07-10 17:23 UTC, Sam Lau
Details | Diff | Splinter Review
Bug 30700: Allow staff users to change their password via staff client (7.00 KB, patch)
2023-10-04 05:16 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword (6.89 KB, patch)
2023-10-04 05:16 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30700: (follow-up) Redirect to patron page after changing password (1.38 KB, patch)
2023-10-04 05:16 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword (6.95 KB, patch)
2023-10-04 05:18 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 30700: (follow-up) Redirect to patron page after changing password (1.44 KB, patch)
2023-10-04 05:18 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2022-05-05 22:46:01 UTC
Patrons must have the additional borrowers->edit_borrowers subpermission to change other patron's passwords.

This is fine, except if a patron has permission to log into the staff client but doesn't have borrowers permissions, they cannot change their own password via the staff client.
Comment 1 David Cook 2022-05-05 23:44:58 UTC
That's a really good point. That would be a great enhancement. I'm not a fan of directing staff to the OPAC for things like password changes.
Comment 2 Aleisha Amohia 2022-05-06 02:30:34 UTC Comment hidden (obsolete)
Comment 3 David Nind 2022-05-09 04:18:15 UTC Comment hidden (obsolete)
Comment 4 Andrew Fuerste-Henry 2022-05-10 12:10:36 UTC
I don't know that we can assume any user that can log into the staff client should be able to change their own password. For example, many libraries create shared logins for the circulation desk, where multiple people use the same account and any individual changing the password would be quite disruptive.

Whether or not users can change their password via the OPAC is governed by the OpacPasswordChange system preference. This enhancement should contain either a comparable syspref for the staff interface or a new user permission to change one's own password.
Comment 5 Aleisha Amohia 2022-05-10 21:53:00 UTC
Having multiple people using one account sounds like it's own security risk... I don't think we should compromise on functionality to support that use case.

The real issue is what David has mentioned in Comment 1. If a person can log into the staff client, it's illogical to then have to direct them to the OPAC (another site) to change their own password. Many Koha libraries don't use the OPAC, or if they do, they may have OpacPasswordChange disabled, and then what? 

Additionally, I can't think of any online accounts I have where I can't change my own password on the site I've logged into. I don't believe we need to wrap this in a syspref or new permission.
Comment 6 David Cook 2022-05-11 00:19:47 UTC
(In reply to Aleisha Amohia from comment #5)
> Having multiple people using one account sounds like it's own security
> risk... I don't think we should compromise on functionality to support that
> use case.
 
Agreed. While account sharing is a common practice, it isn't one that we should encourage or support. 

(In reply to Andrew Fuerste-Henry from comment #4)
> Whether or not users can change their password via the OPAC is governed by
> the OpacPasswordChange system preference. This enhancement should contain
> either a comparable syspref for the staff interface or a new user permission
> to change one's own password.

That's a good point too though. I suppose it is a fairly significant feature change, and we do tend to make new features opt-in.
Comment 7 Andrew Fuerste-Henry 2022-05-11 13:38:42 UTC
> I suppose it is a fairly significant feature
> change, and we do tend to make new features opt-in.

If nothing else, this is true. You are proposing a large behavior change. Our established practice is to make such things optional.
Comment 8 Aleisha Amohia 2022-05-18 04:25:05 UTC Comment hidden (obsolete)
Comment 9 Séverine Queune 2022-05-18 09:11:05 UTC
Hi Aleisha,
I got this error trying to install the patch :

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 30700: Allow staff users to change their password via staff client
Applying: Bug 30700: (follow-up) Add system preference StaffLoginResetPassword
error: sha1 information is lacking or useless (installer/data/mysql/mandatory/sysprefs.sql).
error: could not build fake ancestor
Patch failed at 0001 Bug 30700: (follow-up) Add system preference StaffLoginResetPassword
hint: Use 'git am --show-current-patch=diff' to see the failed patch

I'd be happy to test this new feature when it's fixed !
Comment 10 Aleisha Amohia 2022-05-18 21:21:44 UTC
Created attachment 135173 [details] [review]
Bug 30700: Allow staff users to change their password via staff client

Patrons who can log into the staff client (have the 'catalogue'
permission) should be able to change their own password.

To test:
1) Create a user with only 'catalogue' permissions (Patron A)
2) Log in to the staff client as Patron A
3) Click the menu with your username in the top-right of the window.
Click the 'your account' menu link.
4) Confirm you are forced to a login screen, so you cannot view your
account, which is where the 'change password' link is normally found.
5) Try to access the page to change your password directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap Patron A's borrowernumber in). Confirm you are forced to a login
screen.
6) Apply this patch and restart services. Go back to the mainpage logged
in as Patron A.
7) Click the menu with your username in the top-right of the window.
Notice there is now a 'Change password' menu link. Go to 'change
password'.
8) Confirm you are now shown a page to change your password. Change your
password, and confirm you are redirect to the mainpage.
9) Try to access the page to change someone else's page directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap some other borrowernumber in). Confirm you are redirected to a
404.
10) Log out and log back in as your original borrower. Confirm you are
able to change your password as normal.

Sponsored-by: Education Services Australia SCIS

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Aleisha Amohia 2022-05-18 21:21:49 UTC Comment hidden (obsolete)
Comment 12 Aleisha Amohia 2022-05-19 01:49:54 UTC
Created attachment 135176 [details] [review]
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword

This syspref wraps around the functionality added for patrons with the
'catalogue' permission to reset their own password via the staff client.
Comment 13 Owen Leonard 2022-05-20 20:06:46 UTC
I don't understand what's going on here, which may just be me:

if ( !$patron->has_permission({ borrowers => 'edit_borrowers' }) ) {
    print $input->redirect("/cgi-bin/koha/mainpage.pl");
}

But when I change a patron's password when logged in as a superlibrarian I'm redirected to the main page rather than back to their account.
Comment 14 David Cook 2022-05-23 00:11:35 UTC
(In reply to Owen Leonard from comment #13)
> I don't understand what's going on here, which may just be me:
> 
> if ( !$patron->has_permission({ borrowers => 'edit_borrowers' }) ) {
>     print $input->redirect("/cgi-bin/koha/mainpage.pl");
> }
> 
> But when I change a patron's password when logged in as a superlibrarian I'm
> redirected to the main page rather than back to their account.

Looks like Koha::Patron->has_permission uses C4::Auth::haspermission, which has this line:

return $flags if $flags->{superlibrarian};

So if you're a superlibrarian then that should be returning true.
Comment 15 Aleisha Amohia 2022-05-24 01:37:09 UTC
(In reply to Owen Leonard from comment #13)
> But when I change a patron's password when logged in as a superlibrarian I'm
> redirected to the main page rather than back to their account.

Sorry Owen I wasn't able to reproduce this ... When I'm logged in as a superlibrarian and click Change password I'm not redirected to the mainpage. Then when I reset the password, it is successful and I'm still not redirected to the mainpage. Can you please test again?
Comment 16 Lucas Gass 2022-05-31 21:34:10 UTC
> But when I change a patron's password when logged in as a superlibrarian I'm
> redirected to the main page rather than back to their account.

This is the behavior I get as well. When I reset someone else's password as a superlibrarian I am redirected back to the staff main page.
Comment 17 Hammat wele 2022-11-18 20:29:02 UTC
After apply this patch:
- When i click on the menu with my username 'Patran A' in the top-right there is no link 'Change password' as expected.
- I can directly access to http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X and change the 'Patran A' password
- But i can also access to http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X of other users and i'm not redirected to a 404 page as expected.
Comment 18 Aleisha Amohia 2023-07-10 04:11:41 UTC
Created attachment 153248 [details] [review]
Bug 30700: Allow staff users to change their password via staff client

Patrons who can log into the staff client (have the 'catalogue'
permission) should be able to change their own password.

To test:
0) Apply the patch, install database updates, restart services. Go to System preferences and enable the StaffLoginResetPassword system preference.
1) Create a user with only 'catalogue' permissions (Patron A)
2) Log in to the staff client as Patron A
3) Click the menu with your username in the top-right of the window.
Click the 'your account' menu link.
4) Confirm you are forced to a login screen, so you cannot view your
account, which is where the 'change password' link is normally found.
5) Try to access the page to change your password directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap Patron A's borrowernumber in). Confirm you are forced to a login
screen.
6) Apply this patch and restart services. Go back to the mainpage logged
in as Patron A.
7) Click the menu with your username in the top-right of the window.
Notice there is now a 'Change password' menu link. Go to 'change
password'.
8) Confirm you are now shown a page to change your password. Change your
password, and confirm you are redirect to the mainpage.
9) Try to access the page to change someone else's page directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap some other borrowernumber in). Confirm you are redirected to a
404.
10) Log out and log back in as your original borrower. Confirm you are
able to change your password as normal.

Sponsored-by: Education Services Australia SCIS

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 Aleisha Amohia 2023-07-10 04:11:45 UTC
Created attachment 153249 [details] [review]
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword

This syspref wraps around the functionality added for patrons with the
'catalogue' permission to reset their own password via the staff client.
Comment 20 Aleisha Amohia 2023-07-10 04:11:48 UTC
Created attachment 153250 [details] [review]
Bug 30700: (follow-up) Redirect to patron page after changing password

This patch ensures the logged in user is redirected to the patron detail page after successfully changing another patron's password (rather than the homepage).
Comment 21 Sam Lau 2023-07-10 17:23:08 UTC
Created attachment 153290 [details] [review]
Bug 30700: Allow staff users to change their password via staff client

Patrons who can log into the staff client (have the 'catalogue'
permission) should be able to change their own password.

To test:
0) Apply the patch, install database updates, restart services. Go to System preferences and enable the StaffLoginResetPassword system preference.
1) Create a user with only 'catalogue' permissions (Patron A)
2) Log in to the staff client as Patron A
3) Click the menu with your username in the top-right of the window.
Click the 'your account' menu link.
4) Confirm you are forced to a login screen, so you cannot view your
account, which is where the 'change password' link is normally found.
5) Try to access the page to change your password directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap Patron A's borrowernumber in). Confirm you are forced to a login
screen.
6) Apply this patch and restart services. Go back to the mainpage logged
in as Patron A.
7) Click the menu with your username in the top-right of the window.
Notice there is now a 'Change password' menu link. Go to 'change
password'.
8) Confirm you are now shown a page to change your password. Change your
password, and confirm you are redirect to the mainpage.
9) Try to access the page to change someone else's page directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap some other borrowernumber in). Confirm you are redirected to a
404.
10) Log out and log back in as your original borrower. Confirm you are
able to change your password as normal.

Sponsored-by: Education Services Australia SCIS

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 22 Sam Lau 2023-07-10 17:23:11 UTC
Created attachment 153291 [details] [review]
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword

This syspref wraps around the functionality added for patrons with the
'catalogue' permission to reset their own password via the staff client.

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 23 Sam Lau 2023-07-10 17:23:14 UTC
Created attachment 153292 [details] [review]
Bug 30700: (follow-up) Redirect to patron page after changing password

This patch ensures the logged in user is redirected to the patron detail page after successfully changing another patron's password (rather than the homepage).

Signed-off-by: Sam Lau <samalau@gmail.com>
Comment 24 Victor Grousset/tuxayo 2023-10-04 05:16:07 UTC
Created attachment 156512 [details] [review]
Bug 30700: Allow staff users to change their password via staff client

Patrons who can log into the staff client (have the 'catalogue'
permission) should be able to change their own password.

To test:
0) Apply the patch, install database updates, restart services. Go to System preferences and enable the StaffLoginResetPassword system preference.
1) Create a user with only 'catalogue' permissions (Patron A)
2) Log in to the staff client as Patron A
3) Click the menu with your username in the top-right of the window.
Click the 'your account' menu link.
4) Confirm you are forced to a login screen, so you cannot view your
account, which is where the 'change password' link is normally found.
5) Try to access the page to change your password directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap Patron A's borrowernumber in). Confirm you are forced to a login
screen.
6) Apply this patch and restart services. Go back to the mainpage logged
in as Patron A.
7) Click the menu with your username in the top-right of the window.
Notice there is now a 'Change password' menu link. Go to 'change
password'.
8) Confirm you are now shown a page to change your password. Change your
password, and confirm you are redirect to the mainpage.
9) Try to access the page to change someone else's page directly
http://localhost:8081/cgi-bin/koha/members/member-password.pl?member=X
(swap some other borrowernumber in). Confirm you are redirected to a
404.
10) Log out and log back in as your original borrower. Confirm you are
able to change your password as normal.

Sponsored-by: Education Services Australia SCIS

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Sam Lau <samalau@gmail.com>
Rebased-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Victor Grousset/tuxayo 2023-10-04 05:16:10 UTC Comment hidden (obsolete)
Comment 26 Victor Grousset/tuxayo 2023-10-04 05:16:13 UTC Comment hidden (obsolete)
Comment 27 Victor Grousset/tuxayo 2023-10-04 05:18:29 UTC
Created attachment 156515 [details] [review]
Bug 30700: (follow-up) Add system preference StaffLoginResetPassword

This syspref wraps around the functionality added for patrons with the
'catalogue' permission to reset their own password via the staff client.

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 28 Victor Grousset/tuxayo 2023-10-04 05:18:36 UTC
Created attachment 156516 [details] [review]
Bug 30700: (follow-up) Redirect to patron page after changing password

This patch ensures the logged in user is redirected to the patron detail page after successfully changing another patron's password (rather than the homepage).

Signed-off-by: Sam Lau <samalau@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 29 Victor Grousset/tuxayo 2023-10-04 05:39:22 UTC
Works as expected, here is a 2nd signoff :)

I couldn't get it to change the password of another user via editing and resending the POST query 😈 and the code looks well guarded against that 👍

Code looks good functionally.

I found this:
`print $input->redirect(...)` followed by `exit;` is used in many other pages to make guard closes. So it seems to me we can avoid wrapping the whole happy path in an `else {...}`

That also avoids the need to indent tons of lines. Or to choose to not do it which makes perltidy complain a lot. And about much more than actually changed lines.

So just some polishing and it will be good :)
I'll go ask about what should be the default value of StaffLoginResetPassword for new installs and existing installs.
Comment 30 Marcel de Rooy 2023-10-06 06:42:43 UTC
The approach here feels a bit like a workaround.

member-password:
        flagsrequired   => { borrowers => 'edit_borrowers' },
Shouldnt we lower that permission?

And then there is moremember. It also needs edit_borrowers. Which is a bit weird for your own account.
Should we rework the checks there a bit to include seeing your account (including password change)?

If we do so, there is no need to add another link (we already have my account).
And no pref is needed as well.

Currently, you can set a user to Staff access. He has the account link. But if he clicks, he gets No permission. Not user friendly.
Comment 31 Katrin Fischer 2023-10-06 11:10:28 UTC
(In reply to Marcel de Rooy from comment #30)
> The approach here feels a bit like a workaround.
> 
> member-password:
>         flagsrequired   => { borrowers => 'edit_borrowers' },
> Shouldnt we lower that permission?

Maybe we could make it 'edit_borrowers or circulate_remaining_permissions' which are the 2 permissions most of the pages use that have the 'change password' button. We don't have a real 'view borrowers' right now.

> And then there is moremember. It also needs edit_borrowers. Which is a bit
> weird for your own account.
> Should we rework the checks there a bit to include seeing your account
> (including password change)?

We could make it so you can see your own with catalogue maybe, but it feels like this would be for a separate bug. 2FA uses catalog - but how do you get there in the first place if you can't access pages that have the button?

> If we do so, there is no need to add another link (we already have my
> account).
> And no pref is needed as well.

I think allowing a user to change their own password could be done without a preference. Since we started using Koha this has only caused confusion.

> Currently, you can set a user to Staff access. He has the account link. But
> if he clicks, he gets No permission. Not user friendly.

I think we should try not to get out of scope here and maybe move some weirdness to a separate bug.

But adjusting the permission checks and maybe even forego the pref would make sense to me.
Comment 32 Aleisha Amohia 2023-10-08 19:19:31 UTC
Cool that approach sounds fine.

I won't have capacity to work on a new approach so I've unassigned myself, if anyone is able please feel free to implement.