Add reset MFA button for admin s on user profile edit #6056
No reviewers
Labels
No labels
Focus: A11y
Focus: Admin/Meta
Focus: Authentication
Focus: Back-End
Focus: Database
Focus: Design & UX
Focus: Editor - Markdown
Focus: Editor - WYSIWYG
Focus: Export System
Focus: Front-End
Focus: Translations
Focus: View Customization
Is: Docs Update
Is: Enhancement
Is: Priority
Is: Security
Is: Upstream
Status
Blocked
Status
Open to discussion
Status
Out of scope
Status
Pending Validation
Type
API Request
Type
Bug Report
Type
Feature Request
Type
Happy feedback
Type
Maintenance
Type
Question
Type
Support
No milestone
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
bookstack/bookstack!6056
Loading…
Reference in a new issue
No description provided.
Delete branch "clauvaldez/mfaReset"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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!
@ -211,0 +216,4 @@{$this->checkPermission(Permission::UsersManage);$user = $this->userRepo->getById($id);// Resetear el 2FA del usuarioRemove this comment, I think it's a bit redundant based on context and I'd prefer to keep comments in one language.
@ -211,0 +218,4 @@$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]));Please can you use the
$this->showSuccessNotificationmethod instead to show a notification@ -211,0 +219,4 @@// Resetear el 2FA del usuario$user->mfaValues()->delete();session()->flash('success', trans('settings.users_mfa_reset_success', ['userName' => $user->name]));return redirect()->back();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.
@ -264,2 +264,4 @@'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.',This is kind of incorrect. They will only be prompted to reconfigure on login if MFA is required for their role.
@ -266,0 +267,4 @@'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',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.
@ -71,6 +71,26 @@</div></div>@if(user()->hasSystemRole('admin'))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.
@ -74,0 +83,4 @@<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]) }}')">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!
Thanks for the answers, I am working on the recommendations, it is the first time I make a contribution so there are many details to take into account.
Thank you very much for taking the time, I will work on the details you mentioned and
I wanted to get this into the next release so have gone ahead and made the changes needed within
39a14cff8f, before merging.Thanks again clauvaldez for providing this PR.