]> BookStack Code Mirror - bookstack/commitdiff
Added login redirect system to confirm/mfa
authorDan Brown <redacted>
Sun, 18 Jul 2021 15:52:31 +0000 (16:52 +0100)
committerDan Brown <redacted>
Sun, 18 Jul 2021 15:52:31 +0000 (16:52 +0100)
Also continued a bit on the MFA verification system.
Moved some MFA routes to public space using updated login service to get
the current user that is either logged in or last attempted login (With
correct creds).

app/Auth/Access/LoginService.php
app/Exceptions/StoppedAuthenticationException.php [new file with mode: 0644]
app/Http/Controllers/Auth/ConfirmEmailController.php
app/Http/Controllers/Auth/HandlesPartialLogins.php [new file with mode: 0644]
app/Http/Controllers/Auth/MfaBackupCodesController.php
app/Http/Controllers/Auth/MfaController.php
app/Http/Controllers/Auth/MfaTotpController.php
app/Http/Controllers/Auth/SocialController.php
resources/views/auth/user-unconfirmed.blade.php
resources/views/mfa/verify.blade.php
routes/web.php

index 3d67b1e772c839acbf9332e839193576e9bb960e..86126c3b65dc54c77ca0021f0bf5c8ac5b92d5bc 100644 (file)
@@ -5,13 +5,17 @@ namespace BookStack\Auth\Access;
 use BookStack\Actions\ActivityType;
 use BookStack\Auth\Access\Mfa\MfaSession;
 use BookStack\Auth\User;
+use BookStack\Exceptions\StoppedAuthenticationException;
 use BookStack\Facades\Activity;
 use BookStack\Facades\Theme;
 use BookStack\Theming\ThemeEvents;
+use Exception;
 
 class LoginService
 {
 
+    protected const LAST_LOGIN_ATTEMPTED_SESSION_KEY = 'auth-login-last-attempted';
+
     protected $mfaSession;
 
     public function __construct(MfaSession $mfaSession)
@@ -19,24 +23,18 @@ class LoginService
         $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.
+     * @throws StoppedAuthenticationException
      */
-    public function login(User $user, string $method): bool
+    public function login(User $user, string $method): void
     {
         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.
-
+            $this->setLastLoginAttemptedForUser($user);
+            throw new StoppedAuthenticationException($user, $this);
             // TODO - Does 'remember' still work? Probably not right now.
 
             // Old MFA middleware todos:
@@ -44,15 +42,15 @@ class LoginService
             // 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?
+            //    Or, More likely, Need to add defence to those to prevent access unless
+            //    logged in or during partial auth.
 
             // TODO - Handle email confirmation handling
             //  Left BookStack\Http\Middleware\Authenticate@emailConfirmationErrorResponse in which needs
             //  be removed as an example of old behaviour.
-
-            return false;
         }
 
+        $this->clearLastLoginAttempted();
         auth()->login($user);
         Activity::add(ActivityType::AUTH_LOGIN, "{$method}; {$user->logDescriptor()}");
         Theme::dispatch(ThemeEvents::AUTH_LOGIN, $method, $user);
@@ -64,14 +62,58 @@ class LoginService
                 auth($guard)->login($user);
             }
         }
+    }
+
+    /**
+     * Reattempt a system login after a previous stopped attempt.
+     * @throws Exception
+     */
+    public function reattemptLoginFor(User $user, string $method)
+    {
+        if ($user->id !== $this->getLastLoginAttemptUser()) {
+            throw new Exception('Login reattempt user does align with current session state');
+        }
 
-        return true;
+        $this->login($user, $method);
+    }
+
+    /**
+     * Get the last user that was attempted to be logged in.
+     * Only exists if the last login attempt had correct credentials
+     * but had been prevented by a secondary factor.
+     */
+    public function getLastLoginAttemptUser(): ?User
+    {
+        $id = session()->get(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY);
+        if (!$id) {
+            return null;
+        }
+
+        return User::query()->where('id', '=', $id)->first();
+    }
+
+    /**
+     * Set the last login attempted user.
+     * Must be only used when credentials are correct and a login could be
+     * achieved but a secondary factor has stopped the login.
+     */
+    protected function setLastLoginAttemptedForUser(User $user)
+    {
+        session()->put(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY, $user->id);
+    }
+
+    /**
+     * Clear the last login attempted session value.
+     */
+    protected function clearLastLoginAttempted(): void
+    {
+        session()->remove(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY);
     }
 
     /**
      * Check if MFA verification is needed.
      */
-    protected function needsMfaVerification(User $user): bool
+    public function needsMfaVerification(User $user): bool
     {
         return !$this->mfaSession->isVerifiedForUser($user) && $this->mfaSession->isRequiredForUser($user);
     }
@@ -79,7 +121,7 @@ class LoginService
     /**
      * Check if the given user is awaiting email confirmation.
      */
-    protected function awaitingEmailConfirmation(User $user): bool
+    public function awaitingEmailConfirmation(User $user): bool
     {
         $requireConfirmation = (setting('registration-confirmation') || setting('registration-restrict'));
         return $requireConfirmation && !$user->email_confirmed;
@@ -89,6 +131,9 @@ class LoginService
      * Attempt the login of a user using the given credentials.
      * Meant to mirror Laravel's default guard 'attempt' method
      * but in a manner that always routes through our login system.
+     * May interrupt the flow if extra authentication requirements are imposed.
+     *
+     * @throws StoppedAuthenticationException
      */
     public function attempt(array $credentials, string $method, bool $remember = false): bool
     {
@@ -96,7 +141,7 @@ class LoginService
         if ($result) {
             $user = auth()->user();
             auth()->logout();
-            $result = $this->login($user, $method);
+            $this->login($user, $method);
         }
 
         return $result;
diff --git a/app/Exceptions/StoppedAuthenticationException.php b/app/Exceptions/StoppedAuthenticationException.php
new file mode 100644 (file)
index 0000000..de8898d
--- /dev/null
@@ -0,0 +1,40 @@
+<?php
+
+namespace BookStack\Exceptions;
+
+use BookStack\Auth\Access\LoginService;
+use BookStack\Auth\User;
+use Illuminate\Contracts\Support\Responsable;
+
+class StoppedAuthenticationException extends \Exception implements Responsable
+{
+
+    protected $user;
+    protected $loginService;
+
+    /**
+     * StoppedAuthenticationException constructor.
+     */
+    public function __construct(User $user, LoginService $loginService)
+    {
+        $this->user = $user;
+        $this->loginService = $loginService;
+        parent::__construct();
+    }
+
+    /**
+     * @inheritdoc
+     */
+    public function toResponse($request)
+    {
+        $redirect = '/login';
+
+        if ($this->loginService->awaitingEmailConfirmation($this->user)) {
+            $redirect = '/register/confirm/awaiting';
+        } else if  ($this->loginService->needsMfaVerification($this->user)) {
+            $redirect = '/mfa/verify';
+        }
+
+        return redirect($redirect);
+    }
+}
\ No newline at end of file
index c4280448e91c037f04a2e62d1df6a9d50708c7da..520efdf884cd970b869c800c59cbfa6e246d1808 100644 (file)
@@ -47,12 +47,11 @@ class ConfirmEmailController extends Controller
     /**
      * Shows a notice that a user's email address has not been confirmed,
      * Also has the option to re-send the confirmation email.
-     *
-     * @return View
      */
     public function showAwaiting()
     {
-        return view('auth.user-unconfirmed');
+        $user = $this->loginService->getLastLoginAttemptUser();
+        return view('auth.user-unconfirmed', ['user' => $user]);
     }
 
     /**
diff --git a/app/Http/Controllers/Auth/HandlesPartialLogins.php b/app/Http/Controllers/Auth/HandlesPartialLogins.php
new file mode 100644 (file)
index 0000000..f9bacb9
--- /dev/null
@@ -0,0 +1,22 @@
+<?php
+
+namespace BookStack\Http\Controllers\Auth;
+
+use BookStack\Auth\Access\LoginService;
+use BookStack\Auth\User;
+use BookStack\Exceptions\NotFoundException;
+
+trait HandlesPartialLogins
+{
+    protected function currentOrLastAttemptedUser(): User
+    {
+        $loginService = app()->make(LoginService::class);
+        $user = auth()->user() ?? $loginService->getLastLoginAttemptUser();
+
+        if (!$user) {
+            throw new NotFoundException('A user for this action could not be found');
+        }
+
+        return $user;
+    }
+}
\ No newline at end of file
index ba4b8f581a17d5769ad4a1abb51e79898b9f51f8..b8905056533212b6f9c15c77a4711a12a29b59dc 100644 (file)
@@ -10,6 +10,8 @@ use Exception;
 
 class MfaBackupCodesController extends Controller
 {
+    use HandlesPartialLogins;
+
     protected const SETUP_SECRET_SESSION_KEY = 'mfa-setup-backup-codes';
 
     /**
@@ -39,7 +41,7 @@ class MfaBackupCodesController extends Controller
         }
 
         $codes = decrypt(session()->pull(self::SETUP_SECRET_SESSION_KEY));
-        MfaValue::upsertWithValue(user(), MfaValue::METHOD_BACKUP_CODES, json_encode($codes));
+        MfaValue::upsertWithValue($this->currentOrLastAttemptedUser(), MfaValue::METHOD_BACKUP_CODES, json_encode($codes));
 
         $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'backup-codes');
         return redirect('/mfa/setup');
index 39a4e852f774dafcbbd2425620de155a9e616eda..6d868b6f3473400c7505c2675f5ab157ef3b2d21 100644 (file)
@@ -5,15 +5,19 @@ namespace BookStack\Http\Controllers\Auth;
 use BookStack\Actions\ActivityType;
 use BookStack\Auth\Access\Mfa\MfaValue;
 use BookStack\Http\Controllers\Controller;
+use BookStack\Http\Request;
 
 class MfaController extends Controller
 {
+    use HandlesPartialLogins;
+
     /**
      * Show the view to setup MFA for the current user.
      */
     public function setup()
     {
-        $userMethods = user()->mfaValues()
+        $userMethods = $this->currentOrLastAttemptedUser()
+            ->mfaValues()
             ->get(['id', 'method'])
             ->groupBy('method');
         return view('mfa.setup', [
@@ -41,14 +45,26 @@ class MfaController extends Controller
     /**
      * Show the page to start an MFA verification.
      */
-    public function verify()
+    public function verify(Request $request)
     {
-        $userMethods = user()->mfaValues()
+        // TODO - Test this
+        $desiredMethod = $request->get('method');
+        $userMethods = $this->currentOrLastAttemptedUser()
+            ->mfaValues()
             ->get(['id', 'method'])
             ->groupBy('method');
 
+        // Basic search for the default option for a user.
+        // (Prioritises totp over backup codes)
+        $method = $userMethods->has($desiredMethod) ? $desiredMethod : $userMethods->keys()->sort()->reverse()->first();
+        $otherMethods = $userMethods->keys()->filter(function($userMethod) use ($method) {
+            return $method !== $userMethod;
+        })->all();
+
         return view('mfa.verify', [
             'userMethods' => $userMethods,
+            'method' => $method,
+            'otherMethods' => $otherMethods,
         ]);
     }
 }
index 18f08e7093f65180a1d82bbd4708e24175d6a119..828af6b03281689772696c360cc08f5bfc0b4f2d 100644 (file)
@@ -6,12 +6,15 @@ use BookStack\Actions\ActivityType;
 use BookStack\Auth\Access\Mfa\MfaValue;
 use BookStack\Auth\Access\Mfa\TotpService;
 use BookStack\Auth\Access\Mfa\TotpValidationRule;
+use BookStack\Exceptions\NotFoundException;
 use BookStack\Http\Controllers\Controller;
 use Illuminate\Http\Request;
 use Illuminate\Validation\ValidationException;
 
 class MfaTotpController extends Controller
 {
+    use HandlesPartialLogins;
+
     protected const SETUP_SECRET_SESSION_KEY = 'mfa-setup-totp-secret';
 
     /**
@@ -39,6 +42,7 @@ class MfaTotpController extends Controller
      * Confirm the setup of TOTP and save the auth method secret
      * against the current user.
      * @throws ValidationException
+     * @throws NotFoundException
      */
     public function confirm(Request $request)
     {
@@ -51,7 +55,7 @@ class MfaTotpController extends Controller
             ]
         ]);
 
-        MfaValue::upsertWithValue(user(), MfaValue::METHOD_TOTP, $totpSecret);
+        MfaValue::upsertWithValue($this->currentOrLastAttemptedUser(), MfaValue::METHOD_TOTP, $totpSecret);
         session()->remove(static::SETUP_SECRET_SESSION_KEY);
         $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'totp');
 
index dd567fe18b13843cce2ee97f0c342191199458da..2e9e6216200f2e44710b5e8303870718ff112b7a 100644 (file)
@@ -140,9 +140,9 @@ class SocialController extends Controller
         }
 
         $user = $this->registrationService->registerUser($userData, $socialAccount, $emailVerified);
+        $this->showSuccessNotification(trans('auth.register_success'));
         $this->loginService->login($user, $socialDriver);
 
-        $this->showSuccessNotification(trans('auth.register_success'));
         return redirect('/');
     }
 }
index 85473685b96207e108d07d3093a2c23fa4f43bcd..5f1edd2b9f683f9b1d6e897ab656c0639e7b809b 100644 (file)
@@ -17,8 +17,8 @@
                 {!! csrf_field() !!}
                 <div class="form-group">
                     <label for="email">{{ trans('auth.email') }}</label>
-                    @if(auth()->check())
-                        @include('form.text', ['name' => 'email', 'model' => auth()->user()])
+                    @if($user)
+                        @include('form.text', ['name' => 'email', 'model' => $user])
                     @else
                         @include('form.text', ['name' => 'email'])
                     @endif
index 4ff0e6c49e954717eaebc97d24f9b1e4f3fc113c..58c0c42de2d07ab82e383fa5a3137332ccd7dc6a 100644 (file)
@@ -1,16 +1,28 @@
 @extends('simple-layout')
 
 @section('body')
-    <div class="container small py-xl">
+    <div class="container very-small py-xl">
 
         <div class="card content-wrap auto-height">
             <h1 class="list-heading">Verify Access</h1>
             <p class="mb-none">
                 Your user account requires you to confirm your identity via an additional level
                 of verification before you're granted access.
-                Verify using one of your configure methods to continue.
+                Verify using one of your configured methods to continue.
             </p>
 
+            @if(!$method)
+                <hr class="my-l">
+                <h5>No Methods Configured</h5>
+                <p class="small">
+                    No multi-factor authentication methods could be found for your account.
+                    You'll need to set up at least one method before you gain access.
+                </p>
+                <div>
+                    <a href="{{ url('/mfa/verify/totp') }}" class="button outline">Configure</a>
+                </div>
+            @endif
+
             <div class="setting-list">
                 <div class="grid half gap-xl">
                     <div>
 
             </div>
 
+            @if(count($otherMethods) > 0)
+                <hr class="my-l">
+                @foreach($otherMethods as $otherMethod)
+                    <a href="{{ url("/mfa/verify?method={$otherMethod}") }}">Use {{$otherMethod}}</a>
+                @endforeach
+            @endif
+
         </div>
     </div>
 @stop
index a732877628a1e51fd7f3adf892d52ff800b72d97..437b4b03bbe7f4b321624d19742d2213eae6d1a4 100644 (file)
@@ -224,16 +224,18 @@ Route::group(['middleware' => 'auth'], function () {
         Route::put('/roles/{id}', 'RoleController@update');
     });
 
-    // MFA Routes
-    Route::get('/mfa/setup', 'Auth\MfaController@setup');
-    Route::get('/mfa/totp-generate', 'Auth\MfaTotpController@generate');
-    Route::post('/mfa/totp-confirm', 'Auth\MfaTotpController@confirm');
-    Route::get('/mfa/backup-codes-generate', 'Auth\MfaBackupCodesController@generate');
-    Route::post('/mfa/backup-codes-confirm', 'Auth\MfaBackupCodesController@confirm');
+    // MFA (Auth Mandatory)
     Route::delete('/mfa/remove/{method}', 'Auth\MfaController@remove');
-    Route::get('/mfa/verify', 'Auth\MfaController@verify');
 });
 
+// MFA (Auth Optional)
+Route::get('/mfa/setup', 'Auth\MfaController@setup');
+Route::get('/mfa/totp-generate', 'Auth\MfaTotpController@generate');
+Route::post('/mfa/totp-confirm', 'Auth\MfaTotpController@confirm');
+Route::get('/mfa/backup-codes-generate', 'Auth\MfaBackupCodesController@generate');
+Route::post('/mfa/backup-codes-confirm', 'Auth\MfaBackupCodesController@confirm');
+Route::get('/mfa/verify', 'Auth\MfaController@verify');
+
 // Social auth routes
 Route::get('/login/service/{socialDriver}', 'Auth\SocialController@login');
 Route::get('/login/service/{socialDriver}/callback', 'Auth\SocialController@callback');