From 706c850315577aecf5431f27f9b133edfe053478 Mon Sep 17 00:00:00 2001
From: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Date: Fri, 5 Oct 2018 10:19:14 +0200
Subject: [PATCH] Bug 17776: (QA follow-up) Remove shibboleth package variables
Content-Type: text/plain; charset=utf-8

This is about $shib and $shib_login.
We move in the right direction by calling get_login_shib in
get_template_and_user and checkauth. In the same line we can do the
shib_ok check at that time (just checking cached values). This paves
the way for the third subroutine using the two package vars: checkpw.
Note that checkpw is also called outside Auth.pm. So I would be more
comfortable if we do the same calls like in checkauth and remove both
variables from the package level (especially under Plack of course).

The former changes actually justify a 'use C4::Auth_with_shibboleth'
instead of the current require and import.

Note: When calling checkpw from checkauth, we are calling get_login_shib
twice now. But the time involved for doing so is around zero (cache), so
not really an argument for extra parameters and complexer code.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
---
 C4/Auth.pm                 | 22 ++++++++++------------
 C4/Auth_with_shibboleth.pm |  2 +-
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/C4/Auth.pm b/C4/Auth.pm
index 303db34..ddcaf1a 100644
--- a/C4/Auth.pm
+++ b/C4/Auth.pm
@@ -41,9 +41,10 @@ use Koha::Patron::Consents;
 use POSIX qw/strftime/;
 use List::MoreUtils qw/ any /;
 use Encode qw( encode is_utf8);
+use C4::Auth_with_shibboleth;
 
 # use utf8;
-use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS $debug $ldap $cas $caslogout $shib $shib_login);
+use vars qw(@ISA @EXPORT @EXPORT_OK %EXPORT_TAGS $debug $ldap $cas $caslogout);
 
 BEGIN {
     sub psgi_env { any { /^psgi\./ } keys %ENV }
@@ -62,7 +63,6 @@ BEGIN {
     %EXPORT_TAGS = ( EditPermissions => [qw(get_all_subpermissions get_user_subpermissions)] );
     $ldap      = C4::Context->config('useldapserver') || 0;
     $cas       = C4::Context->preference('casAuthentication');
-    $shib      = C4::Context->config('useshibboleth') || 0;
     $caslogout = C4::Context->preference('casLogout');
     require C4::Auth_with_cas;    # no import
 
@@ -70,14 +70,6 @@ BEGIN {
         require C4::Auth_with_ldap;
         import C4::Auth_with_ldap qw(checkpw_ldap);
     }
-    if ($shib) {
-        require C4::Auth_with_shibboleth;
-        import C4::Auth_with_shibboleth
-          qw(shib_ok checkpw_shib logout_shib login_shib_url get_login_shib);
-
-        # Check for good config
-        $shib = 0 unless shib_ok();
-    }
     if ($cas) {
         import C4::Auth_with_cas qw(check_api_auth_cas checkpw_cas login_cas logout_cas login_cas_url logout_if_required);
     }
@@ -153,7 +145,8 @@ sub get_template_and_user {
     my ( $user, $cookie, $sessionID, $flags );
 
     # Get shibboleth login attribute
-    $shib_login = get_login_shib() if $shib;
+    my $shib = C4::Context->config('useshibboleth') && shib_ok();
+    my $shib_login = $shib ? get_login_shib() : undef;
 
     C4::Context->interface( $in->{type} );
 
@@ -786,7 +779,8 @@ sub checkauth {
     $debug and warn "Checking Auth";
 
     # Get shibboleth login attribute
-    $shib_login = get_login_shib() if $shib;
+    my $shib = C4::Context->config('useshibboleth') && shib_ok();
+    my $shib_login = $shib ? get_login_shib() : undef;
 
     # $authnotrequired will be set for scripts which will run without authentication
     my $authnotrequired = shift;
@@ -1773,6 +1767,10 @@ sub checkpw {
     my ( $dbh, $userid, $password, $query, $type, $no_set_userenv ) = @_;
     $type = 'opac' unless $type;
 
+    # Get shibboleth login attribute
+    my $shib = C4::Context->config('useshibboleth') && shib_ok();
+    my $shib_login = $shib ? get_login_shib() : undef;
+
     my @return;
     my $patron = Koha::Patrons->find({ userid => $userid });
     my $check_internal_as_fallback = 0;
diff --git a/C4/Auth_with_shibboleth.pm b/C4/Auth_with_shibboleth.pm
index 480d5d8..0ff0d25 100644
--- a/C4/Auth_with_shibboleth.pm
+++ b/C4/Auth_with_shibboleth.pm
@@ -171,7 +171,7 @@ sub _get_shib_config {
     my $config = C4::Context->config('shibboleth');
 
     if ( !$config ) {
-        carp 'shibboleth config not defined';
+        carp 'shibboleth config not defined' if $debug;
         return 0;
     }
 
-- 
2.1.4