Description
Liz Rea
2017-01-31 22:04:14 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. 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 |