From a3f19ebe96063eae54384fd56d2da8b66e625c14 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Mon, 2 Aug 2021 15:04:43 +0100 Subject: [PATCH] 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.

- Configure + Configure
@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