Add reset MFA button for admin s on user profile edit#6056
Add reset MFA button for admin s on user profile edit#6056clauvaldez wants to merge 1 commit intoBookStackApp:developmentfrom
Conversation
ssddanbrown
left a comment
ssddanbrown
left a comment
There was a problem hiding this comment.
Thanks for offering this!
I've added a range of comments regarding some changes to be made.
We will also need testing coverage of the core actions here. If unsure, I can add these later.
Note: If you're heavily using AI/LLM generation for this contribution please let me know, as I don't want to be spending time reviewing and providing feedback to a machine via a proxy.
If anything is unclear or you need further guidance just let me know!
| $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])); |
There was a problem hiding this comment.
Please can you use the $this->showSuccessNotification method instead to show a notification
| { | ||
| $this->checkPermission(Permission::UsersManage); | ||
| $user = $this->userRepo->getById($id); | ||
| // Resetear el 2FA del usuario |
There was a problem hiding this comment.
Remove this comment, I think it's a bit redundant based on context and I'd prefer to keep comments in one language.
| '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', |
There was a problem hiding this comment.
Please can you use "multi-factor authentication" instead of "2FA" to keep the language used consistent in the app.
Also, avoid passing through the username here. We only do that where necessary, to keep translations simpler and avoid other issues, but it can be implied by context here so I don't think it's needed.
| </div> | ||
| </div> | ||
|
|
||
| @if(user()->hasSystemRole('admin')) |
There was a problem hiding this comment.
This does not align with the actual permission used in the controller, and we're already checking the relevant permissions before providing the view, so a permission check here is redundant.
Instead, We should probably only show this section if the user has at least one MFA option configured.
| '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.', |
There was a problem hiding this comment.
This is kind of incorrect. They will only be prompted to reconfigure on login if MFA is required for their role.
| // Resetear el 2FA del usuario | ||
| $user->mfaValues()->delete(); | ||
| session()->flash('success', trans('settings.users_mfa_reset_success', ['userName' => $user->name])); | ||
| return redirect()->back(); |
There was a problem hiding this comment.
Can we redirect directly to the user edit view?
Redirect backs like this can go wonky in a range of scenarios, so best to specify a location where known.
| <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]) }}')"> |
There was a problem hiding this comment.
This won't work due to CSP (security) blocking. We avoid any inline JavaScript.
Making the initial a dropdown, with a second button for confirmation, may be better. We do this in a select few other areas I think.
|
Hello! |
2 participants
Summary
This pull request adds a Reset MFA button to the user profile edit page, allowing administrators to easily reset a user's multi-factor authentication configuration.
Motivation
If a user loses access to their MFA device or is unable to complete the authentication process, administrators currently have no quick way to reset MFA from the interface. This feature provides a simple and controlled way for admins to resolve such situations.
Changes
Security
Only administrators with the
UsersManagepermission can perform this action.Result
Administrators can quickly reset MFA for users who are locked out due to lost or misconfigured authentication devices, improving support and account recovery workflows.