Bug 40286

Summary: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier
Product: Koha Reporter: Tomás Cohen Arazi (tcohen) <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi (tcohen) <tomascohen>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: dcook, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905
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:
Bug Depends on: 40275    
Bug Blocks:    
Attachments: Bug 40286: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier
Bug 40286: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier

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>