Description
Chris Cormack
2010-05-21 00:35:28 UTC
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 |