Skip to content

Feature/mfa integration#161

Open
gioraping wants to merge 45 commits into
ForgeRock:developfrom
gioraping:feature/mfa-integration
Open

Feature/mfa integration#161
gioraping wants to merge 45 commits into
ForgeRock:developfrom
gioraping:feature/mfa-integration

Conversation

@gioraping

@gioraping gioraping commented May 27, 2026

Copy link
Copy Markdown

P14C-82521

JIRA "Add PingOneMFA module to the orchestratio SDK on iOS"

Description

  • New PingOneMFA module — a clean async/await wrapper around the binary PingOneSDK (PingOneSDK XCFramework). Bridges all callback-based PingOneSDK APIs to async throws functions via withCheckedThrowingContinuation, using a @globalActor (PingOneMFAActor) for thread-safe state. Exposed surface: initialize(geo:), setDeviceToken(_:), pair(pairingKey:), getDeviceInfo(), getOneTimePasscode(), processRemoteNotification(userInfo:), processRemoteNotificationAction(identifier:authenticationMethod:userInfo:), generateMobilePayload(), getNotificationCategories().
  • Actionable push notifications — registers UNNotificationCategory objects from PingOneSDK so Approve/Deny banner actions work without the app being foregrounded; routes UNUserNotificationCenterDelegate actions through processRemoteNotificationAction.
  • PingExample sample app integration — adds a full MFA section: QR-code scanner for device pairing, paired accounts list, live-countdown OTP screen, push notification modal (approve/deny with number-matching support), and mobile payload view.
    https://pingidentity.atlassian.net/browse/P14C-86659
  • Unit tests — AccountParserTests, PingOneMFATests, PingOneMFAErrorTests, and a MockPingOneMFA covering initialization, pairing, token registration, OTP, push processing, and error bridging.
    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:

  • I ran all unit tests and they pass
  • I added test case coverage for my changes

gioraping and others added 23 commits May 25, 2026 17:55
… 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
@gioraping gioraping marked this pull request as ready for review June 1, 2026 15:19

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🚀

Comment thread PingOneMFA/PingOneMFA/PingOneMFA.swift Outdated
Comment thread PingOneMFA/PingOneMFA/PingOneMFA.swift Outdated
Comment thread PingOneMFA/PingOneMFA/PingOneMFA.swift Outdated
Comment thread PingOneMFA/PingOneMFA/PingOneMFA.swift Outdated
Comment thread PingOneMFA/PingOneMFA/AccountParser.swift Outdated
Comment thread SampleApps/PingExample/PingExample/AppDelegate.swift Outdated
Comment thread SampleApps/PingExample/PingExample/AppDelegate.swift Outdated
Comment thread SampleApps/PingExample/PingExample/AppDelegate.swift Outdated
Comment thread SampleApps/PingExample/PingExample/PingOneMFAOtpViewModel.swift Outdated
Comment thread SampleApps/PingExample/PingExample.xcodeproj/project.pbxproj Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few more comments. Please confirm the lack of Podspec and maybe add a line in the Changelog

Comment thread PingOneMFA/PingOneMFA/PushType.swift
Comment thread PingOneMFA/README.md
Comment thread PingOneMFA/PingOneMFA/PingOneMFAError.swift Outdated
Comment thread PingOneMFA/PingOneMFA/PingOneMFAInternalError.swift Outdated
Comment thread SampleApps/PingExample/PingExample/AppDelegate.swift Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Comment thread PingOneMFA/PingOneMFA/PingOneMfaAccount.swift Outdated
Comment thread PingOneMFA/PingOneMFA/PingOneMFA.swift Outdated
Comment thread PingOneMFA/README.md Outdated
Comment thread PingOneMFA/README.md Outdated
Comment thread SampleApps/PingExample/PingExample/AppDelegate.swift
/// The interaction model required by this notification.
///
/// Use this to determine the UI flow:
/// - `.silent` — no user interaction needed; the authentication completes silently.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be .dry right?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants