[Bug 12371] New: Links in every patron self-registration email points to a single borrower
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Bug ID: 12371 Summary: Links in every patron self-registration email points to a single borrower Change sponsored?: --- Product: Koha Version: 3.14 Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: OPAC Assignee: oleonard@myacpl.org Reporter: barton@bywatersolutions.com QA Contact: testopia@bugs.koha-community.org 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl --- Comment #1 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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..) -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 dmin <dminuck@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P1 - high CC| |dminuck@gmail.com Version|3.14 |3.16 Severity|enhancement |critical --- Comment #2 from dmin <dminuck@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de, | |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 --- Comment #3 from dmin <dminuck@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 --- Comment #4 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 30850 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30850&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|oleonard@myacpl.org |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- 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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@quecheelibrary.org -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #30850|0 |1 is obsolete| | --- Comment #6 from Chris Cormack <chris@bigballofwax.co.nz> --- Created attachment 30877 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=30877&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 --- Comment #7 from Nick Clemens <nick@quecheelibrary.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- 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.
-- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #30877|0 |1 is obsolete| | --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 31037 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=31037&action=edit [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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |tomascohen@gmail.com --- Comment #10 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Patch pushed to master. Thanks Kyle! -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_3_16_4_candidate Version|3.16 |master -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |gmcharlt@gmail.com --- Comment #11 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to 3.16.x for inclusion in 3.16.4. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=13050 --- Comment #12 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Liz Rea <liz@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |liz@catalyst.net.nz --- Comment #13 from Liz Rea <liz@catalyst.net.nz> --- This bug exists in 3.14 as well. -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 --- Comment #14 from Liz Rea <liz@catalyst.net.nz> --- 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 -- You are receiving this mail because: You are watching all bug changes.
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12371 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolyn.somers@biblibre.co | |m --- Comment #15 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Pushed to 3.14.x will be in 3.14.13 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org