Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFIDO collectors now map failures to WebAuthn error codes, expose them as DaVinci action events, and reset error state across lifecycle operations. Collector JSON serialization separates action keys from form data. Sample FIDO flows advance through ChangesFIDO failure state propagation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FidoCollector
participant FidoClient
participant Collectors
FidoCollector->>FidoClient: Start FIDO operation
FidoClient-->>FidoCollector: Return success or exception
FidoCollector->>FidoCollector: Map exception to errorCode
Collectors->>FidoCollector: Read eventType, actionKey, and payload
FidoCollector-->>Collectors: Return submit or action JSON data
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt (1)
22-35: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
eventType()andasJson()can disagree on which collector's action/error wins.
eventType()returns on the FIRSTSubmittablematch (payload or Failable error), butasJson()'s loop processes ALL collectors and lets the LAST matching one overwriteactionKey. If two collectors simultaneously have non-null payload/error (e.g. a failed FIDO collector plus another Submittable/Flow collector with payload), the reportedeventTypeand theactionKeysent to the server can point to different collectors — undermining the goal of correctly surfacing FIDO2 failures asactionevents.♻️ Suggested approach: unify the selection logic
internal fun Collectors.eventType(): String? { - forEach { - when (it) { - is Submittable -> { - val eventType = it.eventType() - if (it.payload() != null || (it is Failable && it.error() != null)) { - return eventType - } - } - else -> {} - } - } - return null + return firstNotNullOfOrNull { it.actingSubmittableOrNull() }?.eventType() } +private fun Collector<*>.actingSubmittableOrNull(): Submittable? = + (this as? Submittable)?.takeIf { payload() != null || (this@actingSubmittableOrNull is Failable && (this@actingSubmittableOrNull as Failable).error() != null) }Then have
asJson()locate the same "acting" collector once (e.g.collectors.firstOrNull { ... }) and use it foractionKey, instead of letting every matching collector overwrite it in the loop.Also applies to: 58-78
🤖 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 `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt` around lines 22 - 35, Unify collector selection between Collectors.eventType() and asJson(): identify the first Submittable whose payload is non-null or whose Failable error is non-null, and use that same collector for actionKey. Update asJson() to select this acting collector once rather than allowing later matching collectors to overwrite the value, while preserving existing serialization for all other collectors.
🤖 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
`@mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt`:
- Line 68: Restrict the visibility of AbstractFidoCollector.error by adding a
private setter, matching the existing key, label, trigger, and required
properties. Change handleError from public to internal so only same-module
subclasses can invoke the exception-mapping logic, while preserving its current
behavior.
---
Outside diff comments:
In `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt`:
- Around line 22-35: Unify collector selection between Collectors.eventType()
and asJson(): identify the first Submittable whose payload is non-null or whose
Failable error is non-null, and use that same collector for actionKey. Update
asJson() to select this acting collector once rather than allowing later
matching collectors to overwrite the value, while preserving existing
serialization for all other collectors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1264a46-22a3-47b9-a75b-e6a9f974281f
📒 Files selected for processing (10)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.ktdavinci/src/test/kotlin/com/pingidentity/davinci/collector/CollectorsTest.ktfoundation/davinci-plugin/src/main/kotlin/com/pingidentity/davinci/plugin/Failable.ktmfa/fido/src/main/kotlin/com/pingidentity/fido/Constants.ktmfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.ktmfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.ktmfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.ktmfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollectorTest.ktmfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollectorTest.ktmfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollectorTest.kt
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
vibhorgoswami
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #230 +/- ##
==============================
==============================
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…Failable interface
…r handling and update related tests
…ror for handling.
|
@coderabbitai walkthrough |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="ForgeRock/ping-android-sdk"
pr="230"
gh pr view "$pr" --repo "$repo" --json title,body,files \
--jq '.title, (.files[] | "\(.path) (+\(.additions)/-\(.deletions))")'
gh pr diff "$pr" --repo "$repo" -- \
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt \
davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt \
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt \
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt \
foundation/davinci-plugin/src/main/kotlin/com/pingidentity/davinci/plugin/ActionKeyProvider.kt \
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/FidoAuthentication.kt \
samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/FidoRegistration.ktLength of output: 2642 🏁 Script executed: #!/bin/bash
set -euo pipefail
files=(
"davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt"
"foundation/davinci-plugin/src/main/kotlin/com/pingidentity/davinci/plugin/ActionKeyProvider.kt"
"mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt"
"mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt"
"mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt"
"samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/DaVinciContinueNode.kt"
"samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/FidoAuthentication.kt"
"samples/pingsampleapp/src/main/java/com/pingidentity/samples/pingsampleapp/davinci/collector/FidoRegistration.kt"
)
for file in "${files[@]}"; do
echo "===== OUTLINE: $file ====="
ast-grep outline "$file" --items all
done
for file in "${files[@]}"; do
echo "===== SOURCE: $file ====="
cat -n "$file"
doneLength of output: 51428
What changes conceptuallyPreviously, FIDO2 failures stayed in the sample app’s local error/retry UI. This PR converts a FIDO failure into a DaVinci action so the Journey server can decide the next step based on the WebAuthn error name. sequenceDiagram
participant UI as Sample FIDO UI
participant Collector as FIDO Collector
participant CM as Android Credential Manager
participant DaVinci as DaVinci request serializer
participant Server as Journey server
UI->>Collector: authenticate() / register()
Collector->>CM: Perform FIDO2 ceremony
alt Success
CM-->>Collector: Assertion / attestation
UI->>DaVinci: onNext()
DaVinci->>Server: submit + formData
else FIDO failure
CM-->>Collector: Credential or DOM exception
Collector->>Collector: Map exception to errorCode
UI->>DaVinci: onNext()
DaVinci->>Server: action + actionKey=<DOM error name>
end
Key implementation pieces
Test coverageThe PR adds focused coverage for collector serialization/event selection plus FIDO error mappings, cancellation propagation, error payload behavior, action-key emission, and reset behavior after |
spetrov
left a comment
spetrov
left a comment
There was a problem hiding this comment.
Changes look good to me. I also performed a few quick tests.
Claude review however flagged the following as "blocking"... TBH, I don't quite understand these, but @witrisna, can you please review and see if these worth our attention?
CancellationExceptionrethrow insideResult.onFailurewill crash the coroutine scope silently.handleError()rethrowsCancellationException, but it's called from inside anonFailure { handleError(exception) }lambda.
Kotlin'sResult.onFailuredoesn't protect against rethrows — theCancellationExceptionpropagates up the coroutine chain and cancels the parent scope rather than surfacing as a failure. The unit testhandleErrorshould rethrowCancellationExceptionvalidates the raw method in isolation, but no test runs the full authenticate()/register() path with aCancellationException.
Fix: guard against it at the call-site (before invoking handleError) rather than rethrowing from within the lambda.
eventType()/asJson()coherence risk iferrorCodeis non-null butpayload()returns null. If actionKey is set (non-null) whilepayload()still returns null (e.g., external mutation beforeauthenticate()runs),eventType()returns null via thepayload() != nullcheck, but actionKey is still emitted inasJson()— producing a JSON payload withactionKeybut noeventType.
This is gated on theerrorCodevar being public-mutable (which CodeRabbit has already flagged but is still unresolved...)
For the CancellationException, it will be good to add test cases for |
…oved eventType handling and error management
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt`:
- Around line 58-62: Update the collector selection flow in Collectors so the
first eligible Submittable collector is selected once and reused for both
eventType() and actionKey, rather than overwriting actionKey while iterating
through later ActionKeyProvider instances. Preserve payload collection for
eligible providers, and add a regression test covering two eligible action-key
collectors to verify both fields come from the selected collector.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7beadc4d-c55f-4577-a14b-4022fc43e1f2
📒 Files selected for processing (9)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.ktdavinci/src/main/kotlin/com/pingidentity/davinci/collector/FlowCollector.ktdavinci/src/main/kotlin/com/pingidentity/davinci/collector/MetadataCollector.ktdavinci/src/main/kotlin/com/pingidentity/davinci/collector/SubmitCollector.ktdavinci/src/test/kotlin/com/pingidentity/davinci/collector/CollectorsTest.ktmfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.ktmfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.ktmfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollectorTest.ktmfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollectorTest.kt
…rror codes and enhance ActionKeyProvider documentation
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
…ng and update related tests
e91a1b8 to
1b05218
Compare
|
|
||
| return buildJsonObject { | ||
| val map = mutableMapOf<String, Any>() | ||
| var actionKeySet = false |
There was a problem hiding this comment.
actionKeySet is a "first-wins guard" that only protects the fallback ActionKeyProvider branch (the third when case)
| Priority | Branch | Behaviour |
|---|---|---|
| 1st | SubmitCollector / FlowCollector |
Always writes actionKey (no guard check) |
| 2nd | MetadataCollector |
Always writes actionKey (no guard check) |
| 3rd | Any other ActionKeyProvider (e.g. FIDO error) |
Writes actionKey only if not already set |
| is Submittable -> { | ||
| val eventType = it.eventType() | ||
| it.payload()?.let { | ||
| if (it.payload() != null) { |
There was a problem hiding this comment.
Any reason to make this change?
There was a problem hiding this comment.
I don't see this change anymore in my code.
| * @return the event type string, or null if no matching collector is found. | ||
| */ | ||
| internal fun Collectors.eventType(): String? { | ||
| // First pass: honor explicit Submit/Flow actions. |
There was a problem hiding this comment.
I believe this is same logic, but clearer
internal fun Collectors.eventType(): String? {
// First pass: honor explicit Submit/Flow actions.
forEach {
if ((it is SubmitCollector || it is FlowCollector) && it.payload() != null) {
return it.eventType()
}
}
// Second pass: fall back to any Submittable with a payload (e.g. FIDO errors).
forEach {
if (it is Submittable && it.payload() != null) {
return it.eventType()
}
}
return null
}
There was a problem hiding this comment.
Hey Andy, thanks for approving the PR. I asked Claude to give me a clarification on what the difference is between the proposed code and the current code.
- The current code
(actionKey != null)makes the intent explicit — first pass is about the user having made a selection, which is exactly what actionKey signals. - The proposed code
(payload() != null)is less intent-revealing — it could be confused with the second pass, which uses the same condition but on any Submittable.
The iOS PR specifically introduced actionKey on SubmitCollector/FlowCollector to make this distinction clear — checking actionKey != null is the idiomatic iOS way, and we should mirror that in Android for consistency.
4 participants
JIRA Ticket
SDKS-4477
Description
error alongside their payload.
Summary by CodeRabbit