Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 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 |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (77.22%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #70 +/- ##
============================================
+ Coverage 70.82% 71.61% +0.78%
Complexity 239 239
============================================
Files 208 170 -38
Lines 24057 22734 -1323
Branches 1175 818 -357
============================================
- Hits 17039 16280 -759
+ Misses 6845 6370 -475
+ Partials 173 84 -89
... and 39 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
rodrigoareis
left a comment
rodrigoareis
left a comment
There was a problem hiding this comment.
Changes looks good to me. Left some minor comments.
| * @param optionsKey Bridge payload key for the collector's WebAuthn options. | ||
| * @param options Transformed WebAuthn options. Always present on a live | ||
| * collector: the native collector sets the field synchronously inside `init()`, | ||
| * which throws `IllegalArgumentException` when the field is missing from the |
There was a problem hiding this comment.
It seems that exception isn't caught between the collector's init() and CoreRuntime's consumers — it propagates up through CollectorFactory.collector() and is only caught by Workflow's top-level catch { }, which turns the whole node into a FailureNode. So a malformed FIDO2 collector from the server fails the entire DaVinci step (not just the FIDO field), while iOS degrades gracefully (empty options, rest of the form still renders) since init(with:) there is non-throwing. Could we add a TODO-PARITY note here (like the ones already added for base64 encoding/trigger) so this asymmetry is tracked, and maybe file an SDK ticket with the Android FIDO team?
This is not something we need to fix in this PR, just want to make sure it's a known/accepted risk rather than a surprise later.
| */ | ||
| @Test | ||
| fun daVinciSerializerReturnsActionAndCreationOptionsForRegistration() { | ||
| RNPingFidoCommon.registerDaVinciSerializer() |
There was a problem hiding this comment.
Tests calls the real RNPingFidoCommon.registerDaVinciSerializer() but doesn't reset the one-shot registration guard in @Before/@After like RNPingFidoSerializerRegistrationTest does. Could we add RNPingFidoCommon.resetSerializerRegistrationForTesting() to a @Before/@After here too, so these tests don't implicitly depend on running before any other test class that already flips the guard?
| handlers.resolve(JsonBridgeMapper.encodeJsonObject(payload)) | ||
| case .failure(let error): | ||
| let nsError = error as NSError | ||
| let code = isRecoverableFidoAuthenticationFailure(nsError) |
There was a problem hiding this comment.
[iOS] User cancel rejects as FIDO_AUTHENTICATE_ERROR instead of FIDO_AUTHENTICATE_CANCELLED (blocking)
The DaVinci collector returns the transformed error from handleError() (FidoError.unsupportedAction("The operation was canceled.")), but isRecoverableFidoAuthenticationFailure only matches raw ASAuthorizationError 1001, so cancel never maps to the cancelled code. (Android is fine: its collector returns the raw exception.)
if let fidoError = error as? FidoError,
case .unsupportedAction(let message) = fidoError,
message == FidoConstants.ERROR_NOT_ALLOWED_MESSAGE {
return true // cancelled
}A test on this path would be good too.
…enticationCollector(SDKS-5302)
…en Android option serialization, and document error/parity gaps (SDKS-5302)
3 participants
Summary
Adds DaVinci passkey support to the React Native FIDO bridge. The
FIDO2DaVinci collector type is now serialized with its full payload on Android and iOS, andFidoClientgainsregisterForDaVinci/authenticateForDaVinciceremonies that run the native WebAuthn flow and submit natively through the collector when the DaVinci flow advances.Changes
fidoCollectorType('FIDO2') andFidoRegistrationCollector/FidoAuthenticationCollector, discriminated byaction: 'REGISTER' | 'AUTHENTICATE'.publicKeyCredentialCreationOptions/publicKeyCredentialRequestOptionspayloads and the Android-onlytriggerfield.FidoClient.registerForDaVinci()andFidoClient.authenticateForDaVinci(). They resolve the collector from a DaVinci instance, run the native ceremony, and return the informational attestation/assertion payload. Submission happens natively viacollector.payload()duringdaVinci.next({ collectors: [] }); the FIDO collector key must not be passed innext()input.indexto the DaVinci ceremony options for selecting among multiple FIDO2 collectors with the same action.createFidoClientregistersFIDO2with the integration collector registry in@ping-identity/rn-typesand eagerly registers the native DaVinci collector serializer, so nodes mapped before the first ceremony still carryactionand the WebAuthn options.key,type,action,label,required,trigger, pre-encoded WebAuthn options).registerForDaVinci/authenticateForDaVinciceremonies with foreground-activity and collector-resolution preconditions.FIDO_COLLECTOR_NOT_FOUND.@unchecked Sendable).@MainActorceremony methods with window-availability preconditions.{key, type}payload. This is the failure mode diagnosed in SDKS-5302, where top-level fields likeactionwere silently dropped.resetDaVinciCollectorSerializersForTestingseam to iOSCoreRuntimefor hermetic serializer tests.DaVinciFidoFieldrenderer with cancel detection,fidoCollectorTypeinhandledCollectorTypes, and associated-domain passkey configuration (Android Digital Asset Linksasset_statements, iOSwebcredentialsentitlements,ASSOCIATED_DOMAINenv).RNPingFidoTestceremony coverage.FidoErrorcode preservation.registerForDaVincithennext({ collectors: [] })submits without the FIDO key) and native-spec contract updates.handledCollectorTypes,indexsemantics) and add theFIDO2integration collector row and guidance to the rn-davinci README.Platform notes
triggerbecause the iOS SDK does not expose it; revisit after upgrading to native SDK 2.2 (tracked by a TODO infido.types.ts).Verification