From f79f47bbeaf080892ac4c778425c5362f38df495 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi 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 . + +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