From fb90f71f717e7adc39c4d6d0f6bdb100f05e4695 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Mon, 29 Sep 2014 07:51:42 +0000 Subject: [PATCH] BUG8446, QA Followup: Use DBIx::Class - Convert Auth_with_shibboleth to use dbic stanzas. Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- C4/Auth.pm | 2 +- C4/Auth_with_shibboleth.pm | 22 ++++++------- t/Auth_with_shibboleth.t | 65 +++++++++++++++++++------------------- 3 files changed, 43 insertions(+), 46 deletions(-) diff --git a/C4/Auth.pm b/C4/Auth.pm index 684faae4fc..7dfe3d21a5 100644 --- a/C4/Auth.pm +++ b/C4/Auth.pm @@ -1639,7 +1639,7 @@ sub checkpw { # Then, we check if it matches a valid koha user if ($shib_login) { - my ( $retval, $retcard, $retuserid ) = C4::Auth_with_shibboleth::checkpw_shib( $dbh, $shib_login ); # EXTERNAL AUTH + my ( $retval, $retcard, $retuserid ) = C4::Auth_with_shibboleth::checkpw_shib( $shib_login ); # EXTERNAL AUTH ($retval) and return ( $retval, $retcard, $retuserid ); return 0; } diff --git a/C4/Auth_with_shibboleth.pm b/C4/Auth_with_shibboleth.pm index d2a4cdf887..f9555fb7a0 100644 --- a/C4/Auth_with_shibboleth.pm +++ b/C4/Auth_with_shibboleth.pm @@ -21,6 +21,7 @@ use Modern::Perl; use C4::Debug; use C4::Context; +use Koha::Database; use Carp; use CGI; @@ -89,21 +90,16 @@ sub get_login_shib { sub checkpw_shib { $debug and warn "checkpw_shib"; - my ( $dbh, $match ) = @_; - my ( $retnumber, $userid ); + my ( $match ) = @_; my $config = _get_shib_config(); $debug and warn "User Shibboleth-authenticated as: $match"; - # Does the given shibboleth attribute value ($match) match a valid koha user ? - my $sth = $dbh->prepare( - "select cardnumber, userid from borrowers where $config->{matchpoint}=?" - ); - $sth->execute($match); - if ( $sth->rows ) { - my @retvals = $sth->fetchrow; - $retnumber = $retvals[0]; - $userid = $retvals[1]; - return ( 1, $retnumber, $userid ); + # Does the given shibboleth attribute value ($match) match a valid koha user ? + my $borrower = + Koha::Database->new()->schema()->resultset('Borrower') + ->find( { $config->{matchpoint} => $match } ); + if ( defined($borrower) ) { + return ( 1, $borrower->get_column('cardnumber'), $borrower->get_column('userid') ); } # If we reach this point, the user is not a valid koha user @@ -265,7 +261,7 @@ Returns the shibboleth login attribute should it be found present in the http se Given a database handle and a shib_login attribute, this routine checks for a matching local user and if found returns true, their cardnumber and their userid. If a match is not found, then this returns false. - my ( $retval, $retcard, $retuserid ) = C4::Auth_with_shibboleth::checkpw_shib( $dbh, $shib_login ); + my ( $retval, $retcard, $retuserid ) = C4::Auth_with_shibboleth::checkpw_shib( $shib_login ); =head1 SEE ALSO diff --git a/t/Auth_with_shibboleth.t b/t/Auth_with_shibboleth.t index bd627e284e..0a39b9b22c 100644 --- a/t/Auth_with_shibboleth.t +++ b/t/Auth_with_shibboleth.t @@ -7,7 +7,7 @@ use Modern::Perl; use Test::More tests => 6; use Test::MockModule; use Test::Warn; -use DBD::Mock; +use Test::DBIx::Class {schema_class => 'Koha::Schema', connect_info => ['dbi:SQLite:dbname=:memory:','','']}; use CGI; use C4::Context; @@ -17,25 +17,10 @@ my $matchpoint = 'userid'; my %mapping = ( 'userid' => { 'is' => 'uid' }, ); $ENV{'uid'} = "test1234"; -#my %shibboleth = ( -# 'matchpoint' => $matchpoint, -# 'mapping' => \%mapping -#); - # Setup Mocks ## Mock Context my $context = new Test::MockModule('C4::Context'); -### Mock ->dbh -$context->mock( - '_new_dbh', - sub { - my $dbh = DBI->connect( 'DBI:Mock:', '', '' ) - || die "Cannot create handle: $DBI::errstr\n"; - return $dbh; - } -); - ### Mock ->config $context->mock( 'config', \&mockedConfig ); @@ -64,8 +49,17 @@ sub mockedPref { return $return; } -# Convenience methods -## Reset Context +## Mock Database +my $database = new Test::MockModule('Koha::Database'); + +### Mock ->schema +$database->mock( 'schema', \&mockedSchema ); + +sub mockedSchema { + return Schema(); +} + +## Convenience method to reset config sub reset_config { $matchpoint = 'userid'; %mapping = ( 'userid' => { 'is' => 'uid' }, ); @@ -75,7 +69,7 @@ sub reset_config { } # Tests -my $dbh = C4::Context->dbh(); +############################################################## # Can module load use_ok('C4::Auth_with_shibboleth'); @@ -155,21 +149,27 @@ subtest "get_login_shib tests" => sub { ## checkpw_shib subtest "checkpw_shib tests" => sub { - plan tests => 12; - - my $shib_login = 'test1234'; - my @borrower_results = - ( [ 'cardnumber', 'userid' ], [ 'testcardnumber', 'test1234' ], ); - $dbh->{mock_add_resultset} = \@borrower_results; + plan tests => 13; + my $shib_login; my ( $retval, $retcard, $retuserid ); + # Setup Mock Database Data + fixtures_ok [ + 'Borrower' => [ + [qw/cardnumber userid surname address city/], + [qw/testcardnumber test1234 renvoize myaddress johnston/], + ], + ], + 'Installed some custom fixtures via the Populate fixture class'; + # debug off $C4::Auth_with_shibboleth::debug = '0'; # good user + $shib_login = "test1234"; warnings_are { - ( $retval, $retcard, $retuserid ) = checkpw_shib( $dbh, $shib_login ); + ( $retval, $retcard, $retuserid ) = checkpw_shib( $shib_login ); } [], "good user with no debug"; is( $retval, "1", "user authenticated" ); @@ -177,21 +177,20 @@ subtest "checkpw_shib tests" => sub { is( $retuserid, "test1234", "expected userid returned" ); # bad user + $shib_login = 'martin'; warnings_are { - ( $retval, $retcard, $retuserid ) = checkpw_shib( $dbh, $shib_login ); + ( $retval, $retcard, $retuserid ) = checkpw_shib( $shib_login ); } [], "bad user with no debug"; is( $retval, "0", "user not authenticated" ); - # reset db mock - $dbh->{mock_add_resultset} = \@borrower_results; - # debug on $C4::Auth_with_shibboleth::debug = '1'; # good user + $shib_login = "test1234"; warnings_exist { - ( $retval, $retcard, $retuserid ) = checkpw_shib( $dbh, $shib_login ); + ( $retval, $retcard, $retuserid ) = checkpw_shib( $shib_login ); } [ qr/checkpw_shib/, qr/User Shibboleth-authenticated as:/ ], "good user with debug enabled"; @@ -200,8 +199,9 @@ subtest "checkpw_shib tests" => sub { is( $retuserid, "test1234", "expected userid returned" ); # bad user + $shib_login = "martin"; warnings_exist { - ( $retval, $retcard, $retuserid ) = checkpw_shib( $dbh, $shib_login ); + ( $retval, $retcard, $retuserid ) = checkpw_shib( $shib_login ); } [ qr/checkpw_shib/, @@ -218,3 +218,4 @@ is( C4::Auth_with_shibboleth::_get_uri(), "https://testopac.com", "https opac uri returned" ); ## _get_shib_config +# Internal helper function, covered in tests above -- 2.39.5