Bugzilla – Attachment 32165 Details for
Bug 8446
Shibboleth authentication
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[PASSED QA] BUG8446, Follow up: Refactor to clean up bad practice
PASSED-QA-BUG8446-Follow-up-Refactor-to-clean-up-b.patch (text/plain), 6.47 KB, created by
Katrin Fischer
on 2014-10-10 15:07:22 UTC
(
hide
)
Description:
[PASSED QA] BUG8446, Follow up: Refactor to clean up bad practice
Filename:
MIME Type:
Creator:
Katrin Fischer
Created:
2014-10-10 15:07:22 UTC
Size:
6.47 KB
patch
obsolete
>From 565b785bd656656e3d9156b7ecb274f3b7eec2af Mon Sep 17 00:00:00 2001 >From: Martin Renvoize <martin.renvoize@ptfs-europe.com> >Date: Thu, 31 Jul 2014 15:06:19 +0000 >Subject: [PATCH] [PASSED QA] BUG8446, Follow up: Refactor to clean up bad > practice > >- A number of issues were highlighted whilst writing sensible unit tests > for this module. > - Removed unnessesary call to context->new(); > - Global variables are BAD! > - Croaking is a wimps way out, we should handle errors early and > properly. > >Signed-off-by: Matthias Meusburger <matthias.meusburger@biblibre.com> >Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> >--- > C4/Auth.pm | 13 +++++-- > C4/Auth_with_shibboleth.pm | 92 ++++++++++++++++++++++++++++++++++------------ > 2 files changed, 79 insertions(+), 26 deletions(-) > >diff --git a/C4/Auth.pm b/C4/Auth.pm >index c384992..646d933 100644 >--- a/C4/Auth.pm >+++ b/C4/Auth.pm >@@ -65,10 +65,17 @@ BEGIN { > } > if ($shib) { > import C4::Auth_with_shibboleth >- qw(checkpw_shib logout_shib login_shib_url get_login_shib); >+ qw(shib_ok checkpw_shib logout_shib login_shib_url get_login_shib); > >- # Get shibboleth login attribute >- $shib_login = get_login_shib(); >+ # Check for good config >+ if ( shib_ok() ) { >+ # Get shibboleth login attribute >+ $shib_login = get_login_shib(); >+ } >+ # Bad config, disable shibboleth >+ else { >+ $shib = 0; >+ } > } > if ($cas) { > import C4::Auth_with_cas qw(check_api_auth_cas checkpw_cas login_cas logout_cas login_cas_url); >diff --git a/C4/Auth_with_shibboleth.pm b/C4/Auth_with_shibboleth.pm >index 18538ab..863ea82 100644 >--- a/C4/Auth_with_shibboleth.pm >+++ b/C4/Auth_with_shibboleth.pm >@@ -32,30 +32,36 @@ BEGIN { > $VERSION = 3.03; # set the version for version checking > $debug = $ENV{DEBUG}; > @ISA = qw(Exporter); >- @EXPORT = qw(logout_shib login_shib_url checkpw_shib get_login_shib); >+ @EXPORT = qw(shib_ok logout_shib login_shib_url checkpw_shib get_login_shib); >+} >+ >+# Check that shib config is not malformed >+sub shib_ok { >+ my $config = _get_shib_config(); >+ >+ if ( $config ) { >+ return 1; >+ } >+ >+ return 0; > } >-my $context = C4::Context->new() or die 'C4::Context->new failed'; >-my $shib = C4::Context->config('shibboleth') or croak 'No <shibboleth> in koha-conf.xml'; >-my $shibbolethMatchField = $shib->{matchpoint} or croak 'No <matchpoint> defined in koha-conf.xml'; >-my $shibbolethMatchAttribute = $shib->{mapping}->{$shibbolethMatchField}->{is} or croak 'Matchpoint not mapped in koha-conf.xml'; >-my $protocol = "https://"; > > # Logout from Shibboleth > sub logout_shib { > my ($query) = @_; >- my $uri = $protocol . C4::Context->preference('OPACBaseURL'); >+ my $uri = _get_uri(); > print $query->redirect( $uri . "/Shibboleth.sso/Logout?return=$uri" ); > } > > # Returns Shibboleth login URL with callback to the requesting URL > sub login_shib_url { >- > my ($query) = @_; >- my $param = $protocol . C4::Context->preference('OPACBaseURL') . $query->script_name(); >+ >+ my $param = _get_uri() . $query->script_name(); > if ( $query->query_string() ) { > $param = $param . '%3F' . $query->query_string(); > } >- my $uri = $protocol . C4::Context->preference('OPACBaseURL') . "/Shibboleth.sso/Login?target=$param"; >+ my $uri = _get_uri() . "/Shibboleth.sso/Login?target=$param"; > return $uri; > } > >@@ -69,11 +75,13 @@ sub get_login_shib { > # Shibboleth attributes are mapped into http environmement variables, so we're getting > # the match point of the user this way > >- $debug and warn "koha borrower field to match: $shibbolethMatchField"; >- $debug and warn "shibboleth attribute to match: $shibbolethMatchAttribute"; >- $debug and warn "$shibbolethMatchAttribute value: $ENV{$shibbolethMatchAttribute}"; >+ # Get shibboleth config >+ my $config = _get_shib_config(); > >- return $ENV{$shibbolethMatchAttribute} || ''; >+ my $matchAttribute = $config->{mapping}->{ $config->{matchpoint} }->{is}; >+ $debug and warn $matchAttribute . " value: " . $ENV{ $matchAttribute }; >+ >+ return $ENV{ $matchAttribute } || ''; > } > > # Checks for password correctness >@@ -81,25 +89,63 @@ sub get_login_shib { > sub checkpw_shib { > $debug and warn "checkpw_shib"; > >- my ( $dbh, $userid ) = @_; >- my $retnumber; >- $debug and warn "User Shibboleth-authenticated as: $userid"; >+ my ( $dbh, $match ) = @_; >+ my ( $retnumber, $userid ); >+ my $config = _get_shib_config(); >+ $debug and warn "User Shibboleth-authenticated as: $match"; > >- # Does the given shibboleth attribute value ($userid) match a valid koha user ? >- my $sth = $dbh->prepare("select cardnumber, userid from borrowers where $shibbolethMatchField=?"); >- $sth->execute($userid); >+ # 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[1]; >- $userid = $retvals[0]; >+ $retnumber = $retvals[0]; >+ $userid = $retvals[1]; > return ( 1, $retnumber, $userid ); > } > > # If we reach this point, the user is not a valid koha user >- $debug and warn "User $userid is not a valid Koha user"; >+ $debug and warn "User with $config->{matchpoint} of $match is not a valid Koha user"; > return 0; > } > >+sub _get_uri { >+ >+ my $protocol = "https://"; >+ >+ my $return = $protocol . C4::Context->preference('OPACBaseURL'); >+ return $return; >+} >+ >+sub _get_shib_config { >+ my $config = C4::Context->config('shibboleth'); >+ >+ if ( !$config ) { >+ carp 'shibboleth config not defined'; >+ return 0; >+ } >+ >+ if ( $config->{matchpoint} >+ && defined( $config->{mapping}->{ $config->{matchpoint} }->{is} ) ) >+ { >+ if ($debug) { >+ warn "koha borrower field to match: " . $config->{matchpoint}; >+ warn "shibboleth attribute to match: " >+ . $config->{mapping}->{ $config->{matchpoint} }->{is}; >+ } >+ return $config; >+ } >+ else { >+ if ( !$config->{matchpoint} ) { >+ carp 'shibboleth matchpoint not defined'; >+ } >+ else { >+ carp 'shibboleth matchpoint not mapped'; >+ } >+ return 0; >+ } >+} >+ > 1; > __END__ > >-- >1.9.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 8446
:
10841
|
22098
|
22107
|
22860
|
23224
|
23225
|
23226
|
25765
|
25766
|
26176
|
26177
|
26178
|
26187
|
26749
|
26750
|
26751
|
26752
|
26760
|
26761
|
26762
|
28892
|
28893
|
28894
|
28895
|
28896
|
28897
|
28898
|
28899
|
28900
|
28901
|
28902
|
28903
|
30017
|
30018
|
30019
|
30020
|
30021
|
30022
|
30023
|
30030
|
30031
|
30033
|
30283
|
30284
|
30285
|
30286
|
30335
|
30353
|
30354
|
30355
|
30356
|
30384
|
30594
|
30604
|
30605
|
30616
|
30617
|
30618
|
30619
|
30620
|
30621
|
30684
|
31407
|
31408
|
31836
|
31905
|
31906
|
31907
|
32125
|
32158
|
32162
|
32163
|
32164
|
32165
|
32166
|
32167
|
32168
|
32169
|
32170
|
32365
|
32366
|
32367
|
32368
|
32369
|
32370
|
32371
|
32372
|
32373
|
32374
|
32426
|
32427