Repository navigation
feat : Add support for MFA in embedded authorize flow #1086
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.
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
Changes from all commits
d848c6d
247ffea
4af710b
02df1cc
52c0b7c
bcb9ece
96f0663
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -36,6 +36,48 @@ public sealed interface NextAction { | |
| override val capability: EmbeddedCapability = EmbeddedCapability.VERIFY_OTP | ||
| } | ||
|
|
||
| /** Continue by challenging a phone factor. Act on it with [EmbeddedAuthClient.challengePhone]. */ | ||
| @ConsistentCopyVisibility | ||
| public data class ChallengePhone internal constructor( | ||
| public val index: Int, | ||
| public val identifier: String, | ||
| public val deliveryMethods: List<PhoneDeliveryMethod>, | ||
| ) : NextAction { | ||
| override val capability: EmbeddedCapability = EmbeddedCapability.CHALLENGE_PHONE | ||
| } | ||
|
|
||
| /** Continue by challenging a push notification factor. Act on it with [EmbeddedAuthClient.challengePush]. */ | ||
| @ConsistentCopyVisibility | ||
| public data class ChallengePush internal constructor( | ||
| public val index: Int, | ||
| public val name: String, | ||
| ) : NextAction { | ||
| override val capability: EmbeddedCapability = EmbeddedCapability.CHALLENGE_PUSH | ||
| } | ||
|
|
||
| /** Continue by verifying an out-of-band (push) notification. Act on it with [EmbeddedAuthClient.verifyOob]. */ | ||
| @ConsistentCopyVisibility | ||
| public data class VerifyOob internal constructor( | ||
| public val pollInMs: Int, | ||
| ) : NextAction { | ||
| override val capability: EmbeddedCapability = EmbeddedCapability.VERIFY_OOB | ||
| } | ||
|
|
||
| /** Continue by verifying a recovery code. Act on it with [EmbeddedAuthClient.verifyRecoveryCode]. */ | ||
| public data object VerifyRecoveryCode : NextAction { | ||
| override val capability: EmbeddedCapability = EmbeddedCapability.VERIFY_RECOVERY_CODE | ||
| } | ||
|
|
||
| /** Continue by confirming a recovery code after verification. Act on it with [EmbeddedAuthClient.confirmRecoveryCode]. */ | ||
| @ConsistentCopyVisibility | ||
| public data class ConfirmRecoveryCode internal constructor( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Valid point. Done |
||
| public val newCode: String, | ||
|
pmathew92 marked this conversation as resolved.
|
||
| ) : NextAction { | ||
| override val capability: EmbeddedCapability = EmbeddedCapability.CONFIRM_RECOVERY_CODE | ||
|
|
||
| override fun toString(): String = "ConfirmRecoveryCode(newCode='xxxxx')" | ||
| } | ||
|
|
||
| /** An action the server offered that this version of the SDK does not model. */ | ||
| @ConsistentCopyVisibility | ||
| public data class Unknown internal constructor( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,10 @@ private const val ACTION_KEY = "action" | |
| private const val CHANNEL_KEY = "channel" | ||
| private const val IDENTIFIER_KEY = "identifier" | ||
| private const val INDEX_KEY = "index" | ||
| private const val DELIVERY_METHODS_KEY = "delivery_methods" | ||
| private const val POLL_IN_MS_KEY = "poll_in_ms" | ||
| private const val NAME_KEY = "name" | ||
| private const val NEW_CODE_KEY = "new_code" | ||
|
|
||
| internal fun List<Map<String, Any>>.toNextActions(): List<NextAction> = mapNotNull { it.toNextAction() } | ||
|
|
||
|
|
@@ -29,12 +33,36 @@ private fun Map<String, Any>.toNextAction(): NextAction? { | |
| identifier = this[IDENTIFIER_KEY] as? String | ||
| ) | ||
| } | ||
| EmbeddedCapability.CHALLENGE_PHONE -> { | ||
| 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) | ||
| val deliveryMethods = (this[DELIVERY_METHODS_KEY] as? List<*>) | ||
| ?.filterIsInstance<String>() | ||
| ?.mapNotNull(PhoneDeliveryMethod::fromValue) | ||
| ?: return dropped(raw, DELIVERY_METHODS_KEY) | ||
| NextAction.ChallengePhone(index, identifier, deliveryMethods) | ||
| } | ||
| EmbeddedCapability.CHALLENGE_PUSH -> { | ||
| val index = (this[INDEX_KEY] as? Number)?.toInt() ?: return dropped(raw, INDEX_KEY) | ||
| val name = this[NAME_KEY] as? String ?: return dropped(raw, NAME_KEY) | ||
| NextAction.ChallengePush(index, name) | ||
| } | ||
| EmbeddedCapability.VERIFY_OOB -> { | ||
| val pollInMs = (this[POLL_IN_MS_KEY] as? Number)?.toInt() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. poll_in_ms is a required property and server is bound to send it. Added logging for the same similar to other actions |
||
| ?: return dropped(raw, POLL_IN_MS_KEY) | ||
| NextAction.VerifyOob(pollInMs) | ||
| } | ||
| EmbeddedCapability.VERIFY_RECOVERY_CODE -> NextAction.VerifyRecoveryCode | ||
| EmbeddedCapability.CONFIRM_RECOVERY_CODE -> { | ||
| val newCode = this[NEW_CODE_KEY] as? String ?: return dropped(raw, NEW_CODE_KEY) | ||
| NextAction.ConfirmRecoveryCode(newCode) | ||
| } | ||
| EmbeddedCapability.UNKNOWN -> NextAction.Unknown(raw) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Logs and drops a recognised next action whose required [field] the server omitted or sent | ||
| * Logs and drops a recognized next action whose required [field] the server omitted or sent | ||
| * malformed. Returns `null` so the entry is filtered out by [toNextActions]. | ||
| */ | ||
| private fun dropped(action: String, field: String): NextAction? { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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