From 459706908311101286dadb87a2f12afbf4192bbb Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Mon, 2 Aug 2021 16:35:37 +0100
Subject: [PATCH] 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)
<hr class="my-l">
@foreach($otherMethods as $otherMethod)
- <a href="{{ url("/mfa/verify?method={$otherMethod}") }}">Use {{$otherMethod}}</a>
+ <div class="text-center">
+ <a href="{{ url("/mfa/verify?method={$otherMethod}") }}">{{ trans('auth.mfa_use_' . $otherMethod) }}</a>
+ </div>
@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 @@
+<div class="setting-list-label">Backup Code</div>
+
+<p class="small mb-m">
+ Enter one of your remaining backup codes below:
+</p>
+
+<form action="{{ url('/mfa/verify/backup_codes') }}" method="post">
+ {{ csrf_field() }}
+ <input type="text"
+ name="code"
+ placeholder="Enter backup code here"
+ class="input-fill-width {{ $errors->has('code') ? 'neg' : '' }}">
+ @if($errors->has('code'))
+ <div class="text-neg text-small px-xs">{{ $errors->first('code') }}</div>
+ @endif
+ <div class="mt-s text-right">
+ <button class="button">{{ trans('common.confirm') }}</button>
+ </div>
+</form>
\ 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() }}
<input type="text"
name="code"
- aria-labelledby="totp-verify-input-details"
placeholder="Provide your app generated code here"
class="input-fill-width {{ $errors->has('code') ? 'neg' : '' }}">
@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<User, string, TestResponse>
*/
@@ -72,4 +180,21 @@ class MfaVerificationTest extends TestCase
return [$user, $secret, $loginResp];
}
+ /**
+ * @return Array<User, string, TestResponse>
+ */
+ 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