[Bug 40286] New: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Bug ID: 40286 Summary: Make C4::Auth::checkpw_internal use Koha::Patrons->find_by_identifier Change sponsored?: --- Product: Koha Version: Main Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |40275, 33905 CC| |tomascohen@gmail.com Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 [Bug 33905] Username and cardnumber should be unique respectively https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 [Bug 40275] Add Koha::Patrons->find_by_identifier() -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #1 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 184448 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=184448&action=edit 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> -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #2 from David Cook <dcook@prosentient.com.au> --- 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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #3 from David Cook <dcook@prosentient.com.au> --- For C4::Auth_with_ldap, I've also raised bug 40463 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #4 from David Cook <dcook@prosentient.com.au> --- Honestly, the C4::Context->set_userenv in C4::Auth could use a bit of love too. But one mouthful of elephant at a time... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|33905 | See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33905 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33905 [Bug 33905] A patron's username and cardnumber must each not be used by another patron in any permutation -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #184448|0 |1 is obsolete| | --- Comment #5 from David Nind <david@davidnind.com> --- Created attachment 185433 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=185433&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #185433|0 |1 is obsolete| | --- Comment #6 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 186764 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=186764&action=edit 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> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@openfifth.c |y.org |o.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |25.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #7 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Nice work everyone! Pushed to main for 25.11 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed CC| |lucas@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Please do not backport this one. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #9 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- This is causing t/db_dependent/api/v1/password_validation.t to fail now on main -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Jonathan Druart <jonathan.druart@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@gmail.com See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=36575 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #10 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- Until we find a solution to fix the failing tests I am reverting. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Failed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Bug 40286 depends on bug 40275, which changed state. Bug 40275 Summary: Add Koha::Patrons->find_by_identifier() https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40275 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #11 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Are you intending to revisit this one Tomas? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #12 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 195344 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=195344&action=edit Bug 40286: (follow-up) Fix password_validation.t for find_by_identifier behavior The change to use find_by_identifier in checkpw_internal means userid is now checked before cardnumber. This updates the test to reflect that when an identifier matches both a userid and cardnumber, the userid match takes precedence. To test: 1. Apply this patch 2. Run: $ ktd --shell k$ prove t/db_dependent/api/v1/password_validation.t => SUCCESS: Tests pass! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|additional_work_needed | --- Comment #13 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- I'm really sorry I missed to follow-up on this one. The behavior change in `checkpw_internal` (userid compared before cardnumber) required a minor adjustment in those tests and I missed it. Best regards. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |42043 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42043 [Bug 42043] SIP recording DBIX class errors -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #14 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Nice one Tomas, thanks for the follow-up -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |Consistently use release notes| |find_by_identifier for | |authentication. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)|25.11.00 |26.05.00,25.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 --- Comment #15 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Thanks for all the hard work! Pushed to main for the next 26.05.00 release as RM Assistant -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=40286 Laura Escamilla <Laura.escamilla@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |Laura.escamilla@bywatersolu | |tions.com Status|Pushed to main |Needs documenting --- Comment #16 from Laura Escamilla <Laura.escamilla@bywatersolutions.com> --- Not backporting to 25.05 as this is an enhancement. Happy to revisit if needed. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org