From 6f1b88a6a6402c7acfdd3e9bef72f50eb5e975c1 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 30 Dec 2019 15:46:12 +0000 Subject: [PATCH] Change email confirmation from own middle to trait Email confirmation middleware caused more mess than good, As caused priority issues and it depended on auth actions. Instead its now a trai used on auth middlewares. Also used 'EncryptCookies' middleware on API instead of custom decryption in custom middleware since we'd need to do replicate all the same actions anyway. Shouldn't have too much effect since it only actions over cookies that exist, of which none should be there for most API requests. Also split out some large guard functions to be a little more readable and appease codeclimate. --- app/Api/ApiTokenGuard.php | 36 ++++++++--- app/Http/Kernel.php | 23 +------ app/Http/Middleware/ApiAuthenticate.php | 10 +++- app/Http/Middleware/Authenticate.php | 8 ++- .../Middleware/ChecksForEmailConfirmation.php | 42 +++++++++++++ app/Http/Middleware/ConfirmEmails.php | 60 ------------------- .../Middleware/StartSessionIfCookieExists.php | 17 ------ 7 files changed, 86 insertions(+), 110 deletions(-) create mode 100644 app/Http/Middleware/ChecksForEmailConfirmation.php delete mode 100644 app/Http/Middleware/ConfirmEmails.php diff --git a/app/Api/ApiTokenGuard.php b/app/Api/ApiTokenGuard.php index b347e536a..cd9c3b178 100644 --- a/app/Api/ApiTokenGuard.php +++ b/app/Api/ApiTokenGuard.php @@ -33,8 +33,7 @@ class ApiTokenGuard implements Guard { $this->request = $request; } - - + /** * @inheritDoc */ @@ -84,6 +83,24 @@ class ApiTokenGuard implements Guard protected function getAuthorisedUserFromRequest(): Authenticatable { $authToken = trim($this->request->headers->get('Authorization', '')); + $this->validateTokenHeaderValue($authToken); + + [$id, $secret] = explode(':', str_replace('Token ', '', $authToken)); + $token = ApiToken::query() + ->where('token_id', '=', $id) + ->with(['user'])->first(); + + $this->validateToken($token, $secret); + + return $token->user; + } + + /** + * Validate the format of the token header value string. + * @throws ApiAuthException + */ + protected function validateTokenHeaderValue(string $authToken): void + { if (empty($authToken)) { throw new ApiAuthException(trans('errors.api_no_authorization_found')); } @@ -91,12 +108,15 @@ class ApiTokenGuard implements Guard if (strpos($authToken, ':') === false || strpos($authToken, 'Token ') !== 0) { throw new ApiAuthException(trans('errors.api_bad_authorization_format')); } + } - [$id, $secret] = explode(':', str_replace('Token ', '', $authToken)); - $token = ApiToken::query() - ->where('token_id', '=', $id) - ->with(['user'])->first(); - + /** + * Validate the given secret against the given token and ensure the token + * currently has access to the instance API. + * @throws ApiAuthException + */ + protected function validateToken(?ApiToken $token, string $secret): void + { if ($token === null) { throw new ApiAuthException(trans('errors.api_user_token_not_found')); } @@ -108,8 +128,6 @@ class ApiTokenGuard implements Guard if (!$token->user->can('access-api')) { throw new ApiAuthException(trans('errors.api_user_no_api_permission'), 403); } - - return $token->user; } /** diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 6a6e736b9..978583a7f 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -13,26 +13,6 @@ class Kernel extends HttpKernel \Illuminate\Foundation\Http\Middleware\ValidatePostSize::class, \BookStack\Http\Middleware\TrimStrings::class, \BookStack\Http\Middleware\TrustProxies::class, - - ]; - - /** - * The priority ordering of middleware. - */ - protected $middlewarePriority = [ - \BookStack\Http\Middleware\EncryptCookies::class, - \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class, - \Illuminate\Session\Middleware\StartSession::class, - \BookStack\Http\Middleware\StartSessionIfCookieExists::class, - \Illuminate\View\Middleware\ShareErrorsFromSession::class, - \Illuminate\Routing\Middleware\ThrottleRequests::class, - \BookStack\Http\Middleware\VerifyCsrfToken::class, - \Illuminate\Routing\Middleware\SubstituteBindings::class, - \BookStack\Http\Middleware\Localization::class, - \BookStack\Http\Middleware\GlobalViewData::class, - \BookStack\Http\Middleware\Authenticate::class, - \BookStack\Http\Middleware\ApiAuthenticate::class, - \BookStack\Http\Middleware\ConfirmEmails::class, ]; /** @@ -50,13 +30,12 @@ class Kernel extends HttpKernel \BookStack\Http\Middleware\VerifyCsrfToken::class, \BookStack\Http\Middleware\Localization::class, \BookStack\Http\Middleware\GlobalViewData::class, - \BookStack\Http\Middleware\ConfirmEmails::class, ], 'api' => [ 'throttle:60,1', + \BookStack\Http\Middleware\EncryptCookies::class, \BookStack\Http\Middleware\StartSessionIfCookieExists::class, \BookStack\Http\Middleware\ApiAuthenticate::class, - \BookStack\Http\Middleware\ConfirmEmails::class, ], ]; diff --git a/app/Http/Middleware/ApiAuthenticate.php b/app/Http/Middleware/ApiAuthenticate.php index 86fb83d58..c7fed405c 100644 --- a/app/Http/Middleware/ApiAuthenticate.php +++ b/app/Http/Middleware/ApiAuthenticate.php @@ -3,11 +3,12 @@ namespace BookStack\Http\Middleware; use BookStack\Exceptions\ApiAuthException; -use BookStack\Http\Request; use Closure; +use Illuminate\Http\Request; class ApiAuthenticate { + use ChecksForEmailConfirmation; /** * Handle an incoming request. @@ -17,6 +18,9 @@ class ApiAuthenticate // Return if the user is already found to be signed in via session-based auth. // This is to make it easy to browser the API via browser after just logging into the system. if (signedInUser()) { + if ($this->awaitingEmailConfirmation()) { + return $this->emailConfirmationErrorResponse($request); + } return $next($request); } @@ -30,6 +34,10 @@ class ApiAuthenticate return $this->unauthorisedResponse($exception->getMessage(), $exception->getCode()); } + if ($this->awaitingEmailConfirmation()) { + return $this->emailConfirmationErrorResponse($request); + } + return $next($request); } diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php index 40acc254b..a171a8a2d 100644 --- a/app/Http/Middleware/Authenticate.php +++ b/app/Http/Middleware/Authenticate.php @@ -2,16 +2,22 @@ namespace BookStack\Http\Middleware; -use BookStack\Http\Request; use Closure; +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 new file mode 100644 index 000000000..684a7e9bc --- /dev/null +++ b/app/Http/Middleware/ChecksForEmailConfirmation.php @@ -0,0 +1,42 @@ +check()) { + $requireConfirmation = (setting('registration-confirmation') || setting('registration-restrict')); + if ($requireConfirmation && !auth()->user()->email_confirmed) { + return true; + } + } + + return false; + } + + /** + * Provide an error response for when the current user's email is not confirmed + * in a system which requires it. + */ + protected function emailConfirmationErrorResponse(Request $request) + { + if ($request->wantsJson()) { + return response()->json([ + 'error' => [ + 'code' => 401, + 'message' => trans('errors.email_confirmation_awaiting') + ] + ], 401); + } + + return redirect('/register/confirm/awaiting'); + } +} \ No newline at end of file diff --git a/app/Http/Middleware/ConfirmEmails.php b/app/Http/Middleware/ConfirmEmails.php deleted file mode 100644 index 3700e9973..000000000 --- a/app/Http/Middleware/ConfirmEmails.php +++ /dev/null @@ -1,60 +0,0 @@ -auth = $auth; - } - - /** - * Handle an incoming request. - */ - public function handle(Request $request, Closure $next) - { - if ($this->auth->check()) { - $requireConfirmation = (setting('registration-confirmation') || setting('registration-restrict')); - if ($requireConfirmation && !$this->auth->user()->email_confirmed) { - return $this->errorResponse($request); - } - } - - return $next($request); - } - - /** - * Provide an error response for when the current user's email is not confirmed - * in a system which requires it. - */ - protected function errorResponse(Request $request) - { - if ($request->wantsJson()) { - return response()->json([ - 'error' => [ - 'code' => 401, - 'message' => trans('errors.email_confirmation_awaiting') - ] - ], 401); - } - - return redirect('/register/confirm/awaiting'); - } -} diff --git a/app/Http/Middleware/StartSessionIfCookieExists.php b/app/Http/Middleware/StartSessionIfCookieExists.php index 99553e294..456508d98 100644 --- a/app/Http/Middleware/StartSessionIfCookieExists.php +++ b/app/Http/Middleware/StartSessionIfCookieExists.php @@ -2,9 +2,7 @@ namespace BookStack\Http\Middleware; -use BookStack\Http\Request; use Closure; -use Exception; use Illuminate\Session\Middleware\StartSession as Middleware; class StartSessionIfCookieExists extends Middleware @@ -16,24 +14,9 @@ class StartSessionIfCookieExists extends Middleware { $sessionCookieName = config('session.cookie'); if ($request->cookies->has($sessionCookieName)) { - $this->decryptSessionCookie($request, $sessionCookieName); return parent::handle($request, $next); } return $next($request); } - - /** - * Attempt decryption of the session cookie. - */ - protected function decryptSessionCookie(Request $request, string $sessionCookieName) - { - try { - $sessionCookie = $request->cookies->get($sessionCookieName); - $sessionCookie = decrypt($sessionCookie, false); - $request->cookies->set($sessionCookieName, $sessionCookie); - } catch (Exception $e) { - // - } - } }