Attachment 51768 Details for Bug 16492 – Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library

[patch] Bug 16492 - Checkouts ( and possibly checkins and other actions ) will use the patron home branch as the logged in library

Bug-16492---Checkouts--and-possibly-checkins-and-o.patch (text/plain), 5.33 KB, created by Chris Cormack on 2016-05-25 12:10:43 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Cormack

Created: 2016-05-25 12:10:43 UTC

Size: 5.33 KB

patch

obsolete

>From d4b274602e3778d56789b3ca7997d4d0ff914f71 Mon Sep 17 00:00:00 2001
>From: Kyle M Hall <kyle@bywatersolutions.com>
>Date: Tue, 10 May 2016 16:36:03 +0000
>Subject: [PATCH] Bug 16492 - Checkouts ( and possibly checkins and other
> actions ) will use the patron home branch as the logged in library
>
>Bug 14507 introduced the use of checkpw in C4::SIP::ILS::Patron so that
>non-Koha internal authentication processes would be able to function via
>SIP ( LDAP et al ).
>
>The problem is that checkpw changes the userenv to that of the patron!
>This is not usually an issue in Koha because most of the time that
>patron running through checkpw is the one to be logged in.
>
>Aside from SIP2 the only other area where this may be an issue is in SCO
>when using SelfCheckoutByLogin.
>
>Test Plan:
>1) On master, check out an item to a patron via SIP2
>2) Note the checkout lists the item as having been checked out
>   from the patron's home library not matter which library is was
>   supposed to be checked out from.
>3) Apply this patch
>4) Re-checkout the item
>5) The item should now be checked out as if it was checked out from
>   the library as defined in the SIP configuration file.
>
>Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
>---
> C4/Auth.pm            | 10 +++++-----
> C4/SIP/ILS/Patron.pm  |  2 +-
> t/db_dependent/Auth.t | 33 ++++++++++++++++++++++++---------
> 3 files changed, 30 insertions(+), 15 deletions(-)
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index c06fc1a..a5cf72c 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -1738,7 +1738,7 @@ sub get_session {
> }
> 
> sub checkpw {
>-    my ( $dbh, $userid, $password, $query, $type ) = @_;
>+    my ( $dbh, $userid, $password, $query, $type, $no_set_userenv ) = @_;
>     $type = 'opac' unless $type;
>     if ($ldap) {
>         $debug and print STDERR "## checkpw - checking LDAP\n";
>@@ -1778,11 +1778,11 @@ sub checkpw {
>     }
> 
>     # INTERNAL AUTH
>-    return checkpw_internal(@_)
>+    return checkpw_internal( $dbh, $userid, $password, $no_set_userenv);
> }
> 
> sub checkpw_internal {
>-    my ( $dbh, $userid, $password ) = @_;
>+    my ( $dbh, $userid, $password, $no_set_userenv ) = @_;
> 
>     $password = Encode::encode( 'UTF-8', $password )
>       if Encode::is_utf8($password);
>@@ -1812,7 +1812,7 @@ sub checkpw_internal {
>         if ( checkpw_hash( $password, $stored_hash ) ) {
> 
>             C4::Context->set_userenv( "$borrowernumber", $userid, $cardnumber,
>-                $firstname, $surname, $branchcode, $branchname, $flags );
>+                $firstname, $surname, $branchcode, $branchname, $flags ) unless $no_set_userenv;
>             return 1, $cardnumber, $userid;
>         }
>     }
>@@ -1829,7 +1829,7 @@ sub checkpw_internal {
>         if ( checkpw_hash( $password, $stored_hash ) ) {
> 
>             C4::Context->set_userenv( $borrowernumber, $userid, $cardnumber,
>-                $firstname, $surname, $branchcode, $branchname, $flags );
>+                $firstname, $surname, $branchcode, $branchname, $flags ) unless $no_set_userenv;
>             return 1, $cardnumber, $userid;
>         }
>     }
>diff --git a/C4/SIP/ILS/Patron.pm b/C4/SIP/ILS/Patron.pm
>index d61b097..63a0a3a 100644
>--- a/C4/SIP/ILS/Patron.pm
>+++ b/C4/SIP/ILS/Patron.pm
>@@ -201,7 +201,7 @@ sub check_password {
> 
>     my $dbh = C4::Context->dbh;
>     my $ret = 0;
>-    ($ret) = checkpw( $dbh, $self->{userid}, $pwd );
>+    ($ret) = checkpw( $dbh, $self->{userid}, $pwd, undef, undef, 1 ); # dbh, userid, query, type, no_set_userenv
>     return $ret;
> }
> 
>diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t
>index 3077969..b5e1ef1 100644
>--- a/t/db_dependent/Auth.t
>+++ b/t/db_dependent/Auth.t
>@@ -8,22 +8,24 @@ use Modern::Perl;
> use CGI qw ( -utf8 );
> use Test::MockModule;
> use List::MoreUtils qw/all any none/;
>-use Test::More tests => 13;
>+use Test::More tests => 18;
> use Test::Warn;
> use t::lib::Mocks;
>+use t::lib::TestBuilder;
>+
>+use C4::Auth qw(checkpw);
> use C4::Members;
> use Koha::AuthUtils qw/hash_password/;
>+use Koha::Database;
> 
> BEGIN {
>-        use_ok('C4::Auth');
>+    use_ok('C4::Auth');
> }
> 
>-my $dbh = C4::Context->dbh;
>-
>-# Start transaction
>-$dbh->{AutoCommit} = 0;
>-$dbh->{RaiseError} = 1;
>-
>+my $schema = Koha::Database->schema;
>+$schema->storage->txn_begin;
>+my $builder = t::lib::TestBuilder->new;
>+my $dbh     = C4::Context->dbh;
> 
> # get_template_and_user tests
> 
>@@ -176,4 +178,17 @@ my $hash2 = hash_password('password');
> ok(C4::Auth::checkpw_hash('password', $hash1), 'password validates with first hash');
> ok(C4::Auth::checkpw_hash('password', $hash2), 'password validates with second hash');
> 
>-$dbh->rollback;
>+my $patron = $builder->build( { source => 'Borrower' } );
>+changepassword( $patron->{userid}, $patron->{borrowernumber}, $hash1 );
>+my $library = $builder->build(
>+    {
>+        source => 'Branch',
>+    }
>+);
>+
>+C4::Context->set_userenv(0,0,0,'firstname','surname', $library->{branchcode}, 'Library 1', 0, '', '');
>+is( C4::Context->userenv->{branch}, $library->{branchcode}, 'Userenv gives correct branch' );
>+ok( checkpw( $dbh, $patron->{userid}, 'password', undef, undef, 1 ), 'checkpw returns true' );
>+is( C4::Context->userenv->{branch}, $library->{branchcode}, 'Userenv branch is preserved if no_set_userenv is true' );
>+ok( checkpw( $dbh, $patron->{userid}, 'password', undef, undef, 0 ), 'checkpw still returns true' );
>+isnt( C4::Context->userenv->{branch}, $library->{branchcode}, 'Userenv branch is overwritten if no_set_userenv is false' );
>-- 
>2.1.4

Actions: View | Diff | Splinter Review

Attachments on bug 16492: 51390 | 51391 | 51744 | 51745 | 51768 | 51797 | 52188 | 52189 | 52190