Bug 20023 - Password recovery should be case insensitive
Summary: Password recovery should be case insensitive
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Blou
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 21771
  Show dependency treegraph
 
Reported: 2018-01-17 20:57 UTC by Blou
Modified: 2018-11-06 14:12 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 20023: Password recovery using email should be case insensitive (1.67 KB, patch)
2018-09-17 20:38 UTC, Blou
Details | Diff | Splinter Review
Internal server error at opac-password-recovery.pl after applying patch (24.51 KB, image/png)
2018-09-19 05:06 UTC, Dilan Johnpullé
Details
Bug 20023: Password recovery using email should be case insensitive (1.67 KB, patch)
2018-09-19 11:48 UTC, Blou
Details | Diff | Splinter Review
Bug 20023: Password recovery using email should be case insensitive (1.72 KB, patch)
2018-09-19 20:09 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20023: Password recovery using email should be case insensitive (1.76 KB, patch)
2018-09-21 09:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20023: (QA follow-up) Tiny simplification (1.45 KB, patch)
2018-09-21 09:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20023: Password recovery using email should be case insensitive (1.81 KB, patch)
2018-09-21 09:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20023: (QA follow-up) Tiny simplification (1.50 KB, patch)
2018-09-21 09:41 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20023: (QA follow-up) Tiny simplification (1.48 KB, patch)
2018-09-24 07:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20023: (QA follow-up) Tiny simplification (1.46 KB, patch)
2018-09-24 07:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20023: (Follow-up) Revert the shift statement (1.24 KB, patch)
2018-11-06 14:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2018-01-17 20:57:14 UTC
When entering an email (or arguably, a userid) to recover the password, a user should not have to know or remember the casing of the given email.

Quick exercise for a quick solution led to no results.

$search_results = [ Koha::Patrons->search( { -or => { email => $email, emailpro => $email, B_email  => $email } } ) ];

Using search_like is not available, and using email => { 'like', $email} does not lead to the intended result.  Here's hoping a similarly simple fix exists.
Comment 1 Blou 2018-09-17 20:38:57 UTC
Created attachment 79037 [details] [review]
Bug 20023: Password recovery using email should be case insensitive

When entering an email to recover the password, a user should not have to know or remember the casing of the given email.

Test
0) enable OpacResetPassword
1) On the OPAC, click "Forgot your password"
2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING
3) Submit.  This will fail.
4) Apply the patch, redo with success.
Comment 2 Blou 2018-09-17 20:40:44 UTC
I added a small modif to clean some warnings in the logs, while at it.
Comment 3 Dilan Johnpullé 2018-09-19 05:06:48 UTC
Created attachment 79070 [details]
Internal server error at opac-password-recovery.pl after applying patch

After applying the patch, opac-password-recovery.pl resolves at a screen complaining of an internal server error. Restarting plack, apache and memcached does not resolve this.
Comment 4 Blou 2018-09-19 11:48:42 UTC
Created attachment 79086 [details] [review]
Bug 20023: Password recovery using email should be case insensitive

When entering an email to recover the password, a user should not have to know or remember the casing of the given email.

Test
0) enable OpacResetPassword
1) On the OPAC, click "Forgot your password"
2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING
3) Submit.  This will fail.
4) Apply the patch, redo with success.
Comment 5 Blou 2018-09-19 11:54:55 UTC
Sorry about that. :(
Now fixed.
Comment 6 Michal Denar 2018-09-19 20:09:48 UTC
Created attachment 79125 [details] [review]
Bug 20023: Password recovery using email should be case insensitive

When entering an email to recover the password, a user should not have to know or remember the casing of the given email.

Test
0) enable OpacResetPassword
1) On the OPAC, click "Forgot your password"
2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING
3) Submit.  This will fail.
4) Apply the patch, redo with success.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 7 Marcel de Rooy 2018-09-21 09:41:08 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2018-09-21 09:41:14 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2018-09-21 09:41:50 UTC
Created attachment 79203 [details] [review]
Bug 20023: Password recovery using email should be case insensitive

When entering an email to recover the password, a user should not have to know or remember the casing of the given email.

Test
0) enable OpacResetPassword
1) On the OPAC, click "Forgot your password"
2) Enter your email address as in your user account BUT WITH A DIFFERENT CASING
3) Submit.  This will fail.
4) Apply the patch, redo with success.

Signed-off-by: Michal Denar <black23@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-09-21 09:41:55 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2018-09-21 13:20:10 UTC
Comment on attachment 79204 [details] [review]
Bug 20023: (QA follow-up) Tiny simplification

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

::: opac/opac-password-recovery.pl
@@ +73,4 @@
>          $errMultipleAccountsForEmail = 1;
>      }
>      elsif ( $borrower = $search_results->next() ) {    # One matching borrower
> +        my @emails = grep { $_ && !/^\s+$/ } ( $borrower->email, $borrower->emailpro, $borrower->B_email );

Not sure this is simplification :)

@@ +77,2 @@
>  
> +        my $firstNonEmptyEmail = @emails ? $emails[0] : undef;

So simplify it, really :)
  my $firstNonEmptyEmail = shift @emails;
Comment 12 Marcel de Rooy 2018-09-24 07:15:11 UTC
Staying sharp, Joubu. Great.
I removed the regex for spaces. It wasn't there before and you can't save three spaces as an email address anymore. Using the shift too. So simple :)
Comment 13 Marcel de Rooy 2018-09-24 07:15:37 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2018-09-24 07:17:40 UTC
Created attachment 79271 [details] [review]
Bug 20023: (QA follow-up) Tiny simplification

Remove empty emails from the list rightaway.
Would be tempted to lc the params->{email} at the start btw..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Nick Clemens 2018-09-26 19:26:21 UTC
Awesome work all!

Pushed to master for 18.11
Comment 16 Martin Renvoize 2018-09-27 12:28:49 UTC
Pushed to 18.05.x for 18.05.05
Comment 17 Fridolin SOMERS 2018-10-03 13:12:44 UTC
Pushed to 17.11.x for 17.11.11
Comment 18 Katrin Fischer 2018-11-06 13:36:26 UTC
The tiny follow-up broke password recovery on 17.11 (haven't tested on other versions yet. Reverting fixes the problem.

The problem: No matter what you enter, no matching account will be found, even if the email address and all data are entered correctly.

The simplification here is almost as long as the original patch, I feel it was not good to push this as a QA follow-up without more testing. (Sorry)
Comment 19 Marcel de Rooy 2018-11-06 13:37:49 UTC
(In reply to Katrin Fischer from comment #18)
> The tiny follow-up broke password recovery on 17.11 (haven't tested on other
> versions yet. Reverting fixes the problem.
> 
> The problem: No matter what you enter, no matching account will be found,
> even if the email address and all data are entered correctly.
> 
> The simplification here is almost as long as the original patch, I feel it
> was not good to push this as a QA follow-up without more testing. (Sorry)

Will have a look
Comment 20 Marcel de Rooy 2018-11-06 13:49:06 UTC
(In reply to Katrin Fischer from comment #18)
> The tiny follow-up broke password recovery on 17.11 (haven't tested on other
> versions yet. Reverting fixes the problem.

Works on current master. Looking further.
Comment 21 Marcel de Rooy 2018-11-06 13:52:34 UTC
(In reply to Marcel de Rooy from comment #20)
> (In reply to Katrin Fischer from comment #18)
> > The tiny follow-up broke password recovery on 17.11 (haven't tested on other
> > versions yet. Reverting fixes the problem.
> 
> Works on current master. Looking further.

Hmm. Userid works but email might not..
Comment 22 Katrin Fischer 2018-11-06 13:56:56 UTC
We were testing with email.
Comment 23 Marcel de Rooy 2018-11-06 13:57:35 UTC
Found the cause. How stupid!
Comment 24 Marcel de Rooy 2018-11-06 13:59:14 UTC
Not to blame anyone. But replacing
  my $firstNonEmptyEmail = @emails ? $emails[0] : undef;
by the shift @emails obviously was not so smart after all.
Because we still test @emails later on !
Comment 25 Marcel de Rooy 2018-11-06 14:00:44 UTC
Patch forthcoming
Comment 26 Katrin Fischer 2018-11-06 14:03:14 UTC
Hi Marcel, can you attach the fix on bug 21771? It's already a bug in released versions, so it will be more clear for release notes.
Comment 27 Marcel de Rooy 2018-11-06 14:05:33 UTC
Created attachment 81985 [details] [review]
Bug 20023: (Follow-up) Revert the shift statement

Obviously you should never shift an items from an array if you need that
item later on :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with entering userid as well as entering email..
Comment 28 Marcel de Rooy 2018-11-06 14:06:35 UTC
Attention RM and Rmaints : Please pick LAST follow-up.
Comment 29 Marcel de Rooy 2018-11-06 14:07:59 UTC
(In reply to Katrin Fischer from comment #26)
> Hi Marcel, can you attach the fix on bug 21771? It's already a bug in
> released versions, so it will be more clear for release notes.

OK
Comment 30 Marcel de Rooy 2018-11-06 14:11:07 UTC
Comment on attachment 81985 [details] [review]
Bug 20023: (Follow-up) Revert the shift statement

Moved to 21771
Comment 31 Marcel de Rooy 2018-11-06 14:12:58 UTC
Sorry for all confusion people. But fixed ;)