Bugzilla – Attachment 183664 Details for
Bug 33905
A patron's username and cardnumber must each not be used by another patron in any permutation
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 33905: Add database triggers to prevent cardnumber/userid interchange
Bug-33905-Add-database-triggers-to-prevent-cardnum.patch (text/plain), 13.70 KB, created by
Tomás Cohen Arazi (tcohen)
on 2025-07-01 14:10:02 UTC
(
hide
)
Description:
Bug 33905: Add database triggers to prevent cardnumber/userid interchange
Filename:
MIME Type:
Creator:
Tomás Cohen Arazi (tcohen)
Created:
2025-07-01 14:10:02 UTC
Size:
13.70 KB
patch
obsolete
>From f79f47bbeaf080892ac4c778425c5362f38df495 Mon Sep 17 00:00:00 2001 >From: Tomas Cohen Arazi <tomascohen@theke.io> >Date: Tue, 1 Jul 2025 08:04:23 -0300 >Subject: [PATCH] Bug 33905: Add database triggers to prevent cardnumber/userid > interchange > >This patch adds database triggers to prevent users from having >cardnumbers that match other users' userids and vice versa. > >This prevents potential security issues and user confusion where >a patron might accidentally or maliciously use another patron's >cardnumber as their userid. > >The solution implementation includes: > >* Database triggers: > - trg_borrowers_cardnumber_userid_insert: Prevents INSERT operations > that would create conflicts > - trg_borrowers_cardnumber_userid_update: Prevents UPDATE operations > that would create conflicts > >* Exception-style error messages: > - Koha::Exceptions::Patron::CardnumberMatchesUserID: When cardnumber > matches another user's userid > - Koha::Exceptions::Patron::UserIDMatchesCardnumber: When userid > matches another user's cardnumber > >* New trigger_exists() utility function in C4::Installer: > - Uses standard information_schema.triggers table for portability > - Returns 1 if trigger exists, 0 if it doesn't > - Includes proper POD documentation > - Available for other atomicupdates to use > >* Database structure updates: > - atomicupdate for existing installations > - kohastructure.sql updates for new installations > >It uses database triggers instead of CHECK constraints >because CHECK constraints with subqueries are not portable >between MySQL/MariaDB and PostgreSQL. > >The exception messages follow Koha's exception naming patterns, >making them easy to catch and handle in application code. > >TODO: > >* Exception handling is not implemented in Koha::Patron->store() >* Exceptions are not defined yet >* There's already Koha::Patron->has_valid_userid which should be > adjusted to not look for duplicates and its tests should be adapted. >* Some existing tests should fail and will need tweaks >* I haven't checked if the TRIGGER definitions fail if existing > inconsistencies are found. We should probably add a check in the > atomicupdate before an attempt to change the DB. > >Test plan: >1. Apply the patches >2. Update the DB structure: > $ ktd --shell > k$ updatedatabase >=> SUCCESS: It works :-D >3. Run: > k$ prove t/db_dependent/Patrons_cardnumber_userid_constraint.t >4. Verify that attempts to create conflicting cardnumber/userid > combinations throw the appropriate exceptions: > - Koha::Exceptions::Patron::CardnumberMatchesUserID > - Koha::Exceptions::Patron::UserIDMatchesCardnumber >5. Verify that valid operations still work normally >6. Test fresh installations to ensure triggers are created: > k$ reset_all >=> SUCCESS: Triggers are created >7. Sign off :-D >--- > C4/Installer.pm | 24 ++++- > .../bug_33905_cardnumber_userid_constraint.pl | 78 ++++++++++++++++ > installer/data/mysql/kohastructure.sql | 58 ++++++++++++ > t/db_dependent/Installer_trigger_exists.t | 91 +++++++++++++++++++ > 4 files changed, 250 insertions(+), 1 deletion(-) > create mode 100755 installer/data/mysql/atomicupdate/bug_33905_cardnumber_userid_constraint.pl > create mode 100755 t/db_dependent/Installer_trigger_exists.t > >diff --git a/C4/Installer.pm b/C4/Installer.pm >index f3937a86510..d23d1486574 100644 >--- a/C4/Installer.pm >+++ b/C4/Installer.pm >@@ -38,7 +38,7 @@ BEGIN { > require Exporter; > @ISA = qw( Exporter ); > push @EXPORT, >- qw( primary_key_exists unique_key_exists foreign_key_exists index_exists column_exists TableExists marc_framework_sql_list TransformToNum CheckVersion NewVersion SetVersion sanitize_zero_date update get_db_entries get_atomic_updates run_atomic_updates has_non_dynamic_row_format ); >+ qw( primary_key_exists unique_key_exists foreign_key_exists index_exists trigger_exists column_exists TableExists marc_framework_sql_list TransformToNum CheckVersion NewVersion SetVersion sanitize_zero_date update get_db_entries get_atomic_updates run_atomic_updates has_non_dynamic_row_format ); > } > > =head1 NAME >@@ -727,6 +727,28 @@ sub index_exists { > return $exists; > } > >+=head2 trigger_exists >+ >+ my $exists = trigger_exists( $trigger_name ); >+ >+Checks if a database trigger named I<$trigger_name> exists in the current database schema. >+ >+=cut >+ >+sub trigger_exists { >+ my ($trigger_name) = @_; >+ my $dbh = C4::Context->dbh; >+ my ($count) = $dbh->selectrow_array( >+ q{ >+ SELECT COUNT(*) >+ FROM information_schema.triggers >+ WHERE trigger_schema = DATABASE() >+ AND trigger_name = ? >+ }, undef, $trigger_name >+ ); >+ return $count > 0 ? 1 : 0; >+} >+ > =head2 column_exists > > Missing POD for column_exists. >diff --git a/installer/data/mysql/atomicupdate/bug_33905_cardnumber_userid_constraint.pl b/installer/data/mysql/atomicupdate/bug_33905_cardnumber_userid_constraint.pl >new file mode 100755 >index 00000000000..9af4e9d4cc9 >--- /dev/null >+++ b/installer/data/mysql/atomicupdate/bug_33905_cardnumber_userid_constraint.pl >@@ -0,0 +1,78 @@ >+use Modern::Perl; >+use C4::Installer qw( trigger_exists ); >+use Koha::Installer::Output qw(say_warning say_success say_info); >+ >+return { >+ bug_number => "33905", >+ description => "Add triggers to prevent cardnumber/userid interchange between users", >+ up => sub { >+ my ($args) = @_; >+ my ( $dbh, $out ) = @$args{qw(dbh out)}; >+ >+ # Create INSERT trigger if it doesn't exist >+ unless ( trigger_exists('trg_borrowers_cardnumber_userid_insert') ) { >+ $dbh->do( >+ q{ >+ CREATE TRIGGER trg_borrowers_cardnumber_userid_insert >+ BEFORE INSERT ON borrowers >+ FOR EACH ROW >+ BEGIN >+ -- Check if new cardnumber exists as userid in other records >+ IF NEW.cardnumber IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE userid = NEW.cardnumber >+ AND borrowernumber != COALESCE(NEW.borrowernumber, 0) >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::CardnumberMatchesUserID'; >+ END IF; >+ >+ -- Check if new userid exists as cardnumber in other records >+ IF NEW.userid IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE cardnumber = NEW.userid >+ AND borrowernumber != COALESCE(NEW.borrowernumber, 0) >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::UserIDMatchesCardnumber'; >+ END IF; >+ END >+ } >+ ); >+ say_success( $out, "Created INSERT trigger to prevent cardnumber/userid conflicts" ); >+ } >+ >+ # Create UPDATE trigger if it doesn't exist >+ unless ( trigger_exists('trg_borrowers_cardnumber_userid_update') ) { >+ $dbh->do( >+ q{ >+ CREATE TRIGGER trg_borrowers_cardnumber_userid_update >+ BEFORE UPDATE ON borrowers >+ FOR EACH ROW >+ BEGIN >+ -- Check if updated cardnumber exists as userid in other records >+ IF NEW.cardnumber IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE userid = NEW.cardnumber >+ AND borrowernumber != NEW.borrowernumber >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::CardnumberMatchesUserID'; >+ END IF; >+ >+ -- Check if updated userid exists as cardnumber in other records >+ IF NEW.userid IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE cardnumber = NEW.userid >+ AND borrowernumber != NEW.borrowernumber >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::UserIDMatchesCardnumber'; >+ END IF; >+ END >+ } >+ ); >+ say_success( $out, "Created UPDATE trigger to prevent cardnumber/userid conflicts" ); >+ } >+ }, >+}; >diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql >index 5d7139383c2..5bdecc03538 100644 >--- a/installer/data/mysql/kohastructure.sql >+++ b/installer/data/mysql/kohastructure.sql >@@ -1627,6 +1627,64 @@ CREATE TABLE `borrowers` ( > ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; > /*!40101 SET character_set_client = @saved_cs_client */; > >+-- >+-- Triggers for table `borrowers` >+-- >+ >+DELIMITER $$ >+ >+CREATE TRIGGER trg_borrowers_cardnumber_userid_insert >+ BEFORE INSERT ON borrowers >+ FOR EACH ROW >+BEGIN >+ -- Check if new cardnumber exists as userid in other records >+ IF NEW.cardnumber IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE userid = NEW.cardnumber >+ AND borrowernumber != COALESCE(NEW.borrowernumber, 0) >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::CardnumberMatchesUserID'; >+ END IF; >+ >+ -- Check if new userid exists as cardnumber in other records >+ IF NEW.userid IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE cardnumber = NEW.userid >+ AND borrowernumber != COALESCE(NEW.borrowernumber, 0) >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::UserIDMatchesCardnumber'; >+ END IF; >+END$$ >+ >+CREATE TRIGGER trg_borrowers_cardnumber_userid_update >+ BEFORE UPDATE ON borrowers >+ FOR EACH ROW >+BEGIN >+ -- Check if updated cardnumber exists as userid in other records >+ IF NEW.cardnumber IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE userid = NEW.cardnumber >+ AND borrowernumber != NEW.borrowernumber >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::CardnumberMatchesUserID'; >+ END IF; >+ >+ -- Check if updated userid exists as cardnumber in other records >+ IF NEW.userid IS NOT NULL AND EXISTS ( >+ SELECT 1 FROM borrowers >+ WHERE cardnumber = NEW.userid >+ AND borrowernumber != NEW.borrowernumber >+ ) THEN >+ SIGNAL SQLSTATE '45000' >+ SET MESSAGE_TEXT = 'Koha::Exceptions::Patron::UserIDMatchesCardnumber'; >+ END IF; >+END$$ >+ >+DELIMITER ; >+ > -- > -- Table structure for table `branch_transfer_limits` > -- >diff --git a/t/db_dependent/Installer_trigger_exists.t b/t/db_dependent/Installer_trigger_exists.t >new file mode 100755 >index 00000000000..471dd349b55 >--- /dev/null >+++ b/t/db_dependent/Installer_trigger_exists.t >@@ -0,0 +1,91 @@ >+#!/usr/bin/perl >+ >+# This file is part of Koha. >+# >+# Koha is free software; you can redistribute it and/or modify it >+# under the terms of the GNU General Public License as published by >+# the Free Software Foundation; either version 3 of the License, or >+# (at your option) any later version. >+# >+# Koha is distributed in the hope that it will be useful, but >+# WITHOUT ANY WARRANTY; without even the implied warranty of >+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >+# GNU General Public License for more details. >+# >+# You should have received a copy of the GNU General Public License >+# along with Koha; if not, see <http://www.gnu.org/licenses>. >+ >+use Modern::Perl; >+ >+use Test::More tests => 4; >+ >+use C4::Context; >+use C4::Installer qw( trigger_exists ); >+ >+my $dbh = C4::Context->dbh; >+ >+# Start transaction for cleanup >+$dbh->begin_work; >+ >+subtest 'trigger_exists function availability' => sub { >+ plan tests => 1; >+ >+ can_ok( 'C4::Installer', 'trigger_exists' ); >+}; >+ >+subtest 'trigger_exists with non-existent trigger' => sub { >+ plan tests => 1; >+ >+ my $exists = trigger_exists('non_existent_trigger_name_12345'); >+ is( $exists, 0, 'Non-existent trigger returns 0' ); >+}; >+ >+subtest 'trigger_exists with existing trigger' => sub { >+ plan tests => 3; >+ >+ # Create a test trigger >+ my $test_trigger_name = 'test_trigger_' . time(); >+ >+ # First verify it doesn't exist >+ my $exists_before = trigger_exists($test_trigger_name); >+ is( $exists_before, 0, 'Test trigger does not exist initially' ); >+ >+ # Create the trigger (using a simple trigger that does nothing) >+ $dbh->do( >+ qq{ >+ CREATE TRIGGER $test_trigger_name >+ BEFORE INSERT ON borrowers >+ FOR EACH ROW >+ BEGIN >+ -- This is a test trigger that does nothing >+ SET NEW.borrowernumber = NEW.borrowernumber; >+ END >+ } >+ ); >+ >+ # Now verify it exists >+ my $exists_after = trigger_exists($test_trigger_name); >+ is( $exists_after, 1, 'Test trigger exists after creation' ); >+ >+ # Clean up - drop the trigger >+ $dbh->do("DROP TRIGGER IF EXISTS $test_trigger_name"); >+ >+ # Verify it's gone >+ my $exists_after_drop = trigger_exists($test_trigger_name); >+ is( $exists_after_drop, 0, 'Test trigger does not exist after dropping' ); >+}; >+ >+subtest 'trigger_exists parameter validation' => sub { >+ plan tests => 2; >+ >+ # Test with undefined parameter >+ my $exists_undef = trigger_exists(undef); >+ is( $exists_undef, 0, 'trigger_exists with undef parameter returns 0' ); >+ >+ # Test with empty string >+ my $exists_empty = trigger_exists(''); >+ is( $exists_empty, 0, 'trigger_exists with empty string returns 0' ); >+}; >+ >+# Rollback transaction to clean up any test data >+$dbh->rollback; >-- >2.50.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 33905
:
183664
|
183687