Skip Menu |

This queue is for tickets about the Catalyst-Authentication-Store-LDAP CPAN distribution.

Report information
The Basics
Id: 57610
Status: resolved
Priority: 0/
Queue: Catalyst-Authentication-Store-LDAP

People
Owner: bobtfish [...] bobtfish.net
Requestors: fried [...] cpan.org
Cc: peter [...] peknet.com
AdminCc:

Bug Information
Severity: Normal
Broken in: 1.009
Fixed in: (no value)



Subject: User.pm
has_attribute has a special case for 'dn' using the underlying ldap_entry but the AUTOLOAD does not have this special case for 'dn' Also AUTOLOAD has a special case for 'username' but has_attribute does not. These two methods should have similar functionality for sanity reasons. I attached a patch file for what I am talking about
Subject: user.patch
--- User.pm 2009-12-11 12:24:44.000000000 -0600 +++ /home/fried/User.pm 2010-05-18 15:59:59.000000000 -0500 @@ -226,6 +226,9 @@ if ( $attribute eq "dn" ) { return $self->ldap_entry->dn; } + elsif ( $attribute eq "username" ) { + return $self->user->{'attributes'}->{$self->store->user_field}; + } elsif ( exists( $self->user->{'attributes'}->{$attribute} ) ) { return $self->user->{'attributes'}->{$attribute}; } @@ -294,20 +297,9 @@ if ( $method eq "DESTROY" ) { return; } - if ( exists( $self->user->{'attributes'}->{$method} ) ) { - return $self->user->{'attributes'}->{$method}; - } - elsif ( $method eq "username" ) { - my $userfield = $self->store->user_field; - my $username = $self->has_attribute($userfield); - if ($username) { - return $username; - } - else { - Catalyst::Exception->throw( "User is missing the " - . $userfield - . " attribute, which should not be possible!" ); - } + + if ( my $attribute = $self->has_attribute($method) ) { + return $attribute; } else { Catalyst::Exception->throw(
Can you supply tests for this patch to demonstrate what it's actually doing / fixing? Cheers t0m
On Tue May 18 17:17:38 2010, BOBTFISH wrote: Show quoted text
> Can you supply tests for this patch to demonstrate what it's actually > doing / fixing? > > Cheers > t0m
I added my test file user.t, replace $user, $pass and ,$domain with valid information. So without my patch the test preforms like this. t/user.t .................. 1/10 # Failed test 'Get DN from AUTOLOAD' # at t/user.t line 14. # Failed test 'Get username from has_attribute' # at t/user.t line 17. Use of uninitialized value in string eq at t/user.t line 18, <DATA> line 828. # Failed test 'username from AUTOLOAD and has_attribute should match' # at t/user.t line 18. # Failed test 'dn from AUTOLOAD and has_attribute should match' # at t/user.t line 19. # Looks like you failed 4 tests of 10. With my patch t/user.t .................. ok
Subject: user.t.txt
use strict; use warnings; use Test::More tests => 10; BEGIN { use_ok('Catalyst::Test', 'Portal') } BEGIN { use_ok 'Portal::User' } ok my $c = Portal->new, 'Create Context Object'; ok $c->authenticate( { username => $user, password => $pass }, $domain ); ok eval { $c->user->dn; },"Get DN from AUTOLOAD"; ok defined $c->user->has_attribute('dn'),"Get dn from has_attribute"; ok $c->user->username,"Get username from AUTOLOAD"; ok defined $c->user->has_attribute('username'),"Get username from has_attribute"; ok $c->user->username eq $c->user->has_attribute('username'),"username from AUTOLOAD and has_attribute should match"; ok eval { $c->user->dn eq $c->user->has_attribute('dn'); },"dn from AUTOLOAD and has_attribute should match";
t0m is that what you need? On Fri May 21 13:58:08 2010, FRIED wrote: Show quoted text
> On Tue May 18 17:17:38 2010, BOBTFISH wrote:
> > Can you supply tests for this patch to demonstrate what it's actually > > doing / fixing? > > > > Cheers > > t0m
Yes, thanks. Looks good from a very quick examination. I'll try to give it a more detailed look and apply the patch in the next 24-36 hours :)
Erm, having reviewed the patch, this looks reasonable, but you haven't included the app the test needs, so I can in no way add the test to the repository to test for regressions. Would it be possible for you to supply a test which works with the test app in the distribution, or is there something stopping that?
On Sun Jun 27 17:24:06 2010, BOBTFISH wrote: Show quoted text
> Erm, having reviewed the patch, this looks reasonable, but you haven't > included the app the test needs, so I can in no way add the test to the > repository to test for regressions. > > Would it be possible for you to supply a test which works with the test > app in the distribution, or is there something stopping that?
Sorry about that, I'll re-model my test using existing tests. BRB
T0m, Here is the new test. I have only one problem with the test LDAP server, it defines an attribute 'dn'. Active Directory does not have this attribute its called 'distinguishedName' instead. This generates a false positive, that will fail on some real world LDAP servers.
Subject: 05-user_attributes.t
#!/usr/bin/perl use strict; use warnings; use Catalyst::Exception; use Test::More tests => 9; use lib 't/lib'; use LDAPTest; SKIP: { eval "use Catalyst::Model::LDAP"; if ($@) { skip "Catalyst::Model::LDAP not installed", 6; } my $server = LDAPTest::spawn_server(); use_ok("Catalyst::Authentication::Store::LDAP::Backend"); my $back = Catalyst::Authentication::Store::LDAP::Backend->new( { 'ldap_server' => LDAPTest::server_host(), 'binddn' => 'anonymous', 'bindpw' => 'dontcarehow', 'start_tls' => 0, 'user_basedn' => 'ou=foobar', 'user_filter' => '(&(objectClass=person)(uid=%s))', 'user_scope' => 'one', 'user_field' => 'uid', 'use_roles' => 0, 'entry_class' => 'EntryClass', } ); isa_ok( $back, "Catalyst::Authentication::Store::LDAP::Backend" ); my $user = $back->find_user( { username => 'somebody' } ); isa_ok( $user, "Catalyst::Authentication::Store::LDAP::User" ); #Check DN ok $user->dn,"Get DN from AUTOLOAD"; #THIS ONLY WORKS BECAUSE dn is included as a user attribute in the test LDAP server. ok defined $user->has_attribute('dn'),"Get dn from has_attribute"; #Check Username ok $user->username, "Get username from AUTOLOAD"; ok defined $user->has_attribute('username'),"Get username from has_attribute"; #Make sure both methods match output ok $user->username eq $user->has_attribute('username'),"username from AUTOLOAD and has_attribute should match"; ok $user->dn eq $user->has_attribute('dn'),"dn from AUTOLOAD and has_attribute should match"; }
Thanks, applied and released.
Subject: Re: [rt.cpan.org #57610] User.pm
Date: Wed, 07 Jul 2010 13:25:46 -0500
To: bug-Catalyst-Authentication-Store-LDAP [...] rt.cpan.org
From: Peter Karman <peter [...] peknet.com>
Tomas Doran via RT wrote on 07/06/2010 04:30 PM: Show quoted text
> Queue: Catalyst-Authentication-Store-LDAP > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=57610 > > > Thanks, applied and released.
looks like 1.010 is failing where Catalyst::Model::LDAP is not installed, due to the incorrect number of skipped tests. It's fixed now in trunk. t0m, if you want to push a 1.011 release, feel free. Otherwise, I'll try and do it tonight. -- Peter Karman . http://peknet.com/ . peter@peknet.com
Thanks for catching that Peter! I've just uploaded 1.011, so I'm closing this (again). Cheers t0m