---- Reported by dswhite42@yahoo.com 2008-02-20 15:53:42 ---- The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this? UNIQUE KEY `userid` (`userid`) ---- Additional Comments From mjr@ttllp.co.uk 2008-06-30 06:25:03 ---- When you write that nothing is enforced ... to prevent patrons from being imported ... with duplicate logins, do you mean imported by mysql tools or by koha tools? The koha staff tool import_borrowers.pl seems to offer overwrite or ignore options for this. ---- Additional Comments From dswhite42@yahoo.com 2008-06-30 07:07:54 ---- In this case, I was thinking of third party tools. Our Koha database gets its usernames and passwords from another database (of church members in this case), and I wrote a Perl script to migrate them over from the member DB format into Koha format. It was while I was using that script that I saw the problem. Which is why I wondered if the Right Solution would be to enforce the uniqueness on the MySQL level (i.e. UNIQUE KEY `userid` (`userid`)) ---- Additional Comments From nengard@gmail.com 2009-12-23 17:58:51 ---- This bug hasn't been touched in nearly a year, please revisit and test and close if appropriate. ---- Additional Comments From dswhite42@yahoo.com 2009-12-27 19:49:59 ---- The SQL code in <SRC>/installer/data/mysql/kohastructure.sql is unchanged, so this bug remains unresolved. I still think that changing KEY `userid` (`userid`), to UNIQUE KEY `userid` (`userid`) in the "borrowers" table is the best solution. --- Bug imported by chris@bigballofwax.co.nz 2010-05-21 00:35 UTC --- This bug was previously known as _bug_ 1861 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=1861 Unknown operating system Linux - Gentoo. Setting to default OS "All". Actual time not defined. Setting to 0.0 The original reporter of this bug does not have an account here. Reassigning to the person who moved it here: chris@bigballofwax.co.nz. Previous reporter was dswhite42@yahoo.com. CC member mjr@ttllp.co.uk does not have an account here
This bug appears to still be valid in master as of 3.12 beta1
Created attachment 29573 [details] [review] Bug 1861 - Unique patrons logins not (totally) enforced The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this: UNIQUE KEY `userid` (`userid`) Test plan: 1) Enter mysql console 2) Attempt to create two borrowers with identical user id's 3) Note you are able to do so 4) Delete these bororwers 5) Apply this patch 6) Run updatedatabase.pl 7) Repeat step 2 8) Note you are now unable to do so
Created attachment 29616 [details] [review] Bug 1861 - Unique patrons logins not (totally) enforced The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this: UNIQUE KEY `userid` (`userid`) Test plan: 1) Enter mysql console 2) Attempt to create two borrowers with identical user id's 3) Note you are able to do so 4) Delete these bororwers 5) Apply this patch 6) Run updatedatabase.pl 7) Repeat step 2 8) Note you are now unable to do so
Merge conflict in installer/data/mysql/updatedatabase.pl
Created attachment 30561 [details] [review] [SIGNED-OFF] Bug 1861 - Unique patrons logins not (totally) enforced The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this: UNIQUE KEY `userid` (`userid`) Test plan: 1) Enter mysql console 2) Attempt to create two borrowers with identical user id's 3) Note you are able to do so 4) Delete these bororwers 5) Apply this patch 6) Run updatedatabase.pl 7) Repeat step 2 8) Note you are now unable to do so Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no koha-qa errors
Just a few notes: I think removing the possibility to add duplicate userids is a good first step. I am a bit worried about existing installations, where duplicate userids might already exist. Maybe we could improve the message from the update script a bit in this case to output a clear warning? [Fri Aug 15 12:43:59 2014] updatedatabase.pl: DBD::mysql::db do failed: Duplicate entry 'test' for key 'userid' at /home/katrin/kohaclone/installer/data/mysql/updatedatabase.pl line 8614. I think another possible problem could appear with doubled up userids and cardnumbers, as we treat both the same for logging into the OPAC.
Created attachment 30828 [details] [review] [PASSED QA] Bug 1861 - Unique patrons logins not (totally) enforced The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this: UNIQUE KEY `userid` (`userid`) Test plan: 1) Enter mysql console 2) Attempt to create two borrowers with identical user id's 3) Note you are able to do so 4) Delete these bororwers 5) Apply this patch 6) Run updatedatabase.pl 7) Repeat step 2 8) Note you are now unable to do so Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no koha-qa errors Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Leaves deletedborrowers as it is, deleting patrons still works ok. Works according to test plan. Database update will only be successful, when no duplicate entries exist in the database.
Hm, I think we need to take another look at this, it seems to make the tests fail: Test Summary Report ------------------- t/db_dependent/Holds.t (Wstat: 65280 Tests: 1 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 32 tests but ran 1. Files=1, Tests=1, 2 wallclock secs ( 0.02 usr 0.01 sys + 1.38 cusr 0.08 csys = 1.49 CPU) Result: FAIL t/db_dependent/HoldsQueue.t .. 3/21 DBD::mysql::st execute failed: Duplicate entry 'firstname.surname' for key 'userid' at /home/katrin/kohaclone/C4/SQLHelper.pm line 184. DBD::mysql::st execute failed: Duplicate entry 'firstname.surname' for key 'userid' at /home/katrin/kohaclone/C4/SQLHelper.pm line 184. DBD::mysql::st execute failed: Column 'borrowernumber' cannot be null at t/db_dependent/HoldsQueue.t line 279. DBD::mysql::st execute failed: Column 'borrowernumber' cannot be null at t/db_dependent/HoldsQueue.t line 279. # Looks like you planned 21 tests but ran 17. # Looks like your test exited with 255 just after 17.
Created attachment 33143 [details] [review] Bug 1861 - Unique patrons logins not (totally) enforced The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this: UNIQUE KEY `userid` (`userid`) Test plan: 1) Enter mysql console 2) Attempt to create two borrowers with identical user id's 3) Note you are able to do so 4) Delete these bororwers 5) Apply this patch 6) Run updatedatabase.pl 7) Repeat step 2 8) Note you are now unable to do so Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no koha-qa errors Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Leaves deletedborrowers as it is, deleting patrons still works ok. Works according to test plan. Database update will only be successful, when no duplicate entries exist in the database.
Created attachment 33144 [details] [review] Bug 1861 [QA Followup] - Fix Check_Userid and unit tests Check_Userid assumes that a borrowernumber will always be passed in and thus fails to to return 0 for an already used userid when creating a new patron. Unit tests must now also me modified to no longer assume it is possible to create multiple patrons with the same userid.
Working on this now.
I think this patch does the right thing, but I am worried for the reason we do it - the possiblity that libraries HAVE duplicated ids in their databases. Is there something smart we could do to not just leave them with: [Thu Nov 6 07:36:04 2014] updatedatabase.pl: DBD::mysql::db do failed: Duplicate entry 'test' for key 'userid' at /home/katrin/kohaclone/installer/data/mysql/updatedatabase.pl line 9015. ?
An eval with a (big) warning message can do the trick.
I don't want to leave them in a state where the change doesn't get done... but I think we can't change the userids... maybe we could leave instructions somewhere on how to find the doubled userids and the command to run once they are cleaned up? (Wiki?)
Created attachment 33749 [details] [review] Bug 1861 - Unique patrons logins not (totally) enforced The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this: UNIQUE KEY `userid` (`userid`) Test plan: 1) Enter mysql console 2) Attempt to create two borrowers with identical user id's 3) Note you are able to do so 4) Delete these bororwers 5) Apply this patch 6) Run updatedatabase.pl 7) Repeat step 2 8) Note you are now unable to do so Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no koha-qa errors Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Leaves deletedborrowers as it is, deleting patrons still works ok. Works according to test plan. Database update will only be successful, when no duplicate entries exist in the database.
Created attachment 33750 [details] [review] Bug 1861 [QA Followup] - Fix Check_Userid and unit tests Check_Userid assumes that a borrowernumber will always be passed in and thus fails to to return 0 for an already used userid when creating a new patron. Unit tests must now also me modified to no longer assume it is possible to create multiple patrons with the same userid.
Created attachment 33751 [details] [review] Bug 1861 [QA Followup] - Don't modify userids, warn if we can't modify the index automatically
Created attachment 33752 [details] [review] Bug 1861 [QA Followup] - Don't modify userids, warn if we can't modify the index automatically
Created attachment 33983 [details] [review] [PUSHED] Bug 1861 - Unique patrons logins not (totally) enforced The Koha 3.0 patron admin page will prevent you from creating a user who has an identical OPAC login as someone else (though I would change the error message from "Login/password already exists." to "Login already exists.", since it's not the password that's the problem). However, there's nothing enforced in the MySQL database to prevent patrons from being imported or manually inserted with duplicate logins. Would it be better to change the MySQL spec from this: KEY `userid` (`userid`) to this: UNIQUE KEY `userid` (`userid`) Test plan: 1) Enter mysql console 2) Attempt to create two borrowers with identical user id's 3) Note you are able to do so 4) Delete these bororwers 5) Apply this patch 6) Run updatedatabase.pl 7) Repeat step 2 8) Note you are now unable to do so Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> Work as described, no koha-qa errors Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> Leaves deletedborrowers as it is, deleting patrons still works ok. Works according to test plan. Database update will only be successful, when no duplicate entries exist in the database. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 33984 [details] [review] [PUSHED] Bug 1861 [QA Followup] - Fix Check_Userid and unit tests Check_Userid assumes that a borrowernumber will always be passed in and thus fails to to return 0 for an already used userid when creating a new patron. Unit tests must now also me modified to no longer assume it is possible to create multiple patrons with the same userid. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 33985 [details] [review] [PUSHED] Bug 1861 [QA Followup] - Don't modify userids, warn if we can't modify the index automatically Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected for me, and passes qa scripts. Very happy now that there are sensible and just warning during upgrade regarding pre-existing duplicates. Passing QA
Pushed to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Patches pushed to master. Thanks Kyle!
Pushed to 3.18.x will be in 3.18.3
There is massive issue with this Specifically the Fix Check_Userid sub Check_Userid { - my ($uid,$member) = @_; - my $dbh = C4::Context->dbh; - my $sth = - $dbh->prepare( - "SELECT * FROM borrowers WHERE userid=? AND borrowernumber != ?"); - $sth->execute( $uid, $member ); - if ( (( $uid ne '' ) && ( my $row = $sth->fetchrow_hashref )) or - (( $uid ne '' ) && ( $uid eq C4::Context->config('user') )) ) { - return 0; - } - else { - return 1; - } + my ( $uid, $borrowernumber ) = @_; + + return 1 unless ($uid); + + return 0 if ( $uid eq C4::Context->config('user') ); + + my $rs = Koha::Database->new()->schema()->resultset('Borrower'); + + my $params; + $params->{userid} = $uid; + $params->{borrowernumber} = { '!=' => $borrowernumber } if ($borrowernumber); + + my $count = $rs->count( $params ); + + return $count ? 0 : 1; } So we return 1 if it's unique. Then we did this - $data{'userid'} = Generate_Userid($data{'borrowernumber'}, $data{'firstname'}, $data{'surname'}) if $data{'userid'} eq ''; + $data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} ) + if ( $data{'userid'} eq '' || Check_Userid( $data{'userid'} ) ); Which says essentially, if it is unique .. don't use it, if it isn't unique .. then use it. (and the db insert then fails). So we can insert borrowers who have a unique userid, but it will never use that userid it will generate one. Quite broken. Patch to follow
Created attachment 34820 [details] [review] BUg 1861 : There is a problem introduced with an earlier patch, on this patchset - $data{'userid'} = Generate_Userid($data{'borrowernumber'}, $data{'firstname'}, $data{'surname'}) if $data{'userid'} eq ''; + $data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} ) + if ( $data{'userid'} eq '' || Check_Userid( $data{'userid'} ) ); Check_Userid returns 1 if it is unique. So this means unique userids will always be discarded and new ones created. This is why all the tests depending on a userid are now failing To test 1/ run perl t/db_dependent/Serials_2.t 2/ Notice lots of tests fail 3/ OR Add a borrower with a userid set, notice the userid is ignored and one is generated instead 4/ Apply patch 5/ Add a new borrower, notice the userid sticks (if it is unique) 6/ Run perl t/db_dependent/Serials_2.t notice tests pass 7/ Run perl t/db_dependent/Members.t notice tests still pass
Pants, how did I miss this. Good catch Chris!
Created attachment 34821 [details] [review] Bug 1861 [SIGNED OFF]: There is a problem introduced with an earlier patch, on this patchset - $data{'userid'} = Generate_Userid($data{'borrowernumber'}, $data{'firstname'}, $data{'surname'}) if $data{'userid'} eq ''; + $data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} ) + if ( $data{'userid'} eq '' || Check_Userid( $data{'userid'} ) ); Check_Userid returns 1 if it is unique. So this means unique userids will always be discarded and new ones created. This is why all the tests depending on a userid are now failing To test 1/ run perl t/db_dependent/Serials_2.t 2/ Notice lots of tests fail 3/ OR Add a borrower with a userid set, notice the userid is ignored and one is generated instead 4/ Apply patch 5/ Add a new borrower, notice the userid sticks (if it is unique) 6/ Run perl t/db_dependent/Serials_2.t notice tests pass 7/ Run perl t/db_dependent/Members.t notice tests still pass Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 34822 [details] [review] Bug 1861 [SIGNED OFF]: There is a problem introduced with an earlier patch, on this patchset - $data{'userid'} = Generate_Userid($data{'borrowernumber'}, $data{'firstname'}, $data{'surname'}) if $data{'userid'} eq ''; + $data{'userid'} = Generate_Userid( $data{'borrowernumber'}, $data{'firstname'}, $data{'surname'} ) + if ( $data{'userid'} eq '' || Check_Userid( $data{'userid'} ) ); Check_Userid returns 1 if it is unique. So this means unique userids will always be discarded and new ones created. This is why all the tests depending on a userid are now failing To test 1/ run perl t/db_dependent/Serials_2.t 2/ Notice lots of tests fail 3/ OR Add a borrower with a userid set, notice the userid is ignored and one is generated instead 4/ Apply patch 5/ Add a new borrower, notice the userid sticks (if it is unique) 6/ Run perl t/db_dependent/Serials_2.t notice tests pass 7/ Run perl t/db_dependent/Members.t notice tests still pass Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 34823 [details] [review] Bug 1861: Add 1 test to AddMember Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Pushed followups to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Followups pushed to master. Thanks Chris and Jonathan!
Contains DB changes, passing for 3.16.x