From 1278fb4969f87d2433a6e1e1f70d63f0e9a41d30 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 17 Jul 2021 18:24:50 +0100
Subject: [PATCH] Started moving MFA and email confirmation to new login flow
Instead of being soley middleware based.
---
app/Auth/Access/LoginService.php | 61 ++++++++++++++++++-
app/Auth/Access/Mfa/MfaSession.php | 34 +++++++----
.../Auth/ConfirmEmailController.php | 7 +--
.../Controllers/Auth/UserInviteController.php | 7 +--
app/Http/Kernel.php | 1 -
app/Http/Middleware/Authenticate.php | 6 --
.../Middleware/ChecksForEmailConfirmation.php | 36 -----------
.../Middleware/EnforceMfaRequirements.php | 48 ---------------
routes/web.php | 2 +-
9 files changed, 84 insertions(+), 118 deletions(-)
delete mode 100644 app/Http/Middleware/ChecksForEmailConfirmation.php
delete mode 100644 app/Http/Middleware/EnforceMfaRequirements.php
diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php
index 2bdef34a2..3d67b1e77 100644
--- a/app/Auth/Access/LoginService.php
+++ b/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;
diff --git a/app/Auth/Access/Mfa/MfaSession.php b/app/Auth/Access/Mfa/MfaSession.php
index 67574cbaf..dabd568f7 100644
--- a/app/Auth/Access/Mfa/MfaSession.php
+++ b/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;
}
}
\ No newline at end of file
diff --git a/app/Http/Controllers/Auth/ConfirmEmailController.php b/app/Http/Controllers/Auth/ConfirmEmailController.php
index 90e5fb76d..c4280448e 100644
--- a/app/Http/Controllers/Auth/ConfirmEmailController.php
+++ b/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('/');
}
diff --git a/app/Http/Controllers/Auth/UserInviteController.php b/app/Http/Controllers/Auth/UserInviteController.php
index 1cc59d6ba..bd1912b0b 100644
--- a/app/Http/Controllers/Auth/UserInviteController.php
+++ b/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('/');
}
diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php
index c9e59ed3e..4f9bfc1e6 100644
--- 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,
- 'mfa' => \BookStack\Http\Middleware\EnforceMfaRequirements::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 3b018cde0..c687c75a2 100644
--- a/app/Http/Middleware/Authenticate.php
+++ b/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);
diff --git a/app/Http/Middleware/ChecksForEmailConfirmation.php b/app/Http/Middleware/ChecksForEmailConfirmation.php
deleted file mode 100644
index 2eabeca16..000000000
--- 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
index 1877e5672..000000000
--- 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 406cfd767..a73287762 100644
--- 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::group(['middleware' => ['auth', 'mfa']], function () {
+Route::group(['middleware' => 'auth'], function () {
// Secure images routing
Route::get('/uploads/images/{path}', 'Images\ImageController@showImage')