diff --git a/app/Auth/Access/LdapService.php b/app/Auth/Access/LdapService.php index c48a72f98..9ffbbfbb7 100644 --- a/app/Auth/Access/LdapService.php +++ b/app/Auth/Access/LdapService.php @@ -88,14 +88,32 @@ class LdapService return null; } + $userCn = $this->getUserResponseProperty($user, 'cn', null); return [ - 'uid' => (isset($user['uid'])) ? $user['uid'][0] : $user['dn'], - 'name' => (isset($user[$displayNameAttr])) ? (is_array($user[$displayNameAttr]) ? $user[$displayNameAttr][0] : $user[$displayNameAttr]) : $user['cn'][0], + 'uid' => $this->getUserResponseProperty($user, 'uid', $user['dn']), + 'name' => $this->getUserResponseProperty($user, $displayNameAttr, $userCn), 'dn' => $user['dn'], - 'email' => (isset($user[$emailAttr])) ? (is_array($user[$emailAttr]) ? $user[$emailAttr][0] : $user[$emailAttr]) : null + 'email' => $this->getUserResponseProperty($user, $emailAttr, null), ]; } + /** + * Get a property from an LDAP user response fetch. + * Handles properties potentially being part of an array. + * @param array $userDetails + * @param string $propertyKey + * @param $defaultValue + * @return mixed + */ + protected function getUserResponseProperty(array $userDetails, string $propertyKey, $defaultValue) + { + if (isset($userDetails[$propertyKey])) { + return (is_array($userDetails[$propertyKey]) ? $userDetails[$propertyKey][0] : $userDetails[$propertyKey]); + } + + return $defaultValue; + } + /** * @param Authenticatable $user * @param string $username diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 54cf84266..5ccb1415e 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -46,6 +46,15 @@ class LdapTest extends BrowserKitTest }); } + protected function mockUserLogin() + { + return $this->visit('/login') + ->see('Username') + ->type($this->mockUser->name, '#username') + ->type($this->mockUser->password, '#password') + ->press('Log In'); + } + public function test_login() { $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); @@ -61,11 +70,7 @@ class LdapTest extends BrowserKitTest $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true); $this->mockEscapes(4); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') + $this->mockUserLogin() ->seePageIs('/login')->see('Please enter an email to use for this account.'); $this->type($this->mockUser->email, '#email') @@ -91,11 +96,7 @@ class LdapTest extends BrowserKitTest $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true); $this->mockEscapes(2); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') + $this->mockUserLogin() ->seePageIs('/') ->see($this->mockUser->name) ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $ldapDn]); @@ -116,11 +117,7 @@ class LdapTest extends BrowserKitTest $this->mockLdap->shouldReceive('bind')->times(3)->andReturn(true, true, false); $this->mockEscapes(2); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') + $this->mockUserLogin() ->seePageIs('/login')->see('These credentials do not match our records.') ->dontSeeInDatabase('users', ['external_auth_id' => $this->mockUser->name]); } @@ -197,12 +194,7 @@ class LdapTest extends BrowserKitTest $this->mockEscapes(5); $this->mockExplodes(6); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') - ->seePageIs('/'); + $this->mockUserLogin()->seePageIs('/'); $user = User::where('email', $this->mockUser->email)->first(); $this->seeInDatabase('role_user', [ @@ -250,12 +242,7 @@ class LdapTest extends BrowserKitTest $this->mockEscapes(4); $this->mockExplodes(2); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') - ->seePageIs('/'); + $this->mockUserLogin()->seePageIs('/'); $user = User::where('email', $this->mockUser->email)->first(); $this->seeInDatabase('role_user', [ @@ -304,12 +291,7 @@ class LdapTest extends BrowserKitTest $this->mockEscapes(4); $this->mockExplodes(2); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') - ->seePageIs('/'); + $this->mockUserLogin()->seePageIs('/'); $user = User::where('email', $this->mockUser->email)->first(); $this->seeInDatabase('role_user', [ @@ -355,12 +337,7 @@ class LdapTest extends BrowserKitTest $this->mockEscapes(5); $this->mockExplodes(6); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') - ->seePageIs('/'); + $this->mockUserLogin()->seePageIs('/'); $user = User::where('email', $this->mockUser->email)->first(); $this->seeInDatabase('role_user', [ @@ -375,7 +352,6 @@ class LdapTest extends BrowserKitTest public function test_login_uses_specified_display_name_attribute() { - $originalAttribute = config('services.ldap.display_name_attribute'); app('config')->set([ 'services.ldap.display_name_attribute' => 'displayName' ]); @@ -394,11 +370,7 @@ class LdapTest extends BrowserKitTest $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true); $this->mockEscapes(4); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') + $this->mockUserLogin() ->seePageIs('/login')->see('Please enter an email to use for this account.'); $this->type($this->mockUser->email, '#email') @@ -406,15 +378,10 @@ class LdapTest extends BrowserKitTest ->seePageIs('/') ->see('displayNameAttribute') ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => 'displayNameAttribute']); - - app('config')->set([ - 'services.ldap.display_name_attribute' => $originalAttribute - ]); } public function test_login_uses_default_display_name_attribute_if_specified_not_present() { - $originalAttribute = config('services.ldap.display_name_attribute'); app('config')->set([ 'services.ldap.display_name_attribute' => 'displayName' ]); @@ -432,11 +399,7 @@ class LdapTest extends BrowserKitTest $this->mockLdap->shouldReceive('bind')->times(6)->andReturn(true); $this->mockEscapes(4); - $this->visit('/login') - ->see('Username') - ->type($this->mockUser->name, '#username') - ->type($this->mockUser->password, '#password') - ->press('Log In') + $this->mockUserLogin() ->seePageIs('/login')->see('Please enter an email to use for this account.'); $this->type($this->mockUser->email, '#email') @@ -444,10 +407,6 @@ class LdapTest extends BrowserKitTest ->seePageIs('/') ->see($this->mockUser->name) ->seeInDatabase('users', ['email' => $this->mockUser->email, 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, 'name' => $this->mockUser->name]); - - app('config')->set([ - 'services.ldap.display_name_attribute' => $originalAttribute - ]); } }