feat: add a feature for an admin to reset the mfa of another user#4270
Open
tchoumi313 wants to merge 2 commits into
Open
feat: add a feature for an admin to reset the mfa of another user#4270tchoumi313 wants to merge 2 commits into
tchoumi313 wants to merge 2 commits into
Conversation
Adds a new admin-only endpoint that wipes all MFA authenticators (TOTP,WebAuthn, recovery codes) for a target user so they can re-enroll on next login, covering the lost-phone / lost-key recovery scenario.
…munity into CA-1708-add-a-feature-for-an-admin-to-reset-the-mfa-of-another-user
Contributor
📝 WalkthroughWalkthroughThis PR introduces an admin-initiated MFA reset feature allowing administrators to wipe multi-factor authentication from users who have lost their second factors. It includes a backend REST API endpoint ( ChangesAdmin MFA Reset Functionality
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
frontend/src/routes/(app)/(internal)/users/[id=uuid]/edit/reset-mfa/+page.server.ts (1)
8-12: 💤 Low valueConsider adding error handling for the user fetch.
If the user ID is invalid or the user doesn't exist, this fetch will fail without a try/catch block. While SvelteKit will handle the error, adding explicit error handling would provide better UX.
🛡️ Suggested error handling
export const load: PageServerLoad = async ({ params, fetch }) => { const objectEndpoint = `${BASE_API_URL}/users/${params.id}/object/`; - const object = await fetch(objectEndpoint).then((res) => res.json()); + const res = await fetch(objectEndpoint); + if (!res.ok) { + throw error(res.status, 'User not found'); + } + const object = await res.json(); return { object, title: m.resetMFA() }; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/src/routes/`(app)/(internal)/users/[id=uuid]/edit/reset-mfa/+page.server.ts around lines 8 - 12, The load function (export const load: PageServerLoad) currently fetches the user object without error handling; wrap the fetch to `${BASE_API_URL}/users/${params.id}/object/` in a try/catch and check the response.ok before calling res.json(), and on failure either throw a SvelteKit error (with appropriate status and message) or return a controlled value (e.g., null plus an error flag) so the page can show a friendly UX; update the return to still include title: m.resetMFA() and the handled error/object shape so downstream code can render an error state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/iam/views.py`:
- Around line 492-500: Replace the two-step exists-then-delete with a single
delete-then-check: call Authenticator.objects.filter(user=target_user).delete()
first (assign to deleted_count, _), then if deleted_count == 0 return the same
Response({"error": "userHasNoMFAEnabled"}, status=status.HTTP_400_BAD_REQUEST);
otherwise continue with the existing flow (e.g., logging via logger.warning and
any subsequent cleanup) so you avoid the race between exists() and delete().
In `@frontend/messages/pl.json`:
- Line 2368: Replace the Polish term "składnika" with the correct MFA term
"czynnika" in the localization entries: update the value for key "resetMFA1"
from "Jeśli ten użytkownik utracił dostęp do drugiego składnika, możesz" to
"Jeśli ten użytkownik utracił dostęp do drugiego czynnika, możesz" and likewise
change the message at the nearby entry on line 2371 (the string that currently
says "Użytkownik straci dostęp do drugiego składnika...") to use "czynnika"
instead.
In `@product-docs/configuration/mfa.md`:
- Around line 83-85: Replace the hard-coded instruction "type `yes`" in the
confirmation step of the MFA reset docs (the line containing: "On the
confirmation page, type `yes` and submit." and its accompanying caption/figure)
with a locale-agnostic instruction such as "type the localized word for 'yes'
shown on screen" so the guidance matches the UI validation across locales;
update both the inline step text and the figure caption to refer to the
localized confirmation word.
---
Nitpick comments:
In
`@frontend/src/routes/`(app)/(internal)/users/[id=uuid]/edit/reset-mfa/+page.server.ts:
- Around line 8-12: The load function (export const load: PageServerLoad)
currently fetches the user object without error handling; wrap the fetch to
`${BASE_API_URL}/users/${params.id}/object/` in a try/catch and check the
response.ok before calling res.json(), and on failure either throw a SvelteKit
error (with appropriate status and message) or return a controlled value (e.g.,
null plus an error flag) so the page can show a friendly UX; update the return
to still include title: m.resetMFA() and the handled error/object shape so
downstream code can render an error state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84584281-dc61-4c64-8e4c-5485d87a30f5
⛔ Files ignored due to path filters (2)
product-docs/.gitbook/assets/mfa-in-settings.pngis excluded by!**/*.pngproduct-docs/.gitbook/assets/mfa-reset-confirmation.pngis excluded by!**/*.png
📒 Files selected for processing (37)
backend/app_tests/api/test_api_reset_mfa.pybackend/core/serializers.pybackend/iam/serializers.pybackend/iam/urls.pybackend/iam/views.pyfrontend/messages/ar.jsonfrontend/messages/cs.jsonfrontend/messages/da.jsonfrontend/messages/de.jsonfrontend/messages/el.jsonfrontend/messages/en.jsonfrontend/messages/es.jsonfrontend/messages/et.jsonfrontend/messages/fr.jsonfrontend/messages/hi.jsonfrontend/messages/hr.jsonfrontend/messages/hu.jsonfrontend/messages/id.jsonfrontend/messages/it.jsonfrontend/messages/ko.jsonfrontend/messages/lt.jsonfrontend/messages/nl.jsonfrontend/messages/pl.jsonfrontend/messages/pt.jsonfrontend/messages/ro.jsonfrontend/messages/sv.jsonfrontend/messages/tr.jsonfrontend/messages/uk.jsonfrontend/messages/ur.jsonfrontend/messages/zh.jsonfrontend/src/lib/utils/schemas.tsfrontend/src/routes/(app)/(internal)/users/[id=uuid]/edit/+page.sveltefrontend/src/routes/(app)/(internal)/users/[id=uuid]/edit/reset-mfa/+page.server.tsfrontend/src/routes/(app)/(internal)/users/[id=uuid]/edit/reset-mfa/+page.svelteproduct-docs/configuration/mfa.mdproduct-docs/configuration/organization/iam-model.mdproduct-docs/configuration/organization/users.md
Comment on lines
+492
to
+500
| authenticators = Authenticator.objects.filter(user=target_user) | ||
| if not authenticators.exists(): | ||
| return Response( | ||
| {"error": "userHasNoMFAEnabled"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| deleted_count, _ = authenticators.delete() | ||
| logger.warning( |
Contributor
There was a problem hiding this comment.
Avoid the exists-then-delete race in MFA reset.
The current two-step check can return success if another request deletes authenticators between the queries. Delete first and branch on deleted rows.
Suggested patch
- authenticators = Authenticator.objects.filter(user=target_user)
- if not authenticators.exists():
- return Response(
- {"error": "userHasNoMFAEnabled"},
- status=status.HTTP_400_BAD_REQUEST,
- )
-
- deleted_count, _ = authenticators.delete()
+ authenticators = Authenticator.objects.filter(user=target_user)
+ deleted_rows, _ = authenticators.delete()
+ if deleted_rows == 0:
+ return Response(
+ {"error": "userHasNoMFAEnabled"},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
logger.warning(
"Admin reset MFA for another user",
admin_id=str(request.user.id),
admin_email=request.user.email,
target_user_id=str(target_user.id),
target_user_email=target_user.email,
- deleted_authenticators=deleted_count,
+ deleted_authenticator_rows=deleted_rows,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| authenticators = Authenticator.objects.filter(user=target_user) | |
| if not authenticators.exists(): | |
| return Response( | |
| {"error": "userHasNoMFAEnabled"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| deleted_count, _ = authenticators.delete() | |
| logger.warning( | |
| authenticators = Authenticator.objects.filter(user=target_user) | |
| deleted_rows, _ = authenticators.delete() | |
| if deleted_rows == 0: | |
| return Response( | |
| {"error": "userHasNoMFAEnabled"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| logger.warning( | |
| "Admin reset MFA for another user", | |
| admin_id=str(request.user.id), | |
| admin_email=request.user.email, | |
| target_user_id=str(target_user.id), | |
| target_user_email=target_user.email, | |
| deleted_authenticator_rows=deleted_rows, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/iam/views.py` around lines 492 - 500, Replace the two-step
exists-then-delete with a single delete-then-check: call
Authenticator.objects.filter(user=target_user).delete() first (assign to
deleted_count, _), then if deleted_count == 0 return the same Response({"error":
"userHasNoMFAEnabled"}, status=status.HTTP_400_BAD_REQUEST); otherwise continue
with the existing flow (e.g., logging via logger.warning and any subsequent
cleanup) so you avoid the race between exists() and delete().
| "builderQuestionTypeMultipleChoice": "Wielokrotny wybór", | ||
| "builderQuestionTypeDate": "Data", | ||
| "resetMFA": "Zresetuj MFA", | ||
| "resetMFA1": "Jeśli ten użytkownik utracił dostęp do drugiego składnika, możesz", |
Contributor
There was a problem hiding this comment.
Terminology: "składnik" should be "czynnik" for MFA context.
In lines 2368 and 2371, "drugiego składnika" (second component) is used, but the standard Polish IT terminology for multi-factor authentication is "drugi czynnik" (second factor). "Składnik" typically means component or ingredient, while "czynnik" means factor — the correct term in this security context.
Suggested correction:
- Line 2368:
"Jeśli ten użytkownik utracił dostęp do drugiego czynnika, możesz" - Line 2371:
"Użytkownik straci dostęp do drugiego czynnika, dopóki nie zarejestruje go ponownie."
Also applies to: 2371-2371
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/messages/pl.json` at line 2368, Replace the Polish term "składnika"
with the correct MFA term "czynnika" in the localization entries: update the
value for key "resetMFA1" from "Jeśli ten użytkownik utracił dostęp do drugiego
składnika, możesz" to "Jeśli ten użytkownik utracił dostęp do drugiego czynnika,
możesz" and likewise change the message at the nearby entry on line 2371 (the
string that currently says "Użytkownik straci dostęp do drugiego składnika...")
to use "czynnika" instead.
Comment on lines
+83
to
+85
| 3. On the confirmation page, type `yes` and submit. | ||
|
|
||
| <figure><img src="../.gitbook/assets/mfa-reset-confirmation.png" alt=""><figcaption><p>The confirmation page requires typing <code>yes</code> before the reset can be submitted.</p></figcaption></figure> |
Contributor
There was a problem hiding this comment.
Use locale-agnostic wording for confirmation input
The docs currently require typing literal yes, but the UI validates against the localized translation of “yes”. This instruction will be wrong outside English locales. Recommend changing to “type the localized word for ‘yes’ shown on screen”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@product-docs/configuration/mfa.md` around lines 83 - 85, Replace the
hard-coded instruction "type `yes`" in the confirmation step of the MFA reset
docs (the line containing: "On the confirmation page, type `yes` and submit."
and its accompanying caption/figure) with a locale-agnostic instruction such as
"type the localized word for 'yes' shown on screen" so the guidance matches the
UI validation across locales; update both the inline step text and the figure
caption to refer to the localized confirmation word.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Enable admin to reset the MFA of their users without having to open a ticket
Closes #
Test plan
Checklist
!set if this is a breaking change)mainBackend — if you touched
backend/ruff formatrun, no new linter errorsmakemigrationsand committed (or none needed)Frontend — if you touched
frontend/pnpm run lintandpnpm run formatrunfrontend/messages/en.json(keep keys, preserve{tokens})Other — libraries, CI, infra
tools/scriptTests & documentation — definition of done
poetry run pytest, frontendpnpm run test/./tests/e2e-tests.sh(if relevant)product-docs/updated, new pages listed inSUMMARY.md, vocabulary terms added (if relevant)Summary by CodeRabbit
Release Notes
New Features
Documentation