fix(MFAClient): Add challengeType parameter to challenge method - #1263
fix(MFAClient): Add challengeType parameter to challenge method#1263Dodothereal wants to merge 1 commit into
Conversation
- Add optional challengeType parameter to MFAClient.challenge() method - Allow specifying 'otp' for OTP authenticators instead of hardcoded 'oob' - Maintain backward compatibility with default 'oob' behavior - Update documentation with usage examples for both OOB and OTP authenticators - Add unit test for OTP challenge type - Update EXAMPLES.md with OTP authenticator usage examples Fixes issue where MFAClient.challenge assumed challenge_type should be 'oob', breaking OTP authentication
📝 WalkthroughWalkthroughThe MFA challenge API now accepts an optional ChangesMFA challenge type support
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/MFA/MFAClient.swift`:
- Around line 132-133: Add a public MFAClient protocol extension with the
original two-argument challenge(with:mfaToken:) method, delegating to the
existing three-argument overload with challengeType set to nil, so existing SDK
callers remain source-compatible.
In `@Auth0Tests/MFA/Auth0MFAClientTests.swift`:
- Around line 342-350: Update testMFAChallengeWithOTPType to follow the
repository’s established testing standard: use UpperCamelCase constants
(ClientId, Domain, AuthenticatorId, MfaToken), place the behavior in the
appropriate QuickSpec subclass using describe/context/it, and use
StubURLProtocol or NetworkStub for network setup instead of MockURLProtocol; if
the repository has migrated to Swift Testing and MockURLProtocol, update the
path instructions and at minimum correct the constant names.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 128b4042-72cc-448e-8817-6f7aebbca67e
📒 Files selected for processing (4)
Auth0/MFA/Auth0MFAClient.swiftAuth0/MFA/MFAClient.swiftAuth0Tests/MFA/Auth0MFAClientTests.swiftEXAMPLES.md
| */ | ||
| func challenge(with authenticatorId: String, mfaToken: String) -> any Requestable<MFAChallenge, MfaChallengeError> | ||
| func challenge(with authenticatorId: String, mfaToken: String, challengeType: String?) -> any Requestable<MFAChallenge, MfaChallengeError> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Retain source compatibility for existing SDK callers.
Adding challengeType: String? to the protocol method signature without retaining the original signature removes the two-argument method. Because Swift protocols do not support default parameter values, any existing consumer code calling Auth0.mfa().challenge(with: "...", mfaToken: "...") will fail to compile due to a missing argument.
To prevent breaking existing callers, please provide a default implementation of the original signature via a protocol extension.
🛠 Proposed protocol extension (append to the end of the file)
public extension MFAClient {
/// Initiates an MFA challenge for an enrolled authenticator.
///
/// - Parameters:
/// - authenticatorId: The ID of the enrolled authenticator.
/// - mfaToken: The MFA token received from an authentication error when MFA is required.
/// - Returns: A request that will yield an MFA challenge.
func challenge(with authenticatorId: String, mfaToken: String) -> any Requestable<MFAChallenge, MfaChallengeError> {
return challenge(with: authenticatorId, mfaToken: mfaToken, challengeType: nil)
}
}🤖 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/MFA/MFAClient.swift` around lines 132 - 133, Add a public MFAClient
protocol extension with the original two-argument challenge(with:mfaToken:)
method, delegating to the existing three-argument overload with challengeType
set to nil, so existing SDK callers remain source-compatible.
| @Test | ||
| func testMFAChallengeWithOTPType() async { | ||
| let clientId = "testClientId" | ||
| let domain = "test.example.com" | ||
| let authenticatorId = "testAuthenticatorId" | ||
| let mfaToken = "testToken" | ||
| let expectedURL = URL(string: "https://\(domain)/mfa/challenge")! | ||
|
|
||
| let request = Auth0.mfa(clientId: clientId, domain: domain, session: makeMockSession()).challenge(with: authenticatorId, mfaToken: mfaToken, challengeType: "otp") |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align test with path instructions for Quick/Nimble, NetworkStub, and constant naming.
This new test deviates from the repository's path instructions in a few areas:
- Test Constants: Constants must use
UpperCamelCase(e.g.,ClientId,Domain,AuthenticatorId,MfaToken) instead ofcamelCase. - Test Structure: Behavior must be organized using
describe/context/itblocks (Quick/Nimble) rather than@Test(Swift Testing). - Network Stubbing: All network interactions must use
StubURLProtocol/NetworkStubinstead ofMockURLProtocol.
As per path instructions:
- "Test constants must use
UpperCamelCase(e.g.AccessToken,ClientId,Domain)." - "Every spec file must be a
QuickSpecsubclass named<Subject>Spec" and behavior organized withdescribe/context/it. - "All network interactions must use
StubURLProtocol/NetworkStub".
(Note: If the repository has recently migrated to Swift Testing and MockURLProtocol, please update the path instructions to reflect the new standard; otherwise, please update the test constants at a minimum to align with the style guide.)
🤖 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 `@Auth0Tests/MFA/Auth0MFAClientTests.swift` around lines 342 - 350, Update
testMFAChallengeWithOTPType to follow the repository’s established testing
standard: use UpperCamelCase constants (ClientId, Domain, AuthenticatorId,
MfaToken), place the behavior in the appropriate QuickSpec subclass using
describe/context/it, and use StubURLProtocol or NetworkStub for network setup
instead of MockURLProtocol; if the repository has migrated to Swift Testing and
MockURLProtocol, update the path instructions and at minimum correct the
constant names.
Source: Path instructions
|
Hi @Dodothereal thanks for raising the PR. Could you please verify the commit signatures and also rebase the branch with the latest master branch |
2 participants
fix(MFAClient): Add challengeType parameter to challenge method
Summary
Fixes MFAClient.challenge() method to accept an optional challengeType parameter, allowing proper OTP authentication challenges instead of hardcoding "oob" for all authenticator types.
Previously, the challenge method always used "oob" as the challenge_type, which breaks OTP (one-time password) authenticator flows that require "otp" as the challenge_type. This change adds backward compatibility by making the parameter optional with a default value of "oob".
Fixes #1262
Changes
Testing
AI assistance disclosure
This contribution was produced by an autonomous AI coding agent (Claude Code) that @Dodothereal operates and monitors. @Dodothereal is accountable for it, will address review feedback promptly, and will close this PR immediately if this kind of contribution is unwelcome in this project. Commits carry an
Assisted-by: Claude Codetrailer.Summary by CodeRabbit
New Features
Documentation
Tests