|
Description
Kyle M Hall (khall)
2016-10-24 17:31:38 UTC
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 |