Quick test of email confirmation routes and fix of tests

app/Auth/Access/EmailConfirmationService.php

@@ -14,9 +14,6 @@ class EmailConfirmationService extends UserTokenService
     /**
      * Create new confirmation for a user,
      * Also removes any existing old ones.
-     *
-     * @param User $user
-     *
      * @throws ConfirmationEmailException
      */
     public function sendConfirmation(User $user)
@@ -33,8 +30,6 @@ class EmailConfirmationService extends UserTokenService
 
     /**
      * Check if confirmation is required in this instance.
-     *
-     * @return bool
      */
     public function confirmationRequired(): bool
     {

app/Auth/Access/LoginService.php

@@ -10,7 +10,6 @@ use BookStack\Facades\Activity;
 use BookStack\Facades\Theme;
 use BookStack\Theming\ThemeEvents;
 use Exception;
-use phpDocumentor\Reflection\DocBlock\Tags\Method;
 
 class LoginService
 {
@@ -18,10 +17,12 @@ class LoginService
     protected const LAST_LOGIN_ATTEMPTED_SESSION_KEY = 'auth-login-last-attempted';
 
     protected $mfaSession;
+    protected $emailConfirmationService;
 
-    public function __construct(MfaSession $mfaSession)
+    public function __construct(MfaSession $mfaSession, EmailConfirmationService $emailConfirmationService)
     {
         $this->mfaSession = $mfaSession;
+        $this->emailConfirmationService = $emailConfirmationService;
     }
 
     /**
@@ -149,8 +150,7 @@ class LoginService
      */
     public function awaitingEmailConfirmation(User $user): bool
     {
-        $requireConfirmation = (setting('registration-confirmation') || setting('registration-restrict'));
+        return $this->emailConfirmationService->confirmationRequired() && !$user->email_confirmed;
-        return $requireConfirmation && !$user->email_confirmed;
     }
 
     /**

app/Auth/Access/RegistrationService.php

@@ -88,7 +88,6 @@ class RegistrationService
                 session()->flash('sent-email-confirmation', true);
             } catch (Exception $e) {
                 $message = trans('auth.email_confirm_send_error');
-
                 throw new UserRegistrationException($message, '/register/confirm');
             }
         }

app/Auth/Access/Saml2Service.php

@@ -6,6 +6,7 @@ use BookStack\Actions\ActivityType;
 use BookStack\Auth\User;
 use BookStack\Exceptions\JsonDebugException;
 use BookStack\Exceptions\SamlException;
+use BookStack\Exceptions\StoppedAuthenticationException;
 use BookStack\Exceptions\UserRegistrationException;
 use BookStack\Facades\Activity;
 use BookStack\Facades\Theme;
@@ -357,6 +358,7 @@ class Saml2Service extends ExternalAuthService
      * @throws SamlException
      * @throws JsonDebugException
      * @throws UserRegistrationException
+     * @throws StoppedAuthenticationException
      */
     public function processLoginCallback(string $samlID, array $samlAttributes): User
     {

app/Exceptions/StoppedAuthenticationException.php

@@ -5,6 +5,7 @@ namespace BookStack\Exceptions;
 use BookStack\Auth\Access\LoginService;
 use BookStack\Auth\User;
 use Illuminate\Contracts\Support\Responsable;
+use Illuminate\Http\Request;
 
 class StoppedAuthenticationException extends \Exception implements Responsable
 {
@@ -30,11 +31,35 @@ class StoppedAuthenticationException extends \Exception implements Responsable
         $redirect = '/login';
 
         if ($this->loginService->awaitingEmailConfirmation($this->user)) {
-            $redirect = '/register/confirm/awaiting';
+            return $this->awaitingEmailConfirmationResponse($request);
-        } else if  ($this->loginService->needsMfaVerification($this->user)) {
+        }
+
+        if ($this->loginService->needsMfaVerification($this->user)) {
             $redirect = '/mfa/verify';
         }
 
         return redirect($redirect);
     }
+
+    /**
+     * Provide an error response for when the current user's email is not confirmed
+     * in a system which requires it.
+     */
+    protected function awaitingEmailConfirmationResponse(Request $request)
+    {
+        if ($request->wantsJson()) {
+            return response()->json([
+                'error' => [
+                    'code' => 401,
+                    'message' => trans('errors.email_confirmation_awaiting'),
+                ],
+            ], 401);
+        }
+
+        if (session()->get('sent-email-confirmation') === true) {
+            return redirect('/register/confirm');
+        }
+
+        return redirect('/register/confirm/awaiting');
+    }
 }

app/Http/Controllers/Auth/RegisterController.php

@@ -6,6 +6,7 @@ use BookStack\Auth\Access\LoginService;
 use BookStack\Auth\Access\RegistrationService;
 use BookStack\Auth\Access\SocialAuthService;
 use BookStack\Auth\User;
+use BookStack\Exceptions\StoppedAuthenticationException;
 use BookStack\Exceptions\UserRegistrationException;
 use BookStack\Http\Controllers\Controller;
 use Illuminate\Foundation\Auth\RegistersUsers;
@@ -93,6 +94,7 @@ class RegisterController extends Controller
      * Handle a registration request for the application.
      *
      * @throws UserRegistrationException
+     * @throws StoppedAuthenticationException
      */
     public function postRegister(Request $request)
     {

app/Http/Middleware/Authenticate.php

@@ -21,26 +21,4 @@ class Authenticate
 
         return $next($request);
     }
-
-    /**
-     * Provide an error response for when the current user's email is not confirmed
-     * in a system which requires it.
-     */
-    protected function emailConfirmationErrorResponse(Request $request)
-    {
-        if ($request->wantsJson()) {
-            return response()->json([
-                'error' => [
-                    'code'    => 401,
-                    'message' => trans('errors.email_confirmation_awaiting'),
-                ],
-            ], 401);
-        }
-
-        if (session()->get('sent-email-confirmation') === true) {
-            return redirect('/register/confirm');
-        }
-
-        return redirect('/register/confirm/awaiting');
-    }
 }

tests/Auth/AuthTest.php

@@ -131,7 +131,8 @@ class AuthTest extends BrowserKitTest
             ->seePageIs('/register/confirm/awaiting')
             ->see('Resend')
             ->visit('/books')
-            ->seePageIs('/register/confirm/awaiting')
+            ->seePageIs('/login')
+            ->visit('/register/confirm/awaiting')
             ->press('Resend Confirmation Email');
 
         // Get confirmation and confirm notification matches
@@ -172,10 +173,7 @@ class AuthTest extends BrowserKitTest
             ->seePageIs('/register/confirm')
             ->seeInDatabase('users', ['name' => $user->name, 'email' => $user->email, 'email_confirmed' => false]);
 
-        $this->visit('/')
+        $this->assertNull(auth()->user());
-            ->seePageIs('/register/confirm/awaiting');
-
-        auth()->logout();
 
         $this->visit('/')->seePageIs('/login')
             ->type($user->email, '#email')
@@ -209,10 +207,8 @@ class AuthTest extends BrowserKitTest
             ->seePageIs('/register/confirm')
             ->seeInDatabase('users', ['name' => $user->name, 'email' => $user->email, 'email_confirmed' => false]);
 
-        $this->visit('/')
+        $this->assertNull(auth()->user());
-            ->seePageIs('/register/confirm/awaiting');
 
-        auth()->logout();
         $this->visit('/')->seePageIs('/login')
             ->type($user->email, '#email')
             ->type($user->password, '#password')

tests/Auth/LdapTest.php

@@ -651,9 +651,9 @@ class LdapTest extends TestCase
             'services.ldap.remove_from_groups' => true,
         ]);
 
-        $this->commonLdapMocks(1, 1, 3, 4, 3, 2);
+        $this->commonLdapMocks(1, 1, 6, 8, 6, 4);
         $this->mockLdap->shouldReceive('searchAndGetEntries')
-            ->times(3)
+            ->times(6)
             ->andReturn(['count' => 1, 0 => [
                 'uid' => [$user->name],
                 'cn' => [$user->name],
@@ -665,7 +665,8 @@ class LdapTest extends TestCase
                 ],
             ]]);
 
-        $this->followingRedirects()->mockUserLogin()->assertSee('Thanks for registering!');
+        $login = $this->followingRedirects()->mockUserLogin();
+        $login->assertSee('Thanks for registering!');
         $this->assertDatabaseHas('users', [
             'email' => $user->email,
             'email_confirmed' => false,
@@ -677,8 +678,13 @@ class LdapTest extends TestCase
             'role_id' => $roleToReceive->id,
         ]);
 
+        $this->assertNull(auth()->user());
+
         $homePage = $this->get('/');
-        $homePage->assertRedirect('/register/confirm/awaiting');
+        $homePage->assertRedirect('/login');
+
+        $login = $this->followingRedirects()->mockUserLogin();
+        $login->assertSee('Email Address Not Confirmed');
     }
 
     public function test_failed_logins_are_logged_when_message_configured()

tests/Auth/Saml2Test.php

@@ -289,16 +289,18 @@ class Saml2Test extends TestCase
 
             $this->assertEquals('http://localhost/register/confirm', url()->current());
             $acsPost->assertSee('Please check your email and click the confirmation button to access BookStack.');
+            /** @var User $user */
             $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');
+            $this->assertFalse(boolval($user->email_confirmed), 'User email remains unconfirmed');
         });
 
+        $this->assertNull(auth()->user());
         $homeGet = $this->get('/');
-        $homeGet->assertRedirect('/register/confirm/awaiting');
+        $homeGet->assertRedirect('/login');
     }
 
     public function test_login_where_existing_non_saml_user_shows_warning()