]> BookStack Code Mirror - bookstack/commitdiff
Added Backup code verification logic
authorDan Brown <redacted>
Mon, 2 Aug 2021 15:35:37 +0000 (16:35 +0100)
committerDan Brown <redacted>
Mon, 2 Aug 2021 15:35:37 +0000 (16:35 +0100)
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.

12 files changed:
app/Auth/Access/LoginService.php
app/Auth/Access/Mfa/BackupCodeService.php
app/Auth/Access/Mfa/MfaValue.php
app/Http/Controllers/Auth/MfaBackupCodesController.php
app/Http/Controllers/Auth/MfaController.php
resources/lang/en/auth.php
resources/lang/en/validation.php
resources/views/mfa/verify.blade.php
resources/views/mfa/verify/backup_codes.blade.php [new file with mode: 0644]
resources/views/mfa/verify/totp.blade.php
routes/web.php
tests/Auth/MfaVerificationTest.php

index 9823caafa65a7b1d7bb8a0457bc19b642dc89c57..cc0cb06f330ee3a35e27aa1ff10c1c04dc33c72f 100644 (file)
@@ -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
index cc533bd317db8162a3caac7eb51cdefacc7ce723..6fbbf64b5d8b50cab078d688f03d7f4485b26c9c 100644 (file)
@@ -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
index ec0d5d563fea7b49b60aaa7cc4ff8e99f4e0e665..228a6d43f917ad9bdd9e3335bdbadd58250cccad 100644 (file)
@@ -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.
      */
index b8905056533212b6f9c15c77a4711a12a29b59dc..1353d4562870658fe08845b1ce5deeed4109932b 100644 (file)
@@ -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();
+    }
 }
index 6d868b6f3473400c7505c2675f5ab157ef3b2d21..75cd46eefecfedf87f8447d8dac5b93bf2dbaf56 100644 (file)
@@ -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()
index d64fce93a62d90889b2297a9e4f6482ad9046475..2159cfb3bbecfe8aea2e0f3879236f4d33b6d2be 100644 (file)
@@ -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
index 5796d04c6a76c69aeb5f29e866beef5b3ee450fa..1963b0df2f9a9bb3d2586fc71fbc2ca04e02f97d 100644 (file)
@@ -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.',
index e818852c23689c0d6145910830e06af8c42e7b67..7cab2edc4a99947efe65a8393098b1bfa9f62683 100644 (file)
@@ -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 (file)
index 0000000..abfb8a9
--- /dev/null
@@ -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
index 7ff691552393c32ecc9681364d83639562382800..55784dc8f9650284a77e3f33245ad6c4728e76f8 100644 (file)
@@ -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'))
index 029649685e9afd07454763ae01cc00a0080c8da0..eadbca5e8fd3bdf3f1432b03205829ae21e0c683 100644 (file)
@@ -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');
index f19700a5a3070d73f9c3816209e9c8299121a3c1..3f272cffbcb043360429041689651518df9e9c2c 100644 (file)
@@ -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