[PM-30703] Introduce CXF payload parser and update to alpha05#6347
[PM-30703] Introduce CXF payload parser and update to alpha05#6347SaintPatrck merged 5 commits intomainfrom
Conversation
github-actions
bot
added
app:password-manager
|
Great job! No new security vulnerabilities introduced in this pull request |
cxf/src/main/kotlin/com/bitwarden/cxf/parser/CredentialExchangePayloadParserImpl.kt
Outdated
Show resolved
Hide resolved
SaintPatrck
force-pushed
the
cred-exchange/dep-updates
branch
from
037610f to
80720fb
Compare
|
Claude finished @SaintPatrck's task in 5m 42s —— View job Code Review: PR #6347 - PM-30703 Introduce CXF payload parser and update to alpha05
SummaryThis PR cleanly updates the Credential Exchange implementation from alpha03 to alpha05, removing the two-layered CXP/CXF structure in favor of a direct JSON payload. The new Findings🎨 Suggestion: The 🤔 Discussion: JSON string interpolation in The export response JSON is constructed via string interpolation: "exporterRpId": "${activity.packageName}",
"exporterDisplayName": "${activity.applicationInfo.name}",
...
"accounts": [${exportResult.payload}]While 👍 Well done: Multi-account processing — The val allCiphers = accountsJsonList.flatMap { accountJson ->
vaultSdkSource
.importCxf(userId = userId, payload = accountJson)
.getOrElse { return ImportCxfPayloadResult.Error(error = it) }
}The early return on first failure is a reasonable strategy for this use case. 👍 Test coverage — Parser tests cover valid payloads, multi-account, empty accounts, unsupported versions, invalid JSON, and serialization errors. Import manager tests cover multi-account aggregation, partial failure (second account), policy filtering, and all result paths. The 👍 Overall this is a solid PR. The simplification from the two-layered structure to direct JSON is a welcome change, and the parser abstraction makes the import manager much cleaner. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
cxf/src/main/kotlin/com/bitwarden/cxf/parser/CredentialExchangePayloadParserImpl.kt
Outdated
Show resolved
Hide resolved
cxf/src/main/kotlin/com/bitwarden/cxf/parser/CredentialExchangePayloadParserImpl.kt
Outdated
Show resolved
Hide resolved
| } else { | ||
| cipherList | ||
| } | ||
| .map { syncVault(it) } |
There was a problem hiding this comment.
The uploadCiphers function converts specific ImportCiphersResponseJson.Invalid responses into a generic ImportCredentialsUnknownErrorException, losing valuable error context.
Problems:
- Lost validation errors:
Invalidcontains avalidationErrorsmap with specific field-level errors, but this is completely discarded - Debugging difficulty: Support and developers lose insight into why the import failed on the server side
- User experience: Generic errors provide no actionable feedback
Recommended fix:
is ImportCiphersResponseJson.Invalid -> {
// Log validation errors for debugging
Timber.w("Import ciphers validation failed: ${response.validationErrors}")
Result.failure(
ImportCredentialsUnknownErrorException(
"Server validation failed: ${response.validationErrors.keys.joinToString()}"
)
)
}Alternatively, consider creating a custom exception type that preserves the validation errors for upstream handling.
SaintPatrck
force-pushed
the
cred-exchange/dep-updates
branch
4 times, most recently
from
b5cb0dd to
9de5ff0
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6347 +/- ##
=======================================
Coverage 86.34% 86.34%
=======================================
Files 794 792 -2
Lines 56782 56712 -70
Branches 8213 8214 +1
=======================================
- Hits 49028 48968 -60
+ Misses 4899 4889 -10
Partials 2855 2855 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
SaintPatrck
force-pushed
the
cred-exchange/dep-updates
branch
from
9de5ff0 to
d71150d
Compare
…d parser
This commit updates the Credential Exchange (CXF) implementation to align with the `androidx.credentials.providerevents:1.0.0-alpha04` library. The primary objective is to adapt to the breaking changes introduced in this version, which simplifies the API and removes the custom JSON request/response structure in favor of direct parameters.
A key part of this refactor is the introduction of a dedicated `CredentialExchangePayloadParser`. This parser is responsible for validating and processing the incoming JSON payload from the exporting credential manager.
Behavioral changes:
- The structure of the `ImportCredentialsRequest` sent to the credential provider has changed. It no longer uses a custom JSON string but instead passes `credentialTypes` and `knownExtensions` directly.
- The response from the credential provider is no longer a nested JSON structure with a Base64 encoded payload. It is now a direct JSON object representing the exported data.
Specific changes:
- **Dependencies**:
- Upgraded `androidx.credentials.providerevents` to `1.0.0-alpha04`.
- Added Hilt dependencies to the `cxf` module for dependency injection.
- **CXF Payload Parsing**:
- Created `CredentialExchangePayloadParser` interface and its `CredentialExchangePayloadParserImpl` implementation to handle the parsing of incoming CXF JSON data.
- Introduced a `CredentialExchangePayload` sealed class to represent the different outcomes of parsing: `Importable`, `NoItems`, and `Error`.
- Added a `CxfModule` to provide the parser implementation via Hilt.
- Added comprehensive unit tests for `CredentialExchangePayloadParserImpl` to cover valid payloads, version checks, invalid JSON, and error conditions.
- **Import Logic**:
- Refactored `CredentialExchangeImporterImpl` to use the new `ImportCredentialsRequest` constructor, passing `credentialTypes` and `knownExtensions` directly instead of building a JSON string.
- Updated `CredentialExchangeImportManagerImpl` to use the new `CredentialExchangePayloadParser`. The manager now delegates payload validation and parsing, simplifying its own logic to focus on the import, upload, and sync process.
- Removed the now-obsolete manual parsing of the two-layered CXP/CXF JSON structure and Base64 decoding.
- **Export Logic**:
- In `CredentialExchangeRegistryImpl`, added the `exportMatcher` WASM binary required by the `alpha04` API when registering an export flow.
- Simplified `CredentialExchangeCompletionManagerImpl` to directly return the export data as a JSON string, removing the previous logic that wrapped it in a `CXP` protocol message with Base64 encoding.
- **Testing**:
- Updated unit tests across `app` and `cxf` modules (`CredentialExchangeImporterTest`, `MainViewModelTest`, `CredentialExchangeImportManagerTest`, etc.) to reflect the API changes in `ImportCredentialsRequest` and the new parsing flow.
- Removed outdated test logic related to the old JSON structure and Base64 decoding.
Log validation errors from ImportCiphersResponseJson.Invalid before returning the failure, preserving debugging context. Co-Authored-By: Claude <noreply@anthropic.com>
Update the CXF payload parser and import manager to process all accounts in a credential exchange payload instead of only the first. Each account is serialized individually and passed to the SDK, with ciphers aggregated into a single batch upload. Import fails fast if any account's SDK call fails. Also bumps credentials provider events dependency to alpha05. Co-Authored-By: Claude <noreply@anthropic.com>
SaintPatrck
force-pushed
the
cred-exchange/dep-updates
branch
from
d71150d to
112204e
Compare
cxf/src/main/kotlin/com/bitwarden/cxf/registry/CredentialExchangeRegistryImpl.kt
Outdated
Show resolved
Hide resolved
cxf/src/test/kotlin/com/bitwarden/cxf/importer/CredentialExchangeImporterTest.kt
Show resolved
Hide resolved
Close InputStream from openRawResource() using .use {} to prevent
resource leaks. Add test verifying ImportCredentialsException catch
path preserves the original exception.
Co-Authored-By: Claude <noreply@anthropic.com>
SaintPatrck
changed the title
| CredentialExchangePayload.Importable( | ||
| accountsJsonList = accountsJsonList, | ||
| ) | ||
| } catch (_: SerializationException) { |
There was a problem hiding this comment.
Should we log this for the flight recorder?
There was a problem hiding this comment.
Added a log statement. I'm not comfortable passing the exception to Timber since it could contain sensitive details.
There was a problem hiding this comment.
Ohh, that makes sense
| vaultSyncManager = vaultSyncManager, | ||
| policyManager = policyManager, | ||
| json = json, | ||
| credentialExchangePayloadParser = credentialExchangePayloadParser, |
| userId: String, | ||
| accountsJsonList: List<String>, | ||
| ): ImportCxfPayloadResult { | ||
| val allCiphers = mutableListOf<Cipher>() |
There was a problem hiding this comment.
Instead of the mutable list can we do something like this?
val allCiphers = accountsJsonList.flatMap { accountJson ->
vaultSdkSource
.importCxf(userId = userId, payload = accountJson)
.getOrElse { return ImportCxfPayloadResult.Error(error = it) }
}This commit refactors the credential exchange import process for improved conciseness and adds error logging for better diagnostics. There are no behavioral changes for the end-user. **Changes:** * In `CredentialExchangeImportManagerImpl`, the `for` loop used for importing CXF payloads has been replaced with a more idiomatic Kotlin `flatMap` operation. This simplifies the code by removing the mutable list and the explicit fold operation. * In `CredentialExchangePayloadParserImpl`, an error log using `Timber.e` has been added to the catch block for `SerializationException`. This will log when the app fails to serialize received accounts during the credential exchange process, aiding in future debugging efforts.
|
Thanks @david-livefront |
2 participants
🎟️ Tracking
PM-30703
📔 Objective
Update the Credential Exchange (CXF/CXP) implementation to align with the
androidx.credentials.providerevents:1.0.0-alpha04library. The previous implementation was based onalpha03and is now obsolete.The primary behavioral change is the removal of the two-layered JSON structure (CXP wrapping CXF). The
alpha04specification simplifies this to a single, direct JSON payload for credential exchange.Specific changes include:
Dependency Update:
androidx.credentials.providereventsfrom1.0.0-alpha03to1.0.0-alpha05.Payload Parsing:
CredentialExchangePayloadParserto handle the new direct JSON payload structure. This parser validates the CXF version, checks for the presence of accounts, and handles various error conditions.CredentialExchangeImportManageris refactored to use this new parser, removing the now-unnecessary logic for decoding the two-layered, base64-encoded payload.Import/Export Flow:
CredentialExchangeImporterImpl: Updated to use the modernImportCredentialsRequestbuilder, which no longer requires manual JSON construction.CredentialExchangeCompletionManagerImpl: Modified to return the new, simplified direct JSON response, removing the CXP wrapper and base64 encoding.CredentialExchangeRegistryImpl: Aligned withalpha04by including a defaultexport_matcher.bin(a WASM binary from Google) in the registration request.DI and Module Structure:
CxfModuleto provide theCredentialExchangePayloadParser.build.gradle.ktsfor thecxfmodule to include Hilt dependencies for DI.Testing:
CredentialExchangePayloadParserto cover valid payloads, version checks, and various error states.CredentialExchangeImporterTestandCredentialExchangeImportManagerTestto reflect the API and structural changes from the library update.⏰ 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