Bug 18025

Summary: Expired password recovery links cause sql crash
Product: Koha Reporter: Liz Rea <liz>
Component: OPACAssignee: Liz Rea <liz>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, kyle, mtj, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 18025 - Expired password recovery links cause sql crash
Bug 18025 - Expired password recovery links cause sql crash
Bug 18025 - Expired password recovery links cause sql crash
Bug 18025 - Expired password recovery links cause sql crash
Bug 18025: Fix test
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber
Bug 18025: Fix test
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber
Bug 18025 - Expired password recovery links cause sql crash
Bug 18025: Fix test
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber
Bug 18025 - Expired password recovery links cause sql crash
Bug 18025: Fix test
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber

Description Liz Rea 2017-01-31 22:04:14 UTC
Trying to reset a password with an expired token leads to a very user unfriendly sql crash: 

DBD::mysql::st execute failed: Duplicate entry '19' for key 'PRIMARY' [for Statement "INSERT INTO `borrower_password_recovery` ( `borrowernumber`, `uuid`, `valid_until`) VALUES ( ?, ?, ? )" with ParamValues: 0='19', 1='$2a$08$O8FjE434Ue3DLtGTgQB3Bu', 2='2017-02-02T22:03:01'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
DBIx::Class::Storage::DBI::_dbh_execute(): Duplicate entry '19' for key 'PRIMARY' at /home/vagrant/kohaclone/Koha/Patron/Password/Recovery.pm line 127

We should delete these if they exist, and issue a new token.
Comment 1 Liz Rea 2017-02-01 23:12:19 UTC
Created attachment 59760 [details] [review]
Bug 18025 - Expired password recovery links cause sql crash

When a user gets an email, but doesn't act or visit it within two days,
     attempting to create a new one causes a collision. We should just
     delete the old one, assuming they still want to reset their
     password.

To test:
create yourself a borrower with a userid and password.
Attempt a password recovery on the OPAC
update the entry in the database for that user to have an expired token
e.g. update borrower_password_recovery set valid_until = '2017-01-25
03:25:26' where borrowernumber = 12;
Attempt another password recovery operation - should error
apply the patch
Try it again - no error, new token is generated and additional email
with new link is sent.
Comment 2 Marc Véron 2017-02-10 10:42:34 UTC
Created attachment 60108 [details] [review]
Bug 18025 - Expired password recovery links cause sql crash

When a user gets an email, but doesn't act or visit it within two days,
     attempting to create a new one causes a collision. We should just
     delete the old one, assuming they still want to reset their
     password.

To test:
create yourself a borrower with a userid and password.
Attempt a password recovery on the OPAC
update the entry in the database for that user to have an expired token
e.g. update borrower_password_recovery set valid_until = '2017-01-25
03:25:26' where borrowernumber = 12;
Attempt another password recovery operation - should error
apply the patch
Try it again - no error, new token is generated and additional email
with new link is sent.

Issue reproduced without patch; resolved with patch.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 3 Jonathan Druart 2017-02-14 09:29:52 UTC
It sounds weird to make to have a validate/check subroutine that actually deletes stuffs.
I think it would be better to delete it, if it exists, before regenerating a new one.

Moreover the search argument does not make sense, there is a 'or' with only one condition.
Comment 4 Liz Rea 2017-02-14 21:35:40 UTC
Created attachment 60223 [details] [review]
Bug 18025 - Expired password recovery links cause sql crash

When a user gets an email, but doesn't act or visit it within two days,
     attempting to create a new one causes a collision. We should just
     delete the old one, assuming they still want to reset their
     password.

To test:
create yourself a borrower with a userid and password.
Attempt a password recovery on the OPAC
update the entry in the database for that user to have an expired token
e.g. update borrower_password_recovery set valid_until = '2017-01-25
03:25:26' where borrowernumber = 12;
Attempt another password recovery operation - should error
apply the patch
Try it again - no error, new token is generated and additional email
with new link is sent.
Comment 5 Marc Véron 2017-02-15 08:53:46 UTC
Created attachment 60245 [details] [review]
Bug 18025 - Expired password recovery links cause sql crash

When a user gets an email, but doesn't act or visit it within two days,
     attempting to create a new one causes a collision. We should just
     delete the old one, assuming they still want to reset their
     password.

To test:
create yourself a borrower with a userid and password.
Attempt a password recovery on the OPAC
update the entry in the database for that user to have an expired token
e.g. update borrower_password_recovery set valid_until = '2017-01-25
03:25:26' where borrowernumber = 12;
Attempt another password recovery operation - should error
apply the patch
Try it again - no error, new token is generated and additional email
with new link is sent.

Issue reproduced - is resolved by patch
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 6 Jonathan Druart 2017-02-15 17:01:16 UTC
Liz, could you double check please? The tests do not pass for me.
Comment 7 Jonathan Druart 2017-02-15 17:14:35 UTC
Created attachment 60281 [details] [review]
Bug 18025: Fix test
Comment 8 Jonathan Druart 2017-02-15 17:14:40 UTC
Created attachment 60282 [details] [review]
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber
Comment 9 Jonathan Druart 2017-02-15 17:15:36 UTC
Liz, Does the last patch sounds ok to you?
Comment 10 Liz Rea 2017-02-15 21:01:53 UTC
Created attachment 60295 [details] [review]
Bug 18025: Fix test

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Seems to work fine
Comment 11 Liz Rea 2017-02-15 21:02:29 UTC
Created attachment 60296 [details] [review]
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
This is fine with me.


 Current status: Signed Off
Comment 12 Jonathan Druart 2017-02-16 07:49:45 UTC
Created attachment 60303 [details] [review]
Bug 18025 - Expired password recovery links cause sql crash

When a user gets an email, but doesn't act or visit it within two days,
     attempting to create a new one causes a collision. We should just
     delete the old one, assuming they still want to reset their
     password.

To test:
create yourself a borrower with a userid and password.
Attempt a password recovery on the OPAC
update the entry in the database for that user to have an expired token
e.g. update borrower_password_recovery set valid_until = '2017-01-25
03:25:26' where borrowernumber = 12;
Attempt another password recovery operation - should error
apply the patch
Try it again - no error, new token is generated and additional email
with new link is sent.

Issue reproduced - is resolved by patch
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 13 Jonathan Druart 2017-02-16 07:49:48 UTC
Created attachment 60304 [details] [review]
Bug 18025: Fix test

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Seems to work fine

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 14 Jonathan Druart 2017-02-16 07:49:54 UTC
Created attachment 60305 [details] [review]
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
This is fine with me.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 15 Jonathan Druart 2017-02-16 10:41:52 UTC
Created attachment 60311 [details] [review]
Bug 18025 - Expired password recovery links cause sql crash

When a user gets an email, but doesn't act or visit it within two days,
     attempting to create a new one causes a collision. We should just
     delete the old one, assuming they still want to reset their
     password.

To test:
create yourself a borrower with a userid and password.
Attempt a password recovery on the OPAC
update the entry in the database for that user to have an expired token
e.g. update borrower_password_recovery set valid_until = '2017-01-25
03:25:26' where borrowernumber = 12;
Attempt another password recovery operation - should error
apply the patch
Try it again - no error, new token is generated and additional email
with new link is sent.

Issue reproduced - is resolved by patch
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2017-02-16 10:41:55 UTC
Created attachment 60312 [details] [review]
Bug 18025: Fix test

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Seems to work fine

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Jonathan Druart 2017-02-16 10:41:58 UTC
Created attachment 60313 [details] [review]
Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
This is fine with me.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Kyle M Hall 2017-02-17 11:24:50 UTC
Pushed to master for 17.05, thanks Liz, Jonathan!
Comment 19 Katrin Fischer 2017-02-19 19:56:53 UTC
These patches have been pushed to 16.11.x and will be in 16.11.04.
Comment 20 Mason James 2017-02-22 23:15:19 UTC
Pushed to 16.05.x, for 16.05.10 release