fix(swiftui-mfa): Journey OIDC config, SDK 2.0, and MFA auto-registration (SDKS-5029)#113
Open
george-bafaloukas-forgerock wants to merge 4 commits into
Open
fix(swiftui-mfa): Journey OIDC config, SDK 2.0, and MFA auto-registration (SDKS-5029)#113george-bafaloukas-forgerock wants to merge 4 commits into
george-bafaloukas-forgerock wants to merge 4 commits into
Conversation
…DKS-5029) - Add PingOidc to project dependencies (build file, frameworks phase, package product) - Replace 3-field Journey stub with full Journey.createJourney block including PingJourney.OidcModule.config (clientId, redirectUri, discoveryEndpoint, scopes) - Add commented-out PingOne block so sample covers both AM/AIC and PingOne environments - Annotate every placeholder with TODO comments explaining what to fill in - Add import PingOidc to JourneyManager.swift - Add TODO comment on Journey tree name in LoginViewModel.startLogin() - Update README: full config examples for both AM/AIC and PingOne, clarify AppConfiguration.swift is optional, fix FailureNode.cause non-optional usage, note journey name is case-sensitive - Package.resolved: pin ping-ios-sdk to 2.0.0 (was develop branch), bump pingone-signals-sdk-ios to 5.4.0 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Remove the commented-out PingOne standalone configuration block from JourneyManager.swift and all corresponding PingOne references from the README — this sample targets PingAM / AIC exclusively. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
…KS-5029) When the Journey returns a HiddenValueCallback with an MFA registration URI, register the credential automatically instead of exposing it in the UI. - JourneyManager: add isMfaRegistering published flag, set true/false around the credential registration call in detectAndHandleMfaRegistration - LoginViewModel: forward isMfaRegistering from JourneyManager; add isMfaRegistrationNode computed property to detect the registration node - LoginScreen: branch on isMfaRegistrationNode before the normal callback list; show MfaRegistrationView (spinner while registering, success + Continue when done) - Add MfaRegistrationView component: ProgressView during registration, checkmark shield on success, Continue button enabled only after completion Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR integrates MFA credential registration support into the iOS SwiftUI sample by adding the PingOidc dependency, configuring Journey with explicit OIDC settings, tracking registration state through the view model, rendering a dedicated registration UI, and updating documentation. ChangesMFA Registration End-to-End Implementation
Sequence Diagram(s)sequenceDiagram
participant User
participant LoginScreen
participant LoginViewModel
participant JourneyManager
participant PingOidc
User->>LoginScreen: Interacts with login flow
LoginViewModel->>JourneyManager: detectAndHandleMfaRegistration()
JourneyManager->>JourneyManager: isMfaRegistering = true
JourneyManager->>PingOidc: registerMfaCredential()
PingOidc-->>JourneyManager: registration callback processed
JourneyManager->>JourneyManager: isMfaRegistering = false
JourneyManager-->>LoginViewModel: publish isMfaRegistering state
LoginViewModel-->>LoginScreen: update isMfaRegistrationNode
LoginScreen->>LoginScreen: render MfaRegistrationView
User->>LoginScreen: taps Continue button
LoginScreen->>LoginViewModel: submitNode()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
🧹 Nitpick comments (3)
iOS/swiftui-mfa/README.md (1)
236-251: ⚡ Quick winConsider documenting URL scheme registration for the OAuth2 redirect URI.
The installation instructions include configuring the
redirectUri(e.g.,"com.example.mfasample://oauth2redirect"), but don't explicitly mention that developers must register the corresponding URL scheme (com.example.mfasample) inInfo.plistunderCFBundleURLTypes.Without this registration, the OAuth2 redirect will fail at runtime. Consider adding a note or step reminding developers to register their custom URL scheme.
🤖 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 `@iOS/swiftui-mfa/README.md` around lines 236 - 251, Add a short note in the README near the redirectUri configuration (the block that calls Journey.createJourney and sets oidcConfig.redirectUri in JourneyManager.swift) instructing developers to register the redirect URI's URL scheme (the scheme part of oidcConfig.redirectUri, e.g., "com.example.mfasample") in the app's Info.plist under CFBundleURLTypes/CFBundleURLSchemes so the OAuth2 redirect will be handled by the app at runtime.iOS/swiftui-mfa/MfaSample/MfaSample/ViewModels/LoginViewModel.swift (1)
33-39: ⚡ Quick winConsider centralizing MFA registration node detection logic.
The detection logic for MFA registration nodes is duplicated between
LoginViewModel.isMfaRegistrationNode(lines 35-37) andJourneyManager.detectAndHandleMfaRegistration(lines 131-133). Both check forHiddenValueCallbackwithvalueId == "mfaDeviceRegistration"and a non-empty value.If the detection criteria change in the future, both locations must be updated in sync. Consider either:
- Exposing a helper method from
JourneyManagerto check if a node is an MFA registration node- Adding a comment linking both implementations to maintain awareness of the coupling
🤖 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 `@iOS/swiftui-mfa/MfaSample/MfaSample/ViewModels/LoginViewModel.swift` around lines 33 - 39, Duplicate MFA-registration detection exists in LoginViewModel.isMfaRegistrationNode and JourneyManager.detectAndHandleMfaRegistration: both look for a HiddenValueCallback with valueId == "mfaDeviceRegistration" and non-empty value; centralize by moving that check into a single helper (e.g., JourneyManager.isMfaRegistrationNode(node: Node) or a static helper on JourneyManager) and update LoginViewModel.isMfaRegistrationNode to call that helper (inspect currentNode as ContinueNode and pass it), then remove the duplicated logic from detectAndHandleMfaRegistration or call the same helper there so both locations reference the single implementation (referenced symbols: isMfaRegistrationNode, currentNode, ContinueNode, HiddenValueCallback, JourneyManager.detectAndHandleMfaRegistration).iOS/swiftui-mfa/MfaSample/MfaSample/Core/Managers/JourneyManager.swift (1)
127-140: ⚡ Quick winConsider surfacing registration failures to the user.
Currently, if
registerMfaCredentialencounters an error (e.g., invalid URI, SDK failure), the error is logged butisMfaRegisteringis still set tofalse, causing the UI to display a success state. Users won't know registration failed until they attempt to authenticate with MFA on a subsequent login.Consider adding an error state that:
- Exposes registration failures via a published property
- Shows an error message in
MfaRegistrationView- Allows retry or informing the user to contact support
🤖 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 `@iOS/swiftui-mfa/MfaSample/MfaSample/Core/Managers/JourneyManager.swift` around lines 127 - 140, detectAndHandleMfaRegistration currently sets isMfaRegistering true then calls registerMfaCredential and unsets isMfaRegistering but does not expose errors to the UI; change it to catch failures from registerMfaCredential and surface them via a new `@Published` property (e.g., mfaRegistrationError: String? or isMfaRegistrationFailed: Bool plus mfaRegistrationErrorMessage) on JourneyManager, ensure isMfaRegistering is cleared in a finally/defer-like block, set the error property when registerMfaCredential throws or returns failure, and update MfaRegistrationView to observe this published property to show the error and offer a retry action that calls detectAndHandleMfaRegistration/registerMfaCredential again.
🤖 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.
Nitpick comments:
In `@iOS/swiftui-mfa/MfaSample/MfaSample/Core/Managers/JourneyManager.swift`:
- Around line 127-140: detectAndHandleMfaRegistration currently sets
isMfaRegistering true then calls registerMfaCredential and unsets
isMfaRegistering but does not expose errors to the UI; change it to catch
failures from registerMfaCredential and surface them via a new `@Published`
property (e.g., mfaRegistrationError: String? or isMfaRegistrationFailed: Bool
plus mfaRegistrationErrorMessage) on JourneyManager, ensure isMfaRegistering is
cleared in a finally/defer-like block, set the error property when
registerMfaCredential throws or returns failure, and update MfaRegistrationView
to observe this published property to show the error and offer a retry action
that calls detectAndHandleMfaRegistration/registerMfaCredential again.
In `@iOS/swiftui-mfa/MfaSample/MfaSample/ViewModels/LoginViewModel.swift`:
- Around line 33-39: Duplicate MFA-registration detection exists in
LoginViewModel.isMfaRegistrationNode and
JourneyManager.detectAndHandleMfaRegistration: both look for a
HiddenValueCallback with valueId == "mfaDeviceRegistration" and non-empty value;
centralize by moving that check into a single helper (e.g.,
JourneyManager.isMfaRegistrationNode(node: Node) or a static helper on
JourneyManager) and update LoginViewModel.isMfaRegistrationNode to call that
helper (inspect currentNode as ContinueNode and pass it), then remove the
duplicated logic from detectAndHandleMfaRegistration or call the same helper
there so both locations reference the single implementation (referenced symbols:
isMfaRegistrationNode, currentNode, ContinueNode, HiddenValueCallback,
JourneyManager.detectAndHandleMfaRegistration).
In `@iOS/swiftui-mfa/README.md`:
- Around line 236-251: Add a short note in the README near the redirectUri
configuration (the block that calls Journey.createJourney and sets
oidcConfig.redirectUri in JourneyManager.swift) instructing developers to
register the redirect URI's URL scheme (the scheme part of
oidcConfig.redirectUri, e.g., "com.example.mfasample") in the app's Info.plist
under CFBundleURLTypes/CFBundleURLSchemes so the OAuth2 redirect will be handled
by the app at runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a309c585-24c1-40ed-b896-2db888593a07
📒 Files selected for processing (6)
iOS/swiftui-mfa/MfaSample/MfaSample.xcodeproj/project.pbxprojiOS/swiftui-mfa/MfaSample/MfaSample.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvediOS/swiftui-mfa/MfaSample/MfaSample/Core/Managers/JourneyManager.swiftiOS/swiftui-mfa/MfaSample/MfaSample/ViewModels/LoginViewModel.swiftiOS/swiftui-mfa/MfaSample/MfaSample/Views/Screens/LoginScreen.swiftiOS/swiftui-mfa/README.md
- Centralize MFA registration node detection: move the HiddenValueCallback check into JourneyManager.nodeIsMfaRegistration (static helper); LoginViewModel.isMfaRegistrationNode delegates to it, eliminating duplication - Surface registration failures: registerMfaCredential now throws; errors are caught in detectAndHandleMfaRegistration and published via mfaRegistrationError; use defer to guarantee isMfaRegistering is always cleared - MfaRegistrationView: accept errorMessage param, show error state with exclamationmark.shield icon, and relabel button to "Retry" on failure - README: add URL scheme registration note next to the redirectUri config step Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes four issues reported in SDKS-5029 in the
swiftui-mfasample app.JourneyManager.swiftwith a fullJourney.createJourneyblock includingPingJourney.OidcModule.config(clientId,redirectUri,discoveryEndpoint,scopes), scoped to PingAM / AIC onlyimport PingOidc: Added toJourneyManager.swift;PingOidcwired into the Xcode project as a linked frameworkLoginViewModel.startLogin()annotated with aTODOcomment explaining where to find the tree name in AM Admin and that it is case-sensitiveHiddenValueCallbackwith an MFA registration URI, the credential is now registered automatically. AMfaRegistrationViewshows a spinner while registration is in progress and a success state with a Continue button when done — the raw URI is never shown to the userping-ios-sdkpinned to2.0.0(wasdevelopbranch);pingone-signals-sdk-iosbumped to5.4.0AppConfiguration.swiftis optional, and a step for setting the Journey tree nameTest plan
HiddenValueCallbackwithvalueId == "mfaDeviceRegistration", the spinner appears automatically and registration completes without user input🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation