From 3890817ef848b52240129233f7e3002ff6a86142 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
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 <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
---
 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 9309ed10c7..c5185e32a8 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 25d2fbd6c5..de40abe583 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.2