Skip to content

Fix users.listMfaFactors() recoveryCode mismatch (#10927) #10951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
Adityakk9031 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix users.listMfaFactors() recoveryCode mismatch (#10927) #10951

Adityakk9031 wants to merge 1 commit into from

Conversation

@Adityakk9031
Copy link

@Adityakk9031 Adityakk9031 commented Dec 13, 2025

Summary
Fixes an inconsistency where GET /v1/users/:userId/mfa/factors returned recoveryCode: false even when recovery codes existed for the user. This makes the users endpoint match the behavior of account.listMfaFactors().

Root cause
The Users controller did not check the user document for the mfaRecoveryCodes attribute. The Account controller already checks that attribute and sets recoveryCode: true when recovery codes exist; the Users controller simply omitted that check and therefore always returned recoveryCode: false.

What this change does

Adds the same recovery-code presence check used by the Account handler to app/controllers/api/users.php so Type::RECOVERY_CODE is set correctly.

Ensures GET /v1/users/:userId/mfa/factors returns recoveryCode: true when recovery codes exist for the user, matching GET /v1/account/mfa/factors.

Files changed

app/controllers/api/users.php — include mfaRecoveryCodes check and set Type::RECOVERY_CODE in the factors response.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 13, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The changes address an inconsistency between two MFA factor listing endpoints. The users.listMfaFactors() endpoint now reads the mfaRecoveryCodes field from user documents and derives a recovery code enabled flag, then includes Type::RECOVERY_CODE in the MFA factors response. Similar logic is applied in the createUser flow to ensure recovery code state is properly prepared when users are created. This aligns the behavior of users.listMfaFactors() with account.listMfaFactors() for recovery code visibility.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Primary concern: Verify that the recovery code detection logic (reading mfaRecoveryCodes and deriving recoveryCodeEnabled flag) is correctly implemented in both the createUser flow and the listMfaFactors endpoint
  • Secondary concern: Ensure the Type::RECOVERY_CODE factor is properly constructed and formatted consistently with existing factor types (TOTP, EMAIL, PHONE)
  • Edge cases to validate: Handling when mfaRecoveryCodes is empty, null, or missing; ensure the recovery code factor state matches between both code paths

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: fixing the recoveryCode mismatch in the users.listMfaFactors() endpoint, directly addressing the linked issue #10927.
Description check ✅ Passed The description clearly explains the inconsistency, root cause, and the fix applied. It directly relates to the changeset and provides sufficient context about the problem and solution.
Linked Issues check ✅ Passed The PR fully addresses the requirement from issue #10927 by adding the mfaRecoveryCodes check to the Users controller to ensure recoveryCode: true is returned when recovery codes exist, matching account.listMfaFactors() behavior.
Out of Scope Changes check ✅ Passed All changes are focused on fixing the specific recoveryCode mismatch in app/controllers/api/users.php. No out-of-scope modifications are present; changes align with the stated objective in issue #10927.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 35fe622 and 7c3728c.

📒 Files selected for processing (1)
  • app/controllers/api/users.php (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/controllers/api/users.php (2)
src/Appwrite/Extend/Exception.php (1)
  • Exception (7-464)
src/Appwrite/Utopia/Response.php (1)
  • dynamic (700-733)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: scan
🔇 Additional comments (1)
app/controllers/api/users.php (1)

1855-1876: Recovery code factor logic looks correct; please verify parity with account.listMfaFactors() and simplify the check.

This fixes the bug as described by deriving Type::RECOVERY_CODE from mfaRecoveryCodes. I’d just align the boolean expression with the existing recovery-codes endpoints and reduce surface for subtle type differences (and verify it matches the Account controller’s exact semantics, especially if MFA is disabled but codes exist).

-    $mfaRecoveryCodes = $user->getAttribute('mfaRecoveryCodes', []);
-    $recoveryCodeEnabled = \is_array($mfaRecoveryCodes) && \count($mfaRecoveryCodes) > 0;
+    $mfaRecoveryCodes = $user->getAttribute('mfaRecoveryCodes', []);
+    $recoveryCodeEnabled = \is_array($mfaRecoveryCodes) && !empty($mfaRecoveryCodes);

Verification idea (no code changes): compare the Account controller’s factors-building logic and add/adjust a test to assert both endpoints return identical factor maps for the same user state.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Dec 13, 2025

Security Scan Results for PR

Docker Image Scan Results

Package Version Vulnerability Severity
binutils 2.44-r2 CVE-2025-5244 HIGH
binutils 2.44-r2 CVE-2025-5245 HIGH
imagemagick 7.1.2.0-r0 CVE-2025-62171 HIGH
imagemagick-c++ 7.1.2.0-r0 CVE-2025-62171 HIGH
imagemagick-dev 7.1.2.0-r0 CVE-2025-62171 HIGH
imagemagick-heic 7.1.2.0-r0 CVE-2025-62171 HIGH
imagemagick-jpeg 7.1.2.0-r0 CVE-2025-62171 HIGH
imagemagick-jxl 7.1.2.0-r0 CVE-2025-62171 HIGH
imagemagick-libs 7.1.2.0-r0 CVE-2025-62171 HIGH
imagemagick-openexr 7.1.2.0-r0 CVE-2025-62171 HIGH
libecpg 17.6-r0 CVE-2025-12818 HIGH
libecpg-dev 17.6-r0 CVE-2025-12818 HIGH
libpng 1.6.47-r0 CVE-2025-64720 HIGH
libpng 1.6.47-r0 CVE-2025-65018 HIGH
libpng 1.6.47-r0 CVE-2025-66293 HIGH
libpng-dev 1.6.47-r0 CVE-2025-64720 HIGH
libpng-dev 1.6.47-r0 CVE-2025-65018 HIGH
libpng-dev 1.6.47-r0 CVE-2025-66293 HIGH
libpq 17.6-r0 CVE-2025-12818 HIGH
libpq-dev 17.6-r0 CVE-2025-12818 HIGH
libxml2 2.13.8-r0 CVE-2025-49794 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49796 CRITICAL
libxml2 2.13.8-r0 CVE-2025-49795 HIGH
libxml2 2.13.8-r0 CVE-2025-6021 HIGH
pcre2 10.43-r1 CVE-2025-58050 CRITICAL
postgresql17-dev 17.6-r0 CVE-2025-12818 HIGH
github.com/containerd/containerd/v2 v2.0.2 CVE-2024-25621 HIGH
golang.org/x/crypto v0.31.0 CVE-2025-22869 HIGH
golang.org/x/oauth2 v0.24.0 CVE-2025-22868 HIGH
stdlib 1.22.10 CVE-2025-47907 HIGH
stdlib 1.22.10 CVE-2025-58183 HIGH
stdlib 1.22.10 CVE-2025-58186 HIGH
stdlib 1.22.10 CVE-2025-58187 HIGH
stdlib 1.22.10 CVE-2025-61729 HIGH

Source Code Scan Results

🎉 No vulnerabilities found!

@Adityakk9031
Copy link
Author

Adityakk9031 commented Dec 13, 2025

Please review this once before closing, as assigning an issue often takes months based on my previous experience.

Aglowharmony
Aglowharmony approved these changes Dec 21, 2025
View reviewed changes
Copy link

@Aglowharmony Aglowharmony left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

works close this pr

@Adityakk9031
Copy link
Author

Adityakk9031 commented Dec 21, 2025

please merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@Aglowharmony Aglowharmony Aglowharmony approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

users.listMfaFactors() returns incorrect recoveryCode value (inconsistent with account.listMfaFactors())

2 participants

@Adityakk9031 @Aglowharmony