[Bug 17494] New: Koha generating duplicate self registration tokens
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Bug ID: 17494 Summary: Koha generating duplicate self registration tokens Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: gmcharlt@gmail.com Reporter: kyle@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org We have reports of patrons receiving the same verification token. I believe this is because we are passing md5_hex a hashref and not a string. I believe what is getting summed is no the patron data, but the memory address that points to the patron hash! We need to stringify the patron hash before passing it to md5_hex. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Have you seen that with your own eyes? :) It seems very unlikely to get twice the same memory address to calculate the md5. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #1)
Have you seen that with your own eyes? :) It seems very unlikely to get twice the same memory address to calculate the md5.
Yes, and I agree it does seem unlikely, but I've actually seen it multiple times! I think maybe memory re-use is possibly higher under plack? Perhaps instead of using the patron hash to generate an md5 we should just use a random alphanumeric string generator, check the db to make sure that number doesn't already exist, and enforce uniqueness on the token column to ensure this situation cannot possibly happen. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Arturo <library@sll.texas.gov> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |library@sll.texas.gov -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 56914 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56914&action=edit Bug 17494: Make sure the same verification token won't be generated twice Well, this patch does not enforce the unique constraint but mimic what is already done in C4::ImportExportFramework and Koha::Upload where md5_hex is used. Test plan: Have a look at the code and confirm that it adds more randomness to the hashed string -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56914|0 |1 is obsolete| | --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 56992 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56992&action=edit Bug 17494: Make sure the same verification token won't be generated twice Well, this patch does not enforce the unique constraint but mimic what is already done in C4::ImportExportFramework and Koha::Upload where md5_hex is used. Test plan: Have a look at the code and confirm that it adds more randomness to the hashed string Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 56993 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56993&action=edit Bug 17494 - Prevent duplicate tokens from getting stored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56993|0 |1 is obsolete| | --- Comment #6 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 57008 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57008&action=edit Bug 17494 - Prevent duplicate tokens from getting stored -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17549 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17549 [Bug 17549] Table borrower_modifications allows for duplication verification tokens -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #7 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Kyle, the new exception file generates an error: FAIL Koha/Exceptions/Patron/Modification.pm FAIL valid Class Koha::Exceptions::Object appears to be a typo as it is only specified in the 'isa' param for Koha::Exceptions::Koha::Patron::Modification::DuplicateVerificationToken -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #7)
Kyle, the new exception file generates an error:
FAIL Koha/Exceptions/Patron/Modification.pm FAIL valid Class Koha::Exceptions::Object appears to be a typo as it is only specified in the 'isa' param for Koha::Exceptions::Koha::Patron::Modification::DuplicateVerificationToken
Ok nevermind, I have a fix! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> 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=17494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56992|0 |1 is obsolete| | Attachment #57008|0 |1 is obsolete| | --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 57204 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57204&action=edit Bug 17494: Make sure the same verification token won't be generated twice Well, this patch does not enforce the unique constraint but mimic what is already done in C4::ImportExportFramework and Koha::Upload where md5_hex is used. Test plan: Have a look at the code and confirm that it adds more randomness to the hashed string Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 57205 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57205&action=edit Bug 17494 - Prevent duplicate tokens from getting stored Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 57206 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57206&action=edit Bug 17494: Fix compilation warning Class Koha::Exceptions::Object appears to be a typo as it is only specified in the 'isa' param for Koha::Exceptions::Koha::Patron::Modification::DuplicateVerificationToken Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57204|0 |1 is obsolete| | --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 57212 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57212&action=edit Bug 17494: Make sure the same verification token won't be generated twice Well, this patch does not enforce the unique constraint but mimic what is already done in C4::ImportExportFramework and Koha::Upload where md5_hex is used. Test plan: Have a look at the code and confirm that it adds more randomness to the hashed string Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57205|0 |1 is obsolete| | --- Comment #13 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 57213 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57213&action=edit Bug 17494: Prevent duplicate tokens from getting stored Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #57206|0 |1 is obsolete| | --- Comment #14 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 57214 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=57214&action=edit Bug 17494: (QA followup) Fix exception name This patch changes the exception name to match the package name. And also adds a test for the thrown exception message. Also creates a general Koha::Exception::Patron::Modification exception to make the new one inherit from it, following the common use in the codebase. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 16.11, thanks Jonathan, Tomas! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com --- Comment #16 from Mason James <mtj@kohaaloha.com> --- Pushed to 16.05.x, for 16.05.06 release -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com Status|Pushed to Master |Pushed to Stable --- Comment #17 from Julian Maurice <julian.maurice@biblibre.com> --- Pushed to 3.22.x for 3.22.14 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |17943 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17943 [Bug 17943] "make test" failed when upgrading from koha-3.22.10 to 3-22.14 , also fresh installation of koha-3.22.14 made the same "make test" failing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #18 from Julian Maurice <julian.maurice@biblibre.com> --- Reverted patch 2 and 3 from 3.22.x as they are not intended for this version. First patch kept because it's useful in 3.22.x -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 Liz Rea <liz@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |liz@catalyst.net.nz --- Comment #19 from Liz Rea <liz@catalyst.net.nz> --- Hi Julian, It seems patches 2 and 3 have snuck back into 3.22.15, have a look please? Cheers, Liz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #20 from Julian Maurice <julian.maurice@biblibre.com> --- (In reply to Liz Rea from comment #19)
It seems patches 2 and 3 have snuck back into 3.22.15, have a look please?
Yes, they are. They were reverted just after the 3.22.15 release, so it should be ok in 3.22.16 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17494 --- Comment #21 from Liz Rea <liz@catalyst.net.nz> --- Cool thank you very much. Just made building a package trickier than necessary. :) Cheers, Liz -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org