diff --git a/app/Auth/Access/Guards/LdapSessionGuard.php b/app/Auth/Access/Guards/LdapSessionGuard.php index 84f54ad29..652141c0c 100644 --- a/app/Auth/Access/Guards/LdapSessionGuard.php +++ b/app/Auth/Access/Guards/LdapSessionGuard.php @@ -60,10 +60,8 @@ class LdapSessionGuard extends ExternalBaseSessionGuard * @param array $credentials * @param bool $remember * @return bool - * @throws LoginAttemptEmailNeededException * @throws LoginAttemptException * @throws LdapException - * @throws UserRegistrationException */ public function attempt(array $credentials = [], $remember = false) { @@ -82,7 +80,11 @@ class LdapSessionGuard extends ExternalBaseSessionGuard } if (is_null($user)) { - $user = $this->createNewFromLdapAndCreds($userDetails, $credentials); + try { + $user = $this->createNewFromLdapAndCreds($userDetails, $credentials); + } catch (UserRegistrationException $exception) { + throw new LoginAttemptException($exception->message); + } } // Sync LDAP groups if required diff --git a/app/Exceptions/NotifyException.php b/app/Exceptions/NotifyException.php index 78ffde05c..4f8105960 100644 --- a/app/Exceptions/NotifyException.php +++ b/app/Exceptions/NotifyException.php @@ -8,8 +8,6 @@ class NotifyException extends \Exception /** * NotifyException constructor. - * @param string $message - * @param string $redirectLocation */ public function __construct(string $message, string $redirectLocation = "/") { diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index de86b9fb6..ed8748f08 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -63,11 +63,22 @@ class LdapTest extends BrowserKitTest ->press('Log In'); } + /** + * Set LDAP method mocks for things we commonly call without altering. + */ + protected function commonLdapMocks(int $connects = 1, int $versions = 1, int $options = 2, int $binds = 4, int $escapes = 2, int $explodes = 0) + { + $this->mockLdap->shouldReceive('connect')->times($connects)->andReturn($this->resourceId); + $this->mockLdap->shouldReceive('setVersion')->times($versions); + $this->mockLdap->shouldReceive('setOption')->times($options); + $this->mockLdap->shouldReceive('bind')->times($binds)->andReturn(true); + $this->mockEscapes($escapes); + $this->mockExplodes($explodes); + } + public function test_login() { - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(2); + $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -75,8 +86,6 @@ class LdapTest extends BrowserKitTest 'cn' => [$this->mockUser->name], 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); - $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true); - $this->mockEscapes(2); $this->mockUserLogin() ->seePageIs('/login')->see('Please enter an email to use for this account.'); @@ -94,9 +103,7 @@ class LdapTest extends BrowserKitTest 'registration-restrict' => 'testing.com' ]); - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(2); + $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -104,8 +111,6 @@ class LdapTest extends BrowserKitTest 'cn' => [$this->mockUser->name], 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); - $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true); - $this->mockEscapes(2); $this->mockUserLogin() ->seePageIs('/login') @@ -122,10 +127,9 @@ class LdapTest extends BrowserKitTest public function test_login_works_when_no_uid_provided_by_ldap_server() { - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn'); - $this->mockLdap->shouldReceive('setOption')->times(1); + + $this->commonLdapMocks(1, 1, 1, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -133,8 +137,6 @@ class LdapTest extends BrowserKitTest 'dn' => $ldapDn, 'mail' => [$this->mockUser->email] ]]); - $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true); - $this->mockEscapes(1); $this->mockUserLogin() ->seePageIs('/') @@ -145,10 +147,9 @@ class LdapTest extends BrowserKitTest public function test_a_custom_uid_attribute_can_be_specified_and_is_used_properly() { config()->set(['services.ldap.id_attribute' => 'my_custom_id']); - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); + + $this->commonLdapMocks(1, 1, 1, 2, 1); $ldapDn = 'cn=test-user,dc=test' . config('services.ldap.base_dn'); - $this->mockLdap->shouldReceive('setOption')->times(1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -159,9 +160,6 @@ class LdapTest extends BrowserKitTest ]]); - $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true); - $this->mockEscapes(1); - $this->mockUserLogin() ->seePageIs('/') ->see($this->mockUser->name) @@ -170,9 +168,7 @@ class LdapTest extends BrowserKitTest public function test_initial_incorrect_credentials() { - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(1); + $this->commonLdapMocks(1, 1, 1, 0, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -181,7 +177,6 @@ class LdapTest extends BrowserKitTest 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true, false); - $this->mockEscapes(1); $this->mockUserLogin() ->seePageIs('/login')->see('These credentials do not match our records.') @@ -190,14 +185,10 @@ class LdapTest extends BrowserKitTest public function test_login_not_found_username() { - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(1); + $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 0]); - $this->mockLdap->shouldReceive('bind')->times(1)->andReturn(true, false); - $this->mockEscapes(1); $this->mockUserLogin() ->seePageIs('/login')->see('These credentials do not match our records.') @@ -257,9 +248,8 @@ class LdapTest extends BrowserKitTest 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => false, ]); - $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->times(1); - $this->mockLdap->shouldReceive('setOption')->times(4); + + $this->commonLdapMocks(1, 1, 4, 5, 4, 6); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -273,9 +263,6 @@ class LdapTest extends BrowserKitTest 1 => "cn=ldaptester-second,ou=groups,dc=example,dc=com", ] ]]); - $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true); - $this->mockEscapes(4); - $this->mockExplodes(6); $this->mockUserLogin()->seePageIs('/'); @@ -306,9 +293,8 @@ class LdapTest extends BrowserKitTest 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => true, ]); - $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->times(1); - $this->mockLdap->shouldReceive('setOption')->times(3); + + $this->commonLdapMocks(1, 1, 3, 4, 3, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(3) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -321,9 +307,6 @@ class LdapTest extends BrowserKitTest 0 => "cn=ldaptester,ou=groups,dc=example,dc=com", ] ]]); - $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true); - $this->mockEscapes(3); - $this->mockExplodes(2); $this->mockUserLogin()->seePageIs('/'); @@ -355,9 +338,8 @@ class LdapTest extends BrowserKitTest 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => true, ]); - $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->times(1); - $this->mockLdap->shouldReceive('setOption')->times(3); + + $this->commonLdapMocks(1, 1, 3, 4, 3, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(3) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -370,9 +352,6 @@ class LdapTest extends BrowserKitTest 0 => "cn=ex-auth-a,ou=groups,dc=example,dc=com", ] ]]); - $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true); - $this->mockEscapes(3); - $this->mockExplodes(2); $this->mockUserLogin()->seePageIs('/'); @@ -400,9 +379,8 @@ class LdapTest extends BrowserKitTest 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => true, ]); - $this->mockLdap->shouldReceive('connect')->times(1)->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->times(1); - $this->mockLdap->shouldReceive('setOption')->times(4); + + $this->commonLdapMocks(1, 1, 4, 5, 4, 6); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -416,9 +394,6 @@ class LdapTest extends BrowserKitTest 1 => "cn=ldaptester-second,ou=groups,dc=example,dc=com", ] ]]); - $this->mockLdap->shouldReceive('bind')->times(5)->andReturn(true); - $this->mockEscapes(4); - $this->mockExplodes(6); $this->mockUserLogin()->seePageIs('/'); @@ -439,9 +414,7 @@ class LdapTest extends BrowserKitTest 'services.ldap.display_name_attribute' => 'displayName' ]); - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(2); + $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -450,8 +423,6 @@ class LdapTest extends BrowserKitTest 'dn' => ['dc=test' . config('services.ldap.base_dn')], 'displayname' => 'displayNameAttribute' ]]); - $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true); - $this->mockEscapes(2); $this->mockUserLogin() ->seePageIs('/login')->see('Please enter an email to use for this account.'); @@ -469,9 +440,7 @@ class LdapTest extends BrowserKitTest 'services.ldap.display_name_attribute' => 'displayName' ]); - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(2); + $this->commonLdapMocks(1, 1, 2, 4, 2); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -479,8 +448,6 @@ class LdapTest extends BrowserKitTest 'cn' => [$this->mockUser->name], 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); - $this->mockLdap->shouldReceive('bind')->times(4)->andReturn(true); - $this->mockEscapes(2); $this->mockUserLogin() ->seePageIs('/login')->see('Please enter an email to use for this account.'); @@ -499,15 +466,12 @@ class LdapTest extends BrowserKitTest ]); // Standard mocks - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(1); + $this->commonLdapMocks(0, 1, 1, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], 'cn' => [$this->mockUser->name], 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); - $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true); - $this->mockEscapes(1); $this->mockLdap->shouldReceive('connect')->once() ->with($expectedHost, $expectedPort)->andReturn($this->resourceId); @@ -567,9 +531,7 @@ class LdapTest extends BrowserKitTest { config()->set(['services.ldap.dump_user_details' => true]); - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(1); + $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ @@ -577,8 +539,6 @@ class LdapTest extends BrowserKitTest 'cn' => [$this->mockUser->name], 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); - $this->mockLdap->shouldReceive('bind')->times(1)->andReturn(true); - $this->mockEscapes(1); $this->post('/login', [ 'username' => $this->mockUser->name, @@ -594,10 +554,7 @@ class LdapTest extends BrowserKitTest { config()->set(['services.ldap.id_attribute' => 'BIN;uid']); $ldapService = app()->make(LdapService::class); - - $this->mockLdap->shouldReceive('connect')->once()->andReturn($this->resourceId); - $this->mockLdap->shouldReceive('setVersion')->once(); - $this->mockLdap->shouldReceive('setOption')->times(1); + $this->commonLdapMocks(1, 1, 1, 1, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), ['cn', 'dn', 'uid', 'mail', 'cn']) ->andReturn(['count' => 1, 0 => [ @@ -605,10 +562,35 @@ class LdapTest extends BrowserKitTest 'cn' => [$this->mockUser->name], 'dn' => ['dc=test' . config('services.ldap.base_dn')] ]]); - $this->mockLdap->shouldReceive('bind')->times(1)->andReturn(true); - $this->mockEscapes(1); $details = $ldapService->getUserDetails('test'); $this->assertEquals('fff8f7', $details['uid']); } + + public function test_new_ldap_user_login_with_already_used_email_address_shows_error_message_to_user() + { + $this->commonLdapMocks(1, 1, 2, 4, 2); + $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) + ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) + ->andReturn(['count' => 1, 0 => [ + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'mail' => 'tester@example.com', + ]], ['count' => 1, 0 => [ + 'uid' => ['Barry'], + 'cn' => ['Scott'], + 'dn' => ['dc=bscott' . config('services.ldap.base_dn')], + 'mail' => 'tester@example.com', + ]]); + + // First user login + $this->mockUserLogin()->seePageIs('/'); + + // Second user login + auth()->logout(); + $this->post('/login', ['username' => 'bscott', 'password' => 'pass'])->followRedirects(); + + $this->see('A user with the email tester@example.com already exists but with different credentials'); + } }