Attachment 148204 Details for Bug 32426 – Bug 32426: Throw InvalidUserid exception in Patron->store

[patch] Bug 32426: Throw InvalidUserid exception in Patron->store

Bug-32426-Throw-InvalidUserid-exception-in-Patron-.patch (text/plain), 4.94 KB, created by Martin Renvoize (ashimema) on 2023-03-15 13:43:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Martin Renvoize (ashimema)

Created: 2023-03-15 13:43:13 UTC

Size: 4.94 KB

patch

obsolete

>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

Actions: View | Diff | Splinter Review

Attachments on bug 32426: 146947 | 146948 | 146949 | 146950 | 146951 | 146952 | 146953 | 146954 | 147252 | 147253 | 147254 | 147255 | 147704 | 147705 | 147706 | 147707 | 147708 | 147709 | 147710 | 147711 | 147712 | 148203 | 148204 | 148205 | 148206 | 148207 | 148208 | 148209 | 148210 | 148211 | 148782 | 148793