From 6b2e92e576a502876e9de5ffbd418b1d48603c76 Mon Sep 17 00:00:00 2001 From: Marcel de Rooy Date: Thu, 8 Dec 2022 16:02:35 +0100 Subject: [PATCH] Bug 32426: Throw InvalidUserid exception in Patron->store When creating we still call generate_userid and verify the result. When modifying we do not accept an invalid userid. When needed, we recreate the userid; this should be very exceptional. Test plan: Run t/db_dependent/Koha/Patrons.t Go to staff interface. Try changing userid of a patron to an existing one. Signed-off-by: Marcel de Rooy Signed-off-by: Kyle M Hall Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi --- Koha/Exceptions/Patron.pm | 5 +++++ Koha/Patron.pm | 16 ++++++++-------- t/db_dependent/Koha/Patrons.t | 21 ++++++++------------- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/Koha/Exceptions/Patron.pm b/Koha/Exceptions/Patron.pm index 596f28c94c..1eb7fc5789 100644 --- a/Koha/Exceptions/Patron.pm +++ b/Koha/Exceptions/Patron.pm @@ -19,6 +19,11 @@ use Exception::Class ( isa => 'Koha::Exceptions::Patron', description => "Deleting patron failed, AnonymousPatron is not deleteable" }, + 'Koha::Exceptions::Patron::InvalidUserid' => { + isa => 'Koha::Exceptions::Patron', + description => 'Field userid is not valid (probably not unique)', + fields => [ 'userid' ], + }, 'Koha::Exceptions::Patron::MissingMandatoryExtendedAttribute' => { isa => 'Koha::Exceptions::Patron', description => "Mandatory extended attribute missing", diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 48aec43d06..5cfb07c14b 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -225,8 +225,9 @@ sub store { unless ( $self->in_storage ) { #AddMember # Generate a valid userid/login if needed - $self->generate_userid - if not $self->userid or not $self->has_valid_userid; + $self->generate_userid unless $self->userid; + Koha::Exceptions::Patron::InvalidUserid->throw( userid => $self->userid ) + unless $self->has_valid_userid; # Add expiration date if it isn't already there unless ( $self->dateexpiry ) { @@ -291,12 +292,11 @@ sub store { else { #ModMember my $self_from_storage = $self->get_from_storage; - # FIXME We should not deal with that here, callers have to do this job - # Moved from ModMember to prevent regressions - unless ( $self->userid ) { - my $stored_userid = $self_from_storage->userid; - $self->userid($stored_userid); - } + + # Do not accept invalid userid here + $self->generate_userid unless $self->userid; + Koha::Exceptions::Patron::InvalidUserid->throw( userid => $self->userid ) + unless $self->has_valid_userid; # Password must be updated using $self->set_password $self->password($self_from_storage->password); diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index 585d3e2ff6..e22c31f227 100755 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -1810,24 +1810,20 @@ subtest 'Test Koha::Patrons::merge' => sub { }; subtest '->store' => sub { - plan tests => 8; + plan tests => 9; my $schema = Koha::Database->new->schema; $schema->storage->txn_begin; - my $print_error = $schema->storage->dbh->{PrintError}; - $schema->storage->dbh->{PrintError} = 0; ; # FIXME This does not longer work - because of the transaction in Koha::Patron->store? - my $patron_1 = $builder->build_object({class=> 'Koha::Patrons'}); my $patron_2 = $builder->build_object({class=> 'Koha::Patrons'}); - { - local *STDERR; - open STDERR, '>', '/dev/null'; - throws_ok { $patron_2->userid( $patron_1->userid )->store; } - 'Koha::Exceptions::Object::DuplicateID', - 'Koha::Patron->store raises an exception on duplicate ID'; - close STDERR; - } + throws_ok { $patron_2->userid( $patron_1->userid )->store; } + 'Koha::Exceptions::Patron::InvalidUserid', + 'Koha::Patron->store raises an exception on duplicate ID'; + + # Clear userid and check regeneration + $patron_2->userid(undef)->store; + like( $patron_2->userid, qr/\w+\.\w+/, 'Userid regenerated' ); # old school userid # Test password t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 ); @@ -1853,7 +1849,6 @@ subtest '->store' => sub { $patron_1->relationship("")->store; is( $patron_1->relationship, undef, ); - $schema->storage->dbh->{PrintError} = $print_error; $schema->storage->txn_rollback; subtest 'skip updated_on for BorrowersLog' => sub { -- 2.39.5