From 39a14cff8f64f51fa3b4be2f4c2a7b85d7c2b600 Mon Sep 17 00:00:00 2001 From: Dan Brown Date: Sun, 17 May 2026 13:00:38 +0100 Subject: [PATCH] User MFA: Reviewed addition of reset, added tests Review of #6056 Added test coverage. --- app/Activity/ActivityType.php | 1 + app/Users/Controllers/UserController.php | 10 ++-- lang/en/activities.php | 2 + lang/en/settings.php | 8 ++-- resources/views/users/edit.blade.php | 44 ++++++++++++------ routes/web.php | 2 +- tests/User/UserManagementMfaTest.php | 59 ++++++++++++++++++++++++ 7 files changed, 102 insertions(+), 24 deletions(-) create mode 100644 tests/User/UserManagementMfaTest.php diff --git a/app/Activity/ActivityType.php b/app/Activity/ActivityType.php index a7f129f71..64532de17 100644 --- a/app/Activity/ActivityType.php +++ b/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'; diff --git a/app/Users/Controllers/UserController.php b/app/Users/Controllers/UserController.php index ba85f99fb..f73104381 100644 --- a/app/Users/Controllers/UserController.php +++ b/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}"); } } diff --git a/lang/en/activities.php b/lang/en/activities.php index 4362fc029..e9344a3d4 100644 --- a/lang/en/activities.php +++ b/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', diff --git a/lang/en/settings.php b/lang/en/settings.php index 920c3c1bc..d03024a89 100644 --- a/lang/en/settings.php +++ b/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', diff --git a/resources/views/users/edit.blade.php b/resources/views/users/edit.blade.php index 64d45f503..8a4a8bcce 100644 --- a/resources/views/users/edit.blade.php +++ b/resources/views/users/edit.blade.php @@ -71,21 +71,35 @@ - @if(user()->hasSystemRole('admin')) -
-
-
-
- {{ trans('settings.users_mfa_reset') }} -

{{ trans('settings.users_mfa_reset_desc', ['userName' => $user->name]) }}

-
-
-
id}/reset-mfa") }}" method="POST" style="display: inline;"> - @csrf - + @if($mfaMethods->count() > 0) +
+ +
+
+

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

+ id}/mfa") }}" method="POST" style="display: inline;"> + {{ csrf_field() }} + {{ method_field('DELETE') }} +
+ + +
diff --git a/routes/web.php b/routes/web.php index 2571da2f3..3e3b5aef3 100644 --- a/routes/web.php +++ b/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']); diff --git a/tests/User/UserManagementMfaTest.php b/tests/User/UserManagementMfaTest.php new file mode 100644 index 000000000..ea05869cb --- /dev/null +++ b/tests/User/UserManagementMfaTest.php @@ -0,0 +1,59 @@ +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()); + } +}