Repository navigation
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
|
||
| /** Continue by confirming a recovery code after verification. Act on it with [EmbeddedAuthClient.confirmRecoveryCode]. */ | ||
| @ConsistentCopyVisibility | ||
| public data class ConfirmRecoveryCode internal constructor( |
There was a problem hiding this comment.
Since this is a data class, toString() prints the new recovery code in plain text. InsufficientAuthorization is a data class too, so something as simple as Log.d(TAG, "${e.error}") or a crash-reporter breadcrumb would leak it. Can we override toString() to mask newCode, and add a quick test for it?
| NextAction.ChallengePush(index, name) | ||
| } | ||
| EmbeddedCapability.VERIFY_OOB -> { | ||
| val pollInMs = (this[POLL_IN_MS_KEY] as? Number)?.toInt() |
There was a problem hiding this comment.
Two things here. Every other required field goes through dropped(), but this one returns null without logging. More importantly, if poll_in_ms is ever missing on a pending response, VerifyOob vanishes from the menu, and the app treats a still-pending push as terminal and gives up. How about falling back to a default interval (say 5s) instead of dropping it?
There was a problem hiding this comment.
poll_in_ms is a required property and server is bound to send it. Added logging for the same similar to other actions
| * On rejection or expiry the request completes with a terminal [EmbeddedAuthError] such as | ||
| * [EmbeddedAuthError.AuthorizationRejected] or [EmbeddedAuthError.ChallengeExpired]. | ||
| */ | ||
| public fun verifyOob(): Request<Credentials, EmbeddedAuthException> { |
There was a problem hiding this comment.
With polling there's now a realistic race. If the user taps "start over" or switches factor while a verifyOob() is still in flight, the late authorization_pending response goes through updateSessionFromFailure and writes the old auth_session over the new flow's session. That function doesn't check which flow a response belongs to. Could we track that and ignore stale responses? If that's too much for now, at least note in this KDoc that the app must cancel its polling before calling anything else.
There was a problem hiding this comment.
Agree this is a valid scenario but there is not much we can do on the SDK side for the same. The client needs to handle this . I have updated the doc to highlight the same
| val index = (this[INDEX_KEY] as? Number)?.toInt() ?: return dropped(raw, INDEX_KEY) | ||
| val identifier = this[IDENTIFIER_KEY] as? String ?: return dropped(raw, IDENTIFIER_KEY) | ||
| @Suppress("UNCHECKED_CAST") | ||
| val rawDeliveryMethods = this[DELIVERY_METHODS_KEY] as? List<String> |
There was a problem hiding this comment.
PhoneDeliveryMethod.fromValue() already exists and ignores case, so this hand-written when duplicates it (and is case-sensitive). Also, as? List is an unchecked cast, so a non-string value in the list would throw a ClassCastException while parsing the error. Something like (this[DELIVERY_METHODS_KEY] as? List<*>)?.filterIsInstance()?.mapNotNull(PhoneDeliveryMethod::fromValue) handles both.
| @@ -141,41 +154,6 @@ when (e.error) { | |||
| } | |||
There was a problem hiding this comment.
On the terminal-errors when just above: the new errors (AuthorizationRejected, NoEligibleFactors, ConsentRequired) aren't listed, so they fall into the "Unknown" else. NoEligibleFactors especially deserves its own line, since the app needs to send the user off to enroll a factor.
| val embedded = EmbeddedAuthClient(Auth0.getInstance("YOUR_CLIENT_ID", "YOUR_DOMAIN")) | ||
|
|
||
| // Next poll delay from a pending-push continuation, or null when the push is terminal. | ||
| // The server always includes poll_in_ms on a pending VerifyOob. |
There was a problem hiding this comment.
This assumes the server always sends poll_in_ms, but the SDK doesn't guarantee that today (see my comment on the mapper). Can we update this once the fallback is settled?
There was a problem hiding this comment.
Server will always send a poll_in_ms
| * [EmbeddedAuthError.InsufficientAuthorization.Reason.AUTHORIZATION_PENDING] or | ||
| * [EmbeddedAuthError.InsufficientAuthorization.Reason.SLOW_DOWN], read the updated | ||
| * [NextAction.VerifyOob.pollInMs] from [EmbeddedAuthError.InsufficientAuthorization.nextActions], | ||
| * wait that interval (back off on SLOW_DOWN), and call this method again. |
There was a problem hiding this comment.
The KDoc says to back off on SLOW_DOWN, but the push example just waits whatever pollInMs the server returns. Which one is it? Should the app add its own back-off, or is the server's pollInMs already the slowed-down value?
There was a problem hiding this comment.
Servers pollInMs is already the slowed down value. App shouldn't add anything on its own
| // which is correct for phone as a first factor; as an MFA step-up, pass the | ||
| // NextAction.ChallengePhone.index the server offered. Continues with VerifyOtp(channel = SMS). | ||
| try { | ||
| embedded.challengePhone(deliveryMethod = PhoneDeliveryMethod.TEXT).await() |
There was a problem hiding this comment.
The comment above says to pass the server's index for MFA, but the code hard-codes the default index and TEXT. Can we show reading ChallengePhone.index and deliveryMethods? Otherwise people will copy this and challenge the wrong authenticator, or ask for voice when the server didn't offer it.
| } | ||
| }); | ||
|
|
||
| embedded.verifyOtp("123456", OtpType.OOB) |
There was a problem hiding this comment.
The Java sample jumps from identify straight to verifyOtp and skips challengePhone. Since it's the new method here, it'd be good to show its Java usage too.
| mockAPI.willReturnInsufficientAuthorization() | ||
|
|
||
| try { | ||
| client.challengePhone(index = 0, deliveryMethod = PhoneDeliveryMethod.VOICE).execute() |
There was a problem hiding this comment.
index = 0 is also the default, so this would still pass if index were ignored. Could you use a non-zero index here, and add a quick check that the default delivery method goes out as "text"?
| } | ||
|
|
||
| @Test | ||
| public fun `verifyOob should exchange the code and return credentials on 200`() { |
There was a problem hiding this comment.
ould we also check that the session is cleared after a successful exchange, i.e. the next call fails with NoActiveSession?
| } | ||
|
|
||
| @Test | ||
| public fun `confirmRecoveryCode should exchange the code and return credentials on 200`() { |
There was a problem hiding this comment.
Same as on the verifyOob test: could we check that the session is cleared after the exchange succeeds?
| * which the SDK surfaces as an [EmbeddedAuthException]. A plain success here is unexpected. | ||
| */ | ||
| private fun runStep(step: suspend () -> Unit) { | ||
| pollJob?.cancel() |
There was a problem hiding this comment.
pollJob?.cancel() doesn't stop an in-flight poll. await() wraps a blocking execute(), so a late response still overwrites the new step's session. That's the race the KDoc warns about. Can the sample wait for the in-flight poll before starting the next step?
There was a problem hiding this comment.
It's a sample app change.Not a blocker
| // 3. Send the code using the index and a delivery method the server offered. Pick VOICE for a | ||
| // voice call; the first offered method is used here. Continues with VerifyOtp. | ||
| try { | ||
| val method = challenge.deliveryMethods.firstOrNull() ?: PhoneDeliveryMethod.TEXT |
There was a problem hiding this comment.
This won't compile because challenge is never defined. Step 2 reads the ChallengePhone but drops it. Also, there's a stray space in challenge.index ,
| val credentials = client.verifyOtp( | ||
| code = "123456", | ||
| type = OtpType.OOB // OOB for emailed codes; TOTP for authenticator-app codes | ||
| type = OtpType.OOB // OOB for email and SMS/voice codes; TOTP for authenticator-app codes |
There was a problem hiding this comment.
The when (error.reason) below (around line 126) has no else, so with the new AUTHORIZATION_PENDING and SLOW_DOWN it won't compile. Can you add those cases or an else -> null?
Changes
This PR adds MFA flow support to the embedded
/e/authorizeflow, building on the email-OTP first-factor work.Supported factors
New public types
NextActionsealed interface —IdentifyPhone,ChallengePhone,ChallengePush,VerifyOob,VerifyRecoveryCode,ConfirmRecoveryCode— surfaced viaInsufficientAuthorization.nextActions(a menu of what the server will accept next)EmbeddedAuthError— added the MFA continuation/terminal cases andInsufficientAuthorization.Reason(AUTHORIZATION_PENDING,SLOW_DOWN) so callers can tell why an input was rejectedIdentifierType(PHONE),PhoneDeliveryMethod(TEXT,VOICE).Push note: the SDK never polls.
verifyOob()performs exactly one poll per call and the app owns the loop — a pending push comes back asInsufficientAuthorizationcarrying a freshVerifyOob(pollInMs)delay to wait before the next call.Testing
Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.
This change adds unit 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