Started moving MFA and email confirmation to new login flow

Instead of being soley middleware based.

app/Auth/Access/LoginService.php

@@ -3,6 +3,7 @@
 namespace BookStack\Auth\Access;
 
 use BookStack\Actions\ActivityType;
+use BookStack\Auth\Access\Mfa\MfaSession;
 use BookStack\Auth\User;
 use BookStack\Facades\Activity;
 use BookStack\Facades\Theme;
@@ -11,11 +12,47 @@ use BookStack\Theming\ThemeEvents;
 class LoginService
 {
 
+    protected $mfaSession;
+
+    public function __construct(MfaSession $mfaSession)
+    {
+        $this->mfaSession = $mfaSession;
+    }
+
+
     /**
      * Log the given user into the system.
+     * Will start a login of the given user but will prevent if there's
+     * a reason to (MFA or Unconfirmed Email).
+     * Returns a boolean to indicate the current login result.
      */
-    public function login(User $user, string $method): void
+    public function login(User $user, string $method): bool
     {
+        if ($this->awaitingEmailConfirmation($user) || $this->needsMfaVerification($user)) {
+            // TODO - Remember who last attempted a login so we can use them after such
+            //  a email confirmation or mfa verification step.
+            //  Create a method to fetch that attempted user for use by the email confirmation
+            //  or MFA verification services.
+            //  Also will need a method to 'reattemptLastAttempted' login for once
+            //  the email confirmation of MFA verification steps have passed.
+            //  Must ensure this remembered last attempted login is cleared upon successful login.
+
+            // TODO - Does 'remember' still work? Probably not right now.
+
+            // Old MFA middleware todos:
+
+            // TODO - Need to redirect to setup if not configured AND ONLY IF NO OPTIONS CONFIGURED
+            //    Might need to change up such routes to start with /configure/ for such identification.
+            //    (Can't allow access to those if already configured)
+            // TODO - Store mfa_pass into session for future requests?
+
+            // TODO - Handle email confirmation handling
+            //  Left BookStack\Http\Middleware\Authenticate@emailConfirmationErrorResponse in which needs
+            //  be removed as an example of old behaviour.
+
+            return false;
+        }
+
         auth()->login($user);
         Activity::add(ActivityType::AUTH_LOGIN, "{$method}; {$user->logDescriptor()}");
         Theme::dispatch(ThemeEvents::AUTH_LOGIN, $method, $user);
@@ -27,12 +64,30 @@ class LoginService
                 auth($guard)->login($user);
             }
         }
+
+        return true;
     }
 
+    /**
+     * Check if MFA verification is needed.
+     */
+    protected function needsMfaVerification(User $user): bool
+    {
+        return !$this->mfaSession->isVerifiedForUser($user) && $this->mfaSession->isRequiredForUser($user);
+    }
+
+    /**
+     * Check if the given user is awaiting email confirmation.
+     */
+    protected function awaitingEmailConfirmation(User $user): bool
+    {
+        $requireConfirmation = (setting('registration-confirmation') || setting('registration-restrict'));
+        return $requireConfirmation && !$user->email_confirmed;
+    }
 
     /**
      * Attempt the login of a user using the given credentials.
-     * Meant to mirror laravel's default guard 'attempt' method
+     * Meant to mirror Laravel's default guard 'attempt' method
      * but in a manner that always routes through our login system.
      */
     public function attempt(array $credentials, string $method, bool $remember = false): bool
@@ -41,7 +96,7 @@ class LoginService
         if ($result) {
             $user = auth()->user();
             auth()->logout();
-            $this->login($user, $method);
+            $result = $this->login($user, $method);
         }
 
         return $result;

app/Auth/Access/Mfa/MfaSession.php

@@ -2,43 +2,51 @@
 
 namespace BookStack\Auth\Access\Mfa;
 
+use BookStack\Auth\User;
+
 class MfaSession
 {
-    private const MFA_VERIFIED_SESSION_KEY = 'mfa-verification-passed';
-
     /**
-     * Check if MFA is required for the current user.
+     * Check if MFA is required for the given user.
      */
-    public function requiredForCurrentUser(): bool
+    public function isRequiredForUser(User $user): bool
     {
         // TODO - Test both these cases
-        return user()->mfaValues()->exists() || $this->currentUserRoleEnforcesMfa();
+        return $user->mfaValues()->exists() || $this->userRoleEnforcesMfa($user);
     }
 
     /**
-     * Check if a role of the current user enforces MFA.
+     * Check if a role of the given user enforces MFA.
      */
-    protected function currentUserRoleEnforcesMfa(): bool
+    protected function userRoleEnforcesMfa(User $user): bool
     {
-        return user()->roles()
+        return $user->roles()
             ->where('mfa_enforced', '=', true)
             ->exists();
     }
 
     /**
-     * Check if the current MFA session has already been verified.
+     * Check if the current MFA session has already been verified for the given user.
      */
-    public function isVerified(): bool
+    public function isVerifiedForUser(User $user): bool
     {
-        return session()->get(self::MFA_VERIFIED_SESSION_KEY) === 'true';
+        return session()->get($this->getMfaVerifiedSessionKey($user)) === 'true';
     }
 
     /**
      * Mark the current session as MFA-verified.
      */
-    public function markVerified(): void
+    public function markVerifiedForUser(User $user): void
     {
-        session()->put(self::MFA_VERIFIED_SESSION_KEY, 'true');
+        session()->put($this->getMfaVerifiedSessionKey($user), 'true');
+    }
+
+    /**
+     * Get the session key in which the MFA verification status is stored.
+     */
+    protected function getMfaVerifiedSessionKey(User $user): string
+    {
+        return 'mfa-verification-passed:' . $user->id;
     }
 
 }

app/Http/Controllers/Auth/ConfirmEmailController.php

@@ -2,16 +2,13 @@
 
 namespace BookStack\Http\Controllers\Auth;
 
-use BookStack\Actions\ActivityType;
 use BookStack\Auth\Access\EmailConfirmationService;
 use BookStack\Auth\Access\LoginService;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\ConfirmationEmailException;
 use BookStack\Exceptions\UserTokenExpiredException;
 use BookStack\Exceptions\UserTokenNotFoundException;
-use BookStack\Facades\Theme;
 use BookStack\Http\Controllers\Controller;
-use BookStack\Theming\ThemeEvents;
 use Exception;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
@@ -94,9 +91,9 @@ class ConfirmEmailController extends Controller
         $user->email_confirmed = true;
         $user->save();
 
-        $this->loginService->login($user, auth()->getDefaultDriver());
-        $this->showSuccessNotification(trans('auth.email_confirm_success'));
         $this->emailConfirmationService->deleteByUser($user);
+        $this->showSuccessNotification(trans('auth.email_confirm_success'));
+        $this->loginService->login($user, auth()->getDefaultDriver());
 
         return redirect('/');
     }

app/Http/Controllers/Auth/UserInviteController.php

@@ -2,15 +2,12 @@
 
 namespace BookStack\Http\Controllers\Auth;
 
-use BookStack\Actions\ActivityType;
 use BookStack\Auth\Access\LoginService;
 use BookStack\Auth\Access\UserInviteService;
 use BookStack\Auth\UserRepo;
 use BookStack\Exceptions\UserTokenExpiredException;
 use BookStack\Exceptions\UserTokenNotFoundException;
-use BookStack\Facades\Theme;
 use BookStack\Http\Controllers\Controller;
-use BookStack\Theming\ThemeEvents;
 use Exception;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
@@ -75,9 +72,9 @@ class UserInviteController extends Controller
         $user->email_confirmed = true;
         $user->save();
 
-        $this->loginService->login($user, auth()->getDefaultDriver());
-        $this->showSuccessNotification(trans('auth.user_invite_success', ['appName' => setting('app-name')]));
         $this->inviteService->deleteByUser($user);
+        $this->showSuccessNotification(trans('auth.user_invite_success', ['appName' => setting('app-name')]));
+        $this->loginService->login($user, auth()->getDefaultDriver());
 
         return redirect('/');
     }

app/Http/Kernel.php

@@ -48,7 +48,6 @@ class Kernel extends HttpKernel
      */
     protected $routeMiddleware = [
         'auth'       => \BookStack\Http\Middleware\Authenticate::class,
-        'mfa'        => \BookStack\Http\Middleware\EnforceMfaRequirements::class,
         'can'        => \Illuminate\Auth\Middleware\Authorize::class,
         'guest'      => \BookStack\Http\Middleware\RedirectIfAuthenticated::class,
         'throttle'   => \Illuminate\Routing\Middleware\ThrottleRequests::class,

app/Http/Middleware/Authenticate.php

@@ -7,17 +7,11 @@ use Illuminate\Http\Request;
 
 class Authenticate
 {
-    use ChecksForEmailConfirmation;
-
     /**
      * Handle an incoming request.
      */
     public function handle(Request $request, Closure $next)
     {
-        if ($this->awaitingEmailConfirmation()) {
-            return $this->emailConfirmationErrorResponse($request);
-        }
-
         if (!hasAppAccess()) {
             if ($request->ajax()) {
                 return response('Unauthorized.', 401);

app/Http/Middleware/ChecksForEmailConfirmation.php

@@ -1,36 +0,0 @@
-<?php
-
-namespace BookStack\Http\Middleware;
-
-use BookStack\Exceptions\UnauthorizedException;
-
-trait ChecksForEmailConfirmation
-{
-    /**
-     * Check if the current user has a confirmed email if the instance deems it as required.
-     * Throws if confirmation is required by the user.
-     *
-     * @throws UnauthorizedException
-     */
-    protected function ensureEmailConfirmedIfRequested()
-    {
-        if ($this->awaitingEmailConfirmation()) {
-            throw new UnauthorizedException(trans('errors.email_confirmation_awaiting'));
-        }
-    }
-
-    /**
-     * Check if email confirmation is required and the current user is awaiting confirmation.
-     */
-    protected function awaitingEmailConfirmation(): bool
-    {
-        if (auth()->check()) {
-            $requireConfirmation = (setting('registration-confirmation') || setting('registration-restrict'));
-            if ($requireConfirmation && !auth()->user()->email_confirmed) {
-                return true;
-            }
-        }
-
-        return false;
-    }
-}

app/Http/Middleware/EnforceMfaRequirements.php

@@ -1,48 +0,0 @@
-<?php
-
-namespace BookStack\Http\Middleware;
-
-use BookStack\Auth\Access\Mfa\MfaSession;
-use Closure;
-
-class EnforceMfaRequirements
-{
-    protected $mfaSession;
-
-    /**
-     * EnforceMfaRequirements constructor.
-     */
-    public function __construct(MfaSession $mfaSession)
-    {
-        $this->mfaSession = $mfaSession;
-    }
-
-    /**
-     * Handle an incoming request.
-     *
-     * @param  \Illuminate\Http\Request  $request
-     * @param  \Closure  $next
-     * @return mixed
-     */
-    public function handle($request, Closure $next)
-    {
-        if (
-            !$this->mfaSession->isVerified()
-            && !$request->is('mfa/verify*', 'uploads/images/user/*')
-            && $this->mfaSession->requiredForCurrentUser()
-        ) {
-//            return redirect('/mfa/verify');
-        }
-
-        // TODO - URI wildcard exceptions above allow access to the 404 page of this user
-        //  which could then expose content. Either need to lock that down (Tricky to do image thing)
-        //  or prevent any level of auth until verified.
-
-        // TODO - Need to redirect to setup if not configured AND ONLY IF NO OPTIONS CONFIGURED
-        //    Might need to change up such routes to start with /configure/ for such identification.
-        //    (Can't allow access to those if already configured)
-        // TODO - Store mfa_pass into session for future requests?
-
-        return $next($request);
-    }
-}

routes/web.php

@@ -4,7 +4,7 @@ Route::get('/status', 'StatusController@show');
 Route::get('/robots.txt', 'HomeController@getRobots');
 
 // Authenticated routes...
-Route::group(['middleware' => ['auth', 'mfa']], function () {
+Route::group(['middleware' => 'auth'], function () {
 
     // Secure images routing
     Route::get('/uploads/images/{path}', 'Images\ImageController@showImage')