From 51d4c67f117574657d2ca9823074f750b10e48db Mon Sep 17 00:00:00 2001 From: Xavier Guimard Date: Thu, 14 Oct 2010 09:50:23 +0000 Subject: [PATCH] * OpenID: more tests for SREG * OpenID: delete trust persistent datas when confirm=-1 * make tidy --- .../lemonldap-ng-common/t/35-Common-Crypto.t | 3 +- .../lib/Lemonldap/NG/Handler/Simple.pm | 16 ++--- .../lib/Lemonldap/NG/Handler/Vhost.pm | 2 +- .../lib/Lemonldap/NG/Manager/Sessions.pm | 2 +- .../lib/Lemonldap/NG/Manager/_Struct.pm | 8 +-- .../lib/Lemonldap/NG/Portal/IssuerDBCAS.pm | 6 +- .../lib/Lemonldap/NG/Portal/IssuerDBOpenID.pm | 2 + .../lib/Lemonldap/NG/Portal/OpenID/SREG.pm | 15 +++-- .../lib/Lemonldap/NG/Portal/Simple.pm | 14 ++--- .../t/27-Lemonldap-NG-Portal-IssuerDBOpenID.t | 62 +++++++++++++++---- 10 files changed, 91 insertions(+), 39 deletions(-) diff --git a/modules/lemonldap-ng-common/t/35-Common-Crypto.t b/modules/lemonldap-ng-common/t/35-Common-Crypto.t index 1129d8ecb..ee12b8b7e 100644 --- a/modules/lemonldap-ng-common/t/35-Common-Crypto.t +++ b/modules/lemonldap-ng-common/t/35-Common-Crypto.t @@ -28,6 +28,7 @@ ok( foreach my $i ( 1 .. 17 ) { my $s = ''; $s = join( '', map { chr( int( rand(94) ) + 33 ) } ( 1 .. $i ) ); - ok( $c->decrypt( $c->encrypt($s) ) eq $s, "Test with $i characters string" ); + ok( $c->decrypt( $c->encrypt($s) ) eq $s, + "Test with $i characters string" ); } diff --git a/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Simple.pm b/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Simple.pm index 16588dd62..0f5d5e68a 100644 --- a/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Simple.pm +++ b/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Simple.pm @@ -966,16 +966,16 @@ sub encodeUrl { # @return URL sub _buildUrl { my ( $class, $s ) = splice @_; - my $portString = $port || $apacheRequest->get_server_port(); - $portString = - ( $https && $portString == 443 ) ? '' - : ( !$https && $portString == 80 ) ? '' - : ':' . $portString; + my $portString = $port || $apacheRequest->get_server_port(); + $portString = + ( $https && $portString == 443 ) ? '' + : ( !$https && $portString == 80 ) ? '' + : ':' . $portString; return "http" - . ( $https ? "s" : "" ) . "://" - . $apacheRequest->get_server_name() - . $portString + . ( $https ? "s" : "" ) . "://" + . $apacheRequest->get_server_name() + . $portString . $s; } diff --git a/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Vhost.pm b/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Vhost.pm index ca6778cf0..e06c06d10 100644 --- a/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Vhost.pm +++ b/modules/lemonldap-ng-handler/lib/Lemonldap/NG/Handler/Vhost.pm @@ -28,7 +28,7 @@ sub defaultValuesInit { # Override with vhost options if ( defined $args->{vhostOptions} ) { - my $n = 'vhost' . ucfirst($t); + my $n = 'vhost' . ucfirst($t); foreach my $k ( keys %{ $args->{vhostOptions} } ) { my $v = $args->{vhostOptions}->{$k}->{$n}; $class->lmLog( "Options $t for vhost $k: $v", 'debug' ); diff --git a/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Sessions.pm b/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Sessions.pm index 1fa53434f..4abd03c0b 100644 --- a/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Sessions.pm +++ b/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/Sessions.pm @@ -66,7 +66,7 @@ sub new { # Now try to load Apache::Session module unless ( $globalStorage->can('populate') ) { eval "require $globalStorage"; - $class->abort( "Unable to load $globalStorage", $@ ) if ($@); + $class->abort( "Unable to load $globalStorage", $@ ) if ($@); } %{ $self->{globalStorageOptions} } = %$globalStorageOptions; $self->{globalStorageOptions}->{backend} = $globalStorage; diff --git a/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/_Struct.pm b/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/_Struct.pm index 6d308247e..1f1c51dcf 100644 --- a/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/_Struct.pm +++ b/modules/lemonldap-ng-manager/lib/Lemonldap/NG/Manager/_Struct.pm @@ -32,7 +32,7 @@ sub cstruct { _nodes => [ qw(rules:rules:rules headers post:post:post vhostOptions) ], - rules => { + rules => { _nodes => ["hash:/locationRules/$k2:rules:rules"], _js => 'rulesRoot' }, @@ -467,7 +467,7 @@ sub struct { # OpenID openIdParams => { - _nodes => [qw(openIdAuthnLevel openIdSecret openIdIDPList)], + _nodes => [qw(openIdAuthnLevel openIdSecret openIdIDPList)], openIdAuthnLevel => 'int:/openIdAuthnLevel', openIdSecret => 'text:/openIdSecret', openIdIDPList => @@ -602,7 +602,7 @@ sub struct { openIdAttr => 'text:openIdAttr', openIdSPList => 'text:/openIdSPList:issuerdbopenid:openididplist', - openIdSreg => { + openIdSreg => { _nodes => [ qw(openIdSreg_fullname openIdSreg_nickname openIdSreg_language openIdSreg_postcode openIdSreg_timezone openIdSreg_country openIdSreg_gender openIdSreg_email openIdSreg_dob) ], @@ -1401,7 +1401,7 @@ sub testStruct { keyMsgFail => 'Bad option name', }, }, - whatToTrace => $lmAttrOrMacro, + whatToTrace => $lmAttrOrMacro, ######## # SAML # diff --git a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBCAS.pm b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBCAS.pm index 1bb60640f..d9226972c 100644 --- a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBCAS.pm +++ b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBCAS.pm @@ -179,7 +179,8 @@ sub issuerForUnAuthUser { } # Get username - my $username = $localSession->{ $self->{casAttr} || $self->{whatToTrace} }; + my $username = + $localSession->{ $self->{casAttr} || $self->{whatToTrace} }; $self->lmLog( "Get username $username", 'debug' ); @@ -388,7 +389,8 @@ sub issuerForUnAuthUser { } # Get username - my $username = $localSession->{ $self->{casAttr} || $self->{whatToTrace} }; + my $username = + $localSession->{ $self->{casAttr} || $self->{whatToTrace} }; $self->lmLog( "Get username $username", 'debug' ); diff --git a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBOpenID.pm b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBOpenID.pm index 60a547c31..8185ce186 100644 --- a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBOpenID.pm +++ b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/IssuerDBOpenID.pm @@ -191,6 +191,8 @@ sub openIDServer { return 1; } elsif ( $self->param("confirm") == -1 ) { + $self->updatePersistentSession( + { "_openidTrust$trust_root" => 0 } ); return 0; } else { diff --git a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/OpenID/SREG.pm b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/OpenID/SREG.pm index c5cd5a309..f0be512d6 100644 --- a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/OpenID/SREG.pm +++ b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/OpenID/SREG.pm @@ -19,10 +19,17 @@ sub sregHook { my ( @req, @opt ); # Refuse federation if rejected by user - return 0 if ( $self->param('confirm') == -1 ); + if ( $self->param('confirm') == -1 ) { + my %h; + $h{$_} = undef foreach ( + qw(fullname nickname language postcode timezone country gender email dob) + ); + $self->updatePersistentSession( \%h ); + return 0; + } # If identity is not trusted, does nothing - return ( 0, $prm ) unless ($is_id and $is_trusted); + return ( 0, $prm ) unless ( $is_id and $is_trusted ); $self->lmLog( "SREG start", 'debug' ); @@ -59,8 +66,8 @@ sub sregHook { # Parse optional attributes elsif ( $k eq 'optional' ) { $self->lmLog( "Optional attr $v", 'debug' ); - push @opt, - grep { defined $self->{"openIdSreg_$_"} } split( /,/, $v ); + push @opt, grep { defined $self->{"openIdSreg_$trust_root$_"} } + split( /,/, $v ); } else { $self->lmLog( "Unknown OpenID SREG request $k", 'error' ); diff --git a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Simple.pm b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Simple.pm index 7fb890ca7..c4efc35d6 100644 --- a/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Simple.pm +++ b/modules/lemonldap-ng-portal/lib/Lemonldap/NG/Portal/Simple.pm @@ -834,7 +834,7 @@ sub updatePersistentSession { if ( defined( $infos->{$_} ) ) { $self->lmLog( "Update persistent session key $_ with " . $infos->{$_}, - 'debug' ); + 'debug' ); $h->{$_} = $infos->{$_}; } else { @@ -873,8 +873,8 @@ sub updateSession { $id ||= $self->{id}; unless ($id) { my %cookies = fetch CGI::Cookie; - $id ||= $cookies{ $self->{cookieName} }->value - if ( defined $cookies{ $self->{cookieName} } ); + $id ||= $cookies{ $self->{cookieName} }->value + if ( defined $cookies{ $self->{cookieName} } ); } if ($id) { @@ -885,8 +885,8 @@ sub updateSession { if ( defined( $infos->{$_} ) ) { $self->lmLog( "Update session key $_ with " . $infos->{$_}, 'debug' ); - $h->{$_} = $infos->{$_}; - } + $h->{$_} = $infos->{$_}; + } else { $self->lmLog( "Delete session key $_", 'debug' ); delete $h->{$_}; @@ -1693,8 +1693,8 @@ sub setSessionInfo { foreach my $k ( keys %$h2 ) { $self->{sessionInfo}->{$k} = $h2->{$k}; } - untie %$h2; - } + untie %$h2; + } } PE_OK; } diff --git a/modules/lemonldap-ng-portal/t/27-Lemonldap-NG-Portal-IssuerDBOpenID.t b/modules/lemonldap-ng-portal/t/27-Lemonldap-NG-Portal-IssuerDBOpenID.t index 54dc1e9c7..c540263e1 100644 --- a/modules/lemonldap-ng-portal/t/27-Lemonldap-NG-Portal-IssuerDBOpenID.t +++ b/modules/lemonldap-ng-portal/t/27-Lemonldap-NG-Portal-IssuerDBOpenID.t @@ -1,6 +1,6 @@ package My::Portal; -use Test::More tests => 8; +use Test::More tests => 13; use strict; our @ISA = qw(Lemonldap::NG::Portal::IssuerDBOpenID @@ -8,14 +8,15 @@ our @ISA = qw(Lemonldap::NG::Portal::IssuerDBOpenID sub lmLog { my ( $self, $msg, $level ) = splice @_; + #print STDERR "[$level] $msg\n"; } -our $CONFIRM; +our $param = { confirm => 0 }; sub param { my ( $self, $key ) = splice @_; - return { confirm => $CONFIRM, }->{$key}; + return $param->{$key}; } sub info { } @@ -33,25 +34,64 @@ SKIP: { use_ok('Lemonldap::NG::Portal::IssuerDBOpenID'); use_ok('Lemonldap::NG::Portal::OpenID::SREG'); - my $p = bless {}, __PACKAGE__; + my $p = bless { + sessionInfo => { + uid => 'test', + mail => 'x.x.org' + }, + }, + __PACKAGE__; my ( $r, $h ); ( $r, $h ) = $p->sregHook( '', '', 0, 0, {} ); - ok( $r == 0, 'Call sregHook with untrusted request' ); - $CONFIRM = -1; - ok( !$p->sregHook( '', '', 1, 1, {} ), 'Call sregHook with confirm => -1' ); - $CONFIRM = 1; - ok( $p->sregHook( '', '', 1, 1, {} ), 'Call sregHook without arguments' ); + ok( $r == 0, 'SREG: Call sregHook with untrusted request' ); + $param->{confirm} = -1; + ok( + !$p->sregHook( '', '', 1, 1, {} ), + 'SREG: call sregHook with confirm => -1' + ); + $param->{confirm} = 1; + ok( + $p->sregHook( '', '', 1, 1, {} ), + 'SREG: call sregHook without arguments' + ); ( $r, $h ) = $p->sregHook( '', '', 1, 1, { required => 'fullname,email', optional => 'nickname' }, ); - ok( $r == 0, 'Unconfigured required attributes' ); + ok( $r == 0, 'SREG: 0 returned unless required attributes are configured' ); $p->{openIdSreg_fullname} = '$uid'; $p->{openIdSreg_email} = '$mail'; + $p->{openIdSreg_nickname} = '$uid'; + ( $r, $h ) = + $p->sregHook( '', '', 1, 1, + { required => 'fullname,email', optional => 'nickname' }, + ); + ok( $r == 1, 'SREG: 1 returned if required attributes are configured' ); + ok( ref($h), 'SREG: Parameters returned as hashref' ); + ok( ( $h->{email} eq 'x.x.org' and $h->{fullname} eq 'test' ), + 'SREG: required attributes returned' ); + ok( !defined( $h->{nickname} ), 'SREG: optional parameter not returned' ); + $param->{sreg_nickname} = 0; ( $r, $h ) = $p->sregHook( '', '', 1, 1, { required => 'fullname,email', optional => 'nickname' }, ); - ok( $r == 1, 'Configured required attributes' ); + ok( !defined( $h->{nickname} ), + 'SREG: optional unwanted parameter not returned' ); + $param->{sreg_nickname} = 'OK'; + ( $r, $h ) = + $p->sregHook( '', '', 1, 1, + { required => 'fullname,email', optional => 'nickname' }, + ); + ok( defined( $h->{nickname} ), 'SREG: optional wanted parameter returned' ); + + $param->{confirm} = 0; + ( $r, $h ) = + $p->sregHook( '', '', 1, 1, + { required => 'fullname,email', optional => 'nickname' }, + ); + ok( ( $r == 0 and ref($h) ), + 'SREG: 0 returned for unconfirmed parameters' ); } +