Surface FIDO2 credential errors as DaVinci action events - #227
Closed
witrisna wants to merge 1 commit into
Closed
Conversation
Add a Failable interface so collectors can report a DOM/credential error alongside their payload. AbstractFidoCollector now maps CreateCredential/GetCredential exceptions (cancellation, unsupported, DOM errors) to their corresponding error names and switches its event type from "submit" to "action" when an error is present, allowing the Journey server to handle FIDO2 failures without treating them as a generic submit failure.
Contributor
📝 WalkthroughWalkthroughFIDO collectors now map failures to error codes, expose failure state through ChangesFIDO failure flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant FidoAuthenticationCollector
participant FidoClient
participant AbstractFidoCollector
participant Collectors
FidoAuthenticationCollector->>FidoClient: authenticate()
FidoClient-->>FidoAuthenticationCollector: Result failure
FidoAuthenticationCollector->>AbstractFidoCollector: handleError(exception)
AbstractFidoCollector-->>FidoAuthenticationCollector: mapped error code
Collectors->>AbstractFidoCollector: error()
AbstractFidoCollector-->>Collectors: action error
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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: 1
🧹 Nitpick comments (3)
davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt (1)
68-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid double-evaluation of
error().You can use
?.letto evaluate the error once and simplify the block safely.♻️ Proposed refactor
- if (it is Failable && it.error() != null) { - put("actionKey", it.error()) - } + if (it is Failable) { + it.error()?.let { error -> put("actionKey", error) } + }🤖 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 68 - 70, Update the Failable handling block in Collectors to evaluate it.error() only once, using a nullable-safe let-style flow to add the "actionKey" entry only when an error exists; preserve the current behavior for non-Failable values and null errors.mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt (2)
69-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale docstring.
The docstring still states the return is
"always \"submit\" for FIDO2 collectors", but the implementation now conditionally returns the action event type whenerroris set. Update the doc to reflect the new branching behavior.📝 Proposed fix
- /** - * Returns the event type that this collector handles. - * - * `@return` The event type string, always "submit" for FIDO2 collectors - */ + /** + * Returns the event type that this collector handles. + * + * `@return` "submit" when there is no error, or the action event type when an error has been recorded + */🤖 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 `@mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt` around lines 69 - 76, Update the KDoc for AbstractFidoCollector.eventType() to describe that it returns Constants.EVENT_TYPE_SUBMIT when error is null and Constants.EVENT_TYPE_ACTION when error is set, replacing the stale claim that it always returns "submit".
66-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestrict
errormutation tointernal.
erroris fully public and mutable, unlike the other collector properties (key,label,trigger,required) which all useprivate set. Since the only intended public contract is theFailable.error(): String?getter, external consumers outside this module can currently overwrite collector state directly. Kotlin test source sets can accessinternalmembers of the main source set by default, so this would not breakAbstractFidoCollectorTest.🔒 Proposed fix
- var error: String? = null + var error: String? = null + internal set🤖 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 `@mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt` around lines 66 - 67, Change the error property in AbstractFidoCollector from publicly mutable to internal-settable, preserving its public getter and existing Failable.error() contract. Keep internal collector code and tests able to update error while preventing external consumers from overwriting it.Source: Coding guidelines
🤖 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/FidoAuthenticationCollector.kt`:
- Around line 95-109: Clear the previous success payload at the start of each
retry: update FidoAuthenticationCollector.authenticate to reset assertionValue
alongside error, and update FidoRegistrationCollector’s corresponding
authentication flow to reset attestationValue alongside error. Apply the changes
at
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt
lines 95-109 and
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt
lines 68-82.
---
Nitpick comments:
In `@davinci/src/main/kotlin/com/pingidentity/davinci/collector/Collectors.kt`:
- Around line 68-70: Update the Failable handling block in Collectors to
evaluate it.error() only once, using a nullable-safe let-style flow to add the
"actionKey" entry only when an error exists; preserve the current behavior for
non-Failable values and null errors.
In
`@mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt`:
- Around line 69-76: Update the KDoc for AbstractFidoCollector.eventType() to
describe that it returns Constants.EVENT_TYPE_SUBMIT when error is null and
Constants.EVENT_TYPE_ACTION when error is set, replacing the stale claim that it
always returns "submit".
- Around line 66-67: Change the error property in AbstractFidoCollector from
publicly mutable to internal-settable, preserving its public getter and existing
Failable.error() contract. Keep internal collector code and tests able to update
error while preventing external consumers from overwriting it.
🪄 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: 4afdbc1d-aaa0-42e4-a8f5-33afefdb23c7
📒 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
Comment on lines
95
to
109
| suspend fun authenticate( | ||
| block: FidoAuthenticateCustomizer.() -> Unit = {} | ||
| ): Result<JsonObject> { | ||
| error = null | ||
| logger.d("Starting FIDO2 authentication") | ||
| return FidoClient { logger = this@FidoAuthenticationCollector.logger }.authenticate( | ||
| publicKeyCredentialRequestOptions, block | ||
| ).onSuccess { | ||
| logger.d("FIDO2 authentication successful") | ||
| assertionValue = it | ||
| }.onFailure { exception -> | ||
| handleError(exception) | ||
| logger.e("FIDO2 authentication failed", exception) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== Files ==\n'
git ls-files 'mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/*' 'mfa/**/Collectors*' 'mfa/**/collector*' | sed -n '1,120p'
printf '\n== Outline: FidoAuthenticationCollector.kt ==\n'
ast-grep outline mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt --view expanded || true
printf '\n== Outline: FidoRegistrationCollector.kt ==\n'
ast-grep outline mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt --view expanded || true
printf '\n== Search for payload/error/asJson ==\n'
rg -n "fun (payload|error|asJson)|payload\\(|error\\(" mfa -g '*.kt'Repository: ForgeRock/ping-android-sdk
Length of output: 7677
🏁 Script executed:
set -euo pipefail
printf '\n== Collectors-related file names ==\n'
fd -a 'Collectors|Collector' mfa -t f | sed -n '1,200p'Repository: ForgeRock/ping-android-sdk
Length of output: 1529
🏁 Script executed:
set -euo pipefail
printf '\n== Read targeted sections ==\n'
for f in \
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt \
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt
do
echo "--- $f ---"
nl -ba "$f" | sed -n '1,220p'
doneRepository: ForgeRock/ping-android-sdk
Length of output: 328
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt'),
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt'),
]:
print(f"\n== {path} ==")
for i, line in enumerate(path.read_text().splitlines(), 1):
if 1 <= i <= 220:
if any(k in line for k in ['assertionValue', 'attestationValue', 'error = null', 'payload(', 'error(', 'asJson']):
print(f"{i}: {line}")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 1001
🏁 Script executed:
set -euo pipefail
echo '## File list'
git ls-files 'mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/*' | sed -n '1,80p'
echo
echo '## Targeted lines in collectors'
for f in \
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt \
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt
do
echo "--- $f ---"
nl -ba "$f" | sed -n '1,220p' | sed -n '/error = null\|assertionValue\|attestationValue\|fun payload\|fun error\|fun authenticate\|fun register/p'
done
echo
echo '## Search for serializer and state plumbing'
rg -n "class Collectors|fun asJson|fun payload\\(|fun error\\(|assertionValue|attestationValue|error =" mfa -g '*.kt'
echo
echo '## Candidate serializer file(s)'
fd -a 'Collectors.kt' mfa -t f | sed -n '1,20p'Repository: ForgeRock/ping-android-sdk
Length of output: 746
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
targets = [
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt'),
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt'),
]
for path in targets:
print(f"\n== {path} ==")
text = path.read_text()
for needle in ['assertionValue', 'attestationValue', 'error = null', 'fun payload', 'fun error', 'asJson', 'data class', 'var ']:
if needle in text:
print(f"FOUND: {needle}")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 496
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt'),
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoCollector.kt'),
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt'),
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt'),
]
for path in paths:
print(f"\n== {path} ==")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(key in line for key in [
'class ', 'interface ', 'override fun payload', 'override fun error',
'fun asJson', 'error = null', 'assertionValue', 'attestationValue',
'close()', 'init(', 'register(', 'authenticate(',
]):
start = max(1, i - 5)
end = min(len(lines), i + 8)
print(f"\n-- around line {i} --")
for j in range(start, end + 1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 22016
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [
Path('mfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollectorTest.kt'),
Path('mfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollectorTest.kt'),
Path('mfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollectorTest.kt'),
]:
print(f"\n== {path} ==")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(k in line for k in ['payload', 'error', 'retry', 'close', 'init', 'assertNull', 'assertNotNull']):
start = max(1, i - 4)
end = min(len(lines), i + 4)
print(f"\n-- around line {i} --")
for j in range(start, end + 1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 50382
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
targets = [
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollector.kt'),
Path('mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoCollector.kt'),
Path('mfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollectorTest.kt'),
Path('mfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollectorTest.kt'),
Path('mfa/fido/src/test/kotlin/com/pingidentity/fido/davinci/AbstractFidoCollectorTest.kt'),
]
for path in targets:
print(f"\n== {path} ==")
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(k in line for k in [
'class ', 'interface ', 'fun asJson', 'override fun payload', 'override fun error',
'error = null', 'assertionValue', 'attestationValue', 'close()', 'retry',
'payload()', 'error()', 'init(', 'authenticate(', 'register(', 'assertNull', 'assertNotNull'
]):
start = max(1, i - 4)
end = min(len(lines), i + 5)
print(f"\n-- around line {i} --")
for j in range(start, end + 1):
print(f"{j:4}: {lines[j-1]}")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 50382
🏁 Script executed:
set -euo pipefail
rg -n "asJson|Collectors" mfa -g '*.kt'Repository: ForgeRock/ping-android-sdk
Length of output: 164
🏁 Script executed:
set -euo pipefail
printf '== asJson / payload / error search (repo-wide) ==\n'
rg -n --hidden --glob '*.kt' --glob '*.kts' --glob '*.java' \
'fun asJson|asJson\(|payload\(\)|error\(\)|eventType\(\)' .
printf '\n== Collector interface / serializer candidates ==\n'
rg -n --hidden --glob '*.kt' \
'interface Collector|class .*Collectors|object .*Collectors|Collectors' mfa .Repository: ForgeRock/ping-android-sdk
Length of output: 40916
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
import subprocess, shlex
patterns = [
r'fun asJson|asJson\(|payload\(\)|error\(\)|eventType\(\)',
r'interface Collector|class .*Collectors|object .*Collectors|Collectors',
]
for pat in patterns:
print(f"\n## pattern: {pat}")
p = subprocess.run(
["rg", "-n", "--hidden", "--glob", "*.kt", "--glob", "*.kts", "--glob", "*.java", pat, "."],
text=True, capture_output=True
)
lines = p.stdout.splitlines()
for line in lines[:120]:
print(line)
if len(lines) > 120:
print(f"... truncated {len(lines)-120} more lines")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 23969
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
import subprocess
queries = [
r'Collectors\.asJson|asJson\(',
r'field.*actionKey|actionKey',
r'formData|payload\(\).*error\(\)|error\(\).*payload\(\)',
]
for q in queries:
print(f"\n## {q}")
p = subprocess.run(["rg", "-n", "--hidden", "--glob", "*.kt", q, "."],
text=True, capture_output=True)
lines = p.stdout.splitlines()
for line in lines[:80]:
print(line)
if len(lines) > 80:
print(f"... truncated {len(lines)-80} more lines")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 16966
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
import subprocess
queries = [
r'formData',
r'actionKey',
r'payload\(\)',
r'eventType\(\)',
r'error\(\)',
]
for q in queries:
print(f"\n## {q}")
p = subprocess.run(
["rg", "-n", "--hidden", "--glob", "*.kt", q, "mfa"],
text=True, capture_output=True
)
lines = p.stdout.splitlines()
for line in lines[:80]:
print(line)
if len(lines) > 80:
print(f"... truncated {len(lines)-80} more lines")
PYRepository: ForgeRock/ping-android-sdk
Length of output: 6885
Clear the previous success payload before retrying
error = null leaves the last successful payload() in place, so a later failure can serialize stale formData together with actionKey in Collectors.asJson().
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt#L95-L109: clearassertionValuewitherror.mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt#L68-L82: clearattestationValuewitherror.
📍 Affects 2 files
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt#L95-L109(this comment)mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt#L68-L82
🤖 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
`@mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt`
around lines 95 - 109, Clear the previous success payload at the start of each
retry: update FidoAuthenticationCollector.authenticate to reset assertionValue
alongside error, and update FidoRegistrationCollector’s corresponding
authentication flow to reset attestationValue alongside error. Apply the changes
at
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoAuthenticationCollector.kt
lines 95-109 and
mfa/fido/src/main/kotlin/com/pingidentity/fido/davinci/FidoRegistrationCollector.kt
lines 68-82.
This was referenced Jul 21, 2026
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.
Add a Failable interface so collectors can report a DOM/credential
error alongside their payload. AbstractFidoCollector now maps
CreateCredential/GetCredential exceptions (cancellation, unsupported,
DOM errors) to their corresponding error names and switches its event
type from "submit" to "action" when an error is present, allowing the
Journey server to handle FIDO2 failures without treating them as a
generic submit failure.
JIRA Ticket
Please link jira ticket here
Description
Briefly describe the change and any information that would help speedup the review and testing.
Summary by CodeRabbit
New Features
Bug Fixes
Tests