Prevented email confirmation exception throw on registration

Was preventing any other registration actions from taking place such as
LDAP/SAML group sync. Email confirmation should be actioned by
middleware on post-registration redirect.

Added testing to cover.
Tested for LDAP, SAML and normal registration with email confirmation
required to ensure flows work as expected.

Fixes #2082

diff --git a/app/Auth/Access/RegistrationService.php b/app/Auth/Access/RegistrationService.php
index 9136b37b5a86ea49f1f0a8ccaac27b1fd25eb27d..00ad630be23dd2cba644e679870d924c44dc721e 100644 (file)
--- a/app/Auth/Access/RegistrationService.php
+++ b/app/Auth/Access/RegistrationService.php
@@ -71,15 +71,14 @@ class RegistrationService
         // Start email confirmation flow if required
         if ($this->emailConfirmationService->confirmationRequired() && !$emailConfirmed) {
             $newUser->save();
-            $message = '';
 
             try {
                 $this->emailConfirmationService->sendConfirmation($newUser);
             } catch (Exception $e) {
                 $message = trans('auth.email_confirm_send_error');
+                throw new UserRegistrationException($message, '/register/confirm');
             }
 
-            throw new UserRegistrationException($message, '/register/confirm');
         }
 
         return $newUser;

diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php
index 8f9a24cdeacd142106c0a92411962bd59de877c5..89ddd0011ecb037c8831b4a79260a18030ee7abe 100644 (file)
--- a/app/Auth/Access/Saml2Service.php
+++ b/app/Auth/Access/Saml2Service.php
@@ -311,7 +311,6 @@ class Saml2Service extends ExternalAuthService
 
     /**
      * Get the user from the database for the specified details.
-     * @throws SamlException
      * @throws UserRegistrationException
      */
     protected function getOrRegisterUser(array $userDetails): ?User

diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php
index df3fd8d21e5053140a66e23298251a3a7725f3d1..02b33ecd68687a41f3139276049b0b6cc3db1f26 100644 (file)
--- a/tests/Auth/LdapTest.php
+++ b/tests/Auth/LdapTest.php
@@ -594,6 +594,48 @@ class LdapTest extends BrowserKitTest
         $this->see('A user with the email tester@example.com already exists but with different credentials');
     }
 
+    public function test_login_with_email_confirmation_required_maps_groups_but_shows_confirmation_screen()
+    {
+        $roleToReceive = factory(Role::class)->create(['display_name' => 'LdapTester']);
+        $user = factory(User::class)->make();
+        setting()->put('registration-confirmation', 'true');
+
+        app('config')->set([
+            'services.ldap.user_to_groups' => true,
+            'services.ldap.group_attribute' => 'memberOf',
+            'services.ldap.remove_from_groups' => true,
+        ]);
+
+        $this->commonLdapMocks(1, 1, 3, 4, 3, 2);
+        $this->mockLdap->shouldReceive('searchAndGetEntries')
+            ->times(3)
+            ->andReturn(['count' => 1, 0 => [
+                'uid' => [$user->name],
+                'cn' => [$user->name],
+                'dn' => ['dc=test' . config('services.ldap.base_dn')],
+                'mail' => [$user->email],
+                'memberof' => [
+                    'count' => 1,
+                    0 => "cn=ldaptester,ou=groups,dc=example,dc=com",
+                ]
+            ]]);
+
+        $this->mockUserLogin()->seePageIs('/register/confirm/awaiting');
+        $this->seeInDatabase('users', [
+            'email' => $user->email,
+            'email_confirmed' => false,
+        ]);
+
+        $user  = User::query()->where('email', '=', $user->email)->first();
+        $this->seeInDatabase('role_user', [
+            'user_id' => $user->id,
+            'role_id' => $roleToReceive->id
+        ]);
+
+        $homePage = $this->get('/');
+        $homePage->assertRedirectedTo('/register/confirm/awaiting');
+    }
+
     public function test_failed_logins_are_logged_when_message_configured()
     {
         $log = $this->withTestLogger();

diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php
index d0da4529735adcb7e2dfe0229c768cac4552aa2e..df0bb81c19a0602a4bcf64a7d49fd148a9932da2 100644 (file)
--- a/tests/Auth/Saml2Test.php
+++ b/tests/Auth/Saml2Test.php
@@ -290,6 +290,33 @@ class Saml2Test extends TestCase
         });
     }
 
+    public function test_group_sync_functions_when_email_confirmation_required()
+    {
+        setting()->put('registration-confirmation', 'true');
+        config()->set([
+            'saml2.onelogin.strict' => false,
+            'saml2.user_to_groups' => true,
+            'saml2.remove_from_groups' => false,
+        ]);
+
+        $memberRole = factory(Role::class)->create(['external_auth_id' => 'member']);
+        $adminRole = Role::getSystemRole('admin');
+
+        $this->withPost(['SAMLResponse' => $this->acsPostData], function () use ($memberRole, $adminRole) {
+            $acsPost = $this->followingRedirects()->post('/saml2/acs');
+            $acsPost->assertSee('Your email address has not yet been confirmed');
+            $user = User::query()->where('external_auth_id', '=', 'user')->first();
+
+            $userRoleIds = $user->roles()->pluck('id');
+            $this->assertContains($memberRole->id, $userRoleIds, 'User was assigned to member role');
+            $this->assertContains($adminRole->id, $userRoleIds, 'User was assigned to admin role');
+            $this->assertTrue($user->email_confirmed == false, 'User email remains unconfirmed');
+        });
+
+        $homeGet = $this->get('/');
+        $homeGet->assertRedirect('/register/confirm/awaiting');
+    }
+
     protected function withGet(array $options, callable $callback)
     {
         return $this->withGlobal($_GET, $options, $callback);