From dda18c086ce6a40cc176a6ab704d19e43631f452 Mon Sep 17 00:00:00 2001 From: Mark Tompsett Date: Sat, 22 Jun 2013 13:00:48 -0400 Subject: [PATCH] Bug 10454 - Duplicate card numbers may be generated Previously, there was a gap in time between the insertion of the card number into borrowers and the generation. This gap meant that the same card number could be calculated by two different processes resulting in two different members being added with the same card number. By serializing the cardnumber requests, at worst we may skip a cardnumber, rather than duplicate one. No locking required, because the INSERT into cardnumber_sequence is atomic! The reason no duplicates happen is because the sequence of cardnumbers is calculated from the previous one, and recalculating would generate exactly the same value. So even if two processes are calculating the same cardnumber for a given UID record, it will result in the same value. Skips can be generated. Clicking refresh while entering a patron, for example, will increment the auto generated number multiple times. --- C4/Members.pm | 180 +++++++++++++++++++++++--------- installer/data/mysql/updatedatabase.pl | 101 ++++++++++++++++++ kohaversion.pl | 2 +- 3 files changed, 230 insertions(+), 53 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index 3221f53..bca4eb8 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -881,72 +881,148 @@ sub changepassword { =head2 fixup_cardnumber -Warning: The caller is responsible for locking the members table in write -mode, to avoid database corruption. +Previously, there was a gap in time between the insertion of the +card number into borrowers and the generation. This gap meant that +the same card number could be calculated by two different processes +resulting in two different members being added with the same card +number. -=cut +By serializing the cardnumber requests, at worst we may skip a +cardnumber, rather than duplicate one. No locking required, +because the INSERT into cardnumber_sequence is atomic! -use vars qw( @weightings ); -my @weightings = ( 8, 4, 6, 3, 5, 2, 1 ); +The reason no duplicates happen is because the sequence of +cardnumbers is calculated from the previous one, and recalculating +would generate exactly the same value. So even if two processes +are calculating the same cardnumber for a given UID record, it +will result in the same value. + +=cut sub fixup_cardnumber { my ($cardnumber) = @_; - my $autonumber_members = C4::Context->boolean_preference('autoMemberNum') || 0; + my @weightings = ( 8, 4, 6, 3, 5, 2, 1 ); + my ( + $autonumber_members, + $checkdigit, + $dbh, + $sth, + $my_UID, + $current_UID, + $previous_UID, + $previous_cardnumber, + $current_cardnumber, + $sum, + $i, + $temp1, + $temp2, + $rem, + $new_cardnumber, + ); # Find out whether member numbers should be generated # automatically. Should be either "1" or something else. # Defaults to "0", which is interpreted as "no". + $autonumber_members = C4::Context->boolean_preference('autoMemberNum') || 0; - # if ($cardnumber !~ /\S/ && $autonumber_members) { + # if we don't auto-generate, then there is nothing to fixup. + # So return what was passed. ($autonumber_members) or return $cardnumber; - my $checkdigit = C4::Context->preference('checkdigit'); - my $dbh = C4::Context->dbh; - if ( $checkdigit and $checkdigit eq 'katipo' ) { - - # if checkdigit is selected, calculate katipo-style cardnumber. - # otherwise, just use the max() - # purpose: generate checksum'd member numbers. - # We'll assume we just got the max value of digits 2-8 of member #'s - # from the database and our job is to increment that by one, - # determine the 1st and 9th digits and return the full string. - my $sth = $dbh->prepare( - "select max(substring(borrowers.cardnumber,2,7)) as new_num from borrowers" - ); - $sth->execute; - my $data = $sth->fetchrow_hashref; - $cardnumber = $data->{new_num}; - if ( !$cardnumber ) { # If DB has no values, - $cardnumber = 1000000; # start at 1000000 - } else { - $cardnumber += 1; - } - - my $sum = 0; - for ( my $i = 0 ; $i < 8 ; $i += 1 ) { - # read weightings, left to right, 1 char at a time - my $temp1 = $weightings[$i]; - # sequence left to right, 1 char at a time - my $temp2 = substr( $cardnumber, $i, 1 ); - - # mult each char 1-7 by its corresponding weighting - $sum += $temp1 * $temp2; + # determine which kind of auto-generate we are doing + $checkdigit = C4::Context->preference('checkdigit'); + + # grab a database handle. + $dbh = C4::Context->dbh; + + # first we need to show our intent to generate a cardnumber. + $sth = $dbh->prepare(" +INSERT INTO cardnumber_sequence + (cardnumber,checkdigit) VALUES + ('',?); + "); + $sth->execute($checkdigit); + $my_UID = $dbh->last_insert_id(undef,undef,'cardnumber_sequence','UID'); + + # next we need to find the first ungenerated cardnumber. + # if we don't find any, another process filled them for us! + # if we find some, then we need to start generating. + $sth = $dbh->prepare(" +SELECT MIN(UID) AS UID FROM cardnumber_sequence +WHERE cardnumber='' AND checkdigit=?; + "); + $sth->execute($checkdigit); + $current_UID = $sth->fetchrow(); + + # if there is a current_UID, we need to calculate + while ($current_UID) { + + # so find the previous UID + $sth = $dbh->prepare(" +SELECT MAX(UID) AS UID FROM cardnumber_sequence +WHERE UID < ? AND checkdigit=?; + "); + $sth->execute($current_UID,$checkdigit); + $previous_UID = $sth->fetchrow(); + + # grab the previous cardnumber + $sth = $dbh->prepare(" +SELECT cardnumber FROM cardnumber_sequence +WHERE UID = ?; + "); + $sth->execute($previous_UID); + $previous_cardnumber = $sth->fetchrow(); + + # now calculate the current cardnumber. + if ($checkdigit && $checkdigit eq 'katipo') { + $current_cardnumber = substr($previous_cardnumber,1,7); + $current_cardnumber = $current_cardnumber+1; + $sum = 0; + foreach $i (1..7) { + # read weightings, left to right, 1 char at a time + $temp1 = $weightings[$i-1]; + + # sequence left to right, 1 char at a time + $temp2 = substr( $current_cardnumber, $i-1, 1 ); + + # mult each char 1-7 by its corresponding weighting + $sum += $temp1 * $temp2; + } + $rem = ( $sum % 11 ); + $rem = 'X' if $rem == 10; + $current_cardnumber = "V$current_cardnumber$rem"; + } + else { + $current_cardnumber = $previous_cardnumber+1; } - my $rem = ( $sum % 11 ); - $rem = 'X' if $rem == 10; - - return "V$cardnumber$rem"; - } else { - - my $sth = $dbh->prepare( - 'SELECT MAX( CAST( cardnumber AS SIGNED ) ) FROM borrowers WHERE cardnumber REGEXP "^-?[0-9]+$"' - ); - $sth->execute; - my ($result) = $sth->fetchrow; - return $result + 1; - } - return $cardnumber; # just here as a fallback/reminder + # update the current_UID record with the generated cardnumber. + $sth = $dbh->prepare(" +UPDATE cardnumber_sequence SET cardnumber=? WHERE UID=?; + "); + $sth->execute($current_cardnumber,$current_UID); + + # we need to find the next ungenerated cardnumber. + # if we don't find any, we are done generating everything. + $sth = $dbh->prepare(" +SELECT MIN(UID) AS UID FROM cardnumber_sequence +WHERE cardnumber='' and checkdigit=?; + "); + $sth->execute($checkdigit); + $current_UID = $sth->fetchrow(); + + } + + # need already know our UID ($my_UID) + # let's grab the cardnumber. + $sth = $dbh->prepare(" +SELECT cardnumber FROM cardnumber_sequence +WHERE UID=?; + "); + $sth->execute($my_UID); + $new_cardnumber = $sth->fetchrow(); + + return $new_cardnumber; } =head2 GetGuarantees diff --git a/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index 5963d24..7a128eb 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -7010,6 +7010,107 @@ CREATE TABLE IF NOT EXISTS borrower_files ( SetVersion($DBversion); } +$DBversion = "3.13.00.XXX"; +if ( CheckVersion($DBversion) ) { + + # Create new table used to serialize cardnumber requests. + $dbh->do(" +CREATE TABLE IF NOT EXISTS cardnumber_sequence ( + UID INT(16) NOT NULL AUTO_INCREMENT, + cardnumber VARCHAR(16) NOT NULL, + checkdigit TEXT NOT NULL, + PRIMARY KEY (UID) +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + "); + + my ( + $dbh, + $sth, + @cardnumbers_katipo, + @cardnumbers_none, + @cardnumbers, + $cardnumber, + $data, + ); + + # Add 2 records for the case that borrowers is empty. + $dbh = C4::Context->dbh; + $sth = $dbh->prepare(" +INSERT INTO cardnumber_sequence + (cardnumber,checkdigit) VALUES + ('0','none'); + "); + $sth->execute(); + + $sth = $dbh->prepare(" +INSERT INTO cardnumber_sequence + (cardnumber,checkdigit) VALUES + ('V09999992','katipo'); + "); + $sth->execute(); + + # Convert existing borrowers records into records that + # were supposedly generated. + + # grab checkdigit = katipo records + $sth = $dbh->prepare(" +SELECT cardnumber FROM borrowers +WHERE SUBSTR(cardnumber,1,1) + NOT IN ('0','1','2','3','4','5','6','7','8','9') +ORDER BY cardnumber; + "); + $sth->execute(); + $data = $sth->fetchrow_hashref(); + $cardnumber = undef; + $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; + while ($data->{'cardnumber'}) { + push @cardnumbers_katipo,$data->{'cardnumber'}; + $data = $sth->fetchrow_hashref(); + $cardnumber = undef; + $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; + } + + # grab checkdigit = none records + $sth = $dbh->prepare(" +SELECT cardnumber FROM borrowers +WHERE SUBSTR(cardnumber,1,1) + IN ('0','1','2','3','4','5','6','7','8','9') +ORDER BY CAST(cardnumber AS SIGNED); + "); + $sth->execute(); + $data = $sth->fetchrow_hashref(); + $cardnumber = undef; + $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; + while ($data->{'cardnumber'}) { + push @cardnumbers_none,$data->{'cardnumber'}; + $data = $sth->fetchrow_hashref(); + $cardnumber = undef; + $cardnumber = $data->{'cardnumber'} if $data->{'cardnumber'}; + } + + # add the card numbers in. + foreach $cardnumber (@cardnumbers_none) { + $sth = $dbh->prepare(" +INSERT INTO cardnumber_sequence + (cardnumber,checkdigit) VALUES + (?,'none'); + "); + $sth->execute($cardnumber); + } + + foreach $cardnumber (@cardnumbers_katipo) { + $sth = $dbh->prepare(" +INSERT INTO cardnumber_sequence + (cardnumber,checkdigit) VALUES + (?,'katipo'); + "); + $sth->execute($cardnumber); + } + + print "Upgrade to $DBversion done (Bug 10454: Duplicate card numbers may be generated)\n"; + SetVersion($DBversion); +} + =head1 FUNCTIONS =head2 TableExists($table) diff --git a/kohaversion.pl b/kohaversion.pl index 71938e4..863d91f 100644 --- a/kohaversion.pl +++ b/kohaversion.pl @@ -16,7 +16,7 @@ the kohaversion is divided in 4 parts : use strict; sub kohaversion { - our $VERSION = '3.13.00.008'; + our $VERSION = '3.13.00.XXX'; # version needs to be set this way # so that it can be picked up by Makefile.PL # during install -- 1.7.9.5