User MFA: Reviewed addition of reset, added tests

Review of #6056
Added test coverage.

app/Activity/ActivityType.php

@@ -46,6 +46,7 @@ class ActivityType
     const USER_CREATE = 'user_create';
     const USER_UPDATE = 'user_update';
     const USER_DELETE = 'user_delete';
+    const USER_MFA_RESET = 'user_mfa_reset';
 
     const API_TOKEN_CREATE = 'api_token_create';
     const API_TOKEN_UPDATE = 'api_token_update';

app/Users/Controllers/UserController.php

@@ -4,6 +4,7 @@ namespace BookStack\Users\Controllers;
 
 use BookStack\Access\SocialDriverManager;
 use BookStack\Access\UserInviteException;
+use BookStack\Activity\ActivityType;
 use BookStack\Exceptions\ImageUploadException;
 use BookStack\Exceptions\UserUpdateException;
 use BookStack\Http\Controller;
@@ -214,11 +215,14 @@ class UserController extends Controller
      */
     public function resetMfa(Request $request, int $id)
     {
+        $this->preventAccessInDemoMode();
         $this->checkPermission(Permission::UsersManage);
+
         $user = $this->userRepo->getById($id);
-        // Resetear el 2FA del usuario 
         $user->mfaValues()->delete();
-        session()->flash('success', trans('settings.users_mfa_reset_success', ['userName' => $user->name]));
-        return redirect()->back();
+
+        $this->logActivity(ActivityType::USER_MFA_RESET, $user);
+
+        return redirect("/settings/users/{$user->id}");
     }
 }

lang/en/activities.php

@@ -99,6 +99,8 @@ return [
     'user_update_notification' => 'User successfully updated',
     'user_delete' => 'deleted user',
     'user_delete_notification' => 'User successfully removed',
+    'user_mfa_reset' => 'reset MFA for user',
+    'user_mfa_reset_notification' => 'Multi-factor authentication methods reset',
 
     // API Tokens
     'api_token_create' => 'created API token',

lang/en/settings.php

@@ -264,11 +264,9 @@ return [
     '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',
-    'users_mfa_reset' => 'Reset 2FA',
-    'users_mfa_reset_desc' => 'Reset and clear all configured MFA methods for :userName. They will be prompted to reconfigure on next login.',
-    'users_mfa_reset_confirm' => 'Are you sure you want to reset 2FA for :userName?',
-    'users_mfa_reset_success' => '2FA has been reset for :userName',
-    'users_mfa_reset_error' => 'Failed to reset 2FA for :userName',
+    'users_mfa_reset' => 'Reset Multi-Factor Authentication Methods',
+    'users_mfa_reset_desc' => 'This will reset and clear all configured multi-factor authentication methods for this user. If multi-factor authentication is required by any of their roles, they\'ll be prompted to configure new methods on their next login.',
+    'users_mfa_reset_confirm' => 'Are you sure you want to reset multi-factor authentication for this user?',
 
     // API Tokens
     'user_api_token_create' => 'Create API Token',

resources/views/users/edit.blade.php

@@ -71,21 +71,35 @@
                 </div>
             </div>
 
-            @if(user()->hasSystemRole('admin'))
-                <div class="mt-xl">
-                    <hr class="my-m">
-                    <div class="grid half gap-xl v-center">
-                        <div>
-                            <strong class="text-neg">{{ trans('settings.users_mfa_reset') }}</strong>
-                            <p class="text-small text-muted">{{ trans('settings.users_mfa_reset_desc', ['userName' => $user->name]) }}</p>
-                        </div>
-                        <div class="text-m-right">
-                            <form action="{{ url("/settings/users/{$user->id}/reset-mfa") }}" method="POST" style="display: inline;">
-                                @csrf
-                                <button type="submit" class="button neg" 
-                                        onclick="return confirm('{{ trans('settings.users_mfa_reset_confirm', ['userName' => $user->name]) }}')">
-                                    {{ trans('settings.users_mfa_reset') }}
-                                </button>
+            @if($mfaMethods->count() > 0)
+                <div id="reset-mfa" component="collapsible" class="form-group collapsible mt-s">
+                    <button refs="collapsible@trigger" type="button" class="collapse-title text-link" aria-expanded="false">
+                        <label for="mfa-reset">{{ trans('settings.users_mfa_reset') }}</label>
+                    </button>
+                    <div refs="collapsible@content" class="collapse-content">
+                        <div class="flex-container-row justify-space-between gap-m wrap items-center">
+                            <p class="text-small text-muted mb-none flex-2 min-width-m">{{ trans('settings.users_mfa_reset_desc') }}</p>
+                            <form action="{{ url("/settings/users/{$user->id}/mfa") }}" method="POST" style="display: inline;">
+                                {{ csrf_field() }}
+                                {{ method_field('DELETE') }}
+                                <div class="dropdown-container" component="dropdown" option:dropdown:bubble-escapes="true">
+                                    <button class="button" refs="dropdown@toggle"
+                                            type="button"
+                                            aria-haspopup="listbox"
+                                            aria-expanded="{{ trans('settings.users_mfa_reset') }}">
+                                        {{ trans('common.reset') }}
+                                    </button>
+                                    <div refs="dropdown@menu" class="dropdown-menu" role="menu" aria-label="{{ trans('settings.users_mfa_reset') }}">
+                                        <p class="text-neg small px-m mb-xs">
+                                            {{ trans('settings.users_mfa_reset_confirm') }}
+                                        </p>
+                                        <div class="text-link small delete text-item">
+                                            <button type="submit" class="text-button neg">
+                                                {{ trans('common.reset') }}
+                                            </button>
+                                        </div>
+                                    </div>
+                                </div>
                             </form>
                         </div>
                     </div>

routes/web.php

@@ -251,7 +251,7 @@ Route::middleware('auth')->group(function () {
     Route::get('/settings/users/{id}', [UserControllers\UserController::class, 'edit']);
     Route::put('/settings/users/{id}', [UserControllers\UserController::class, 'update']);
     Route::delete('/settings/users/{id}', [UserControllers\UserController::class, 'destroy']);
-    Route::post('/settings/users/{id}/reset-mfa', [UserControllers\UserController::class, 'resetMfa']);
+    Route::delete('/settings/users/{id}/mfa', [UserControllers\UserController::class, 'resetMfa']);
 
     // User Account
     Route::get('/my-account', [UserControllers\UserAccountController::class, 'redirect']);

tests/User/UserManagementMfaTest.php

@@ -0,0 +1,59 @@
+<?php
+
+namespace Tests\User;
+
+use BookStack\Access\Mfa\MfaValue;
+use BookStack\Activity\ActivityType;
+use BookStack\Permissions\Permission;
+use Tests\TestCase;
+
+class UserManagementMfaTest extends TestCase
+{
+    public function test_configured_mfa_options_visible_on_user_edit()
+    {
+        $editor = $this->users->editor();
+
+        $resp = $this->asAdmin()->get($editor->getEditUrl());
+        $resp->assertSeeText('0 methods configured');
+
+        MfaValue::factory()->create(['user_id' => $editor->id, 'method' => MfaValue::METHOD_BACKUP_CODES]);
+
+        $resp = $this->get($editor->getEditUrl());
+        $resp->assertSeeText('1 method configured');
+        $resp->assertDontSeeText('0 methods configured');
+    }
+
+    public function test_reset_mfa_flow()
+    {
+        $editor = $this->users->editor();
+        MfaValue::factory()->create(['user_id' => $editor->id, 'method' => MfaValue::METHOD_BACKUP_CODES]);
+        MfaValue::factory()->create(['user_id' => $editor->id, 'method' => MfaValue::METHOD_TOTP]);
+
+        $this->assertEquals(2, $editor->mfaValues()->count());
+
+        $resp = $this->asAdmin()->get($editor->getEditUrl());
+        $this->withHtml($resp)->assertElementContains('form[action$="/mfa"] button[type="submit"]', 'Reset');
+
+        $resp = $this->delete($editor->getEditUrl('/mfa'));
+        $resp->assertRedirect($editor->getEditUrl());
+        $this->assertActivityExists(ActivityType::USER_MFA_RESET);
+
+        $resp = $this->followRedirects($resp);
+        $resp->assertSee('Multi-factor authentication methods reset');
+
+        $this->assertEquals(0, $editor->mfaValues()->count());
+    }
+
+    public function test_users_manage_permission_required_for_mfa_reset()
+    {
+        $editor = $this->users->editor();
+        $resp = $this->actingAs($editor)->delete($editor->getEditUrl('/mfa'));
+        $this->assertPermissionError($resp);
+
+        $this->permissions->grantUserRolePermissions($editor, [Permission::UsersManage]);
+
+        $resp = $this->delete($editor->getEditUrl('/mfa'));
+        $this->assertNotPermissionError($resp);
+        $resp->assertRedirect($editor->getEditUrl());
+    }
+}