Started moving MFA and email confirmation to new login flow

Instead of being soley middleware based.

diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php
index 2bdef34a25388dd2c69dbd228e0e40734fca21be..3d67b1e772c839acbf9332e839193576e9bb960e 100644 (file)
--- a/app/Auth/Access/LoginService.php
+++ b/app/Auth/Access/LoginService.php
@@ -3,6 +3,7 @@
 namespace BookStack\Auth\Access;
 
 use BookStack\Actions\ActivityType;
 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;
 use BookStack\Auth\User;
 use BookStack\Facades\Activity;
 use BookStack\Facades\Theme;


@@ -11,11 +12,47 @@ use BookStack\Theming\ThemeEvents;
 class LoginService
 {
 
 class LoginService
 {
 

+    protected $mfaSession;
+
+    public function __construct(MfaSession $mfaSession)
+    {
+        $this->mfaSession = $mfaSession;
+    }
+
+

     /**
      * Log the given user into the system.
     /**
      * 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);
         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);
             }
         }
                 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.
 
     /**
      * 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
      * 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();
         if ($result) {
             $user = auth()->user();
             auth()->logout();

-            $this->login($user, $method);
+            $result = $this->login($user, $method);

         }
 
         return $result;
         }
 
         return $result;

diff --git a/app/Auth/Access/Mfa/MfaSession.php b/app/Auth/Access/Mfa/MfaSession.php
index 67574cbaf1607cecab0c9bf691b1f35b345b140e..dabd568f7e80b1941289f178c997b5909dae3bc4 100644 (file)
--- a/app/Auth/Access/Mfa/MfaSession.php
+++ b/app/Auth/Access/Mfa/MfaSession.php
@@ -2,43 +2,51 @@
 
 namespace BookStack\Auth\Access\Mfa;
 
 
 namespace BookStack\Auth\Access\Mfa;
 

+use BookStack\Auth\User;
+

 class MfaSession
 {
 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
     {
         // 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();
     }
 
     /**
             ->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.
      */
     }
 
     /**
      * Mark the current session as MFA-verified.
      */

-    public function markVerified(): void
+    public function markVerifiedForUser(User $user): void
+    {
+        session()->put($this->getMfaVerifiedSessionKey($user), 'true');
+    }
+
+    /**
+     * Get the session key in which the MFA verification status is stored.
+     */
+    protected function getMfaVerifiedSessionKey(User $user): string

     {
     {

-        session()->put(self::MFA_VERIFIED_SESSION_KEY, 'true');
+        return 'mfa-verification-passed:' . $user->id;

     }
 
 }
\ No newline at end of file
     }
 
 }
\ No newline at end of file

diff --git a/app/Http/Controllers/Auth/ConfirmEmailController.php b/app/Http/Controllers/Auth/ConfirmEmailController.php
index 90e5fb76dfb94aebd1282cf7c3d4aa141d948c12..c4280448e91c037f04a2e62d1df6a9d50708c7da 100644 (file)
--- a/app/Http/Controllers/Auth/ConfirmEmailController.php
+++ b/app/Http/Controllers/Auth/ConfirmEmailController.php
@@ -2,16 +2,13 @@
 
 namespace BookStack\Http\Controllers\Auth;
 
 
 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\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\Http\Controllers\Controller;

-use BookStack\Theming\ThemeEvents;

 use Exception;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Exception;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;


@@ -94,9 +91,9 @@ class ConfirmEmailController extends Controller
         $user->email_confirmed = true;
         $user->save();
 
         $user->email_confirmed = true;
         $user->save();
 

-        $this->loginService->login($user, auth()->getDefaultDriver());
-        $this->showSuccessNotification(trans('auth.email_confirm_success'));

         $this->emailConfirmationService->deleteByUser($user);
         $this->emailConfirmationService->deleteByUser($user);

+        $this->showSuccessNotification(trans('auth.email_confirm_success'));
+        $this->loginService->login($user, auth()->getDefaultDriver());

 
         return redirect('/');
     }
 
         return redirect('/');
     }

diff --git a/app/Http/Controllers/Auth/UserInviteController.php b/app/Http/Controllers/Auth/UserInviteController.php
index 1cc59d6ba0c04398c948c41d42a97b7be31f3195..bd1912b0b494313d410be80b0844ec15d6943ede 100644 (file)
--- a/app/Http/Controllers/Auth/UserInviteController.php
+++ b/app/Http/Controllers/Auth/UserInviteController.php
@@ -2,15 +2,12 @@
 
 namespace BookStack\Http\Controllers\Auth;
 
 
 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\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\Http\Controllers\Controller;

-use BookStack\Theming\ThemeEvents;

 use Exception;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;
 use Exception;
 use Illuminate\Http\RedirectResponse;
 use Illuminate\Http\Request;


@@ -75,9 +72,9 @@ class UserInviteController extends Controller
         $user->email_confirmed = true;
         $user->save();
 
         $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->inviteService->deleteByUser($user);

+        $this->showSuccessNotification(trans('auth.user_invite_success', ['appName' => setting('app-name')]));
+        $this->loginService->login($user, auth()->getDefaultDriver());

 
         return redirect('/');
     }
 
         return redirect('/');
     }

diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php
index c9e59ed3e30863b5bb5eb3826c6f7f45d92fd56d..4f9bfc1e64050dcd7a244c12b453db9ccc04461d 100644 (file)
--- a/app/Http/Kernel.php
+++ b/app/Http/Kernel.php
@@ -48,7 +48,6 @@ class Kernel extends HttpKernel
      */
     protected $routeMiddleware = [
         'auth'       => \BookStack\Http\Middleware\Authenticate::class,
      */
     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,
         'can'        => \Illuminate\Auth\Middleware\Authorize::class,
         'guest'      => \BookStack\Http\Middleware\RedirectIfAuthenticated::class,
         'throttle'   => \Illuminate\Routing\Middleware\ThrottleRequests::class,

diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php
index 3b018cde0f349f3b087507b7ea34864f48267f2a..c687c75a205ca925eaf88f88725498799d74c3bf 100644 (file)
--- a/app/Http/Middleware/Authenticate.php
+++ b/app/Http/Middleware/Authenticate.php
@@ -7,17 +7,11 @@ use Illuminate\Http\Request;
 
 class Authenticate
 {
 
 class Authenticate
 {

-    use ChecksForEmailConfirmation;
-

     /**
      * Handle an incoming request.
      */
     public function handle(Request $request, Closure $next)
     {
     /**
      * 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);
         if (!hasAppAccess()) {
             if ($request->ajax()) {
                 return response('Unauthorized.', 401);

diff --git a/app/Http/Middleware/ChecksForEmailConfirmation.php b/app/Http/Middleware/ChecksForEmailConfirmation.php
deleted file mode 100644 (file)
index 2eabeca..0000000
--- a/app/Http/Middleware/ChecksForEmailConfirmation.php
+++ /dev/null
@@ -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;
-    }
-}

diff --git a/app/Http/Middleware/EnforceMfaRequirements.php b/app/Http/Middleware/EnforceMfaRequirements.php
deleted file mode 100644 (file)
index 1877e56..0000000
--- a/app/Http/Middleware/EnforceMfaRequirements.php
+++ /dev/null
@@ -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);
-    }
-}

diff --git a/routes/web.php b/routes/web.php
index 406cfd767e23dabc02f35f713bc73884b0de6026..a732877628a1e51fd7f3adf892d52ff800b72d97 100644 (file)
--- a/routes/web.php
+++ b/routes/web.php
@@ -4,7 +4,7 @@ Route::get('/status', 'StatusController@show');
 Route::get('/robots.txt', 'HomeController@getRobots');
 
 // Authenticated routes...
 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')
 
     // Secure images routing
     Route::get('/uploads/images/{path}', 'Images\ImageController@showImage')