diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php index 3d67b1e77..86126c3b6 100644 --- a/app/Auth/Access/LoginService.php +++ b/app/Auth/Access/LoginService.php @@ -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); } } + } - return true; + /** + * 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'); + } + + $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 index 000000000..de8898df7 --- /dev/null +++ b/app/Exceptions/StoppedAuthenticationException.php @@ -0,0 +1,40 @@ +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 diff --git a/app/Http/Controllers/Auth/ConfirmEmailController.php b/app/Http/Controllers/Auth/ConfirmEmailController.php index c4280448e..520efdf88 100644 --- a/app/Http/Controllers/Auth/ConfirmEmailController.php +++ b/app/Http/Controllers/Auth/ConfirmEmailController.php @@ -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 index 000000000..f9bacb95d --- /dev/null +++ b/app/Http/Controllers/Auth/HandlesPartialLogins.php @@ -0,0 +1,22 @@ +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 diff --git a/app/Http/Controllers/Auth/MfaBackupCodesController.php b/app/Http/Controllers/Auth/MfaBackupCodesController.php index ba4b8f581..b89050565 100644 --- a/app/Http/Controllers/Auth/MfaBackupCodesController.php +++ b/app/Http/Controllers/Auth/MfaBackupCodesController.php @@ -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'); diff --git a/app/Http/Controllers/Auth/MfaController.php b/app/Http/Controllers/Auth/MfaController.php index 39a4e852f..6d868b6f3 100644 --- a/app/Http/Controllers/Auth/MfaController.php +++ b/app/Http/Controllers/Auth/MfaController.php @@ -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, ]); } } diff --git a/app/Http/Controllers/Auth/MfaTotpController.php b/app/Http/Controllers/Auth/MfaTotpController.php index 18f08e709..828af6b03 100644 --- a/app/Http/Controllers/Auth/MfaTotpController.php +++ b/app/Http/Controllers/Auth/MfaTotpController.php @@ -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'); diff --git a/app/Http/Controllers/Auth/SocialController.php b/app/Http/Controllers/Auth/SocialController.php index dd567fe18..2e9e62162 100644 --- a/app/Http/Controllers/Auth/SocialController.php +++ b/app/Http/Controllers/Auth/SocialController.php @@ -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('/'); } } diff --git a/resources/views/auth/user-unconfirmed.blade.php b/resources/views/auth/user-unconfirmed.blade.php index 85473685b..5f1edd2b9 100644 --- a/resources/views/auth/user-unconfirmed.blade.php +++ b/resources/views/auth/user-unconfirmed.blade.php @@ -17,8 +17,8 @@ {!! csrf_field() !!}
- @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 diff --git a/resources/views/mfa/verify.blade.php b/resources/views/mfa/verify.blade.php index 4ff0e6c49..58c0c42de 100644 --- a/resources/views/mfa/verify.blade.php +++ b/resources/views/mfa/verify.blade.php @@ -1,16 +1,28 @@ @extends('simple-layout') @section('body') -
+

Verify Access

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.

+ @if(!$method) +
+
No Methods Configured
+

+ 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. +

+
+ Configure +
+ @endif +
@@ -26,6 +38,13 @@
+ @if(count($otherMethods) > 0) +
+ @foreach($otherMethods as $otherMethod) + Use {{$otherMethod}} + @endforeach + @endif +
@stop diff --git a/routes/web.php b/routes/web.php index a73287762..437b4b03b 100644 --- a/routes/web.php +++ b/routes/web.php @@ -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');