Bug 40286 - Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier
Summary: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Tomás Cohen Arazi (tcohen)
QA Contact: Testopia
URL:
Keywords:
Depends on: 40275
Blocks:
  Show dependency treegraph
 
Reported: 2025-07-01 15:05 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-08-14 22:08 UTC (History)
2 users (show)

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


Attachments
Bug 40286: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier (4.03 KB, patch)
2025-07-21 17:00 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40286: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier (4.07 KB, patch)
2025-08-14 22:08 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi (tcohen) 2025-07-01 15:05:21 UTC
Bug 40275 introduced this method and this is the missing place for refactoring.

The current tests for this method include the creation of actually conflicting patrons, matching their userid and cardnumber respectively. As making the proposed change is indeed a behavior change, I file this report in order to discuss how to handle it properly. The other use cases are clear and already covered in bug 40275.

The blocker for this is bug 33905, in which we need to decide how to ensure identifier uniqueness, and then get rid of the tests.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-07-21 17:00:16 UTC
Created attachment 184448 [details] [review]
Bug 40286: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier

This patch does what the title says. The code change is clear to
understand.

The check for userid vs. cardnumber is done in the `find_by_identifier`
method so no need to do it here.

Note: When `find_by_identifier` was written, I picked some other place's
logic (API password validation) so `userid` is used first for finding
the patron. Because of this, tests in t/db_dependent/Auth.t need to be
adjusted. I believe this is not an issue, as those tests should be
removed once we move forward with bug 33905, entirely.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 David Cook 2025-07-22 00:26:54 UTC
I'm not sure I understand the issue but sounds good to me?

Alternatively, we could use this as an opportunity to do some refactoring.

--

checkpw_internal() looks like it's used one place in C4::Auth::checkpw() and one place in C4::Auth_with_ldap::_do_changepassword().

And checkpw_internal() looks like it could be decomposed. At the moment, we're doing 3 things inside it. 

1. Find patron (but Koha::Patrons->find_by_identifier() can already do this outside this function)
2. Check the password (via checkpw_hash())
3. Set the userenv unless $no_set_userenv

If we created a new function "C4::Auth::set_userenv({ patron => $patron })" or "C4::Context->set_userenv_from_patron" or something like that, then we don't really need checkpw_internal(). 

--

In the case of C4::Auth_with_ldap, we actually already have a borrowernumber and patron object before we even call checkpw_internal() anyway. We just need checkpw_hash() and maybe the userenv setting. The code in C4::Auth_wiith_ldap could use some TLC...
Comment 3 David Cook 2025-07-22 00:28:42 UTC
For C4::Auth_with_ldap, I've also raised bug 40463
Comment 4 David Cook 2025-07-22 00:31:25 UTC
Honestly, the C4::Context->set_userenv in C4::Auth could use a bit of love too. But one mouthful of elephant at a time...
Comment 5 David Nind 2025-08-14 22:08:13 UTC
Created attachment 185433 [details] [review]
Bug 40286: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier

This patch does what the title says. The code change is clear to
understand.

The check for userid vs. cardnumber is done in the `find_by_identifier`
method so no need to do it here.

Note: When `find_by_identifier` was written, I picked some other place's
logic (API password validation) so `userid` is used first for finding
the patron. Because of this, tests in t/db_dependent/Auth.t need to be
adjusted. I believe this is not an issue, as those tests should be
removed once we move forward with bug 33905, entirely.

To test:
1. Apply this patch
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Auth.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: David Nind <david@davidnind.com>