Attachment 142110 Details for Bug 31739 – Bug 31739: Password recovery from staff fails if previous expired reset-entry exists.

[patch] Bug 31739: Password recovery from staff fails if previous expired reset-entry exists.

Bug-31739-Password-recovery-from-staff-fails-if-pr.patch (text/plain), 5.37 KB, created by David Nind on 2022-10-18 22:20:25 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: David Nind

Created: 2022-10-18 22:20:25 UTC

Size: 5.37 KB

patch

obsolete

>From cfe23bcc45119c1b0ef3dae841c597fd143cbe44 Mon Sep 17 00:00:00 2001
>From: =?UTF-8?q?Bj=C3=B6rn=20Nyl=C3=A9n?= <bjorn.nylen@ub.lu.se>
>Date: Wed, 12 Oct 2022 11:54:11 +0000
>Subject: [PATCH] Bug 31739: Password recovery from staff fails if previous
> expired reset-entry exists.
>
>SendPasswordRecoveryEmail relies on the calling script to tell if there is an
>existing valid recovery already. If there's an expired recovery-entry the
>members/notices.pl script will try to create a new entry resulting in a duplicate
>key error.
>
>This patch fixes the bug by removing the need for the calling script to do the check as
>since SendPasswordRecoveryEmail does the same thing anyway.
>SendPasswordRecoveryEmail will now use DBIx ->update_or_create instead of looking at
>the $update param to determine if it should update an existing entry or create a new.
>
>The update param is removed from all calling scripts and test are updated.
>
>To test:
>1. Generate a password recovery mail for a patron
>2. Let it expire.
>3. Generate a new password recovery from staff to the same patron - Fail!
>4: Apply patch
>5. Generate a new password recovery from staff to the same patron - Success!
>6. Opac password recovery flow should also work.
>7. Tests pass.
>
>Sponsored-by: Lund University Library
>
>Signed-off-by: David Nind <david@davidnind.com>
>---
> Koha/Patron/Password/Recovery.pm  | 15 +++------------
> members/notices.pl                |  5 +----
> opac/opac-password-recovery.pl    |  2 +-
> t/db_dependent/Passwordrecovery.t |  4 ++--
> 4 files changed, 7 insertions(+), 19 deletions(-)
>
>diff --git a/Koha/Patron/Password/Recovery.pm b/Koha/Patron/Password/Recovery.pm
>index 0f1882bf84..1e462be368 100644
>--- a/Koha/Patron/Password/Recovery.pm
>+++ b/Koha/Patron/Password/Recovery.pm
>@@ -106,7 +106,6 @@ sub GetValidLinkInfo {
> sub SendPasswordRecoveryEmail {
>     my $borrower  = shift;    # Koha::Patron
>     my $userEmail = shift;    #to_address (the one specified in the request)
>-    my $update    = shift;
>     my $staff     = shift // 0;
> 
>     my $schema = Koha::Database->new->schema;
>@@ -121,22 +120,14 @@ sub SendPasswordRecoveryEmail {
>     my $days = $staff ? STAFF : PATRON;
>     my $expirydate =
>       dt_from_string()->add( days => $days );
>-    if ($update) {
>-        my $rs =
>-          $schema->resultset('BorrowerPasswordRecovery')
>-          ->search( { borrowernumber => $borrower->borrowernumber, } );
>-        $rs->update(
>-            { uuid => $uuid_str, valid_until => $expirydate->datetime() } );
>-    }
>-    else {
>-        my $rs = $schema->resultset('BorrowerPasswordRecovery')->create(
>+    my $rs = $schema->resultset('BorrowerPasswordRecovery')->update_or_create(
>             {
>                 borrowernumber => $borrower->borrowernumber,
>                 uuid           => $uuid_str,
>                 valid_until    => $expirydate->datetime()
>-            }
>+            },
>+            { key => 'primary' }
>         );
>-    }
> 
>     # create link
>     my $opacbase = C4::Context->preference('OPACBaseURL') || '';
>diff --git a/members/notices.pl b/members/notices.pl
>index 806ec4a5a8..11f3faa917 100755
>--- a/members/notices.pl
>+++ b/members/notices.pl
>@@ -100,11 +100,8 @@ if ( $op eq 'send_password_reset' ) {
> 
>     if ($emailaddr) {
> 
>-        # check if there's already a recovery in process
>-        my $update = ValidateBorrowernumber( $patron->borrowernumber );
>-
>         # send staff initiated password recovery
>-        SendPasswordRecoveryEmail( $patron, $emailaddr, $update, 1 );
>+        SendPasswordRecoveryEmail( $patron, $emailaddr, 1 );
>     }
> 
>     # redirect to self to avoid form submission on refresh
>diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl
>index a5c2058b92..5d56829846 100755
>--- a/opac/opac-password-recovery.pl
>+++ b/opac/opac-password-recovery.pl
>@@ -133,7 +133,7 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>             username                => $username
>         );
>     }
>-    elsif ( SendPasswordRecoveryEmail( $borrower, $email, scalar $query->param('resendEmail') ) ) {    # generate uuid and send recovery email
>+    elsif ( SendPasswordRecoveryEmail( $borrower, $email ) ) {    # generate uuid and send recovery email
>         $template->param(
>             mail_sent => 1,
>             email     => $email
>diff --git a/t/db_dependent/Passwordrecovery.t b/t/db_dependent/Passwordrecovery.t
>index 36bde4c780..02001a913b 100755
>--- a/t/db_dependent/Passwordrecovery.t
>+++ b/t/db_dependent/Passwordrecovery.t
>@@ -200,7 +200,7 @@ ok( Koha::Patron::Password::Recovery::DeleteExpiredPasswordRecovery($borrowernum
> my $borrower = Koha::Patrons->search( { userid => $userid1 } )->next;
> my $success;
> warning_is {
>-    $success = Koha::Patron::Password::Recovery::SendPasswordRecoveryEmail($borrower, $email1, 0); }
>+    $success = Koha::Patron::Password::Recovery::SendPasswordRecoveryEmail($borrower, $email1 ); }
>     "Fake sendmail",
>     '[SendPasswordRecoveryEmail] expecting fake sendmail';
> ok( $success == 1, '[SendPasswordRecoveryEmail] Returns 1 on success');
>@@ -212,7 +212,7 @@ my $bpr = $schema->resultset('BorrowerPasswordRecovery')->search( { borrowernumb
> my $tempuuid1 = $bpr->next->uuid;
> 
> warning_is {
>-    Koha::Patron::Password::Recovery::SendPasswordRecoveryEmail($borrower, $email1, 1); }
>+    Koha::Patron::Password::Recovery::SendPasswordRecoveryEmail($borrower, $email1 ); }
>     "Fake sendmail",
>     '[SendPasswordRecoveryEmail] expecting fake sendmail';
> 
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 31739: 141732 | 142110 | 142399