From efb6a6b457ac8e20bbbb39d8a730921850c2751a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 28 Jun 2021 22:02:45 +0100 Subject: [PATCH 01/25] Started barebones work of MFA system --- app/Http/Controllers/Auth/MfaController.php | 29 +++ composer.json | 2 + composer.lock | 221 +++++++++++++++++++- resources/views/mfa/setup.blade.php | 16 ++ routes/web.php | 2 + 5 files changed, 269 insertions(+), 1 deletion(-) create mode 100644 app/Http/Controllers/Auth/MfaController.php create mode 100644 resources/views/mfa/setup.blade.php diff --git a/app/Http/Controllers/Auth/MfaController.php b/app/Http/Controllers/Auth/MfaController.php new file mode 100644 index 000000000..1d0dbd1a4 --- /dev/null +++ b/app/Http/Controllers/Auth/MfaController.php @@ -0,0 +1,29 @@ + + +
+

Setup Multi-Factor Authentication

+

+ Setup multi-factor authentication as an extra layer of security + for your user account. + To use multi-factor authentication you'll need a mobile application + that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator. +

+
+ +@stop diff --git a/routes/web.php b/routes/web.php index bc9705e10..7807d5477 100644 --- a/routes/web.php +++ b/routes/web.php @@ -223,6 +223,8 @@ Route::group(['middleware' => 'auth'], function () { Route::get('/roles/{id}', 'RoleController@edit'); Route::put('/roles/{id}', 'RoleController@update'); }); + + Route::get('/mfa/setup', 'Auth\MfaController@setup'); }); // Social auth routes From d25cd83d8e2a8741a6476ce2d7ff6efc728ecc6e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Tue, 29 Jun 2021 22:06:49 +0100 Subject: [PATCH 02/25] Added TOTP generation view and started verification stage Also updated MFA setup view to have settings-like listed interface to make it possible to extend with extra options in the future. --- app/Http/Controllers/Auth/MfaController.php | 65 +++++++++++++++++++-- resources/sass/_forms.scss | 4 ++ resources/views/mfa/setup.blade.php | 33 ++++++++++- resources/views/mfa/totp-generate.blade.php | 44 ++++++++++++++ routes/web.php | 2 + 5 files changed, 141 insertions(+), 7 deletions(-) create mode 100644 resources/views/mfa/totp-generate.blade.php diff --git a/app/Http/Controllers/Auth/MfaController.php b/app/Http/Controllers/Auth/MfaController.php index 1d0dbd1a4..be381e3b0 100644 --- a/app/Http/Controllers/Auth/MfaController.php +++ b/app/Http/Controllers/Auth/MfaController.php @@ -2,11 +2,24 @@ namespace BookStack\Http\Controllers\Auth; +use BaconQrCode\Renderer\Color\Rgb; +use BaconQrCode\Renderer\Image\SvgImageBackEnd; +use BaconQrCode\Renderer\ImageRenderer; +use BaconQrCode\Renderer\RendererStyle\Fill; +use BaconQrCode\Renderer\RendererStyle\RendererStyle; +use BaconQrCode\Writer; use BookStack\Http\Controllers\Controller; use Illuminate\Http\Request; +use Illuminate\Validation\ValidationException; +use PragmaRX\Google2FA\Exceptions\IncompatibleWithGoogleAuthenticatorException; +use PragmaRX\Google2FA\Exceptions\InvalidCharactersException; +use PragmaRX\Google2FA\Exceptions\SecretKeyTooShortException; +use PragmaRX\Google2FA\Google2FA; class MfaController extends Controller { + protected const TOTP_SETUP_SECRET_SESSION_KEY = 'mfa-setup-totp-secret'; + /** * Show the view to setup MFA for the current user. */ @@ -17,13 +30,57 @@ class MfaController extends Controller return view('mfa.setup'); } - public function generateQr() + /** + * Show a view that generates and displays a TOTP QR code. + * @throws IncompatibleWithGoogleAuthenticatorException + * @throws InvalidCharactersException + * @throws SecretKeyTooShortException + */ + public function totpGenerate() { - // https://github.com/antonioribeiro/google2fa#how-to-generate-and-use-two-factor-authentication + // TODO - Ensure a QR code doesn't already exist? Or overwrite? + $google2fa = new Google2FA(); + if (session()->has(static::TOTP_SETUP_SECRET_SESSION_KEY)) { + $totpSecret = decrypt(session()->get(static::TOTP_SETUP_SECRET_SESSION_KEY)); + } else { + $totpSecret = $google2fa->generateSecretKey(); + session()->put(static::TOTP_SETUP_SECRET_SESSION_KEY, encrypt($totpSecret)); + } + + $qrCodeUrl = $google2fa->getQRCodeUrl( + setting('app-name'), + user()->email, + $totpSecret + ); + + $color = Fill::uniformColor(new Rgb(255, 255, 255), new Rgb(32, 110, 167)); + $svg = (new Writer( + new ImageRenderer( + new RendererStyle(192, 0, null, null, $color), + new SvgImageBackEnd + ) + ))->writeString($qrCodeUrl); - // Generate secret key - // Store key in session? // Get user to verify setup via responding once. // If correct response, Save key against user + return view('mfa.totp-generate', [ + 'secret' => $totpSecret, + 'svg' => $svg, + ]); + } + + /** + * Confirm the setup of TOTP and save the auth method secret + * against the current user. + * @throws ValidationException + */ + public function totpConfirm(Request $request) + { + $this->validate($request, [ + 'code' => 'required|max:12|min:4' + ]); + + // TODO - Confirm code + dd($request->input('code')); } } diff --git a/resources/sass/_forms.scss b/resources/sass/_forms.scss index 953d1d060..bb6d17f82 100644 --- a/resources/sass/_forms.scss +++ b/resources/sass/_forms.scss @@ -29,6 +29,10 @@ } } +.input-fill-width { + width: 100% !important; +} + .fake-input { @extend .input-base; overflow: auto; diff --git a/resources/views/mfa/setup.blade.php b/resources/views/mfa/setup.blade.php index 25eb5d925..577841af5 100644 --- a/resources/views/mfa/setup.blade.php +++ b/resources/views/mfa/setup.blade.php @@ -5,12 +5,39 @@

Setup Multi-Factor Authentication

-

+

Setup multi-factor authentication as an extra layer of security for your user account. - To use multi-factor authentication you'll need a mobile application - that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator.

+ +
+
+
+
Mobile App
+

+ To use multi-factor authentication you'll need a mobile application + that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator. +

+
+
+ Setup +
+
+ +
+
+
Backup Codes
+

+ Print out or securely store a set of one-time backup codes + which you can enter to verify your identity. +

+
+
+ Setup +
+
+
+
@stop diff --git a/resources/views/mfa/totp-generate.blade.php b/resources/views/mfa/totp-generate.blade.php new file mode 100644 index 000000000..17d38adaa --- /dev/null +++ b/resources/views/mfa/totp-generate.blade.php @@ -0,0 +1,44 @@ +@extends('simple-layout') + +@section('body') + +
+
+

Mobile App Setup

+

+ To use multi-factor authentication you'll need a mobile application + that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator. +

+

+ Scan the QR code below using your preferred authentication app to get started. +

+ +
+
+ {!! $svg !!} +
+
+ +

Verify Setup

+

+ Verify that all is working by entering a code, generated within your + authentication app, in the input box below: +

+
+ {{ csrf_field() }} + + @if($errors->has('code')) +
{{ $errors->first('code') }}
+ @endif +
+ +
+
+
+
+ +@stop diff --git a/routes/web.php b/routes/web.php index 7807d5477..f9967465b 100644 --- a/routes/web.php +++ b/routes/web.php @@ -225,6 +225,8 @@ Route::group(['middleware' => 'auth'], function () { }); Route::get('/mfa/setup', 'Auth\MfaController@setup'); + Route::get('/mfa/totp-generate', 'Auth\MfaController@totpGenerate'); + Route::post('/mfa/totp-confirm', 'Auth\MfaController@totpConfirm'); }); // Social auth routes From 916a82616f1e2c750a1f01109e65ad2b603a79ce Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 30 Jun 2021 22:10:02 +0100 Subject: [PATCH 03/25] Complete base flow for TOTP setup - Includes DB storage and code validation. - Extracted TOTP work to its own service file. - Still needs testing to cover this side of things. --- app/Actions/ActivityType.php | 2 + app/Auth/Access/Mfa/MfaValue.php | 54 +++++++++++++++ app/Auth/Access/Mfa/TotpService.php | 66 +++++++++++++++++++ app/Auth/Access/Mfa/TotpValidationRule.php | 38 +++++++++++ app/Auth/User.php | 9 +++ app/Auth/UserRepo.php | 1 + app/Http/Controllers/Auth/MfaController.php | 62 +++++++---------- ...1_06_30_173111_create_mfa_values_table.php | 34 ++++++++++ resources/lang/en/activities.php | 3 + resources/lang/en/validation.php | 1 + resources/views/mfa/setup.blade.php | 10 ++- tests/Auth/MfaTotpTest.php | 10 +++ 12 files changed, 251 insertions(+), 39 deletions(-) create mode 100644 app/Auth/Access/Mfa/MfaValue.php create mode 100644 app/Auth/Access/Mfa/TotpService.php create mode 100644 app/Auth/Access/Mfa/TotpValidationRule.php create mode 100644 database/migrations/2021_06_30_173111_create_mfa_values_table.php create mode 100644 tests/Auth/MfaTotpTest.php diff --git a/app/Actions/ActivityType.php b/app/Actions/ActivityType.php index a19f143b7..e2e7b5a5d 100644 --- a/app/Actions/ActivityType.php +++ b/app/Actions/ActivityType.php @@ -50,4 +50,6 @@ class ActivityType const AUTH_PASSWORD_RESET_UPDATE = 'auth_password_reset_update'; const AUTH_LOGIN = 'auth_login'; const AUTH_REGISTER = 'auth_register'; + + const MFA_SETUP_METHOD = 'mfa_setup_method'; } diff --git a/app/Auth/Access/Mfa/MfaValue.php b/app/Auth/Access/Mfa/MfaValue.php new file mode 100644 index 000000000..6e9049c3c --- /dev/null +++ b/app/Auth/Access/Mfa/MfaValue.php @@ -0,0 +1,54 @@ +firstOrNew([ + 'user_id' => $user->id, + 'method' => $method + ]); + $mfaVal->setValue($value); + $mfaVal->save(); + } + + /** + * Decrypt the value attribute upon access. + */ + public function getValue(): string + { + return decrypt($this->value); + } + + /** + * Encrypt the value attribute upon access. + */ + public function setValue($value): void + { + $this->value = encrypt($value); + } +} diff --git a/app/Auth/Access/Mfa/TotpService.php b/app/Auth/Access/Mfa/TotpService.php new file mode 100644 index 000000000..f9a9f416e --- /dev/null +++ b/app/Auth/Access/Mfa/TotpService.php @@ -0,0 +1,66 @@ +google2fa = $google2fa; + } + + /** + * Generate a new totp secret key. + */ + public function generateSecret(): string + { + /** @noinspection PhpUnhandledExceptionInspection */ + return $this->google2fa->generateSecretKey(); + } + + /** + * Generate a TOTP URL from secret key. + */ + public function generateUrl(string $secret): string + { + return $this->google2fa->getQRCodeUrl( + setting('app-name'), + user()->email, + $secret + ); + } + + /** + * Generate a QR code to display a TOTP URL. + */ + public function generateQrCodeSvg(string $url): string + { + $color = Fill::uniformColor(new Rgb(255, 255, 255), new Rgb(32, 110, 167)); + return (new Writer( + new ImageRenderer( + new RendererStyle(192, 0, null, null, $color), + new SvgImageBackEnd + ) + ))->writeString($url); + } + + /** + * Verify that the user provided code is valid for the secret. + * The secret must be known, not user-provided. + */ + public function verifyCode(string $code, string $secret): bool + { + /** @noinspection PhpUnhandledExceptionInspection */ + return $this->google2fa->verifyKey($secret, $code); + } +} \ No newline at end of file diff --git a/app/Auth/Access/Mfa/TotpValidationRule.php b/app/Auth/Access/Mfa/TotpValidationRule.php new file mode 100644 index 000000000..7fe3d8504 --- /dev/null +++ b/app/Auth/Access/Mfa/TotpValidationRule.php @@ -0,0 +1,38 @@ +secret = $secret; + $this->totpService = app()->make(TotpService::class); + } + + /** + * Determine if the validation rule passes. + */ + public function passes($attribute, $value) + { + return $this->totpService->verifyCode($value, $this->secret); + } + + /** + * Get the validation error message. + */ + public function message() + { + return trans('validation.totp'); + } +} diff --git a/app/Auth/User.php b/app/Auth/User.php index 1a3560691..f4fd45281 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -4,6 +4,7 @@ namespace BookStack\Auth; use BookStack\Actions\Favourite; use BookStack\Api\ApiToken; +use BookStack\Auth\Access\Mfa\MfaValue; use BookStack\Entities\Tools\SlugGenerator; use BookStack\Interfaces\Loggable; use BookStack\Interfaces\Sluggable; @@ -265,6 +266,14 @@ class User extends Model implements AuthenticatableContract, CanResetPasswordCon return $this->hasMany(Favourite::class); } + /** + * Get the MFA values belonging to this use. + */ + public function mfaValues(): HasMany + { + return $this->hasMany(MfaValue::class); + } + /** * Get the last activity time for this user. */ diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 61ca12dcc..9faeb8ae2 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -188,6 +188,7 @@ class UserRepo $user->socialAccounts()->delete(); $user->apiTokens()->delete(); $user->favourites()->delete(); + $user->mfaValues()->delete(); $user->delete(); // Delete user profile images diff --git a/app/Http/Controllers/Auth/MfaController.php b/app/Http/Controllers/Auth/MfaController.php index be381e3b0..8ddccaa98 100644 --- a/app/Http/Controllers/Auth/MfaController.php +++ b/app/Http/Controllers/Auth/MfaController.php @@ -2,19 +2,13 @@ namespace BookStack\Http\Controllers\Auth; -use BaconQrCode\Renderer\Color\Rgb; -use BaconQrCode\Renderer\Image\SvgImageBackEnd; -use BaconQrCode\Renderer\ImageRenderer; -use BaconQrCode\Renderer\RendererStyle\Fill; -use BaconQrCode\Renderer\RendererStyle\RendererStyle; -use BaconQrCode\Writer; +use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\Mfa\MfaValue; +use BookStack\Auth\Access\Mfa\TotpService; +use BookStack\Auth\Access\Mfa\TotpValidationRule; use BookStack\Http\Controllers\Controller; use Illuminate\Http\Request; use Illuminate\Validation\ValidationException; -use PragmaRX\Google2FA\Exceptions\IncompatibleWithGoogleAuthenticatorException; -use PragmaRX\Google2FA\Exceptions\InvalidCharactersException; -use PragmaRX\Google2FA\Exceptions\SecretKeyTooShortException; -use PragmaRX\Google2FA\Google2FA; class MfaController extends Controller { @@ -25,44 +19,29 @@ class MfaController extends Controller */ public function setup() { - // TODO - Redirect back to profile/edit if already setup? - // Show MFA setup route - return view('mfa.setup'); + $userMethods = user()->mfaValues() + ->get(['id', 'method']) + ->groupBy('method'); + return view('mfa.setup', [ + 'userMethods' => $userMethods, + ]); } /** * Show a view that generates and displays a TOTP QR code. - * @throws IncompatibleWithGoogleAuthenticatorException - * @throws InvalidCharactersException - * @throws SecretKeyTooShortException */ - public function totpGenerate() + public function totpGenerate(TotpService $totp) { - // TODO - Ensure a QR code doesn't already exist? Or overwrite? - $google2fa = new Google2FA(); if (session()->has(static::TOTP_SETUP_SECRET_SESSION_KEY)) { $totpSecret = decrypt(session()->get(static::TOTP_SETUP_SECRET_SESSION_KEY)); } else { - $totpSecret = $google2fa->generateSecretKey(); + $totpSecret = $totp->generateSecret(); session()->put(static::TOTP_SETUP_SECRET_SESSION_KEY, encrypt($totpSecret)); } - $qrCodeUrl = $google2fa->getQRCodeUrl( - setting('app-name'), - user()->email, - $totpSecret - ); + $qrCodeUrl = $totp->generateUrl($totpSecret); + $svg = $totp->generateQrCodeSvg($qrCodeUrl); - $color = Fill::uniformColor(new Rgb(255, 255, 255), new Rgb(32, 110, 167)); - $svg = (new Writer( - new ImageRenderer( - new RendererStyle(192, 0, null, null, $color), - new SvgImageBackEnd - ) - ))->writeString($qrCodeUrl); - - // Get user to verify setup via responding once. - // If correct response, Save key against user return view('mfa.totp-generate', [ 'secret' => $totpSecret, 'svg' => $svg, @@ -76,11 +55,18 @@ class MfaController extends Controller */ public function totpConfirm(Request $request) { + $totpSecret = decrypt(session()->get(static::TOTP_SETUP_SECRET_SESSION_KEY)); $this->validate($request, [ - 'code' => 'required|max:12|min:4' + 'code' => [ + 'required', + 'max:12', 'min:4', + new TotpValidationRule($totpSecret), + ] ]); - // TODO - Confirm code - dd($request->input('code')); + MfaValue::upsertWithValue(user(), MfaValue::METHOD_TOTP, $totpSecret); + $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'totp'); + + return redirect('/mfa/setup'); } } diff --git a/database/migrations/2021_06_30_173111_create_mfa_values_table.php b/database/migrations/2021_06_30_173111_create_mfa_values_table.php new file mode 100644 index 000000000..937fd31d9 --- /dev/null +++ b/database/migrations/2021_06_30_173111_create_mfa_values_table.php @@ -0,0 +1,34 @@ +increments('id'); + $table->integer('user_id')->index(); + $table->string('method', 20)->index(); + $table->text('value'); + $table->timestamps(); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::dropIfExists('mfa_values'); + } +} diff --git a/resources/lang/en/activities.php b/resources/lang/en/activities.php index 5917de2cf..2c371729b 100644 --- a/resources/lang/en/activities.php +++ b/resources/lang/en/activities.php @@ -47,6 +47,9 @@ return [ 'favourite_add_notification' => '":name" has been added to your favourites', 'favourite_remove_notification' => '":name" has been removed from your favourites', + // MFA + 'mfa_setup_method_notification' => 'Multi-factor method successfully configured', + // Other 'commented_on' => 'commented on', 'permissions_update' => 'updated permissions', diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php index 4031de2ae..5796d04c6 100644 --- a/resources/lang/en/validation.php +++ b/resources/lang/en/validation.php @@ -98,6 +98,7 @@ return [ ], 'string' => 'The :attribute must be a string.', 'timezone' => 'The :attribute must be a valid zone.', + 'totp' => 'The provided code is not valid or has expired.', 'unique' => 'The :attribute has already been taken.', 'url' => 'The :attribute format is invalid.', 'uploaded' => 'The file could not be uploaded. The server may not accept files of this size.', diff --git a/resources/views/mfa/setup.blade.php b/resources/views/mfa/setup.blade.php index 577841af5..d8fe50947 100644 --- a/resources/views/mfa/setup.blade.php +++ b/resources/views/mfa/setup.blade.php @@ -20,7 +20,15 @@

- Setup + @if($userMethods->has('totp')) +
+ @icon('check-circle') + Already configured +
+ Reconfigure + @else + Setup + @endif
diff --git a/tests/Auth/MfaTotpTest.php b/tests/Auth/MfaTotpTest.php new file mode 100644 index 000000000..f922063d6 --- /dev/null +++ b/tests/Auth/MfaTotpTest.php @@ -0,0 +1,10 @@ + Date: Fri, 2 Jul 2021 19:51:30 +0100 Subject: [PATCH 04/25] Covered TOTP setup with testing --- tests/Auth/MfaConfigurationTest.php | 52 +++++++++++++++++++++++++++++ tests/Auth/MfaTotpTest.php | 10 ------ tests/TestResponse.php | 8 +++++ 3 files changed, 60 insertions(+), 10 deletions(-) create mode 100644 tests/Auth/MfaConfigurationTest.php delete mode 100644 tests/Auth/MfaTotpTest.php diff --git a/tests/Auth/MfaConfigurationTest.php b/tests/Auth/MfaConfigurationTest.php new file mode 100644 index 000000000..9407c3735 --- /dev/null +++ b/tests/Auth/MfaConfigurationTest.php @@ -0,0 +1,52 @@ +getEditor(); + $this->assertDatabaseMissing('mfa_values', ['user_id' => $editor->id]); + + // Setup page state + $resp = $this->actingAs($editor)->get('/mfa/setup'); + $resp->assertElementContains('a[href$="/mfa/totp-generate"]', 'Setup'); + + // Generate page access + $resp = $this->get('/mfa/totp-generate'); + $resp->assertSee('Mobile App Setup'); + $resp->assertSee('Verify Setup'); + $resp->assertElementExists('form[action$="/mfa/totp-confirm"] button'); + $this->assertSessionHas('mfa-setup-totp-secret'); + $svg = $resp->getElementHtml('#main-content .card svg'); + + // Validation error, code should remain the same + $resp = $this->post('/mfa/totp-confirm', [ + 'code' => 'abc123', + ]); + $resp->assertRedirect('/mfa/totp-generate'); + $resp = $this->followRedirects($resp); + $resp->assertSee('The provided code is not valid or has expired.'); + $revisitSvg = $resp->getElementHtml('#main-content .card svg'); + $this->assertTrue($svg === $revisitSvg); + + // Successful confirmation + $google2fa = new Google2FA(); + $otp = $google2fa->getCurrentOtp(decrypt(session()->get('mfa-setup-totp-secret'))); + $resp = $this->post('/mfa/totp-confirm', [ + 'code' => $otp, + ]); + $resp->assertRedirect('/mfa/setup'); + + // Confirmation of setup + $resp = $this->followRedirects($resp); + $resp->assertSee('Multi-factor method successfully configured'); + $resp->assertElementContains('a[href$="/mfa/totp-generate"]', 'Reconfigure'); + } + +} \ No newline at end of file diff --git a/tests/Auth/MfaTotpTest.php b/tests/Auth/MfaTotpTest.php deleted file mode 100644 index f922063d6..000000000 --- a/tests/Auth/MfaTotpTest.php +++ /dev/null @@ -1,10 +0,0 @@ -crawlerInstance; } + /** + * Get the HTML of the first element at the given selector. + */ + public function getElementHtml(string $selector): string + { + return $this->crawler()->filter($selector)->first()->outerHtml(); + } + /** * Assert the response contains the specified element. * From 529971c53470c687f4e7c65900fc6f1f92c951c3 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 2 Jul 2021 20:53:33 +0100 Subject: [PATCH 05/25] Added backup code setup flow - Includes testing to cover flow. - Moved TOTP logic to its own controller. - Added some extra totp tests. --- app/Auth/Access/Mfa/BackupCodeService.php | 21 +++++++ app/Auth/Access/Mfa/MfaValue.php | 2 +- .../Auth/MfaBackupCodesController.php | 47 +++++++++++++++ app/Http/Controllers/Auth/MfaController.php | 51 ---------------- .../Controllers/Auth/MfaTotpController.php | 60 +++++++++++++++++++ .../views/mfa/backup-codes-generate.blade.php | 40 +++++++++++++ resources/views/mfa/setup.blade.php | 12 +++- resources/views/mfa/totp-generate.blade.php | 1 + routes/web.php | 6 +- tests/Auth/MfaConfigurationTest.php | 59 +++++++++++++++++- 10 files changed, 242 insertions(+), 57 deletions(-) create mode 100644 app/Auth/Access/Mfa/BackupCodeService.php create mode 100644 app/Http/Controllers/Auth/MfaBackupCodesController.php create mode 100644 app/Http/Controllers/Auth/MfaTotpController.php create mode 100644 resources/views/mfa/backup-codes-generate.blade.php diff --git a/app/Auth/Access/Mfa/BackupCodeService.php b/app/Auth/Access/Mfa/BackupCodeService.php new file mode 100644 index 000000000..cc533bd31 --- /dev/null +++ b/app/Auth/Access/Mfa/BackupCodeService.php @@ -0,0 +1,21 @@ +generateNewSet(); + session()->put(self::SETUP_SECRET_SESSION_KEY, encrypt($codes)); + + $downloadUrl = 'data:application/octet-stream;base64,' . base64_encode(implode("\n\n", $codes)); + + return view('mfa.backup-codes-generate', [ + 'codes' => $codes, + 'downloadUrl' => $downloadUrl, + ]); + } + + /** + * Confirm the setup of backup codes, storing them against the user. + * @throws Exception + */ + public function confirm() + { + if (!session()->has(self::SETUP_SECRET_SESSION_KEY)) { + return response('No generated codes found in the session', 500); + } + + $codes = decrypt(session()->pull(self::SETUP_SECRET_SESSION_KEY)); + MfaValue::upsertWithValue(user(), 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 8ddccaa98..caee416d3 100644 --- a/app/Http/Controllers/Auth/MfaController.php +++ b/app/Http/Controllers/Auth/MfaController.php @@ -2,18 +2,10 @@ namespace BookStack\Http\Controllers\Auth; -use BookStack\Actions\ActivityType; -use BookStack\Auth\Access\Mfa\MfaValue; -use BookStack\Auth\Access\Mfa\TotpService; -use BookStack\Auth\Access\Mfa\TotpValidationRule; use BookStack\Http\Controllers\Controller; -use Illuminate\Http\Request; -use Illuminate\Validation\ValidationException; class MfaController extends Controller { - protected const TOTP_SETUP_SECRET_SESSION_KEY = 'mfa-setup-totp-secret'; - /** * Show the view to setup MFA for the current user. */ @@ -26,47 +18,4 @@ class MfaController extends Controller 'userMethods' => $userMethods, ]); } - - /** - * Show a view that generates and displays a TOTP QR code. - */ - public function totpGenerate(TotpService $totp) - { - if (session()->has(static::TOTP_SETUP_SECRET_SESSION_KEY)) { - $totpSecret = decrypt(session()->get(static::TOTP_SETUP_SECRET_SESSION_KEY)); - } else { - $totpSecret = $totp->generateSecret(); - session()->put(static::TOTP_SETUP_SECRET_SESSION_KEY, encrypt($totpSecret)); - } - - $qrCodeUrl = $totp->generateUrl($totpSecret); - $svg = $totp->generateQrCodeSvg($qrCodeUrl); - - return view('mfa.totp-generate', [ - 'secret' => $totpSecret, - 'svg' => $svg, - ]); - } - - /** - * Confirm the setup of TOTP and save the auth method secret - * against the current user. - * @throws ValidationException - */ - public function totpConfirm(Request $request) - { - $totpSecret = decrypt(session()->get(static::TOTP_SETUP_SECRET_SESSION_KEY)); - $this->validate($request, [ - 'code' => [ - 'required', - 'max:12', 'min:4', - new TotpValidationRule($totpSecret), - ] - ]); - - MfaValue::upsertWithValue(user(), MfaValue::METHOD_TOTP, $totpSecret); - $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'totp'); - - return redirect('/mfa/setup'); - } } diff --git a/app/Http/Controllers/Auth/MfaTotpController.php b/app/Http/Controllers/Auth/MfaTotpController.php new file mode 100644 index 000000000..18f08e709 --- /dev/null +++ b/app/Http/Controllers/Auth/MfaTotpController.php @@ -0,0 +1,60 @@ +has(static::SETUP_SECRET_SESSION_KEY)) { + $totpSecret = decrypt(session()->get(static::SETUP_SECRET_SESSION_KEY)); + } else { + $totpSecret = $totp->generateSecret(); + session()->put(static::SETUP_SECRET_SESSION_KEY, encrypt($totpSecret)); + } + + $qrCodeUrl = $totp->generateUrl($totpSecret); + $svg = $totp->generateQrCodeSvg($qrCodeUrl); + + return view('mfa.totp-generate', [ + 'secret' => $totpSecret, + 'svg' => $svg, + ]); + } + + /** + * Confirm the setup of TOTP and save the auth method secret + * against the current user. + * @throws ValidationException + */ + public function confirm(Request $request) + { + $totpSecret = decrypt(session()->get(static::SETUP_SECRET_SESSION_KEY)); + $this->validate($request, [ + 'code' => [ + 'required', + 'max:12', 'min:4', + new TotpValidationRule($totpSecret), + ] + ]); + + MfaValue::upsertWithValue(user(), MfaValue::METHOD_TOTP, $totpSecret); + session()->remove(static::SETUP_SECRET_SESSION_KEY); + $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'totp'); + + return redirect('/mfa/setup'); + } +} diff --git a/resources/views/mfa/backup-codes-generate.blade.php b/resources/views/mfa/backup-codes-generate.blade.php new file mode 100644 index 000000000..8b437846e --- /dev/null +++ b/resources/views/mfa/backup-codes-generate.blade.php @@ -0,0 +1,40 @@ +@extends('simple-layout') + +@section('body') + +
+
+

Backup Codes

+

+ Store the below list of codes in a safe place. + When accessing the system you'll be able to use one of the codes + as a second authentication mechanism. +

+ +
+
+ @foreach($codes as $code) + {{ $code }}
+ @endforeach +
+
+ +

+ Download Codes +

+ +

+ Each code can only be used once +

+ +
+ {{ csrf_field() }} +
+ {{ trans('common.cancel') }} + +
+
+
+
+ +@stop diff --git a/resources/views/mfa/setup.blade.php b/resources/views/mfa/setup.blade.php index d8fe50947..c98d78885 100644 --- a/resources/views/mfa/setup.blade.php +++ b/resources/views/mfa/setup.blade.php @@ -36,12 +36,20 @@
Backup Codes

- Print out or securely store a set of one-time backup codes + Securely store a set of one-time-use backup codes which you can enter to verify your identity.

- Setup + @if($userMethods->has('backup_codes')) +
+ @icon('check-circle') + Already configured +
+ Reconfigure + @else + Setup + @endif
diff --git a/resources/views/mfa/totp-generate.blade.php b/resources/views/mfa/totp-generate.blade.php index 17d38adaa..c1e7547d5 100644 --- a/resources/views/mfa/totp-generate.blade.php +++ b/resources/views/mfa/totp-generate.blade.php @@ -35,6 +35,7 @@
{{ $errors->first('code') }}
@endif
+ {{ trans('common.cancel') }}
diff --git a/routes/web.php b/routes/web.php index f9967465b..7ab5890e0 100644 --- a/routes/web.php +++ b/routes/web.php @@ -225,8 +225,10 @@ Route::group(['middleware' => 'auth'], function () { }); Route::get('/mfa/setup', 'Auth\MfaController@setup'); - Route::get('/mfa/totp-generate', 'Auth\MfaController@totpGenerate'); - Route::post('/mfa/totp-confirm', 'Auth\MfaController@totpConfirm'); + 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'); }); // Social auth routes diff --git a/tests/Auth/MfaConfigurationTest.php b/tests/Auth/MfaConfigurationTest.php index 9407c3735..870850a73 100644 --- a/tests/Auth/MfaConfigurationTest.php +++ b/tests/Auth/MfaConfigurationTest.php @@ -2,6 +2,7 @@ namespace Tests\Auth; +use BookStack\Auth\Access\Mfa\MfaValue; use PragmaRX\Google2FA\Google2FA; use Tests\TestCase; @@ -37,7 +38,8 @@ class MfaConfigurationTest extends TestCase // Successful confirmation $google2fa = new Google2FA(); - $otp = $google2fa->getCurrentOtp(decrypt(session()->get('mfa-setup-totp-secret'))); + $secret = decrypt(session()->get('mfa-setup-totp-secret')); + $otp = $google2fa->getCurrentOtp($secret); $resp = $this->post('/mfa/totp-confirm', [ 'code' => $otp, ]); @@ -47,6 +49,61 @@ class MfaConfigurationTest extends TestCase $resp = $this->followRedirects($resp); $resp->assertSee('Multi-factor method successfully configured'); $resp->assertElementContains('a[href$="/mfa/totp-generate"]', 'Reconfigure'); + + $this->assertDatabaseHas('mfa_values', [ + 'user_id' => $editor->id, + 'method' => 'totp', + ]); + $this->assertFalse(session()->has('mfa-setup-totp-secret')); + $value = MfaValue::query()->where('user_id', '=', $editor->id) + ->where('method', '=', 'totp')->first(); + $this->assertEquals($secret, decrypt($value->value)); + } + + public function test_backup_codes_setup() + { + $editor = $this->getEditor(); + $this->assertDatabaseMissing('mfa_values', ['user_id' => $editor->id]); + + // Setup page state + $resp = $this->actingAs($editor)->get('/mfa/setup'); + $resp->assertElementContains('a[href$="/mfa/backup-codes-generate"]', 'Setup'); + + // Generate page access + $resp = $this->get('/mfa/backup-codes-generate'); + $resp->assertSee('Backup Codes'); + $resp->assertElementContains('form[action$="/mfa/backup-codes-confirm"]', 'Confirm and Enable'); + $this->assertSessionHas('mfa-setup-backup-codes'); + $codes = decrypt(session()->get('mfa-setup-backup-codes')); + // Check code format + $this->assertCount(16, $codes); + $this->assertEquals(16*11, strlen(implode('', $codes))); + // Check download link + $resp->assertSee(base64_encode(implode("\n\n", $codes))); + + // Confirm submit + $resp = $this->post('/mfa/backup-codes-confirm'); + $resp->assertRedirect('/mfa/setup'); + + // Confirmation of setup + $resp = $this->followRedirects($resp); + $resp->assertSee('Multi-factor method successfully configured'); + $resp->assertElementContains('a[href$="/mfa/backup-codes-generate"]', 'Reconfigure'); + + $this->assertDatabaseHas('mfa_values', [ + 'user_id' => $editor->id, + 'method' => 'backup_codes', + ]); + $this->assertFalse(session()->has('mfa-setup-backup-codes')); + $value = MfaValue::query()->where('user_id', '=', $editor->id) + ->where('method', '=', 'backup_codes')->first(); + $this->assertEquals($codes, json_decode(decrypt($value->value))); + } + + public function test_backup_codes_cannot_be_confirmed_if_not_previously_generated() + { + $resp = $this->asEditor()->post('/mfa/backup-codes-confirm'); + $resp->assertStatus(500); } } \ No newline at end of file From 09c2814dc7b142b475c571b59bd8c902af183ad3 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 3 Jul 2021 13:34:48 +0100 Subject: [PATCH 06/25] Added role based MFA control - Added new DB column for control and role updated create/update actions. - Created new middleware as a start to actual enforcement logic. - Added indicator to role list of whether MFA is enforced. --- app/Auth/Permissions/PermissionsRepo.php | 2 ++ app/Auth/Role.php | 1 + .../Middleware/EnforceMfaRequirements.php | 24 ++++++++++++++ ...085038_add_mfa_enforced_to_roles_table.php | 32 +++++++++++++++++++ resources/lang/en/settings.php | 1 + resources/views/settings/roles/form.blade.php | 7 ++-- .../views/settings/roles/index.blade.php | 7 +++- routes/web.php | 1 + tests/Permissions/RolesTest.php | 5 +-- 9 files changed, 75 insertions(+), 5 deletions(-) create mode 100644 app/Http/Middleware/EnforceMfaRequirements.php create mode 100644 database/migrations/2021_07_03_085038_add_mfa_enforced_to_roles_table.php diff --git a/app/Auth/Permissions/PermissionsRepo.php b/app/Auth/Permissions/PermissionsRepo.php index 4d191679d..988146700 100644 --- a/app/Auth/Permissions/PermissionsRepo.php +++ b/app/Auth/Permissions/PermissionsRepo.php @@ -57,6 +57,7 @@ class PermissionsRepo public function saveNewRole(array $roleData): Role { $role = $this->role->newInstance($roleData); + $role->mfa_enforced = ($roleData['mfa_enforced'] ?? 'false') === 'true'; $role->save(); $permissions = isset($roleData['permissions']) ? array_keys($roleData['permissions']) : []; @@ -90,6 +91,7 @@ class PermissionsRepo $this->assignRolePermissions($role, $permissions); $role->fill($roleData); + $role->mfa_enforced = ($roleData['mfa_enforced'] ?? 'false') === 'true'; $role->save(); $this->permissionService->buildJointPermissionForRole($role); Activity::add(ActivityType::ROLE_UPDATE, $role); diff --git a/app/Auth/Role.php b/app/Auth/Role.php index 94ba39d1d..dcd960948 100644 --- a/app/Auth/Role.php +++ b/app/Auth/Role.php @@ -18,6 +18,7 @@ use Illuminate\Database\Eloquent\Relations\HasMany; * @property string $description * @property string $external_auth_id * @property string $system_name + * @property bool $mfa_enforced */ class Role extends Model implements Loggable { diff --git a/app/Http/Middleware/EnforceMfaRequirements.php b/app/Http/Middleware/EnforceMfaRequirements.php new file mode 100644 index 000000000..957b42ae1 --- /dev/null +++ b/app/Http/Middleware/EnforceMfaRequirements.php @@ -0,0 +1,24 @@ +roles()->where('mfa_enforced', '=', true)->exists(); + // TODO - Run this after auth (If authenticated) + // TODO - Redirect user to setup MFA or verify via MFA. + // TODO - Store mfa_pass into session for future requests? + return $next($request); + } +} diff --git a/database/migrations/2021_07_03_085038_add_mfa_enforced_to_roles_table.php b/database/migrations/2021_07_03_085038_add_mfa_enforced_to_roles_table.php new file mode 100644 index 000000000..c14d47ea7 --- /dev/null +++ b/database/migrations/2021_07_03_085038_add_mfa_enforced_to_roles_table.php @@ -0,0 +1,32 @@ +boolean('mfa_enforced'); + }); + } + + /** + * Reverse the migrations. + * + * @return void + */ + public function down() + { + Schema::table('roles', function (Blueprint $table) { + $table->dropColumn('mfa_enforced'); + }); + } +} diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index 789ef9d1b..d9b4854fe 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -138,6 +138,7 @@ return [ 'role_details' => 'Role Details', 'role_name' => 'Role Name', 'role_desc' => 'Short Description of Role', + 'role_mfa_enforced' => 'Requires Multi-Factor Authentication', 'role_external_auth_id' => 'External Authentication IDs', 'role_system' => 'System Permissions', 'role_manage_users' => 'Manage users', diff --git a/resources/views/settings/roles/form.blade.php b/resources/views/settings/roles/form.blade.php index 604acbb16..d1a61f0cd 100644 --- a/resources/views/settings/roles/form.blade.php +++ b/resources/views/settings/roles/form.blade.php @@ -11,13 +11,16 @@
- + @include('form.text', ['name' => 'display_name'])
- + @include('form.text', ['name' => 'description'])
+
+ @include('form.checkbox', ['name' => 'mfa_enforced', 'label' => trans('settings.role_mfa_enforced') ]) +
@if(config('auth.method') === 'ldap' || config('auth.method') === 'saml2')
diff --git a/resources/views/settings/roles/index.blade.php b/resources/views/settings/roles/index.blade.php index 47cd8c920..898a96eef 100644 --- a/resources/views/settings/roles/index.blade.php +++ b/resources/views/settings/roles/index.blade.php @@ -27,7 +27,12 @@ @foreach($roles as $role) id}") }}">{{ $role->display_name }} - {{ $role->description }} + + @if($role->mfa_enforced) + @icon('lock') + @endif + {{ $role->description }} + {{ $role->users->count() }} @endforeach diff --git a/routes/web.php b/routes/web.php index 7ab5890e0..3be6218b0 100644 --- a/routes/web.php +++ b/routes/web.php @@ -224,6 +224,7 @@ Route::group(['middleware' => 'auth'], function () { Route::put('/roles/{id}', 'RoleController@update'); }); + // MFA Setup Routes Route::get('/mfa/setup', 'Auth\MfaController@setup'); Route::get('/mfa/totp-generate', 'Auth\MfaTotpController@generate'); Route::post('/mfa/totp-confirm', 'Auth\MfaTotpController@confirm'); diff --git a/tests/Permissions/RolesTest.php b/tests/Permissions/RolesTest.php index 09c3233e3..b9b1805b6 100644 --- a/tests/Permissions/RolesTest.php +++ b/tests/Permissions/RolesTest.php @@ -64,15 +64,16 @@ class RolesTest extends BrowserKitTest ->type('Test Role', 'display_name') ->type('A little test description', 'description') ->press('Save Role') - ->seeInDatabase('roles', ['display_name' => $testRoleName, 'description' => $testRoleDesc]) + ->seeInDatabase('roles', ['display_name' => $testRoleName, 'description' => $testRoleDesc, 'mfa_enforced' => false]) ->seePageIs('/settings/roles'); // Updating $this->asAdmin()->visit('/settings/roles') ->see($testRoleDesc) ->click($testRoleName) ->type($testRoleUpdateName, '#display_name') + ->check('#mfa_enforced') ->press('Save Role') - ->seeInDatabase('roles', ['display_name' => $testRoleUpdateName, 'description' => $testRoleDesc]) + ->seeInDatabase('roles', ['display_name' => $testRoleUpdateName, 'description' => $testRoleDesc, 'mfa_enforced' => true]) ->seePageIs('/settings/roles'); // Deleting $this->asAdmin()->visit('/settings/roles') From bb43acef21702a4083a5137db0e3bf5ff95df576 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 14 Jul 2021 20:06:41 +0100 Subject: [PATCH 07/25] Added MFA setup link on user edit view --- app/Http/Controllers/UserController.php | 2 ++ resources/views/users/edit.blade.php | 24 ++++++++++++++++++++++++ tests/Auth/MfaConfigurationTest.php | 25 +++++++++++++++++++++++++ 3 files changed, 51 insertions(+) diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index f7b2afef8..7f42e94cc 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -128,12 +128,14 @@ class UserController extends Controller $authMethod = ($user->system_name) ? 'system' : config('auth.method'); $activeSocialDrivers = $socialAuthService->getActiveDrivers(); + $mfaMethods = user()->mfaValues()->get(['id', 'method'])->groupBy('method'); $this->setPageTitle(trans('settings.user_profile')); $roles = $this->userRepo->getAllRoles(); return view('users.edit', [ 'user' => $user, 'activeSocialDrivers' => $activeSocialDrivers, + 'mfaMethods' => $mfaMethods, 'authMethod' => $authMethod, 'roles' => $roles, ]); diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 5712855e6..2d719668f 100644 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -63,6 +63,30 @@ +
+

Multi-Factor Authentication

+

+ Setup multi-factor authentication as an extra layer of security + for your user account. +

+
+
+ @if ($mfaMethods->count() > 0) + @icon('check-circle') + @else + @icon('cancel') + @endif + {{ $mfaMethods->count() }} {{ $mfaMethods->count() === 1 ? 'method' : 'methods' }} configured +
+
+ @if($user->id === user()->id) + Configure Methods + @endif +
+
+ +
+ @if(user()->id === $user->id && count($activeSocialDrivers) > 0)

{{ trans('settings.users_social_accounts') }}

diff --git a/tests/Auth/MfaConfigurationTest.php b/tests/Auth/MfaConfigurationTest.php index 870850a73..f332b6721 100644 --- a/tests/Auth/MfaConfigurationTest.php +++ b/tests/Auth/MfaConfigurationTest.php @@ -106,4 +106,29 @@ class MfaConfigurationTest extends TestCase $resp->assertStatus(500); } + public function test_mfa_method_count_is_visible_on_user_edit_page() + { + $admin = $this->getAdmin(); + $resp = $this->actingAs($admin)->get($admin->getEditUrl()); + $resp->assertSee('0 methods configured'); + + MfaValue::upsertWithValue($admin, MfaValue::METHOD_TOTP, 'test'); + $resp = $this->actingAs($admin)->get($admin->getEditUrl()); + $resp->assertSee('1 method configured'); + + MfaValue::upsertWithValue($admin, MfaValue::METHOD_BACKUP_CODES, 'test'); + $resp = $this->actingAs($admin)->get($admin->getEditUrl()); + $resp->assertSee('2 methods configured'); + } + + public function test_mfa_setup_link_only_shown_when_viewing_own_user_edit_page() + { + $admin = $this->getAdmin(); + $resp = $this->actingAs($admin)->get($admin->getEditUrl()); + $resp->assertElementExists('a[href$="/mfa/setup"]'); + + $resp = $this->actingAs($admin)->get($this->getEditor()->getEditUrl()); + $resp->assertElementNotExists('a[href$="/mfa/setup"]'); + } + } \ No newline at end of file From cfc0c593dba2c0b4ce31b98e13d90779f8c50453 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 14 Jul 2021 20:18:48 +0100 Subject: [PATCH 08/25] Added MFA indicator to user list Also fixed issue with showing incorrect MFA method count on user edit page changes done in last commit --- app/Auth/User.php | 1 + app/Auth/UserRepo.php | 1 + app/Http/Controllers/UserController.php | 5 +++-- resources/views/users/index.blade.php | 7 ++++++- tests/Auth/MfaConfigurationTest.php | 26 +++++++++++++++++++------ 5 files changed, 31 insertions(+), 9 deletions(-) diff --git a/app/Auth/User.php b/app/Auth/User.php index f4fd45281..0a6849fe0 100644 --- a/app/Auth/User.php +++ b/app/Auth/User.php @@ -39,6 +39,7 @@ use Illuminate\Support\Collection; * @property string $external_auth_id * @property string $system_name * @property Collection $roles + * @property Collection $mfaValues */ class User extends Model implements AuthenticatableContract, CanResetPasswordContract, Loggable, Sluggable { diff --git a/app/Auth/UserRepo.php b/app/Auth/UserRepo.php index 9faeb8ae2..e1a040fc2 100644 --- a/app/Auth/UserRepo.php +++ b/app/Auth/UserRepo.php @@ -71,6 +71,7 @@ class UserRepo $query = User::query()->select(['*']) ->withLastActivityAt() ->with(['roles', 'avatar']) + ->withCount('mfaValues') ->orderBy($sort, $sortData['order']); if ($sortData['search']) { diff --git a/app/Http/Controllers/UserController.php b/app/Http/Controllers/UserController.php index 7f42e94cc..a0da220ee 100644 --- a/app/Http/Controllers/UserController.php +++ b/app/Http/Controllers/UserController.php @@ -123,12 +123,13 @@ class UserController extends Controller { $this->checkPermissionOrCurrentUser('users-manage', $id); - $user = $this->user->newQuery()->with(['apiTokens'])->findOrFail($id); + /** @var User $user */ + $user = $this->user->newQuery()->with(['apiTokens', 'mfaValues'])->findOrFail($id); $authMethod = ($user->system_name) ? 'system' : config('auth.method'); $activeSocialDrivers = $socialAuthService->getActiveDrivers(); - $mfaMethods = user()->mfaValues()->get(['id', 'method'])->groupBy('method'); + $mfaMethods = $user->mfaValues->groupBy('method'); $this->setPageTitle(trans('settings.user_profile')); $roles = $this->userRepo->getAllRoles(); diff --git a/resources/views/users/index.blade.php b/resources/views/users/index.blade.php index 5eef51175..9a9221242 100644 --- a/resources/views/users/index.blade.php +++ b/resources/views/users/index.blade.php @@ -43,7 +43,12 @@ {{ $user->name }} id}") }}"> - {{ $user->name }}
{{ $user->email }} + {{ $user->name }} +
+ {{ $user->email }} + @if($user->mfa_values_count > 0) + @icon('lock') + @endif diff --git a/tests/Auth/MfaConfigurationTest.php b/tests/Auth/MfaConfigurationTest.php index f332b6721..adeb66189 100644 --- a/tests/Auth/MfaConfigurationTest.php +++ b/tests/Auth/MfaConfigurationTest.php @@ -3,6 +3,7 @@ namespace Tests\Auth; use BookStack\Auth\Access\Mfa\MfaValue; +use BookStack\Auth\User; use PragmaRX\Google2FA\Google2FA; use Tests\TestCase; @@ -108,16 +109,16 @@ class MfaConfigurationTest extends TestCase public function test_mfa_method_count_is_visible_on_user_edit_page() { - $admin = $this->getAdmin(); - $resp = $this->actingAs($admin)->get($admin->getEditUrl()); + $user = $this->getEditor(); + $resp = $this->actingAs($this->getAdmin())->get($user->getEditUrl()); $resp->assertSee('0 methods configured'); - MfaValue::upsertWithValue($admin, MfaValue::METHOD_TOTP, 'test'); - $resp = $this->actingAs($admin)->get($admin->getEditUrl()); + MfaValue::upsertWithValue($user, MfaValue::METHOD_TOTP, 'test'); + $resp = $this->get($user->getEditUrl()); $resp->assertSee('1 method configured'); - MfaValue::upsertWithValue($admin, MfaValue::METHOD_BACKUP_CODES, 'test'); - $resp = $this->actingAs($admin)->get($admin->getEditUrl()); + MfaValue::upsertWithValue($user, MfaValue::METHOD_BACKUP_CODES, 'test'); + $resp = $this->get($user->getEditUrl()); $resp->assertSee('2 methods configured'); } @@ -131,4 +132,17 @@ class MfaConfigurationTest extends TestCase $resp->assertElementNotExists('a[href$="/mfa/setup"]'); } + public function test_mfa_indicator_shows_in_user_list() + { + $admin = $this->getAdmin(); + User::query()->where('id', '!=', $admin->id)->delete(); + + $resp = $this->actingAs($admin)->get('/settings/users'); + $resp->assertElementNotExists('[title="MFA Configured"] svg'); + + MfaValue::upsertWithValue($admin, MfaValue::METHOD_TOTP, 'test'); + $resp = $this->actingAs($admin)->get('/settings/users'); + $resp->assertElementExists('[title="MFA Configured"] svg'); + } + } \ No newline at end of file From 7c86c26cd0585a063ca4905a1302301e3c1d2e19 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 14 Jul 2021 20:50:36 +0100 Subject: [PATCH 09/25] Added command to reset user MFA Includes tests to cover the command. --- app/Console/Commands/ResetMfa.php | 74 ++++++++++++++++++++++++++ tests/Commands/ResetMfaCommandTest.php | 65 ++++++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 app/Console/Commands/ResetMfa.php create mode 100644 tests/Commands/ResetMfaCommandTest.php diff --git a/app/Console/Commands/ResetMfa.php b/app/Console/Commands/ResetMfa.php new file mode 100644 index 000000000..feb477943 --- /dev/null +++ b/app/Console/Commands/ResetMfa.php @@ -0,0 +1,74 @@ +option('id'); + $email = $this->option('email'); + if (!$id && !$email) { + $this->error('Either a --id= or --email= option must be provided.'); + return 1; + } + + /** @var User $user */ + $field = $id ? 'id' : 'email'; + $value = $id ?: $email; + $user = User::query() + ->where($field, '=', $value) + ->first(); + + if (!$user) { + $this->error("A user where {$field}={$value} could not be found."); + return 1; + } + + $this->info("This will delete any configure multi-factor authentication methods for user: \n- ID: {$user->id}\n- Name: {$user->name}\n- Email: {$user->email}\n"); + $this->info('If multi-factor authentication is required for this user they will be asked to reconfigure their methods on next login.'); + $confirm = $this->confirm('Are you sure you want to proceed?'); + if ($confirm) { + $user->mfaValues()->delete(); + $this->info('User MFA methods have been reset.'); + return 0; + } + + return 1; + } +} diff --git a/tests/Commands/ResetMfaCommandTest.php b/tests/Commands/ResetMfaCommandTest.php new file mode 100644 index 000000000..550f6d390 --- /dev/null +++ b/tests/Commands/ResetMfaCommandTest.php @@ -0,0 +1,65 @@ +artisan('bookstack:reset-mfa') + ->expectsOutput('Either a --id= or --email= option must be provided.') + ->assertExitCode(1); + } + + public function test_command_runs_with_provided_email() + { + /** @var User $user */ + $user = User::query()->first(); + MfaValue::upsertWithValue($user, MfaValue::METHOD_TOTP, 'test'); + + $this->assertEquals(1, $user->mfaValues()->count()); + $this->artisan("bookstack:reset-mfa --email={$user->email}") + ->expectsQuestion('Are you sure you want to proceed?', true) + ->expectsOutput('User MFA methods have been reset.') + ->assertExitCode(0); + $this->assertEquals(0, $user->mfaValues()->count()); + } + + public function test_command_runs_with_provided_id() + { + /** @var User $user */ + $user = User::query()->first(); + MfaValue::upsertWithValue($user, MfaValue::METHOD_TOTP, 'test'); + + $this->assertEquals(1, $user->mfaValues()->count()); + $this->artisan("bookstack:reset-mfa --id={$user->id}") + ->expectsQuestion('Are you sure you want to proceed?', true) + ->expectsOutput('User MFA methods have been reset.') + ->assertExitCode(0); + $this->assertEquals(0, $user->mfaValues()->count()); + } + + public function test_saying_no_to_confirmation_does_not_reset_mfa() + { + /** @var User $user */ + $user = User::query()->first(); + MfaValue::upsertWithValue($user, MfaValue::METHOD_TOTP, 'test'); + + $this->assertEquals(1, $user->mfaValues()->count()); + $this->artisan("bookstack:reset-mfa --id={$user->id}") + ->expectsQuestion('Are you sure you want to proceed?', false) + ->assertExitCode(1); + $this->assertEquals(1, $user->mfaValues()->count()); + } + + public function test_giving_non_existing_user_shows_error_message() + { + $this->artisan("bookstack:reset-mfa --email=donkeys@example.com") + ->expectsOutput('A user where email=donkeys@example.com could not be found.') + ->assertExitCode(1); + } +} From f696aa5eea5a06c7e21130fb9b01a6b60c50c2db Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Wed, 14 Jul 2021 21:27:21 +0100 Subject: [PATCH 10/25] Added the ability to remove an MFA method Includes testing to cover --- app/Actions/ActivityType.php | 1 + app/Auth/Access/Mfa/MfaValue.php | 8 ++++++++ app/Http/Controllers/Auth/MfaController.php | 19 ++++++++++++++++++ resources/lang/en/activities.php | 1 + resources/sass/_layout.scss | 4 ++++ resources/views/mfa/setup.blade.php | 22 +++++++++++++++++++++ routes/web.php | 1 + tests/Auth/MfaConfigurationTest.php | 19 ++++++++++++++++++ 8 files changed, 75 insertions(+) diff --git a/app/Actions/ActivityType.php b/app/Actions/ActivityType.php index e2e7b5a5d..60b1630e0 100644 --- a/app/Actions/ActivityType.php +++ b/app/Actions/ActivityType.php @@ -52,4 +52,5 @@ class ActivityType const AUTH_REGISTER = 'auth_register'; const MFA_SETUP_METHOD = 'mfa_setup_method'; + const MFA_REMOVE_METHOD = 'mfa_remove_method'; } diff --git a/app/Auth/Access/Mfa/MfaValue.php b/app/Auth/Access/Mfa/MfaValue.php index cba90dcac..9f9ab29a5 100644 --- a/app/Auth/Access/Mfa/MfaValue.php +++ b/app/Auth/Access/Mfa/MfaValue.php @@ -21,6 +21,14 @@ class MfaValue extends Model const METHOD_TOTP = 'totp'; const METHOD_BACKUP_CODES = 'backup_codes'; + /** + * Get all the MFA methods available. + */ + public static function allMethods(): array + { + return [self::METHOD_TOTP, self::METHOD_BACKUP_CODES]; + } + /** * Upsert a new MFA value for the given user and method * using the provided value. diff --git a/app/Http/Controllers/Auth/MfaController.php b/app/Http/Controllers/Auth/MfaController.php index caee416d3..9feda9433 100644 --- a/app/Http/Controllers/Auth/MfaController.php +++ b/app/Http/Controllers/Auth/MfaController.php @@ -2,6 +2,8 @@ namespace BookStack\Http\Controllers\Auth; +use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\Mfa\MfaValue; use BookStack\Http\Controllers\Controller; class MfaController extends Controller @@ -18,4 +20,21 @@ class MfaController extends Controller 'userMethods' => $userMethods, ]); } + + /** + * Remove an MFA method for the current user. + * @throws \Exception + */ + public function remove(string $method) + { + if (in_array($method, MfaValue::allMethods())) { + $value = user()->mfaValues()->where('method', '=', $method)->first(); + if ($value) { + $value->delete(); + $this->logActivity(ActivityType::MFA_REMOVE_METHOD, $method); + } + } + + return redirect('/mfa/setup'); + } } diff --git a/resources/lang/en/activities.php b/resources/lang/en/activities.php index 2c371729b..50bda60bd 100644 --- a/resources/lang/en/activities.php +++ b/resources/lang/en/activities.php @@ -49,6 +49,7 @@ return [ // MFA 'mfa_setup_method_notification' => 'Multi-factor method successfully configured', + 'mfa_remove_method_notification' => 'Multi-factor method successfully removed', // Other 'commented_on' => 'commented on', diff --git a/resources/sass/_layout.scss b/resources/sass/_layout.scss index 516d7d612..e26948301 100644 --- a/resources/sass/_layout.scss +++ b/resources/sass/_layout.scss @@ -181,6 +181,10 @@ body.flexbox { display: inline-block !important; } +.relative { + position: relative; +} + .hidden { display: none !important; } diff --git a/resources/views/mfa/setup.blade.php b/resources/views/mfa/setup.blade.php index c98d78885..2ec8d0f77 100644 --- a/resources/views/mfa/setup.blade.php +++ b/resources/views/mfa/setup.blade.php @@ -26,6 +26,17 @@ Already configured
Reconfigure +
+ +
+

Are you sure you want to remove this multi-factor authentication method?

+
+ {{ csrf_field() }} + {{ method_field('delete') }} + +
+
+
@else Setup @endif @@ -47,6 +58,17 @@ Already configured
Reconfigure +
+ +
+

Are you sure you want to remove this multi-factor authentication method?

+
+ {{ csrf_field() }} + {{ method_field('delete') }} + +
+
+
@else Setup @endif diff --git a/routes/web.php b/routes/web.php index 3be6218b0..e3329edc4 100644 --- a/routes/web.php +++ b/routes/web.php @@ -230,6 +230,7 @@ Route::group(['middleware' => 'auth'], function () { 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::delete('/mfa/remove/{method}', 'Auth\MfaController@remove'); }); // Social auth routes diff --git a/tests/Auth/MfaConfigurationTest.php b/tests/Auth/MfaConfigurationTest.php index adeb66189..a8ca815fb 100644 --- a/tests/Auth/MfaConfigurationTest.php +++ b/tests/Auth/MfaConfigurationTest.php @@ -2,6 +2,7 @@ namespace Tests\Auth; +use BookStack\Actions\ActivityType; use BookStack\Auth\Access\Mfa\MfaValue; use BookStack\Auth\User; use PragmaRX\Google2FA\Google2FA; @@ -145,4 +146,22 @@ class MfaConfigurationTest extends TestCase $resp->assertElementExists('[title="MFA Configured"] svg'); } + public function test_remove_mfa_method() + { + $admin = $this->getAdmin(); + + MfaValue::upsertWithValue($admin, MfaValue::METHOD_TOTP, 'test'); + $this->assertEquals(1, $admin->mfaValues()->count()); + $resp = $this->actingAs($admin)->get('/mfa/setup'); + $resp->assertElementExists('form[action$="/mfa/remove/totp"]'); + + $resp = $this->delete("/mfa/remove/totp"); + $resp->assertRedirect("/mfa/setup"); + $resp = $this->followRedirects($resp); + $resp->assertSee('Multi-factor method successfully removed'); + + $this->assertActivityExists(ActivityType::MFA_REMOVE_METHOD); + $this->assertEquals(0, $admin->mfaValues()->count()); + } + } \ No newline at end of file From 78f9c01519d9d4ea1a2aeff6ef1346ca4ee9e6ff Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Fri, 16 Jul 2021 23:23:36 +0100 Subject: [PATCH 11/25] Started on some MFA access-time checks Discovered some difficult edge cases: - User image loading in header bar when using local_secure storage - 404s showing user-specific visible content due to content listing on 404 page since user is in semi-logged in state. Maybe need to go through and change up how logins are handled to centralise and provide us better control at login time to prevent any auth level. --- app/Auth/Access/Mfa/MfaSession.php | 44 +++++++++++++++++++ app/Http/Controllers/Auth/MfaController.php | 14 ++++++ app/Http/Kernel.php | 1 + .../Middleware/EnforceMfaRequirements.php | 30 +++++++++++-- resources/views/mfa/verify.blade.php | 31 +++++++++++++ routes/web.php | 5 ++- 6 files changed, 120 insertions(+), 5 deletions(-) create mode 100644 app/Auth/Access/Mfa/MfaSession.php create mode 100644 resources/views/mfa/verify.blade.php diff --git a/app/Auth/Access/Mfa/MfaSession.php b/app/Auth/Access/Mfa/MfaSession.php new file mode 100644 index 000000000..67574cbaf --- /dev/null +++ b/app/Auth/Access/Mfa/MfaSession.php @@ -0,0 +1,44 @@ +mfaValues()->exists() || $this->currentUserRoleEnforcesMfa(); + } + + /** + * Check if a role of the current user enforces MFA. + */ + protected function currentUserRoleEnforcesMfa(): bool + { + return user()->roles() + ->where('mfa_enforced', '=', true) + ->exists(); + } + + /** + * Check if the current MFA session has already been verified. + */ + public function isVerified(): bool + { + return session()->get(self::MFA_VERIFIED_SESSION_KEY) === 'true'; + } + + /** + * Mark the current session as MFA-verified. + */ + public function markVerified(): void + { + session()->put(self::MFA_VERIFIED_SESSION_KEY, 'true'); + } + +} \ No newline at end of file diff --git a/app/Http/Controllers/Auth/MfaController.php b/app/Http/Controllers/Auth/MfaController.php index 9feda9433..39a4e852f 100644 --- a/app/Http/Controllers/Auth/MfaController.php +++ b/app/Http/Controllers/Auth/MfaController.php @@ -37,4 +37,18 @@ class MfaController extends Controller return redirect('/mfa/setup'); } + + /** + * Show the page to start an MFA verification. + */ + public function verify() + { + $userMethods = user()->mfaValues() + ->get(['id', 'method']) + ->groupBy('method'); + + return view('mfa.verify', [ + 'userMethods' => $userMethods, + ]); + } } diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index 4f9bfc1e6..c9e59ed3e 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -48,6 +48,7 @@ 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/EnforceMfaRequirements.php b/app/Http/Middleware/EnforceMfaRequirements.php index 957b42ae1..ac3c9609b 100644 --- a/app/Http/Middleware/EnforceMfaRequirements.php +++ b/app/Http/Middleware/EnforceMfaRequirements.php @@ -2,10 +2,21 @@ 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. * @@ -15,10 +26,23 @@ class EnforceMfaRequirements */ public function handle($request, Closure $next) { - $mfaRequired = user()->roles()->where('mfa_enforced', '=', true)->exists(); - // TODO - Run this after auth (If authenticated) - // TODO - Redirect user to setup MFA or verify via MFA. + 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/resources/views/mfa/verify.blade.php b/resources/views/mfa/verify.blade.php new file mode 100644 index 000000000..4ff0e6c49 --- /dev/null +++ b/resources/views/mfa/verify.blade.php @@ -0,0 +1,31 @@ +@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. +

+ +
+
+
+
METHOD A
+

+ ... +

+
+
+ BUTTON +
+
+ +
+ +
+
+@stop diff --git a/routes/web.php b/routes/web.php index e3329edc4..406cfd767 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'], function () { +Route::group(['middleware' => ['auth', 'mfa']], function () { // Secure images routing Route::get('/uploads/images/{path}', 'Images\ImageController@showImage') @@ -224,13 +224,14 @@ Route::group(['middleware' => 'auth'], function () { Route::put('/roles/{id}', 'RoleController@update'); }); - // MFA Setup Routes + // 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'); Route::delete('/mfa/remove/{method}', 'Auth\MfaController@remove'); + Route::get('/mfa/verify', 'Auth\MfaController@verify'); }); // Social auth routes From 9249addb5c458d30a5b06ff1ccc3a4e1b5d29acb Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 17 Jul 2021 17:45:00 +0100 Subject: [PATCH 12/25] Updated all login events to route through single service --- .../Guards/ExternalBaseSessionGuard.php | 8 +-- app/Auth/Access/LoginService.php | 50 +++++++++++++++++++ app/Auth/Access/Saml2Service.php | 13 ++--- app/Auth/Access/SocialAuthService.php | 17 +++---- .../Auth/ConfirmEmailController.php | 13 +++-- app/Http/Controllers/Auth/LoginController.php | 33 ++++++------ .../Controllers/Auth/RegisterController.php | 16 +++--- .../Controllers/Auth/SocialController.php | 17 ++++--- .../Controllers/Auth/UserInviteController.php | 9 ++-- .../Middleware/EnforceMfaRequirements.php | 2 +- app/Providers/AppServiceProvider.php | 3 +- 11 files changed, 118 insertions(+), 63 deletions(-) create mode 100644 app/Auth/Access/LoginService.php diff --git a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php index 9c3b47e97..99bfd2e79 100644 --- a/app/Auth/Access/Guards/ExternalBaseSessionGuard.php +++ b/app/Auth/Access/Guards/ExternalBaseSessionGuard.php @@ -186,12 +186,8 @@ class ExternalBaseSessionGuard implements StatefulGuard */ public function loginUsingId($id, $remember = false) { - if (!is_null($user = $this->provider->retrieveById($id))) { - $this->login($user, $remember); - - return $user; - } - + // Always return false as to disable this method, + // Logins should route through LoginService. return false; } diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php new file mode 100644 index 000000000..2bdef34a2 --- /dev/null +++ b/app/Auth/Access/LoginService.php @@ -0,0 +1,50 @@ +login($user); + Activity::add(ActivityType::AUTH_LOGIN, "{$method}; {$user->logDescriptor()}"); + Theme::dispatch(ThemeEvents::AUTH_LOGIN, $method, $user); + + // Authenticate on all session guards if a likely admin + if ($user->can('users-manage') && $user->can('user-roles-manage')) { + $guards = ['standard', 'ldap', 'saml2']; + foreach ($guards as $guard) { + auth($guard)->login($user); + } + } + } + + + /** + * 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. + */ + public function attempt(array $credentials, string $method, bool $remember = false): bool + { + $result = auth()->attempt($credentials, $remember); + if ($result) { + $user = auth()->user(); + auth()->logout(); + $this->login($user, $method); + } + + return $result; + } + +} \ No newline at end of file diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php index 28d4d4030..d0b6a655b 100644 --- a/app/Auth/Access/Saml2Service.php +++ b/app/Auth/Access/Saml2Service.php @@ -25,16 +25,16 @@ class Saml2Service extends ExternalAuthService { protected $config; protected $registrationService; - protected $user; + protected $loginService; /** * Saml2Service constructor. */ - public function __construct(RegistrationService $registrationService, User $user) + public function __construct(RegistrationService $registrationService, LoginService $loginService) { $this->config = config('saml2'); $this->registrationService = $registrationService; - $this->user = $user; + $this->loginService = $loginService; } /** @@ -332,7 +332,7 @@ class Saml2Service extends ExternalAuthService */ protected function getOrRegisterUser(array $userDetails): ?User { - $user = $this->user->newQuery() + $user = User::query() ->where('external_auth_id', '=', $userDetails['external_id']) ->first(); @@ -389,10 +389,7 @@ class Saml2Service extends ExternalAuthService $this->syncWithGroups($user, $groups); } - auth()->login($user); - Activity::add(ActivityType::AUTH_LOGIN, "saml2; {$user->logDescriptor()}"); - Theme::dispatch(ThemeEvents::AUTH_LOGIN, 'saml2', $user); - + $this->loginService->login($user, 'saml2'); return $user; } } diff --git a/app/Auth/Access/SocialAuthService.php b/app/Auth/Access/SocialAuthService.php index 2f1a6876a..70f3fc3d3 100644 --- a/app/Auth/Access/SocialAuthService.php +++ b/app/Auth/Access/SocialAuthService.php @@ -2,15 +2,11 @@ namespace BookStack\Auth\Access; -use BookStack\Actions\ActivityType; use BookStack\Auth\SocialAccount; use BookStack\Auth\User; use BookStack\Exceptions\SocialDriverNotConfigured; use BookStack\Exceptions\SocialSignInAccountNotUsed; use BookStack\Exceptions\UserRegistrationException; -use BookStack\Facades\Activity; -use BookStack\Facades\Theme; -use BookStack\Theming\ThemeEvents; use Illuminate\Support\Facades\Event; use Illuminate\Support\Str; use Laravel\Socialite\Contracts\Factory as Socialite; @@ -28,6 +24,11 @@ class SocialAuthService */ protected $socialite; + /** + * @var LoginService + */ + protected $loginService; + /** * The default built-in social drivers we support. * @@ -59,9 +60,10 @@ class SocialAuthService /** * SocialAuthService constructor. */ - public function __construct(Socialite $socialite) + public function __construct(Socialite $socialite, LoginService $loginService) { $this->socialite = $socialite; + $this->loginService = $loginService; } /** @@ -139,10 +141,7 @@ class SocialAuthService // When a user is not logged in and a matching SocialAccount exists, // Simply log the user into the application. if (!$isLoggedIn && $socialAccount !== null) { - auth()->login($socialAccount->user); - Activity::add(ActivityType::AUTH_LOGIN, $socialAccount); - Theme::dispatch(ThemeEvents::AUTH_LOGIN, $socialDriver, $socialAccount->user); - + $this->loginService->login($socialAccount->user, $socialAccount); return redirect()->intended('/'); } diff --git a/app/Http/Controllers/Auth/ConfirmEmailController.php b/app/Http/Controllers/Auth/ConfirmEmailController.php index 63f220a68..90e5fb76d 100644 --- a/app/Http/Controllers/Auth/ConfirmEmailController.php +++ b/app/Http/Controllers/Auth/ConfirmEmailController.php @@ -4,6 +4,7 @@ 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; @@ -20,14 +21,20 @@ use Illuminate\View\View; class ConfirmEmailController extends Controller { protected $emailConfirmationService; + protected $loginService; protected $userRepo; /** * Create a new controller instance. */ - public function __construct(EmailConfirmationService $emailConfirmationService, UserRepo $userRepo) + public function __construct( + EmailConfirmationService $emailConfirmationService, + LoginService $loginService, + UserRepo $userRepo + ) { $this->emailConfirmationService = $emailConfirmationService; + $this->loginService = $loginService; $this->userRepo = $userRepo; } @@ -87,9 +94,7 @@ class ConfirmEmailController extends Controller $user->email_confirmed = true; $user->save(); - auth()->login($user); - Theme::dispatch(ThemeEvents::AUTH_LOGIN, auth()->getDefaultDriver(), $user); - $this->logActivity(ActivityType::AUTH_LOGIN, $user); + $this->loginService->login($user, auth()->getDefaultDriver()); $this->showSuccessNotification(trans('auth.email_confirm_success')); $this->emailConfirmationService->deleteByUser($user); diff --git a/app/Http/Controllers/Auth/LoginController.php b/app/Http/Controllers/Auth/LoginController.php index 5154f7e97..174770bf9 100644 --- a/app/Http/Controllers/Auth/LoginController.php +++ b/app/Http/Controllers/Auth/LoginController.php @@ -3,13 +3,11 @@ namespace BookStack\Http\Controllers\Auth; use Activity; -use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\SocialAuthService; use BookStack\Exceptions\LoginAttemptEmailNeededException; use BookStack\Exceptions\LoginAttemptException; -use BookStack\Facades\Theme; use BookStack\Http\Controllers\Controller; -use BookStack\Theming\ThemeEvents; use Illuminate\Foundation\Auth\AuthenticatesUsers; use Illuminate\Http\Request; use Illuminate\Validation\ValidationException; @@ -37,16 +35,19 @@ class LoginController extends Controller protected $redirectAfterLogout = '/login'; protected $socialAuthService; + protected $loginService; /** * Create a new controller instance. */ - public function __construct(SocialAuthService $socialAuthService) + public function __construct(SocialAuthService $socialAuthService, LoginService $loginService) { $this->middleware('guest', ['only' => ['getLogin', 'login']]); $this->middleware('guard:standard,ldap', ['only' => ['login', 'logout']]); $this->socialAuthService = $socialAuthService; + $this->loginService = $loginService; + $this->redirectPath = url('/'); $this->redirectAfterLogout = url('/login'); } @@ -140,6 +141,19 @@ class LoginController extends Controller return $this->sendFailedLoginResponse($request); } + /** + * Attempt to log the user into the application. + * + * @param \Illuminate\Http\Request $request + * @return bool + */ + protected function attemptLogin(Request $request) + { + return $this->loginService->attempt( + $this->credentials($request), auth()->getDefaultDriver(), $request->filled('remember') + ); + } + /** * The user has been authenticated. * @@ -150,17 +164,6 @@ class LoginController extends Controller */ protected function authenticated(Request $request, $user) { - // Authenticate on all session guards if a likely admin - if ($user->can('users-manage') && $user->can('user-roles-manage')) { - $guards = ['standard', 'ldap', 'saml2']; - foreach ($guards as $guard) { - auth($guard)->login($user); - } - } - - Theme::dispatch(ThemeEvents::AUTH_LOGIN, auth()->getDefaultDriver(), $user); - $this->logActivity(ActivityType::AUTH_LOGIN, $user); - return redirect()->intended($this->redirectPath()); } diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php index 1728ece32..4008439bc 100644 --- a/app/Http/Controllers/Auth/RegisterController.php +++ b/app/Http/Controllers/Auth/RegisterController.php @@ -2,14 +2,12 @@ namespace BookStack\Http\Controllers\Auth; -use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\RegistrationService; use BookStack\Auth\Access\SocialAuthService; use BookStack\Auth\User; use BookStack\Exceptions\UserRegistrationException; -use BookStack\Facades\Theme; use BookStack\Http\Controllers\Controller; -use BookStack\Theming\ThemeEvents; use Illuminate\Foundation\Auth\RegistersUsers; use Illuminate\Http\Request; use Illuminate\Support\Facades\Hash; @@ -32,6 +30,7 @@ class RegisterController extends Controller protected $socialAuthService; protected $registrationService; + protected $loginService; /** * Where to redirect users after login / registration. @@ -44,13 +43,18 @@ class RegisterController extends Controller /** * Create a new controller instance. */ - public function __construct(SocialAuthService $socialAuthService, RegistrationService $registrationService) + public function __construct( + SocialAuthService $socialAuthService, + RegistrationService $registrationService, + LoginService $loginService + ) { $this->middleware('guest'); $this->middleware('guard:standard'); $this->socialAuthService = $socialAuthService; $this->registrationService = $registrationService; + $this->loginService = $loginService; $this->redirectTo = url('/'); $this->redirectPath = url('/'); @@ -98,9 +102,7 @@ class RegisterController extends Controller try { $user = $this->registrationService->registerUser($userData); - auth()->login($user); - Theme::dispatch(ThemeEvents::AUTH_LOGIN, auth()->getDefaultDriver(), $user); - $this->logActivity(ActivityType::AUTH_LOGIN, $user); + $this->loginService->login($user, auth()->getDefaultDriver()); } catch (UserRegistrationException $exception) { if ($exception->getMessage()) { $this->showErrorNotification($exception->getMessage()); diff --git a/app/Http/Controllers/Auth/SocialController.php b/app/Http/Controllers/Auth/SocialController.php index 1d1468698..dd567fe18 100644 --- a/app/Http/Controllers/Auth/SocialController.php +++ b/app/Http/Controllers/Auth/SocialController.php @@ -2,16 +2,14 @@ namespace BookStack\Http\Controllers\Auth; -use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\RegistrationService; use BookStack\Auth\Access\SocialAuthService; use BookStack\Exceptions\SocialDriverNotConfigured; use BookStack\Exceptions\SocialSignInAccountNotUsed; use BookStack\Exceptions\SocialSignInException; use BookStack\Exceptions\UserRegistrationException; -use BookStack\Facades\Theme; use BookStack\Http\Controllers\Controller; -use BookStack\Theming\ThemeEvents; use Illuminate\Http\Request; use Illuminate\Support\Str; use Laravel\Socialite\Contracts\User as SocialUser; @@ -20,15 +18,21 @@ class SocialController extends Controller { protected $socialAuthService; protected $registrationService; + protected $loginService; /** * SocialController constructor. */ - public function __construct(SocialAuthService $socialAuthService, RegistrationService $registrationService) + public function __construct( + SocialAuthService $socialAuthService, + RegistrationService $registrationService, + LoginService $loginService + ) { $this->middleware('guest')->only(['getRegister', 'postRegister']); $this->socialAuthService = $socialAuthService; $this->registrationService = $registrationService; + $this->loginService = $loginService; } /** @@ -136,12 +140,9 @@ class SocialController extends Controller } $user = $this->registrationService->registerUser($userData, $socialAccount, $emailVerified); - auth()->login($user); - Theme::dispatch(ThemeEvents::AUTH_LOGIN, $socialDriver, $user); - $this->logActivity(ActivityType::AUTH_LOGIN, $user); + $this->loginService->login($user, $socialDriver); $this->showSuccessNotification(trans('auth.register_success')); - return redirect('/'); } } diff --git a/app/Http/Controllers/Auth/UserInviteController.php b/app/Http/Controllers/Auth/UserInviteController.php index 253e25507..1cc59d6ba 100644 --- a/app/Http/Controllers/Auth/UserInviteController.php +++ b/app/Http/Controllers/Auth/UserInviteController.php @@ -3,6 +3,7 @@ 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; @@ -18,17 +19,19 @@ use Illuminate\Routing\Redirector; class UserInviteController extends Controller { protected $inviteService; + protected $loginService; protected $userRepo; /** * Create a new controller instance. */ - public function __construct(UserInviteService $inviteService, UserRepo $userRepo) + public function __construct(UserInviteService $inviteService, LoginService $loginService, UserRepo $userRepo) { $this->middleware('guest'); $this->middleware('guard:standard'); $this->inviteService = $inviteService; + $this->loginService = $loginService; $this->userRepo = $userRepo; } @@ -72,9 +75,7 @@ class UserInviteController extends Controller $user->email_confirmed = true; $user->save(); - auth()->login($user); - Theme::dispatch(ThemeEvents::AUTH_LOGIN, auth()->getDefaultDriver(), $user); - $this->logActivity(ActivityType::AUTH_LOGIN, $user); + $this->loginService->login($user, auth()->getDefaultDriver()); $this->showSuccessNotification(trans('auth.user_invite_success', ['appName' => setting('app-name')])); $this->inviteService->deleteByUser($user); diff --git a/app/Http/Middleware/EnforceMfaRequirements.php b/app/Http/Middleware/EnforceMfaRequirements.php index ac3c9609b..1877e5672 100644 --- a/app/Http/Middleware/EnforceMfaRequirements.php +++ b/app/Http/Middleware/EnforceMfaRequirements.php @@ -31,7 +31,7 @@ class EnforceMfaRequirements && !$request->is('mfa/verify*', 'uploads/images/user/*') && $this->mfaSession->requiredForCurrentUser() ) { - return redirect('/mfa/verify'); +// return redirect('/mfa/verify'); } // TODO - URI wildcard exceptions above allow access to the 404 page of this user diff --git a/app/Providers/AppServiceProvider.php b/app/Providers/AppServiceProvider.php index abee6bcda..f7c8fe492 100644 --- a/app/Providers/AppServiceProvider.php +++ b/app/Providers/AppServiceProvider.php @@ -3,6 +3,7 @@ namespace BookStack\Providers; use Blade; +use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\SocialAuthService; use BookStack\Entities\BreadcrumbsViewComposer; use BookStack\Entities\Models\Book; @@ -68,7 +69,7 @@ class AppServiceProvider extends ServiceProvider }); $this->app->singleton(SocialAuthService::class, function ($app) { - return new SocialAuthService($app->make(SocialiteFactory::class)); + return new SocialAuthService($app->make(SocialiteFactory::class), $app->make(LoginService::class)); }); } } From 1278fb4969f87d2433a6e1e1f70d63f0e9a41d30 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 17 Jul 2021 18:24:50 +0100 Subject: [PATCH 13/25] 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 @@ -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 @@ -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') From 1af5bbf3f7404ef9380477657ac1b5df0df119aa Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 18 Jul 2021 16:52:31 +0100 Subject: [PATCH 14/25] Added login redirect system to confirm/mfa 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 | 79 +++++++++++++++---- .../StoppedAuthenticationException.php | 40 ++++++++++ .../Auth/ConfirmEmailController.php | 5 +- .../Controllers/Auth/HandlesPartialLogins.php | 22 ++++++ .../Auth/MfaBackupCodesController.php | 4 +- app/Http/Controllers/Auth/MfaController.php | 22 +++++- .../Controllers/Auth/MfaTotpController.php | 6 +- .../Controllers/Auth/SocialController.php | 2 +- .../views/auth/user-unconfirmed.blade.php | 4 +- resources/views/mfa/verify.blade.php | 23 +++++- routes/web.php | 16 ++-- 11 files changed, 186 insertions(+), 37 deletions(-) create mode 100644 app/Exceptions/StoppedAuthenticationException.php create mode 100644 app/Http/Controllers/Auth/HandlesPartialLogins.php 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'); From a3f19ebe96063eae54384fd56d2da8b66e625c14 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 2 Aug 2021 15:04:43 +0100 Subject: [PATCH 15/25] Added TOTP verification upon access --- app/Auth/Access/LoginService.php | 2 +- app/Auth/Access/Mfa/MfaValue.php | 18 ++++- .../Controllers/Auth/HandlesPartialLogins.php | 3 + .../Controllers/Auth/MfaTotpController.php | 25 +++++++ resources/views/mfa/verify.blade.php | 19 ++--- resources/views/mfa/verify/totp.blade.php | 20 +++++ routes/web.php | 1 + tests/Auth/MfaVerificationTest.php | 75 +++++++++++++++++++ 8 files changed, 146 insertions(+), 17 deletions(-) create mode 100644 resources/views/mfa/verify/totp.blade.php create mode 100644 tests/Auth/MfaVerificationTest.php diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php index 86126c3b6..9823caafa 100644 --- a/app/Auth/Access/LoginService.php +++ b/app/Auth/Access/LoginService.php @@ -70,7 +70,7 @@ class LoginService */ public function reattemptLoginFor(User $user, string $method) { - if ($user->id !== $this->getLastLoginAttemptUser()) { + if ($user->id !== ($this->getLastLoginAttemptUser()->id ?? null)) { throw new Exception('Login reattempt user does align with current session state'); } diff --git a/app/Auth/Access/Mfa/MfaValue.php b/app/Auth/Access/Mfa/MfaValue.php index 9f9ab29a5..ec0d5d563 100644 --- a/app/Auth/Access/Mfa/MfaValue.php +++ b/app/Auth/Access/Mfa/MfaValue.php @@ -44,10 +44,24 @@ class MfaValue extends Model $mfaVal->save(); } + /** + * Easily get the decrypted MFA value for the given user and method. + */ + public static function getValueForUser(User $user, string $method): ?string + { + /** @var MfaValue $mfaVal */ + $mfaVal = static::query() + ->where('user_id', '=', $user->id) + ->where('method', '=', $method) + ->first(); + + return $mfaVal ? $mfaVal->getValue() : null; + } + /** * Decrypt the value attribute upon access. */ - public function getValue(): string + protected function getValue(): string { return decrypt($this->value); } @@ -55,7 +69,7 @@ class MfaValue extends Model /** * Encrypt the value attribute upon access. */ - public function setValue($value): void + protected function setValue($value): void { $this->value = encrypt($value); } diff --git a/app/Http/Controllers/Auth/HandlesPartialLogins.php b/app/Http/Controllers/Auth/HandlesPartialLogins.php index f9bacb95d..4ce67d236 100644 --- a/app/Http/Controllers/Auth/HandlesPartialLogins.php +++ b/app/Http/Controllers/Auth/HandlesPartialLogins.php @@ -8,6 +8,9 @@ use BookStack\Exceptions\NotFoundException; trait HandlesPartialLogins { + /** + * @throws NotFoundException + */ protected function currentOrLastAttemptedUser(): User { $loginService = app()->make(LoginService::class); diff --git a/app/Http/Controllers/Auth/MfaTotpController.php b/app/Http/Controllers/Auth/MfaTotpController.php index 828af6b03..dd1651970 100644 --- a/app/Http/Controllers/Auth/MfaTotpController.php +++ b/app/Http/Controllers/Auth/MfaTotpController.php @@ -3,6 +3,8 @@ namespace BookStack\Http\Controllers\Auth; use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\LoginService; +use BookStack\Auth\Access\Mfa\MfaSession; use BookStack\Auth\Access\Mfa\MfaValue; use BookStack\Auth\Access\Mfa\TotpService; use BookStack\Auth\Access\Mfa\TotpValidationRule; @@ -61,4 +63,27 @@ class MfaTotpController extends Controller return redirect('/mfa/setup'); } + + /** + * Verify the MFA method submission on check. + * @throws NotFoundException + */ + public function verify(Request $request, LoginService $loginService, MfaSession $mfaSession) + { + $user = $this->currentOrLastAttemptedUser(); + $totpSecret = MfaValue::getValueForUser($user, MfaValue::METHOD_TOTP); + + $this->validate($request, [ + 'code' => [ + 'required', + 'max:12', 'min:4', + new TotpValidationRule($totpSecret), + ] + ]); + + $mfaSession->markVerifiedForUser($user); + $loginService->reattemptLoginFor($user, 'mfa-totp'); + + return redirect()->intended(); + } } diff --git a/resources/views/mfa/verify.blade.php b/resources/views/mfa/verify.blade.php index 58c0c42de..e818852c2 100644 --- a/resources/views/mfa/verify.blade.php +++ b/resources/views/mfa/verify.blade.php @@ -19,24 +19,15 @@ You'll need to set up at least one method before you gain access.

@endif -
-
-
-
METHOD A
-

- ... -

-
-
- BUTTON -
-
-
+ @if($method) +
+ @include('mfa.verify.' . $method) + @endif @if(count($otherMethods) > 0)
diff --git a/resources/views/mfa/verify/totp.blade.php b/resources/views/mfa/verify/totp.blade.php new file mode 100644 index 000000000..7ff691552 --- /dev/null +++ b/resources/views/mfa/verify/totp.blade.php @@ -0,0 +1,20 @@ +
Mobile App
+ +

+ Enter the code, generated using your mobile app, below: +

+ +
+ {{ csrf_field() }} + + @if($errors->has('code')) +
{{ $errors->first('code') }}
+ @endif +
+ +
+
\ No newline at end of file diff --git a/routes/web.php b/routes/web.php index 437b4b03b..029649685 100644 --- a/routes/web.php +++ b/routes/web.php @@ -235,6 +235,7 @@ 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'); +Route::post('/mfa/verify/totp', 'Auth\MfaTotpController@verify'); // Social auth routes Route::get('/login/service/{socialDriver}', 'Auth\SocialController@login'); diff --git a/tests/Auth/MfaVerificationTest.php b/tests/Auth/MfaVerificationTest.php new file mode 100644 index 000000000..f19700a5a --- /dev/null +++ b/tests/Auth/MfaVerificationTest.php @@ -0,0 +1,75 @@ +startTotpLogin(); + $loginResp->assertRedirect('/mfa/verify'); + + $resp = $this->get('/mfa/verify'); + $resp->assertSee('Verify Access'); + $resp->assertSee('Enter the code, generated using your mobile app, below:'); + $resp->assertElementExists('form[action$="/mfa/verify/totp"] input[name="code"]'); + + $google2fa = new Google2FA(); + $resp = $this->post('/mfa/verify/totp', [ + 'code' => $google2fa->getCurrentOtp($secret), + ]); + $resp->assertRedirect('/'); + $this->assertEquals($user->id, auth()->user()->id); + } + + public function test_totp_verification_fails_on_missing_invalid_code() + { + [$user, $secret, $loginResp] = $this->startTotpLogin(); + + $resp = $this->get('/mfa/verify'); + $resp = $this->post('/mfa/verify/totp', [ + 'code' => '', + ]); + $resp->assertRedirect('/mfa/verify'); + + $resp = $this->get('/mfa/verify'); + $resp->assertSeeText('The code field is required.'); + $this->assertNull(auth()->user()); + + $resp = $this->post('/mfa/verify/totp', [ + 'code' => '123321', + ]); + $resp->assertRedirect('/mfa/verify'); + $resp = $this->get('/mfa/verify'); + + $resp->assertSeeText('The provided code is not valid or has expired.'); + $this->assertNull(auth()->user()); + } + + /** + * @return Array + */ + protected function startTotpLogin(): array + { + $secret = $this->app->make(TotpService::class)->generateSecret(); + $user = $this->getEditor(); + $user->password = Hash::make('password'); + $user->save(); + MfaValue::upsertWithValue($user, MfaValue::METHOD_TOTP, $secret); + $loginResp = $this->post('/login', [ + 'email' => $user->email, + 'password' => 'password', + ]); + + return [$user, $secret, $loginResp]; + } + +} \ No newline at end of file From 459706908311101286dadb87a2f12afbf4192bbb Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 2 Aug 2021 16:35:37 +0100 Subject: [PATCH 16/25] Added Backup code verification logic Also added testing to cover as part of this in addition to adding the core backup code handling required. Also added the standardised translations for switching mfa mode and adding testing for this switching. --- app/Auth/Access/LoginService.php | 2 + app/Auth/Access/Mfa/BackupCodeService.php | 49 ++++++- app/Auth/Access/Mfa/MfaValue.php | 11 ++ .../Auth/MfaBackupCodesController.php | 40 ++++++ app/Http/Controllers/Auth/MfaController.php | 3 +- resources/lang/en/auth.php | 6 +- resources/lang/en/validation.php | 1 + resources/views/mfa/verify.blade.php | 4 +- .../views/mfa/verify/backup_codes.blade.php | 19 +++ resources/views/mfa/verify/totp.blade.php | 1 - routes/web.php | 1 + tests/Auth/MfaVerificationTest.php | 125 ++++++++++++++++++ 12 files changed, 255 insertions(+), 7 deletions(-) create mode 100644 resources/views/mfa/verify/backup_codes.blade.php diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php index 9823caafa..cc0cb06f3 100644 --- a/app/Auth/Access/LoginService.php +++ b/app/Auth/Access/LoginService.php @@ -37,6 +37,8 @@ class LoginService throw new StoppedAuthenticationException($user, $this); // TODO - Does 'remember' still work? Probably not right now. + // TODO - Need to clear MFA sessions out upon logout + // Old MFA middleware todos: // TODO - Need to redirect to setup if not configured AND ONLY IF NO OPTIONS CONFIGURED diff --git a/app/Auth/Access/Mfa/BackupCodeService.php b/app/Auth/Access/Mfa/BackupCodeService.php index cc533bd31..6fbbf64b5 100644 --- a/app/Auth/Access/Mfa/BackupCodeService.php +++ b/app/Auth/Access/Mfa/BackupCodeService.php @@ -12,10 +12,55 @@ class BackupCodeService public function generateNewSet(): array { $codes = []; - for ($i = 0; $i < 16; $i++) { + while (count($codes) < 16) { $code = Str::random(5) . '-' . Str::random(5); - $codes[] = strtolower($code); + if (!in_array($code, $codes)) { + $codes[] = strtolower($code); + } } + return $codes; } + + /** + * Check if the given code matches one of the available options. + */ + public function inputCodeExistsInSet(string $code, string $codeSet): bool + { + $cleanCode = $this->cleanInputCode($code); + $codes = json_decode($codeSet); + return in_array($cleanCode, $codes); + } + + /** + * Remove the given input code from the given available options. + * Will return null if no codes remain otherwise will be a JSON string to contain + * the codes. + */ + public function removeInputCodeFromSet(string $code, string $codeSet): ?string + { + $cleanCode = $this->cleanInputCode($code); + $codes = json_decode($codeSet); + $pos = array_search($cleanCode, $codes, true); + array_splice($codes, $pos, 1); + + if (count($codes) === 0) { + return null; + } + + return json_encode($codes); + } + + /** + * Count the number of codes in the given set. + */ + public function countCodesInSet(string $codeSet): int + { + return count(json_decode($codeSet)); + } + + protected function cleanInputCode(string $code): string + { + return strtolower(str_replace(' ', '-', trim($code))); + } } \ No newline at end of file diff --git a/app/Auth/Access/Mfa/MfaValue.php b/app/Auth/Access/Mfa/MfaValue.php index ec0d5d563..228a6d43f 100644 --- a/app/Auth/Access/Mfa/MfaValue.php +++ b/app/Auth/Access/Mfa/MfaValue.php @@ -58,6 +58,17 @@ class MfaValue extends Model return $mfaVal ? $mfaVal->getValue() : null; } + /** + * Delete any stored MFA values for the given user and method. + */ + public static function deleteValuesForUser(User $user, string $method): void + { + static::query() + ->where('user_id', '=', $user->id) + ->where('method', '=', $method) + ->delete(); + } + /** * Decrypt the value attribute upon access. */ diff --git a/app/Http/Controllers/Auth/MfaBackupCodesController.php b/app/Http/Controllers/Auth/MfaBackupCodesController.php index b89050565..1353d4562 100644 --- a/app/Http/Controllers/Auth/MfaBackupCodesController.php +++ b/app/Http/Controllers/Auth/MfaBackupCodesController.php @@ -3,10 +3,15 @@ namespace BookStack\Http\Controllers\Auth; use BookStack\Actions\ActivityType; +use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\Mfa\BackupCodeService; +use BookStack\Auth\Access\Mfa\MfaSession; use BookStack\Auth\Access\Mfa\MfaValue; +use BookStack\Exceptions\NotFoundException; use BookStack\Http\Controllers\Controller; use Exception; +use Illuminate\Http\Request; +use Illuminate\Validation\ValidationException; class MfaBackupCodesController extends Controller { @@ -46,4 +51,39 @@ class MfaBackupCodesController extends Controller $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'backup-codes'); return redirect('/mfa/setup'); } + + /** + * Verify the MFA method submission on check. + * @throws NotFoundException + * @throws ValidationException + */ + public function verify(Request $request, BackupCodeService $codeService, MfaSession $mfaSession, LoginService $loginService) + { + $user = $this->currentOrLastAttemptedUser(); + $codes = MfaValue::getValueForUser($user, MfaValue::METHOD_BACKUP_CODES) ?? '[]'; + + $this->validate($request, [ + 'code' => [ + 'required', + 'max:12', 'min:8', + function ($attribute, $value, $fail) use ($codeService, $codes) { + if (!$codeService->inputCodeExistsInSet($value, $codes)) { + $fail(trans('validation.backup_codes')); + } + } + ] + ]); + + $updatedCodes = $codeService->removeInputCodeFromSet($request->get('code'), $codes); + MfaValue::upsertWithValue($user, MfaValue::METHOD_BACKUP_CODES, $updatedCodes); + + $mfaSession->markVerifiedForUser($user); + $loginService->reattemptLoginFor($user, 'mfa-backup_codes'); + + 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.'); + } + + return redirect()->intended(); + } } diff --git a/app/Http/Controllers/Auth/MfaController.php b/app/Http/Controllers/Auth/MfaController.php index 6d868b6f3..75cd46eef 100644 --- a/app/Http/Controllers/Auth/MfaController.php +++ b/app/Http/Controllers/Auth/MfaController.php @@ -5,7 +5,7 @@ namespace BookStack\Http\Controllers\Auth; use BookStack\Actions\ActivityType; use BookStack\Auth\Access\Mfa\MfaValue; use BookStack\Http\Controllers\Controller; -use BookStack\Http\Request; +use Illuminate\Http\Request; class MfaController extends Controller { @@ -47,7 +47,6 @@ class MfaController extends Controller */ public function verify(Request $request) { - // TODO - Test this $desiredMethod = $request->get('method'); $userMethods = $this->currentOrLastAttemptedUser() ->mfaValues() diff --git a/resources/lang/en/auth.php b/resources/lang/en/auth.php index d64fce93a..2159cfb3b 100644 --- a/resources/lang/en/auth.php +++ b/resources/lang/en/auth.php @@ -73,5 +73,9 @@ return [ 'user_invite_page_welcome' => 'Welcome to :appName!', 'user_invite_page_text' => 'To finalise your account and gain access you need to set a password which will be used to log-in to :appName on future visits.', 'user_invite_page_confirm_button' => 'Confirm Password', - 'user_invite_success' => 'Password set, you now have access to :appName!' + 'user_invite_success' => 'Password set, you now have access to :appName!', + + // Multi-factor Authentication + 'mfa_use_totp' => 'Verify using a mobile app', + 'mfa_use_backup_codes' => 'Verify using a backup code', ]; \ No newline at end of file diff --git a/resources/lang/en/validation.php b/resources/lang/en/validation.php index 5796d04c6..1963b0df2 100644 --- a/resources/lang/en/validation.php +++ b/resources/lang/en/validation.php @@ -15,6 +15,7 @@ return [ 'alpha_dash' => 'The :attribute may only contain letters, numbers, dashes and underscores.', 'alpha_num' => 'The :attribute may only contain letters and numbers.', 'array' => 'The :attribute must be an array.', + 'backup_codes' => 'The provided code is not valid or has already been used.', 'before' => 'The :attribute must be a date before :date.', 'between' => [ 'numeric' => 'The :attribute must be between :min and :max.', diff --git a/resources/views/mfa/verify.blade.php b/resources/views/mfa/verify.blade.php index e818852c2..7cab2edc4 100644 --- a/resources/views/mfa/verify.blade.php +++ b/resources/views/mfa/verify.blade.php @@ -32,7 +32,9 @@ @if(count($otherMethods) > 0)
@foreach($otherMethods as $otherMethod) - Use {{$otherMethod}} +
+ {{ trans('auth.mfa_use_' . $otherMethod) }} +
@endforeach @endif diff --git a/resources/views/mfa/verify/backup_codes.blade.php b/resources/views/mfa/verify/backup_codes.blade.php new file mode 100644 index 000000000..abfb8a9d0 --- /dev/null +++ b/resources/views/mfa/verify/backup_codes.blade.php @@ -0,0 +1,19 @@ +
Backup Code
+ +

+ Enter one of your remaining backup codes below: +

+ +
+ {{ csrf_field() }} + + @if($errors->has('code')) +
{{ $errors->first('code') }}
+ @endif +
+ +
+
\ No newline at end of file diff --git a/resources/views/mfa/verify/totp.blade.php b/resources/views/mfa/verify/totp.blade.php index 7ff691552..55784dc8f 100644 --- a/resources/views/mfa/verify/totp.blade.php +++ b/resources/views/mfa/verify/totp.blade.php @@ -8,7 +8,6 @@ {{ csrf_field() }} @if($errors->has('code')) diff --git a/routes/web.php b/routes/web.php index 029649685..eadbca5e8 100644 --- a/routes/web.php +++ b/routes/web.php @@ -236,6 +236,7 @@ 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'); Route::post('/mfa/verify/totp', 'Auth\MfaTotpController@verify'); +Route::post('/mfa/verify/backup_codes', 'Auth\MfaBackupCodesController@verify'); // Social auth routes Route::get('/login/service/{socialDriver}', 'Auth\SocialController@login'); diff --git a/tests/Auth/MfaVerificationTest.php b/tests/Auth/MfaVerificationTest.php index f19700a5a..3f272cffb 100644 --- a/tests/Auth/MfaVerificationTest.php +++ b/tests/Auth/MfaVerificationTest.php @@ -54,6 +54,114 @@ class MfaVerificationTest extends TestCase $this->assertNull(auth()->user()); } + public function test_backup_code_verification() + { + [$user, $codes, $loginResp] = $this->startBackupCodeLogin(); + $loginResp->assertRedirect('/mfa/verify'); + + $resp = $this->get('/mfa/verify'); + $resp->assertSee('Verify Access'); + $resp->assertSee('Backup Code'); + $resp->assertSee('Enter one of your remaining backup codes below:'); + $resp->assertElementExists('form[action$="/mfa/verify/backup_codes"] input[name="code"]'); + + $resp = $this->post('/mfa/verify/backup_codes', [ + 'code' => $codes[1], + ]); + + $resp->assertRedirect('/'); + $this->assertEquals($user->id, auth()->user()->id); + // Ensure code no longer exists in available set + $userCodes = MfaValue::getValueForUser($user, MfaValue::METHOD_BACKUP_CODES); + $this->assertStringNotContainsString($codes[1], $userCodes); + $this->assertStringContainsString($codes[0], $userCodes); + } + + public function test_backup_code_verification_fails_on_missing_or_invalid_code() + { + [$user, $codes, $loginResp] = $this->startBackupCodeLogin(); + + $resp = $this->get('/mfa/verify'); + $resp = $this->post('/mfa/verify/backup_codes', [ + 'code' => '', + ]); + $resp->assertRedirect('/mfa/verify'); + + $resp = $this->get('/mfa/verify'); + $resp->assertSeeText('The code field is required.'); + $this->assertNull(auth()->user()); + + $resp = $this->post('/mfa/verify/backup_codes', [ + 'code' => 'ab123-ab456', + ]); + $resp->assertRedirect('/mfa/verify'); + + $resp = $this->get('/mfa/verify'); + $resp->assertSeeText('The provided code is not valid or has already been used.'); + $this->assertNull(auth()->user()); + } + + public function test_backup_code_verification_fails_on_attempted_code_reuse() + { + [$user, $codes, $loginResp] = $this->startBackupCodeLogin(); + + $this->post('/mfa/verify/backup_codes', [ + 'code' => $codes[0], + ]); + $this->assertNotNull(auth()->user()); + auth()->logout(); + session()->flush(); + + $this->post('/login', ['email' => $user->email, 'password' => 'password']); + $this->get('/mfa/verify'); + $resp = $this->post('/mfa/verify/backup_codes', [ + 'code' => $codes[0], + ]); + $resp->assertRedirect('/mfa/verify'); + $this->assertNull(auth()->user()); + + $resp = $this->get('/mfa/verify'); + $resp->assertSeeText('The provided code is not valid or has already been used.'); + } + + public function test_backup_code_verification_shows_warning_when_limited_codes_remain() + { + [$user, $codes, $loginResp] = $this->startBackupCodeLogin(['abc12-def45', 'abc12-def46']); + + $resp = $this->post('/mfa/verify/backup_codes', [ + 'code' => $codes[0], + ]); + $resp = $this->followRedirects($resp); + $resp->assertSeeText('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.'); + } + + public function test_both_mfa_options_available_if_set_on_profile() + { + $user = $this->getEditor(); + $user->password = Hash::make('password'); + $user->save(); + + MfaValue::upsertWithValue($user, MfaValue::METHOD_TOTP, 'abc123'); + MfaValue::upsertWithValue($user, MfaValue::METHOD_BACKUP_CODES, '["abc12-def456"]'); + + /** @var TestResponse $mfaView */ + $mfaView = $this->followingRedirects()->post('/login', [ + 'email' => $user->email, + 'password' => 'password', + ]); + + // Totp shown by default + $mfaView->assertElementExists('form[action$="/mfa/verify/totp"] input[name="code"]'); + $mfaView->assertElementContains('a[href$="/mfa/verify?method=backup_codes"]', 'Verify using a backup code'); + + // Ensure can view backup_codes view + $resp = $this->get('/mfa/verify?method=backup_codes'); + $resp->assertElementExists('form[action$="/mfa/verify/backup_codes"] input[name="code"]'); + $resp->assertElementContains('a[href$="/mfa/verify?method=totp"]', 'Verify using a mobile app'); + } + + // TODO !! - Test no-existing MFA + /** * @return Array */ @@ -72,4 +180,21 @@ class MfaVerificationTest extends TestCase return [$user, $secret, $loginResp]; } + /** + * @return Array + */ + protected function startBackupCodeLogin($codes = ['kzzu6-1pgll','bzxnf-plygd','bwdsp-ysl51','1vo93-ioy7n','lf7nw-wdyka','xmtrd-oplac']): array + { + $user = $this->getEditor(); + $user->password = Hash::make('password'); + $user->save(); + MfaValue::upsertWithValue($user, MfaValue::METHOD_BACKUP_CODES, json_encode($codes)); + $loginResp = $this->post('/login', [ + 'email' => $user->email, + 'password' => 'password', + ]); + + return [$user, $codes, $loginResp]; + } + } \ No newline at end of file From 9b271e559fd0ca98319cf5ba0d7c26916511b62a Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 2 Aug 2021 22:02:25 +0100 Subject: [PATCH 17/25] Worked on MFA setup required flow - Restructured some of the route naming to be a little more consistent. - Moved the routes about to be more logically in one place. - Created a new middleware to handle the auth of people that should be allowed access to mfa setup routes, since these could be used by existing logged in users or by people needing to setup MFA on access. - Added testing to cover MFA setup required flow. - Added TTL and method tracking to session last-login tracking system. --- app/Auth/Access/LoginService.php | 54 ++++++--- app/Auth/Access/Mfa/MfaSession.php | 9 ++ .../Auth/MfaBackupCodesController.php | 2 +- .../Controllers/Auth/MfaTotpController.php | 2 +- app/Http/Kernel.php | 1 + app/Http/Middleware/Authenticate.php | 3 +- .../Middleware/AuthenticatedOrPendingMfa.php | 41 +++++++ .../views/mfa/backup-codes-generate.blade.php | 2 +- resources/views/mfa/setup.blade.php | 8 +- resources/views/mfa/totp-generate.blade.php | 2 +- .../views/mfa/verify/backup_codes.blade.php | 2 +- resources/views/mfa/verify/totp.blade.php | 2 +- routes/web.php | 29 ++--- tests/Auth/MfaConfigurationTest.php | 26 ++--- tests/Auth/MfaVerificationTest.php | 103 +++++++++++++++--- 15 files changed, 218 insertions(+), 68 deletions(-) create mode 100644 app/Http/Middleware/AuthenticatedOrPendingMfa.php 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

-
+ {{ csrf_field() }}
{{ trans('common.cancel') }} diff --git a/resources/views/mfa/setup.blade.php b/resources/views/mfa/setup.blade.php index 2ec8d0f77..f670541bf 100644 --- a/resources/views/mfa/setup.blade.php +++ b/resources/views/mfa/setup.blade.php @@ -25,7 +25,7 @@ @icon('check-circle') Already configured
- Reconfigure + Reconfigure
@@ -38,7 +38,7 @@
@else - Setup + Setup @endif
@@ -57,7 +57,7 @@ @icon('check-circle') Already configured
- Reconfigure + Reconfigure
@@ -70,7 +70,7 @@
@else - Setup + Setup @endif
diff --git a/resources/views/mfa/totp-generate.blade.php b/resources/views/mfa/totp-generate.blade.php index c1e7547d5..2ed2677d1 100644 --- a/resources/views/mfa/totp-generate.blade.php +++ b/resources/views/mfa/totp-generate.blade.php @@ -24,7 +24,7 @@ Verify that all is working by entering a code, generated within your authentication app, in the input box below:

- + {{ csrf_field() }} - + {{ csrf_field() }} - + {{ csrf_field() }} 'auth'], function () { Route::put('/roles/{id}', 'RoleController@update'); }); - // MFA (Auth Mandatory) - Route::delete('/mfa/remove/{method}', 'Auth\MfaController@remove'); }); -// 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'); -Route::post('/mfa/verify/totp', 'Auth\MfaTotpController@verify'); -Route::post('/mfa/verify/backup_codes', 'Auth\MfaBackupCodesController@verify'); +// MFA routes +Route::group(['middleware' => 'mfa-setup'], function() { + 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::group(['middleware' => 'guest'], function() { + Route::get('/mfa/verify', 'Auth\MfaController@verify'); + Route::post('/mfa/totp/verify', 'Auth\MfaTotpController@verify'); + Route::post('/mfa/backup_codes/verify', 'Auth\MfaBackupCodesController@verify'); +}); +Route::delete('/mfa/remove/{method}', 'Auth\MfaController@remove')->middleware('auth'); // Social auth routes Route::get('/login/service/{socialDriver}', 'Auth\SocialController@login'); Route::get('/login/service/{socialDriver}/callback', 'Auth\SocialController@callback'); -Route::group(['middleware' => 'auth'], function () { - Route::post('/login/service/{socialDriver}/detach', 'Auth\SocialController@detach'); -}); +Route::post('/login/service/{socialDriver}/detach', 'Auth\SocialController@detach')->middleware('auth'); Route::get('/register/service/{socialDriver}', 'Auth\SocialController@register'); // Login/Logout routes diff --git a/tests/Auth/MfaConfigurationTest.php b/tests/Auth/MfaConfigurationTest.php index a8ca815fb..63a5fa4dd 100644 --- a/tests/Auth/MfaConfigurationTest.php +++ b/tests/Auth/MfaConfigurationTest.php @@ -18,21 +18,21 @@ class MfaConfigurationTest extends TestCase // Setup page state $resp = $this->actingAs($editor)->get('/mfa/setup'); - $resp->assertElementContains('a[href$="/mfa/totp-generate"]', 'Setup'); + $resp->assertElementContains('a[href$="/mfa/totp/generate"]', 'Setup'); // Generate page access - $resp = $this->get('/mfa/totp-generate'); + $resp = $this->get('/mfa/totp/generate'); $resp->assertSee('Mobile App Setup'); $resp->assertSee('Verify Setup'); - $resp->assertElementExists('form[action$="/mfa/totp-confirm"] button'); + $resp->assertElementExists('form[action$="/mfa/totp/confirm"] button'); $this->assertSessionHas('mfa-setup-totp-secret'); $svg = $resp->getElementHtml('#main-content .card svg'); // Validation error, code should remain the same - $resp = $this->post('/mfa/totp-confirm', [ + $resp = $this->post('/mfa/totp/confirm', [ 'code' => 'abc123', ]); - $resp->assertRedirect('/mfa/totp-generate'); + $resp->assertRedirect('/mfa/totp/generate'); $resp = $this->followRedirects($resp); $resp->assertSee('The provided code is not valid or has expired.'); $revisitSvg = $resp->getElementHtml('#main-content .card svg'); @@ -42,7 +42,7 @@ class MfaConfigurationTest extends TestCase $google2fa = new Google2FA(); $secret = decrypt(session()->get('mfa-setup-totp-secret')); $otp = $google2fa->getCurrentOtp($secret); - $resp = $this->post('/mfa/totp-confirm', [ + $resp = $this->post('/mfa/totp/confirm', [ 'code' => $otp, ]); $resp->assertRedirect('/mfa/setup'); @@ -50,7 +50,7 @@ class MfaConfigurationTest extends TestCase // Confirmation of setup $resp = $this->followRedirects($resp); $resp->assertSee('Multi-factor method successfully configured'); - $resp->assertElementContains('a[href$="/mfa/totp-generate"]', 'Reconfigure'); + $resp->assertElementContains('a[href$="/mfa/totp/generate"]', 'Reconfigure'); $this->assertDatabaseHas('mfa_values', [ 'user_id' => $editor->id, @@ -69,12 +69,12 @@ class MfaConfigurationTest extends TestCase // Setup page state $resp = $this->actingAs($editor)->get('/mfa/setup'); - $resp->assertElementContains('a[href$="/mfa/backup-codes-generate"]', 'Setup'); + $resp->assertElementContains('a[href$="/mfa/backup_codes/generate"]', 'Setup'); // Generate page access - $resp = $this->get('/mfa/backup-codes-generate'); + $resp = $this->get('/mfa/backup_codes/generate'); $resp->assertSee('Backup Codes'); - $resp->assertElementContains('form[action$="/mfa/backup-codes-confirm"]', 'Confirm and Enable'); + $resp->assertElementContains('form[action$="/mfa/backup_codes/confirm"]', 'Confirm and Enable'); $this->assertSessionHas('mfa-setup-backup-codes'); $codes = decrypt(session()->get('mfa-setup-backup-codes')); // Check code format @@ -84,13 +84,13 @@ class MfaConfigurationTest extends TestCase $resp->assertSee(base64_encode(implode("\n\n", $codes))); // Confirm submit - $resp = $this->post('/mfa/backup-codes-confirm'); + $resp = $this->post('/mfa/backup_codes/confirm'); $resp->assertRedirect('/mfa/setup'); // Confirmation of setup $resp = $this->followRedirects($resp); $resp->assertSee('Multi-factor method successfully configured'); - $resp->assertElementContains('a[href$="/mfa/backup-codes-generate"]', 'Reconfigure'); + $resp->assertElementContains('a[href$="/mfa/backup_codes/generate"]', 'Reconfigure'); $this->assertDatabaseHas('mfa_values', [ 'user_id' => $editor->id, @@ -104,7 +104,7 @@ class MfaConfigurationTest extends TestCase public function test_backup_codes_cannot_be_confirmed_if_not_previously_generated() { - $resp = $this->asEditor()->post('/mfa/backup-codes-confirm'); + $resp = $this->asEditor()->post('/mfa/backup_codes/confirm'); $resp->assertStatus(500); } diff --git a/tests/Auth/MfaVerificationTest.php b/tests/Auth/MfaVerificationTest.php index 3f272cffb..d007fa490 100644 --- a/tests/Auth/MfaVerificationTest.php +++ b/tests/Auth/MfaVerificationTest.php @@ -2,9 +2,12 @@ namespace Tests\Auth; +use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\Mfa\MfaValue; use BookStack\Auth\Access\Mfa\TotpService; +use BookStack\Auth\Role; use BookStack\Auth\User; +use BookStack\Exceptions\StoppedAuthenticationException; use Illuminate\Support\Facades\Hash; use PragmaRX\Google2FA\Google2FA; use Tests\TestCase; @@ -20,10 +23,10 @@ class MfaVerificationTest extends TestCase $resp = $this->get('/mfa/verify'); $resp->assertSee('Verify Access'); $resp->assertSee('Enter the code, generated using your mobile app, below:'); - $resp->assertElementExists('form[action$="/mfa/verify/totp"] input[name="code"]'); + $resp->assertElementExists('form[action$="/mfa/totp/verify"] input[name="code"]'); $google2fa = new Google2FA(); - $resp = $this->post('/mfa/verify/totp', [ + $resp = $this->post('/mfa/totp/verify', [ 'code' => $google2fa->getCurrentOtp($secret), ]); $resp->assertRedirect('/'); @@ -35,7 +38,7 @@ class MfaVerificationTest extends TestCase [$user, $secret, $loginResp] = $this->startTotpLogin(); $resp = $this->get('/mfa/verify'); - $resp = $this->post('/mfa/verify/totp', [ + $resp = $this->post('/mfa/totp/verify', [ 'code' => '', ]); $resp->assertRedirect('/mfa/verify'); @@ -44,7 +47,7 @@ class MfaVerificationTest extends TestCase $resp->assertSeeText('The code field is required.'); $this->assertNull(auth()->user()); - $resp = $this->post('/mfa/verify/totp', [ + $resp = $this->post('/mfa/totp/verify', [ 'code' => '123321', ]); $resp->assertRedirect('/mfa/verify'); @@ -63,9 +66,9 @@ class MfaVerificationTest extends TestCase $resp->assertSee('Verify Access'); $resp->assertSee('Backup Code'); $resp->assertSee('Enter one of your remaining backup codes below:'); - $resp->assertElementExists('form[action$="/mfa/verify/backup_codes"] input[name="code"]'); + $resp->assertElementExists('form[action$="/mfa/backup_codes/verify"] input[name="code"]'); - $resp = $this->post('/mfa/verify/backup_codes', [ + $resp = $this->post('/mfa/backup_codes/verify', [ 'code' => $codes[1], ]); @@ -82,7 +85,7 @@ class MfaVerificationTest extends TestCase [$user, $codes, $loginResp] = $this->startBackupCodeLogin(); $resp = $this->get('/mfa/verify'); - $resp = $this->post('/mfa/verify/backup_codes', [ + $resp = $this->post('/mfa/backup_codes/verify', [ 'code' => '', ]); $resp->assertRedirect('/mfa/verify'); @@ -91,7 +94,7 @@ class MfaVerificationTest extends TestCase $resp->assertSeeText('The code field is required.'); $this->assertNull(auth()->user()); - $resp = $this->post('/mfa/verify/backup_codes', [ + $resp = $this->post('/mfa/backup_codes/verify', [ 'code' => 'ab123-ab456', ]); $resp->assertRedirect('/mfa/verify'); @@ -105,7 +108,7 @@ class MfaVerificationTest extends TestCase { [$user, $codes, $loginResp] = $this->startBackupCodeLogin(); - $this->post('/mfa/verify/backup_codes', [ + $this->post('/mfa/backup_codes/verify', [ 'code' => $codes[0], ]); $this->assertNotNull(auth()->user()); @@ -114,7 +117,7 @@ class MfaVerificationTest extends TestCase $this->post('/login', ['email' => $user->email, 'password' => 'password']); $this->get('/mfa/verify'); - $resp = $this->post('/mfa/verify/backup_codes', [ + $resp = $this->post('/mfa/backup_codes/verify', [ 'code' => $codes[0], ]); $resp->assertRedirect('/mfa/verify'); @@ -128,7 +131,7 @@ class MfaVerificationTest extends TestCase { [$user, $codes, $loginResp] = $this->startBackupCodeLogin(['abc12-def45', 'abc12-def46']); - $resp = $this->post('/mfa/verify/backup_codes', [ + $resp = $this->post('/mfa/backup_codes/verify', [ 'code' => $codes[0], ]); $resp = $this->followRedirects($resp); @@ -151,16 +154,88 @@ class MfaVerificationTest extends TestCase ]); // Totp shown by default - $mfaView->assertElementExists('form[action$="/mfa/verify/totp"] input[name="code"]'); + $mfaView->assertElementExists('form[action$="/mfa/totp/verify"] input[name="code"]'); $mfaView->assertElementContains('a[href$="/mfa/verify?method=backup_codes"]', 'Verify using a backup code'); // Ensure can view backup_codes view $resp = $this->get('/mfa/verify?method=backup_codes'); - $resp->assertElementExists('form[action$="/mfa/verify/backup_codes"] input[name="code"]'); + $resp->assertElementExists('form[action$="/mfa/backup_codes/verify"] input[name="code"]'); $resp->assertElementContains('a[href$="/mfa/verify?method=totp"]', 'Verify using a mobile app'); } - // TODO !! - Test no-existing MFA + public function test_mfa_required_with_no_methods_leads_to_setup() + { + $user = $this->getEditor(); + $user->password = Hash::make('password'); + $user->save(); + /** @var Role $role */ + $role = $user->roles->first(); + $role->mfa_enforced = true; + $role->save(); + + $this->assertDatabaseMissing('mfa_values', [ + 'user_id' => $user->id, + ]); + + /** @var TestResponse $resp */ + $resp = $this->followingRedirects()->post('/login', [ + 'email' => $user->email, + 'password' => 'password', + ]); + + $resp->assertSeeText('No Methods Configured'); + $resp->assertElementContains('a[href$="/mfa/setup"]', 'Configure'); + + $this->get('/mfa/backup_codes/generate'); + $this->followingRedirects()->post('/mfa/backup_codes/confirm'); + $this->assertDatabaseHas('mfa_values', [ + 'user_id' => $user->id, + ]); + + $resp = $this->followingRedirects()->post('/login', [ + 'email' => $user->email, + 'password' => 'password', + ]); + $resp->assertSeeText('Enter one of your remaining backup codes below:'); + } + + public function test_mfa_setup_route_access() + { + $routes = [ + ['get', '/mfa/setup'], + ['get', '/mfa/totp/generate'], + ['post', '/mfa/totp/confirm'], + ['get', '/mfa/backup_codes/generate'], + ['post', '/mfa/backup_codes/confirm'], + ]; + + // Non-auth access + foreach ($routes as [$method, $path]) { + $resp = $this->call($method, $path); + $resp->assertRedirect('/login'); + } + + // Attempted login user, who has configured mfa, access + // Sets up user that has MFA required after attempted login. + $loginService = $this->app->make(LoginService::class); + $user = $this->getEditor(); + /** @var Role $role */ + $role = $user->roles->first(); + $role->mfa_enforced = true; + $role->save(); + try { + $loginService->login($user, 'testing'); + } catch (StoppedAuthenticationException $e) { + } + $this->assertNotNull($loginService->getLastLoginAttemptUser()); + + MfaValue::upsertWithValue($user, MfaValue::METHOD_BACKUP_CODES, '[]'); + foreach ($routes as [$method, $path]) { + $resp = $this->call($method, $path); + $resp->assertRedirect('/login'); + } + + } /** * @return Array From 70f39757b1e405bcbb40ec1618e5259d13c642dc Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Thu, 5 Aug 2021 22:07:08 +0100 Subject: [PATCH 18/25] Updated API auth handling of email confirmations Email confirmations are now done within the guard during auth checking instead of at the middleware layer. --- app/Api/ApiTokenGuard.php | 13 ++++++++++++- app/Http/Middleware/ApiAuthenticate.php | 3 --- app/Providers/AuthServiceProvider.php | 7 ++++--- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/app/Api/ApiTokenGuard.php b/app/Api/ApiTokenGuard.php index 75ed5cb35..8b9cbc8e1 100644 --- a/app/Api/ApiTokenGuard.php +++ b/app/Api/ApiTokenGuard.php @@ -2,6 +2,7 @@ namespace BookStack\Api; +use BookStack\Auth\Access\LoginService; use BookStack\Exceptions\ApiAuthException; use Illuminate\Auth\GuardHelpers; use Illuminate\Contracts\Auth\Authenticatable; @@ -19,6 +20,11 @@ class ApiTokenGuard implements Guard */ protected $request; + /** + * @var LoginService + */ + protected $loginService; + /** * The last auth exception thrown in this request. * @@ -29,9 +35,10 @@ class ApiTokenGuard implements Guard /** * ApiTokenGuard constructor. */ - public function __construct(Request $request) + public function __construct(Request $request, LoginService $loginService) { $this->request = $request; + $this->loginService = $loginService; } /** @@ -95,6 +102,10 @@ class ApiTokenGuard implements Guard $this->validateToken($token, $secret); + if ($this->loginService->awaitingEmailConfirmation($token->user)) { + throw new ApiAuthException(trans('errors.email_confirmation_awaiting')); + } + return $token->user; } diff --git a/app/Http/Middleware/ApiAuthenticate.php b/app/Http/Middleware/ApiAuthenticate.php index 21d69810f..73192b0cf 100644 --- a/app/Http/Middleware/ApiAuthenticate.php +++ b/app/Http/Middleware/ApiAuthenticate.php @@ -9,7 +9,6 @@ use Illuminate\Http\Request; class ApiAuthenticate { - use ChecksForEmailConfirmation; /** * Handle an incoming request. @@ -37,7 +36,6 @@ 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() || session()->isStarted()) { - $this->ensureEmailConfirmedIfRequested(); if (!user()->can('access-api')) { throw new ApiAuthException(trans('errors.api_user_no_api_permission'), 403); } @@ -50,7 +48,6 @@ class ApiAuthenticate // Validate the token and it's users API access auth()->authenticate(); - $this->ensureEmailConfirmedIfRequested(); } /** diff --git a/app/Providers/AuthServiceProvider.php b/app/Providers/AuthServiceProvider.php index 1a78214dc..71b7ab016 100644 --- a/app/Providers/AuthServiceProvider.php +++ b/app/Providers/AuthServiceProvider.php @@ -8,6 +8,7 @@ use BookStack\Auth\Access\ExternalBaseUserProvider; use BookStack\Auth\Access\Guards\LdapSessionGuard; use BookStack\Auth\Access\Guards\Saml2SessionGuard; use BookStack\Auth\Access\LdapService; +use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\RegistrationService; use Illuminate\Support\ServiceProvider; @@ -21,7 +22,7 @@ class AuthServiceProvider extends ServiceProvider public function boot() { Auth::extend('api-token', function ($app, $name, array $config) { - return new ApiTokenGuard($app['request']); + return new ApiTokenGuard($app['request'], $app->make(LoginService::class)); }); Auth::extend('ldap-session', function ($app, $name, array $config) { @@ -30,7 +31,7 @@ class AuthServiceProvider extends ServiceProvider return new LdapSessionGuard( $name, $provider, - $this->app['session.store'], + $app['session.store'], $app[LdapService::class], $app[RegistrationService::class] ); @@ -42,7 +43,7 @@ class AuthServiceProvider extends ServiceProvider return new Saml2SessionGuard( $name, $provider, - $this->app['session.store'], + $app['session.store'], $app[RegistrationService::class] ); }); From 39a205ed28f2dcfbab7e34fd231fe97b0e57df83 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 7 Aug 2021 21:18:59 +0100 Subject: [PATCH 19/25] Quick test of email confirmation routes and fix of tests --- app/Auth/Access/EmailConfirmationService.php | 5 - app/Auth/Access/LoginService.php | 8 +- app/Auth/Access/RegistrationService.php | 1 - app/Auth/Access/Saml2Service.php | 2 + .../StoppedAuthenticationException.php | 29 ++- .../Controllers/Auth/RegisterController.php | 2 + app/Http/Middleware/Authenticate.php | 22 -- tests/Auth/AuthTest.php | 12 +- tests/Auth/LdapTest.php | 200 +++++++++--------- tests/Auth/Saml2Test.php | 6 +- 10 files changed, 146 insertions(+), 141 deletions(-) diff --git a/app/Auth/Access/EmailConfirmationService.php b/app/Auth/Access/EmailConfirmationService.php index 3425c1e72..75c03b318 100644 --- a/app/Auth/Access/EmailConfirmationService.php +++ b/app/Auth/Access/EmailConfirmationService.php @@ -14,9 +14,6 @@ class EmailConfirmationService extends UserTokenService /** * Create new confirmation for a user, * Also removes any existing old ones. - * - * @param User $user - * * @throws ConfirmationEmailException */ public function sendConfirmation(User $user) @@ -33,8 +30,6 @@ class EmailConfirmationService extends UserTokenService /** * Check if confirmation is required in this instance. - * - * @return bool */ public function confirmationRequired(): bool { diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php index f6ea7517f..998259dee 100644 --- a/app/Auth/Access/LoginService.php +++ b/app/Auth/Access/LoginService.php @@ -10,7 +10,6 @@ use BookStack\Facades\Activity; use BookStack\Facades\Theme; use BookStack\Theming\ThemeEvents; use Exception; -use phpDocumentor\Reflection\DocBlock\Tags\Method; class LoginService { @@ -18,10 +17,12 @@ class LoginService protected const LAST_LOGIN_ATTEMPTED_SESSION_KEY = 'auth-login-last-attempted'; protected $mfaSession; + protected $emailConfirmationService; - public function __construct(MfaSession $mfaSession) + public function __construct(MfaSession $mfaSession, EmailConfirmationService $emailConfirmationService) { $this->mfaSession = $mfaSession; + $this->emailConfirmationService = $emailConfirmationService; } /** @@ -149,8 +150,7 @@ class LoginService */ public function awaitingEmailConfirmation(User $user): bool { - $requireConfirmation = (setting('registration-confirmation') || setting('registration-restrict')); - return $requireConfirmation && !$user->email_confirmed; + return $this->emailConfirmationService->confirmationRequired() && !$user->email_confirmed; } /** diff --git a/app/Auth/Access/RegistrationService.php b/app/Auth/Access/RegistrationService.php index 16e3edbb4..4c8b53c0d 100644 --- a/app/Auth/Access/RegistrationService.php +++ b/app/Auth/Access/RegistrationService.php @@ -88,7 +88,6 @@ class RegistrationService session()->flash('sent-email-confirmation', true); } catch (Exception $e) { $message = trans('auth.email_confirm_send_error'); - throw new UserRegistrationException($message, '/register/confirm'); } } diff --git a/app/Auth/Access/Saml2Service.php b/app/Auth/Access/Saml2Service.php index d0b6a655b..3f0f40ccc 100644 --- a/app/Auth/Access/Saml2Service.php +++ b/app/Auth/Access/Saml2Service.php @@ -6,6 +6,7 @@ use BookStack\Actions\ActivityType; use BookStack\Auth\User; use BookStack\Exceptions\JsonDebugException; use BookStack\Exceptions\SamlException; +use BookStack\Exceptions\StoppedAuthenticationException; use BookStack\Exceptions\UserRegistrationException; use BookStack\Facades\Activity; use BookStack\Facades\Theme; @@ -357,6 +358,7 @@ class Saml2Service extends ExternalAuthService * @throws SamlException * @throws JsonDebugException * @throws UserRegistrationException + * @throws StoppedAuthenticationException */ public function processLoginCallback(string $samlID, array $samlAttributes): User { diff --git a/app/Exceptions/StoppedAuthenticationException.php b/app/Exceptions/StoppedAuthenticationException.php index de8898df7..7a978ffc6 100644 --- a/app/Exceptions/StoppedAuthenticationException.php +++ b/app/Exceptions/StoppedAuthenticationException.php @@ -5,6 +5,7 @@ namespace BookStack\Exceptions; use BookStack\Auth\Access\LoginService; use BookStack\Auth\User; use Illuminate\Contracts\Support\Responsable; +use Illuminate\Http\Request; class StoppedAuthenticationException extends \Exception implements Responsable { @@ -30,11 +31,35 @@ class StoppedAuthenticationException extends \Exception implements Responsable $redirect = '/login'; if ($this->loginService->awaitingEmailConfirmation($this->user)) { - $redirect = '/register/confirm/awaiting'; - } else if ($this->loginService->needsMfaVerification($this->user)) { + return $this->awaitingEmailConfirmationResponse($request); + } + + if ($this->loginService->needsMfaVerification($this->user)) { $redirect = '/mfa/verify'; } return redirect($redirect); } + + /** + * Provide an error response for when the current user's email is not confirmed + * in a system which requires it. + */ + protected function awaitingEmailConfirmationResponse(Request $request) + { + if ($request->wantsJson()) { + return response()->json([ + 'error' => [ + 'code' => 401, + 'message' => trans('errors.email_confirmation_awaiting'), + ], + ], 401); + } + + if (session()->get('sent-email-confirmation') === true) { + return redirect('/register/confirm'); + } + + return redirect('/register/confirm/awaiting'); + } } \ No newline at end of file diff --git a/app/Http/Controllers/Auth/RegisterController.php b/app/Http/Controllers/Auth/RegisterController.php index 4008439bc..e372b38ef 100644 --- a/app/Http/Controllers/Auth/RegisterController.php +++ b/app/Http/Controllers/Auth/RegisterController.php @@ -6,6 +6,7 @@ use BookStack\Auth\Access\LoginService; use BookStack\Auth\Access\RegistrationService; use BookStack\Auth\Access\SocialAuthService; use BookStack\Auth\User; +use BookStack\Exceptions\StoppedAuthenticationException; use BookStack\Exceptions\UserRegistrationException; use BookStack\Http\Controllers\Controller; use Illuminate\Foundation\Auth\RegistersUsers; @@ -93,6 +94,7 @@ class RegisterController extends Controller * Handle a registration request for the application. * * @throws UserRegistrationException + * @throws StoppedAuthenticationException */ public function postRegister(Request $request) { diff --git a/app/Http/Middleware/Authenticate.php b/app/Http/Middleware/Authenticate.php index b9b5545f2..dede8f260 100644 --- a/app/Http/Middleware/Authenticate.php +++ b/app/Http/Middleware/Authenticate.php @@ -21,26 +21,4 @@ class Authenticate 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 emailConfirmationErrorResponse(Request $request) - { - if ($request->wantsJson()) { - return response()->json([ - 'error' => [ - 'code' => 401, - 'message' => trans('errors.email_confirmation_awaiting'), - ], - ], 401); - } - - if (session()->get('sent-email-confirmation') === true) { - return redirect('/register/confirm'); - } - - return redirect('/register/confirm/awaiting'); - } } diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php index febf58399..d57a3253f 100644 --- a/tests/Auth/AuthTest.php +++ b/tests/Auth/AuthTest.php @@ -131,7 +131,8 @@ class AuthTest extends BrowserKitTest ->seePageIs('/register/confirm/awaiting') ->see('Resend') ->visit('/books') - ->seePageIs('/register/confirm/awaiting') + ->seePageIs('/login') + ->visit('/register/confirm/awaiting') ->press('Resend Confirmation Email'); // Get confirmation and confirm notification matches @@ -172,10 +173,7 @@ class AuthTest extends BrowserKitTest ->seePageIs('/register/confirm') ->seeInDatabase('users', ['name' => $user->name, 'email' => $user->email, 'email_confirmed' => false]); - $this->visit('/') - ->seePageIs('/register/confirm/awaiting'); - - auth()->logout(); + $this->assertNull(auth()->user()); $this->visit('/')->seePageIs('/login') ->type($user->email, '#email') @@ -209,10 +207,8 @@ class AuthTest extends BrowserKitTest ->seePageIs('/register/confirm') ->seeInDatabase('users', ['name' => $user->name, 'email' => $user->email, 'email_confirmed' => false]); - $this->visit('/') - ->seePageIs('/register/confirm/awaiting'); + $this->assertNull(auth()->user()); - auth()->logout(); $this->visit('/')->seePageIs('/login') ->type($user->email, '#email') ->type($user->password, '#password') diff --git a/tests/Auth/LdapTest.php b/tests/Auth/LdapTest.php index 9d491fd0f..3bd6988e4 100644 --- a/tests/Auth/LdapTest.php +++ b/tests/Auth/LdapTest.php @@ -27,18 +27,18 @@ class LdapTest extends TestCase define('LDAP_OPT_REFERRALS', 1); } config()->set([ - 'auth.method' => 'ldap', - 'auth.defaults.guard' => 'ldap', - 'services.ldap.base_dn' => 'dc=ldap,dc=local', - 'services.ldap.email_attribute' => 'mail', + 'auth.method' => 'ldap', + 'auth.defaults.guard' => 'ldap', + 'services.ldap.base_dn' => 'dc=ldap,dc=local', + 'services.ldap.email_attribute' => 'mail', 'services.ldap.display_name_attribute' => 'cn', - 'services.ldap.id_attribute' => 'uid', - 'services.ldap.user_to_groups' => false, - 'services.ldap.version' => '3', - 'services.ldap.user_filter' => '(&(uid=${user}))', - 'services.ldap.follow_referrals' => false, - 'services.ldap.tls_insecure' => false, - 'services.ldap.thumbnail_attribute' => null, + 'services.ldap.id_attribute' => 'uid', + 'services.ldap.user_to_groups' => false, + 'services.ldap.version' => '3', + 'services.ldap.user_filter' => '(&(uid=${user}))', + 'services.ldap.follow_referrals' => false, + 'services.ldap.tls_insecure' => false, + 'services.ldap.thumbnail_attribute' => null, ]); $this->mockLdap = \Mockery::mock(Ldap::class); $this->app[Ldap::class] = $this->mockLdap; @@ -70,9 +70,9 @@ class LdapTest extends TestCase protected function mockUserLogin(?string $email = null): TestResponse { return $this->post('/login', [ - 'username' => $this->mockUser->name, - 'password' => $this->mockUser->password, - ] + ($email ? ['email' => $email] : [])); + 'username' => $this->mockUser->name, + 'password' => $this->mockUser->password, + ] + ($email ? ['email' => $email] : [])); } /** @@ -95,8 +95,8 @@ class LdapTest extends TestCase ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], ]]); $resp = $this->mockUserLogin(); @@ -109,8 +109,8 @@ class LdapTest extends TestCase $resp->assertElementExists('#home-default'); $resp->assertSee($this->mockUser->name); $this->assertDatabaseHas('users', [ - 'email' => $this->mockUser->email, - 'email_confirmed' => false, + 'email' => $this->mockUser->email, + 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, ]); } @@ -126,8 +126,8 @@ class LdapTest extends TestCase ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], ]]); $resp = $this->mockUserLogin(); @@ -150,8 +150,8 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'cn' => [$this->mockUser->name], - 'dn' => $ldapDn, + 'cn' => [$this->mockUser->name], + 'dn' => $ldapDn, 'mail' => [$this->mockUser->email], ]]); @@ -170,10 +170,10 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'cn' => [$this->mockUser->name], - 'dn' => $ldapDn, + 'cn' => [$this->mockUser->name], + 'dn' => $ldapDn, 'my_custom_id' => ['cooluser456'], - 'mail' => [$this->mockUser->email], + 'mail' => [$this->mockUser->email], ]]); $resp = $this->mockUserLogin(); @@ -189,8 +189,8 @@ class LdapTest extends TestCase ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], ]]); $this->mockLdap->shouldReceive('bind')->times(2)->andReturn(true, false); @@ -219,14 +219,14 @@ class LdapTest extends TestCase $userForm->assertDontSee('Password'); $save = $this->post('/settings/users/create', [ - 'name' => $this->mockUser->name, + 'name' => $this->mockUser->name, 'email' => $this->mockUser->email, ]); $save->assertSessionHasErrors(['external_auth_id' => 'The external auth id field is required.']); $save = $this->post('/settings/users/create', [ - 'name' => $this->mockUser->name, - 'email' => $this->mockUser->email, + 'name' => $this->mockUser->name, + 'email' => $this->mockUser->email, 'external_auth_id' => $this->mockUser->name, ]); $save->assertRedirect('/settings/users'); @@ -241,8 +241,8 @@ class LdapTest extends TestCase $editPage->assertDontSee('Password'); $update = $this->put("/settings/users/{$editUser->id}", [ - 'name' => $editUser->name, - 'email' => $editUser->email, + 'name' => $editUser->name, + 'email' => $editUser->email, 'external_auth_id' => 'test_auth_id', ]); $update->assertRedirect('/settings/users'); @@ -271,8 +271,8 @@ class LdapTest extends TestCase $this->mockUser->attachRole($existingRole); app('config')->set([ - 'services.ldap.user_to_groups' => true, - 'services.ldap.group_attribute' => 'memberOf', + 'services.ldap.user_to_groups' => true, + 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => false, ]); @@ -280,14 +280,14 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], - 'mail' => [$this->mockUser->email], + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'mail' => [$this->mockUser->email], 'memberof' => [ 'count' => 2, - 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', - 1 => 'cn=ldaptester-second,ou=groups,dc=example,dc=com', + 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', + 1 => 'cn=ldaptester-second,ou=groups,dc=example,dc=com', ], ]]); @@ -316,8 +316,8 @@ class LdapTest extends TestCase $this->mockUser->attachRole($existingRole); app('config')->set([ - 'services.ldap.user_to_groups' => true, - 'services.ldap.group_attribute' => 'memberOf', + 'services.ldap.user_to_groups' => true, + 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => true, ]); @@ -325,13 +325,13 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(3) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], - 'mail' => [$this->mockUser->email], + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'mail' => [$this->mockUser->email], 'memberof' => [ 'count' => 1, - 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', + 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', ], ]]); @@ -361,8 +361,8 @@ class LdapTest extends TestCase $roleToNotReceive = factory(Role::class)->create(['display_name' => 'ex-auth-a', 'external_auth_id' => 'test-second-param']); app('config')->set([ - 'services.ldap.user_to_groups' => true, - 'services.ldap.group_attribute' => 'memberOf', + 'services.ldap.user_to_groups' => true, + 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => true, ]); @@ -370,13 +370,13 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(3) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], - 'mail' => [$this->mockUser->email], + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'mail' => [$this->mockUser->email], 'memberof' => [ 'count' => 1, - 0 => 'cn=ex-auth-a,ou=groups,dc=example,dc=com', + 0 => 'cn=ex-auth-a,ou=groups,dc=example,dc=com', ], ]]); @@ -402,8 +402,8 @@ class LdapTest extends TestCase setting()->put('registration-role', $roleToReceive->id); app('config')->set([ - 'services.ldap.user_to_groups' => true, - 'services.ldap.group_attribute' => 'memberOf', + 'services.ldap.user_to_groups' => true, + 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => true, ]); @@ -411,14 +411,14 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(4) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], - 'mail' => [$this->mockUser->email], + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'mail' => [$this->mockUser->email], 'memberof' => [ 'count' => 2, - 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', - 1 => 'cn=ldaptester-second,ou=groups,dc=example,dc=com', + 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', + 1 => 'cn=ldaptester-second,ou=groups,dc=example,dc=com', ], ]]); @@ -445,9 +445,9 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], 'displayname' => 'displayNameAttribute', ]]); @@ -471,8 +471,8 @@ class LdapTest extends TestCase ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], ]]); $this->mockUserLogin()->assertRedirect('/login'); @@ -482,10 +482,10 @@ class LdapTest extends TestCase $resp->assertRedirect('/'); $this->get('/')->assertSee($this->mockUser->name); $this->assertDatabaseHas('users', [ - 'email' => $this->mockUser->email, - 'email_confirmed' => false, + 'email' => $this->mockUser->email, + 'email_confirmed' => false, 'external_auth_id' => $this->mockUser->name, - 'name' => $this->mockUser->name, + 'name' => $this->mockUser->name, ]); } @@ -499,8 +499,8 @@ class LdapTest extends TestCase $this->commonLdapMocks(0, 1, 1, 2, 1); $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1)->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], ]]); $this->mockLdap->shouldReceive('connect')->once() @@ -566,8 +566,8 @@ class LdapTest extends TestCase ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], ]]); $resp = $this->post('/login', [ @@ -575,7 +575,7 @@ class LdapTest extends TestCase 'password' => $this->mockUser->password, ]); $resp->assertJsonStructure([ - 'details_from_ldap' => [], + 'details_from_ldap' => [], 'details_bookstack_parsed' => [], ]); } @@ -605,8 +605,8 @@ class LdapTest extends TestCase ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), ['cn', 'dn', 'uid', 'mail', 'cn']) ->andReturn(['count' => 1, 0 => [ 'uid' => [hex2bin('FFF8F7')], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], ]]); $details = $ldapService->getUserDetails('test'); @@ -619,14 +619,14 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(2) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'uid' => [$this->mockUser->name], - 'cn' => [$this->mockUser->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'uid' => [$this->mockUser->name], + 'cn' => [$this->mockUser->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], 'mail' => 'tester@example.com', ]], ['count' => 1, 0 => [ - 'uid' => ['Barry'], - 'cn' => ['Scott'], - 'dn' => ['dc=bscott' . config('services.ldap.base_dn')], + 'uid' => ['Barry'], + 'cn' => ['Scott'], + 'dn' => ['dc=bscott' . config('services.ldap.base_dn')], 'mail' => 'tester@example.com', ]]); @@ -646,28 +646,29 @@ class LdapTest extends TestCase setting()->put('registration-confirmation', 'true'); app('config')->set([ - 'services.ldap.user_to_groups' => true, - 'services.ldap.group_attribute' => 'memberOf', + 'services.ldap.user_to_groups' => true, + 'services.ldap.group_attribute' => 'memberOf', 'services.ldap.remove_from_groups' => true, ]); - $this->commonLdapMocks(1, 1, 3, 4, 3, 2); + $this->commonLdapMocks(1, 1, 6, 8, 6, 4); $this->mockLdap->shouldReceive('searchAndGetEntries') - ->times(3) + ->times(6) ->andReturn(['count' => 1, 0 => [ - 'uid' => [$user->name], - 'cn' => [$user->name], - 'dn' => ['dc=test' . config('services.ldap.base_dn')], - 'mail' => [$user->email], + 'uid' => [$user->name], + 'cn' => [$user->name], + 'dn' => ['dc=test' . config('services.ldap.base_dn')], + 'mail' => [$user->email], 'memberof' => [ 'count' => 1, - 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', + 0 => 'cn=ldaptester,ou=groups,dc=example,dc=com', ], ]]); - $this->followingRedirects()->mockUserLogin()->assertSee('Thanks for registering!'); + $login = $this->followingRedirects()->mockUserLogin(); + $login->assertSee('Thanks for registering!'); $this->assertDatabaseHas('users', [ - 'email' => $user->email, + 'email' => $user->email, 'email_confirmed' => false, ]); @@ -677,8 +678,13 @@ class LdapTest extends TestCase 'role_id' => $roleToReceive->id, ]); + $this->assertNull(auth()->user()); + $homePage = $this->get('/'); - $homePage->assertRedirect('/register/confirm/awaiting'); + $homePage->assertRedirect('/login'); + + $login = $this->followingRedirects()->mockUserLogin(); + $login->assertSee('Email Address Not Confirmed'); } public function test_failed_logins_are_logged_when_message_configured() @@ -698,8 +704,8 @@ class LdapTest extends TestCase $this->mockLdap->shouldReceive('searchAndGetEntries')->times(1) ->with($this->resourceId, config('services.ldap.base_dn'), \Mockery::type('string'), \Mockery::type('array')) ->andReturn(['count' => 1, 0 => [ - 'cn' => [$this->mockUser->name], - 'dn' => $ldapDn, + 'cn' => [$this->mockUser->name], + 'dn' => $ldapDn, 'jpegphoto' => [base64_decode('/9j/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8Q EBEQCgwSExIQEw8QEBD/yQALCAABAAEBAREA/8wABgAQEAX/2gAIAQEAAD8A0s8g/9k=')], 'mail' => [$this->mockUser->email], diff --git a/tests/Auth/Saml2Test.php b/tests/Auth/Saml2Test.php index 096f862e7..8ace3e2ee 100644 --- a/tests/Auth/Saml2Test.php +++ b/tests/Auth/Saml2Test.php @@ -289,16 +289,18 @@ class Saml2Test extends TestCase $this->assertEquals('http://localhost/register/confirm', url()->current()); $acsPost->assertSee('Please check your email and click the confirmation button to access BookStack.'); + /** @var User $user */ $user = User::query()->where('external_auth_id', '=', 'user')->first(); $userRoleIds = $user->roles()->pluck('id'); $this->assertContains($memberRole->id, $userRoleIds, 'User was assigned to member role'); $this->assertContains($adminRole->id, $userRoleIds, 'User was assigned to admin role'); - $this->assertTrue($user->email_confirmed == false, 'User email remains unconfirmed'); + $this->assertFalse(boolval($user->email_confirmed), 'User email remains unconfirmed'); }); + $this->assertNull(auth()->user()); $homeGet = $this->get('/'); - $homeGet->assertRedirect('/register/confirm/awaiting'); + $homeGet->assertRedirect('/login'); } public function test_login_where_existing_non_saml_user_shows_warning() From ef9354a0cb3299d809253a8ef436c6b6d5410b8e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 7 Aug 2021 21:53:13 +0100 Subject: [PATCH 20/25] Verified mfa session expires on logout Since sessions are invalidated upon logout. --- app/Auth/Access/LoginService.php | 8 -------- tests/Auth/AuthTest.php | 13 +++++++++++++ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php index 998259dee..3aab0a247 100644 --- a/app/Auth/Access/LoginService.php +++ b/app/Auth/Access/LoginService.php @@ -38,14 +38,6 @@ class LoginService $this->setLastLoginAttemptedForUser($user, $method); throw new StoppedAuthenticationException($user, $this); // TODO - Does 'remember' still work? Probably not right now. - - // TODO - Need to clear MFA sessions out upon logout - - // Old MFA middleware todos: - - // TODO - Handle email confirmation handling - // Left BookStack\Http\Middleware\Authenticate@emailConfirmationErrorResponse in which needs - // be removed as an example of old behaviour. } $this->clearLastLoginAttempted(); diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php index d57a3253f..085482c35 100644 --- a/tests/Auth/AuthTest.php +++ b/tests/Auth/AuthTest.php @@ -2,6 +2,7 @@ namespace Tests\Auth; +use BookStack\Auth\Access\Mfa\MfaSession; use BookStack\Auth\Role; use BookStack\Auth\User; use BookStack\Entities\Models\Page; @@ -326,6 +327,18 @@ class AuthTest extends BrowserKitTest ->seePageIs('/login'); } + public function test_mfa_session_cleared_on_logout() + { + $user = $this->getEditor(); + $mfaSession = $this->app->make(MfaSession::class); + + $mfaSession->markVerifiedForUser($user);; + $this->assertTrue($mfaSession->isVerifiedForUser($user)); + + $this->asAdmin()->visit('/logout'); + $this->assertFalse($mfaSession->isVerifiedForUser($user)); + } + public function test_reset_password_flow() { Notification::fake(); From 773be963bade9ecdb79daf3f2f4bfddbf7922cee Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 7 Aug 2021 22:32:19 +0100 Subject: [PATCH 21/25] Updated auth changes to work with remember me --- app/Auth/Access/LoginService.php | 30 +++++++++++------------------- 1 file changed, 11 insertions(+), 19 deletions(-) diff --git a/app/Auth/Access/LoginService.php b/app/Auth/Access/LoginService.php index 3aab0a247..b251e4cc3 100644 --- a/app/Auth/Access/LoginService.php +++ b/app/Auth/Access/LoginService.php @@ -32,16 +32,15 @@ class LoginService * Returns a boolean to indicate the current login result. * @throws StoppedAuthenticationException */ - public function login(User $user, string $method): void + public function login(User $user, string $method, bool $remember = false): void { if ($this->awaitingEmailConfirmation($user) || $this->needsMfaVerification($user)) { - $this->setLastLoginAttemptedForUser($user, $method); + $this->setLastLoginAttemptedForUser($user, $method, $remember); throw new StoppedAuthenticationException($user, $this); - // TODO - Does 'remember' still work? Probably not right now. } $this->clearLastLoginAttempted(); - auth()->login($user); + auth()->login($user, $remember); Activity::add(ActivityType::AUTH_LOGIN, "{$method}; {$user->logDescriptor()}"); Theme::dispatch(ThemeEvents::AUTH_LOGIN, $method, $user); @@ -64,7 +63,8 @@ class LoginService throw new Exception('Login reattempt user does align with current session state'); } - $this->login($user, $this->getLastLoginAttemptMethod()); + $lastLoginDetails = $this->getLastLoginAttemptDetails(); + $this->login($user, $lastLoginDetails['method'], $lastLoginDetails['remember'] ?? false); } /** @@ -78,18 +78,10 @@ class LoginService 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} + * @return array{user_id: ?string, method: ?string, remember: bool} */ protected function getLastLoginAttemptDetails(): array { @@ -98,14 +90,14 @@ class LoginService return ['user_id' => null, 'method' => null]; } - [$id, $method, $time] = explode(':', $value); + [$id, $method, $remember, $time] = explode(':', $value); $hourAgo = time() - (60*60); if ($time < $hourAgo) { $this->clearLastLoginAttempted(); return ['user_id' => null, 'method' => null]; } - return ['user_id' => $id, 'method' => $method]; + return ['user_id' => $id, 'method' => $method, 'remember' => boolval($remember)]; } /** @@ -113,11 +105,11 @@ 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, string $method) + protected function setLastLoginAttemptedForUser(User $user, string $method, bool $remember) { session()->put( self::LAST_LOGIN_ATTEMPTED_SESSION_KEY, - implode(':', [$user->id, $method, time()]) + implode(':', [$user->id, $method, $remember, time()]) ); } @@ -159,7 +151,7 @@ class LoginService if ($result) { $user = auth()->user(); auth()->logout(); - $this->login($user, $method); + $this->login($user, $method, $remember); } return $result; From f1f59cf0861adb8f753f3685ff3c82c048ed4838 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 8 Aug 2021 14:24:44 +0100 Subject: [PATCH 22/25] Extracted text to translation files Also aligned mfa method delete route to align with others. --- .../Auth/MfaBackupCodesController.php | 2 +- resources/lang/en/auth.php | 34 ++++++++- resources/lang/en/common.php | 1 + resources/lang/en/settings.php | 4 + .../views/mfa/backup-codes-generate.blade.php | 14 ++-- .../views/mfa/setup-method-row.blade.php | 30 ++++++++ resources/views/mfa/setup.blade.php | 73 ++----------------- resources/views/mfa/totp-generate.blade.php | 22 ++---- resources/views/mfa/verify.blade.php | 20 ++--- .../views/mfa/verify/backup_codes.blade.php | 8 +- resources/views/mfa/verify/totp.blade.php | 8 +- resources/views/users/edit.blade.php | 10 +-- routes/web.php | 2 +- tests/Auth/MfaConfigurationTest.php | 4 +- 14 files changed, 104 insertions(+), 128 deletions(-) create mode 100644 resources/views/mfa/setup-method-row.blade.php diff --git a/app/Http/Controllers/Auth/MfaBackupCodesController.php b/app/Http/Controllers/Auth/MfaBackupCodesController.php index 41c161d7c..65c809196 100644 --- a/app/Http/Controllers/Auth/MfaBackupCodesController.php +++ b/app/Http/Controllers/Auth/MfaBackupCodesController.php @@ -81,7 +81,7 @@ class MfaBackupCodesController extends Controller $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.'); + $this->showWarningNotification(trans('auth.mfa_backup_codes_usage_limit_warning')); } return redirect()->intended(); diff --git a/resources/lang/en/auth.php b/resources/lang/en/auth.php index 2159cfb3b..a24ededd7 100644 --- a/resources/lang/en/auth.php +++ b/resources/lang/en/auth.php @@ -76,6 +76,36 @@ return [ 'user_invite_success' => 'Password set, you now have access to :appName!', // Multi-factor Authentication - 'mfa_use_totp' => 'Verify using a mobile app', - 'mfa_use_backup_codes' => 'Verify using a backup code', + 'mfa_setup' => 'Setup Multi-Factor Authentication', + 'mfa_setup_desc' => 'Setup multi-factor authentication as an extra layer of security for your user account.', + 'mfa_setup_configured' => 'Already Configured', + 'mfa_setup_reconfigure' => 'Reconfigure', + 'mfa_setup_remove_confirmation' => 'Are you sure you want to remove this multi-factor authentication method?', + 'mfa_setup_action' => 'Setup', + 'mfa_backup_codes_usage_limit_warning' => '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.', + 'mfa_option_totp_title' => 'Mobile App', + 'mfa_option_totp_desc' => 'To use multi-factor authentication you\'ll need a mobile application that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator.', + 'mfa_option_backup_codes_title' => 'Backup Codes', + 'mfa_option_backup_codes_desc' => 'Securely store a set of one-time-use backup codes which you can enter to verify your identity.', + 'mfa_gen_confirm_and_enable' => 'Confirm and Enable', + 'mfa_gen_backup_codes_title' => 'Backup Codes Setup', + 'mfa_gen_backup_codes_desc' => 'Store the below list of codes in a safe place. When accessing the system you\'ll be able to use one of the codes as a second authentication mechanism.', + 'mfa_gen_backup_codes_download' => 'Download Codes', + 'mfa_gen_backup_codes_usage_warning' => 'Each code can only be used once', + 'mfa_gen_totp_title' => 'Mobile App Setup', + 'mfa_gen_totp_desc' => 'To use multi-factor authentication you\'ll need a mobile application that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator.', + 'mfa_gen_totp_scan' => 'Scan the QR code below using your preferred authentication app to get started.', + 'mfa_gen_totp_verify_setup' => 'Verify Setup', + 'mfa_gen_totp_verify_setup_desc' => 'Verify that all is working by entering a code, generated within your authentication app, in the input box below:', + 'mfa_gen_totp_provide_code_here' => 'Provide your app generated code here', + 'mfa_verify_access' => 'Verify Access', + 'mfa_verify_access_desc' => '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 configured methods to continue.', + 'mfa_verify_no_methods' => 'No Methods Configured', + 'mfa_verify_no_methods_desc' => '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.', + 'mfa_verify_use_totp' => 'Verify using a mobile app', + 'mfa_verify_use_backup_codes' => 'Verify using a backup code', + 'mfa_verify_backup_code' => 'Backup Code', + 'mfa_verify_backup_code_desc' => 'Enter one of your remaining backup codes below:', + 'mfa_verify_backup_code_enter_here' => 'Enter backup code here', + 'mfa_verify_totp_desc' => 'Enter the code, generated using your mobile app, below:', ]; \ No newline at end of file diff --git a/resources/lang/en/common.php b/resources/lang/en/common.php index 1861869e3..f93fb034b 100644 --- a/resources/lang/en/common.php +++ b/resources/lang/en/common.php @@ -39,6 +39,7 @@ return [ 'reset' => 'Reset', 'remove' => 'Remove', 'add' => 'Add', + 'configure' => 'Configure', 'fullscreen' => 'Fullscreen', 'favourite' => 'Favourite', 'unfavourite' => 'Unfavourite', diff --git a/resources/lang/en/settings.php b/resources/lang/en/settings.php index d9b4854fe..87f672e4d 100755 --- a/resources/lang/en/settings.php +++ b/resources/lang/en/settings.php @@ -205,6 +205,10 @@ return [ 'users_api_tokens_create' => 'Create Token', 'users_api_tokens_expires' => 'Expires', 'users_api_tokens_docs' => 'API Documentation', + 'users_mfa' => 'Multi-Factor Authentication', + 'users_mfa_desc' => 'Setup multi-factor authentication as an extra layer of security for your user account.', + 'users_mfa_x_methods' => ':count method configured|:count methods configured', + 'users_mfa_configure' => 'Configure Methods', // API Tokens 'user_api_token_create' => 'Create API Token', diff --git a/resources/views/mfa/backup-codes-generate.blade.php b/resources/views/mfa/backup-codes-generate.blade.php index 6a491fc07..279ca68a0 100644 --- a/resources/views/mfa/backup-codes-generate.blade.php +++ b/resources/views/mfa/backup-codes-generate.blade.php @@ -4,12 +4,8 @@
-

Backup Codes

-

- Store the below list of codes in a safe place. - When accessing the system you'll be able to use one of the codes - as a second authentication mechanism. -

+

{{ trans('auth.mfa_gen_backup_codes_title') }}

+

{{ trans('auth.mfa_gen_backup_codes_desc') }}

@@ -20,18 +16,18 @@

- Download Codes + {{ trans('auth.mfa_gen_backup_codes_download') }}

- Each code can only be used once + {{ trans('auth.mfa_gen_backup_codes_usage_warning') }}

{{ csrf_field() }}
{{ trans('common.cancel') }} - +
diff --git a/resources/views/mfa/setup-method-row.blade.php b/resources/views/mfa/setup-method-row.blade.php new file mode 100644 index 000000000..e195174c1 --- /dev/null +++ b/resources/views/mfa/setup-method-row.blade.php @@ -0,0 +1,30 @@ +
+
+
{{ trans('auth.mfa_option_' . $method . '_title') }}
+

+ {{ trans('auth.mfa_option_' . $method . '_desc') }} +

+
+
+ @if($userMethods->has($method)) +
+ @icon('check-circle') + {{ trans('auth.mfa_setup_configured') }} +
+ {{ trans('auth.mfa_setup_reconfigure') }} +
+ +
+

{{ trans('auth.mfa_setup_remove_confirmation') }}

+
+ {{ csrf_field() }} + {{ method_field('delete') }} + +
+
+
+ @else + {{ trans('auth.mfa_setup_action') }} + @endif +
+
\ No newline at end of file diff --git a/resources/views/mfa/setup.blade.php b/resources/views/mfa/setup.blade.php index f670541bf..f3f290d4b 100644 --- a/resources/views/mfa/setup.blade.php +++ b/resources/views/mfa/setup.blade.php @@ -4,76 +4,13 @@
-

Setup Multi-Factor Authentication

-

- Setup multi-factor authentication as an extra layer of security - for your user account. -

+

{{ trans('auth.mfa_setup') }}

+

{{ trans('auth.mfa_setup_desc') }}

-
-
-
Mobile App
-

- To use multi-factor authentication you'll need a mobile application - that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator. -

-
-
- @if($userMethods->has('totp')) -
- @icon('check-circle') - Already configured -
- Reconfigure -
- -
-

Are you sure you want to remove this multi-factor authentication method?

-
- {{ csrf_field() }} - {{ method_field('delete') }} - -
-
-
- @else - Setup - @endif -
-
- -
-
-
Backup Codes
-

- Securely store a set of one-time-use backup codes - which you can enter to verify your identity. -

-
-
- @if($userMethods->has('backup_codes')) -
- @icon('check-circle') - Already configured -
- Reconfigure -
- -
-

Are you sure you want to remove this multi-factor authentication method?

-
- {{ csrf_field() }} - {{ method_field('delete') }} - -
-
-
- @else - Setup - @endif -
-
+ @foreach(['totp', 'backup_codes'] as $method) + @include('mfa.setup-method-row', ['method' => $method]) + @endforeach
diff --git a/resources/views/mfa/totp-generate.blade.php b/resources/views/mfa/totp-generate.blade.php index 2ed2677d1..8c470516f 100644 --- a/resources/views/mfa/totp-generate.blade.php +++ b/resources/views/mfa/totp-generate.blade.php @@ -4,14 +4,9 @@
-

Mobile App Setup

-

- To use multi-factor authentication you'll need a mobile application - that supports TOTP such as Google Authenticator, Authy or Microsoft Authenticator. -

-

- Scan the QR code below using your preferred authentication app to get started. -

+

{{ trans('auth.mfa_gen_totp_title') }}

+

{{ trans('auth.mfa_gen_totp_desc') }}

+

{{ trans('auth.mfa_gen_totp_scan') }}

@@ -19,24 +14,21 @@
-

Verify Setup

-

- Verify that all is working by entering a code, generated within your - authentication app, in the input box below: -

+

{{ trans('auth.mfa_gen_totp_verify_setup') }}

+

{{ trans('auth.mfa_gen_totp_verify_setup_desc') }}

{{ csrf_field() }} @if($errors->has('code'))
{{ $errors->first('code') }}
@endif
{{ trans('common.cancel') }} - +
diff --git a/resources/views/mfa/verify.blade.php b/resources/views/mfa/verify.blade.php index 7cab2edc4..3a1905e97 100644 --- a/resources/views/mfa/verify.blade.php +++ b/resources/views/mfa/verify.blade.php @@ -4,26 +4,18 @@
-

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 configured methods to continue. -

+

{{ trans('auth.mfa_verify_access') }}

+

{{ trans('auth.mfa_verify_access_desc') }}

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

+
{{ trans('auth.mfa_verify_no_methods') }}
+

{{ trans('auth.mfa_verify_no_methods_desc') }}

@endif - @if($method)
@include('mfa.verify.' . $method) @@ -33,7 +25,7 @@
@foreach($otherMethods as $otherMethod)
- {{ trans('auth.mfa_use_' . $otherMethod) }} + {{ trans('auth.mfa_verify_use_' . $otherMethod) }}
@endforeach @endif diff --git a/resources/views/mfa/verify/backup_codes.blade.php b/resources/views/mfa/verify/backup_codes.blade.php index 7179696cb..0e5b82086 100644 --- a/resources/views/mfa/verify/backup_codes.blade.php +++ b/resources/views/mfa/verify/backup_codes.blade.php @@ -1,14 +1,12 @@ -
Backup Code
+
{{ trans('auth.mfa_verify_backup_code') }}
-

- Enter one of your remaining backup codes below: -

+

{{ trans('auth.mfa_verify_backup_code_desc') }}

{{ csrf_field() }} @if($errors->has('code'))
{{ $errors->first('code') }}
diff --git a/resources/views/mfa/verify/totp.blade.php b/resources/views/mfa/verify/totp.blade.php index fffd30352..9a861fc6c 100644 --- a/resources/views/mfa/verify/totp.blade.php +++ b/resources/views/mfa/verify/totp.blade.php @@ -1,14 +1,12 @@ -
Mobile App
+
{{ trans('auth.mfa_option_totp_title') }}
-

- Enter the code, generated using your mobile app, below: -

+

{{ trans('auth.mfa_verify_totp_desc') }}

{{ csrf_field() }} @if($errors->has('code'))
{{ $errors->first('code') }}
diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 2d719668f..4c733e6f6 100644 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -64,11 +64,8 @@
-

Multi-Factor Authentication

-

- Setup multi-factor authentication as an extra layer of security - for your user account. -

+

{{ trans('settings.users_mfa') }}

+

{{ trans('settings.users_mfa_desc') }}

@if ($mfaMethods->count() > 0) @@ -76,11 +73,12 @@ @else @icon('cancel') @endif + {{ trans_choice('settings.users_mfa_x_methods', $mfaMethods->count()) }} {{ $mfaMethods->count() }} {{ $mfaMethods->count() === 1 ? 'method' : 'methods' }} configured
@if($user->id === user()->id) - Configure Methods + {{ trans('settings.users_mfa_configure') }} @endif
diff --git a/routes/web.php b/routes/web.php index b6590429c..a70d0d818 100644 --- a/routes/web.php +++ b/routes/web.php @@ -239,7 +239,7 @@ Route::group(['middleware' => 'guest'], function() { Route::post('/mfa/totp/verify', 'Auth\MfaTotpController@verify'); Route::post('/mfa/backup_codes/verify', 'Auth\MfaBackupCodesController@verify'); }); -Route::delete('/mfa/remove/{method}', 'Auth\MfaController@remove')->middleware('auth'); +Route::delete('/mfa/{method}/remove', 'Auth\MfaController@remove')->middleware('auth'); // Social auth routes Route::get('/login/service/{socialDriver}', 'Auth\SocialController@login'); diff --git a/tests/Auth/MfaConfigurationTest.php b/tests/Auth/MfaConfigurationTest.php index 63a5fa4dd..bdf26fb20 100644 --- a/tests/Auth/MfaConfigurationTest.php +++ b/tests/Auth/MfaConfigurationTest.php @@ -153,9 +153,9 @@ class MfaConfigurationTest extends TestCase MfaValue::upsertWithValue($admin, MfaValue::METHOD_TOTP, 'test'); $this->assertEquals(1, $admin->mfaValues()->count()); $resp = $this->actingAs($admin)->get('/mfa/setup'); - $resp->assertElementExists('form[action$="/mfa/remove/totp"]'); + $resp->assertElementExists('form[action$="/mfa/totp/remove"]'); - $resp = $this->delete("/mfa/remove/totp"); + $resp = $this->delete("/mfa/totp/remove"); $resp->assertRedirect("/mfa/setup"); $resp = $this->followRedirects($resp); $resp->assertSee('Multi-factor method successfully removed'); From 622ea03c6531df069ac8a4ed056707b0f33d0e9b Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 8 Aug 2021 14:52:29 +0100 Subject: [PATCH 23/25] Added attribution for new libs added - Also hard-set TOTP algorithm with comment from testing others. --- app/Auth/Access/Mfa/TotpService.php | 5 +++++ readme.md | 4 +++- resources/views/users/edit.blade.php | 1 - 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/app/Auth/Access/Mfa/TotpService.php b/app/Auth/Access/Mfa/TotpService.php index f9a9f416e..d1013978b 100644 --- a/app/Auth/Access/Mfa/TotpService.php +++ b/app/Auth/Access/Mfa/TotpService.php @@ -9,6 +9,7 @@ use BaconQrCode\Renderer\RendererStyle\Fill; use BaconQrCode\Renderer\RendererStyle\RendererStyle; use BaconQrCode\Writer; use PragmaRX\Google2FA\Google2FA; +use PragmaRX\Google2FA\Support\Constants; class TotpService { @@ -17,6 +18,10 @@ class TotpService public function __construct(Google2FA $google2fa) { $this->google2fa = $google2fa; + // Use SHA1 as a default, Personal testing of other options in 2021 found + // many apps lack support for other algorithms yet still will scan + // the code causing a confusing UX. + $this->google2fa->setAlgorithm(Constants::SHA1); } /** diff --git a/readme.md b/readme.md index 1b8c66061..c0bef7c7a 100644 --- a/readme.md +++ b/readme.md @@ -189,4 +189,6 @@ These are the great open-source projects used to help build BookStack: * [OneLogin's SAML PHP Toolkit](https://github.com/onelogin/php-saml) * [League/CommonMark](https://commonmark.thephpleague.com/) * [League/Flysystem](https://flysystem.thephpleague.com) -* [StyleCI](https://styleci.io/) \ No newline at end of file +* [StyleCI](https://styleci.io/) +* [pragmarx/google2fa](https://github.com/antonioribeiro/google2fa) +* [Bacon/BaconQrCode](https://github.com/Bacon/BaconQrCode) \ No newline at end of file diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 4c733e6f6..d882558a4 100644 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -74,7 +74,6 @@ @icon('cancel') @endif {{ trans_choice('settings.users_mfa_x_methods', $mfaMethods->count()) }} - {{ $mfaMethods->count() }} {{ $mfaMethods->count() === 1 ? 'method' : 'methods' }} configured
@if($user->id === user()->id) From 78e94bb0035d7ecc6f4267a4ffa919ec34b1ac2e Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 21 Aug 2021 15:14:24 +0100 Subject: [PATCH 24/25] Improved login redirect and setup experience - Updated auth system for mfa to not update intended URL so that the user is not redirected to mfa setup after eventual login. - Added notification for users setting up MFA, after setup when redirected back to login screen to advise that MFA setup was complete but they need to login again. - Updated some bits of wording to display better. --- app/Http/Controllers/Auth/MfaBackupCodesController.php | 6 ++++++ app/Http/Controllers/Auth/MfaTotpController.php | 5 +++++ app/Http/Middleware/AuthenticatedOrPendingMfa.php | 2 +- resources/lang/en/auth.php | 3 ++- tests/Auth/AuthTest.php | 8 ++++++++ tests/Auth/MfaVerificationTest.php | 6 +++++- 6 files changed, 27 insertions(+), 3 deletions(-) diff --git a/app/Http/Controllers/Auth/MfaBackupCodesController.php b/app/Http/Controllers/Auth/MfaBackupCodesController.php index 65c809196..4b4e11659 100644 --- a/app/Http/Controllers/Auth/MfaBackupCodesController.php +++ b/app/Http/Controllers/Auth/MfaBackupCodesController.php @@ -49,6 +49,12 @@ class MfaBackupCodesController extends Controller MfaValue::upsertWithValue($this->currentOrLastAttemptedUser(), MfaValue::METHOD_BACKUP_CODES, json_encode($codes)); $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'backup-codes'); + + if (!auth()->check()) { + $this->showSuccessNotification(trans('auth.mfa_setup_login_notification')); + return redirect('/login'); + } + return redirect('/mfa/setup'); } diff --git a/app/Http/Controllers/Auth/MfaTotpController.php b/app/Http/Controllers/Auth/MfaTotpController.php index a1701c4ce..d55f08cff 100644 --- a/app/Http/Controllers/Auth/MfaTotpController.php +++ b/app/Http/Controllers/Auth/MfaTotpController.php @@ -61,6 +61,11 @@ class MfaTotpController extends Controller session()->remove(static::SETUP_SECRET_SESSION_KEY); $this->logActivity(ActivityType::MFA_SETUP_METHOD, 'totp'); + if (!auth()->check()) { + $this->showSuccessNotification(trans('auth.mfa_setup_login_notification')); + return redirect('/login'); + } + return redirect('/mfa/setup'); } diff --git a/app/Http/Middleware/AuthenticatedOrPendingMfa.php b/app/Http/Middleware/AuthenticatedOrPendingMfa.php index 2d68a2a57..febfef207 100644 --- a/app/Http/Middleware/AuthenticatedOrPendingMfa.php +++ b/app/Http/Middleware/AuthenticatedOrPendingMfa.php @@ -36,6 +36,6 @@ class AuthenticatedOrPendingMfa return $next($request); } - return redirect()->guest(url('/login')); + return redirect()->to(url('/login')); } } diff --git a/resources/lang/en/auth.php b/resources/lang/en/auth.php index a24ededd7..e4d4c425b 100644 --- a/resources/lang/en/auth.php +++ b/resources/lang/en/auth.php @@ -78,7 +78,7 @@ return [ // Multi-factor Authentication 'mfa_setup' => 'Setup Multi-Factor Authentication', 'mfa_setup_desc' => 'Setup multi-factor authentication as an extra layer of security for your user account.', - 'mfa_setup_configured' => 'Already Configured', + 'mfa_setup_configured' => 'Already configured', 'mfa_setup_reconfigure' => 'Reconfigure', 'mfa_setup_remove_confirmation' => 'Are you sure you want to remove this multi-factor authentication method?', 'mfa_setup_action' => 'Setup', @@ -108,4 +108,5 @@ return [ 'mfa_verify_backup_code_desc' => 'Enter one of your remaining backup codes below:', 'mfa_verify_backup_code_enter_here' => 'Enter backup code here', 'mfa_verify_totp_desc' => 'Enter the code, generated using your mobile app, below:', + 'mfa_setup_login_notification' => 'Multi-factor method configured, Please now login again using the configured method.', ]; \ No newline at end of file diff --git a/tests/Auth/AuthTest.php b/tests/Auth/AuthTest.php index 085482c35..b4b99d130 100644 --- a/tests/Auth/AuthTest.php +++ b/tests/Auth/AuthTest.php @@ -419,6 +419,14 @@ class AuthTest extends BrowserKitTest $login->assertRedirectedTo('http://localhost'); } + public function test_login_intended_redirect_does_not_factor_mfa_routes() + { + $this->get('/books')->assertRedirectedTo('/login'); + $this->get('/mfa/setup')->assertRedirectedTo('/login'); + $login = $this->post('/login', ['email' => 'admin@admin.com', 'password' => 'password']); + $login->assertRedirectedTo('/books'); + } + public function test_login_authenticates_admins_on_all_guards() { $this->post('/login', ['email' => 'admin@admin.com', 'password' => 'password']); diff --git a/tests/Auth/MfaVerificationTest.php b/tests/Auth/MfaVerificationTest.php index d007fa490..e63094303 100644 --- a/tests/Auth/MfaVerificationTest.php +++ b/tests/Auth/MfaVerificationTest.php @@ -187,11 +187,15 @@ class MfaVerificationTest extends TestCase $resp->assertElementContains('a[href$="/mfa/setup"]', 'Configure'); $this->get('/mfa/backup_codes/generate'); - $this->followingRedirects()->post('/mfa/backup_codes/confirm'); + $resp = $this->post('/mfa/backup_codes/confirm'); + $resp->assertRedirect('/login'); $this->assertDatabaseHas('mfa_values', [ 'user_id' => $user->id, ]); + $resp = $this->get('/login'); + $resp->assertSeeText('Multi-factor method configured, Please now login again using the configured method.'); + $resp = $this->followingRedirects()->post('/login', [ 'email' => $user->email, 'password' => 'password', From 2d306949b55f272504e745c167a2f46ccf72dbcb Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sat, 21 Aug 2021 15:38:43 +0100 Subject: [PATCH 25/25] Cleaned some unused elements during testing --- app/Auth/Access/Mfa/BackupCodeService.php | 10 ++-------- app/Auth/Access/Mfa/MfaValue.php | 11 ----------- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/app/Auth/Access/Mfa/BackupCodeService.php b/app/Auth/Access/Mfa/BackupCodeService.php index 6fbbf64b5..ca58e7404 100644 --- a/app/Auth/Access/Mfa/BackupCodeService.php +++ b/app/Auth/Access/Mfa/BackupCodeService.php @@ -34,20 +34,14 @@ class BackupCodeService /** * Remove the given input code from the given available options. - * Will return null if no codes remain otherwise will be a JSON string to contain - * the codes. + * Will return a JSON string containing the codes. */ - public function removeInputCodeFromSet(string $code, string $codeSet): ?string + public function removeInputCodeFromSet(string $code, string $codeSet): string { $cleanCode = $this->cleanInputCode($code); $codes = json_decode($codeSet); $pos = array_search($cleanCode, $codes, true); array_splice($codes, $pos, 1); - - if (count($codes) === 0) { - return null; - } - return json_encode($codes); } diff --git a/app/Auth/Access/Mfa/MfaValue.php b/app/Auth/Access/Mfa/MfaValue.php index 228a6d43f..ec0d5d563 100644 --- a/app/Auth/Access/Mfa/MfaValue.php +++ b/app/Auth/Access/Mfa/MfaValue.php @@ -58,17 +58,6 @@ class MfaValue extends Model return $mfaVal ? $mfaVal->getValue() : null; } - /** - * Delete any stored MFA values for the given user and method. - */ - public static function deleteValuesForUser(User $user, string $method): void - { - static::query() - ->where('user_id', '=', $user->id) - ->where('method', '=', $method) - ->delete(); - } - /** * Decrypt the value attribute upon access. */