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.
Have you seen that with your own eyes? :) It seems very unlikely to get twice the same memory address to calculate the md5.
(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.
Created attachment 56914 [details] [review] 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
Created attachment 56992 [details] [review] 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>
Created attachment 56993 [details] [review] Bug 17494 - Prevent duplicate tokens from getting stored
Created attachment 57008 [details] [review] Bug 17494 - Prevent duplicate tokens from getting stored
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
(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!
Created attachment 57204 [details] [review] 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>
Created attachment 57205 [details] [review] Bug 17494 - Prevent duplicate tokens from getting stored Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 57206 [details] [review] 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>
Created attachment 57212 [details] [review] 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>
Created attachment 57213 [details] [review] 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>
Created attachment 57214 [details] [review] 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>
Pushed to master for 16.11, thanks Jonathan, Tomas!
Pushed to 16.05.x, for 16.05.06 release
Pushed to 3.22.x for 3.22.14
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
Hi Julian, It seems patches 2 and 3 have snuck back into 3.22.15, have a look please? Cheers, Liz
(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
Cool thank you very much. Just made building a package trickier than necessary. :) Cheers, Liz