Feature/pingonemfa integration#205
Open
EvgeniyMish wants to merge 14 commits into
Open
Conversation
…ingOneMFA SDK into the orchestration SDK. * initial commit POC PingOneMFA Android SDK is wrapped in the DV SDK, created new sample app * Introduce the first phase of a proof of concept for integrating the PingOneMFA SDK into the orchestration SDK. This change adds: - A pingonemfa module that wraps the PingOneMFA SDK APIs required for core flows: - Account pairing - Retrieval of paired accounts - Push authentication - OTP authentication - A PingOneMFApp: sample application demonstrating the supported flows In this initial phase, all flows operate against the PingOne MFA SSO policy. * Introduce the first phase of a proof of concept for integrating the PingOneMFA SDK into the orchestration SDK. This change adds: - A pingonemfa module that wraps the PingOneMFA SDK APIs required for core flows: - Account pairing - Retrieval of paired accounts - Push authentication - OTP authentication - A PingOneMFApp: sample application demonstrating the supported flows In this initial phase, all flows operate against the PingOne MFA SSO policy. * Introduce the first phase of a proof of concept for integrating the PingOneMFA SDK into the orchestration SDK. This change adds: - A pingonemfa module that wraps the PingOneMFA SDK APIs required for core flows: - Account pairing - Retrieval of paired accounts - Push authentication - OTP authentication - A PingOneMFApp: sample application demonstrating the supported flows In this initial phase, all flows operate against the PingOne MFA SSO policy. * added comments and backward compatibility support * added comments and backward compatibility support --------- Signed-off-by: Evgeniy Mishustin <33718049+EvgeniyMish@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughAdds a new pingonemfa Android library (coroutine wrapper over PingOne MFA SDK) with push approval/background service, models, parsers, and tests; integrates it into the sample app with startup initialization, navigation, ViewModel, Compose screens, QR scanner, notification helpers, and push-routing. ChangesPingOne MFA Library Module
PingSampleApp Integration
Sequence Diagram(s)sequenceDiagram
participant App as PingSampleApp
participant FCM as Firebase (FCM)
participant PushSvc as PushNotificationService
participant PingLib as PingOneMFA (library)
participant UI as PingOneNotificationHelper / Activity
App->>FCM: device registers token
FCM->>App: remote message (PingOne data)
App->>PushSvc: onMessageReceived(remoteMessage)
PushSvc->>PingLib: processRemoteNotification(remoteMessage)
PingLib-->>PushSvc: PushNotification result
alt cancel
PushSvc->>UI: remove notification via NotificationManager & broadcast cancel
else present
PushSvc->>UI: showPushNotification or start Activity
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
… into develop # Conflicts: # gradle/libs.versions.toml # pingonemfa/.gitignore # samples/pingonemfapp/build.gradle.kts # samples/pingonemfapp/src/main/AndroidManifest.xml # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/DiagnosticLogger.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/UserPreferences.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/BiometricPromptActivity.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/NotificationHelper.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/service/PushNotificationService.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AboutScreen.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/DiagnosticLogsScreen.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/LoginScreen.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/QrScannerScreen.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/SettingsScreen.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/AccountAvatar.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/BackNavigationTopAppBar.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/EmptyStateMessage.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/ErrorAlertDialog.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/LoadingIndicator.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/SettingItem.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/theme/Color.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/theme/Theme.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/theme/Type.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/util/NavigationAnimations.kt # samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/util/QrCodeAnalyzer.kt # samples/pingonemfapp/src/main/res/drawable/ic_check.xml # samples/pingonemfapp/src/main/res/drawable/ic_close.xml # samples/pingonemfapp/src/main/res/drawable/ic_fingerprint.xml # samples/pingonemfapp/src/main/res/drawable/ic_launcher_foreground.xml # samples/pingonemfapp/src/main/res/drawable/ic_notification.xml # samples/pingonemfapp/src/main/res/drawable/ping_logo.xml # samples/pingonemfapp/src/main/res/mipmap-anydpi-v26/ic_launcher.xml # samples/pingonemfapp/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xml # samples/pingonemfapp/src/main/res/values/ic_launcher_background.xml # samples/pingonemfapp/src/main/res/values/strings.xml # samples/pingonemfapp/src/main/res/values/themes.xml # settings.gradle.kts
EvgeniyMish
had a problem deploying
to
github-pages
GitHub Actions
Failure
…sh/ping-android-sdk-fork into feature/pingonemfa-integration
EvgeniyMish
had a problem deploying
to
github-pages
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
protect/src/main/kotlin/com/pingidentity/protect/Protect.kt (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the Kotlin file copyright year range.
This file was modified in 2026, so the header should be updated to a range (for example,
2025-2026) instead of a single year.Based on learnings: existing modified Kotlin/Java files should use a year range from original creation year through the current year.
🤖 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 `@protect/src/main/kotlin/com/pingidentity/protect/Protect.kt` at line 2, Update the file header in Protect.kt to use a year range instead of a single year: change the copyright year from "2025" to "2025-2026" in the top-of-file comment so the header reflects the file was modified in 2026.
🧹 Nitpick comments (25)
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePushNotificationScreen.kt (1)
133-139: 💤 Low valueHardcoded
contentDescriptionnot localized.This PR extracts UI strings to
strings.xml, but the back-button description is a literal"Back". Screen readers will read an unlocalized string. Consider routing it through a string resource for consistency.♻️ Suggested change
Icon( imageVector = Icons.AutoMirrored.Filled.ArrowBack, - contentDescription = "Back", + contentDescription = stringResource(R.string.back), )🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePushNotificationScreen.kt` around lines 133 - 139, The hardcoded contentDescription in PingOnePushNotificationScreen's navigationIcon (inside the IconButton/Icon block) should be replaced with a localized string resource; update the Icon's contentDescription to use stringResource(R.string.some_back_label) (or a descriptive key like R.string.back_navigation) and add the corresponding entry to strings.xml (and localized variants), ensuring accessibility uses the resource rather than the literal "Back".samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/util/PingOneMFAQrCodeAnalyzer.kt (1)
52-70: ⚡ Quick winThrottle is ineffective until a code is detected.
lastAnalyzedTimestampis only updated inside the success branch (Line 65). Until a valid QR is found, the elapsed-time check always passes, so ML Kitprocess()runs on essentially every frame — the intended 1s throttle never applies during the common "scanning, nothing detected yet" case, increasing CPU/battery usage. Stamp the timestamp when a frame is actually submitted for processing instead.♻️ Suggested fix
if (currentTimestamp - lastAnalyzedTimestamp >= TimeUnit.SECONDS.toMillis(1)) { + lastAnalyzedTimestamp = currentTimestamp imageProxy.image?.let { image -> val inputImage = InputImage.fromMediaImage(image, imageProxy.imageInfo.rotationDegrees) scanner.process(inputImage) .addOnSuccessListener { barcodes -> val found = barcodes.find { barcode -> barcode.format == Barcode.FORMAT_QR_CODE && barcode.rawValue?.matchesPingOnePairingKeyScheme() == true } found?.rawValue?.let { pairingKey -> - lastAnalyzedTimestamp = currentTimestamp onQrCodeDetected(pairingKey) } }🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/util/PingOneMFAQrCodeAnalyzer.kt` around lines 52 - 70, The throttle is only updated when a QR is detected because lastAnalyzedTimestamp is set inside the success branch; instead, update lastAnalyzedTimestamp when you submit a frame for processing so frames are rate-limited even if no QR is found. In the block where you check TimeUnit.SECONDS.toMillis(1) and before calling scanner.process(inputImage) (referencing lastAnalyzedTimestamp, currentTimestamp, scanner.process, inputImage, imageProxy, and onQrCodeDetected), assign lastAnalyzedTimestamp = currentTimestamp immediately after passing the elapsed-time check and before invoking scanner.process, leaving the existing success/failure handlers otherwise unchanged.samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authenticator/service/PushNotificationService.kt (1)
102-133: ⚡ Quick winMake PingOne and generic push routing mutually exclusive.
For a PingOne MFA message,
remoteMessage.datais non-empty, so after the PingOne branch (Lines 103-117) the generic block at Lines 119-133 also runs and feeds the PingOne payload intopushClient?.processNotification(...). This double-processes the message and will likely emit a misleading"Error processing notification"diagnostic for every PingOne push. Route to the generic path only when the PingOne marker is absent.♻️ Proposed guard
// Check if the message received from PingOne MFA Push by looking for expected key if (remoteMessage.data.containsKey("PingOne")) { diagnosticLogger.d("Received PingOne MFA Push message") scope.launch { // Process the notification using PingOneMFA's processRemoteNotification method, which handles decryption and validation PingOneMFA.processRemoteNotification(remoteMessage) .onSuccess { diagnosticLogger.d("Successfully collected PingOne MFA push notification") // Handle the notification (this will display a system notification or full-screen notification based on app state) handlePingOneNotification(it) } .onFailure { error -> diagnosticLogger.e("Failed to collect PingOne MFA push notification: ${error.message}") } } + return } // Handle the message data payload if (remoteMessage.data.isNotEmpty()) {🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authenticator/service/PushNotificationService.kt` around lines 102 - 133, The PingOne branch and the generic branch both run for PingOne pushes because remoteMessage.data is non-empty; change the control flow so they are mutually exclusive by checking for the PingOne marker before running the generic path (e.g., use an else branch or return after invoking PingOneMFA.processRemoteNotification), ensuring that when PingOneMFA.processRemoteNotification(remoteMessage) (and handlePingOneNotification(...)) is invoked you do not also call pushClient?.processNotification(remoteMessage.data ...) or handleNotification(...).samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AboutScreen.kt (1)
117-121: ⚡ Quick winUse
fillMaxWidth()instead offillMaxSize()here.This
Cardsits inside aColumnwithverticalScroll, so the height constraint is unbounded andfillMaxHeight(part offillMaxSize) is a no-op. The intent is to fill width.♻️ Proposed change
Card( modifier = Modifier - .fillMaxSize() + .fillMaxWidth() .padding(horizontal = 8.dp) ) {🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AboutScreen.kt` around lines 117 - 121, In the AboutScreen Card declaration replace the Modifier.fillMaxSize() call with Modifier.fillMaxWidth() because the Card is inside a vertically scrolling Column (unbounded height) so fillMaxHeight is a no-op; update the modifier on the Card in AboutScreen (Card { ... } modifier usage) to use fillMaxWidth(). Ensure you only change the modifier chain (keep .padding(horizontal = 8.dp)) and remove the fillMaxSize reference.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/NotificationResponseScreen.kt (1)
285-331: ⚡ Quick winRemove the large commented-out block (and the smaller ones at Lines 119-121, 149).
This dead/commented code (the locked-credential branch and
credential/imageURLsnippets) adds noise and references symbols not present in this module. Drop it; version control preserves the history if needed later.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/NotificationResponseScreen.kt` around lines 285 - 331, Delete the large commented-out locked-credential UI block in NotificationResponseScreen.kt along with the smaller commented credential/imageURL snippets; remove references to symbols inside the comments (credential, isLocked, lockingPolicy, BiometricAvailablePolicy, DeviceTamperingPolicy, lockMessage, imageURL) so the file contains only active code—keep no traces of these commented branches since VCS preserves history.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AccountsScreen.kt (5)
162-162: ⚡ Quick winExtract hardcoded strings to string resources.
Lines 162 ("Settings") and 267 ("No accounts added yet") contain hardcoded strings that should be extracted to
strings.xmlfor consistency and localization support. Line 179 correctly usesstringResource(id = R.string.menu_about)for similar text.🌐 Proposed fix
Text("Settings") + Text(stringResource(id = R.string.menu_settings))EmptyStateMessage( - title = "No accounts added yet", + title = stringResource(id = R.string.accounts_empty_state_title), subtitle = stringResource(id = R.string.accounts_empty_state_subtitle)Note:
R.string.menu_settingsandR.string.accounts_empty_state_titlealready exist instrings.xml.Also applies to: 267-267
🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AccountsScreen.kt` at line 162, Replace hardcoded UI strings in the AccountsScreen composable by using string resources: change the Text("Settings") instance to use stringResource(id = R.string.menu_settings) and change the Text("No accounts added yet") instance to use stringResource(id = R.string.accounts_empty_state_title); ensure you import androidx.compose.ui.res.stringResource and reference the existing R.string IDs (menu_settings, accounts_empty_state_title) in the AccountsScreen function so all displayed text comes from strings.xml.
193-234: ⚡ Quick winClarify or remove redundant secondary FAB animation.
The
showFabMenustate andAnimatedVisibilitywrapper (lines 194-218) suggest a multi-action FAB menu, but both the animated secondary FAB (line 207:onScanQrCode()) and the primary FAB (line 224:onScanQrCode()) perform the same action. Additionally,showFabMenuis set tofalseimmediately when clicked (line 206, 223), so the secondary FAB never actually animates in. This setup appears incomplete or unnecessary.🧹 Proposed fix
If only one action is needed, remove the animated secondary FAB:
floatingActionButton = { - Column(horizontalAlignment = Alignment.End) { - AnimatedVisibility( - visible = showFabMenu, - enter = fadeIn(), - exit = fadeOut() - ) { - Column( - horizontalAlignment = Alignment.End, - verticalArrangement = Arrangement.spacedBy(8.dp) - ) { - // Scan QR code option - FloatingActionButton( - onClick = { - showFabMenu = false - onScanQrCode() - }, - modifier = Modifier.size(48.dp), - containerColor = MaterialTheme.colorScheme.secondaryContainer - ) { - Icon( - imageVector = Icons.Default.QrCodeScanner, - contentDescription = "Scan QR Code" - ) - } - } - } - - // Primary FAB - FloatingActionButton( + FloatingActionButton( onClick = { - showFabMenu = false onScanQrCode() }, containerColor = MaterialTheme.colorScheme.primaryContainer, contentColor = MaterialTheme.colorScheme.onPrimaryContainer ) { Icon( imageVector = Icons.Default.Add, contentDescription = stringResource(id = R.string.content_description_add_account) ) } - } },Also remove the unused
showFabMenustate declaration (line 101).🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AccountsScreen.kt` around lines 193 - 234, The AnimatedVisibility block and the showFabMenu state are redundant because both the secondary and primary FloatingActionButton call onScanQrCode() and showFabMenu is immediately set to false, so remove the entire AnimatedVisibility column (the secondary FloatingActionButton that calls onScanQrCode()) and delete the showFabMenu state declaration; keep only the primary FloatingActionButton (the one using Icons.Default.Add and stringResource(id = R.string.content_description_add_account)) so a single FAB performs the scan action.
280-285: 💤 Low valueFix key generation comment to match implementation.
The comment claims the key uses "issuer, account name, and all credential IDs" but the actual implementation uses
userIdanddeviceId. Update the comment to reflect the actual key composition.📝 Proposed fix
key = { account -> - // Create a unique key using issuer, account name, and all credential IDs + // Create a unique key using user ID and device ID val userId = account.id val deviceId = account.deviceId🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AccountsScreen.kt` around lines 280 - 285, Update the misleading comment above the key lambda so it accurately describes the implemented key composition: replace the text claiming it uses "issuer, account name, and all credential IDs" with a concise note that the key is built from account.id and account.deviceId (variables userId and deviceId) used in the key = { account -> ... } block.
254-264: ⚡ Quick winConsolidate redundant loading state checks.
Both
isInitialLoadingandisLoadingPingOneAccountsdisplay identicalLoadingIndicatormessages. This duplication can be simplified.♻️ Proposed refactor
when { - uiState.isInitialLoading -> { - LoadingIndicator( - message = stringResource(id = R.string.loading_accounts) - ) - } - uiState.isLoadingPingOneAccounts -> { + uiState.isInitialLoading || uiState.isLoadingPingOneAccounts -> { LoadingIndicator( message = stringResource(id = R.string.loading_accounts) )🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AccountsScreen.kt` around lines 254 - 264, The two identical branches in the when block of AccountsScreen (checking uiState.isInitialLoading and uiState.isLoadingPingOneAccounts) should be consolidated into a single branch that checks the combined condition (e.g., uiState.isInitialLoading || uiState.isLoadingPingOneAccounts) and renders LoadingIndicator once; update the when in the AccountsScreen composable to replace the duplicate branches with one branch using the combined predicate so LoadingIndicator (message = stringResource(id = R.string.loading_accounts)) is only declared once.
300-304: ⚡ Quick winReplace non-null assertion with safe error handling.
Using
!!onuiState.error(line 302) can crash the app if the state is unexpectedly null. The null-check on line 300 guards this, but using aletblock is safer and more idiomatic.🛡️ Proposed fix
// Error handling - if (uiState.error != null) { - ErrorAlertDialog( - errorMessage = uiState.error!!, - onDismiss = { viewModel.clearError() } - ) - } + uiState.error?.let { errorMessage -> + ErrorAlertDialog( + errorMessage = errorMessage, + onDismiss = { viewModel.clearError() } + ) + }🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AccountsScreen.kt` around lines 300 - 304, The current AccountsScreen uses a non-null assertion uiState.error!! when calling ErrorAlertDialog which risks an NPE; replace this pattern by using a safe let scope on uiState.error (e.g., uiState.error?.let { error -> ErrorAlertDialog(errorMessage = error, onDismiss = { viewModel.clearError() }) }) so ErrorAlertDialog only receives a non-null String and you can remove the explicit null-check + !!; keep the onDismiss calling viewModel.clearError() as-is.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/OtpScreen.kt (1)
44-48: 💤 Low valueExtract the loading message to
strings.xml.The PR objective notes UI strings are externalized; this literal should use
stringResource(...)for consistency and localization.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/OtpScreen.kt` around lines 44 - 48, The hard-coded loading text in OtpScreen's state.isLoading branch should be moved to resources: add a new string entry (e.g., name="loading_otp_code") in strings.xml, then replace the literal "Loading OTP code..." in the LoadingIndicator call with stringResource(R.string.loading_otp_code). Update imports to include androidx.compose.ui.res.stringResource if needed and ensure the change is made inside the OtpScreen composable where state.isLoading is handled.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/ExpiringOtpCode.kt (1)
42-42: 💤 Low valueStale comment.
The comment says "Green -> Yellow -> Red", but the interpolation is
PingLightBlue→PingRed. Update or remove to avoid confusion.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/ExpiringOtpCode.kt` at line 42, The inline comment above the color interpolation in ExpiringOtpCode.kt is stale ("Green -> Yellow -> Red") and should be updated or removed to match the actual interpolation (PingLightBlue → PingRed); locate the comment near the interpolation logic in the ExpiringOtpCode component and either replace the text with an accurate description like "Interpolate between PingLightBlue -> PingRed" or delete the comment entirely to avoid confusion.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/SettingItem.kt (1)
96-101: 💤 Low valueAvoid a hardcoded content description.
The chevron's
contentDescription = "Navigate"is a hardcoded literal. Since it's a decorative affordance for an already-labeled row, prefernull, or source fromstrings.xmlif it should be announced.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/SettingItem.kt` around lines 96 - 101, The Icon in SettingItem (inside the hasNavigation && onNavigate != null branch) uses a hardcoded contentDescription "Navigate"; change it to null (or load a localized string resource if it must be announced) so the decorative chevron isn't redundantly announced — update the Icon call in SettingItem.kt where hasNavigation and onNavigate are checked to set contentDescription = null (or use stringResource(...) if you decide to provide an accessible label).samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/SettingsScreen.kt (1)
70-107: 💤 Low valueExtract hardcoded UI strings to
strings.xml.This screen hardcodes user-facing text ("Settings", "Theme", the theme description, "Enable diagnostic logging", "View diagnostic logs", etc.), whereas
BackNavigationTopAppBaralready sources its content description viastringResource. Since the app already maintains astrings.xml, move these literals there for consistency and localization.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/SettingsScreen.kt` around lines 70 - 107, Replace hardcoded UI strings in SettingsScreen.kt with string resources: move literals like "Settings", "Theme", the theme description, "Enable diagnostic logging", and "View diagnostic logs" into strings.xml and use stringResource(...) in the UI. Update BackNavigationTopAppBar title to use stringResource(R.string.settings), replace SettingItem title/description arguments to use stringResource(R.string.theme) and stringResource(R.string.theme_description, getThemeDisplayName(themeMode)) for the interpolated theme text, and similarly use stringResource for the diagnosticLogging titles/descriptions; keep the same calls to SettingItem, getThemeDisplayName(themeMode), viewModel.setDiagnosticLogging, and the diagnosticLogging conditional but substitute the string literals with stringResource references.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/util/QrCodeAnalyzer.kt (1)
56-59: 💤 Low valueReplace
printStackTrace()with structured logging.Scan failures are silently dumped to stderr via
printStackTrace(), which is invisible in production diagnostics. The app already has aDiagnosticLogger; route failures there instead so they’re captured consistently.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/util/QrCodeAnalyzer.kt` around lines 56 - 59, In QrCodeAnalyzer replace the use of exception.printStackTrace() inside the addOnFailureListener lambda with a call to the app's DiagnosticLogger (e.g., DiagnosticLogger.error or the equivalent logging method used elsewhere) so scan failures are recorded in structured logs; include a clear contextual message (e.g., "QR scan failed") and pass the exception object to the logger to preserve stack/kv details.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/QrScannerScreen.kt (1)
108-117: 💤 Low valueRemove commented-out dead code.
These commented snackbar/
lastAddedOathCredentiallines are leftover scaffolding and add noise. Delete them before merge.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/QrScannerScreen.kt` around lines 108 - 117, Remove the commented-out dead code block related to showing a snackbar for lastAddedOathCredential: delete the LaunchedEffect block and its references to uiState.lastAddedOathCredential, snackbarHostState.showSnackbar(...), viewModel.clearLastAddedOathCredential(), and onScanComplete() in QrScannerScreen.kt so only active code remains and no leftover scaffold comments are left behind.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/MainViewModel.kt (1)
101-108: 💤 Low valueNo-op
copyon success path.
it.copy(pingOneMfaAccounts = it.pingOneMfaAccounts, error = null)reassigns the accounts list to itself; onlyerror = nullhas effect (account updates flow in viasetupStateFlows). The redundant field is harmless but misleading — drop it for clarity.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/MainViewModel.kt` around lines 101 - 108, In loadPingOneMfaAccounts(), the success branch calls _uiState.update with a no-op copy that reassigns pingOneMfaAccounts to itself; remove that redundant field so the update only clears error (e.g., _uiState.update { it.copy(error = null) }), leaving pingOneMfaAccounts to be updated by the existing setupStateFlows; keep pingOneMfaAccountsLoaded = true and the accountsManager.loadAccounts().onFailure block unchanged.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/MainActivity.kt (1)
107-126: 🏗️ Heavy liftViewModel is constructed directly, so it won't survive configuration changes.
Instantiating
PingOneMFAViewModelwith its constructor insidelifecycleScoperebuilds it (and reloads accounts) on everyonCreate, including rotation, defeating the purpose of aViewModel. Obtain it throughViewModelProvider/viewModels()with a factory so its state is retained.🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/MainActivity.kt` around lines 107 - 126, setupViewModels currently constructs PingOneMFAViewModel directly inside lifecycleScope which causes a new instance on every onCreate; instead create a PingOneMFAViewModelFactory that accepts Application, UserPreferences, AccountsManager, OTPManager (or providers for async init) and use ViewModelProvider / by viewModels(factory) to obtain the ViewModel so it survives configuration changes; move client creation into the factory or provide singleton managers (AccountsManager, OTPManager) that are created once and passed into the factory, and replace the direct constructor call in setupViewModels with ViewModelProvider(this, factory).get(PingOneMFAViewModel::class.java) (or equivalent viewModels usage).samples/pingonemfapp/build.gradle.kts (1)
62-65: 💤 Low valueRemove
composeOptions.kotlinCompilerExtensionVersionsince the module uses the Kotlin Compose compiler Gradle plugin
- In
samples/pingsampleapp/build.gradle.kts(lines 58-60),composeOptions { kotlinCompilerExtensionVersion = "1.5.9" }is likely redundant/misleading because the module appliesalias(libs.plugins.compose.compiler)(which isorg.jetbrains.kotlin.plugin.composeversioned by the Kotlin version ingradle/libs.versions.toml).composeOptions { kotlinCompilerExtensionVersion = "1.5.9" }Remove the
composeOptionsblock to avoid implying that1.5.9controls the Compose compiler.🤖 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 `@samples/pingonemfapp/build.gradle.kts` around lines 62 - 65, The composeOptions block setting kotlinCompilerExtensionVersion is redundant because this module applies the Kotlin Compose compiler plugin (alias(libs.plugins.compose.compiler) / org.jetbrains.kotlin.plugin.compose) which controls the compiler version; remove the composeOptions { kotlinCompilerExtensionVersion = "1.5.8" } block from the build.gradle.kts (eliminate the composeOptions/kotlinCompilerExtensionVersion entry) so the plugin-managed version from gradle/libs.versions.toml is used instead.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/NotificationHelper.kt (1)
84-84: 💤 Low valueVariable shadows parameter with same name.
val title = title ?: ...shadows the function parametertitle, which can be confusing. Use a different name for clarity.Suggested fix
- val title = title ?: context.getString(R.string.system_notification_title) + val notificationTitle = title ?: context.getString(R.string.system_notification_title) val content = when { body != null -> body else -> context.getString(R.string.system_notification_content) } // Build the notification val builder = NotificationCompat.Builder(context, CHANNEL_ID) .setSmallIcon(R.drawable.ic_notification) - .setContentTitle(title) + .setContentTitle(notificationTitle)🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/NotificationHelper.kt` at line 84, The parameter name title in the relevant function is being shadowed by a local val (val title = title ?: ...); rename the local variable to a non-conflicting name (for example resolvedTitle or fallbackTitle) and update subsequent uses in the function to reference that new name; ensure the parameter name remains unchanged and the fallback logic uses context.getString(R.string.system_notification_title) when title is null.samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/managers/OTPManager.kt (1)
23-23: ⚡ Quick winPublic mutable Job field allows external interference.
otpRefreshJobis a publicvar, allowing callers to reassign it and break the single-job invariant enforced bystartAutoRefresh. Consider making it private or exposing only a read-only view.Suggested fix
- var otpRefreshJob: Job? = null + private var otpRefreshJob: Job? = null🤖 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 `@samples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/managers/OTPManager.kt` at line 23, otpRefreshJob is a public mutable var which lets callers reassign or cancel it; make it private to preserve the single-job invariant enforced by startAutoRefresh and only expose a safe read-only view. Change otpRefreshJob to a private var otpRefreshJob: Job? and, if external visibility is required, add a public read-only accessor (e.g., val isAutoRefreshActive: Boolean get() = otpRefreshJob?.isActive == true or val currentOtpRefreshJob: Job? get() = otpRefreshJob) and update startAutoRefresh / any cancelAutoRefresh logic to use the private field.pingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFAException.kt (1)
26-33: ⚡ Quick winPreserve the original
causefor better diagnostics.
internal constructor(cause: Exception)only forwardscause.message; the original throwable is discarded, so its stack trace won't be chained in logs/crash reports. Thread the cause through toException(message, cause).♻️ Proposed change to retain the cause chain
-class PingOneMFAException(message: String?) : Exception(message) { +class PingOneMFAException(message: String?, cause: Throwable? = null) : Exception(message, cause) { // Internal factory constructors — accept native SDK type but keep them off the public API surface entirely. internal constructor(error: PingOneSDKError) : this( "Code=${error.code} \"${error.message}\" UserInfo=${error.userInfo}" ) - internal constructor(cause: Exception) : this(cause.message ?: "Unknown error") + internal constructor(cause: Exception) : this(cause.message ?: "Unknown error", cause) }🤖 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/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFAException.kt` around lines 26 - 33, The secondary constructor in PingOneMFAException discards the original cause; update the exception class to preserve cause chaining by adding a cause parameter to the primary constructor and passing it to the superclass (e.g., change class PingOneMFAException(message: String?) : Exception(message) to accept an optional cause and call Exception(message, cause)), then change internal constructor(cause: Exception) to delegate to the primary constructor with this(cause.message ?: "Unknown error", cause) so the original throwable is preserved in the stack trace.pingonemfa/src/main/java/com/pingidentity/pingonemfa/push/PushApprovalService.kt (2)
97-130: ⚡ Quick winReuse the existing
PushNotificationsuspend wrappers instead of re-implementing approve/deny.
PushNotificationalready exposesapproveNotification(context, authenticationMethod, numberChallenge)anddenyNotification(context)that bridge the same callbacks intoResult. These two private helpers duplicate that logic. Calling the existing functions keeps a single source of truth for the callback bridging and error mapping.♻️ Suggested reuse
scope.launch { try { if (userAction.equals("approve", ignoreCase = true)) { - approveNotificationWithAppInBackground(notificationObject, authMethod) - }else{ - denyNotificationWithAppInBackground(notificationObject) + notificationObject.approveNotification(this@PushApprovalService, authMethod) + .getOrThrow() + } else { + notificationObject.denyNotification(this@PushApprovalService) + .getOrThrow() } } catch (e: Exception) {🤖 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/src/main/java/com/pingidentity/pingonemfa/push/PushApprovalService.kt` around lines 97 - 130, The two private suspend helpers approveNotificationWithAppInBackground and denyNotificationWithAppInBackground duplicate existing suspend wrappers on PushNotification; replace their internal implementations (and call sites) to delegate to PushNotification.approveNotification(context, authenticationMethod, numberChallenge) and PushNotification.denyNotification(context) respectively, then remove the duplicate callback-to-coroutine bridging code so the single source of truth in PushNotification handles error/result mapping.
84-91: 💤 Low valueHardcoded, non-localized notification strings in a library.
The channel name and notification title/text are hardcoded English literals. Since this is a reusable library surface shown to end users, consider sourcing them from string resources so consumers can localize.
🤖 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/src/main/java/com/pingidentity/pingonemfa/push/PushApprovalService.kt` around lines 84 - 91, The notification channel name and notification title/text are hardcoded in PushApprovalService when creating the NotificationChannel and building the NotificationCompat.Builder; replace those literals with string resources and load them via getString(...) (e.g., use getString(R.string.mfa_approval_channel_name) for the channel name and getString(R.string.approving_login_title) / getString(R.string.contacting_server_text) for the notification title and text) so consumers can localize; update any related XML string resource files with appropriate keys and ensure NotificationChannel creation and NotificationCompat.Builder use the resource strings.pingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.kt (1)
331-349: ⚡ Quick winDeduplicate and harden the
apsJSON parsing.These two helpers parse the same
message.data["aps"]string independently (parsed twice per notification at Lines 240–241), andJson.parseToJsonElementthrowsSerializationExceptionon malformed input. If the SDK callback runs off the registering thread, that throw escapes thetryinprocessRemoteNotification. Consider parsing once into a nullable model withrunCatchingand extracting both fields.♻️ Suggested consolidation
- title = getTitleFromRemoteMessageData(message.data["aps"]), - message = getBodyFromRemoteMessageData(message.data["aps"]) + title = parseAps(message.data["aps"])?.title, + message = parseAps(message.data["aps"])?.body- private fun getTitleFromRemoteMessageData(data: String?): String? = - data?.let { - Json.parseToJsonElement(it) - .jsonObject["alert"] - ?.jsonObject - ?.get("title") - ?.jsonPrimitive - ?.contentOrNull - } - - private fun getBodyFromRemoteMessageData(data: String?): String? = - data?.let { - Json.parseToJsonElement(it) - .jsonObject["alert"] - ?.jsonObject - ?.get("body") - ?.jsonPrimitive - ?.contentOrNull - } + private data class Aps(val title: String?, val body: String?) + + private fun parseAps(data: String?): Aps? = data?.let { + runCatching { + val alert = Json.parseToJsonElement(it).jsonObject["alert"]?.jsonObject + Aps( + title = alert?.get("title")?.jsonPrimitive?.contentOrNull, + body = alert?.get("body")?.jsonPrimitive?.contentOrNull, + ) + }.getOrNull() + }🤖 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/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.kt` around lines 331 - 349, The two helpers getTitleFromRemoteMessageData and getBodyFromRemoteMessageData both re-parse the same JSON (message.data["aps"]) and can throw SerializationException; refactor by parsing the aps JSON once (e.g., in processRemoteNotification or a new private parseApsModel function) using runCatching to return a nullable parsed model (data class or JsonObject), then derive title and body from that single parsed result rather than calling Json.parseToJsonElement twice; ensure the parsing is wrapped in runCatching/nullable handling so malformed input doesn't throw out of processRemoteNotification and update callers to use the single parsed result to populate title/body.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5be0659a-a2ef-44fc-9f2e-ce7c4f9dfe7a
📒 Files selected for processing (95)
README.mdgradle/libs.versions.tomlpingonemfa/.gitignorepingonemfa/README.mdpingonemfa/build.gradle.ktspingonemfa/src/main/AndroidManifest.xmlpingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/Geo.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFAException.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMfaAccount.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/otp/OtpCodeInfo.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/push/PushApprovalService.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/push/PushNotification.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/push/PushType.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/util/AccountParser.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/GeoTest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/PingOneMFATest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/push/PushApprovalServiceTest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/push/PushNotificationTest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/util/AccountParserTest.ktprotect/src/main/kotlin/com/pingidentity/protect/Protect.ktsamples/pingonemfapp/.gitignoresamples/pingonemfapp/README.mdsamples/pingonemfapp/build.gradle.ktssamples/pingonemfapp/src/main/AndroidManifest.xmlsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/MainActivity.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/PingOneMFApp.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/DiagnosticLogger.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/MainViewModel.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/UiModels.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/data/UserPreferences.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/managers/AccountsManager.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/managers/OTPManager.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/BiometricPromptActivity.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/NotificationActionReceiver.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/NotificationHelper.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/notification/PushNotificationActivity.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/service/PushNotificationService.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AboutScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AccountsScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/AuthenticatorNavHost.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/DiagnosticLogsScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/LoginScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/NotificationResponseScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/OtpScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/QrScannerScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/SettingsScreen.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/AccountAvatar.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/AccountCard.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/BackNavigationTopAppBar.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/EmptyStateMessage.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/ErrorAlertDialog.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/ExpiringOtpCode.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/LoadingIndicator.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/components/SettingItem.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/theme/Color.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/theme/Theme.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/ui/theme/Type.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/util/NavigationAnimations.ktsamples/pingonemfapp/src/main/kotlin/com/pingidentity/pingonemfapp/util/QrCodeAnalyzer.ktsamples/pingonemfapp/src/main/res/drawable/ic_check.xmlsamples/pingonemfapp/src/main/res/drawable/ic_close.xmlsamples/pingonemfapp/src/main/res/drawable/ic_fingerprint.xmlsamples/pingonemfapp/src/main/res/drawable/ic_launcher_foreground.xmlsamples/pingonemfapp/src/main/res/drawable/ic_notification.xmlsamples/pingonemfapp/src/main/res/drawable/ping_logo.xmlsamples/pingonemfapp/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlsamples/pingonemfapp/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xmlsamples/pingonemfapp/src/main/res/values/ic_launcher_background.xmlsamples/pingonemfapp/src/main/res/values/strings.xmlsamples/pingonemfapp/src/main/res/values/themes.xmlsamples/pingsampleapp/README.mdsamples/pingsampleapp/build.gradle.ktssamples/pingsampleapp/src/main/AndroidManifest.xmlsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/PingSampleApplication.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/authenticator/service/PushNotificationService.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/home/HomeApp.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/navigation/Navigation.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/PingOneMFAState.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/PingOneMFAViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/notification/PingOneNotificationActionReceiver.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/notification/PingOneNotificationHelper.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/notification/PingOnePushNotificationActivity.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/notification/PushNotificationStore.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneMFAAccountsScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePayloadScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePushNotificationScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneQrScannerScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/components/ApproveDenyRow.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/components/ManualNumberChallenge.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/components/NumberChallengeOptions.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/util/PingOneMFAQrCodeAnalyzer.ktsamples/pingsampleapp/src/main/res/values/strings.xmlsettings.gradle.kts
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## develop #205 +/- ##
==============================================
- Coverage 42.64% 25.90% -16.75%
+ Complexity 1287 932 -355
==============================================
Files 312 306 -6
Lines 9447 9199 -248
Branches 1403 1369 -34
==============================================
- Hits 4029 2383 -1646
- Misses 4862 6500 +1638
+ Partials 556 316 -240
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/push/PushNotificationTest.kt (1)
152-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the method result, not the mock field directly.
Line 159 currently checks
push.notificationObject.numberMatchingOptions, which bypassesgetNumbersChallenge()and weakens the test’s stated intent. Assert the method output instead.Proposed fix
- assertTrue(push.notificationObject.numberMatchingOptions == null || push.notificationObject.numberMatchingOptions.isEmpty()) + val result = push.getNumbersChallenge() + assertTrue(result == null || result.isEmpty())🤖 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/src/test/kotlin/com/pingidentity/pingonemfa/push/PushNotificationTest.kt` around lines 152 - 160, The test currently asserts the mock field directly instead of the method under test; change the assertion to call PushNotification.getNumbersChallenge() on the push instance (created with the mocked notificationObject and numberMatchingType returning null) and assert that the returned collection/array is empty (or null according to the method's contract), removing the direct check of push.notificationObject.numberMatchingOptions so the test verifies getNumbersChallenge() behavior.
🤖 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.
Outside diff comments:
In
`@pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/push/PushNotificationTest.kt`:
- Around line 152-160: The test currently asserts the mock field directly
instead of the method under test; change the assertion to call
PushNotification.getNumbersChallenge() on the push instance (created with the
mocked notificationObject and numberMatchingType returning null) and assert that
the returned collection/array is empty (or null according to the method's
contract), removing the direct check of
push.notificationObject.numberMatchingOptions so the test verifies
getNumbersChallenge() behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cceea350-4450-4a18-af80-aa3445bcfcfb
📒 Files selected for processing (3)
pingonemfa/src/main/java/com/pingidentity/pingonemfa/push/PushApprovalService.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/push/PushNotificationTest.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/PingOneMFAViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/PingOneMFAViewModel.kt
- pingonemfa/src/main/java/com/pingidentity/pingonemfa/push/PushApprovalService.kt
EvgeniyMish
had a problem deploying
to
github-pages
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.kt (1)
96-100:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading copy for the not-paired state.
The
isOtpDeviceNotPairedbranch reusesaccounts_empty_state_title/subtitle, which describe an empty accounts list rather than "device not paired — pair a device to view its OTP." Consider a dedicated string so the message matches the actual condition.🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.kt` around lines 96 - 100, The not-paired branch for state.isOtpDeviceNotPaired incorrectly reuses accounts_empty_state_title/subtitle; update the EmptyStateMessage call in the isOtpDeviceNotPaired branch to use dedicated string resources (e.g., otp_device_not_paired_title and otp_device_not_paired_subtitle) and add those entries to the strings.xml with copy that clearly instructs the user to pair a device to view OTPs; keep the EmptyStateMessage usage and parameters (title/subtitle) unchanged except for the new stringResource keys so the UI text matches the condition.
🧹 Nitpick comments (5)
pingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.kt (2)
338-364: 💤 Low valueExtract duplicated intent-extra keys/builder.
approvePushNotificationFromBanneranddenyPushNotificationFromBannerare identical except for theuser_actionvalue, and the extra keys ("notification","auth_method","user_action") are string literals shared withPushApprovalService. Diverging keys here would silently break the service contract. Consider shared constants and a single private helper parameterized by action.🤖 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/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.kt` around lines 338 - 364, approvePushNotificationFromBanner and denyPushNotificationFromBanner duplicate intent building and use hard-coded extra keys that must match PushApprovalService; refactor by introducing shared constants for the intent extras (e.g., NOTIFICATION_EXTRA, AUTH_METHOD_EXTRA, USER_ACTION_EXTRA) and a single private helper method (e.g., sendPushApprovalFromBanner(action: String)) that constructs the Intent, sets the extras using the constants, and calls ContextCompat.startForegroundService; update approvePushNotificationFromBanner and denyPushNotificationFromBanner to call this helper with "approve" and "deny" respectively and keep references to ContextProvider.context and PushApprovalService unchanged.
267-290: ⚡ Quick winA malformed
apspayload fails an otherwise-valid notification.
getTitleFromRemoteMessageData/getBodyFromRemoteMessageDatacallJson.parseToJsonElement(...).jsonObject, which throws on non-JSON or non-object input. Because these run inside thenotificationObject?.let { ... }success branch (Line 275-276) within the surroundingtry, a parse failure is caught at Line 293 and turns a perfectly validnotificationObjectinto aResult.failure. Consider parsing title/body defensively (returningnullon failure) so the notification still succeeds without a display title/body.🤖 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/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.kt` around lines 267 - 290, The current logic in the PushNotification success branch calls getTitleFromRemoteMessageData and getBodyFromRemoteMessageData which throw when the "aps" payload is malformed, causing an otherwise-valid notification to be turned into a failure; change those parsing helpers (getTitleFromRemoteMessageData/getBodyFromRemoteMessageData) to parse defensively and return null on parse errors (catch JSON parse/format exceptions and return null), so the PushNotification construction in the notificationObject?.let { ... } branch uses nullable title/message and still returns Result.success when notificationObject is present.samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.kt (1)
116-120: ⚡ Quick winHardcoded countdown string.
"Refreshes in ${state.otpSecondsRemaining}s"is not localizable and is inconsistent with the strings-in-strings.xmlapproach used elsewhere. Prefer a parameterized string resource.♻️ Use a string resource
Text( - text = "Refreshes in ${state.otpSecondsRemaining}s", + text = stringResource(R.string.text_pingone_mfa_otp_refreshes_in, state.otpSecondsRemaining), style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant, )🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.kt` around lines 116 - 120, Replace the hardcoded countdown text in the PingOneOTPScreen Text composable with a parameterized string resource: add a string entry (e.g., "refreshes_in_seconds") to strings.xml that accepts an integer placeholder, then use stringResource(R.string.refreshes_in_seconds, state.otpSecondsRemaining) in the Text call (located in PingOneOTPScreen.kt where the Text currently shows "Refreshes in ${state.otpSecondsRemaining}s"); ensure the proper import for stringResource is present and remove the inline "s" suffix so the localization string controls formatting.samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneMFAAccountsScreen.kt (1)
156-163: ⚡ Quick winMove the "Region:" / "ID:" labels into
strings.xml.These literals are hardcoded while the rest of the screen uses
stringResource. This is inconsistent with the PR's "UI strings moved to strings.xml" goal and isn't localizable.♻️ Use string resources
Text( - text = "Region: ${account.region}", + text = stringResource(R.string.text_pingone_mfa_account_region, account.region), style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant, ) Text( - text = "ID: ${account.id}", + text = stringResource(R.string.text_pingone_mfa_account_id, account.id), style = MaterialTheme.typography.bodyMedium, color = MaterialTheme.colorScheme.onSurfaceVariant, )🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneMFAAccountsScreen.kt` around lines 156 - 163, The hardcoded "Region: ${account.region}" and "ID: ${account.id}" Text usages in PingOneMFAAccountsScreen.kt should be moved to string resources and accessed via stringResource with placeholders; add entries like ping_region_label="%1$s: %2$s" or "Region: %1$s"/or better "Region: %s" and ping_id_label="ID: %s" in strings.xml, then replace the Text calls to use stringResource(R.string.ping_region_label, "Region", account.region) or (preferably) define the full localized label with a single placeholder and call stringResource(R.string.ping_region_label, account.region) and stringResource(R.string.ping_id_label, account.id) so the UI matches the rest of the screen and is localizable.samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePayloadScreen.kt (1)
130-137: ⚡ Quick winFlag the copied MFA payload as sensitive
Place a sensitive flag on the
ClipData’sClipDescriptionso the system can hide/obfuscate clipboard previews (this is a UI rendering hint, not extra protection for the underlying clipboard contents).🛡️ Flag clip as sensitive
scope.launch { - clipboardManager.setClipEntry( - ClipEntry(ClipData.newPlainText("payload", state.payload)) - ) + val clip = ClipData.newPlainText("payload", state.payload).apply { + description.extras = PersistableBundle().apply { + putBoolean(ClipDescription.EXTRA_IS_SENSITIVE, true) + } + } + clipboardManager.setClipEntry(ClipEntry(clip)) }
LocalClipboard/ suspendClipboard.setClipEntry(ClipEntry(ClipData)) are available in androidx.compose.ui 1.11.0-rc01+—so ensure this module resolves to that version or newer.🤖 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 `@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePayloadScreen.kt` around lines 130 - 137, The clipboard copy currently uses ClipData.newPlainText(...) without marking the clip as sensitive; update the Button handler to build a ClipDescription with the MIMETYPE_TEXT_PLAIN and the ClipDescription.FLAG_IS_SENSITIVE flag and create the ClipData using that description (or otherwise ensure the ClipData’s ClipDescription has FLAG_IS_SENSITIVE) before calling clipboardManager.setClipEntry(ClipEntry(clipData)); modify the code around ClipEntry, ClipData.newPlainText(...) and clipboardManager.setClipEntry(...) so the MFA payload is flagged as sensitive (requires androidx.compose.ui 1.11.0-rc01+).
🤖 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
`@pingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFAException.kt`:
- Around line 73-77: The secondary constructor internal constructor(errors:
Array<PingOneSDKError>) can NPE when errors[0] is actually null; change it to
pick the first non-null PingOneSDKError (e.g. errors.firstOrNull { it != null }
or errors.filterNotNull().firstOrNull()) and use its message via a safe call
with a fallback (e.g. firstNonNull?.message ?: "Unknown error"); keep
internalErrorsList = ErrorParser.fromPingOneSDKErrors(errors) unchanged.
In `@pingonemfa/src/main/java/com/pingidentity/pingonemfa/util/AccountParser.kt`:
- Line 51: The username property in AccountParser (username: String?) is
currently treated as required by kotlinx.serialization because it lacks an
explicit default; update the AccountParser data/class declaration to give
username a default value (e.g., username: String? = null) so deserialization
does not throw MissingFieldException when the "username" key is absent, matching
the pattern used for sibling nullable fields.
---
Outside diff comments:
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.kt`:
- Around line 96-100: The not-paired branch for state.isOtpDeviceNotPaired
incorrectly reuses accounts_empty_state_title/subtitle; update the
EmptyStateMessage call in the isOtpDeviceNotPaired branch to use dedicated
string resources (e.g., otp_device_not_paired_title and
otp_device_not_paired_subtitle) and add those entries to the strings.xml with
copy that clearly instructs the user to pair a device to view OTPs; keep the
EmptyStateMessage usage and parameters (title/subtitle) unchanged except for the
new stringResource keys so the UI text matches the condition.
---
Nitpick comments:
In `@pingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.kt`:
- Around line 338-364: approvePushNotificationFromBanner and
denyPushNotificationFromBanner duplicate intent building and use hard-coded
extra keys that must match PushApprovalService; refactor by introducing shared
constants for the intent extras (e.g., NOTIFICATION_EXTRA, AUTH_METHOD_EXTRA,
USER_ACTION_EXTRA) and a single private helper method (e.g.,
sendPushApprovalFromBanner(action: String)) that constructs the Intent, sets the
extras using the constants, and calls ContextCompat.startForegroundService;
update approvePushNotificationFromBanner and denyPushNotificationFromBanner to
call this helper with "approve" and "deny" respectively and keep references to
ContextProvider.context and PushApprovalService unchanged.
- Around line 267-290: The current logic in the PushNotification success branch
calls getTitleFromRemoteMessageData and getBodyFromRemoteMessageData which throw
when the "aps" payload is malformed, causing an otherwise-valid notification to
be turned into a failure; change those parsing helpers
(getTitleFromRemoteMessageData/getBodyFromRemoteMessageData) to parse
defensively and return null on parse errors (catch JSON parse/format exceptions
and return null), so the PushNotification construction in the
notificationObject?.let { ... } branch uses nullable title/message and still
returns Result.success when notificationObject is present.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneMFAAccountsScreen.kt`:
- Around line 156-163: The hardcoded "Region: ${account.region}" and "ID:
${account.id}" Text usages in PingOneMFAAccountsScreen.kt should be moved to
string resources and accessed via stringResource with placeholders; add entries
like ping_region_label="%1$s: %2$s" or "Region: %1$s"/or better "Region: %s" and
ping_id_label="ID: %s" in strings.xml, then replace the Text calls to use
stringResource(R.string.ping_region_label, "Region", account.region) or
(preferably) define the full localized label with a single placeholder and call
stringResource(R.string.ping_region_label, account.region) and
stringResource(R.string.ping_id_label, account.id) so the UI matches the rest of
the screen and is localizable.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.kt`:
- Around line 116-120: Replace the hardcoded countdown text in the
PingOneOTPScreen Text composable with a parameterized string resource: add a
string entry (e.g., "refreshes_in_seconds") to strings.xml that accepts an
integer placeholder, then use stringResource(R.string.refreshes_in_seconds,
state.otpSecondsRemaining) in the Text call (located in PingOneOTPScreen.kt
where the Text currently shows "Refreshes in ${state.otpSecondsRemaining}s");
ensure the proper import for stringResource is present and remove the inline "s"
suffix so the localization string controls formatting.
In
`@samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePayloadScreen.kt`:
- Around line 130-137: The clipboard copy currently uses
ClipData.newPlainText(...) without marking the clip as sensitive; update the
Button handler to build a ClipDescription with the MIMETYPE_TEXT_PLAIN and the
ClipDescription.FLAG_IS_SENSITIVE flag and create the ClipData using that
description (or otherwise ensure the ClipData’s ClipDescription has
FLAG_IS_SENSITIVE) before calling
clipboardManager.setClipEntry(ClipEntry(clipData)); modify the code around
ClipEntry, ClipData.newPlainText(...) and clipboardManager.setClipEntry(...) so
the MFA payload is flagged as sensitive (requires androidx.compose.ui
1.11.0-rc01+).
🪄 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
Run ID: 060b7f5b-a676-4095-b5e2-d3be557fadba
📒 Files selected for processing (12)
pingonemfa/README.mdpingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFA.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFAException.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/util/AccountParser.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/PingOneMFATest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/push/PushNotificationTest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/util/AccountParserTest.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/PingOneMFAViewModel.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneMFAAccountsScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOneOTPScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePayloadScreen.ktsamples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePushNotificationScreen.kt
✅ Files skipped from review due to trivial changes (1)
- pingonemfa/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/util/AccountParserTest.kt
- samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/PingOneMFAViewModel.kt
- samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/pingonemfa/ui/PingOnePushNotificationScreen.kt
- pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/push/PushNotificationTest.kt
- pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/PingOneMFATest.kt
EvgeniyMish
had a problem deploying
to
github-pages
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/PingOneMFAExceptionTest.kt (1)
147-152: 💤 Low valueRemove the leftover
printlndebug statement. Line 149 is a debug artifact that pollutes test output.🧹 Proposed cleanup
val errors = arrayOf(PingOneSDKError(10003, null)) - println("SDK error message: ${errors[0].message}") val e = PingOneMFAException(errors) assertEquals("Unknown error", e.message)🤖 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/src/test/kotlin/com/pingidentity/pingonemfa/commons/PingOneMFAExceptionTest.kt` around lines 147 - 152, In PingOneMFAExceptionTest.remove the leftover debug print by deleting the println("SDK error message: ${errors[0].message}") call in the test method named `constructor with error array defaults message when first error message is null`; keep the rest of the test intact so it still constructs `errors`, instantiates `PingOneMFAException(errors)`, and asserts the message equals "Unknown error".pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/util/ErrorParserTest.kt (1)
42-52: 💤 Low valueThe test name says "when SDK userInfo is null", but as the inline comment notes,
PingOneSDKError.userInfodefaults to an empty map and is never null here — so this exercises the empty (not null) path. Consider renaming to reflect what's actually verified, or explicitly setuserInfo = nullif the SDK type allows it, to cover the?: emptyMap()branch.🤖 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/src/test/kotlin/com/pingidentity/pingonemfa/util/ErrorParserTest.kt` around lines 42 - 52, Rename the test or modify the setup so it actually covers the null-guard branch: either (A) rename the test function `fromPingOneSDKError returns empty userInfo when SDK userInfo is null` to reflect that it currently verifies an empty-map case (e.g., `...when SDK userInfo is empty`), or (B) if the SDK type allows, instantiate `PingOneSDKError` with `userInfo = null` before calling `parser.fromPingOneSDKError(sdkError)` to exercise the `?: emptyMap()` path; update the assertion on `result.first().userInfo` accordingly. Ensure changes reference the existing test function and the `PingOneSDKError`/`parser.fromPingOneSDKError` usage so the intent is clear.pingonemfa/src/main/java/com/pingidentity/pingonemfa/util/ErrorParser.kt (1)
13-16: 💤 Low valueUse a KDoc block (
/**) for the class doc. This comment uses[PingOneSDKError]/[Error]doc-link syntax but is opened with/*, so it won't be parsed as KDoc and the links won't resolve.📝 Proposed fix
-/* +/** * Converts native [PingOneSDKError] objects from the `pingidsdkv2` AAR into the * wrapper [Error] type, so callers never need a direct dependency on the native SDK. */🤖 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/src/main/java/com/pingidentity/pingonemfa/util/ErrorParser.kt` around lines 13 - 16, Update the top-of-file block comment to a proper KDoc block (/** ... */) so the bracketed doc links like [PingOneSDKError] and [Error] are parsed and resolved; replace the existing /* ... */ comment above the converter (the class or top-level mapper in ErrorParser.kt that converts PingOneSDKError to the wrapper Error type) with a KDoc comment preserving the text and links.pingonemfa/build.gradle.kts (1)
9-16: ⚡ Quick winRemove redundant Android/Kotlin plugin aliases in
pingonemfa
pingonemfa/build.gradle.ktsappliesid("com.pingidentity.convention.android.library"), andbuild-logic/convention/src/main/kotlin/com/pingidentity/convention/AndroidLibraryConventionPlugin.ktalready doespluginManager.apply("com.android.library"), soalias(libs.plugins.androidLibrary)is redundant.It also applies
id("com.pingidentity.convention.centralPublish"), andbuild-logic/convention/src/main/kotlin/com/pingidentity/convention/MavenCentralPublishConventionPlugin.ktalready doespluginManager.apply("kotlin-android"), soalias(libs.plugins.kotlinAndroid)is redundant in this module (though it’s unlikely to break since Gradle won’t need to apply the same plugin twice).plugins { id("com.pingidentity.convention.android.library") id("com.pingidentity.convention.centralPublish") id("kotlin-parcelize") alias(libs.plugins.androidLibrary) alias(libs.plugins.kotlinAndroid) alias(libs.plugins.kotlinSerialization) }🤖 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/build.gradle.kts` around lines 9 - 16, Remove the redundant plugin aliases from the plugins block in pingonemfa/build.gradle.kts: delete alias(libs.plugins.androidLibrary) and alias(libs.plugins.kotlinAndroid) because id("com.pingidentity.convention.android.library") (see AndroidLibraryConventionPlugin) already applies "com.android.library" and id("com.pingidentity.convention.centralPublish") (see MavenCentralPublishConventionPlugin) already applies "kotlin-android"; keep the remaining plugins such as id("kotlin-parcelize") and alias(libs.plugins.kotlinSerialization).
🤖 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 `@pingonemfa/build.gradle.kts`:
- Around line 9-16: Remove the redundant plugin aliases from the plugins block
in pingonemfa/build.gradle.kts: delete alias(libs.plugins.androidLibrary) and
alias(libs.plugins.kotlinAndroid) because
id("com.pingidentity.convention.android.library") (see
AndroidLibraryConventionPlugin) already applies "com.android.library" and
id("com.pingidentity.convention.centralPublish") (see
MavenCentralPublishConventionPlugin) already applies "kotlin-android"; keep the
remaining plugins such as id("kotlin-parcelize") and
alias(libs.plugins.kotlinSerialization).
In `@pingonemfa/src/main/java/com/pingidentity/pingonemfa/util/ErrorParser.kt`:
- Around line 13-16: Update the top-of-file block comment to a proper KDoc block
(/** ... */) so the bracketed doc links like [PingOneSDKError] and [Error] are
parsed and resolved; replace the existing /* ... */ comment above the converter
(the class or top-level mapper in ErrorParser.kt that converts PingOneSDKError
to the wrapper Error type) with a KDoc comment preserving the text and links.
In
`@pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/PingOneMFAExceptionTest.kt`:
- Around line 147-152: In PingOneMFAExceptionTest.remove the leftover debug
print by deleting the println("SDK error message: ${errors[0].message}") call in
the test method named `constructor with error array defaults message when first
error message is null`; keep the rest of the test intact so it still constructs
`errors`, instantiates `PingOneMFAException(errors)`, and asserts the message
equals "Unknown error".
In
`@pingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/util/ErrorParserTest.kt`:
- Around line 42-52: Rename the test or modify the setup so it actually covers
the null-guard branch: either (A) rename the test function `fromPingOneSDKError
returns empty userInfo when SDK userInfo is null` to reflect that it currently
verifies an empty-map case (e.g., `...when SDK userInfo is empty`), or (B) if
the SDK type allows, instantiate `PingOneSDKError` with `userInfo = null` before
calling `parser.fromPingOneSDKError(sdkError)` to exercise the `?: emptyMap()`
path; update the assertion on `result.first().userInfo` accordingly. Ensure
changes reference the existing test function and the
`PingOneSDKError`/`parser.fromPingOneSDKError` usage so the intent is clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 01c90658-2245-4052-a02f-aa57313301eb
📒 Files selected for processing (8)
pingonemfa/build.gradle.ktspingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/Error.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFAException.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/util/AccountParser.ktpingonemfa/src/main/java/com/pingidentity/pingonemfa/util/ErrorParser.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/ErrorTest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/commons/PingOneMFAExceptionTest.ktpingonemfa/src/test/kotlin/com/pingidentity/pingonemfa/util/ErrorParserTest.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- pingonemfa/src/main/java/com/pingidentity/pingonemfa/commons/PingOneMFAException.kt
- pingonemfa/src/main/java/com/pingidentity/pingonemfa/util/AccountParser.kt
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.
JIRA Ticket
https://pingidentity.atlassian.net/browse/P14C-86660
Description
Summary
Integrates the pingonemfa wrapper module with the Orchestration SDK and wires it into pingsampleapp as a fully-functional reference implementation. This is Phase 1 — wrapper APIs only, no Collector integration.
What's in this PR
pingonemfa module (updated)
Full coroutine wrapper over the native PingOneMFA SDK
PushApprovalService foreground service for background banner approvals/denials
Geo enum mapping public regions to native SDK regions without leaking the native type
Unit tests
Full KDoc on all public API surface
README.md aligned with other documents in this repo
pingsampleapp integration (new)
5 screens: QR pairing scanner, accounts list, OTP with live countdown, mobile payload with copy-to-clipboard,
Push notification screen: handles DEFAULT (approve/deny), CHALLENGE (number options or free-form), and DRY (dismiss), with AlertDialog for success/error outcomes
Background push: PingOneNotificationHelper builds system banners; PingOneNotificationActionReceiver handles banner button taps; PingOnePushNotificationActivity handles foreground full-screen flow
Server-side cancellation: incoming cancel push dismisses the system banner and, if the activity is visible, closes it immediately via LocalBroadcastManager
All UI strings extracted to strings.xml; all AlertDialog feedback uses existing app theme
PingSampleApplication initializes PingOneMFA at startup and registers the FCM token
README.md updated to contain PingOneMFA information
Testing
Set-up native application in PingOne environment
Build with a valid google-services.json
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
Summary by CodeRabbit
New Features
UI / App Integration
Documentation
Tests