Bug 17494 - Koha generating duplicate self registration tokens
Summary: Koha generating duplicate self registration tokens
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 17549 17943
  Show dependency treegraph
 
Reported: 2016-10-24 17:31 UTC by Kyle M Hall
Modified: 2017-02-02 20:52 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 17494: Make sure the same verification token won't be generated twice (1.15 KB, patch)
2016-10-27 08:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17494: Make sure the same verification token won't be generated twice (1.21 KB, patch)
2016-10-31 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17494 - Prevent duplicate tokens from getting stored (2.53 KB, patch)
2016-10-31 11:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17494 - Prevent duplicate tokens from getting stored (3.86 KB, patch)
2016-10-31 18:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17494: Make sure the same verification token won't be generated twice (1.27 KB, patch)
2016-11-04 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17494 - Prevent duplicate tokens from getting stored (3.92 KB, patch)
2016-11-04 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17494: Fix compilation warning (993 bytes, patch)
2016-11-04 14:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17494: Make sure the same verification token won't be generated twice (1.32 KB, patch)
2016-11-04 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17494: Prevent duplicate tokens from getting stored (3.97 KB, patch)
2016-11-04 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 17494: (QA followup) Fix exception name (3.18 KB, patch)
2016-11-04 15:32 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2016-10-24 17:31:38 UTC
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.
Comment 1 Jonathan Druart 2016-10-25 07:29: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.
Comment 2 Kyle M Hall 2016-10-25 11:46:21 UTC
(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.
Comment 3 Jonathan Druart 2016-10-27 08:15:43 UTC
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
Comment 4 Kyle M Hall 2016-10-31 11:17:10 UTC
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>
Comment 5 Kyle M Hall 2016-10-31 11:17:25 UTC
Created attachment 56993 [details] [review]
Bug 17494 - Prevent duplicate tokens from getting stored
Comment 6 Kyle M Hall 2016-10-31 18:22:07 UTC
Created attachment 57008 [details] [review]
Bug 17494 - Prevent duplicate tokens from getting stored
Comment 7 Jonathan Druart 2016-11-04 14:25:43 UTC
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
Comment 8 Jonathan Druart 2016-11-04 14:29:20 UTC
(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!
Comment 9 Jonathan Druart 2016-11-04 14:33:47 UTC
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>
Comment 10 Jonathan Druart 2016-11-04 14:33:52 UTC
Created attachment 57205 [details] [review]
Bug 17494 - Prevent duplicate tokens from getting stored

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 11 Jonathan Druart 2016-11-04 14:33:59 UTC
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>
Comment 12 Tomás Cohen Arazi 2016-11-04 15:32:09 UTC
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>
Comment 13 Tomás Cohen Arazi 2016-11-04 15:32:20 UTC
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>
Comment 14 Tomás Cohen Arazi 2016-11-04 15:32:28 UTC
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>
Comment 15 Kyle M Hall 2016-11-04 15:46:28 UTC
Pushed to master for 16.11, thanks Jonathan, Tomas!
Comment 16 Mason James 2016-12-14 10:07:06 UTC
Pushed to 16.05.x, for 16.05.06 release
Comment 17 Julian Maurice 2016-12-19 10:40:20 UTC
Pushed to 3.22.x for 3.22.14
Comment 18 Julian Maurice 2017-01-25 15:27:22 UTC
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
Comment 19 Liz Rea 2017-02-02 03:26:31 UTC
Hi Julian,

It seems patches 2 and 3 have snuck back into 3.22.15, have a look please?

Cheers,
Liz
Comment 20 Julian Maurice 2017-02-02 08:13:49 UTC
(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
Comment 21 Liz Rea 2017-02-02 20:52:06 UTC
Cool thank you very much. Just made building a package trickier than necessary. :)

Cheers,
Liz