diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php index cc0cb06f3..f6ea7517f 100644 --- a/app/Auth/Access/LoginService.php +++ b/app/Auth/Access/LoginService.php @@ -10,6 +10,7 @@ use BookStack\Facades\Activity; use BookStack\Facades\Theme; use BookStack\Theming\ThemeEvents; use Exception; +use phpDocumentor\Reflection\DocBlock\Tags\Method; class LoginService { @@ -33,7 +34,7 @@ class LoginService public function login(User $user, string $method): void { if ($this->awaitingEmailConfirmation($user) || $this->needsMfaVerification($user)) { - $this->setLastLoginAttemptedForUser($user); + $this->setLastLoginAttemptedForUser($user, $method); throw new StoppedAuthenticationException($user, $this); // TODO - Does 'remember' still work? Probably not right now. @@ -41,12 +42,6 @@ class LoginService // 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) - // 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. @@ -70,13 +65,13 @@ class LoginService * Reattempt a system login after a previous stopped attempt. * @throws Exception */ - public function reattemptLoginFor(User $user, string $method) + public function reattemptLoginFor(User $user) { if ($user->id !== ($this->getLastLoginAttemptUser()->id ?? null)) { throw new Exception('Login reattempt user does align with current session state'); } - $this->login($user, $method); + $this->login($user, $this->getLastLoginAttemptMethod()); } /** @@ -86,12 +81,38 @@ class LoginService */ public function getLastLoginAttemptUser(): ?User { - $id = session()->get(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY); - if (!$id) { - return null; + $id = $this->getLastLoginAttemptDetails()['user_id']; + return User::query()->where('id', '=', $id)->first(); + } + + /** + * Get the method for the last login attempt. + */ + protected function getLastLoginAttemptMethod(): ?string + { + return $this->getLastLoginAttemptDetails()['method']; + } + + /** + * Get the details of the last login attempt. + * Checks upon a ttl of about 1 hour since that last attempted login. + * @return array{user_id: ?string, method: ?string} + */ + protected function getLastLoginAttemptDetails(): array + { + $value = session()->get(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY); + if (!$value) { + return ['user_id' => null, 'method' => null]; } - return User::query()->where('id', '=', $id)->first(); + [$id, $method, $time] = explode(':', $value); + $hourAgo = time() - (60*60); + if ($time < $hourAgo) { + $this->clearLastLoginAttempted(); + return ['user_id' => null, 'method' => null]; + } + + return ['user_id' => $id, 'method' => $method]; } /** @@ -99,9 +120,12 @@ class LoginService * 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) + protected function setLastLoginAttemptedForUser(User $user, string $method) { - session()->put(self::LAST_LOGIN_ATTEMPTED_SESSION_KEY, $user->id); + session()->put( + self::LAST_LOGIN_ATTEMPTED_SESSION_KEY, + implode(':', [$user->id, $method, time()]) + ); } /** diff --git a/app/Auth/Access/Mfa/MfaSession.php b/app/Auth/Access/Mfa/MfaSession.php index dabd568f7..8821dbb9d 100644 --- a/app/Auth/Access/Mfa/MfaSession.php +++ b/app/Auth/Access/Mfa/MfaSession.php @@ -15,6 +15,15 @@ class MfaSession return $user->mfaValues()->exists() || $this->userRoleEnforcesMfa($user); } + /** + * Check if the given user is pending MFA setup. + * (MFA required but not yet configured). + */ + public function isPendingMfaSetup(User $user): bool + { + return $this->isRequiredForUser($user) && !$user->mfaValues()->exists(); + } + /** * Check if a role of the given user enforces MFA. */ diff --git a/app/Http/Controllers/Auth/MfaBackupCodesController.php b/app/Http/Controllers/Auth/MfaBackupCodesController.php index 1353d4562..41c161d7c 100644 --- a/app/Http/Controllers/Auth/MfaBackupCodesController.php +++ b/app/Http/Controllers/Auth/MfaBackupCodesController.php @@ -78,7 +78,7 @@ class MfaBackupCodesController extends Controller MfaValue::upsertWithValue($user, MfaValue::METHOD_BACKUP_CODES, $updatedCodes); $mfaSession->markVerifiedForUser($user); - $loginService->reattemptLoginFor($user, 'mfa-backup_codes'); + $loginService->reattemptLoginFor($user); if ($codeService->countCodesInSet($updatedCodes) < 5) { $this->showWarningNotification('You have less than 5 backup codes remaining, Please generate and store a new set before you run out of codes to prevent being locked out of your account.'); diff --git a/app/Http/Controllers/Auth/MfaTotpController.php b/app/Http/Controllers/Auth/MfaTotpController.php index dd1651970..a1701c4ce 100644 --- a/app/Http/Controllers/Auth/MfaTotpController.php +++ b/app/Http/Controllers/Auth/MfaTotpController.php @@ -82,7 +82,7 @@ class MfaTotpController extends Controller ]); $mfaSession->markVerifiedForUser($user); - $loginService->reattemptLoginFor($user, 'mfa-totp'); + $loginService->reattemptLoginFor($user); return redirect()->intended(); } diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 4f9bfc1e6..d1f5de917 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -53,5 +53,6 @@ class Kernel extends HttpKernel 'throttle' => \Illuminate\Routing\Middleware\ThrottleRequests::class, 'perm' => \BookStack\Http\Middleware\PermissionMiddleware::class, 'guard' => \BookStack\Http\Middleware\CheckGuard::class, + 'mfa-setup' => \BookStack\Http\Middleware\AuthenticatedOrPendingMfa::class, ]; } diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php index c687c75a2..b9b5545f2 100644 --- a/app/Http/Middleware/Authenticate.php +++ b/app/Http/Middleware/Authenticate.php @@ -15,9 +15,8 @@ class Authenticate if (!hasAppAccess()) { if ($request->ajax()) { return response('Unauthorized.', 401); - } else { - return redirect()->guest(url('/login')); } + return redirect()->guest(url('/login')); } return $next($request); diff --git a/app/Http/Middleware/AuthenticatedOrPendingMfa.php b/app/Http/Middleware/AuthenticatedOrPendingMfa.php new file mode 100644 index 000000000..2d68a2a57 --- /dev/null +++ b/app/Http/Middleware/AuthenticatedOrPendingMfa.php @@ -0,0 +1,41 @@ +loginService = $loginService; + $this->mfaSession = $mfaSession; + } + + + /** + * Handle an incoming request. + * + * @param \Illuminate\Http\Request $request + * @param \Closure $next + * @return mixed + */ + public function handle($request, Closure $next) + { + $user = auth()->user(); + $loggedIn = $user !== null; + $lastAttemptUser = $this->loginService->getLastLoginAttemptUser(); + + if ($loggedIn || ($lastAttemptUser && $this->mfaSession->isPendingMfaSetup($lastAttemptUser))) { + return $next($request); + } + + return redirect()->guest(url('/login')); + } +} diff --git a/resources/views/mfa/backup-codes-generate.blade.php b/resources/views/mfa/backup-codes-generate.blade.php index 8b437846e..6a491fc07 100644 --- a/resources/views/mfa/backup-codes-generate.blade.php +++ b/resources/views/mfa/backup-codes-generate.blade.php @@ -27,7 +27,7 @@ Each code can only be used once
-