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.
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.
Created attachment 30850 [details] [review] 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
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.
Created attachment 30877 [details] [review] 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
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