Bug 33815 - Crash when librarian changes their own username in the staff interface
Summary: Crash when librarian changes their own username in the staff interface
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
: 31431 (view as bug list)
Depends on:
Blocks: 33820
  Show dependency treegraph
 
Reported: 2023-05-23 18:20 UTC by Owen Leonard
Modified: 2023-12-28 20:46 UTC (History)
6 users (show)

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


Attachments
Bug 33815: Do not explode if logged in user modify their own userid (1.42 KB, patch)
2023-05-24 07:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33815: Add a hint on the patron edit form (1.37 KB, patch)
2023-05-24 07:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33815: Regression tests (1.75 KB, patch)
2023-05-24 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33815: Do not explode if logged in user modify their own userid (1.42 KB, patch)
2023-05-24 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33815: Add a hint on the patron edit form (1.37 KB, patch)
2023-05-24 14:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 33815: Regression tests (1.79 KB, patch)
2023-05-24 15:31 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33815: Do not explode if logged in user modify their own userid (1.47 KB, patch)
2023-05-24 15:31 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33815: Regression tests (1.84 KB, patch)
2023-05-26 00:04 UTC, David Cook
Details | Diff | Splinter Review
Bug 33815: Do not explode if logged in user modify their own userid (1.52 KB, patch)
2023-05-26 00:04 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2023-05-23 18:20:43 UTC
I tried in our production system (22.05.11) to change the username on my own account (an account with superlibrarian privileges) and it let to a white page blank except for the text "Internal Server Error." I wasn't able to access any page in Koha until I cleared my browser's cookies.

When I test in master I see this error:

Can't call method "password_expired" on an undefined value at /kohadevbox/koha/C4/Auth.pm line 1780

I think the problem happens only when you change the username field but you enter your existing password.
Comment 1 Jonathan Druart 2023-05-24 07:31:25 UTC
Created attachment 151609 [details] [review]
Bug 33815: Do not explode if logged in user modify their own userid

If the logged in librarian modifies their own userid they will get the
following error when submitting the form:
Can't call method "password_expired" on an undefined value at /kohadevbox/koha/C4/Auth.pm line 1780

We could handle this situation and flag the session as expired. Better
would be to deal with this specific user case and update the cookie (?)
Comment 2 Jonathan Druart 2023-05-24 07:31:28 UTC
Created attachment 151610 [details] [review]
Bug 33815: Add a hint on the patron edit form
Comment 3 Jonathan Druart 2023-05-24 07:32:00 UTC
Is a logout acceptable?
Comment 4 Jonathan Druart 2023-05-24 07:32:12 UTC
We need tests for the first patch.
Comment 5 Tomás Cohen Arazi 2023-05-24 14:55:36 UTC
Created attachment 151633 [details] [review]
Bug 33815: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2023-05-24 14:55:38 UTC
Created attachment 151634 [details] [review]
Bug 33815: Do not explode if logged in user modify their own userid

If the logged in librarian modifies their own userid they will get the
following error when submitting the form:
Can't call method "password_expired" on an undefined value at /kohadevbox/koha/C4/Auth.pm line 1780

We could handle this situation and flag the session as expired. Better
would be to deal with this specific user case and update the cookie (?)
Comment 7 Tomás Cohen Arazi 2023-05-24 14:55:41 UTC
Created attachment 151635 [details] [review]
Bug 33815: Add a hint on the patron edit form
Comment 8 Owen Leonard 2023-05-24 15:31:51 UTC
Created attachment 151642 [details] [review]
Bug 33815: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2023-05-24 15:31:54 UTC
Created attachment 151643 [details] [review]
Bug 33815: Do not explode if logged in user modify their own userid

If the logged in librarian modifies their own userid they will get the
following error when submitting the form:
Can't call method "password_expired" on an undefined value at /kohadevbox/koha/C4/Auth.pm line 1780

We could handle this situation and flag the session as expired. Better
would be to deal with this specific user case and update the cookie (?)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 David Cook 2023-05-24 23:48:04 UTC
I might have an idea for this one...
Comment 11 David Cook 2023-05-24 23:54:11 UTC
(In reply to David Cook from comment #10)
> I might have an idea for this one...

The ID that can't be changed is borrowernumber, so in check_cookie_auth() we should get $patron using $session->param('number') instead of userid/cardnumber;

But... the $session will still contain the wrong details. In theory, this same problem applies beyond just userid/id. If the user's details are changed and the session details are used anywhere else, they'll be wrong until they log out and log back in. (Of course, that's not uncommon among many systems.)
Comment 12 David Cook 2023-05-25 00:44:37 UTC
(In reply to David Cook from comment #11)
> But... the $session will still contain the wrong details. In theory, this
> same problem applies beyond just userid/id. If the user's details are
> changed and the session details are used anywhere else, they'll be wrong
> until they log out and log back in. (Of course, that's not uncommon among
> many systems.)

As I suspected, at line 559 of ./members/memberentry.pl there is a C4::Auth::haspermission check that uses C4::Context->userenv->{id} which is populated from the database session which is the wrong value, and that triggers a permission/authorization failure (if you use borrowernumber to find the patron to get through check_cookie_auth).

--

This is tricky. 

I suppose we could update/refresh the session, if we stored "updated_on" in the $session as well, and then checked that against the database at authentication time.

That said, for an authenticate related change like userid perhaps it is best to expire the session, as a critical aspect of the user account has been changed. 

If we stored "updated_on" in the $session, we could also notify users if their user id has been changed. This would probably be easier on the staff interface than the OPAC.
Comment 13 Owen Leonard 2023-05-25 14:13:08 UTC
I would be in favor of a perfect solution being a later follow-up to a good solution!
Comment 14 David Cook 2023-05-25 23:38:27 UTC
(In reply to Owen Leonard from comment #13)
> I would be in favor of a perfect solution being a later follow-up to a good
> solution!

Agreed although I don't know if there is a perfect solution for this one!

I'll look at QAing this shortly...
Comment 15 David Cook 2023-05-26 00:04:54 UTC
Created attachment 151715 [details] [review]
Bug 33815: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 16 David Cook 2023-05-26 00:04:57 UTC
Created attachment 151716 [details] [review]
Bug 33815: Do not explode if logged in user modify their own userid

If the logged in librarian modifies their own userid they will get the
following error when submitting the form:
Can't call method "password_expired" on an undefined value at /kohadevbox/koha/C4/Auth.pm line 1780

We could handle this situation and flag the session as expired. Better
would be to deal with this specific user case and update the cookie (?)

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 17 Tomás Cohen Arazi 2023-05-26 12:51:06 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 18 Laura Escamilla 2023-06-05 20:03:25 UTC
Will this be backported to 22.11.xx?
Comment 19 Matt Blenkinsop 2023-06-08 17:16:00 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 20 Katrin Fischer 2023-06-17 11:42:41 UTC
*** Bug 31431 has been marked as a duplicate of this bug. ***