Bug #43397 for Catalyst-Authentication-Store-LDAP: Should not throw an exception when user cannot be found

Hiya When a user cannot be found in your store, you should return undef rather than throwing an exception. If you throw an exception, it ripples up and has to be caught by the caller who called $c->authenticate, whereas usually you just use the truth of the return value to determine a user logged in. Currently the behavior isn't documented or tested, so I've attached a which corrects the behavior and adds tests / docs.

diff -ur Catalyst-Authentication-Store-LDAP-0.1004/Changes Catalyst-Authentication-Store-LDAP-0.1004.t0m/Changes --- Catalyst-Authentication-Store-LDAP-0.1004/Changes 2008-10-22 02:54:50.000000000 +0100 +++ Catalyst-Authentication-Store-LDAP-0.1004.t0m/Changes 2009-02-18 15:06:14.000000000 +0000 @@ -1,3 +1,8 @@ +0.1005 UNRELEASED + - Stop throwing an exception when the lookup_user method fails + to find a user and instead return undef. (t0m) + - Add tests for this (t0m) + 0.1004 21 Oct 2008 - Add the ability to have the user inflated into a custom user class with the user_class option (t0m) diff -ur Catalyst-Authentication-Store-LDAP-0.1004/lib/Catalyst/Authentication/Store/LDAP/Backend.pm Catalyst-Authentication-Store-LDAP-0.1004.t0m/lib/Catalyst/Authentication/Store/LDAP/Backend.pm --- Catalyst-Authentication-Store-LDAP-0.1004/lib/Catalyst/Authentication/Store/LDAP/Backend.pm 2008-10-22 02:46:27.000000000 +0100 +++ Catalyst-Authentication-Store-LDAP-0.1004.t0m/lib/Catalyst/Authentication/Store/LDAP/Backend.pm 2009-02-18 15:17:57.000000000 +0000 @@ -80,7 +80,7 @@ use strict; use warnings; -our $VERSION = '0.1004'; +our $VERSION = '0.1005'; use Catalyst::Authentication::Store::LDAP::User; use Net::LDAP; @@ -263,11 +263,12 @@ A) Bind to the directory using the configured binddn and bindpw B) Perform a search for the User Object in the directory, using user_basedn, user_filter, and user_scope. - C) Assuming we found the object, we will walk it's attributes + C) Assuming we found the object, we will walk it's attributes using L<Net::LDAP::Entry>'s get_value method. We store the - results in a hashref. - D) Return a hashref that looks like: - + results in a hashref. If we do not find the object, then + undef is returned. + D) Return a hashref that looks like: + $results = { 'ldap_entry' => $entry, # The Net::LDAP::Entry object 'attributes' => $attributes, @@ -300,10 +301,9 @@ push( @searchopts, %{ $self->user_search_options } ); } my $usersearch = $ldap->search(@searchopts); - if ( $usersearch->is_error ) { - Catalyst::Exception->throw( - "LDAP Error while searching for user: " . $usersearch->error ); - } + + return if ( $usersearch->is_error ); + my $userentry; my $user_field = $self->user_field; my $results_filter = $self->user_results_filter; diff -ur Catalyst-Authentication-Store-LDAP-0.1004/lib/Catalyst/Authentication/Store/LDAP/User.pm Catalyst-Authentication-Store-LDAP-0.1004.t0m/lib/Catalyst/Authentication/Store/LDAP/User.pm --- Catalyst-Authentication-Store-LDAP-0.1004/lib/Catalyst/Authentication/Store/LDAP/User.pm 2008-10-22 02:46:27.000000000 +0100 +++ Catalyst-Authentication-Store-LDAP-0.1004.t0m/lib/Catalyst/Authentication/Store/LDAP/User.pm 2009-02-18 15:06:40.000000000 +0000 @@ -46,7 +46,7 @@ use strict; use warnings; -our $VERSION = '0.1004'; +our $VERSION = '0.1005'; BEGIN { __PACKAGE__->mk_accessors(qw/user store/) } diff -ur Catalyst-Authentication-Store-LDAP-0.1004/lib/Catalyst/Authentication/Store/LDAP.pm Catalyst-Authentication-Store-LDAP-0.1004.t0m/lib/Catalyst/Authentication/Store/LDAP.pm --- Catalyst-Authentication-Store-LDAP-0.1004/lib/Catalyst/Authentication/Store/LDAP.pm 2008-10-22 02:46:27.000000000 +0100 +++ Catalyst-Authentication-Store-LDAP-0.1004.t0m/lib/Catalyst/Authentication/Store/LDAP.pm 2009-02-18 15:06:23.000000000 +0000 @@ -3,7 +3,7 @@ use strict; use warnings; -our $VERSION = '0.1004'; +our $VERSION = '0.1005'; use Catalyst::Authentication::Store::LDAP::Backend; diff -ur Catalyst-Authentication-Store-LDAP-0.1004/Makefile.PL Catalyst-Authentication-Store-LDAP-0.1004.t0m/Makefile.PL --- Catalyst-Authentication-Store-LDAP-0.1004/Makefile.PL 2008-10-22 02:45:33.000000000 +0100 +++ Catalyst-Authentication-Store-LDAP-0.1004.t0m/Makefile.PL 2009-02-18 15:04:36.000000000 +0000 @@ -13,6 +13,7 @@ build_requires('Net::LDAP::Server::Test' => '0.07'); build_requires('Test::More'); build_requires('Test::MockObject'); +build_required('Test::Exception'); auto_install(); diff -ur Catalyst-Authentication-Store-LDAP-0.1004/t/10-roles-mock.t Catalyst-Authentication-Store-LDAP-0.1004.t0m/t/10-roles-mock.t --- Catalyst-Authentication-Store-LDAP-0.1004/t/10-roles-mock.t 2008-10-22 02:44:50.000000000 +0100 +++ Catalyst-Authentication-Store-LDAP-0.1004.t0m/t/10-roles-mock.t 2009-02-18 15:22:20.000000000 +0000 @@ -4,8 +4,9 @@ use warnings; use Catalyst::Exception; -use Test::More tests => 7; +use Test::More tests => 11; use Test::MockObject::Extends; +use Test::Exception; use Net::LDAP::Entry; use lib 't/lib'; @@ -46,7 +47,8 @@ $ldap->mock('unbind' => sub {}); $ldap->mock('disconnect' => sub {}); my $search_res = Test::MockObject->new(); - $search_res->mock(is_error => sub {}); # Never an error + my $search_is_error = 0; + $search_res->mock(is_error => sub { $search_is_error }); $search_res->mock(entries => sub { return map { my $id = $_; @@ -73,12 +75,21 @@ is_deeply( [sort $user->roles], [sort qw/quuxone quuxtwo/], "User has the expected set of roles" ); + + $search_is_error = 1; + lives_ok { + ok !$back->find_user( { username => 'doesnotexist' } ), + 'Nonexistent user returns undef'; + } 'No exception thrown for nonexistent user'; + } is_deeply(\@searches, [ ['base', 'ou=foobar', 'filter', '(&(objectClass=inetOrgPerson)(uid=somebody))', 'scope', 'one'], ['base', 'ou=roles', 'filter', '(&(objectClass=posixGroup)(memberUid=test))', 'scope', 'one', 'attrs', [ 'userinrole' ]], + ['base', 'ou=foobar', 'filter', '(&(objectClass=inetOrgPerson)(uid=doesnotexist))', 'scope', 'one'], ['base', 'ou=foobar', 'filter', '(&(objectClass=inetOrgPerson)(uid=somebody))', 'scope', 'one'], ['base', 'ou=roles', 'filter', '(&(objectClass=posixGroup)(memberUid=test))', 'scope', 'one', 'attrs', [ 'userinrole' ]], + ['base', 'ou=foobar', 'filter', '(&(objectClass=inetOrgPerson)(uid=doesnotexist))', 'scope', 'one'], ], 'User searches as expected'); is_deeply(\@binds, [ [ undef ], # First user search @@ -90,12 +101,14 @@ [ undef ], # Rebind with initial credentials to find roles + [ undef ], # Second user search # 2nd pass round main loop [ undef ], # First user search [ 'ou=foobar', 'password', 'password' - ] # Rebind to confirm user _and_ lookup roles; + ], # Rebind to confirm user _and_ lookup roles; + [ undef ], # Second user search ], 'Binds as expected'); }