[PM-24258] Building a specific Fido2AttestationResponse to work with Binance #5986
Conversation
|
Fixed Issues (2)Great job! The following issues were fixed in this Pull Request
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5986 +/- ##
========================================
Coverage 84.58% 84.59%
========================================
Files 718 722 +4
Lines 54618 54882 +264
Branches 7521 7569 +48
========================================
+ Hits 46199 46427 +228
- Misses 5787 5805 +18
- Partials 2632 2650 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aj-rosado
requested review from
a team,
brian-livefront and
david-livefront
as code owners
| * Converts the SDK attestation response to a [Fido2AttestationResponse] that can be serialized into | ||
| * the expected system JSON. | ||
| */ | ||
| @Suppress("MaxLineLength") |
There was a problem hiding this comment.
Can we drop this suppression now?
| val publicKey: String? = null, | ||
| @SerialName("authenticatorData") | ||
| val authenticatorData: String?, | ||
| val authenticatorData: String? = null, |
There was a problem hiding this comment.
Could we leave this unchanged and add the explicit nulls for the Binance case.
This should be temporary and we only need these default values in this one scenario
| callingPackageName: String?, | ||
| ): Fido2AttestationResponse { | ||
| val registrationResponse = if (callingPackageName == BINANCE_PACKAGE_NAME) { | ||
| // This is a special case only necessary for Binance. |
There was a problem hiding this comment.
Can we generate a ticket to track that this needs to be removed eventually?
Only setting transport as null on Binance flow as it is the only necessary data to be null
| transports = null, | ||
| publicKeyAlgorithm = response.publicKeyAlgorithm, | ||
| publicKey = response.publicKey?.base64EncodeForFido2Response(), | ||
| authenticatorData = response.authenticatorData.base64EncodeForFido2Response(), |
There was a problem hiding this comment.
Based on the previous code, I thought more of this data would be null. Has something changed?
There was a problem hiding this comment.
Binance only complains with the extra transports, but only needs the clientDataJson and attestationObject.
publicKeyAlgorithm, publicKey, authenticatorData are present inside the attestationObject and they are only needed here for chromium browsers. For some reason chromium browsers won't work without any of these fields.
Both chromium and firefox needs transports, it is not present on the attestationObject.
I had only tested previously with clientDataJson, attestationObject and transports as all other fields are in attestation. But publicKeyAlgorithm is not nullable so gave it a try and it works as well, with all other fields also work, so seems that is only an issue with transports.
To keep the changes minimal, despite not necessary I am sending all the other data as well and just nulling transports
There was a problem hiding this comment.
If it's just this one property, can we simplify this?
Fido2AttestationResponse.RegistrationResponse(
clientDataJson = response.clientDataJson.base64EncodeForFido2Response(),
attestationObject = response.attestationObject.base64EncodeForFido2Response(),
transports = response.transports.takeUnless {
// Setting transports as null, otherwise Binance labels the passkey broken
// PM-26734 remove this flow if not necessary anymore
callingPackageName == BINANCE_PACKAGE_NAME
},
publicKeyAlgorithm = response.publicKeyAlgorithm,
publicKey = response.publicKey?.base64EncodeForFido2Response(),
authenticatorData = response.authenticatorData.base64EncodeForFido2Response(),
)There was a problem hiding this comment.
yes that looks better
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-24258
📔 Objective
When creating a passkey on Binance it is being labeled as "Broken".
Sending a sanitized
Fido2AttestationResponsewith theresponseobject without thetransportson the Binance flow fixes that.fixes #5608
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes