fix: MFAClient getAuthenticators filtering based on Authenticator.type field#998
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughMFA authenticator filtering now matches exact authenticator types, and TOTP enrollment handling now uses the updated associate response fields across the model, tests, and examples. ChangesMFA authenticator filtering and examples
TOTP enrollment fields and examples
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
auth0/src/main/java/com/auth0/android/authentication/mfa/MfaApiClient.kt (1)
305-319: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSame
"phone"vs"oob"concern applies to this doc block.This restates the exact-match filtering contract using
factorsAllowed = ["phone", "email", "otp"]as an example. Same concern as the public KDoc above — please confirm this matches the realAuthenticator.typevalues returned byGET /mfa/authenticators.🤖 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 `@auth0/src/main/java/com/auth0/android/authentication/mfa/MfaApiClient.kt` around lines 305 - 319, The KDoc for the JSON adapter in MfaApiClient still uses examples that may not match the real Authenticator.type values returned by GET /mfa/authenticators. Update the doc block to use only documented/verified factor-type examples and make the exact-match filtering contract consistent with the actual API response values; locate the affected text in the adapter documentation near the filtering description.
🤖 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 `@auth0/src/main/java/com/auth0/android/authentication/mfa/MfaApiClient.kt`:
- Around line 127-140: The KDoc example in MfaApiClient.getAuthenticators uses
outdated factor values that won’t match SMS/email authenticators. Update the
sample callback and the `@param` factorsAllowed examples to use "oob" instead of
"phone" and "email", keeping the exact-match filtering note aligned with
Authenticator.type.
---
Duplicate comments:
In `@auth0/src/main/java/com/auth0/android/authentication/mfa/MfaApiClient.kt`:
- Around line 305-319: The KDoc for the JSON adapter in MfaApiClient still uses
examples that may not match the real Authenticator.type values returned by GET
/mfa/authenticators. Update the doc block to use only documented/verified
factor-type examples and make the exact-match filtering contract consistent with
the actual API response values; locate the affected text in the adapter
documentation near the filtering description.
🪄 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 Plus
Run ID: 653affda-cf64-4839-b7ef-2ed4fdc1f553
📒 Files selected for processing (3)
EXAMPLES.mdauth0/src/main/java/com/auth0/android/authentication/mfa/MfaApiClient.ktauth0/src/test/java/com/auth0/android/authentication/MfaApiClientTest.kt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@auth0/src/main/java/com/auth0/android/result/EnrollmentChallenge.kt`:
- Around line 72-86: Keep TotpEnrollmentChallenge.id and authSession backward
compatible by avoiding a public API change from non-null to nullable in
EnrollmentChallenge.kt. Update the TotpEnrollmentChallenge definition so
existing callers can still rely on these properties as non-null, or otherwise
provide a compatibility-preserving alternative in the
TotpEnrollmentChallenge/EnrollmentChallenge hierarchy. If the nullability change
is unavoidable, document the breaking change clearly in release notes and API
docs.
🪄 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 Plus
Run ID: f260e646-fa53-4d86-94f6-b2dfcc6e5c87
📒 Files selected for processing (3)
EXAMPLES.mdauth0/src/main/java/com/auth0/android/result/EnrollmentChallenge.ktauth0/src/test/java/com/auth0/android/authentication/MfaApiClientTest.kt
| override val id: String? = null, | ||
| @SerializedName("auth_session") | ||
| override val authSession: String, | ||
| override val authSession: String? = null, |
There was a problem hiding this comment.
Document these nullable property change
Changes
This PR contains two related MFA fixes.
1.
getAuthenticators— filter onAuthenticator.type(exact match)Updates how
MfaApiClient.getAuthenticators(factorsAllowed)filters the authenticators returned byGET /mfa/authenticators.What changed
Authenticator.typevalue directly against thefactorsAllowedlist using exact (case-sensitive) equality. Previously the SDK derived an "effective type" by inspectingoob_channelandauthenticator_type(with aliasing, e.g.sms/phone,otp/totp, and treatingoobas a wildcard). That derivation and its helpers have been removed.matchesFactorType(...)andgetEffectiveType(...)inMfaApiClient.getAuthenticators(factorsAllowed: List<String>)keeps the same signature; only its internal filtering behavior changed.MfaApiClientKDoc andEXAMPLES.mdupdated so thefactorsAllowedexamples use the same factor values as thetypefield (e.g.phone,email,otp,recovery-code) instead of channel-level values likesms/oob.Why it matters
The
factorsAllowedvalues now map one-to-one to theAuthenticator.typefield, so filtering is predictable and matches the values a caller gets fromMfaRequirements(MfaFactor.type). This is a behavioral change: callers passing channel-level or wildcard values such assmsoroobwill no longer match — they should pass the resolvedtypevalue (e.g.phone). Confirmed against live FGS that a TOTP factor hastype: "totp"(authenticator_type: "otp"), so callers must passtotpto surface it.2.
TotpEnrollmentChallenge— correct field mapping for/mfa/associateTotpEnrollmentChallengeis shared by two endpoints with different response shapes, but was modeled only on the My Account/authentication-methodsshape. Used against the ROPG/mfa/associateendpoint (whichMfaApiClient.enroll(Otp)calls), this caused:idandauthSessiondeclared non-null but populated asnullby Gson (null-safety bypass / latent NPE hazard),manualInputCodealwaysnull(the endpoint returns the key assecret, notmanual_input_code),secretandrecovery_codessilently dropped.What changed
idandauthSessionnullable (manualInputCodewas already nullable) and addedauthenticatorType,secret,recoveryCodes, so a single class deserializes both endpoints correctly. This matches Auth0.swift'sOTPMFAEnrollmentChallenge(the only cross-SDK peer that implements/mfa/associate).id/authSession/manualInputCode/barcodeUri) still map identically; the change only loosens nullability and adds fields. No runtime break for existing users; recompiling My Account consumers may need a null check onid/authSession(the safe direction).EXAMPLES.mdMFAenroll(Otp)snippet corrected to readsecret(notmanualInputCode) and now showsrecoveryCodes.Usage
References
Testing
getAuthenticators unit tests in
MfaApiClientTestexercise exact-type matching: fixtures include a top-leveltypefield and assert only authenticators whosetypeis infactorsAllowedare returned.TOTP enroll test updated to use the real
/mfa/associateresponse shape, assertingsecret/recoveryCodes/authenticatorTypepopulate.MyAccountAPIClientTestre-run to confirm the shared-class change does not affect the My Account TOTP/push flows.Verified end-to-end on a physical device (Pixel 7a) against live FGS: login → enroll OTP → verify OTP → verify recovery code, all succeeded;
secret/recovery_codespopulate correctly.This change adds unit test coverage
This change adds integration test coverage
This change has been tested on the latest version of the platform/language or why not
Checklist
I have read the Auth0 general contribution guidelines
I have read the Auth0 Code of Conduct
All existing and new tests complete without errors
Summary by CodeRabbit