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.
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.
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>
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.
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.
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>
Liz, could you double check please? The tests do not pass for me.
Created attachment 60281 [details] [review] Bug 18025: Fix test
Created attachment 60282 [details] [review] Bug 18025: Simplify logic and avoid 1 call to ValidateBorrowernumber
Liz, Does the last patch sounds ok to you?
Created attachment 60295 [details] [review] Bug 18025: Fix test Signed-off-by: Liz Rea <liz@catalyst.net.nz> Seems to work fine
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
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>
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>
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>
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>
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>
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>
Pushed to master for 17.05, thanks Liz, Jonathan!
These patches have been pushed to 16.11.x and will be in 16.11.04.
Pushed to 16.05.x, for 16.05.10 release