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.
Created attachment 183647 [details] [review] Bug 40275: Unit tests
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
Created attachment 183649 [details] [review] Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase
# 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.
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.
Created attachment 183659 [details] [review] Bug 40275: Unit tests Signed-off-by: David Nind <david@davidnind.com>
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>
Created attachment 183661 [details] [review] Bug 40275: Use Koha::Patrons->find_by_identifier in the codebase Signed-off-by: David Nind <david@davidnind.com>
Testing notes (using KTD): 1. Tests pass before and after the patches.
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.
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.
(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)
(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;
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>
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>
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>
Created attachment 183764 [details] [review] Bug 40275: Implement find_by_identifier for check_cookie_auth
Created attachment 183765 [details] [review] Bug 40275: Implement find_by_identifier for check_cookie_auth Test Plan: 1) prove t/db_dependent/Auth.t
(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.
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.
(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.
(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
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!
(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