Bug 12371 - Links in every patron self-registration email points to a single borrower
Summary: Links in every patron self-registration email points to a single borrower
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P1 - high critical (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-05 14:57 UTC by Barton Chittenden
Modified: 2019-06-27 09:24 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12371 - Links in every patron self-registration email points to a single borrower (1.67 KB, patch)
2014-08-15 16:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 12371 - Links in every patron self-registration email points to a single borrower (1.85 KB, patch)
2014-08-16 20:52 UTC, Chris Cormack
Details | Diff | Splinter Review
[PASSED QA] Bug 12371 - Links in every patron self-registration email points to a single borrower (1.99 KB, patch)
2014-08-21 07:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2014-06-05 14:57:14 UTC
Library has the following system preferences set for Patron Self Registration:

*************************** 1. row ***************************
   variable: PatronSelfRegistration
      value: 1
    options: NULL
explanation: If enabled, patrons will be able to register themselves via the OPAC.
       type: YesNo
*************************** 2. row ***************************
   variable: PatronSelfRegistrationAdditionalInstructions
      value: Thank you for registering for a library card.<br>

To complete your registration, please come to your library (Cook, Jackson, or Madison) with identification documents, that show:
<ul> 
<li>proof of residency (physical address) </li>
<li>mailing address</li>
<li>photo ID</li>
</ul>
<br>
We will save your information for 30 days, please stop in soon!
    options: NULL
explanation: A free text field to display additional instructions to newly self registered patrons.
       type: free
*************************** 3. row ***************************
   variable: PatronSelfRegistrationBorrowerMandatoryField
      value: surname|firstname|email|phone
    options: NULL
explanation: Choose the mandatory fields for a patron's account, when registering via the OPAC.
       type: free
*************************** 4. row ***************************
   variable: PatronSelfRegistrationBorrowerUnwantedField
      value: sex
    options: NULL
explanation: Name the fields you don't want to display when registering a new patron via the OPAC.
       type: free
*************************** 5. row ***************************
   variable: PatronSelfRegistrationDefaultCategory
      value: ONLINEREG
    options: 
explanation: A patron registered via the OPAC will receive a borrower category code set in this system preference.
       type: free
*************************** 6. row ***************************
   variable: PatronSelfRegistrationExpireTemporaryAccountsDelay
      value: 30
    options: NULL
explanation: If PatronSelfRegistrationDefaultCategory is enabled, this system preference controls how long a patron can have a temporary status before the acc
ount is deleted automatically. It is an integer value representing a number of days to wait before deleting a temporary patron account. Setting it to 0 disabl
es the deleting of temporary accounts.
       type: Integer
*************************** 7. row ***************************
   variable: PatronSelfRegistrationVerifyByEmail
      value: 1
    options: NULL
explanation: If enabled, any patron attempting to register themselves via the OPAC will be required to verify themselves via email to activate his or her acco
unt.
       type: YesNo

---

The self registration email is sent emails contining unique tokens, but all of the tokens are associated with borrowernumber 0:

select verification_token, count(verification_token), borrowernumber from  borrower_modifications where verification_token is not null group by verification_token;
+----------------------------------+---------------------------+----------------+
| verification_token               | count(verification_token) | borrowernumber |
+----------------------------------+---------------------------+----------------+
| 8748c0cf981f3fadd6ec78433a5beddb |                         1 |              0 |
| 96738d8a27251326a7eb3971b1a03eb5 |                         1 |              0 |
| a5de1159d20fd3ed3f4a8d189db3fb32 |                         1 |              0 |
| aa131a40d93057e676c363a3e11be060 |                         1 |              0 |
| adf1bbfd6715714e9e8642e38d0c4462 |                         1 |              0 |
| b28898063358455c350e7eff9dc4cb94 |                         1 |              0 |
| ba24d496a711aa07f4a20a99c10c6d58 |                         1 |              0 |
| e035486fb47e2b6f3af641550098f6ea |                         1 |              0 |
| ec09849390835652149736252010b66e |                         1 |              0 |
| fdf76c060abb49b2ad1dc7aa2bdea7fb |                         1 |              0 |
+----------------------------------+---------------------------+----------------+
10 rows in set (0.00 sec)

This means that when a patron clicks on the link to verify their identity, they will all connect to the same patron record.
Comment 1 Marcel de Rooy 2014-07-21 10:48:43 UTC
Barton: Do you have a borrower with borrowernumber 0 then? You should not have. This just appears to say that there is no borrower yet. Did the self registration email reach the correct address?

Note that I reached this report after having a funny experience with selfregistration too. Apparently, there still was a pending self registration in the database, I entered a new one but received the credentials of the other user instead (on the email address of the last added registration). Cannot reproduce it (at least rightaway..)
Comment 2 dmin 2014-08-14 16:35:27 UTC
When two (or mote) patrons are unverified, this issue causes all of the patrons to receive a verification email with the same token.  

If this link is used by the patron who is not associated with the token in the borrower_modifications table, the user name and password for the borrower who is associated with that token are displayed, providing access to the account and personal details of another patron.  

This represents a critical privacy issue with self-registrations.

This issue is known to affect 3.16.X (did not use self-registration in 3.14.X.

Additonally, our borrower_modifications table always shows borrower # as 0, since borrower number is not generated until the patron is added to the borrowers table in opac-registration-verify.pl using AddMember_OPAC.

It appears the issue is stemming from the section of opac-memberentry.pl where the verification email is generated (as all tokens in the borrower_modifications table are unique) and only the token in the email is incorrect.
Comment 3 dmin 2014-08-14 19:43:43 UTC
(In reply to dmin from comment #2)
> The problem comes from the borrowernumber for new registrants (who haven't been added to the borrowers table yet) being 0 and the query on line 591 of Letters.pm:

($table eq 'borrower_modifications') ? "SELECT * FROM $table WHERE borrowernumber = ? OR verification_token = ?

Since all new, unverified borrowers have the same borrower number, the Boolean expression to match on always matches on the borrower # and doesn't look at the token.  Since, in the detault notice, the only field used in the letter OPAC_REG_VERIFY is the token, it's not immediately obvious that it grabbed the wrong record from the table.  

A workaround, which does not appear to impact any other is to alter the query to ensure the token is checked if the borrower number is 0.  There is probably a better way than this, but it as worked for me:

($table eq 'borrower_modifications') ? "SELECT * FROM $table WHERE (borrowernumber = 0 OR borrowernumber = ?) AND verification_token =?"

I'm not familiar with how to propose a patch properly, but this is a small change in a single line of a module, so I thought I should share.  If this should have gone in the comments, sorry, I'm new to all this.
Comment 4 Kyle M Hall 2014-08-15 16:48:19 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2014-08-15 16:50:05 UTC
Thanks dmin! Your comments were incredibly helpful for writing this patch!

Kyle

(In reply to dmin from comment #3)
> (In reply to dmin from comment #2)
> > The problem comes from the borrowernumber for new registrants (who haven't been added to the borrowers table yet) being 0 and the query on line 591 of Letters.pm:
> 
> ($table eq 'borrower_modifications') ? "SELECT * FROM $table WHERE
> borrowernumber = ? OR verification_token = ?
> 
> Since all new, unverified borrowers have the same borrower number, the
> Boolean expression to match on always matches on the borrower # and doesn't
> look at the token.  Since, in the detault notice, the only field used in the
> letter OPAC_REG_VERIFY is the token, it's not immediately obvious that it
> grabbed the wrong record from the table.  
> 
> A workaround, which does not appear to impact any other is to alter the
> query to ensure the token is checked if the borrower number is 0.  There is
> probably a better way than this, but it as worked for me:
> 
> ($table eq 'borrower_modifications') ? "SELECT * FROM $table WHERE
> (borrowernumber = 0 OR borrowernumber = ?) AND verification_token =?"
> 
> I'm not familiar with how to propose a patch properly, but this is a small
> change in a single line of a module, so I thought I should share.  If this
> should have gone in the comments, sorry, I'm new to all this.
Comment 6 Chris Cormack 2014-08-16 20:52:55 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2014-08-17 01:19:45 UTC
It is possible I am wrong, but I couldn't recreate the problem in testing.

Looking at the code that dmin mentions it appears that opac-memberentry.pl passes two variables to the sql query in Letters.pm

It appears that the function in Letters.pm was designed to allow for general use in pulling from 'borrower_modifications' using either borrowernumber or verification_token.

When trying to pull data by verification_token, opacmemberentry.pl passes the verification_token for both variables (borrowernumber compare and verification_token compare)

In my testing, when mysql gets a string as a variable to compare to an integer (borrowernumber) it will just take the first integer and chop the rest of the string, meaning any verification_token that beings with zero will compare successfully to borrowernumber zero

It should be possible to leave the original sql in Letters.pm and just replace the first variable passed from opacmemberentry.pl  from verification_token to 'a' to prevent matching to zero and to force a comparison of the tokens.
Comment 8 Kyle M Hall 2014-08-20 12:18:22 UTC
While I agree your solution would work, I think this one is better because it solves it at the root of the problem. Fixing the issue at the opacmemberentry.pl level would make it easier to introduce regressions in the future. Thanks for the input though!

(In reply to Nick Clemens from comment #7)
> It is possible I am wrong, but I couldn't recreate the problem in testing.
> 
> Looking at the code that dmin mentions it appears that opac-memberentry.pl
> passes two variables to the sql query in Letters.pm
> 
> It appears that the function in Letters.pm was designed to allow for general
> use in pulling from 'borrower_modifications' using either borrowernumber or
> verification_token.
> 
> When trying to pull data by verification_token, opacmemberentry.pl passes
> the verification_token for both variables (borrowernumber compare and
> verification_token compare)
> 
> In my testing, when mysql gets a string as a variable to compare to an
> integer (borrowernumber) it will just take the first integer and chop the
> rest of the string, meaning any verification_token that beings with zero
> will compare successfully to borrowernumber zero
> 
> It should be possible to leave the original sql in Letters.pm and just
> replace the first variable passed from opacmemberentry.pl  from
> verification_token to 'a' to prevent matching to zero and to force a
> comparison of the tokens.
Comment 9 Katrin Fischer 2014-08-21 07:16:28 UTC
Created attachment 31037 [details] [review]
[PASSED QA] Bug 12371 - Links in every patron self-registration email points to a single borrower

If multiple registrations are submitted, the first patron to register
will be used for the first patron to click the registration confirmation
link!

Test Plan:
1) Submit 2 new patron registrations
2) Use the confirm link from the 2nd registration
3) Note you end up registering as the first submitted registration
4) Apply the patch
5) Repeat steps 1 and 2
6) Note you are now confirmed correctly

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Test plan appears to work fine, I have a feeling the sql could be
written better but can't come up with it on a Sunday morning

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described and fixes a critical bug.
Passes tests and QA script.
Comment 10 Tomás Cohen Arazi 2014-08-21 15:38:51 UTC
Patch pushed to master.

Thanks Kyle!
Comment 11 Galen Charlton 2014-09-23 04:43:49 UTC
Pushed to 3.16.x for inclusion in 3.16.4.
Comment 12 Marcel de Rooy 2014-10-09 11:27:51 UTC
I agree with Chris and Nick that this query is not very clear and easy to maintain. Actually we do only need the unique token here (at this time we are creating a new self-registered borrower without number).
Sending a follow-up on report 13050.
Comment 13 Liz Rea 2014-12-10 22:32:51 UTC
This bug exists in 3.14 as well.
Comment 14 Liz Rea 2014-12-10 22:51:52 UTC
conveniently, it looks like this patch will go into 3.14 pretty much as is. The follow up (bug 13050) won't go cleanly, however.

Liz
Comment 15 Fridolin Somers 2015-01-23 12:45:34 UTC
Pushed to 3.14.x will be in 3.14.13