Feature/mfa integration - #161
Conversation
… and PingExample sample app — including category registration, banner-action and foreground push routing, and an approve/deny modal with number-matching UI. ## What Was Built Add PingOneMFA actionable push notification support to the SDK module and PingExample sample app — including category registration, banner-action and foreground push routing, and an approve/deny modal with number-matching UI. ## Key Decisions - Category routing via runtime Set<UNNotificationCategory> (no hardcoded strings) - Number selection IS approval for SELECT_NUMBER/ENTER_MANUALLY flows - authenticationMethod: "user" hardcoded per InternalApp reference - Modal-only — no new tab or navigation destination - willPresent and didReceive retry initializePingOneMFAClient() when SDK is uninitialized ## Changes - PingOneMFA/PingOneMFA/PingOneMFA.swift — getNotificationCategories(), processNotificationAction(), parseAPNSAlert helper - PingOneMFA/PingOneMFA/PushNotification.swift — numberMatchingOptions, numberMatchingType, Identifiable conformance - PingOneMFA/PingOneMFATests/ — mock tracking state + test21/22/23 - SampleApps/PingExample/PingExample/AppDelegate.swift — full PingOneMFA push wiring - SampleApps/PingExample/PingExample/PingOneMFANotificationView.swift — new approve/deny modal - SampleApps/PingExample/PingExample/ContentView.swift — ShowPingOneMFANotification sheet Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…otificationView Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…class name collision PingOneMFA.PushNotification is ambiguous at call sites that import PingOneMFA because Swift resolves 'PingOneMFA' as the class, not the module. MFAPushNotification is an unambiguous alias used in AppDelegate and ContentView. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…moteNotificationsWithDeviceToken Matches the PingPush getInitializedPushClient() pattern: extract ensurePingOneMFAInitialized() helper and call it in the token handler and notification delegates instead of silently skipping when not yet initialized. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
passing geo diretcly to initialize
# Conflicts: # SampleApps/PingExample/PingExample.xcodeproj/project.pbxproj # SampleApps/PingExample/PingExample/ContentView.swift
george-bafaloukas-forgerock
left a comment
george-bafaloukas-forgerock
left a comment
There was a problem hiding this comment.
Thanks for this — it's a substantial, well-structured module and the doc comments across the public surface are genuinely excellent. 🙏 I've left inline comments below from a fairly pedantic pass focused on the new PingOneMFA SDK code (plus the README, tests, and the sample-app integration).
A few things I really liked: the Geo enum cleanly isolates PingOneGeo so consumers never import PingOneSDK, the same goes for not leaking NSError/PingOneSDKError, and getDeviceInfo()'s "neither data nor errors" branch correctly avoids hanging the continuation — exactly the defensive resume that prevents continuation-leak crashes. The AccountParserTests edge-case coverage is thorough too.
The handful of items I'd consider blocking before merge:
- Racy idempotency in
initialize(geo:)that contradicts its documented guarantee. - Background banner actions can be lost because
completionHandler()fires before the async approve/deny completes. - README example for
getDeviceInfo()doesn't compile (wrong return type). - Two tests that don't test what they claim — including a data race inside the "thread safety" test.
Everything else is lower-stakes polish. Really nice work overall — happy to talk through any of these. 🚀
# Conflicts: # SampleApps/PingExample/PingExample.xcodeproj/project.pbxproj
george-bafaloukas-forgerock
left a comment
george-bafaloukas-forgerock
left a comment
There was a problem hiding this comment.
Few more comments. Please confirm the lack of Podspec and maybe add a line in the Changelog
george-bafaloukas-forgerock
left a comment
george-bafaloukas-forgerock
left a comment
There was a problem hiding this comment.
A few small follow-ups from the latest push-flow changes — all minor/doc-level, nothing blocking. The redaction, Equatable, and the .dry sheet check all look good; thanks for those. 🙏 Left the specifics inline. (Also re: the podspec — saw the note that PingOneSDK is SPM-only, so disregard my earlier CocoaPods-parity comment; that's a reasonable constraint.)
george-bafaloukas-forgerock
left a comment
george-bafaloukas-forgerock
left a comment
There was a problem hiding this comment.
Looks good to me, please see the 1 new open comment. I would not merge this one yet, unless it has been cleared by QA
# Conflicts: # Package.resolved
WalkthroughAdds a new ChangesPingOne MFA module
PingExample integration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant APNS
participant AppDelegate
participant PingOneMFA
participant PingOneSDK
participant ContentView
APNS->>AppDelegate: deliver notification
AppDelegate->>PingOneMFA: process notification or action
PingOneMFA->>PingOneSDK: invoke notification API
PingOneSDK-->>PingOneMFA: notification result
PingOneMFA-->>AppDelegate: PushNotification?
AppDelegate->>ContentView: post ShowPingOneMFANotification
ContentView-->>PingOneMFA: approve or deny action
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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: 8
🧹 Nitpick comments (2)
PingOneMFA/PingOneMFATests/PingOneMFATests.swift (1)
482-493: 📐 Maintainability & Code Quality | 🔵 TrivialSleep-based polling instead of a proper async signal.
waitForConfigureCallCountpolls viaTask.sleepin a loop to detect whenconfigure()has started. SinceConfigureProbeis already an actor, this could instead expose an async method that suspends untilcallCountreaches the threshold (e.g., its own continuation), avoiding wall-clock delays in the test.As per path instructions, "Flag tests that use
sleepor wall-clock delays instead of proper async/await or expectations."🤖 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 `@PingOneMFA/PingOneMFATests/PingOneMFATests.swift` around lines 482 - 493, Replace the sleep-based polling in waitForConfigureCallCount with a proper async signal owned by ConfigureProbe, such as a continuation-based method that suspends until callCount reaches the requested minimum. Update the test helper to await that method and preserve the existing threshold result without wall-clock delays or retry limits.Source: Path instructions
SampleApps/PingExample/PingExample/PingOneMFAAccountsView.swift (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse a two-way
Bindingfor error alerts instead of.constant(). Three views use.alert(..., isPresented: .constant(viewModel.errorMessage != nil)), whilePingOneMFANotificationView.swift(same PR) correctly usesBinding(get:set:)for the equivalent alert. Functionally workable here, but inconsistent and slightly fragile if dismissal ever needs to originate from the system rather than the "OK" button.
SampleApps/PingExample/PingExample/PingOneMFAAccountsView.swift#L71-79: replace.constant(viewModel.errorMessage != nil)with aBinding(get: { viewModel.errorMessage != nil }, set: { if !$0 { viewModel.errorMessage = nil } }).SampleApps/PingExample/PingExample/PingOneMFAOtpView.swift#L54-62: same replacement.SampleApps/PingExample/PingExample/PingOneMFAPayloadView.swift#L44-52: same replacement.🤖 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 `@SampleApps/PingExample/PingExample/PingOneMFAAccountsView.swift` at line 1, Replace the constant alert bindings in PingOneMFAAccountsView, PingOneMFAOtpView, and PingOneMFAPayloadView with two-way Binding(get:set:) values that reflect whether errorMessage exists and clear viewModel.errorMessage when the alert is dismissed.
🤖 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 `@Package.swift`:
- Line 51: Add a root-level PingOneMFA.podspec corresponding to the PingOneMFA
library target declared in Package.swift. Match the repository’s existing
podspec conventions and configure the pod to expose the PingOneMFA target,
preserving distribution parity with SPM.
- Around line 308-317: Update the PingOneMFA target’s PingOneSDK product
dependency in the dependencies array to include a platform condition restricting
it to iOS via .when(platforms: [.iOS]); leave the target’s other configuration
unchanged.
In `@PingOneMFA/PingOneMFA/OtpCodeInfo.swift`:
- Around line 25-27: Update OtpCodeInfo.init(code:secondsRemaining:) to enforce
the documented non-negative validity invariant by clamping negative
secondsRemaining inputs to zero or explicitly rejecting them. Preserve the
provided value when it is zero or positive so consumers never receive an
impossible negative OTP state.
In `@PingOneMFA/PingOneMFA/PingOneMFAError.swift`:
- Around line 29-36: In PingOneMFAError, add /// documentation comments for the
public message property and errorDescription computed property, describing their
exposed error text. Leave internalErrorsList and the existing behavior
unchanged.
In `@PingOneMFA/PingOneMFATests/AccountParserTests.swift`:
- Around line 44-48: Replace direct accounts[0] and euAccounts[0] indexing in
the affected test assertions with try XCTUnwrap(accounts.first) and try
XCTUnwrap(euAccounts.first), reusing the unwrapped account values for subsequent
property checks. Preserve the existing count assertions and all expected field
values while ensuring failures produce normal XCTest failures instead of
out-of-bounds crashes.
In `@SampleApps/PingExample/PingExample/ContentView.swift`:
- Around line 248-249: Update the pingOneMFANotification state and its usage in
PingOneMFANotificationView to use the notification type exposed by the imported
PingOneMFA package, or adjust the view initializer to accept that package type;
ensure the related code at the notification handling block remains
type-compatible and the sample app compiles without relying on an undefined
local MFAPushNotification symbol.
In `@SampleApps/PingExample/PingExample/PingOneMFANotificationView.swift`:
- Around line 90-96: Update selectNumberSection so its number-matching buttons
are disabled or hidden whenever viewModel.isLoading is true, matching the
existing loading behavior in enterManuallySection. Ensure all number-selection
actions cannot trigger another approve() or approveNotification call while the
current request is in flight.
In `@SampleApps/PingExample/PingExample/PingOneMFAPayloadView.swift`:
- Around line 102-134: Update the copy action in copyButton to write the MFA
payload through UIPasteboard.setItems with pasteboard options that make it
local-only and set an expiration date, rather than assigning
UIPasteboard.general.string directly. Preserve the existing payload nil guard
and button behavior.
---
Nitpick comments:
In `@PingOneMFA/PingOneMFATests/PingOneMFATests.swift`:
- Around line 482-493: Replace the sleep-based polling in
waitForConfigureCallCount with a proper async signal owned by ConfigureProbe,
such as a continuation-based method that suspends until callCount reaches the
requested minimum. Update the test helper to await that method and preserve the
existing threshold result without wall-clock delays or retry limits.
In `@SampleApps/PingExample/PingExample/PingOneMFAAccountsView.swift`:
- Line 1: Replace the constant alert bindings in PingOneMFAAccountsView,
PingOneMFAOtpView, and PingOneMFAPayloadView with two-way Binding(get:set:)
values that reflect whether errorMessage exists and clear viewModel.errorMessage
when the alert is dismissed.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 13817764-ef4a-4f25-bd97-e0fe6ee23cfc
⛔ Files ignored due to path filters (2)
Package.resolvedis excluded by!**/Package.resolvedSampleApps/Ping.xcworkspace/xcshareddata/swiftpm/Package.resolvedis excluded by!**/Package.resolved
📒 Files selected for processing (35)
Package.swiftPingOneMFA/PingOneMFA.xcodeproj/project.pbxprojPingOneMFA/PingOneMFA.xcodeproj/xcshareddata/IDETemplateMacros.plistPingOneMFA/PingOneMFA.xcodeproj/xcshareddata/xcschemes/PingOneMFATests.xcschemePingOneMFA/PingOneMFA/AccountParser.swiftPingOneMFA/PingOneMFA/Geo.swiftPingOneMFA/PingOneMFA/OtpCodeInfo.swiftPingOneMFA/PingOneMFA/PingOneMFA.hPingOneMFA/PingOneMFA/PingOneMFA.swiftPingOneMFA/PingOneMFA/PingOneMFAError.swiftPingOneMFA/PingOneMFA/PingOneMFAInternalError.swiftPingOneMFA/PingOneMFA/PingOneMfaAccount.swiftPingOneMFA/PingOneMFA/PrivacyInfo.xcprivacyPingOneMFA/PingOneMFA/PushNotification.swiftPingOneMFA/PingOneMFA/PushType.swiftPingOneMFA/PingOneMFATests/AccountParserTests.swiftPingOneMFA/PingOneMFATests/MockPingOneMFA.swiftPingOneMFA/PingOneMFATests/PingOneMFAErrorTests.swiftPingOneMFA/PingOneMFATests/PingOneMFATests.swiftPingOneMFA/README.mdPingTestHost/PingTestHost.xcodeproj/xcshareddata/xcschemes/PingTestHost.xcschemePingTestHost/PingTestHost.xctestplanSampleApps/PingExample/PingExample.xcodeproj/project.pbxprojSampleApps/PingExample/PingExample/AppDelegate.swiftSampleApps/PingExample/PingExample/ConfigurationManager.swiftSampleApps/PingExample/PingExample/ContentView.swiftSampleApps/PingExample/PingExample/PingOneMFAAccountsView.swiftSampleApps/PingExample/PingExample/PingOneMFAAccountsViewModel.swiftSampleApps/PingExample/PingExample/PingOneMFANotificationView.swiftSampleApps/PingExample/PingExample/PingOneMFAOtpView.swiftSampleApps/PingExample/PingExample/PingOneMFAOtpViewModel.swiftSampleApps/PingExample/PingExample/PingOneMFAPayloadView.swiftSampleApps/PingExample/PingExample/PingOneMFAPayloadViewModel.swiftSampleApps/PingExample/PingExample/PingOneMFAScannerContainerView.swiftSampleApps/PingExample/PingExample/PingOneMFAScannerViewModel.swift
| .library(name: "PingOath", targets: ["PingOath"]), | ||
| .library(name: "PingPush", targets: ["PingPush"]), | ||
| .library(name: "PingAuthMigration", targets: ["PingAuthMigration"]), | ||
| .library(name: "PingOneMFA", targets: ["PingOneMFA"]), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i 'PingOneMFA.podspec'
rg -n "testTarget" Package.swiftRepository: ForgeRock/ping-ios-sdk
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -u
echo "== Package.swift relevant sections =="
sed -n '40,70p' Package.swift
sed -n '290,325p' Package.swift
echo "== Files named PingOneMFA / PingOneMFA.* =="
git ls-files | rg -i '(^|/)PingOneMFA(\.podspec|\.xcodeproj|/.*)$|PingTestHost|Package\.manifest|Package\.swif'
echo "== All podspec-related files =="
git ls-files | rg '\.podspec$|Package\.swift'
echo "== testTarget occurrences =="
rg -n "\.testTarget|testTarget" -S Package.swift . 2>/dev/null || trueRepository: ForgeRock/ping-ios-sdk
Length of output: 5372
Add PingOneMFA.podspec for distribution parity.
Package.swift declares the new PingOneMFA library/target, and PingOneMFATests exists, but there is no corresponding root-level PingOneMFA.podspec. CocoaPods distribution of this SDK target will remain out of sync with SPM without it.
🤖 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 `@Package.swift` at line 51, Add a root-level PingOneMFA.podspec corresponding
to the PingOneMFA library target declared in Package.swift. Match the
repository’s existing podspec conventions and configure the pod to expose the
PingOneMFA target, preserving distribution parity with SPM.
Source: Path instructions
| .target( | ||
| name: "PingOneMFA", | ||
| dependencies: [ | ||
| .product(name: "PingOneSDK", package: "pingone-mobile-sdk-ios") | ||
| ], | ||
| path: "PingOneMFA/PingOneMFA", | ||
| exclude: ["PingOneMFA.h"], | ||
| resources: [.copy("PrivacyInfo.xcprivacy")] | ||
| ), | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check top-level platforms declaration and whether other iOS-only vendor deps are platform-guarded
rg -n "platforms:" -A5 Package.swift
rg -n "\.when\(platforms:" Package.swiftRepository: ForgeRock/ping-ios-sdk
Length of output: 2100
🌐 Web query:
PingOne mobile SDK pingone-mobile-sdk-ios macOS support Package.swift
💡 Result:
The PingOne Mobile SDK for iOS does not currently provide official support for macOS. According to the official documentation and the repository's configuration, the SDK is explicitly designed for iOS and requires a minimum of iOS 15.0 and Xcode 14 or later [1][2][3]. The project's Package.swift file defines the SDK as a library intended for iOS integration [4]. While the SDK can be installed via the Swift Package Manager, the compatibility guidelines provided by Ping Identity consistently specify only iOS as the target platform [1][3]. There is no mention of macOS, iPadOS, or other Apple platforms in the official support requirements or the package definition [1][4][3].
Citations:
- 1: https://github.com/pingidentity/pingone-mobile-sdk-ios
- 2: https://developer.pingidentity.com/pingone-api/native-sdks/pingone-mfa-mobile-sdks/pingone-mobile-sdk-for-ios.html
- 3: https://github.com/pingidentity/pingone-mobile-sdk-ios/blob/main/README.md
- 4: https://github.com/pingidentity/pingone-mobile-sdk-ios/blob/main/Package.swift
Guard PingOneSDK to iOS for the PingOneMFA dependency.
Package.swift declares macOS 13 support, but pingone-mobile-sdk-ios is iOS-only with no macOS support. Add .when(platforms: [.iOS]) to the PingOneSDK product dependency here.
🤖 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 `@Package.swift` around lines 308 - 317, Update the PingOneMFA target’s
PingOneSDK product dependency in the dependencies array to include a platform
condition restricting it to iOS via .when(platforms: [.iOS]); leave the target’s
other configuration unchanged.
Source: Coding guidelines
| public init(code: String, secondsRemaining: Int) { | ||
| self.code = code | ||
| self.secondsRemaining = secondsRemaining |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Enforce the non-negative OTP validity invariant.
The public initializer accepts negative secondsRemaining values even though the API documents expired codes as 0. Clamp negative inputs to zero or reject them explicitly; otherwise consumers can receive an impossible OTP state.
🤖 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 `@PingOneMFA/PingOneMFA/OtpCodeInfo.swift` around lines 25 - 27, Update
OtpCodeInfo.init(code:secondsRemaining:) to enforce the documented non-negative
validity invariant by clamping negative secondsRemaining inputs to zero or
explicitly rejecting them. Preserve the provided value when it is zero or
positive so consumers never receive an impossible negative OTP state.
| public struct PingOneMFAError: Error, LocalizedError, Sendable { | ||
| public let message: String | ||
|
|
||
| /// Structured list of individual SDK errors, or `nil` when the failure did not originate | ||
| /// from the native SDK (e.g. an unexpected exception). | ||
| public let internalErrorsList: [PingOneMFAInternalError]? | ||
|
|
||
| public var errorDescription: String? { message } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Document all public error members.
Add /// documentation for message and errorDescription; both are part of the public SDK surface and currently lack member-level documentation.
🤖 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 `@PingOneMFA/PingOneMFA/PingOneMFAError.swift` around lines 29 - 36, In
PingOneMFAError, add /// documentation comments for the public message property
and errorDescription computed property, describing their exposed error text.
Leave internalErrorsList and the existing behavior unchanged.
Source: Path instructions
| XCTAssertEqual(accounts.count, 1) | ||
| XCTAssertEqual(accounts[0].region, "NorthAmerica") | ||
| XCTAssertEqual(accounts[0].id, "c845dcd4-9696-45ce-b1b8-8797da941538") | ||
| XCTAssertEqual(accounts[0].deviceId, "05280532-42b0-4d29-93f2-9f2ed7acefc1") | ||
| XCTAssertEqual(accounts[0].environmentId, "803ca4d4-cd92-4cb8-9dd1-6fe68de0a5f0") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid unchecked array indexing after assertions.
If a preceding count assertion fails, accounts[0] or euAccounts[0] can crash the test. Use try XCTUnwrap(accounts.first) / try XCTUnwrap(euAccounts.first) so the test reports a normal XCTest failure.
Also applies to: 83-90, 127-129
🤖 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 `@PingOneMFA/PingOneMFATests/AccountParserTests.swift` around lines 44 - 48,
Replace direct accounts[0] and euAccounts[0] indexing in the affected test
assertions with try XCTUnwrap(accounts.first) and try
XCTUnwrap(euAccounts.first), reusing the unwrapped account values for subsequent
property checks. Preserve the existing count assertions and all expected field
values while ensuring failures produce normal XCTest failures instead of
out-of-bounds crashes.
Source: Path instructions
| @State private var pingOneMFANotification: MFAPushNotification? = nil | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether MFAPushNotification is a typealias/alias of PushNotification.
rg -n 'MFAPushNotification' --type=swift -C2
rg -n 'typealias\s+MFAPushNotification|struct\s+PushNotification|class\s+PushNotification|typealias\s+PushNotification' --type=swiftRepository: ForgeRock/ping-ios-sdk
Length of output: 295
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Swift files containing PingOneMFANotificationView =="
rg -n 'PingOneMFANotificationView|processRemoteNotification|PushNotification|MFAPushNotification|import PingOneMFA' --type=swift -C 3
echo
echo "== Locate candidate files =="
fd -e swift -i 'PingOneMFANotificationView|SampleApps/PingExample/PingExample/ContentView.swift|AppDelegate.swift' .Repository: ForgeRock/ping-ios-sdk
Length of output: 383
Ensure MFA notification type compatibility.
pingOneMFANotification is received as MFAPushNotification and passed to PingOneMFANotificationView, but this module does not define MFAPushNotification or alias it to PushNotification. Gate this behind the PingOneMFA package API (or adjust the view initializer) so the sample app compiles against the dependency it imports. Also applies to lines 281-288.
🤖 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 `@SampleApps/PingExample/PingExample/ContentView.swift` around lines 248 - 249,
Update the pingOneMFANotification state and its usage in
PingOneMFANotificationView to use the notification type exposed by the imported
PingOneMFA package, or adjust the view initializer to accept that package type;
ensure the related code at the notification handling block remains
type-compatible and the sample app compiles without relying on an undefined
local MFAPushNotification symbol.
| if viewModel.notification.pushType == .challenge { | ||
| if !viewModel.notification.getNumbersChallenge.isEmpty { | ||
| selectNumberSection | ||
| } else { | ||
| enterManuallySection | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Number-matching buttons stay tappable during isLoading, unlike the manual-entry path.
enterManuallySection's confirm button disappears while viewModel.isLoading (line 231), but selectNumberSection's number buttons have no such guard — a user can tap another number (or the same one twice) while an approve() call is in flight, firing concurrent approveNotification calls against the SDK.
🔒 Suggested fix
let options = viewModel.notification.getNumbersChallenge
if options.isEmpty {
Text("No options available")
.font(.system(size: 14))
.foregroundColor(.red)
} else {
HStack(spacing: 16) {
ForEach(options, id: \.self) { number in
Button {
viewModel.approve(numberChallenge: number)
} label: {
Text("\(number)")
.font(.system(size: 24, weight: .bold))
.foregroundColor(.themeButtonBackground)
.frame(width: 80, height: 80)
.background(Color.clear)
.overlay(
Circle()
.stroke(Color.themeButtonBackground, lineWidth: 2)
)
}
+ .disabled(viewModel.isLoading)
}
}
}Also applies to: 182-214
🤖 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 `@SampleApps/PingExample/PingExample/PingOneMFANotificationView.swift` around
lines 90 - 96, Update selectNumberSection so its number-matching buttons are
disabled or hidden whenever viewModel.isLoading is true, matching the existing
loading behavior in enterManuallySection. Ensure all number-selection actions
cannot trigger another approve() or approveNotification call while the current
request is in flight.
| private var copyButton: some View { | ||
| Button { | ||
| if let payload = viewModel.payload { | ||
| UIPasteboard.general.string = payload | ||
| } | ||
| } label: { | ||
| HStack(spacing: 8) { | ||
| Image(systemName: "doc.on.doc") | ||
| .font(.system(size: 16)) | ||
| Text("Copy") | ||
| .font(.system(size: 16, weight: .medium)) | ||
| } | ||
| .foregroundColor(.white) | ||
| .frame(maxWidth: .infinity) | ||
| .padding(.vertical, 14) | ||
| .background( | ||
| viewModel.payload != nil | ||
| ? LinearGradient( | ||
| colors: [.themeButtonBackground, Color(red: 0.6, green: 0.1, blue: 0.1)], | ||
| startPoint: .topLeading, | ||
| endPoint: .bottomTrailing | ||
| ) | ||
| : LinearGradient( | ||
| colors: [Color.gray, Color.gray], | ||
| startPoint: .topLeading, | ||
| endPoint: .bottomTrailing | ||
| ) | ||
| ) | ||
| .clipShape(RoundedRectangle(cornerRadius: 12)) | ||
| } | ||
| .disabled(viewModel.payload == nil) | ||
| .accessibilityIdentifier("copyPayloadButton") | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Copying MFA payload to the general pasteboard.
copyButton writes the mobile payload — generated for authentication/registration — to UIPasteboard.general, which persists and is readable by other apps. Consider setItems with .expirationDate/.localOnly to limit exposure of what may be sensitive authentication material.
🛡️ Suggested fix
if let payload = viewModel.payload {
- UIPasteboard.general.string = payload
+ UIPasteboard.general.setItems(
+ [[UIPasteboard.typeAutomatic: payload]],
+ options: [.expirationDate: Date().addingTimeInterval(60), .localOnly: true]
+ )
}As per path instructions, sample app code should still flag security issues involving real tokens.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private var copyButton: some View { | |
| Button { | |
| if let payload = viewModel.payload { | |
| UIPasteboard.general.string = payload | |
| } | |
| } label: { | |
| HStack(spacing: 8) { | |
| Image(systemName: "doc.on.doc") | |
| .font(.system(size: 16)) | |
| Text("Copy") | |
| .font(.system(size: 16, weight: .medium)) | |
| } | |
| .foregroundColor(.white) | |
| .frame(maxWidth: .infinity) | |
| .padding(.vertical, 14) | |
| .background( | |
| viewModel.payload != nil | |
| ? LinearGradient( | |
| colors: [.themeButtonBackground, Color(red: 0.6, green: 0.1, blue: 0.1)], | |
| startPoint: .topLeading, | |
| endPoint: .bottomTrailing | |
| ) | |
| : LinearGradient( | |
| colors: [Color.gray, Color.gray], | |
| startPoint: .topLeading, | |
| endPoint: .bottomTrailing | |
| ) | |
| ) | |
| .clipShape(RoundedRectangle(cornerRadius: 12)) | |
| } | |
| .disabled(viewModel.payload == nil) | |
| .accessibilityIdentifier("copyPayloadButton") | |
| } | |
| private var copyButton: some View { | |
| Button { | |
| if let payload = viewModel.payload { | |
| UIPasteboard.general.setItems( | |
| [[UIPasteboard.typeAutomatic: payload]], | |
| options: [.expirationDate: Date().addingTimeInterval(60), .localOnly: true] | |
| ) | |
| } | |
| } label: { | |
| HStack(spacing: 8) { | |
| Image(systemName: "doc.on.doc") | |
| .font(.system(size: 16)) | |
| Text("Copy") | |
| .font(.system(size: 16, weight: .medium)) | |
| } | |
| .foregroundColor(.white) | |
| .frame(maxWidth: .infinity) | |
| .padding(.vertical, 14) | |
| .background( | |
| viewModel.payload != nil | |
| ? LinearGradient( | |
| colors: [.themeButtonBackground, Color(red: 0.6, green: 0.1, blue: 0.1)], | |
| startPoint: .topLeading, | |
| endPoint: .bottomTrailing | |
| ) | |
| : LinearGradient( | |
| colors: [Color.gray, Color.gray], | |
| startPoint: .topLeading, | |
| endPoint: .bottomTrailing | |
| ) | |
| ) | |
| .clipShape(RoundedRectangle(cornerRadius: 12)) | |
| } | |
| .disabled(viewModel.payload == nil) | |
| .accessibilityIdentifier("copyPayloadButton") | |
| } |
🤖 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 `@SampleApps/PingExample/PingExample/PingOneMFAPayloadView.swift` around lines
102 - 134, Update the copy action in copyButton to write the MFA payload through
UIPasteboard.setItems with pasteboard options that make it local-only and set an
expiration date, rather than assigning UIPasteboard.general.string directly.
Preserve the existing payload nil guard and button behavior.
Source: Path instructions
4 participants
P14C-82521
JIRA "Add PingOneMFA module to the orchestratio SDK on iOS"
Description
https://pingidentity.atlassian.net/browse/P14C-86659
Package.swift / Package.resolved updates — adds PingOneMFA as a new library target and pulls in the PingOneSDK binary dependency.
Testing
Set-up native application in PingOne environment
Pairing: tap QR Code Registration → scan a PingOne pairing QR → success dialog → accounts list shows the paired account
OTP: tap One-Time Passcode → code displays with countdown; refreshes automatically at zero
Payload: tap Mobile Payload → payload shown; Copy button writes to clipboard
Push (foreground): trigger a push from PingOne while app is open → approval screen appears; approve/deny shows success dialog
Push (background): trigger a push while app is backgrounded → system banner with Allow/Deny buttons → tapping either completes the request
Cancellation: approve on a second device while the approval screen is open → screen dismisses automatically
Checklist:
Summary by CodeRabbit