Attachment 162151 Details for Bug 35904 – Bug 35904: Make C4::Auth::checkauth testable easily

[patch] Bug 35904: Make C4::Auth::checkauth testable easily

Bug-35904-Make-C4Authcheckauth-testable-easily.patch (text/plain), 16.57 KB, created by Kyle M Hall (khall) on 2024-02-14 16:08:23 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Kyle M Hall (khall)

Created: 2024-02-14 16:08:23 UTC

Size: 16.57 KB

patch

obsolete

>From 231066f1a1e47db7e3a226266b5447e700ab786c Mon Sep 17 00:00:00 2001
>From: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
>Date: Thu, 25 Jan 2024 10:35:41 +0100
>Subject: [PATCH] Bug 35904: Make C4::Auth::checkauth testable easily
>
>This patch suggests to add a new flag do_not_print to
>C4::Auth::checkauth to not print the headers and allow to test this
>subroutine more easily.
>
>We do no longer need to mock safe_exit and redirect STDOUT to test its
>return values.
>
>There are still 3 left:
>1.
>733         # checkauth will redirect and safe_exit if not authenticated and not authorized
>=> Better to keep this one, not trivial to replace
>
>2.
>806         # This will fail on permissions
>This should be replaced but testing $template->{VARS}->{nopermission}
>fails, I dont' think the comment is better.
>
>3.
>828         # Patron does not have the borrowers permission
>Same as 2.
>
>2. and 3. should be investigated a bit more.
>
>This patch also move duplicated code to set patron's password to a
>subroutine set_weak_password.
>
>Test plan:
>Read the code and confirm that everything makes sense.
>QA: Do you have a better way for this? Yes it's dirty!
>
>Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
>---
> C4/Auth.pm            |  14 ++--
> t/db_dependent/Auth.t | 164 ++++++++++++++----------------------------
> 2 files changed, 64 insertions(+), 114 deletions(-)
>
>diff --git a/C4/Auth.pm b/C4/Auth.pm
>index ca4ee0c8af4..ee0e81c0573 100644
>--- a/C4/Auth.pm
>+++ b/C4/Auth.pm
>@@ -791,6 +791,7 @@ sub checkauth {
>     my $type            = shift;
>     my $emailaddress    = shift;
>     my $template_name   = shift;
>+    my $params          = shift || {}; # do_not_print
>     $type = 'opac' unless $type;
> 
>     if ( $type eq 'opac' && !C4::Context->preference("OpacPublic") ) {
>@@ -1332,8 +1333,10 @@ sub checkauth {
>             $uri->query_param_delete('userid');
>             $uri->query_param_delete('password');
>             $uri->query_param_delete('koha_login_context');
>-            print $query->redirect(-uri => $uri->as_string, -cookie => $cookie, -status=>'303 See other');
>-            safe_exit;
>+            unless ( $params->{do_not_print} ) {
>+                print $query->redirect(-uri => $uri->as_string, -cookie => $cookie, -status=>'303 See other');
>+                safe_exit;
>+            }
>         }
> 
>         return ( $userid, $cookie, $sessionID, $flags );
>@@ -1481,8 +1484,11 @@ sub checkauth {
>     );
>     $template->param(%info);
> 
>-    #    $cookie = $query->cookie(CGISESSID => $session->id
>-    #   );
>+    if ( $params->{do_not_print} ) {
>+        # This must be used for testing purpose only!
>+        return ( undef, undef, undef, undef, $template );
>+    }
>+
>     print $query->header(
>         {   type              => 'text/html',
>             charset           => 'utf-8',
>diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t
>index fa622ab09b4..d6ee58d7449 100755
>--- a/t/db_dependent/Auth.t
>+++ b/t/db_dependent/Auth.t
>@@ -14,7 +14,8 @@ use t::lib::TestBuilder;
> 
> use C4::Auth;
> use C4::Members;
>-use Koha::AuthUtils qw/hash_password/;
>+use Koha::AuthUtils qw( hash_password );
>+use Koha::DateUtils qw( dt_from_string );
> use Koha::Database;
> use Koha::Patrons;
> use Koha::Auth::TwoFactorAuth;
>@@ -87,9 +88,7 @@ subtest 'checkauth() tests' => sub {
> 
>         my $patron = $builder->build_object(
>             { class => 'Koha::Patrons', value => { flags => 1 } } );
>-        my $password = 'password';
>-        t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
>-        $patron->set_password( { password => $password } );
>+        my $password = set_weak_password($patron);
>         $cgi = Test::MockObject->new();
>         $cgi->mock( 'cookie', sub { return; } );
>         $cgi->mock(
>@@ -145,39 +144,20 @@ subtest 'checkauth() tests' => sub {
> 
>         my $password_expired;
> 
>-        my $patron_class = Test::MockModule->new('Koha::Patron');
>-        $patron_class->mock( 'password_expired', sub { return $password_expired; } );
>-
>-        my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { flags => 1 } });
>-        my $password = 'password';
>-        t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
>-        $patron->set_password( { password => $password } );
>-
>-        my $cgi_mock = Test::MockModule->new('CGI')->mock( 'request_method', 'POST' );
>-        my $cgi = CGI->new;
>-        $cgi->param( -name => 'userid',   -value => $patron->userid );
>-        $cgi->param( -name => 'password', -value => $password );
>+        my $patron = $builder->build_object( { class => 'Koha::Patrons' } );
> 
>-        my $auth = Test::MockModule->new( 'C4::Auth' );
>-        # Tests will fail if we hit safe_exit
>-        $auth->mock( 'safe_exit', sub { return } );
>+        my $password = set_weak_password($patron);
>+        $patron->password_expiration_date( dt_from_string->subtract(days => 1) )->store;
> 
>-        my ( $userid, $cookie, $sessionID, $flags );
>+        my $cgi_mock = Test::MockModule->new('CGI');
>+        $cgi_mock->mock( 'request_method', sub { return 'POST' } );
>+        my $cgi  = CGI->new;
>+        # Simulating the login form submission
>+        $cgi->param( 'userid',   $patron->userid );
>+        $cgi->param( 'password', $password );
> 
>-        {
>-            t::lib::Mocks::mock_preference( 'DumpTemplateVarsOpac', 1 );
>-            # checkauth will redirect and safe_exit if not authenticated and not authorized
>-            local *STDOUT;
>-            my $stdout;
>-            open STDOUT, '>', \$stdout;
>-
>-            # Password has expired
>-            $password_expired = 1;
>-            C4::Auth::checkauth( $cgi, 0, { catalogue => 1 } );
>-            like( $stdout, qr{'password_has_expired' => 1}, 'password_has_expired is set to 1' );
>-
>-            close STDOUT;
>-        };
>+        my ( $userid, $cookie, $sessionID, $flags, $template ) = C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } );
>+        is( $template->{VARS}->{password_has_expired}, 1 );
>     };
> 
>     subtest 'Reset auth state when changing users' => sub {
>@@ -201,8 +181,6 @@ subtest 'checkauth() tests' => sub {
>             C4::Auth::check_cookie_auth( $sessionID, undef, { skip_version_check => 1, remote_addr => '1.2.3.4' } );
>         is( $return, 'ok', 'Patron authenticated' );
> 
>-        my $mock1 = Test::MockModule->new('C4::Auth');
>-        $mock1->mock( 'safe_exit', sub { return 'safe_exit_redirect' } );
>         my $mock2 = Test::MockModule->new('CGI');
>         $mock2->mock( 'request_method', 'POST' );
>         $mock2->mock( 'cookie',         sub { return $sessionID; } );    # oversimplified..
>@@ -211,21 +189,14 @@ subtest 'checkauth() tests' => sub {
>         $cgi->param( -name => 'userid',             -value => 'Bond' );
>         $cgi->param( -name => 'password',           -value => 'James Bond' );
>         $cgi->param( -name => 'koha_login_context', -value => 1 );
>-        my ( @return, $stdout );
>-        {
>-            local *STDOUT;
>-            local %ENV;
>-            $ENV{REMOTE_ADDR} = '1.2.3.4';
>-            open STDOUT, '>', \$stdout;
>-            @return = C4::Auth::checkauth( $cgi, 0, {} );
>-            close STDOUT;
>-        }
>-        is( $return[0], 'safe_exit_redirect', 'Changing to non-existent user causes a redirect to login' );
>+        my ( $userid, $cookie, $flags, $template );
>+        ( $userid, $cookie, $sessionID, $flags, $template ) =
>+            C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } );
>+        is( $template->{VARS}->{loginprompt}, 1, 'Changing to non-existent user causes a redirect to login' );
>     };
> 
>-
>     subtest 'While still logged in, relogin with another user' => sub {
>-        plan tests => 6;
>+        plan tests => 5;
> 
>         my $patron = $builder->build_object({ class => 'Koha::Patrons', value => {} });
>         my $patron2 = $builder->build_object({ class => 'Koha::Patrons', value => {} });
>@@ -237,36 +208,28 @@ subtest 'checkauth() tests' => sub {
>         $session->param( 'lasttime',     time() );
>         $session->param( 'interface',    'opac' );
>         $session->flush;
>-        my $sessionID = $session->id;
>-        C4::Context->_new_userenv($sessionID);
>+        my $previous_sessionID = $session->id;
>+        C4::Context->_new_userenv($previous_sessionID);
> 
>-        my ( $return ) = C4::Auth::check_cookie_auth( $sessionID, undef, { skip_version_check => 1, remote_addr => '1.2.3.4' } );
>+        my ( $return ) = C4::Auth::check_cookie_auth( $previous_sessionID, undef, { skip_version_check => 1, remote_addr => '1.2.3.4' } );
>         is( $return, 'ok', 'Former session in shape now' );
> 
>         my $mock1 = Test::MockModule->new('C4::Auth');
>         $mock1->mock( 'safe_exit', sub {} );
>         my $mock2 = Test::MockModule->new('CGI');
>         $mock2->mock( 'request_method', 'POST' );
>-        $mock2->mock( 'cookie', sub { return $sessionID; } ); # oversimplified..
>+        $mock2->mock( 'cookie', sub { return $previous_sessionID; } ); # oversimplified..
>         my $cgi = CGI->new;
>         my $password = 'Incr3d1blyZtr@ng93$';
>         $patron2->set_password({ password => $password });
>         $cgi->param( -name => 'userid',             -value => $patron2->userid );
>         $cgi->param( -name => 'password',           -value => $password );
>         $cgi->param( -name => 'koha_login_context', -value => 1 );
>-        my ( @return, $stdout );
>-        {
>-            local *STDOUT;
>-            local %ENV;
>-            $ENV{REMOTE_ADDR} = '1.2.3.4';
>-            open STDOUT, '>', \$stdout;
>-            @return = C4::Auth::checkauth( $cgi, 0, {} );
>-            close STDOUT;
>-        }
>-        # Note: We can test return values from checkauth here since we mocked the safe_exit after the Redirect 303
>-        is( $return[0], $patron2->userid, 'Login of patron2 approved' );
>-        isnt( $return[2], $sessionID, 'Did not return previous session ID' );
>-        ok( $return[2], 'New session ID not empty' );
>+        my ( $userid, $cookie, $sessionID, $flags, $template ) =
>+            C4::Auth::checkauth( $cgi, 0, {}, 'opac', undef, undef, { do_not_print => 1 } );
>+        is( $userid, $patron2->userid, 'Login of patron2 approved' );
>+        isnt( $sessionID, $previous_sessionID, 'Did not return previous session ID' );
>+        ok( $sessionID, 'New session ID not empty' );
> 
>         # Similar situation: Relogin with former session of $patron, new user $patron2 has no permissions
>         $patron2->flags(undef)->store;
>@@ -274,22 +237,14 @@ subtest 'checkauth() tests' => sub {
>         $session->param( 'id',           $patron->userid );
>         $session->param( 'interface',    'intranet' );
>         $session->flush;
>-        $sessionID = $session->id;
>-        C4::Context->_new_userenv($sessionID);
>+        $previous_sessionID = $session->id;
>+        C4::Context->_new_userenv($previous_sessionID);
>         $cgi->param( -name => 'userid',             -value => $patron2->userid );
>         $cgi->param( -name => 'password',           -value => $password );
>         $cgi->param( -name => 'koha_login_context', -value => 1 );
>-        {
>-            local *STDOUT;
>-            local %ENV;
>-            $ENV{REMOTE_ADDR} = '1.2.3.4';
>-            $stdout = q{};
>-            open STDOUT, '>', \$stdout;
>-            @return = C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet' ); # patron2 has no catalogue perm
>-            close STDOUT;
>-        }
>-        like( $stdout, qr/You do not have permission to access this page/, 'No permission response' );
>-        is( @return, 0, 'checkauth returned failure' );
>+        ( $userid, $cookie, $sessionID, $flags, $template ) =
>+            C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } );
>+        is( $template->{VARS}->{nopermission}, 1, 'No permission response' );
>     };
> 
>     subtest 'Two-factor authentication' => sub {
>@@ -408,9 +363,7 @@ subtest 'checkauth() tests' => sub {
> 
>         my $branch = $builder->build_object({ class => 'Koha::Libraries' });
> 
>-        my $password = 'password';
>-        t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
>-        $staff_user->set_password( { password => $password } );
>+        my $password = set_weak_password($staff_user);
>         my $cgi = Test::MockObject->new();
>         $cgi->mock( 'cookie', sub { return; } );
>         $cgi->mock(
>@@ -449,10 +402,8 @@ subtest 'no_set_userenv parameter tests' => sub {
> 
>     my $library = $builder->build_object( { class => 'Koha::Libraries' } );
>     my $patron  = $builder->build_object( { class => 'Koha::Patrons' } );
>-    my $password = 'password';
> 
>-    t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
>-    $patron->set_password({ password => $password });
>+    my $password = set_weak_password($patron);
> 
>     ok( checkpw( $patron->userid, $password, undef, undef, 1 ), 'checkpw returns true' );
>     is( C4::Context->userenv, undef, 'Userenv should be undef as required' );
>@@ -471,10 +422,8 @@ subtest 'checkpw lockout tests' => sub {
> 
>     my $library = $builder->build_object( { class => 'Koha::Libraries' } );
>     my $patron  = $builder->build_object( { class => 'Koha::Patrons' } );
>-    my $password = 'password';
>-    t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
>+    my $password = set_weak_password($patron);
>     t::lib::Mocks::mock_preference( 'FailedLoginAttempts', 1 );
>-    $patron->set_password({ password => $password });
> 
>     my ( $checkpw, undef, undef ) = checkpw( $patron->cardnumber, $password, undef, undef, 1 );
>     ok( $checkpw, 'checkpw returns true with right password when logging in via cardnumber' );
>@@ -792,13 +741,11 @@ subtest 'check_cookie_auth' => sub {
> };
> 
> subtest 'checkauth & check_cookie_auth' => sub {
>-    plan tests => 35;
>+    plan tests => 34;
> 
>     # flags = 4 => { catalogue => 1 }
>     my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { flags => 4 } });
>-    my $password = 'password';
>-    t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
>-    $patron->set_password( { password => $password } );
>+    my $password = set_weak_password($patron);
> 
>     my $cgi_mock = Test::MockModule->new('CGI');
>     $cgi_mock->mock( 'request_method', sub { return 'POST' } );
>@@ -825,16 +772,10 @@ subtest 'checkauth & check_cookie_auth' => sub {
>     my $first_sessionID = $sessionID;
> 
>     $ENV{"HTTP_COOKIE"} = "CGISESSID=$sessionID";
>-    # Not authenticated yet, checkauth didn't return the session
>-    {
>-        local *STDOUT;
>-        my $stdout;
>-        open STDOUT, '>', \$stdout;
>-        ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1} );
>-        close STDOUT;
>-    }
>-    is( $sessionID, undef);
>-    is( $userid, undef);
>+    # Not authenticated yet, the login form is displayed
>+    my $template;
>+    ( $userid, $cookie, $sessionID, $flags, $template ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1}, 'intranet', undef, undef, { do_not_print => 1 } );
>+    is( $template->{VARS}->{loginprompt}, 1, );
> 
>     # Sending undefined fails obviously
>     my ( $auth_status, $session ) = C4::Auth::check_cookie_auth($sessionID, {catalogue => 1} );
>@@ -874,13 +815,9 @@ subtest 'checkauth & check_cookie_auth' => sub {
>     # Logging out!
>     $cgi->param('logout.x', 1);
>     $cgi->delete( 'userid', 'password' );
>-    {
>-        local *STDOUT;
>-        my $stdout;
>-        open STDOUT, '>', \$stdout;
>-        ( $userid, $cookie, $sessionID, $flags ) = C4::Auth::checkauth($cgi, 0, {catalogue => 1});
>-        close STDOUT;
>-    }
>+    ( $userid, $cookie, $sessionID, $flags, $template ) =
>+        C4::Auth::checkauth( $cgi, 0, { catalogue => 1 }, 'intranet', undef, undef, { do_not_print => 1 } );
>+
>     is( $sessionID, undef );
>     is( $ENV{"HTTP_COOKIE"}, "CGISESSID=$first_sessionID", 'HTTP_COOKIE not unset' );
>     ( $auth_status, $session) = C4::Auth::check_cookie_auth( $first_sessionID, {catalogue => 1} );
>@@ -1138,8 +1075,7 @@ subtest 'checkpw() return values tests' => sub {
>         $C4::Auth::ldap = 0;
> 
>         my $patron   = $builder->build_object( { class => 'Koha::Patrons' } );
>-        my $password = 'thePassword123';
>-        $patron->set_password( { password => $password, skip_validation => 1 } );
>+        my $password = set_weak_password($patron);
> 
>         my $patron_to_delete  = $builder->build_object( { class => 'Koha::Patrons' } );
>         my $unused_userid     = $patron_to_delete->userid;
>@@ -1331,3 +1267,11 @@ subtest 'checkpw() return values tests' => sub {
>         $schema->storage->txn_rollback;
>     };
> };
>+
>+sub set_weak_password {
>+    my ($patron) = @_;
>+    my $password = 'password';
>+    t::lib::Mocks::mock_preference( 'RequireStrongPassword', 0 );
>+    $patron->set_password( { password => $password } );
>+    return $password;
>+}
>-- 
>2.30.2

Actions: View | Diff | Splinter Review

Attachments on bug 35904: 161377 | 161378 | 162134 | 162135 | 162151 | 162168 | 162169 | 162187 | 162188 | 162217