Improve CXF message handling #5982
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
SaintPatrck
force-pushed
the
cxf/app/integration-fixes
branch
from
7e8e615 to
6e922c9
Compare
SaintPatrck
requested review from
a team,
brian-livefront and
david-livefront
as code owners
SaintPatrck
force-pushed
the
cxf/app/integration-fixes
branch
from
6e922c9 to
bd7a2e9
Compare
| ImportCredentialsInvalidJsonException("Invalid CXP JSON."), | ||
| ) | ||
|
|
||
| if (SUPPORTED_CXP_FORMAT_VERSIONS[credentialExchangeExportResult.version.major] |
There was a problem hiding this comment.
How does this work exactly, the major version contains a set of supported minor versions?
There was a problem hiding this comment.
Correct. This may change later, once we determine how much of the validation and payload mutation we can offload to the SDK. IMO, the SDK should be handling all of this, but it only accepts the account JSON at the moment. 😞
| .Success(itemCount = cipherList.size) | ||
| .asSuccess() | ||
| val accountsJson = try { | ||
| json.encodeToString(exportResponse.accounts.firstOrNull()) |
There was a problem hiding this comment.
Is it OK to encode null here?
There was a problem hiding this comment.
We could check it's not empty and exit early. This effectively achieves the same because we're catching SerializationExceptions and propagating it to the caller.
There was a problem hiding this comment.
Parsing null will not throw, it will make the string null
There was a problem hiding this comment.
Ah. True. I'll check if it's empty first, in that case.
| val username: String, | ||
| val email: String, | ||
| val collections: JsonArray, | ||
| val items: JsonArray, |
There was a problem hiding this comment.
Do we not care about modeling these?
There was a problem hiding this comment.
No. We have to extract a single "account" JSON object from the array because the SDK only consumes a single account (for now). We don't really care about the actual properties, just need the JSON. This seems like the better approach so that we don't have to continue updating the collections and items models as the Spec changes.
This commit implements the FIDO Credential Exchange (CXF) export message formats. It introduces the necessary data models for the CXF protocol, including `CredentialExchangeProtocolMessage`, `CredentialExchangeExportResponse`, and `CredentialExchangeVersion`. The `CredentialExchangeCompletionManager` is updated to build and encode the export response according to the CXF specification, including the header and base64-encoded payload. The import logic in `CredentialExchangeImportManager` is also updated to parse this new format, validate versioning, and decode the payload before processing.
SaintPatrck
force-pushed
the
cxf/app/integration-fixes
branch
from
bd7a2e9 to
8919d3c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5982 +/- ##
=======================================
Coverage 84.54% 84.54%
=======================================
Files 717 717
Lines 54649 54708 +59
Branches 7509 7517 +8
=======================================
+ Hits 46201 46255 +54
- Misses 5818 5823 +5
Partials 2630 2630 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
PM-26599
📔 Objective
Improve handling of the FIDO Credential Exchange (CXF) message formats.
It introduces the necessary data models for the CXF protocol, including
CredentialExchangeProtocolMessage,CredentialExchangeExportResponse, andCredentialExchangeVersion.The
CredentialExchangeCompletionManageris updated to build and encode the export response according to the CXF specification, including the header and base64-encoded payload. The import logic inCredentialExchangeImportManageris also updated to parse this new format, validate versioning, and decode the payload before processing.📸 Screenshots
Import to Bitwarden
Screen_recording_20251007_095046.mp4
Import from Bitwarden
Screen_recording_20251007_095005.mp4
⏰ 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