-
Notifications
You must be signed in to change notification settings - Fork 166
Expanding Flexible Factors Grant Android Support #896
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
base: main
Are you sure you want to change the base?
Expanding Flexible Factors Grant Android Support #896
Conversation
| factorsAllowed: List<String>? = null | ||
| ): Request<List<Authenticator>, MfaListAuthenticatorsException> { | ||
| // SDK validation: factorsAllowed cannot be empty | ||
| if (factorsAllowed != null && factorsAllowed.isEmpty()) { |
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.
This is not the correct way. Use the addValidator method of the Request class to handle the validation logic
| ) : MfaException("MFA authenticator listing failed: $code") { | ||
|
|
||
| internal constructor(values: Map<String, Any>, statusCode: Int) : this( | ||
| code = (values["error"] as? String) ?: FALLBACK_ERROR_CODE, |
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.
default error code needs to be removed as discussed
| * @param mfaToken The token received in the 'mfa_required' error from a login attempt. | ||
| * @return A new [MfaApiClient] instance configured for the transaction. | ||
| */ | ||
| public fun mfa(mfaToken: String): MfaApiClient { |
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.
Rename it to mfaClient
| * The MFA token returned when multi-factor authentication is required. | ||
| * This token should be used to create an [MfaApiClient] to continue the MFA flow. | ||
| */ | ||
| public val mfaToken: String? |
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.
Keep this as part of the MfaRequirements error body and not separate.
| * The MFA requirements returned when multi-factor authentication is required. | ||
| * Contains information about the required challenge types. | ||
| */ | ||
| public val mfaRequirements: MfaRequirements? |
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.
Hope the name is consistent with Auth0.Swift implementation
| private const val JWKS_FILE_PATH = "jwks.json" | ||
| private const val TAG = "AuthenticationAPIClient" | ||
| private fun createErrorAdapter(): ErrorAdapter<AuthenticationException> { | ||
| internal fun createErrorAdapter(): ErrorAdapter<AuthenticationException> { |
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.
Why make them internal ?
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.
Pull request overview
This PR adds comprehensive Multi-Factor Authentication (MFA) support to the Android SDK, enabling developers to implement TOTP, SMS, Email, and recovery code authentication flows.
Changes:
- Introduced new MfaApiClient with methods for listing authenticators, enrollment, challenge, and verification operations
- Added MfaException sealed class hierarchy for type-safe MFA error handling with specific exceptions for different operations
- Extended AuthenticationException and CredentialsManagerException to expose MFA tokens and requirements for continuing MFA flows
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| MfaRequirements.kt | New data model representing MFA requirements returned by Auth0 (challenge/enroll types) |
| Authenticator.kt | New data model representing enrolled MFA authenticator details |
| MfaException.kt | New sealed class hierarchy for MFA-specific exceptions with fallback error codes |
| MfaApiClient.kt | New API client providing methods for MFA operations (list, enroll, challenge, verify) |
| AuthenticationAPIClient.kt | Added factory method mfa() to create MfaApiClient instances; exposed internal error adapter |
| AuthenticationException.kt | Added mfaToken and mfaRequirements properties to support MFA flow continuation |
| CredentialsManagerException.kt | Added MFA_REQUIRED error code and properties for MFA token/requirements |
| CredentialsManager.kt | Added MFA error detection and exception wrapping in credential renewal flows |
| SecureCredentialsManager.kt | Added MFA error detection and exception wrapping in credential renewal flows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * The MFA token required to continue the multi-factor authentication flow. | ||
| * This is only available when the error code is [Code.MFA_REQUIRED]. | ||
| */ | ||
| public val mfaToken: String? | ||
| get() = mfaTokenValue | ||
|
|
||
| /** | ||
| * The MFA requirements when multi-factor authentication is required. | ||
| * This is only available when the error code is [Code.MFA_REQUIRED]. | ||
| */ | ||
| public val mfaRequirements: MfaRequirements? | ||
| get() = mfaRequirementsValue |
Copilot
AI
Jan 20, 2026
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.
The new mfaToken and mfaRequirements properties in CredentialsManagerException are missing the @JvmName annotation. If Java callers want to access these properties, they would need to use getMfaToken() and getMfaRequirements(), but since these are custom getters, the Java method names might not be ideal. Consider adding @JvmName annotations for consistency with Java interoperability.
| public val mfaRequirements: Map<String, Any>? | ||
| get() = getValue("mfa_requirements") as? Map<String, Any> |
Copilot
AI
Jan 20, 2026
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.
The MfaRequiredException.mfaRequirements property returns Map<String, Any>? instead of the more type-safe MfaRequirements class that exists in the codebase. This forces consumers to work with untyped maps when a proper data class is available, reducing type safety and developer experience.
| public val mfaRequirements: Map<String, Any>? | |
| get() = getValue("mfa_requirements") as? Map<String, Any> | |
| public data class MfaRequirements( | |
| val enroll: List<String>?, | |
| val challenge: List<String>? | |
| ) | |
| public val mfaRequirements: MfaRequirements? | |
| get() { | |
| val raw = getValue("mfa_requirements") as? Map<*, *> ?: return null | |
| val enroll = (raw["enroll"] as? List<*>)?.filterIsInstance<String>() | |
| val challenge = (raw["challenge"] as? List<*>)?.filterIsInstance<String>() | |
| return MfaRequirements(enroll = enroll, challenge = challenge) | |
| } |
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. Here instead of the Map we can return the defined MfaRequirements instance
| public fun enroll( | ||
| factorType: String, | ||
| phoneNumber: String? = null, | ||
| email: String? = null, | ||
| authenticatorType: String? = null | ||
| ): Request<EnrollmentChallenge, MfaEnrollmentException> { | ||
| // Auth0 API expects authenticator_types as an array and oob_channels for OOB types | ||
| // Map the factorType to the correct Auth0 API format | ||
| val authenticatorTypesArray: List<String> | ||
| val oobChannelsArray: List<String>? | ||
|
|
||
| when (factorType.lowercase()) { | ||
| "phone" -> { | ||
| // SMS enrollment: authenticator_types=["oob"], oob_channels=["sms"] | ||
| authenticatorTypesArray = listOf("oob") | ||
| oobChannelsArray = listOf("sms") | ||
| } | ||
| "email" -> { | ||
| // Email enrollment: authenticator_types=["oob"], oob_channels=["email"] | ||
| authenticatorTypesArray = listOf("oob") | ||
| oobChannelsArray = listOf("email") | ||
| } | ||
| "totp" -> { | ||
| // TOTP enrollment: authenticator_types=["otp"] | ||
| authenticatorTypesArray = listOf("otp") | ||
| oobChannelsArray = null | ||
| } | ||
| "push" -> { | ||
| // Push enrollment: authenticator_types=["push-notification"] | ||
| authenticatorTypesArray = listOf("push-notification") | ||
| oobChannelsArray = null | ||
| } | ||
| else -> { | ||
| // Use authenticatorType if provided, otherwise use factorType as-is | ||
| authenticatorTypesArray = if (authenticatorType != null) { | ||
| listOf(authenticatorType) | ||
| } else { | ||
| listOf(factorType) | ||
| } | ||
| oobChannelsArray = null | ||
| } | ||
| } | ||
|
|
||
| val parameters = ParameterBuilder.newBuilder() | ||
| .setClientId(clientId) | ||
| .set(MFA_TOKEN_KEY, mfaToken) | ||
| .set(PHONE_NUMBER_KEY, phoneNumber) | ||
| .set(EMAIL_KEY, email) | ||
| .asDictionary() | ||
|
|
||
| val url = baseURL.toHttpUrl().newBuilder() | ||
| .addPathSegment(MFA_PATH) | ||
| .addPathSegment(ASSOCIATE_PATH) | ||
| .build() | ||
|
|
||
| val enrollmentAdapter: JsonAdapter<EnrollmentChallenge> = GsonAdapter( | ||
| EnrollmentChallenge::class.java, gson | ||
| ) | ||
|
|
||
| val request = enrollmentFactory.post(url.toString(), enrollmentAdapter) | ||
| .addParameters(parameters) | ||
|
|
||
| // Add array parameters using addParameter(name, Any) which handles serialization | ||
| request.addParameter(AUTHENTICATOR_TYPES_KEY, authenticatorTypesArray) | ||
|
|
||
| if (oobChannelsArray != null) { | ||
| request.addParameter(OOB_CHANNELS_KEY, oobChannelsArray) | ||
| } | ||
|
|
||
| return request | ||
| } |
Copilot
AI
Jan 20, 2026
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.
The enroll method has complex parameter validation logic based on factorType, but there's no input validation for required parameters. For example, when factorType is "phone", phoneNumber should be required but there's no check to ensure it's provided. This could lead to API errors that would be better caught earlier with proper validation.
| val authenticatorTypesArray: List<String> | ||
| val oobChannelsArray: List<String>? | ||
|
|
||
| when (factorType.lowercase()) { |
Copilot
AI
Jan 20, 2026
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.
The enroll method uses locale-sensitive lowercase() for factorType comparison. This could lead to unexpected behavior with Turkish or other locales where lowercase transformations differ. Use factorType.lowercase(Locale.ROOT) or factorType.lowercase(Locale.ENGLISH) for consistent behavior across all locales.
| * mfaClient.challenge("oob", "{authenticator_id}") | ||
| * .start(object : Callback<Challenge, MfaChallengeException> { | ||
| * override fun onSuccess(result: Challenge) { | ||
| * // Code sent, now prompt user for the OTP they received |
Copilot
AI
Jan 20, 2026
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.
The documentation example has extra whitespace before the comment text on line 160. This formatting inconsistency should be corrected to maintain code quality standards.
| * // Code sent, now prompt user for the OTP they received | |
| * // Code sent, now prompt user for the OTP they received |
| public fun getAvailableAuthenticators( | ||
| factorsAllowed: List<String>? = null | ||
| ): Request<List<Authenticator>, MfaListAuthenticatorsException> { | ||
| // SDK validation: factorsAllowed cannot be empty | ||
| if (factorsAllowed != null && factorsAllowed.isEmpty()) { | ||
| throw MfaListAuthenticatorsException.invalidRequest( | ||
| "challengeType is required and must contain at least one challenge type. " + | ||
| "Pass null to retrieve all authenticators, or provide at least one factor type (e.g., \"otp\", \"oob\", \"recovery-code\")." | ||
| ) | ||
| } |
Copilot
AI
Jan 20, 2026
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.
The getAvailableAuthenticators method throws MfaListAuthenticatorsException for validation errors, which is inconsistent with standard Kotlin/Java practices. SDK validation errors should typically throw IllegalArgumentException or similar, while MfaListAuthenticatorsException should be reserved for API/network errors that occur during request execution. This creates confusion as the exception can be thrown synchronously (validation) or asynchronously (API error).
| if (error.isMultifactorRequired) { | ||
| callback.onFailure( | ||
| CredentialsManagerException( | ||
| CredentialsManagerException.Code.MFA_REQUIRED, | ||
| error.message ?: "Multi-factor authentication is required to complete the credential renewal.", | ||
| error, | ||
| error.mfaToken, | ||
| error.mfaRequirements | ||
| ) | ||
| ) | ||
| return@execute | ||
| } |
Copilot
AI
Jan 20, 2026
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.
There is code duplication in the MFA error handling across CredentialsManager and SecureCredentialsManager. The same MFA detection and exception creation logic appears in multiple places (lines 547-558, 674-685, 915-926, 1074-1085). Consider extracting this into a shared helper method to reduce duplication and improve maintainability.
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.
The duplication is intentional for clarity at each call site. The duplicated block is only ~10 lines and each location has different surrounding error handling. We can consider extracting a helper in a future refactoring PR if needed.
| private const val JWKS_FILE_PATH = "jwks.json" | ||
| private const val TAG = "AuthenticationAPIClient" | ||
| private fun createErrorAdapter(): ErrorAdapter<AuthenticationException> { | ||
| internal fun createErrorAdapter(): ErrorAdapter<AuthenticationException> { |
Copilot
AI
Jan 20, 2026
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.
The companion object visibility is changed from private to internal, which expands the API surface. Unless there's a specific requirement for internal access to createErrorAdapter, this change increases coupling between internal components and may make future refactoring more difficult.
| internal fun createErrorAdapter(): ErrorAdapter<AuthenticationException> { | |
| private fun createErrorAdapter(): ErrorAdapter<AuthenticationException> { |
| * Example usage: | ||
| * ``` | ||
| * try { | ||
| * val credentials = mfaClient.verifyOtp("123456").await() |
Copilot
AI
Jan 20, 2026
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.
The documentation example at line 176 shows calling "verifyOtp" but the actual method name is "verifyWithOtp". This inconsistency in the documentation will confuse developers trying to use the API.
| * val credentials = mfaClient.verifyOtp("123456").await() | |
| * val credentials = mfaClient.verifyWithOtp("123456").await() |
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.
The method is correctly named verifyOtp() in the implementation. The Copilot suggestion is outdated - it was referencing an old name verifyWithOtp that was renamed per another review comment. The documentation matches the actual method name.
| public val mfaRequirements: MfaRequirements? | ||
| get() = (getValue("mfa_requirements") as? Map<*, *>)?.let { | ||
| @Suppress("UNCHECKED_CAST") | ||
| GsonProvider.gson.fromJson( | ||
| GsonProvider.gson.toJson(it), | ||
| MfaRequirements::class.java | ||
| ) | ||
| } |
Copilot
AI
Jan 20, 2026
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.
The AuthenticationException.mfaRequirements property performs JSON serialization/deserialization on every access, which is inefficient if the property is accessed multiple times. Consider caching the deserialized MfaRequirements object in a lazy-initialized backing field to avoid repeated conversions.
| mfaToken, | ||
| RequestFactory<AuthenticationException>( | ||
| auth0.networkingClient, | ||
| AuthenticationAPIClient.createErrorAdapter() |
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.
Create a new Adapter for this class instead of reusing the AuthenticationException one. All errors from this set of APIs should return the new error type
| * @throws MfaListAuthenticatorsException if factorsAllowed is an empty list (SDK validation error) | ||
| */ | ||
| public fun getAvailableAuthenticators( | ||
| factorsAllowed: List<String>? = null |
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.
factorsAllowed is not an optional parameter.
| // Apply filtering if factorsAllowed is provided and not empty | ||
| if (factorsAllowed != null) { | ||
| urlBuilder.addQueryParameter("factorsAllowed", factorsAllowed.joinToString(",")) | ||
| } |
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.
Filtering should be done by the SDK
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.
Done. Removed the query parameter approach. Filtering is now done by the SDK using createFilteringAuthenticatorsAdapter() which filters the API response based on factorsAllowed before returning to the caller.
| phoneNumber: String? = null, | ||
| email: String? = null, | ||
| authenticatorType: String? = null | ||
| ): Request<EnrollmentChallenge, MfaEnrollmentException> { |
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.
Would suggest your make this a private method and expose public methods for individual factors like what you have written for enrollTotp. This method doesn't look DX friendly. I think web and Swift is also exposing individual public APIs
| * @param otp the one-time password provided by the user | ||
| * @return an authentication request to configure and start that will yield [Credentials] | ||
| */ | ||
| public fun verifyWithOtp(otp: String): AuthenticationRequest { |
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.
AuthenticationRequest is bound to an AuthenticationException . Since we will have a completely new error class here , I would suggest we use the Request interface. This will also be consistent with other methods in this class
| /** | ||
| * Creates error adapter for getAuthenticators() operations. | ||
| * Returns MfaListAuthenticatorsException with fallback error code if API doesn't provide one. | ||
| */ |
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.
I don't think we would need separate error classes for different APIs. A single MfaException class should suffice IMO. Check with @NandanPrabhu and decide on a single class
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.
Swift SDK - also uses separate error classes (MfaListAuthenticatorsError, MfaEnrollmentError, MfaChallengeError, MFAVerifyError). Our Android implementation follows the same pattern for consistency across SDKs.
| * Base class for MFA-related exceptions. | ||
| * All MFA-specific errors inherit from this class for easier error handling. | ||
| */ | ||
| public sealed class MfaException( |
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.
Lets create a single Exception class
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.
The Swift SDK uses separate error classes (MfaListAuthenticatorsError, MfaEnrollmentError, MfaChallengeError, MFAVerifyError). Our Android implementation uses a sealed class hierarchy for consistency with Swift and to provide type-safe error handling. Users can still catch MfaException as a single base class if they prefer unified handling.
| * } | ||
| * ``` | ||
| */ | ||
| public class MfaRequiredException internal constructor( |
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.
where is this MfaRequiredException being used . Doesn't the existing login APIs return AuthenticationException. So users would catch it and check for MfaRequired error and proceed with Mfa flow
| CredentialsManagerException.Code.MFA_REQUIRED, | ||
| error.message ?: "Multi-factor authentication is required to complete the credential renewal.", | ||
| error, | ||
| error.mfaToken, |
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.
lets return token as part of the MfaRequirements
| } | ||
|
|
||
| private var code: Code? | ||
| private var mfaTokenValue: String? = null |
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.
Keep mfaToken as part of MfaRequirements
| */ | ||
| public data class MfaRequirements( | ||
| @SerializedName("challenge") val challenge: List<MfaChallengeRequirement>?, | ||
| @SerializedName("enroll") val enroll: List<MfaChallengeRequirement>? |
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.
Lets add mfaToken also as part of this
gyaneshgouraw-okta
left a comment
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.
@utkrishtsahu I don't see examples updated as part of this PR. Please update examples also as for all new mfa methods added.
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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 20 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```kotlin | ||
| mfaClient | ||
| .verifyOtp(otp = "123456") | ||
| .validateClaims() | ||
| .start(object: Callback<Credentials, MfaVerifyException> { | ||
| override fun onFailure(exception: MfaVerifyException) { } | ||
|
|
||
| override fun onSuccess(credentials: Credentials) { | ||
| // MFA verification successful - user is now logged in | ||
| } | ||
| }) |
Copilot
AI
Jan 30, 2026
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.
The examples call .validateClaims() on mfaClient.verifyOtp(...) / verifyOob(...) / verifyRecoveryCode(...), but those methods return Request<Credentials, MfaVerifyException> (not AuthenticationRequest), so validateClaims() won’t be available. The docs should either omit validateClaims() here or provide an alternative claim-validation approach for MFA token exchanges.
| * Exception thrown when listing authenticators fails. | ||
| * | ||
| * SDK-thrown errors: | ||
| * - `invalid_request`: challengeType is required and must contain at least one challenge type |
Copilot
AI
Jan 30, 2026
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.
The KDoc for invalid_request mentions challengeType and “challenge type”, but the actual validation/error refers to factorsAllowed (factor types). Please update the documentation text so it matches the API parameter and meaning.
| * - `invalid_request`: challengeType is required and must contain at least one challenge type | |
| * - `invalid_request`: factorsAllowed is required and must contain at least one factor type |
| override fun fromException(cause: Throwable): MfaListAuthenticatorsException { | ||
| return if (isNetworkError(cause)) { | ||
| MfaListAuthenticatorsException( | ||
| code = "network_error", | ||
| description = "Failed to execute the network request" | ||
| ) | ||
| } else { | ||
| MfaListAuthenticatorsException( | ||
| code = Auth0Exception.UNKNOWN_ERROR, | ||
| description = cause.message ?: "Something went wrong" | ||
| ) | ||
| } | ||
| } |
Copilot
AI
Jan 30, 2026
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.
fromException creates a new MFA exception but drops the original cause (network and non-network). This loses stack traces and error classification (compare to AuthenticationAPIClient’s error adapter which wraps causes). Consider passing the throwable as the exception cause (and using NetworkErrorException for network failures).
| override fun fromException(cause: Throwable): MfaEnrollmentException { | ||
| return if (isNetworkError(cause)) { | ||
| MfaEnrollmentException( | ||
| code = "network_error", | ||
| description = "Failed to execute the network request" | ||
| ) | ||
| } else { | ||
| MfaEnrollmentException( | ||
| code = Auth0Exception.UNKNOWN_ERROR, | ||
| description = cause.message ?: "Something went wrong" | ||
| ) | ||
| } | ||
| } |
Copilot
AI
Jan 30, 2026
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.
Same as above: createEnrollmentErrorAdapter().fromException discards the original throwable. Please preserve cause when constructing MfaEnrollmentException so callers can inspect root errors and stack traces.
| ```kotlin | ||
| mfaClient | ||
| .getAuthenticators(factorsAllowed = requirements?.challenge ?: emptyList()) | ||
| .start(object: Callback<List<MfaAuthenticator>, MfaListAuthenticatorsException> { | ||
| override fun onFailure(exception: MfaListAuthenticatorsException) { | ||
| // Handle error | ||
| } | ||
|
|
||
| override fun onSuccess(authenticators: List<MfaAuthenticator>) { | ||
| // Display authenticators for user to choose | ||
| authenticators.forEach { auth -> | ||
| println("Type: ${auth.authenticatorType}, ID: ${auth.id}") | ||
| } | ||
| } | ||
| }) |
Copilot
AI
Jan 30, 2026
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.
The example doesn’t match the actual SDK types/signatures: requirements?.challenge is a List<MfaFactor> (not List<String>), and MfaAuthenticator isn’t a type in the SDK (the model is com.auth0.android.result.Authenticator). The snippet should map requirements.challenge to it.type and use Callback<List<Authenticator>, MfaListAuthenticatorsException>.
| ```kotlin | ||
| mfaClient | ||
| .enrollPhone("+11234567890", PhoneEnrollmentType.SMS) | ||
| .start(object: Callback<MfaEnrollment, MfaEnrollmentException> { | ||
| override fun onFailure(exception: MfaEnrollmentException) { } | ||
|
|
||
| override fun onSuccess(enrollment: MfaEnrollment) { | ||
| // Phone enrolled - need to verify with OOB code | ||
| val oobCode = enrollment.oobCode | ||
| val bindingMethod = enrollment.bindingMethod | ||
| } | ||
| }) |
Copilot
AI
Jan 30, 2026
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.
This enrollment example references non-existent APIs/types (enrollPhone(..., PhoneEnrollmentType.SMS) and MfaEnrollment). In the current implementation, enrollPhone only accepts a phone number and returns EnrollmentChallenge (or a specific subtype). Please update the example to use the real method signature and return type(s).
| val requirements = mfaPayload?.mfaRequirements | ||
|
|
||
| // Check what actions are available | ||
| val canChallenge = requirements?.challenge // List of authenticators to challenge |
Copilot
AI
Jan 30, 2026
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.
In the MFA-required example, requirements?.challenge / requirements?.enroll are lists of MfaFactor (factor types), not “authenticators to challenge”. The comments and variable naming imply you can directly challenge these; instead, you still need to call getAuthenticators(...) and then challenge(authenticatorId).
| val canChallenge = requirements?.challenge // List of authenticators to challenge | |
| val canChallenge = requirements?.challenge // List of factor types that can be challenged |
| "sms", "phone" -> effectiveType == "sms" || effectiveType == "phone" | ||
| "email" -> effectiveType == "email" | ||
| "otp", "totp" -> effectiveType == "otp" || effectiveType == "totp" | ||
| "oob" -> authenticator.authenticatorType == "oob" |
Copilot
AI
Jan 30, 2026
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.
matchesFactorType’s "oob" branch only checks authenticator.authenticatorType == "oob". If the API omits authenticator_type but returns type: "oob" (as in some test JSON), OOB authenticators won’t match and will be filtered out. Consider matching against authenticator.type and/or effectiveType instead.
| "oob" -> authenticator.authenticatorType == "oob" | |
| "oob" -> authenticator.authenticatorType == "oob" || effectiveType == "oob" |
| override fun fromException(cause: Throwable): MfaChallengeException { | ||
| return if (isNetworkError(cause)) { | ||
| MfaChallengeException( | ||
| code = "network_error", | ||
| description = "Failed to execute the network request" | ||
| ) | ||
| } else { | ||
| MfaChallengeException( | ||
| code = Auth0Exception.UNKNOWN_ERROR, | ||
| description = cause.message ?: "Something went wrong" | ||
| ) | ||
| } | ||
| } |
Copilot
AI
Jan 30, 2026
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.
createChallengeErrorAdapter().fromException currently drops the original throwable. Consider storing it as the cause (and wrapping network errors similarly to other SDK clients) to keep diagnostics intact.
| public sealed class EnrollmentChallenge { | ||
| public abstract val id: String? | ||
| public abstract val authSession: String | ||
| public abstract val authSession: String? | ||
| public open val oobCode: String? = null |
Copilot
AI
Jan 30, 2026
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.
EnrollmentChallenge.authSession was changed to nullable. Since this is a public model used across the SDK (e.g., MyAccount enrollment/verification flows), making it nullable is a breaking API change for consumers. If the API always returns auth_session, keep it non-null; if it can be absent, consider introducing a separate subtype or documenting/handling the nullability explicitly.
|
|
||
| > The default scope used is `openid profile email`. Regardless of the scopes set to the request, the `openid` scope is always enforced. | ||
|
|
||
| ### MFA Flexible Factors Grant |
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.
@utkrishtsahu Please add a disclaimer that this feature is in early access.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
|
|
||
| // Check what actions are available | ||
| val canChallenge = requirements?.challenge // List of authenticators to challenge | ||
| val canEnroll = requirements?.enroll // List of factor types that can be enrolled |
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.
It can be either challenge type or enroll type. The example should showcase only one can be present at a time. Check SPA for reference
| * @param authenticators The list of authenticators to deduplicate | ||
| * @return A deduplicated list with one authenticator per unique identity | ||
| */ | ||
| private fun deduplicateAuthenticators(authenticators: List<Authenticator>): List<Authenticator> { |
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.
Is this a valid scenario ? Do we need to handle duplicate authenticators post filtering ? @gyaneshgouraw-okta
| .thenByDescending { it.createdAt ?: "" } | ||
| ).first() | ||
| } | ||
| } |
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.
nit: move all private methods after the public methods
The order should be:
private properties
public properties
public methods
private methods / /As these are generally helper methods
| * @see [Auth0 Guardian](https://auth0.com/docs/secure/multi-factor-authentication/auth0-guardian) | ||
| */ | ||
| public fun enrollPush(): Request<EnrollmentChallenge, MfaEnrollmentException> { | ||
| return enrollOob(oobChannel = "auth0") |
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.
Is auth0 the only oob channel value possible ? Can we have push notification via other authenticator apps ?
cc @gyaneshgouraw-okta
| */ | ||
| @get:JvmName("getMfaToken") | ||
| public val mfaToken: String? | ||
| get() = mfaRequiredErrorPayloadValue?.mfaToken |
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.
do we need this property now since mfaRequiredErrorPayload contains the token already
| public sealed class EnrollmentChallenge { | ||
| public abstract val id: String? | ||
| public abstract val authSession: String | ||
| public abstract val authSession: String? |
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.
Why is this being made nullable type?
| public data class MfaEnrollmentChallenge( | ||
| @SerializedName("id") | ||
| override val id: String, | ||
| override val id: String?, |
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.
Why is this being made nullable ?
| assertThat(verifyException.message, containsString("custom_error_code")) | ||
| } | ||
|
|
||
| } |
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.
CredentialsManager exceptions are not being handled in test cases ?
Summary:
Adds comprehensive Multi-Factor Authentication (MFA) support to the Android SDK, enabling TOTP, SMS, Email, and recovery code authentication flows.
Changes
New Public APIs:
AuthenticationAPIClient.mfa(mfaToken): MfaApiClient - Factory method for MFA operations
MfaApiClient - Complete MFA client with methods for:
New Error Types:
MfaException sealed class hierarchy for type-safe MFA error handling
Specific exceptions: MfaListAuthenticatorsException, MfaEnrollmentException, MfaChallengeException, MfaVerifyException, MfaRequiredException
Includes fallback error codes aligned
New Data Models:
Authenticator, Challenge, EnrollmentChallenge, TotpEnrollmentChallenge, OobEnrollmentChallenge
Endpoints:
Testing
Tested on physical device with sample app.
Unit Test yet to write.
Sample app includes comprehensive MFA workflow examples
Reference Ticket
https://auth0team.atlassian.net/jira/software/c/projects/SDK/boards/2607?assignee=712020%3A746f4583-dada-45dc-8f76-07f273104490&selectedIssue=SDK-6405