Implement SMS method selection for SSPR v2 - #3092
Implement SMS method selection for SSPR v2#3092Sergei Demchenko (antrix1989) with Copilot wants to merge 6 commits into
Conversation
Co-authored-by: antrix1989 <1989385+antrix1989@users.noreply.github.com>
Co-authored-by: antrix1989 <1989385+antrix1989@users.noreply.github.com>
Co-authored-by: antrix1989 <1989385+antrix1989@users.noreply.github.com>
Co-authored-by: antrix1989 <1989385+antrix1989@users.noreply.github.com>
Co-authored-by: antrix1989 <1989385+antrix1989@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR extends Native Auth v2 SSPR (password reset) to support SMS as a first-factor method alongside email, including a reusable public “auth method selection required” state/delegate for flows that need the app/user to choose between multiple available methods.
Changes:
- Add a generic experimental public
MSALNativeAuthAuthMethodSelectionRequiredState+MSALNativeAuthAuthMethodSelectionRequiredDelegate, and route it through the v2 response dispatcher. - Add SMS support to v2 challenge-method parsing/modeling and update the reset-password controller flow to auto-select when there’s only one valid method, or return the selection state when there are multiple.
- Add unit + deterministic integration coverage and ensure new Swift sources/tests are included in Xcode targets; add a changelog entry under
#TBD.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| MSAL/src/native_auth/public/state_machine/v2/state/MSALNativeAuthAuthMethodSelectionRequiredState.swift | Introduces reusable public method-selection state/delegate (experimental API). |
| MSAL/src/native_auth/controllers/v2/MSALNativeAuthFlowController.swift | Implements SMS-aware method filtering, selection-state routing, and password-reset selection handling. |
| MSAL/src/native_auth/controllers/v2/MSALNativeAuthFlowResponseDispatcher.swift | Dispatches the new selection-required state to an opt-in typed delegate with correct scenario routing. |
| MSAL/src/native_auth/network/responses/v2/parser/MSALNativeAuthV2ParsedResponses.swift | Adds sms channel type to v2 parsed challenge method channel enum. |
| MSAL/test/unit/native_auth/public/state_machine/v2/MSALNativeAuthAuthMethodSelectionRequiredStateTests.swift | Verifies public state exposes methods and forwards selection to controller. |
| MSAL/test/unit/native_auth/network/v2/MSALNativeAuthV2ResponseParserTests.swift | Adds parser coverage for single SMS and mixed email/SMS methods. |
| MSAL/test/unit/native_auth/mock/v2/MSALNativeAuthFlowControllerMock.swift | Captures selection inputs for state-machine forwarding tests. |
| MSAL/test/unit/native_auth/controllers/v2/MSALNativeAuthFlowResponseDispatcherTests.swift | Adds dispatcher coverage for conforming vs non-conforming selection delegates (scenario/correlation + telemetry behavior). |
| MSAL/test/unit/native_auth/controllers/v2/MSALNativeAuthFlowControllerTests.swift | Adds controller coverage for 0/1/many methods, SMS flows, selection routing, and resend behavior with SMS. |
| MSAL/test/integration/native_auth/end_to_end/reset_password/MSALNativeAuthResetPasswordV2EndToEndTests.swift | Updates deterministic reset-password E2E delegate to handle method-selection-required and continue the flow. |
| MSAL/MSAL.xcodeproj/project.pbxproj | Adds new Swift source/test files to the appropriate targets. |
| CHANGELOG.md | Adds a #TBD entry for Native Auth v2 SSPR SMS method selection support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case .signIn: | ||
| apiId = .telemetryApiIdV2MFAGetAuthMethods | ||
| case .passwordReset: | ||
| apiId = .telemetryApiIdV2ResetPasswordStart |
There was a problem hiding this comment.
Reusing telemetryApiIdV2ResetPasswordStart here means a multi-method reset emits API id 76010 twice under the same correlation id — once from resetPassword() and again from this selection step. Telemetry won't be able to separate "reset started" from "user picked a method", and the start-op success rate gets diluted by selection failures.
Sign-in avoids exactly this by having a distinct telemetryApiIdV2MFAGetAuthMethods for the equivalent step. Can we get a dedicated id (e.g. telemetryApiIdV2ResetPasswordSelectAuthMethod) allocated instead of overloading the start id? If it has to ship as-is, please leave a TODO here so it doesn't silently become permanent.
| return handleMFASelectAuthMethodResult(result, flowContinuationState: flowContinuationState, step: step) | ||
| case .passwordReset: | ||
| return await handlePasswordResetChallengeResult(result, flowContinuationState: flowContinuationState, step: step) | ||
| default: |
There was a problem hiding this comment.
This default is unreachable — the switch at line 437 already returns for anything that isn't .signIn/.passwordReset. Fine today, but it's a trap: if a scenario is later added to the first switch and not to this one, we return notImplementedResponse after starting the telemetry event (line 445) and firing the network request (line 463), and the event is never stopped. notImplementedResponse also mints a fresh UUID() correlation id, so that failure won't correlate with the request we just sent.
Cleaner to resolve the api id and the result handler together in a single switch so the two can't drift apart.
| switch result { | ||
| case .verificationRequired(let token, let verifyHref, let resendHref, let sentTo, let channelType, let codeLength): | ||
| guard channelType.isEmailType else { | ||
| guard channelType.isEmailType || channelType.isSMSType else { |
There was a problem hiding this comment.
handleResendCodeResult is shared with sign-in — line 846 right below branches on flowScenario == .signIn. So widening this guard silently changes sign-in resend behaviour too: a sms channel that previously produced a general error now returns a MSALNativeAuthCodeRequiredState.
That's probably the behaviour we want long term, but it's outside the stated "preserve existing sign-in behaviour" scope and it's untested — makeState in MSALNativeAuthFlowControllerTests hardcodes .passwordReset, and MSALNativeAuthFlowControllerSignInTests wasn't touched. Either gate the SMS allowance on flowScenario == .passwordReset, or add sign-in coverage and call the change out in the changelog.
| ) | ||
| } | ||
|
|
||
| private func makeAuthMethodSelectionContinuation( |
There was a problem hiding this comment.
This is a near-copy of makeMFAContinuation (line 697) — same resolver, same .method(id:) link map, same continuation rebuild. The one difference is error semantics: makeMFAContinuation uses try? and silently drops methods it can't resolve, while this one fails the entire flow on the first bad href.
So the same malformed server payload now behaves differently depending on whether it arrives via MFA or SSPR. I'd rather see one helper with one policy (fail-closed is the better policy — silently dropping a method the user can see in the server response is worse). Worth consolidating while both call sites are still new.
Separately: the guard let continuationToken on line 941 can't fail from the only caller — .challengeRequired carries a non-optional token — but if it ever did, the caller maps nil to invalidAuthMethodLinkResponse, so a missing token would be reported to the app as "Invalid challenge link for authentication method". Returning a typed reason instead of nil would keep that honest.
| let resolver = MSALNativeAuthV2HrefURLResolver(config: config) | ||
| var resolvedLinks: [MSALNativeAuthV2LinkKey: URL] = [:] | ||
| for method in methods { | ||
| guard method.challengeHref.rangeOfCharacter(from: .whitespacesAndNewlines) == nil else { |
There was a problem hiding this comment.
This guard contradicts the resolver it's guarding. MSALNativeAuthV2HrefURLResolver.url(forHref:) deliberately starts with href.trimmingCharacters(in: .whitespacesAndNewlines), so leading/trailing whitespace is explicitly supported everywhere else in the SDK. Here a trailing \n on an otherwise valid href fails the whole reset flow — and only in the multi-method path, since the single-method path at line 199 hands method.challengeHref straight to requestProvider.challenge(href:) with no validation at all. Same server data, two different outcomes.
The try resolver.url(forHref:) on line 953 already rejects unresolvable hrefs, so this guard looks like it exists to make test_resetPassword_whenMultipleMethodsContainInvalidChallengeLink_returnsError deterministic rather than to enforce a real invariant — meaning that test is asserting on this guard, not on URL resolution. Suggest dropping the guard and letting the resolver throw; if a stricter check is genuinely needed, trim first and only reject interior whitespace.
| return response(.actionRequired(state: state), context: step.context) | ||
| } | ||
|
|
||
| private func publicAuthMethods(from methods: [MSALNativeAuthV2ChallengeMethod]) -> [MSALAuthMethod] { |
There was a problem hiding this comment.
This is byte-for-byte the mapping already inlined in mfaRequiredResponse (lines 725–732). Now that it's been extracted, please have mfaRequiredResponse call publicAuthMethods(from:) too so the MSALAuthMethod construction lives in one place.
|
|
||
| enum MSALNativeAuthV2ChallengeMethodChannelType: String { | ||
| case email | ||
| case sms |
There was a problem hiding this comment.
Worth confirming the casing contract before this ships. parseChallengeMethods builds this via MSALNativeAuthV2ChallengeMethodChannelType(rawValue: method.type ?? ""), which is case-sensitive, and a single unrecognized entry sends the whole response to invalidMethod — so a server returning "SMS" instead of "sms" doesn't just drop the SMS option, it fails the entire reset flow. Dropping .lowercased() from isEmailType/isPasswordType below makes the code read as if casing is deliberate, but the actual sensitivity is in the parser, not here.
Given SMS is new on the wire, either confirm the service always emits lowercase, or lowercase method.type at the parse site.
| /// - verificationContact: An optional contact value to verify for flows that require the app to | ||
| /// provide one. Pass `nil` when the server-provided method already contains the destination. | ||
| /// - delegate: The delegate that receives the next flow callback. | ||
| public func selectAuthMethod( |
There was a problem hiding this comment.
MSALNativeAuthMFARequiredState — which this class mirrors almost exactly (same stored property, same init, same selectAuthMethod body) — also ships a convenience overload:
public func selectAuthMethod(_ method: MSALAuthMethod, delegate: MSALNativeAuthFlowDelegate)For password reset verificationContact is always nil (the server-provided method already carries the destination — your own doc comment and every call site say so), so every caller in this flow is forced to write verificationContact: nil. Please add the same two-argument overload for parity.
More broadly: this PR now leaves two parallel public selection APIs with identical shapes and two near-identical dispatcher cases, while MSALNativeAuthMFARequiredState isn't deprecated or pointed at the new "generic" one. If the intent is that this becomes the selection state, it'd help to say so here in the doc comment (and open the follow-up to migrate MFA), otherwise integrators won't know which to conform to.
|
|
||
| /* Begin PBXBuildFile section */ | ||
| 01462653AC546A8B95A0D912 /* MSALNativeAuthMFARequiredState.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8FE5D3DE054DBA691A401E3D /* MSALNativeAuthMFARequiredState.swift */; }; | ||
| 0AA10000000000000000AA01 /* MSALNativeAuthAuthMethodSelectionRequiredState.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0AA10000000000000000AF01 /* MSALNativeAuthAuthMethodSelectionRequiredState.swift */; }; |
There was a problem hiding this comment.
These object ids are hand-minted placeholders (0AA10000000000000000AA01…AA04, …AF01/AF02) rather than the random 24-hex ids Xcode generates. Two practical problems:
- A sequential, low-entropy prefix like this is much likelier to collide with another hand-written id in a future PR than a random one, and pbxproj id collisions fail in confusing ways.
- They're inserted out of order —
0AA10000…lands between01462653…and01F6FDA4…in every section. Xcode re-sorts on the next project save, so whoever next opens the project in the IDE will produce an unrelated reshuffle diff.
Please regenerate these by adding the files through Xcode (or with random hex) so the ids and ordering match the rest of the file.
Original prompt
Implement and deliver SSPR v2 SMS method selection in
AzureAD/microsoft-authentication-library-for-objc.Base the pull request on
dev. Follow the repository's.github/copilot-instructions.mdand path-scoped instructions strictly. Workautonomously through implementation, tests, pull-request creation, and required
CI repairs. Do not stop when the pull request is merely open.
Requirements
The server's HAL reset-password response has
challengeContext.authenticationFactor = singleFactor; its_embedded.methodsarray may contain bothemailandsms, each with anid, maskedloginHint, and method-specific challenge link.an SSPR-specific API. Expose
[MSALAuthMethod],selectAuthMethod, and atyped delegate callback carrying
MSALNativeAuthFlowScenario. For this flow,the scenario must be
.passwordReset. Name it consistently with existing v2APIs (for example,
MSALNativeAuthAuthMethodSelectionRequiredStateandMSALNativeAuthAuthMethodSelectionRequiredDelegate). It must beObjective-C-visible and reusable later for sign-in email/password method
selection.
offers one email or SMS method, challenge it automatically. If it offers
multiple methods, return the generic public selection state.
and return
MSALNativeAuthCodeRequiredStatewith the correct channel andmasked destination.
selectAuthMethodmust route byflowScenario:.passwordResetuses the password-reset challenge-resulthandler;
.signInretains MFA handling.MSALNativeAuthV2ChallengeMethodChannelTypeand the v2 parsermapping. Public
MSALNativeAuthChannelTypealready supports SMS.MSALNativeAuthFlowResponseDispatcherso a conforming delegatereceives the generic state with
.passwordReset; a non-conforming delegatereceives
.notImplementedwith the same scenario and correlation ID.reset-password operation ID unless maintainers provide a dedicated ID.
email, selecting SMS, invalid method/link, missing continuation token, and
scenario propagation;
and telemetry completion;
it, without introducing a live SMS inbox dependency.
a user-visible entry under
#TBDinCHANGELOG.md.yongdi/native-auth-v2-sign-inas a reference, adapted to Apple behavior:auto-select one method and invoke selection only for multiple methods.
unrelated changes and never place captured trace PII in code or fixtures.
Likely files
MSAL/src/native_auth/network/responses/v2/parser/MSALNativeAuthV2ParsedResponses.swiftMSAL/src/native_auth/network/responses/v2/parser/MSALNativeAuthV2ResponseParser.swiftMSAL/src/native_auth/public/state_machine/v2/state/MSAL/src/native_auth/controllers/v2/MSALNativeAuthFlowController.swiftMSAL/src/native_auth/controllers/v2/MSALNativeAuthFlowResponseDispatcher.swiftMSAL/MSAL.xcodeproj/project.pbxprojCHANGELOG.mdValidation and delivery
code signing disabled:
CODE_SIGN_IDENTITY="" CODE_SIGNING_REQUIRED=NO.origin/devfor scope, PII, Xcode targetmembership, API docs, and changelog placement.
AzureAD/microsoft-authentication-library-for-objcwith basedev.its job/log details, fix all actionable code/test/project/configuration
failures, retry transient infrastructure failures when supported, push the
correction, and restart the required-check watch.
pull-request creation, or partial green status are not completion.