Bug 1861 - Unique patrons logins not (totally) enforced
Summary: Unique patrons logins not (totally) enforced
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Authentication (show other bugs)
Version: 3.18
Hardware: All All
: P4 normal (vote)
Assignee: Kyle M Hall
QA Contact: Martin Renvoize
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 8397 12543 13502 15840
  Show dependency treegraph
 
Reported: 2008-02-20 03:53 UTC by Chris Cormack
Modified: 2016-12-05 21:23 UTC (History)
9 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 1861 - Unique patrons logins not (totally) enforced (2.44 KB, patch)
2014-07-08 13:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 1861 - Unique patrons logins not (totally) enforced (2.53 KB, patch)
2014-07-10 17:22 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 1861 - Unique patrons logins not (totally) enforced (2.64 KB, patch)
2014-08-06 20:10 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 1861 - Unique patrons logins not (totally) enforced (3.13 KB, patch)
2014-08-15 10:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 1861 - Unique patrons logins not (totally) enforced (3.00 KB, patch)
2014-11-03 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 1861 [QA Followup] - Fix Check_Userid and unit tests (3.99 KB, patch)
2014-11-03 13:31 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 1861 - Unique patrons logins not (totally) enforced (2.87 KB, patch)
2014-11-20 17:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 1861 [QA Followup] - Fix Check_Userid and unit tests (3.99 KB, patch)
2014-11-20 17:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 1861 [QA Followup] - Don't modify userids, warn if we can't modify the index automatically (1.88 KB, patch)
2014-11-20 17:01 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 1861 [QA Followup] - Don't modify userids, warn if we can't modify the index automatically (1.88 KB, patch)
2014-11-20 17:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PUSHED] Bug 1861 - Unique patrons logins not (totally) enforced (2.94 KB, patch)
2014-11-27 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PUSHED] Bug 1861 [QA Followup] - Fix Check_Userid and unit tests (4.07 KB, patch)
2014-11-27 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PUSHED] Bug 1861 [QA Followup] - Don't modify userids, warn if we can't modify the index automatically (1.96 KB, patch)
2014-11-27 09:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
BUg 1861 : There is a problem introduced with an earlier patch, on this patchset (1.70 KB, patch)
2014-12-30 02:42 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 1861 [SIGNED OFF]: There is a problem introduced with an earlier patch, on this patchset (1.78 KB, patch)
2014-12-30 08:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 1861 [SIGNED OFF]: There is a problem introduced with an earlier patch, on this patchset (1.84 KB, patch)
2014-12-30 08:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 1861: Add 1 test to AddMember (1.32 KB, patch)
2014-12-30 08:53 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 00:35:28 UTC


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

Comment 1 Owen Leonard 2013-03-25 15:45:08 UTC
This bug appears to still be valid in master as of 3.12 beta1
Comment 2 Kyle M Hall 2014-07-08 13:53:07 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2014-07-10 17:22:33 UTC Comment hidden (obsolete)
Comment 4 Marc Véron 2014-08-05 20:34:07 UTC
Merge conflict in installer/data/mysql/updatedatabase.pl
Comment 5 Bernardo Gonzalez Kriegel 2014-08-06 20:10:41 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2014-08-15 10:45:56 UTC
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.
Comment 7 Katrin Fischer 2014-08-15 10:54:47 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2014-08-15 11:05:12 UTC
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.
Comment 9 Kyle M Hall 2014-11-03 13:31:18 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2014-11-03 13:31:26 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-11-06 06:31:36 UTC
Working on this now.
Comment 12 Katrin Fischer 2014-11-06 06:41:34 UTC
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.

?
Comment 13 Jonathan Druart 2014-11-12 14:20:55 UTC
An eval with a (big) warning message can do the trick.
Comment 14 Katrin Fischer 2014-11-12 14:22:49 UTC
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?)
Comment 15 Kyle M Hall 2014-11-20 17:01:33 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2014-11-20 17:01:42 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2014-11-20 17:01:45 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2014-11-20 17:03:32 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize 2014-11-27 09:07:42 UTC
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>
Comment 20 Martin Renvoize 2014-11-27 09:07:49 UTC
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>
Comment 21 Martin Renvoize 2014-11-27 09:07:57 UTC
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>
Comment 22 Martin Renvoize 2014-11-27 09:08:55 UTC
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
Comment 23 Martin Renvoize 2014-12-17 08:38:28 UTC
Pushed to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Comment 24 Tomás Cohen Arazi 2014-12-28 23:02:56 UTC
Patches pushed to master.

Thanks Kyle!
Comment 25 Chris Cormack 2014-12-29 19:21:06 UTC
Pushed to 3.18.x will be in 3.18.3
Comment 26 Chris Cormack 2014-12-30 02:33:52 UTC
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
Comment 27 Chris Cormack 2014-12-30 02:42:31 UTC Comment hidden (obsolete)
Comment 28 Martin Renvoize 2014-12-30 07:59:33 UTC
Pants, how did I miss this. Good catch Chris!
Comment 29 Martin Renvoize 2014-12-30 08:04:56 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2014-12-30 08:53:01 UTC
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>
Comment 31 Jonathan Druart 2014-12-30 08:53:17 UTC
Created attachment 34823 [details] [review]
Bug 1861: Add 1 test to AddMember

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 32 Martin Renvoize 2014-12-30 09:25:09 UTC
Pushed followups to Module Maintainers Branch - https://github.com/mrenvoize/Koha/tree/authentication
Comment 33 Tomás Cohen Arazi 2014-12-31 19:54:11 UTC
Followups pushed to master.

Thanks Chris and Jonathan!
Comment 34 Mason James 2015-01-16 19:25:53 UTC
Contains DB changes, passing for 3.16.x