A better way to handle this is to send a uniquely generated link to the user which will allow him to reset its password.

*** Bug 3059 has been marked as a duplicate of this bug. ***

(In reply to comment #1)
> A better way to handle this is to send a uniquely generated link to the user
> which will allow him to reset its password.

I agree, this is the common best practice, we should never be sending passwords to users (unless they are onetime use only) ... i'd like to remove the code that already does this in staff, rather than adding more.

This says it's assigned - is someone actually working on this?

Nicole

We actually have this code running on our 3.10.x version.

It's juste a matter of finding time to merge on the main branch...

TEST PLAN:

1) apply the patch
2) go to system preferences OPAC>>Privacy and set 'OpacResetPassword' to ON.  That will cause the link 'Forgot your password' to show up on the welcome page, below connection box.
3) refresh front page, click on 'Forgot your password' and enter a VALID address (one that is associated to an entry in borrowers.email or borrowers.email_pro.
3b) Also try an INVALID address (valid yet not in your koha db).  An error message will show up.
4) An email should be received at that address with a link.
5) Follow the link in the mail to fill the new password.
   Until a satisfactory new password is entered, the old password is not reset.
6) Go to main page try the new password.

Looks like this requires an update to the database, but I don't see any changes in updatedatabase.pl.

The second patch "Bug 8753 - Add forgot password link to OPAC" contains the db scripts.

Patch applied cleanly, go forth and signoff

Patch didn't apply one the sandboxes. There was a conflict in updatedatabase.pl

Thank you sir!  I barely had time to notice the issue email then set up my environment to rebase, that you had already solved it.

Changing the status from "Patch doesn't apply" to "Need signoff".
Not sure if that was needed since rebaser knows the business, please let me if otherwise.

Looks like the table borrower_password_recovery was left out of the database update by accident.

Well, seems I'm fully forgot updatedatabase.pl in first patch.
Rebased and fixed.

Early QA comment:
1/ new routine needs unit tests
2/ remove tab characters
3/ perltidy new pl script
4/ use Modern::Perl; (you only use strict)
5/ only 1 syspref is added to sysprefs.sql
6/ why don't you sent the email using utf8?
7/ on generating the new password char, maybe should you use the alphabet syspref (+ numbers)
8/ why don't you use koha opac header for opac-send-password-recovery template file?
9/ use the tt plugin to access to syspref value (Koha.Preference("mypref")).
10/ what is the 'UNAUTHORIZ' check (in opac-password-recovery.pl)?

10 is a good number.

Comment on attachment 24586 [details] [review]
Bug 8753 - Add forgot password link to OPAC

Review of attachment 24586 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +1068,5 @@
>          PatronSelfRegistrationDefaultCategory => C4::Context->preference("PatronSelfRegistrationDefaultCategory"),
>          persona            => C4::Context->preference("Persona"),
>          opac_css_override => $ENV{'OPAC_CSS_OVERRIDE'},
> +        OpacResetPassword => C4::Context->preference("OpacResetPassword"),
> +        OpacPasswordChange => C4::Context->preference("OpacPasswordChange"),

No longer necessary.

::: C4/Members.pm
@@ +2273,5 @@
> +    my $uuid_str;
> +    $uuid_str .= $chars[rand @chars] for 1..32;
> +
> +    #insert into database
> +    my $sth = $dbh->prepare( 'INSERT INTO borrower_password_recovery VALUES (? ,? , ADDDATE(NOW(), INTERVAL 2 DAY) )' );

Is this ANSI SQL?

@@ +2281,5 @@
> +    #define to/from emails
> +    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' );
> +
> +    #create link
> +    my $uuidLink = "http://" . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";

Does someone have that funky code to determine http vs https in a URL handy? Please don't hard code http.

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ +556,5 @@
> +              default: 1
> +              choices:
> +                  yes: "On"
> +                  no: "Off"
> +            - ". If On, the user can reset his password on OPAC."

There are prettier ways to format this.
        -
            - "The user "
            - pref: OpacResetPassword
              default: 1
              choices:
                  yes: "can reset"
                  no: "can not reset"
            - " their password on OPAC."

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt
@@ +48,5 @@
> +            More than one account has been found for the email address: "<strong>[% email %]</strong>"
> +            <br/>Try yo use your alternative email if you have another.
> +        [% ELSIF (errAlreadyStartRecovery) %]
> +            The process of password recovery has already started for this account ("<strong>[% email %]</strong>")
> +            <br/>Check your emails; you should recieve the link to reset your password.

receive typo.

@@ +74,5 @@
> +
> +[% ELSIF (new_password) %]
> +    [% IF (errLinkNotValid) %]
> +        <span class="TxtErreur"><h6>
> +        We could not authentify you as the account owner.

authenticate

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-auth.tt
@@ +86,5 @@
>  
>  <input type="submit" value="Log In" class="submit" />
> +[% IF Koha.Preference('OpacPasswordChange') && Koha.Preference('OpacResetPassword') %]
> +    <p><a href="/cgi-bin/koha/opac-password-recovery.pl">Forgot your password?</a></p>
> +[% END %]

Could this be in the nologininstructions div?

::: koha-tmpl/opac-tmpl/prog/en/modules/opac-password-recovery.tt
@@ +46,5 @@
> +            More than one account has been found for the email address: "<strong>[% email %]</strong>"
> +            <br/>Try yo use your alternative email if you have another.
> +        [% ELSIF (errAlreadyStartRecovery) %]
> +            The process of password recovery has already started for this account ("<strong>[% email %]</strong>")
> +            <br/>Check your emails; you should recieve the link to reset your password.

receive typo.

@@ +72,5 @@
> +
> +[% ELSIF (new_password) %]
> +    [% IF (errLinkNotValid) %]
> +        <span class="TxtErreur"><h6>
> +        We could not authentify you as the account owner.

authenticate

::: opac/opac-main.pl
@@ +58,4 @@
>      koha_news_count     => $koha_news_count,
>      display_daily_quote => C4::Context->preference('QuoteOfTheDay'),
>      daily_quote         => $quote,
> +    OpacResetPassword   => C4::Context->preference('OpacResetPassword'),

You tweaked opac-main.tt to use the Koha.Preference method of grabbing system preferences in the template. There's no need to modify this file.

::: opac/opac-password-recovery.pl
@@ +48,5 @@
> +my $errPassTooShort;
> +
> +my $dbh = C4::Context->dbh;
> +
> +$template->param( OpacResetPassword => C4::Context->preference("OpacResetPassword") );

This is not needed.

Please look at my eyeball comments, and deal with this:
 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 98)
                forbidden pattern: tab char (line 100)
                forbidden pattern: tab char (line 99)
   OK     tt_valid
   OK     valid_template

Comment on attachment 24586 [details] [review]
Bug 8753 - Add forgot password link to OPAC

Review of attachment 24586 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-password-recovery.pl
@@ +102,5 @@
> +            email                   => HTML::Entities::encode($email),
> +        );
> +    }
> +    else {
> +        #generate uuid and send recovery email

Bug 11575 has:
    ($in->{'query'}->https() ? "https://" : "http://")
$query->https() may be what you need. Some way of passing that to SendPasswordRecoveryEmail, so you don't hardcode http or https.

Comment on attachment 24781 [details] [review]
Bug 8753 - Add forgot password link to OPAC

Review of attachment 24781 [details] [review]:
-----------------------------------------------------------------

::: opac/opac-password-recovery.pl
@@ +135,5 @@
> +    if (   ($borrower_number)
> +        && ( $password eq $repeatPassword )
> +        && ( length($password) >= $minPassLength ) )
> +    {  #apply changes
> +        changepassword( $username, $borrower_number, md5_base64($password) );

We no longer md5 hash the password in Koha, you should do 

use Koha::AuthUtils;

hash_password($password) instead.  

(It's using bcrypt with a salt). Old passwords still work, we check both, but anytime people change their password it should be hashed with the much more secure bcrypt

Comment on attachment 24846 [details] [review]
Bug 8753 - Add forgot password link to OPAC

Review of attachment 24846 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +1068,4 @@
>          PatronSelfRegistrationDefaultCategory => C4::Context->preference("PatronSelfRegistrationDefaultCategory"),
>          persona            => C4::Context->preference("Persona"),
>          opac_css_override => $ENV{'OPAC_CSS_OVERRIDE'},
> +        OpacResetPassword => C4::Context->preference("OpacResetPassword"),

You added OpacResetPassword, and all the templates use the Koha system preference grabbing plugin. There's no need for this.

@@ +1068,5 @@
>          PatronSelfRegistrationDefaultCategory => C4::Context->preference("PatronSelfRegistrationDefaultCategory"),
>          persona            => C4::Context->preference("Persona"),
>          opac_css_override => $ENV{'OPAC_CSS_OVERRIDE'},
> +        OpacResetPassword => C4::Context->preference("OpacResetPassword"),
> +        OpacPasswordChange => C4::Context->preference("OpacPasswordChange"),

This wasn't here, so I don't see a reason to add it, since all the template changes you made use the Koha system preference grabbing plugin.

::: opac/opac-main.pl
@@ +58,4 @@
>      koha_news_count     => $koha_news_count,
>      display_daily_quote => C4::Context->preference('QuoteOfTheDay'),
>      daily_quote         => $quote,
> +    OpacResetPassword   => C4::Context->preference('OpacResetPassword'),

There's no point to have this.

Comment on attachment 24846 [details] [review]
Bug 8753 - Add forgot password link to OPAC

Review of attachment 24846 [details] [review]:
-----------------------------------------------------------------

::: C4/Members.pm
@@ +2266,5 @@
> +
> +sub SendPasswordRecoveryEmail {
> +    my $borrowernumber = shift;
> +    my $email = shift;
> +    my $query = shift;

Perhaps with the code later you were trying to handle:
    my $query = shift // CGI->new();

@@ +2285,5 @@
> +    #define to/from emails
> +    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' );
> +
> +    #create link
> +    my $https = $query->https() ? "https://" : "http://";

I believe I have seen others call this protocol, but changing this isn't necessary.

@@ +2290,5 @@
> +    my $uuidLink = $https . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";
> +    #warn $uuidLink;
> +
> +    #build email content
> +    my $query = new CGI;

Why do you need this?! You have line 2270.

Comment on attachment 24855 [details] [review]
Bug 8753 - Add forgot password link to OPAC

Review of attachment 24855 [details] [review]:
-----------------------------------------------------------------

::: C4/Auth.pm
@@ +1068,5 @@
>          PatronSelfRegistrationDefaultCategory => C4::Context->preference("PatronSelfRegistrationDefaultCategory"),
>          persona            => C4::Context->preference("Persona"),
>          opac_css_override => $ENV{'OPAC_CSS_OVERRIDE'},
> +        OpacResetPassword => C4::Context->preference("OpacResetPassword"),
> +        OpacPasswordChange => C4::Context->preference("OpacPasswordChange"),

They're back. :(

::: opac/opac-main.pl
@@ +58,4 @@
>      koha_news_count     => $koha_news_count,
>      display_daily_quote => C4::Context->preference('QuoteOfTheDay'),
>      daily_quote         => $quote,
> +    OpacResetPassword   => C4::Context->preference('OpacResetPassword'),

Oops.

You forget to delete the password reset entry after resetting the password.

Please test with /cgi-bin/koha/opac-password-recovery.pl?uniqueKey=OneThatDoesNotExist

404.pl is not the correct behaviour, in my opinion.

(In reply to M. Tompsett from comment #37)
> You forget to delete the password reset entry after resetting the password.

You mean from table borrower_password_recovery ?  I validated it does delete the entry.

And I can't reproduce the 404 of your next comment.  When I append your line to my opac base url, it sends me to a message:

"
We could not authenticate you as the account owner.
Be sure to use the link you received in your email. 
"

Are you sure the patch was correctly applied?

Thanks again for your time!

The generate a new link process can create multiple links (potentially huge number) for a single user. It should overwrite the existing one. No more than one entry per borrowernumber.

Vast improvement on the DELETE.
Sadly, still have two problems.

"Framing Problem"
Click forget password link
Type in username and password to log in to OPAC
Click link in email. (screen 1)
Log out of OPAC
Click link in email. (screen 2)
Both screens are framed differently.

"Email Problem"
http://irc.koha-community.org/koha/2014-02-04#i_1460899

I notice this feature is enabled by default. I think it should be disabled by default.

There are many problems with the template which I'll work on a follow-up for.

For what it's worth, I tested this and didn't get a password recovery email. I noticed in trying to understand how the emails should be sent that the feature doesn't use notice templates or the message queue. Should we be adding a feature that uses yet another method for handling emails?

My Ubuntu VM has email set up in a manner similar to
https://help.ubuntu.com/community/Postfix

This uses the Mail::SendMail library directly, which is already part of the dependencies for Koha. Checkout the code in C4/Members.pm around line 2314 after applying the patch.
The "if ( sendmail %mail )" is what actually does the emailing.

If you don't have postfix configured, then nullmailer is installed. Hopefully, that is configured correctly. I have yet to read sensible and useful documentation on it.

Some mail hosts delay sending/receiving when mail is relayed. Patience may be required while testing this.

(In reply to M. Tompsett from comment #50)
> My Ubuntu VM has email set up in a manner similar to
> https://help.ubuntu.com/community/Postfix
> 
> This uses the Mail::SendMail library directly, which is already part of the
> dependencies for Koha. Checkout the code in C4/Members.pm around line 2314
> after applying the patch.
> The "if ( sendmail %mail )" is what actually does the emailing.
> 
> If you don't have postfix configured, then nullmailer is installed.
> Hopefully, that is configured correctly. I have yet to read sensible and
> useful documentation on it.
> 
> Some mail hosts delay sending/receiving when mail is relayed. Patience may
> be required while testing this.

While other parts of Koha do rely on Sendmail directly, I think Owen has a very good point. We should definitely be using the notice templates, and we should probably be using the message queue.

The notice templates allow libraries to provide their own wording/translations to the message. I think that's a *must*. 

Using the message queue also helps Koha administrators to track the messages going out, since they might not have direct access to the MTA or mail server. 

Owen: I'm not sure what your testing scenario is but I noticed recently that my Koha VM's MTA is configured out of the box not to send email outside of its local domain. I don't know if that's relevant for you though.

I came into Koha today looking to see if there was any work on a patron being able to reset a forgotten password, and was happy to see this bug in progress. 

I want to +1 on the use of the existing notice templates and built-in messaging system in Koha for this feature. As the manager of a large, multi-branch system, that just gained access to the ability to finally have ALL notices sent from the branch email not the admin email (thank you developers for bugfix #5544!), I would hate to see password reset messages separately again come from the admin email address. 

Using the message queue will also be able to help us troubleshoot and work with patrons who repeatedly say "I reset my password but it didn't work." 

I hope this patch is eventually successful!

With the moved to DBIx, the Schema file needs to be created as well. Hence, the change back to ASSIGNED. Sorry. Wasn't me that changed the underlying technologies in mid-stream. :(

Interesting! Just some thoughts: 
The SQL code in the opac-recovery script will not make it pass QA. Please move it to module level (in DBIx?). 
Can you unit test SendPasswordRecoveryEmail?

I would not mind a mail with a library password; other info is more sensitive. If you can read/intercept the password from the mail, you can also read the unique userid for the reset password form. Same result: a hacked account. This approach is fine with me, feels more safe but is not per se safer imo.

Thinking out loud: if you do not include sensitive keywords as "password" in your mail (so rename your script?), would that be little more safe? Would not attract attention?

(In reply to M. de Rooy from comment #57)
> Interesting! Just some thoughts: 
> The SQL code in the opac-recovery script will not make it pass QA. Please
> move it to module level (in DBIx?). 
> Can you unit test SendPasswordRecoveryEmail?
> 
> I would not mind a mail with a library password; other info is more
> sensitive. If you can read/intercept the password from the mail, you can
> also read the unique userid for the reset password form. Same result: a
> hacked account. This approach is fine with me, feels more safe but is not
> per se safer imo.

I disagree, a token should be time based, and will require the person to set a password. 
This password only they will know. 

It's true that if someone intercepts the token and uses it to set your password for you, that is still a problem. However when you go to use the token and find out it has been used you will know this. If they intercept your password, they could use it for a bunch of time without you ever knowing.

Sending passwords in the clear is a bad idea. Please do not do it.

I agree with Chris, I think. Also we have a similar approach for the patron verification - sending a link to a page where you learn about your password. It seems a good way to do this.

The token sent to the user is valid only for 2 days or if the link is clicked.

Most of the time, a use who requests a password will click the link in the following minutes (if not seconds).

Of course, this doesn't prevent your email to be intercepted. As far as I know, this feature was implemented the way most of the « Forgot your password ? » links are implemented.

Also, if you send the password in the clear, if it is intercepted once, and you are one of (more people than we'd like to admit) those people who uses the same password in multiple accounts, it's not only your library account that is now possibly hacked. We should not ever, ever, ever show a password in the clear. Ever. 

I am glad this works the way it does, it is the proper way.

Liz

Juste wondering how this clear text password came in the discussion.

That was never the case with this development.

Comment 57, 2nd paragraph - when someone doing QA suggests mailing a cleartext password might be ok, it seems right to emphatically tell them "no, that's a bad idea."

I for one appreciate the fact that it was never part of this dev, thanks for doing it properly. :)

Liz

(In reply to M. de Rooy from comment #57)
> Interesting! Just some thoughts: 
> The SQL code in the opac-recovery script will not make it pass QA. Please
> move it to module level (in DBIx?). 
> Can you unit test SendPasswordRecoveryEmail?

Though, I would make sure this comment didn't get missed in the midst of everyone discussing password logic.

M. de Rooy, are you talking about just the SELECT? What about the DELETE? Could you clarify which bits you mean by commenting on a splinter review?

We don't want any SQL code in the .pl files - so I'd say both.

(In reply to Liz Rea from comment #63)
> Comment 57, 2nd paragraph - when someone doing QA suggests mailing a
> cleartext password might be ok, it seems right to emphatically tell them
> "no, that's a bad idea."

Please keep it in context. Hope I do not intercept any undertone about QA work here? 

For completeness: I completely agree that sending password by mail is theoretically very bad. However, we probably do not have a 100% safe solution for this. So therefore in a pragmatic view the sensitivity argument etc. 
And to add: As long as many OPACs are going over HTTP, we send the password clear text all over the place..

But again, also comment 57 This approach is fine with me!

(In reply to M. de Rooy from comment #66)
> (In reply to Liz Rea from comment #63)
> > Comment 57, 2nd paragraph - when someone doing QA suggests mailing a
> > cleartext password might be ok, it seems right to emphatically tell them
> > "no, that's a bad idea."
> 
> Please keep it in context. Hope I do not intercept any undertone about QA
> work here? 
> 
> For completeness: I completely agree that sending password by mail is
> theoretically very bad. However, we probably do not have a 100% safe
> solution for this. So therefore in a pragmatic view the sensitivity argument
> etc. 
> And to add: As long as many OPACs are going over HTTP, we send the password
> clear text all over the place..
> 
> But again, also comment 57 This approach is fine with me!

When it comes to security/safety, there is never a 100% guarantee. However, there are better and worse ways of doing things. It sounds like the approach used in these patches is a reasonable one. As Eric mentioned, it's the "standard" way of doing it.

As for sending passwords in the clear when using HTTP, that's not an issue with Koha's code. That's an end-user configuration issue. If people are using HTTP instead of HTTPS, that's their responsibility.

If you use the same password recover link twice you get an error which is inaccurate:

"We could not authenticate you as the account owner

Be sure to use the link you received in your email."

It might make sense to retain entries in the borrower_password_recovery table for a certain amount of time after they have been "used" so that we can accurately tell the user that they have followed a link which is out of date.

opac/opac-password-recovery.pl line 163:

warn "INLIBRO username $username";

The script contains a lot of SQL, which is a violation of coding guidelines:

http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL8:_SQL_code_in_.pl_scripts

The template contains a check for the variable "errTooManyEmailFound" but I don't see where that variable is created. In fact there doesn't seems to be any handling of multiple identical email addresses. This is a problem for my library where parents often put their email addresses on their kids' accounts.

Does not apply

Applying: Bug 8753 - Add forgot password link to OPAC
Using index info to reconstruct a base tree...
<stdin>:471: trailing whitespace.
    
<stdin>:500: trailing whitespace.
    );  
<stdin>:511: trailing whitespace.
    } );     
<stdin>:25: new blank line at EOF.
+
<stdin>:126: new blank line at EOF.
+
warning: squelched 1 whitespace error
warning: 6 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC

simith.doliveira@inlibro.com, could you squash your patches together too? Just my preference. :)

(In reply to M. Tompsett from comment #72)
> Just my preference. :)

Last I heard it was not the RM's preference, but that may have been a previous RM. Perhaps this is something we should add to the coding guidelines?

The patches were never signed off. I figure a squash isn't so bad at this stage. :)

Comment on attachment 34472 [details] [review]
SQL code in the .pl files removed. New .pm and .t files created.

>From b775b68292a4c9757f7d0b93d2675d9165c18cc4 Mon Sep 17 00:00:00 2001
>From: simith <simith@inlibro.com>
>Date: Tue, 16 Dec 2014 13:22:26 -0500
>Subject: [PATCH] SQL code in the .pl files removed. New .pm and .t files
> created.
>
>http://bugs.koha-community.org/show_bug.cgi?id=8753
>---
> C4/Passwordrecovery.pm                             | 159 +++++++++++++++++++++
> .../data/mysql/en/mandatory/sample_notices.sql     |   2 +-
> opac/opac-password-recovery.pl                     | 100 ++-----------
> 3 files changed, 173 insertions(+), 88 deletions(-)
> create mode 100644 C4/Passwordrecovery.pm
>
>diff --git a/C4/Passwordrecovery.pm b/C4/Passwordrecovery.pm
>new file mode 100644
>index 0000000..f1b26e9
>--- /dev/null
>+++ b/C4/Passwordrecovery.pm
>@@ -0,0 +1,159 @@
>+package C4::Passwordrecovery;
>+
>+# Copyright 2014 PTFS Europe
>+#
>+# This file is part of Koha.
>+#
>+# Koha is free software; you can redistribute it and/or modify it
>+# under the terms of the GNU General Public License as published by
>+# the Free Software Foundation; either version 3 of the License, or
>+# (at your option) any later version.
>+#
>+# Koha is distributed in the hope that it will be useful, but
>+# WITHOUT ANY WARRANTY; without even the implied warranty of
>+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>+# GNU General Public License for more details.
>+#
>+# You should have received a copy of the GNU General Public License
>+# along with Koha; if not, see <http://www.gnu.org/licenses>.
>+
>+use Modern::Perl;
>+use C4::Context;
>+
>+use vars qw($VERSION @ISA @EXPORT);
>+
>+BEGIN {
>+    # set the version for version checking
>+    $VERSION = 3.07.00.049;
>+    require Exporter;
>+    @ISA    = qw(Exporter);
>+    push @EXPORT, qw(
>+        &ValidateBorrowernumber
>+        &SendPasswordRecoveryEmail
>+        &GetValidLinkInfo
>+    );
>+}
>+
>+=head1 NAME
>+
>+C4::Passwordrecovery - Koha password recovery module
>+
>+=head1 SYNOPSIS
>+
>+use C4::Passwordrecovery;
>+
>+=head1 FUNCTIONS
>+
>+=head2 ValidateBorrowernumber
>+
>+$alread = ValidateBorrowernumber( $borrower_number );
>+
>+Check if the system already start recovery
>+
>+Returns true false
>+
>+=cut
>+
>+sub ValidateBorrowernumber {
>+    my ($borrower_number) = @_;
>+    my $schema = Koha::Database->new->schema;
>+
>+    my $rs = $schema->resultset('BorrowerPasswordRecovery')->search(
>+    {
>+       borrowernumber => $borrower_number,
>+       valid_until => \'> NOW()'
>+    }, {
>+        columns => 'borrowernumber'
>+    });
>+
>+    if ($rs->next){
>+        return 1;
>+    }
>+
>+    return 0;
>+}
>+
>+=head2 GetValidLinkInfo
>+
>+    Check if the link is still valid and return some info.
>+
>+=cut
>+
>+sub GetValidLinkInfo {
>+    my ($uniqueKey) = @_;
>+    my $dbh = C4::Context->dbh;
>+    my $query = '
>+    SELECT borrower_password_recovery.borrowernumber, userid
>+    FROM borrower_password_recovery, borrowers
>+    WHERE borrowers.borrowernumber = borrower_password_recovery.borrowernumber
>+    AND NOW() < valid_until
>+    AND uuid = ?
>+    ';
>+    my $sth = $dbh->prepare($query);
>+    $sth->execute($uniqueKey);
>+    return $sth->fetchrow;
>+}
>+
>+=head2 SendPasswordRecoveryEmail
>+
>+ It creates an email using the templates and send it to the user, using the specified email
>+
>+=cut
>+
>+sub SendPasswordRecoveryEmail {
>+    my $borrower = shift; # from GetMember
>+    my $userEmail = shift; #to_address (the one specified in the request)
>+    my $protocol = shift; #only required to determine if 'http' or 'https'
>+    my $update = shift;
>+
>+    my $schema = Koha::Database->new->schema;
>+
>+    # generate UUID
>+    my @chars = ("A".."Z", "a".."z", "0".."9");
>+    my $uuid_str;
>+    $uuid_str .= $chars[rand @chars] for 1..32;
>+
>+    # insert into database
>+    my $expirydate = DateTime->now(time_zone => C4::Context->tz())->add( days => 2 );
>+    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({
>+            borrowernumber=>$borrower->{'borrowernumber'},
>+            uuid => $uuid_str,
>+            valid_until=> $expirydate->datetime()
>+         });
>+    }
>+
>+    # create link
>+    my $uuidLink = $protocol . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";
>+
>+    # prepare the email
>+    my $letter = C4::Letters::GetPreparedLetter (
>+        module => 'members',
>+        letter_code => 'PASSWORD_RESET',
>+        branchcode => $borrower->{branchcode},
>+        substitute => {passwordreseturl => $uuidLink, user => $borrower->{userid} },
>+    );
>+
>+    # define to/from emails
>+    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' ); # from
>+
>+    C4::Letters::EnqueueLetter( {
>+         letter => $letter,
>+         borrowernumber => $borrower->{borrowernumber},
>+         to_address => $userEmail,
>+         from_address => $kohaEmail,
>+         message_transport_type => 'email',
>+    } );
>+
>+    return 1;
>+}
>+
>+END { }    # module clean-up code here (global destructor)
>+
>+1;
>\ No newline at end of file
>diff --git a/installer/data/mysql/en/mandatory/sample_notices.sql b/installer/data/mysql/en/mandatory/sample_notices.sql
>index 37e9d9f..67c8b34 100644
>--- a/installer/data/mysql/en/mandatory/sample_notices.sql
>+++ b/installer/data/mysql/en/mandatory/sample_notices.sql
>@@ -144,5 +144,5 @@ Your library.'
> );
> 
> INSERT INTO `letter` (module, code, branchcode, name, is_html, title, content, message_transport_type)
>-VALUES ('members','PASSWORD_RESET','','Online password reset',1,'Koha password recovery','<html>\r\n<p>This email has been sent in response to your password recovery request for the account <strong><< borrowers.userid>></strong>.\r\n</p>\r\n<p>\r\nYou can now create your new password using the following link:\r\n<br/><a href=\"<<passwordreseturl>>\"><<passwordreseturl>></a>\r\n</p>\r\n<p>This link will be valid for 2 days from this email\'s reception, then you must reapply if you do not change your password.</p>\r\n<p>Thank you.</p>\r\n</html>\r\n','email'
>+VALUES ('members','PASSWORD_RESET','','Online password reset',1,'Koha password recovery','<html>\r\n<p>This email has been sent in response to your password recovery request for the account <strong><<user>></strong>.\r\n</p>\r\n<p>\r\nYou can now create your new password using the following link:\r\n<br/><a href=\"<<passwordreseturl>>\"><<passwordreseturl>></a>\r\n</p>\r\n<p>This link will be valid for 2 days from this email\'s reception, then you must reapply if you do not change your password.</p>\r\n<p>Thank you.</p>\r\n</html>\r\n','email'
> );
>diff --git a/opac/opac-password-recovery.pl b/opac/opac-password-recovery.pl
>index 27c8ea6..b5cd2c2 100755
>--- a/opac/opac-password-recovery.pl
>+++ b/opac/opac-password-recovery.pl
>@@ -9,6 +9,7 @@ use C4::Koha;
> use C4::Members qw(changepassword GetMember GetMemberDetails );
> use C4::Output;
> use C4::Context;
>+use C4::Passwordrecovery qw(SendPasswordRecoveryEmail ValidateBorrowernumber GetValidLinkInfo);
> use Koha::AuthUtils qw(hash_password);
> my $query = new CGI;
> use HTML::Entities;
>@@ -44,11 +45,8 @@ my $errLinkNotValid;
> my $errPassNotMatch;
> my $errPassTooShort;
> 
>-my $dbh = C4::Context->dbh;
>-
> if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>-    #send mail + confirmation
>-
>+    my $protocol = $query->https() ? "https://" : "http://";
>     #try with the main email
>     $email ||= ''; # avoid undef
>     my $borrower_infos = GetMember( email => $email );
>@@ -63,11 +61,9 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>         $errNoEmailFound = 1;
>     }
>     elsif ( !$query->param('resendEmail') ) {
>-        my $sth = $dbh->prepare(
>-"SELECT borrowernumber FROM borrower_password_recovery WHERE NOW() < valid_until AND borrowernumber = ?"
>-        );
>-        $sth->execute($borrower_number);
>-        if ( my $already = $sth->fetchrow ) {
>+        my $already = ValidateBorrowernumber( $borrower_number );
>+
>+        if ( $already ) {
>             $hasError                = 1;
>             $errAlreadyStartRecovery = 1;
>         }
>@@ -82,7 +78,7 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>             email                   => HTML::Entities::encode($email),
>         );
>     }
>-    elsif ( SendPasswordRecoveryEmail( $borrower_infos, $email, $query, $query->param('resendEmail') ) ) {#generate uuid and send recovery email
>+    elsif ( SendPasswordRecoveryEmail( $borrower_infos, $email, $protocol, $query->param('resendEmail') ) ) {#generate uuid and send recovery email
>         $template->param(
>             mail_sent => 1,
>             email     => $email
>@@ -96,18 +92,7 @@ if ( $query->param('sendEmail') || $query->param('resendEmail') ) {
>     }
> }
> elsif ( $query->param('passwordReset') ) {
>-    #new password form
>-    #check if the link is still valid
>-    my $sth = $dbh->prepare(
>-        "SELECT borrower_password_recovery.borrowernumber, userid
>-                              FROM borrower_password_recovery, borrowers
>-                              WHERE borrowers.borrowernumber = borrower_password_recovery.borrowernumber
>-                              AND NOW() < valid_until
>-                              AND uuid = ?"
>-    );
>-    $sth->execute($uniqueKey);
>-    ( $borrower_number, $username ) = $sth->fetchrow;
>-
>+    ( $borrower_number, $username ) = GetValidLinkInfo($uniqueKey);
>     #validate password length & match
>     if (   ($borrower_number)
>         && ( $password eq $repeatPassword )
>@@ -116,8 +101,9 @@ elsif ( $query->param('passwordReset') ) {
>         changepassword( $username, $borrower_number, hash_password($password) );
> 
>         #remove entry
>-        my $sth = $dbh->prepare("DELETE FROM borrower_password_recovery WHERE uuid = ? or NOW() > valid_until");
>-        $sth->execute($uniqueKey);
>+        my $schema = Koha::Database->new->schema;
>+        my $rs = $schema->resultset('BorrowerPasswordRecovery')->search({-or => [uuid => $uniqueKey, valid_until => \'< NOW()']});
>+        $rs->delete;
> 
>         $template->param(
>             password_reset_done => 1,
>@@ -148,19 +134,12 @@ elsif ( $query->param('passwordReset') ) {
> }
> elsif ($uniqueKey) {  #reset password form
>     #check if the link is valid
>-    my $sth = $dbh->prepare(
>-        "SELECT borrower_password_recovery.borrowernumber, userid
>-                              FROM borrower_password_recovery, borrowers
>-                              WHERE borrowers.borrowernumber = borrower_password_recovery.borrowernumber
>-                              AND NOW() < valid_until
>-                              AND uuid = ?"
>-    );
>-    $sth->execute($uniqueKey);
>-    ( $borrower_number, $username ) = $sth->fetchrow;
>+    ( $borrower_number, $username ) = GetValidLinkInfo($uniqueKey);
>+
>     if ( !$borrower_number ) {
>         $errLinkNotValid = 1;
>     }
>-warn "INLIBRO username $username";
>+
>     $template->param(
>         new_password    => 1,
>         minPassLength   => $minPassLength,
>@@ -175,56 +154,3 @@ else { #password recovery form (to send email)
> }
> 
> output_html_with_http_headers $query, $cookie, $template->output;
>-
>-#
>-# It creates an email using the templates and send it to the user, using the specified email
>-#
>-sub SendPasswordRecoveryEmail {
>-    my $borrower = shift; # from GetMember
>-    my $userEmail = shift; #to_address (the one specified in the request)
>-    my $query = shift; #only required to determine if 'http' or 'https'
>-    my $update = shift;
>-
>-    my $dbh = C4::Context->dbh;
>-
>-    # generate UUID
>-    my @chars = ("A".."Z", "a".."z", "0".."9");
>-    my $uuid_str;
>-    $uuid_str .= $chars[rand @chars] for 1..32;
>-
>-    # insert into database
>-    my $expirydate = DateTime->now(time_zone => C4::Context->tz())->add( days => 2 );
>-    if($update){
>-       my $sth = $dbh->prepare( 'UPDATE borrower_password_recovery set uuid=?, valid_until=? where borrowernumber=? ');
>-       $sth->execute($uuid_str, $expirydate->datetime(), $borrower->{'borrowernumber'});
>-    } else {
>-       my $sth = $dbh->prepare( 'INSERT INTO borrower_password_recovery VALUES (?, ?, ?)');
>-       $sth->execute($borrower->{'borrowernumber'}, $uuid_str, $expirydate->datetime());
>-    }
>-
>-    # create link
>-    my $protocol = $query->https() ? "https://" : "http://";
>-    my $uuidLink = $protocol . C4::Context->preference( 'OPACBaseURL' ) . "/cgi-bin/koha/opac-password-recovery.pl?uniqueKey=$uuid_str";
>-
>-    # prepare the email
>-    my $letter = C4::Letters::GetPreparedLetter (
>-        module => 'members',
>-        letter_code => 'PASSWORD_RESET',
>-        branchcode => $borrower->{branchcode},
>-        tables => {borrowers => $borrower},
>-        substitute => {passwordreseturl => $uuidLink},
>-    );
>-
>-    # define to/from emails
>-    my $kohaEmail = C4::Context->preference( 'KohaAdminEmailAddress' ); # from
>-
>-    C4::Letters::EnqueueLetter( {
>-         letter => $letter,
>-         borrowernumber => $borrower->{'borrowernumber'},
>-         to_address => $userEmail,
>-         from_address => $kohaEmail,
>-         message_transport_type => 'email',
>-    } );
>-
>-    return 1;
>-}
>-- 
>1.9.1

Sorry for #76

Conflit fixed and patches squashed

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 8753:
<h1>Something went wrong !</h1>Applying: Bug 8753 - Add forgot password link to OPAC Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql Auto-merging installer/data/mysql/kohastructure.sql
Auto-merging installer/data/mysql/en/mandatory/sample_notices.sql
CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/sample_notices.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC

When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 8753 - Add forgot password link to OPAC

36819 - Bug 8753 - Add forgot password link to OPAC

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-8753---Add-forgot-password-link-to-OPAC-VCLXC4.patch .

Hi, all. My library recently switched to Koha, and this is my first attempt at testing a patch, but I am not sure if I am doing this correctly or if I've truly run into an issue.

I followed the instructions at http://wiki.koha-community.org/wiki/Sandboxes to test this patch, bug 8753. I set up Sandbox 6 from BibLibre succesfully, logged in correctly, and did not see any error messages in the 'news' area, so it seems the patch was applied successfully.

Before I could follow the test plan listed in this thread, I randomly selected a  patron (I chose Colin Campbell), set a password for him, and set his e-mail address to my e-mail address so that I'd receive the e-mail notifications. Then I followed the test plan. Everything seemed OK except that I never received the e-mail to reset the pw. I then remembered that by default, Koha installations do not have e-mail enabled. Could that be the reason? Or would it be safe to assume that e-mail is enabled in the BibLibre sandboxes? I couldn't find that information anywhere.

Thanks for any assistance anyonce can provide!

(In reply to Arturo from comment #80)
> Hi, all. My library recently switched to Koha, and this is my first attempt
> at testing a patch, but I am not sure if I am doing this correctly or if
> I've truly run into an issue.
> 
> I followed the instructions at http://wiki.koha-community.org/wiki/Sandboxes
> to test this patch, bug 8753. I set up Sandbox 6 from BibLibre succesfully,
> logged in correctly, and did not see any error messages in the 'news' area,
> so it seems the patch was applied successfully.

I have just tested to apply this patch on sandbox 6, I received an email within 5 minutes:
  sandbox is not ready !

There is a conflict with the patch.

Did you see the pref OpacResetPassword?

(In reply to Jonathan Druart from comment #81)
> (In reply to Arturo from comment #80)
> > Hi, all. My library recently switched to Koha, and this is my first attempt
> > at testing a patch, but I am not sure if I am doing this correctly or if
> > I've truly run into an issue.
> > 
> > I followed the instructions at http://wiki.koha-community.org/wiki/Sandboxes
> > to test this patch, bug 8753. I set up Sandbox 6 from BibLibre succesfully,
> > logged in correctly, and did not see any error messages in the 'news' area,
> > so it seems the patch was applied successfully.
> 
> I have just tested to apply this patch on sandbox 6, I received an email
> within 5 minutes:
>   sandbox is not ready !
> 
> There is a conflict with the patch.
> 
> Did you see the pref OpacResetPassword?

Yes, I did see the OpacResetPassword preference, turned it on along with the pref to allow patrons to change their passwords, and went to the homepage and saw the "Forgot my password" link. Below is the result I got when I set up the sandbox. It does seem to start off with a warning, perhaps ("Use of uninitialized value"), but the message does say that the patch for bug 8753 were applied successfully.

The sandbox you've requested is now ready.
The patches attached to bugzilla 8753 have been applied, and the result is:

Updatedatabase:
Use of uninitialized value  in substitution (s///) at /home/koha/src/installer/data/mysql/updatedatabase.pl line 9676.
Upgrade to 3.19.00.004 done (Bug 13346: OpacExportOptions is now multiple) Upgrade to 3.19.00.005 done (Bug 13379 - Modify authorised_values.category to varchar(32)) Upgrade to 3.19.00.006 done (Bug 11944 - Convert DB tables to utf8_unicode_ci) Upgrade to 3.19.00.007 done (Bug 12905: Check budget integrity: OK) Upgrade to 3.19.00.008 done (Bug 12601 - Add new foreign key aqorders.budget_id) Upgrade to 3.19.00.009 done (Bug 13007 - Add new foreign key suggestions.budgetid) Upgrade to 3.19.00.010 done (Bug 5511 - SessionRestrictionByIP) Upgrade to 3.19.00.011 done (Bug 13417: Add permission to delete public lists) Upgrade to 3.19.00.012 done (Bug 13523 - Remove NOT NULL restriction on field marcxml due to mysql STRICT_TRANS_TABLES) Upgrade to 3.19.00.013 done (Bug 11395: Add permission tools_records_batchmod) Upgrade to 3.19.00.014 done (Bug 12648: Add letter ACQ_NOTIF_ON_RECEIV ) Upgrade to 3.19.00.015 done (Bug 11430 - Add primary key for search_history) Upgrade to 3.19.00.016 done (Bug 13380: Add the ORDER_CANCELLATION_REASON authorised value) Upgrade to 3.19.00.017 done (Bug 5786 - Move AllowOnShelfHolds to circulation matrix; Move OPACItemHolds system preference to circulation matrix) Upgrade to 3.19.00.018 done (Bug 10328: Rename opaccolorstylesheet to OpacAdditionalStylesheet Upgrade to 3.19.00.019 done (Bug 9580: Cover image from Coce, a remote image URL cache) Upgrade to XXX done (Bug 8753: Add forgot password link to OPAC) If you use this system for the 1st time, you may find usefull to reach the wiki page http://wiki.koha-community.org/wiki/Sandboxes to have some information about those results, to check if everything went well & if you can test the patch properly

I test on sandbox 13, I have a conflict on installer/data/mysql/updatedatabase.pl
But i solved it manually

I've found :
opac-sendbasket.pl: Error sending mail: Bad or missing From address: ''

You need to specify an address in syspref KohaAdminEmailAddress before testing sending emails.
In UNIMARC database at least.

Works for me with a cart.

(In reply to Fridolin SOMERS from comment #84)
> You need to specify an address in syspref KohaAdminEmailAddress before
> testing sending emails.
> In UNIMARC database at least.
Same for MARC21 database

Patch not loading on sandbox:

The sandbox you've requested is not ready.
Some problems occurred applying patches from bug 8753:
<h1>Something went wrong !</h1>Applying: Bug 8753 - Add forgot password link to OPAC
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/sysprefs.sql
Auto-merging installer/data/mysql/kohastructure.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC

When you have resolved this problem run git bz apply --continue.
If you would prefer to skip this patch, instead run git bz apply --skip.
To restore the original branch and stop patching run git bz apply --abort.
Bug 8753 - Add forgot password link to OPAC

37492 - Bug 8753 - Add forgot password link to OPAC

Apply? [(y)es, (n)o, (i)nteractive] Patch left in /tmp/Bug-8753---Add-forgot-password-link-to-OPAC-QS82WH.patch .

Hi,

I had a chance to take a look at this today. My comments are not code-level, they are "I applied this and had a look at the actual function" level. This is definitely a desirable feature, please don't give up. :)

I saw several issues with this when I was testing:

1. The patch won't apply, it has easily resolved conflicts in database structure and update database. I fixed them for my testing purposes, just wanted you to be aware.

1a. As noted elsewhere, the feature should go into Koha turned off, instead of turned on. We don't want surprises for our users.

2. In the bootstrap theme, we will want to have the "Forgot password" link on the home page beneath the login, on /cgi-bin/koha/opac-user.pl, as well as in the modal box. This can be a follow up.

3. The markup on the opac-password-recovery pages is not consistent with the rest of the OPAC in the Bootstrap theme. This will need to be fixed.

4. On the "reset password" phase ("Password recovery form") - I don't understand why the first field is "Surname in lower case" and the second field is "Confirm password" - the standard behaviour for functions like this is to have something like "Type password" and "repeat password." The function works, but it wasn't clear to me when testing that the "surname" field was actually supposed to be the initial go at the password. It may be a relic from the implementation for your client, but I don't think we want it to be default Koha behaviour for it to suggest the surname as a password. Please fix.

5. *Good thing* submitting the form, (when I realised that both fields are for passwords) works just fine, and the password is reset, and I was able to log in with no issue.

6. Having a mismatch in the passwords on the reset form seems to pop up a box asking me to confirm which user I am changing the password for - this doesn't seem right to me, please check. It doesn't seem to actually *work* but it's a bit scary to see, as it suggests that I could possibly change the password for a user that is not "me."

7. This functionality does not work when you have more than one borrower with the same email address, it presents "We chould not authenticate you as the account owner." I think we may need to use a combination of userid and email address to authenticate users. Userid's do have to be unique, maybe use that instead and pop an error if the user does not have a registered email address. 

8. *Good thing* the email seems to work fine. Maybe minor updates to the text from a native English speaker are needed, but the message and the link are fine. Any changes can come in a follow up.

I understand this is a lot of things to deal with - I think everyone in the community wants this patch/function to be successful, so please say if you need help or if you have any questions.

If you feel like you would rather not continue on with this development, please say so that someone else will feel better about picking up where you left off. 

Cheers,
Liz Rea

A couple of other things that I saw just after I posted the last message - 

- Please check the copyright - should it be InLibro or PTFS Europe?
- It occurs to me that by default, the message queue sends things every 15 minutes and that's a pretty long time in password-reset land.

Hi,

Spectacular, thanks I'll have another look. :)

Cheers,
Liz

Hi,
I tested this with opacpublic enabled and disabled - we probably need to modify the templates to not include the opacnav blocks if opacpublic is disabled. This can be a followup.

I'm not super keen on the syspref text, I may send a followup with text that more clearly differentiates the purpose of this new syspref from OpacPasswordChange.

Otherwise, thank you very much for the updated patch!

Liz

 FAIL   C4/Passwordrecovery.pm
   FAIL   pod
                *** ERROR: Spurious text after =cut  in file C4/Passwordrecovery.pm
*** ERROR: Spurious =cut command  in file C4/Passwordrecovery.pm


 FAIL   opac/opac-password-recovery.pl
   FAIL   valid
                shift on reference is experimental 
                Smartmatch is experimental 


 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 155)

Following test plan, entered a valid email address and received this software error:

Undefined subroutine &C4::Members::Search called at /home/koha/src/opac/opac-password-recovery.pl line 63.

The same happens with a valid user name.

I get this error as well now too. I believe it was introduced by the pushing of bug 12633, which removed C4::Members::Search.

Cheers,
Liz

Hi,

Just wondering if you all at inLibro were going to come back to this bug?

Cheers,
Liz

I think this bug's history shows we are ALWAYS coming back.

We shall fight on the beach,
we shall fight on the tests,
we shall fight on the bugs,
we shall never surrender.

(bur our resources might be stuck on another beachhead right now)

(In reply to Blou from comment #102)
> I think this bug's history shows we are ALWAYS coming back.
> 
> We shall fight on the beach,
> we shall fight on the tests,
> we shall fight on the bugs,
> we shall never surrender.
> 
> (bur our resources might be stuck on another beachhead right now)

This is possibly the best bug reply ever.

:D

Had a look at this, first time using kohadevbox and git bz, so could be something I did or didn't do. Looked good, no stray text, but when I tried resetting a password I got error message 

DBIx::Class::ResultSet::next(): Table koha_kohadev.borrower_password_recovery' doesn't exist at /home/vagrant/kohaclone/C4/Passwordrecovery.pm line 70

Any clues?

Hi Tim, not sure if that's the reason - but did you run the database update? you can run updatedatabase.pl from the command line or change kohaversion.pl to +1 and then after the update is run change it back to the number it had before.

... that should read 'add 1 to the version number at the end...' or similar :)

The system breaks for me if I don't provide an email or username (leave both spaces empty) to send the reset link to.
Get this error: opac-password-recovery.pl: Can't use an undefined value as an ARRAY reference at /home/vagrant/kohaclone/opac/opac-password-recovery.pl line 66

Also could be a good idea to fix the title in the metadata - at the moment it says <title>Waitaki District Libraries catalog

I wasn't able to replicate Aleisha's 2nd issue, it may be a local issue to her environment.

Liz

Signing off! Works for me

 FAIL   opac/opac-password-recovery.pl
   FAIL   spelling
                 doesnt  ==> doesn't
   FAIL   valid
                defined(@array) is deprecated 
                Smartmatch is experimental 
                shift on reference is experimental 
                        (Maybe you should just omit the defined()?)

 FAIL   C4/Passwordrecovery.pm
   FAIL   pod
*** ERROR: Spurious text after =cut  in file C4/Passwordrecovery.pm
*** ERROR: Spurious =cut command  in file C4/Passwordrecovery.pm

 FAIL   koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-password-recovery.tt
   FAIL   spelling
                 occured  ==> occurred

Does the status should be Signoff?

I would say we should not provide any information if the user does not fill correct login or email.
The message should be generic "Bad info" instead of "Bad login" or "Bad email".

From perldoc rand:

rand() is not cryptographically secure. You should not rely on it in security-sensitive situations. As of this writing, a number of third-party CPAN modules offer random number generators intended by their authors to be cryptographically secure, including: Data::Entropy, Crypt::Random, Math::Random::Secure, and Math::TrulyRandom.

1/ Receive the email
2/ Click on the link
3/ Change the pwd
4/ Click again on the link
5/ Change the pwd
6/ You get an error

The error should appear at step 5

comment 21 did not get an answer. For instance tests are still missing.

Glad to see recent movement on this.  Realize it's already made it to QA, but if there's anything I/my library can do to help or just encourage, I'm up for it.

I get the following error message when I try to install patch:

Some problems occurred applying patches from bug 8753:
<h1>Something went wrong !</h1>Applying: Bug 8753 - Add forgot password link to OPAC
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-main.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-auth.tt
Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/includes/masthead.inc
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
Auto-merging installer/data/mysql/en/mandatory/sample_notices.sql
CONFLICT (content): Merge conflict in installer/data/mysql/en/mandatory/sample_notices.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 8753 - Add forgot password link to OPAC

That latest one should resolve the merge issue.

I just squashed my followups to make this a bit tidier, where I could.

Comment on attachment 41192 [details]
Stray text appears above header

This is fixed.

This is a must for 3.24.

I agree, but rebase needed...

Will be done today.

We will never surrender...

I had to resubmit every patches because the first one had the merge conflicts.

If anybody knows how to obsolete the first patch of a queue without messing the queue order, let me know, it would be appreciated.

I think that files kohastructure.sql and sysprefs.sql, should be updated too, not only to add atomicupdate sql file...

(In reply to Josef Moravec from comment #157)
> I think that files kohastructure.sql and sysprefs.sql, should be updated
> too, not only to add atomicupdate sql file...

The changes brought by the atomicupdate file are now visible in the kohastructure and sysprefs files.

I don't think C4::Passwordrecovery is a good namespace. Should this not be something like Koha::Patron::Password::Recovery or Koha::Borrower::Password::Recovery? We should not be adding modules to the C4 namespace at this point.

Koha::Patron please :)

May we have this heavily tested version in, and move it to the new namespace in a follow up? This is an egregiously missing feature, and one that we have been waiting a long, long time for.

It's for the users, let's get it in.

Liz

As the local pedant in the Koha community, I'm with Liz on this one.

While we shouldn't be adding anything to the C4 namespace, I think perhaps an exception should be made for this particular patch set, as the feature is rather important in modernizing Koha.

Out in the world, there's pretty much no service where you can't reset your own password. Yet, you can't do it in Koha. Surely this frustrates users and puts a burden on librarians. It's an aspect of the modern web experience which Koha fails at.

This patch set has been a long time coming. While it should be fairly easy to do a search and replace through the patches, mistakes do happen, and we'd probably need to re-test all the patches again to make sure that nothing was missed. It seems like it's already been tested to the Nth degree, so perhaps a pass should be made this one time.

I suppose it would qualify as technical debt, and I hate technical debt. But this does seem to be an extraordinary case. If it were anything but a password reset, I'd say change the namespace. But this is such a missing piece of functionality in Koha, that I think the faster we get it in the better. 

Of course, I defer to the QA team and the RM. But just adding that perspective to the comments.

As a librarian I can confirm that the lack of this feature frustrates me, my colleagues and our users. We've been waiting for this for a while and would dearly love to have it sooner rather than later.

I can't pretend to understand the ramifications of an imperfect namespace choice, but I know we currently lose users due to them being locked out and going elsewhere rather than approaching us for resets or reminders.

If it can be released as is and fixed later, as Liz suggests, I for one would be very grateful.

(And thanks for everyone's work on it. Looking forward to it, whenever we get it).

Cheers

+1 for getting it in!

After thinking this over for a bit I would be in favor of pushing it as it is. This is a long needed development that probably predates most of the work in the Koha namespace. Changing the namespace now would require more testing and it might give us a lot of conflicts in other places. So it might be easier to fix the namespace separately after this is pushed.

+1 for getting this in ASAP. :-)

In the name of the whole personnel at inLibro, which I've cycled through to support this patch in the past 2.5 years (so many good people fell on the battlefield, see comment #102), I say "yeah! it's about time!".

Of course, we're now at 180 comments and counting, so there's a side of me that would like to see that keep increasing and beat a record of some sort.  ;-)

(In reply to Katrin Fischer from comment #174)
> After thinking this over for a bit I would be in favor of pushing it as it
> is. This is a long needed development that probably predates most of the
> work in the Koha namespace. Changing the namespace now would require more
> testing and it might give us a lot of conflicts in other places. So it might
> be easier to fix the namespace separately after this is pushed.

If it doesn't get fixed now, it won't get fixed. This patch was submitted long after the creation of the Koha namespace. Changing the namespace is not that onerous a request. We can't allow code in that isn't fit just because we like the feature itself.

Noooooo!!!  Not the dreaded "In Discussion" !!

I would have prefered Failed QA.

Not sure what is left to discuss.  Everyone already chimed in.  But I respect the QA's right to fail it.

At this point, since it would require "full retest", I will ask Charles to fix it and SQUASH everything.  Considering the amount of support this feature has, I suppose a re-signoff wouldn't be long.  *sigh*

*sigh*

Oh well, can't be helped I guess. I will test the new patch as soon as it is up.

Hopefully this new super strict QA standard will be across the board and I won't see people pushing patches they wrote skipping qa steps anymore.

Switching back to assigned, while we wait for the new patch.

(In reply to Chris Cormack from comment #179)
> Hopefully this new super strict QA standard will be across the board and I
> won't see people pushing patches they wrote skipping qa steps anymore.

I do not favor a super strict QA standard. Common_sense++

(In reply to Marcel de Rooy from comment #180)
> (In reply to Chris Cormack from comment #179)
> > Hopefully this new super strict QA standard will be across the board and I
> > won't see people pushing patches they wrote skipping qa steps anymore.
> 
> I do not favor a super strict QA standard. Common_sense++

Neither do I, but if we are going to be super strict about this one, we need to at least be consistent. That was my point.

Kyle, can we imagine to make an exception for this one, open a new bug report to move the new module to the Koha namespace and ask for the author to commit to submit a patch?

(In reply to Blou from comment #176)
> Of course, we're now at 180 comments and counting, so there's a side of me
> that would like to see that keep increasing and beat a record of some sort. 
> ;-)

Don't worry about that, we won't reach it (there are 247 comments on bug 7167 and it as been abandoned, there is another one with more than 500 comments iirc).

(In reply to Jonathan Druart from comment #182)
> Kyle, can we imagine to make an exception for this one, open a new bug
> report to move the new module to the Koha namespace and ask for the author
> to commit to submit a patch?

Yes! I set this bug to in discussion while we were actually discussing the issue. I purposefully did not set it to failed qa.

My fear is that if this is pushed as is, there is no impetus for the namespace to be fixed. All I'd like to see is a commitment from someone that the patches to move this to a better namespace *will be submitted*, preferably before the next major release if possible. So far I haven't seen anyone make that commitment. I should have written that more clearly than I had.

Philippe, if you can vouch that Charles or someone will submit the patch to move this module to a new namespace, I have no problem with it proceeding as is. Now would be the time to file the followup bug report and assign it to the proper person. If this works for you please go ahead and reset the status to Signed Off.

This is not a problem for us.  To be sure, just create the new bug and assign it to me immediately.  I assure you you'll get a patch within a week after this has been pushed to master.

(In reply to Blou from comment #185)
> This is not a problem for us.  To be sure, just create the new bug and
> assign it to me immediately.  I assure you you'll get a patch within a week
> after this has been pushed to master.

Thanks! I'll go ahead and take care of that!

(In reply to Chris Cormack from comment #179)
> Hopefully this new super strict QA standard will be across the board and I
> won't see people pushing patches they wrote skipping qa steps anymore.

I'd like to address this. I'm not in favor of super strict QA standards. What I'm in favor of are transparent, standardized, and consistently applied QA rules.

My experience as a developer has informed my work as a QA team member. I have had to rewrite C4 modules as Koha modules, I've asked others to rewrite C4 modules as Koha modules. Code quality guidelines should be completely divorced from the necessity or popularity of a bugfix or feature.

I think our coding guidelines need a bit of a revamp to make sure all of the things we've taken for granted as being necessary to pass QA are actually in the coding guidelines. If a QA team member fails QA on a patch, the QA'er should be able to link directly to the reason and explanation for it failing QA. If the QA'er believes the patch should fail QA but the reason is not part of the coding guidelines, then a meeting should be called, a vote should be made, and a new rule added to the guidelines.

While I have put coding guideline additions on the agenda a lot of times in the past, I don't believe that we can ever have a complete set of rules that will fit every situation and possible question.

Another common practice we QA has used where they haven't been sure about something is asking for more opinions - I think this has helped in a lot of cases to get more views and a better picture of the problem and possible solutions.

That said, I am happy with the 2 step approach here.

(In reply to Katrin Fischer from comment #188)
> While I have put coding guideline additions on the agenda a lot of times in
> the past, I don't believe that we can ever have a complete set of rules that
> will fit every situation and possible question.
> 
> Another common practice we QA has used where they haven't been sure about
> something is asking for more opinions - I think this has helped in a lot of
> cases to get more views and a better picture of the problem and possible
> solutions.

That simply highlights the problem. With this methodology, a developer will not know if code will pass qa or not until it has been written and submitted.

I agree, we will never have a complete set of rules, but we can build that set as we go. The more we have written down for developers to be aware in advance, the better. We need documented guidelines. The approach of "this doesn't feel right to me" just doesn't work any more. It can only lead to future frustration for developers. We can't have one set of rules for bug A and another set of rules for bug B.

I think we are actually not in disagreement - I just want to warn about wanting to solve everything with rules. That will be trying for the impossible. And a 20 pages catalog of guidelines... is kind of scaring away devs too.

(In reply to Katrin Fischer from comment #190)
> I think we are actually not in disagreement - I just want to warn about
> wanting to solve everything with rules. That will be trying for the
> impossible. And a 20 pages catalog of guidelines... is kind of scaring away
> devs too.

Lol, yeah, I agree with that too. I just think at the moment we have too few guidelines on paper and too many guidelines only in the heads of the QA team ; )

I also want to add that the guidelines would be an explanation of they something failed qa. Instead of "Here is a link to the community decided reason" it is currently more of a "because I said so" situation. I personally dislike failing qa on something without having those reasons. I'm far more apt to submit follow ups to avoid doing so.

QA: Working on this one..

Created attachment 47164 [details] [review]
Bug 8753 - Add forgot password link to OPAC

I've addressed a lot of Liz Rea's points.

1.  I have moved the code from updatedatabase.pl and kohastructure.sql to a file in the atomicupdates directory.
1a. The feature is now off by default when the atomicupdate is run.

2.  The password reset link is now visible on the home page, in the modal box and on opac-user.pl .

3.  The password recovery pages now use bootstrap markup.

4.  I am unsure here. I see "New Password:" and "Confirm new password:".

5.  This should still work :).

6.  I could not reproduce.

7.  I have added the userid field.
    You can now reset the password by submitting either your useid or email address.
    Both fields can be filled, but the email address must be one of the borrower's (email, emailpro or b_email).
    When entering only the email address and two borrowers use that same address, the system tells the user to try with another address or to specify his userid.

8.  The text is in the atomicupdate file. Have at it, anyone.

Concerning the email. It is inconvenient for the use to have to wait X minutes for the message queue the be processed.
Maybe we could add a sub in Letters.pm that:
    Takes the same argments as EnqueueLetter
    Sends the letter.
    Saves the letter in the message queue with a 'sent' status.

 TEST PLAN:

Setup)
    1) apply the patch
    2) go to system preferences OPAC>>Privacy and set 'OpacResetPassword' to ON.
    2b) make sure that OpacPasswordChange is also ON.
A)
    1) refresh front page, click on 'Forgot your password' and enter a VALID address
    1b) Also try an INVALID address (valid yet not in your koha db).  An error message will show up.
    2) An email should be received at that address with a link.
    3) Follow the link in the mail to fill the new password.
    Until a satisfactory new password is entered, the old password is not reset.
    4) Go to main page try the new password.
B)
    1) Repeat the password reset, this time use the userid (username) field.
    2) Try to reset the password using a userid and an email not linked to the account. An error appears.
    3) Make sure the borrower has many available email addresses.
    4) For each email, reset the password using both the userid and the email. The link should be sent to the specified address
C)
    1) Make sure two borrowers use the same email.
    2) Repeat the reset procedure in test case A). An error message appears

http://bugs.koha-community.org/show_bug.cgi?id=13068

 Author:    Maxime Beaulieu <maxime.beaulieu@inlibro.com>

Followed test plan. Works as described.
Signed-off-by: Marc Veron <veron@veron.ch>

New sign-off after testing all patches together
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 47165 [details] [review]
Bug 8753 - Followup - change value text on syspref

Followup changes text from "The user can reset | can not reset their password on OPAC" to "Library users are allowed | not allowed to recover their password via e-mail in the OPAC"

This change more clearly differentiates the purpose of this new preference from OpacPasswordChange.

Bug 8753 - followup - update text for link to match common UI paradigms, fixes OpacPublic disabled view

Also corrects OpacNav being included on the reset page on private catalogues.

Updated the link for forgotten passwords to more closely match common UI paradigms, i.e. Facebook and Twitter

To test:
apply all patches, link should now be the less verbose "Forgot your password?"
disable OpacPublic, anything in opacnav should not appear (you may need to add something to opacnav to test properly)

Bug 8753 - [followup] fix the title on opac-password-recovery.tt

The title stanza was missing a <title></title> around it, causing the extra text to appear.

To test, apply all patches and make sure it looks ok and there is no extra text at the top or bottom of the page.

Bug 8753 - [followup} Correcting spelling mistakes

Make sure it all still works

Bug 8753 - [followup] fix error when no information is provided

To test:

All normal checks plus make sure that a nice error is displayed when no data is provided.

fixing the deprecated thing

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 47166 [details] [review]
Bug 8753 - Use Koha::Borrowers instead of C4::Members

Use the new library to search for borrowers.
Changed how the $borrower variable is used since it is now a Koha::Borrower object.

Removed the $protocol parameter from the generated link. It should be included in the OPACBaseURL syspref.

	modified:   C4/Passwordrecovery.pm
	modified:   opac/opac-password-recovery.pl

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 47167 [details] [review]
Bug 8753 - [followup] POD cleanup

Check to make sure the messages from the qa checker are gone.

Note I'm not sure about this one because my test tools are apparently broken, and I can't get the error to manifest.

Amended patch by Jonathan Druart: Add a blank line before =head2
It now passes.

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 47168 [details] [review]
Bug 8753 - Smartmatch substitute, Math::Random::Secure, Perltidy, Passwordrecovery.t

This is a collection of changes taken from different comments (but mostly comment 21 and comment 122).

Passes qa and prove, on my machine at least.

There's also a new test file, Passwordrecovery.t, which covers every method of C4::Passwordrecovery.

To test:

All normal checks plus :

    1/ Receive the email
    2/ Click on the link
    3/ Change the pwd
    4/ Click again on the link
    5/ You should immediately get an error message

Problems with Math/Random/Secure.pm, is solved in following patch, signing off
Signed-off-by: Marc Veron <veron@veron.ch>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 47169 [details] [review]
Bug 8753 - Various little things - removing new dependency, changes to errors, textual updates

Koha already has a sub that creates salts, so lets use that instead of math::Random::secure, so as not to add a new dependency.

Made the references to "Forgotten password" consistent, including adding it to the title of the page.

Also removed the individual error for "this email doesn't belong to this account" as that could expose the existence of a login, which I think we'd rather not do.

Made some of the text more grammatically correct, and more library specific.

To test:

Apply on top of all of the other patches.

All the usual checks, plus make sure there are no typos in any text references.

Signed-off-by: Marc Veron <veron@veron.ch>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 47170 [details] [review]
Bug 8753 - propagating the changes to kohastructure.sql and sysprefs.sql

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Looks good with a new install.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 47171 [details] [review]
Bug 8753: [QA Follow-up] Primary key and collation

This patch includes:
[1] Adds primary key borrowernumber to new table.
[2] Fixes collation.
[3] Removes manual PK in DBIx schema file.
[4] Fixes typo CompletePasswordRevovery.
[5] Removes use strict from opac-password-recovery; Modern::Perl is used.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

QA Comment:
With reference to the discussion above (and time passed), I am moving the status to Passed QA. 
Looks good to me. Added a small follow-up.
An important detail to fix before the new release is: New notice should be added in all language files! Please fix that asap in a follow-up report after this one has been pushed. This would normally be a reason to Fail QA on itself...
Note to RM: Watch the conflict with bug 15548 (Borrower vs Patron); and the promised fix under 15585.

Minor points:
Spotted "FIXME: That ugly shift-grep contraption".
The term "Forgotten password recovery"  sounds funny to me (no native speaker :) Is Password recovery not sufficient?
Are dots in the unique key (especially at the end) user friendly? 

Passed QA

Pushed to Master - Should be in the May 2016 release.  Thanks!

Blou - please don't forget the 2 follow-ups for the namespace and the notices (last comment from Marcel!)

(In reply to Katrin Fischer from comment #204)
> Blou - please don't forget the 2 follow-ups for the namespace and the
> notices (last comment from Marcel!)

Yes. Please add the bug number in the See also or Blocks too.
Thnx

Brendan
It seems that you forgot to move the atomicupdate revision to the code in updatedatabase.
Currently, part of your job as RM :)

Marcel

(In reply to Marcel de Rooy from comment #206)
> Brendan
> It seems that you forgot to move the atomicupdate revision to the code in
> updatedatabase.
> Currently, part of your job as RM :)
> 
> Marcel

Yes thanks.

Created attachment 47366 [details] [review]
[PASSED QA] Bug 8753 [QA Followup] - Add notice to translations

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Follow up Pushed (Jan 27 - 2016)

The follow-up patch to change the namespace of this development is in bug 15585

Comment on attachment 47164 [details] [review]
Bug 8753 - Add forgot password link to OPAC

Review of attachment 47164 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref
@@ +357,3 @@
>              - pref: OPACPatronDetails
>                choices:
>                    yes: Allow

When I am applying this (v.3.22.06), OPAC Global preference page (cgi-bin/koha/admin/preferences.pl?tab=opac) is showing the following error 

Syck parser (line 356, column 0): syntax error at /usr/lib/perl5/YAML/Syck.pm line 72.

I have also tried with changing the syntax as 
    - pref: OpacResetPassword
              default: 1
              choices:
                  yes: "can reset"
                  no: "can not reset"
			-The user can reset their password on OPAC.	
		-
But no luck. 
Please review.

The patches here have been written for the next version 16.05 (3.24) and are not supposed to work with any earlier code. It's a completely new feature that will only be available in new versions.