Bug 40275 - Add Koha::Patrons->find_by_identifier()
Summary: Add 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: Aleisha Amohia
URL:
Keywords:
Depends on: 23771 36575
Blocks: 40286 40463
  Show dependency treegraph
 
Reported: 2025-06-30 13:46 UTC by Tomás Cohen Arazi (tcohen)
Modified: 2025-07-22 00:31 UTC (History)
9 users (show)

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


Attachments
Bug 40275: Unit tests (4.86 KB, patch)
2025-06-30 18:30 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` (2.41 KB, patch)
2025-06-30 18:30 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase (5.08 KB, patch)
2025-06-30 18:30 UTC, Tomás Cohen Arazi (tcohen)
Details | Diff | Splinter Review
Bug 40275: Unit tests (4.91 KB, patch)
2025-06-30 21:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` (2.46 KB, patch)
2025-06-30 21:41 UTC, David Nind
Details | Diff | Splinter Review
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase (5.11 KB, patch)
2025-06-30 21:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 40275: Unit tests (4.98 KB, patch)
2025-07-03 13:15 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()` (2.53 KB, patch)
2025-07-03 13:15 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase (5.20 KB, patch)
2025-07-03 13:15 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40275: Implement find_by_identifier for check_cookie_auth (1.21 KB, patch)
2025-07-03 13:24 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 40275: Implement find_by_identifier for check_cookie_auth (1.25 KB, patch)
2025-07-03 13:25 UTC, Kyle M Hall (khall)
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-06-30 13:46:04 UTC
As suggested by Frido in some other context, I file this bug to introduce the mentioned sub.

I resurrected the idea after tracking down some warnings in SIP, and identifying bug 36575 introduced them by calling `->find` without passing a primary key. This kind of errors/warns:

```
[2025/06/30 13:03:23] [24253] [ERROR] [undef]@[undef]: DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint(): Unable to satisfy requested constraint 'primary', missing values for column(s): 'borrowernumber' at /kohadevbox/koha/Koha/Objects.pm line 98 C4::SIP::Trapper::__ANON__ /kohadevbox/koha/C4/SIP/Trapper.pm (70)
```

Instead of solving it locally, I propose to introduce a sane and tested method, and reuse it anywhere this pattern is being used.
Comment 1 Tomás Cohen Arazi (tcohen) 2025-06-30 18:30:08 UTC
Created attachment 183647 [details] [review]
Bug 40275: Unit tests
Comment 2 Tomás Cohen Arazi (tcohen) 2025-06-30 18:30:11 UTC
Created attachment 183648 [details] [review]
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()`

In our codebase, there's a pattern we tend to use for searching patrons
by userid and falling back to find them by cardnumber:

* C4::Auth
* C4::SIP::ILS::Patron
* (slightly different) Koha::REST::V1::Auth::Password
* (slightly different) Koha::REST::V1::Auth
* C4::Circulation
* C4::Auth_with_cas

It generally uses the following pattern:

```perl
my $patron = Koha::Patrons->find( { userid => $identifier } );
$patron //= Koha::Patrons->find( { cardnumber => $identifier } );
```

The problem with this is that `find` actually produces a DBIX::Class
warn because `find` is being called without primary key parameters. I
haven't seen it in the wild, until the latest change in
`checkpw_internal` which made it throw warnings in SIP. My
interpretation is that SIP's special approach with the Trapper.pm class
made it show where other places just get it hidden.

That said, I implemented this using `search()` to overcome this.

To test:
1. Apply the patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 3 Tomás Cohen Arazi (tcohen) 2025-06-30 18:30:14 UTC
Created attachment 183649 [details] [review]
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase
Comment 4 Tomás Cohen Arazi (tcohen) 2025-06-30 18:47:10 UTC
# Testing notes

All seems to work. I ran the following related tests successfuly:

```
prove -r t/db_dependent/SIP \
         t/db_dependent/Circ* \
         t/db_dependent/Auth*
```

# Caveats

As I understand it, `checkpw_internal()` should be adapted to use this as well. But it implies a behavior change and I need more feedback in that.

The method seems to validate password for a patron found by `userid` and if that validation fails, it tries with the `cardnumber`. I understand we don't allow a cardnumber to match an existing userid, and the other way around. But I want to hear from other devs about it. If that was the case, we could just use `find_by_identifier()` and remove one of the blocks.
Comment 5 Nick Clemens (kidclamp) 2025-06-30 18:59:37 UTC
I think this makes sense, and I see the problem I introduced in bug 36575

The limiting of patrons to sharing cardnumber/userid hasn't been finished (bug 14323 and bug 33905) so we should not let those block this cleanup


(In reply to Tomás Cohen Arazi (tcohen) from comment #4)
> # Testing notes
> 
> All seems to work. I ran the following related tests successfuly:
> 
> ```
> prove -r t/db_dependent/SIP \
>          t/db_dependent/Circ* \
>          t/db_dependent/Auth*
> ```
> 
> # Caveats
> 
> As I understand it, `checkpw_internal()` should be adapted to use this as
> well. But it implies a behavior change and I need more feedback in that.
> 
> The method seems to validate password for a patron found by `userid` and if
> that validation fails, it tries with the `cardnumber`. I understand we don't
> allow a cardnumber to match an existing userid, and the other way around.
> But I want to hear from other devs about it. If that was the case, we could
> just use `find_by_identifier()` and remove one of the blocks.
Comment 6 David Nind 2025-06-30 21:41:56 UTC
Created attachment 183659 [details] [review]
Bug 40275: Unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 7 David Nind 2025-06-30 21:41:59 UTC
Created attachment 183660 [details] [review]
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()`

In our codebase, there's a pattern we tend to use for searching patrons
by userid and falling back to find them by cardnumber:

* C4::Auth
* C4::SIP::ILS::Patron
* (slightly different) Koha::REST::V1::Auth::Password
* (slightly different) Koha::REST::V1::Auth
* C4::Circulation
* C4::Auth_with_cas

It generally uses the following pattern:

```perl
my $patron = Koha::Patrons->find( { userid => $identifier } );
$patron //= Koha::Patrons->find( { cardnumber => $identifier } );
```

The problem with this is that `find` actually produces a DBIX::Class
warn because `find` is being called without primary key parameters. I
haven't seen it in the wild, until the latest change in
`checkpw_internal` which made it throw warnings in SIP. My
interpretation is that SIP's special approach with the Trapper.pm class
made it show where other places just get it hidden.

That said, I implemented this using `search()` to overcome this.

To test:
1. Apply the patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2025-06-30 21:42:02 UTC
Created attachment 183661 [details] [review]
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase

Signed-off-by: David Nind <david@davidnind.com>
Comment 9 David Nind 2025-06-30 21:42:31 UTC
Testing notes (using KTD):
1. Tests pass before and after the patches.
Comment 10 Tomás Cohen Arazi (tcohen) 2025-07-01 14:55:20 UTC
I wanted to mention I didn't use the new method in `C4::Auth::checkpw_internal` (yet) because the tests explicitly check the use case of matching cardnumber-userid between different users, which we really want to forbid (see bug 33905). I think making `checkpw_internal` use this method should depend on what we do on that bug report, and as a follow-up. This report stands on its own.
Comment 11 Aleisha Amohia 2025-07-03 00:01:36 UTC
This is a great fix. There's still quite a few more places in the codebase that use Koha::Patrons->find and pass in the userid or cardnumber. I started listing them all and then there were heaps and heaps ... and from what you've said here, they'll all cause those errors/warns. I feel like they should be addressed within this patchset even though I realise that creates a lot more work - otherwise it ends up being one of those situations where we have the old bad code in place at the same time as the new good code.

I'll set this to Failed QA because of the other instances where I think this needs to be implemented all at once, but happy to hear your thoughts.
Comment 12 David Cook 2025-07-03 01:37:44 UTC
(In reply to Aleisha Amohia from comment #11)
> This is a great fix. There's still quite a few more places in the codebase
> that use Koha::Patrons->find and pass in the userid or cardnumber. I started
> listing them all and then there were heaps and heaps ... and from what
> you've said here, they'll all cause those errors/warns. I feel like they
> should be addressed within this patchset even though I realise that creates
> a lot more work - otherwise it ends up being one of those situations where
> we have the old bad code in place at the same time as the new good code.
> 
> I'll set this to Failed QA because of the other instances where I think this
> needs to be implemented all at once, but happy to hear your thoughts.

Do you have some examples of some other places?

There are a number of places where I think we should actually be moving to using borrowernumber instead of cardnumber/userid, so some places might need that fix instead of this one? (For example bug 34018 or bug 39535)
Comment 13 Aleisha Amohia 2025-07-03 01:45:04 UTC
(In reply to David Cook from comment #12)
> Do you have some examples of some other places?
> 
> There are a number of places where I think we should actually be moving to
> using borrowernumber instead of cardnumber/userid, so some places might need
> that fix instead of this one? (For example bug 34018 or bug 39535)

I started making that list, and then stopped because I had the same thought about using borrowernumber instead, and being out of scope.

But as an example, there is this instance in C4::Auth->check_cookie_auth() that I'm not sure about?

1943         } elsif ($userid) {
1944             $session->param( 'lasttime', time() );
1945             my $patron = Koha::Patrons->find( { userid => $userid } );
1946 
1947             # If the user modify their own userid
1948             # Better than 500 but we could do better
1949             unless ($patron) {
1950                 $session->delete();
1951                 $session->flush;
1952                 return ( "expired", undef );
1953             }
1954 
1955             $patron = Koha::Patrons->find( { cardnumber => $userid } )
1956                 unless $patron;
Comment 14 Kyle M Hall (khall) 2025-07-03 13:15:03 UTC
Created attachment 183761 [details] [review]
Bug 40275: Unit tests

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall (khall) 2025-07-03 13:15:12 UTC
Created attachment 183762 [details] [review]
Bug 40275: Introduce `Koha::Patrons->find_by_identifier()`

In our codebase, there's a pattern we tend to use for searching patrons
by userid and falling back to find them by cardnumber:

* C4::Auth
* C4::SIP::ILS::Patron
* (slightly different) Koha::REST::V1::Auth::Password
* (slightly different) Koha::REST::V1::Auth
* C4::Circulation
* C4::Auth_with_cas

It generally uses the following pattern:

```perl
my $patron = Koha::Patrons->find( { userid => $identifier } );
$patron //= Koha::Patrons->find( { cardnumber => $identifier } );
```

The problem with this is that `find` actually produces a DBIX::Class
warn because `find` is being called without primary key parameters. I
haven't seen it in the wild, until the latest change in
`checkpw_internal` which made it throw warnings in SIP. My
interpretation is that SIP's special approach with the Trapper.pm class
made it show where other places just get it hidden.

That said, I implemented this using `search()` to overcome this.

To test:
1. Apply the patches
2. Run:
   $ ktd --shell
  k$ prove t/db_dependent/Koha/Patrons.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall (khall) 2025-07-03 13:15:14 UTC
Created attachment 183763 [details] [review]
Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall (khall) 2025-07-03 13:24:11 UTC
Created attachment 183764 [details] [review]
Bug 40275: Implement find_by_identifier for check_cookie_auth
Comment 18 Kyle M Hall (khall) 2025-07-03 13:25:22 UTC
Created attachment 183765 [details] [review]
Bug 40275: Implement find_by_identifier for check_cookie_auth

Test Plan:
1) prove t/db_dependent/Auth.t
Comment 19 David Cook 2025-07-04 03:54:21 UTC
(In reply to Aleisha Amohia from comment #13)
> (In reply to David Cook from comment #12)
> > Do you have some examples of some other places?
> > 
> > There are a number of places where I think we should actually be moving to
> > using borrowernumber instead of cardnumber/userid, so some places might need
> > that fix instead of this one? (For example bug 34018 or bug 39535)
> 
> I started making that list, and then stopped because I had the same thought
> about using borrowernumber instead, and being out of scope.
> 
> But as an example, there is this instance in C4::Auth->check_cookie_auth()
> that I'm not sure about?
> 
> 1943         } elsif ($userid) {
> 1944             $session->param( 'lasttime', time() );
> 1945             my $patron = Koha::Patrons->find( { userid => $userid } );
> 1946 
> 1947             # If the user modify their own userid
> 1948             # Better than 500 but we could do better
> 1949             unless ($patron) {
> 1950                 $session->delete();
> 1951                 $session->flush;
> 1952                 return ( "expired", undef );
> 1953             }
> 1954 
> 1955             $patron = Koha::Patrons->find( { cardnumber => $userid } )
> 1956                 unless $patron;

Yeah, I'm pretty sure this is one of those places we should be using borrowernumber (ie $session->param('number') rather than the userid in $session->param('id')) instead. The current lookup by userid and cardnumber there is... not good.
Comment 20 David Cook 2025-07-04 03:59:45 UTC
Ohhh I just saw that Kyle added the patch for Koha::Patrons->find_by_identifier to check_cookie_auth in response to Aleisha's comment. 

Yeah, rather than doing that let's do something more like this:

my $patron = Koha::Patrons->find({ borrowernumber => $session->param('number') });

That's going to be way better. 

--

I'm just going to quickly pop this to "In Discussion". 

Do we want to drop the patch "Implement find_by_identifier for check_cookie_auth" and handle the move to borrowernumber on a separate bug? 

Alternatively, we leave the patch in, but knowing that it really should be replaced with a borrowernumber lookup? 

I think Aleisha raises a really interesting point here.
Comment 21 David Cook 2025-07-04 04:06:11 UTC
(In reply to David Cook from comment #20)
> I'm just going to quickly pop this to "In Discussion". 
> 
> Do we want to drop the patch "Implement find_by_identifier for
> check_cookie_auth" and handle the move to borrowernumber on a separate bug? 
> 
> Alternatively, we leave the patch in, but knowing that it really should be
> replaced with a borrowernumber lookup? 
> 
> I think Aleisha raises a really interesting point here.

FYI I'm just looking for something like "Yeah, let's handle it on a separate bug", or "No, maybe we should handle it here". Not trying to derail the progress here.

Might be worthwhile just opening an Omnibus bug for the "Use borrowernumber instead of userid/cardnumber where possible", and then we raise a child report for "check_cookie_auth" (and all the other bugs I've reported ont his topic). I think I'm probably inclined to go that direction myself, but curious what other people think.
Comment 22 Aleisha Amohia 2025-07-06 21:10:57 UTC
(In reply to David Cook from comment #21)
>
> FYI I'm just looking for something like "Yeah, let's handle it on a separate
> bug", or "No, maybe we should handle it here". Not trying to derail the
> progress here.
> 
> Might be worthwhile just opening an Omnibus bug for the "Use borrowernumber
> instead of userid/cardnumber where possible", and then we raise a child
> report for "check_cookie_auth" (and all the other bugs I've reported ont his
> topic). I think I'm probably inclined to go that direction myself, but
> curious what other people think.

I agree with this - I don't want to block progress here but would like to see intention for the other cases to get handled somehow
Comment 23 Tomás Cohen Arazi (tcohen) 2025-07-17 12:27:44 UTC
Hi all.

This patch set addresses a pattern that we use in the codebase and creates a fully tested method for reusing it.

As with many places in Koha, old code that's been patched for years hides bugs, and small refactoring intentions end up being a pain because 'why not fix it all'.

I agree and have always tried to fix all the things, but let's make it a separate bug report. I would like to work on that, but it is better to have a well summarized description of the problem to be solved. And some places are so tricky we should probably have an individual bug for them.

In the case of `check_cookie_auth`, I'd say that fallback mechanism is hiding another bug somewhere else. Because it was probably added as a workaround. As we should really just have the borrowernumber there... so a longer conversation that should happen in a separate report. Thanks Kyle anyway!

Setting this back to SO so we can move forward.

Can someone file a follow-up bug in which we can discuss the other issues? Happy to work on them!
Comment 24 David Cook 2025-07-21 03:54:13 UTC
(In reply to Tomás Cohen Arazi (tcohen) from comment #23)
> This patch set addresses a pattern that we use in the codebase and creates a
> fully tested method for reusing it.

Definitely a good move forward.
 
> As with many places in Koha, old code that's been patched for years hides
> bugs, and small refactoring intentions end up being a pain because 'why not
> fix it all'.
> 
> I agree and have always tried to fix all the things, but let's make it a
> separate bug report. I would like to work on that, but it is better to have
> a well summarized description of the problem to be solved. And some places
> are so tricky we should probably have an individual bug for them.

Agreed about making separate bug reports. Trying to fix all the things at once rarely works. Just want to make sure all the key people know there are other bug reports to consider.

> In the case of `check_cookie_auth`, I'd say that fallback mechanism is
> hiding another bug somewhere else. Because it was probably added as a
> workaround. As we should really just have the borrowernumber there... so a
> longer conversation that should happen in a separate report. Thanks Kyle
> anyway!

+1

> Setting this back to SO so we can move forward.
> 
> Can someone file a follow-up bug in which we can discuss the other issues?
> Happy to work on them!

I've raised bug 40457 and I'll try to attach all related reports I know about