Bug 12371

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..)

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.

(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.

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.

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.

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.

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.

Patch pushed to master.

Thanks Kyle!

Pushed to 3.16.x for inclusion in 3.16.4.

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.

This bug exists in 3.14 as well.

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

Pushed to 3.14.x will be in 3.14.13