Bug 16711

Summary: OPAC Password recovery: Handling if multiple accounts have the same mail address
Product: Koha Reporter: Marc Véron <veron>
Component: OPACAssignee: Marc Véron <veron>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: cbrannon, dcook, fridolin.somers, jonathan.druart, katrin.fischer, mtj, nick, sally.healey, severine.queune, techservspec
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=2237
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11943
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12566
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18616
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18955
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 18616    
Attachments: Bug 16711: OPAC Password recovery: Handling if multiple accounts have the same mail address
[SIGNED-OFF] Bug 16711: OPAC Password recovery: Handling if multiple accounts have the same mail address
Added the code from bug 18616, which allows patron to also use their card number as a login id since there were merge conflicts with that patch.
Bug 16711: (QA-followup) Use count directly
Bug 16711: OPAC Password recovery: Handling if multiple accounts have the same mail address
Bug 16711: (QA-followup) Use count directly

Description Marc Véron 2016-06-10 18:35:51 UTC
If multiple accounts have the same email address set as primary email and you ask for password recovery with only filling in the email address, a email is sent for one account only.

Example:
Create 3 Accounts, login names are test01, test02, test 03, Email is the same for all.

Go to OPAC -> Password recovery and indicate E-Mail only

You will get a mail for only one of the accounts above, something like:

---------
This email has been sent in response to your password recovery request for the account test01

You can now create your new password using the following link:
http://.......
(...)
---------
Comment 1 Christopher Brannon 2016-10-10 14:20:55 UTC
Perhaps require the library card number associated with the account when requesting the recovery/reset?
Comment 2 David Cook 2016-10-10 23:05:51 UTC
I wonder how having multiple accounts with the same email address would affect the Mozilla Persona stuff.

I seem to recall Chris saying that an email address should only belong to one account, although that's not enforced anywhere...
Comment 3 Christopher Brannon 2016-10-10 23:16:49 UTC
(In reply to David Cook from comment #2)
> I wonder how having multiple accounts with the same email address would
> affect the Mozilla Persona stuff.
> 
> I seem to recall Chris saying that an email address should only belong to
> one account, although that's not enforced anywhere...

It's not a realistic expectation.  Are parents supposed to start a new e-mail account for every child that has a library card???
Comment 4 Christopher Brannon 2016-10-10 23:17:50 UTC
(In reply to Christopher Brannon from comment #3)
> (In reply to David Cook from comment #2)
> > I wonder how having multiple accounts with the same email address would
> > affect the Mozilla Persona stuff.
> > 
> > I seem to recall Chris saying that an email address should only belong to
> > one account, although that's not enforced anywhere...
> 
> It's not a realistic expectation.  Are parents supposed to start a new
> e-mail account for every child that has a library card???

That being said, it may have to come down to either logging in with the card number or a combination of e-mail and card number.
Comment 5 David Cook 2016-10-11 06:14:10 UTC
(In reply to Christopher Brannon from comment #3)
> It's not a realistic expectation.  Are parents supposed to start a new
> e-mail account for every child that has a library card???

That's certainly a valid point. I'm just pointing out that it will probably cause unexpected behaviour when using Mozilla Persona, but I don't know how many people actually use Persona anyway.

As for the password reset, yeah I think it would make sense to provide userid or cardnumber as that's the unique identifier used for the patron, no? 

You could just put in your cardnumber/userid, and then get a message saying:

"A password reset email has been send to the email address registered for account test01"

Or something like that.

We actually have a local password reset service... I should look how I implemented it...
Comment 6 David Kuhn 2017-05-20 23:49:43 UTC
I second the idea that the card number/user id should be required to reset a password, since that should always be a unique identifier.
Comment 7 Marc Véron 2017-05-21 05:34:30 UTC
Thinking loud about sending information about all library accounts if in password recovery an email with multiple library accounts attached is given:

What could be the reasons to have one email address and multiple accounts?

- An individual has multiple library accounts, e.g. one for professional purposes and a private one. For exammple a teacher needs books for school and wants to separate that from their private lecture.

In that case sending a reset links for multiple accounts would be OK (no privacy breach) because it is the same person. But atm the mail contains only a reset link, no information that would help to identifiy the account. So the individual could reset the wrong password.

- A group (family, couple) shares the same email but they have different library accounts

In that case, privacy is defined by the group: Either one individual has access to the email or all of the group have access. If all have access, one individual could change the password for an other one. That would be a privacy breach.

OK then, it is not a good idea to send reset links for all accounts attached to an email account.

IMO the easiest solution would be not to send recovery link(s), but to display an information similar to the one that appears if an email address is not found in the database (Error No account was found with the provided information. Please...)

The message could be something like: 
-------------
Information
Multiple library accounts are attached to this email. Please fill the field 'Login' to identify which password you want to reset. Please contact the library if you need further assistance.
-------------

Additionally, there could be more enhancements (to be covered by separate bugs):
- Have a syspref to prevent multiple accounts with same email (could get complicated for existing installs)
- Have a precooked report that displays library accounts sharing one email address
Comment 8 Marc Véron 2017-05-21 16:40:05 UTC Comment hidden (obsolete)
Comment 9 Josef Moravec 2017-05-22 08:35:17 UTC
Created attachment 63600 [details] [review]
[SIGNED-OFF] Bug 16711: OPAC Password recovery: Handling if multiple accounts have the same mail address

To reproduce:
- Create 3 Accounts, login names are test01, test02, test03, Email is the same
for all.
- Go to OPAC -> Password recovery and indicate E-Mail only
- You will get an email for only one of the accounts above.

To test:
- Apply patch, restart memcached and plack
- Go to db, delete from borrower_password_recovery;
- Try steps above to reproduce. You will get an error message:
    Account identification with this email address only is ambiguous.
    Please use the field 'Login' as well.
- Verify that other cases work as before (provide valid / invalid login only,
  provide valid email for an existing account, provide unknown email, provide
  both login and email with all combinations of valid / invalid)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 David Kuhn 2017-06-06 16:43:35 UTC
Created attachment 64064 [details] [review]
Added the code from bug 18616, which allows patron to also use their card number as a login id since there were merge conflicts with that patch.
Comment 11 Marc Véron 2017-06-06 19:31:18 UTC
Comment on attachment 64064 [details] [review]
Added the code from bug 18616, which allows patron to also use their card number as a login id since there were merge conflicts with that patch.

Made 18616 dependent on 16711 and moved David's patch back to 18616, and signed off 18616
Comment 12 Marc Véron 2017-06-20 15:12:46 UTC
Still applies on current master
Comment 13 Nick Clemens 2017-06-23 15:14:56 UTC
opac-password-recovery.pl:
$search_results = [ Koha::Patrons->search( { -or => { email => $email, emailpro => $email, B_email  => $email } } ) ];
$resultCount = Koha::Patrons->search( { -or => { email => $email, emailpro => $email, B_email  => $email } } ) -> count;

It seems we could just not get the results as an array and use count directly, casting scalar seems unnecessary as well here

i.e.
$search_results = Koha::Patrons->search( { -or => { email => $email, emailpro => $email, B_email  => $email } } );
...
if ( not $search_results ) {
...
elsif ( $username && $search_results->count > 1)
...
etc.
Comment 14 Marc Véron 2017-06-23 15:49:54 UTC
Created attachment 64598 [details] [review]
Bug 16711: (QA-followup) Use count directly

See comment # 13
Comment 15 Marc Véron 2017-06-23 15:50:40 UTC
Hope it's OK to set back to SO.
Comment 16 Nick Clemens 2017-06-23 22:03:27 UTC
Created attachment 64604 [details] [review]
Bug 16711: OPAC Password recovery: Handling if multiple accounts have the same mail address

To reproduce:
- Create 3 Accounts, login names are test01, test02, test03, Email is the same
for all.
- Go to OPAC -> Password recovery and indicate E-Mail only
- You will get an email for only one of the accounts above.

To test:
- Apply patch, restart memcached and plack
- Go to db, delete from borrower_password_recovery;
- Try steps above to reproduce. You will get an error message:
    Account identification with this email address only is ambiguous.
    Please use the field 'Login' as well.
- Verify that other cases work as before (provide valid / invalid login only,
  provide valid email for an existing account, provide unknown email, provide
  both login and email with all combinations of valid / invalid)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens 2017-06-23 22:03:30 UTC
Created attachment 64605 [details] [review]
Bug 16711: (QA-followup) Use count directly

See comment # 13

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Jonathan Druart 2017-07-06 17:54:49 UTC
Patches will be pushed squashed.
Comment 19 Jonathan Druart 2017-07-06 18:48:25 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 20 Fridolin Somers 2017-07-12 10:00:59 UTC
Enhancement not pushed to 17.05.x
Comment 21 Katrin Fischer 2017-07-14 11:20:13 UTC
I wonder if we should treat this as a bug instead, as it causes a problem for libraries with family accounts. What do you think?
Comment 22 Marc Véron 2017-07-15 16:22:06 UTC
(In reply to Katrin Fischer from comment #21)
> I wonder if we should treat this as a bug instead, as it causes a problem
> for libraries with family accounts. What do you think?

+1
Comment 23 Jonathan Druart 2017-07-17 18:15:46 UTC
Sent back to the RMaint queue.
Comment 24 Fridolin Somers 2017-07-19 07:13:33 UTC
(In reply to Katrin Fischer from comment #21)
> I wonder if we should treat this as a bug instead, as it causes a problem
> for libraries with family accounts. What do you think?

Ok I'm in.
Patch is small indeed
Comment 25 Fridolin Somers 2017-07-19 07:20:02 UTC
Pushed to 17.05.x, will be in 17.05.02.

If userid was mandatory, there would be no problem and this form would be more protected against attacks I's say.
Comment 26 Katrin Fischer 2017-07-19 10:14:54 UTC
In some libraries the patrons simply don't know their userid as it's not used by the library. Koha generates it, but the patrons don't know about it. So I don't think it should be mandatory.
There is another patch I think would be very useful for 17.05 and 16.11 - bug 18616, that will also allow to use the cardnumber.
Comment 27 Katrin Fischer 2017-07-27 23:07:50 UTC
Contains strings - holding this for next 16.11.x release.
Comment 28 Katrin Fischer 2017-08-01 05:19:19 UTC
This patch has been pushed to 16.11.x and will be in 16.11.10.
Comment 29 Katrin Fischer 2017-08-01 05:31:36 UTC
Sorry, 16.11.11!
Comment 30 Mason James 2017-11-23 21:12:11 UTC
Pushed to 16.05.x, for 16.05.16 release