From 4a0e3340c0160de5aca5c02b6a8dbefa545a10dc Mon Sep 17 00:00:00 2001
From: Agustin Moyano <agustinmoyano@theke.io>
Date: Fri, 11 Nov 2022 22:22:23 +0000
Subject: [PATCH] Bug 32178: Remove security breach introduced in bug 31378

This patch removes a security breach in C4::Auth::check_api_auth introduced by bug 31378, where when someone called an api with the parameters userid and auth_client_login, check_api_auth would automatically asume the user calling was that userid.

This patch also introduces C4::Auth::create_basic_session(), a function that creates a session and adds the minimum basic parameters.

Signed-off-by: David Cook <dcook@prosentient.com.au>
---
 C4/Auth.pm                   | 37 +++++++++++++++++++++++++++++++-----
 Koha/REST/Plugin/Auth/IdP.pm | 23 +++++++---------------
 Koha/REST/V1/OAuth/Client.pm |  4 ++--
 t/db_dependent/Auth.t        | 24 ++++++++++++++++++++++-
 4 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/C4/Auth.pm b/C4/Auth.pm
index ac73f2d018..785291f050 100644
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -74,7 +74,7 @@ BEGIN {
     @EXPORT_OK = qw(
       checkauth check_api_auth get_session check_cookie_auth checkpw checkpw_internal checkpw_hash
       get_all_subpermissions get_user_subpermissions track_login_daily in_iprange
-      get_template_and_user haspermission
+      get_template_and_user haspermission create_basic_session
     );
 
     $ldap      = C4::Context->config('useldapserver') || 0;
@@ -1561,7 +1561,6 @@ sub check_api_auth {
         # new login
         my $userid   = $query->param('userid');
         my $password = $query->param('password');
-        my $auth_client_login = $query->param('auth_client_login');
         my ( $return, $cardnumber, $cas_ticket );
 
         # Proxy CAS auth
@@ -1571,9 +1570,6 @@ sub check_api_auth {
             # In case of a CAS authentication, we use the ticket instead of the password
             my $PT = $query->param('PT');
             ( $return, $cardnumber, $userid, $cas_ticket ) = check_api_auth_cas( $PT, $query );    # EXTERNAL AUTH
-        } elsif ( $auth_client_login && ( $userid || $query->param('cardnumber') ) ) {
-            $cardnumber = $query->param('cardnumber');
-            $return = 1;
         } else {
 
             # User / password auth
@@ -1871,6 +1867,37 @@ sub get_session {
     return $session;
 }
 
+=head2 create_basic_session
+
+my $session = create_basic_session({ patron => $patron, interface => $interface });
+
+Creates a session and adds all basic parameters for a session to work
+
+=cut
+
+sub create_basic_session {
+    my $params    = shift;
+    my $patron    = $params->{patron};
+    my $interface = $params->{interface};
+
+    my $session = get_session("");
+
+    $session->param( 'number',       $patron->borrowernumber );
+    $session->param( 'id',           $patron->userid );
+    $session->param( 'cardnumber',   $patron->cardnumber );
+    $session->param( 'firstname',    $patron->firstname );
+    $session->param( 'surname',      $patron->surname );
+    $session->param( 'branch',       $patron->branchcode );
+    $session->param( 'branchname',   $patron->library->branchname );
+    $session->param( 'flags',        $patron->flags );
+    $session->param( 'emailaddress', $patron->email );
+    $session->param( 'ip',           $session->remote_addr() );
+    $session->param( 'lasttime',     time() );
+    $session->param( 'interface',    $interface);
+
+    return $session;
+}
+
 
 # FIXME no_set_userenv may be replaced with force_branchcode_for_userenv
 # (or something similar)
diff --git a/Koha/REST/Plugin/Auth/IdP.pm b/Koha/REST/Plugin/Auth/IdP.pm
index e1a0e2f593..501f7b549e 100644
--- a/Koha/REST/Plugin/Auth/IdP.pm
+++ b/Koha/REST/Plugin/Auth/IdP.pm
@@ -81,24 +81,15 @@ Generates a new session.
 
     $app->helper(
         'auth.session' => sub {
-            my ( $c, $patron ) = @_;
-            my $userid     = $patron->userid;
-            my $cardnumber = $patron->cardnumber;
-            my $cgi        = CGI->new;
-
-            $cgi->param( userid            => $userid );
-            $cgi->param( cardnumber        => $cardnumber );
-            $cgi->param( auth_client_login => 1 );
-
-            my ( $status, $cookie, $session_id ) = C4::Auth::check_api_auth($cgi);
-
-            Koha::Exceptions::UnderMaintenance->throw( code => 503 )
-              if $status eq "maintenance";
+            my ( $c, $params ) = @_;
+            my $patron      = $params->{patron};
+            my $interface   = $params->{interface};
+            my $provider    = $params->{provider};
 
-            Koha::Exceptions::Auth::CannotCreateSession->throw( code => 500 )
-              unless $status eq "ok";
+            my $session = C4::Auth::create_basic_session({ patron => $patron, interface => $interface });
+            $session->param('idp_code', $provider);
 
-            return ( $status, $cookie, $session_id );
+            return $session->id;
         }
     );
 }
diff --git a/Koha/REST/V1/OAuth/Client.pm b/Koha/REST/V1/OAuth/Client.pm
index 19079a31e1..5378cec0fe 100644
--- a/Koha/REST/V1/OAuth/Client.pm
+++ b/Koha/REST/V1/OAuth/Client.pm
@@ -65,7 +65,7 @@ sub login {
         $uri = '/cgi-bin/koha/mainpage.pl';
     }
 
-    unless ( $provider_config ) {
+    unless ( $provider_config && $provider_config->{authorize_url} ) {
         my $error = "No configuration found for your provider";
         return $c->redirect_to($uri."?auth_error=$error");
     }
@@ -99,7 +99,7 @@ sub login {
                     );
                 }
 
-                my ( $status, $cookie, $session_id ) = $c->auth->session($patron);
+                my $session_id = $c->auth->session({ patron => $patron, interface => $interface, provider => $provider });
 
                 $c->cookie( CGISESSID => $session_id, { path => "/" } );
 
diff --git a/t/db_dependent/Auth.t b/t/db_dependent/Auth.t
index c8da8bd89d..197dec5d95 100755
--- a/t/db_dependent/Auth.t
+++ b/t/db_dependent/Auth.t
@@ -10,7 +10,7 @@ use CGI qw ( -utf8 );
 use Test::MockObject;
 use Test::MockModule;
 use List::MoreUtils qw/all any none/;
-use Test::More tests => 16;
+use Test::More tests => 17;
 use Test::Warn;
 use t::lib::Mocks;
 use t::lib::TestBuilder;
@@ -833,4 +833,26 @@ subtest 'Userenv clearing in check_cookie_auth' => sub {
     is( C4::Context->userenv, undef, 'Environment should be cleared again' );
 };
 
+subtest 'create_basic_session tests' => sub {
+    plan tests => 12;
+
+    my $patron = $builder->build_object({ class => 'Koha::Patrons' });
+    my $interface = 'opac';
+
+    my $session = C4::Auth::create_basic_session({ patron => $patron, interface => $interface });
+
+    isnt($session->id, undef, 'A new sessionID was created');
+    is( $session->param('number'), $patron->borrowernumber, 'Session parameter number matches' );
+    is( $session->param('id'), $patron->userid, 'Session parameter id matches' );
+    is( $session->param('cardnumber'), $patron->cardnumber, 'Session parameter cardnumber matches' );
+    is( $session->param('firstname'), $patron->firstname, 'Session parameter firstname matches' );
+    is( $session->param('surname'), $patron->surname, 'Session parameter surname matches' );
+    is( $session->param('branch'), $patron->branchcode, 'Session parameter branch matches' );
+    is( $session->param('branchname'), $patron->library->branchname, 'Session parameter branchname matches' );
+    is( $session->param('flags'), $patron->flags, 'Session parameter flags matches' );
+    is( $session->param('emailaddress'), $patron->email, 'Session parameter emailaddress matches' );
+    is( $session->param('ip'), $session->remote_addr(), 'Session parameter ip matches' );
+    is( $session->param('interface'), $interface, 'Session parameter interface matches' );
+};
+
 $schema->storage->txn_rollback;
-- 
2.30.2