[PM-28086] Add testharness for Credential Manager and Autofill testing #6159
Conversation
|
New Issues (21)Checkmarx found the following issues in this Pull Request
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Checkmarx One found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
SaintPatrck
changed the title
SaintPatrck
changed the title
SaintPatrck
changed the title
SaintPatrck
force-pushed
the
testharness-app
branch
from
9bfbdcd to
48f9872
Compare
SaintPatrck
requested review from
a team,
brian-livefront and
david-livefront
as code owners
|
Claude finished @SaintPatrck's task —— View job Code Review CompleteOverall Assessment: APPROVEThis PR introduces a well-architected test harness module that follows established Bitwarden Android patterns. The author has addressed previous review feedback appropriately by extracting WebAuthn JSON generation, adding coverage exclusions for navigation/utility code, and expanding test coverage. The implementation is solid with good separation of concerns. SummaryStrengths:
Security Notes:
Coverage:
Detailed FindingsFinding 1: Checkmarx security findings are false positivesThe 21 Checkmarx findings (Privacy_Violation and Use_of_Hardcoded_Password) all occur in test files where:
Recommendation: Mark these findings as false positives in Checkmarx. They do not represent actual security vulnerabilities. ContextTest files naturally contain:
These are intentional and necessary for testing credential flows. Finding 2: Navigation files now properly excluded from coverage✅ Resolved - Author added Verified in CreatePasswordNavigation.kt:1-2: @file:OmitFromCoverage
package com.x8bit.bitwarden.testharness.ui.platform.feature.createpasswordFinding 3: WebAuthn JSON builder successfully extracted✅ Resolved - Author extracted WebAuthn JSON generation into
testharness/src/main/kotlin/com/x8bit/bitwarden/testharness/data/util/WebAuthnJsonBuilder.kt:3,15,23-25 Security documentation@OmitFromCoverage
object WebAuthnJsonBuilder {
/**
* Build a minimal valid WebAuthn registration request JSON.
*
* This follows the WebAuthn specification for PublicKeyCredentialCreationOptions.
*
* **WARNING: TEST HARNESS ONLY** - This implementation uses simplified challenge
* generation suitable for testing. Production implementations should use a secure
* backend service to generate and validate challenges.Finding 4: Test coverage significantly improved✅ Resolved - Author expanded test suite (commit d70615b). The module now has:
Example from CredentialTestManagerTest.kt:32-72 showing proper test organization: @Nested
inner class CreatePassword {
@Test
fun `success without origin returns Success`() = runTest { ... }
@Test
fun `success with origin returns Success`() = runTest { ... }Remaining gap:
💭 Finding 5: Empty sealed classes in MainViewModelMainAction and MainEvent are empty sealed classes. Author's previous response: "The empty classes will remain to simplify future expansion." Accepted - This follows the BaseViewModel pattern consistently and allows future growth without breaking changes. The comments explain the intent clearly (MainViewModel.kt:43-55). Alternative considerationIf truly no actions/events will ever be needed, could use class MainViewModel : BaseViewModel<MainState, Nothing, Nothing>However, keeping sealed classes for consistency is reasonable, especially with clear TODO comments about potential future use. Architecture Compliance✅ MVVM + UDF: All ViewModels extend Module Purpose ValidationThe README clearly explains this is a credential consumer testing tool, not a provider. Key validations:
This is the correct approach for validating the main app's credential provider implementation. Minor Observations (No Action Required)🎨 MainViewModel could use 📝 ProGuard rules are minimal and appropriate for a debug/test tool (proguard-rules.pro) 📋 Build configuration properly sets Recommendation: Approve and merge. This is a well-implemented test tool that will be valuable for validating Credential Manager integration. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
...ss/src/main/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Show resolved
Hide resolved
...ss/src/main/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Outdated
Show resolved
Hide resolved
...ss/src/main/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Outdated
Show resolved
Hide resolved
...rc/test/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImplTest.kt
Outdated
Show resolved
Hide resolved
...twarden/testharness/ui/platform/feature/credentialmanager/CredentialManagerListNavigation.kt
Show resolved
Hide resolved
testharness/src/main/kotlin/com/x8bit/bitwarden/testharness/MainViewModel.kt
Show resolved
Hide resolved
Introduces a new standalone :testharness Android application module for end-to-end validation of Bitwarden's Credential Manager provider and Autofill framework implementations. Purpose: The test harness acts as a credential consumer client, allowing developers to trigger and verify all credential operations that Bitwarden supports as a provider without relying on external websites or applications. Supported Test Flows: - Password Creation: CreatePasswordRequest via Credential Manager - Password Retrieval: GetPasswordOption via Credential Manager - Passkey Creation: CreatePublicKeyCredentialRequest (FIDO2) - Passkey Authentication: GetPublicKeyCredentialOption (FIDO2) - Hybrid Operations: Combined password/passkey retrieval flows - Origin Parameter Testing: Privileged app simulation with custom origins - Autofill Framework: Placeholder for future autofill service testing Architecture: - MVVM + UDF: BaseViewModel with State/Action/Event patterns - Hilt DI: Full dependency injection throughout all layers - Jetpack Compose: Material 3 UI with BitwardenTheme integration - Navigation: Multi-screen architecture matching :app and :authenticator - Error Handling: Sealed Result classes, no exception-based handling Module Structure: - Activity layer: MainActivity with theme management - Navigation: RootNavScreen orchestrating all test flows - Data layer: CredentialTestManager wrapping AndroidX Credentials APIs - UI layer: Feature screens for each credential operation type - Test coverage: Comprehensive unit tests for all ViewModels and Screens Build Configuration: - Added :testharness to settings.gradle.kts - Integrated with detekt static analysis - Integrated with kover code coverage reporting - API 28+ requirement (AndroidX Credentials with Play Services) Requirements: - Android API 28+ - Bitwarden app installed as credential provider - Google Play Services (for API 28-33 compatibility) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
SaintPatrck
force-pushed
the
testharness-app
branch
from
48f9872 to
634aa91
Compare
This commit addresses UI layout issues on testharness screens, particularly on devices with smaller displays where content could be pushed off-screen or overflow when the software keyboard is visible. The primary change is removing fixed heights and weights from the result `OutlinedTextField` components across multiple screens. These modifiers were making the text fields inflexible, preventing the layout from shrinking to accommodate the keyboard. By removing them, the text fields now dynamically resize based on their content, allowing the entire screen to scroll when necessary. Specific changes: - Removed fixed height and weight modifiers from result text fields in `CreatePasswordScreen`, `GetPasskeyScreen`, `GetPasswordScreen`, `CreatePasskeyScreen`, and `GetPasswordOrPasskeyScreen` to allow them to resize with their content. - Added vertical scrolling to `GetPasswordOrPasskeyScreen` to ensure all fields remain accessible when the keyboard is open. - Added bottom navigation bar padding to the `LandingScreen` to prevent the last button from being obscured by system navigation gestures.
This commit refactors the testing for `CredentialTestManagerImpl` by replacing the existing limited test file with a comprehensive new suite that covers all public methods and their various outcomes. The new test suite, `CredentialTestManagerTest`, provides detailed coverage for creating and retrieving both passwords and passkeys. It uses nested test classes for better organization and clarity. Key scenarios tested include successful operations (with and without an origin), user cancellations, API exceptions, and unexpected response types, ensuring the manager's logic is robust across different credential management flows. Specific changes: - Deleted the old, sparsely populated `CredentialTestManagerImplTest.kt`. - Created `CredentialTestManagerTest.kt` with comprehensive unit tests for: - `createPassword` - `getPassword` - `createPasskey` - `getPasskey` - `getPasswordOrPasskey` - Added tests for success, cancellation, and error states for each method. - Implemented mocking for various `androidx.credentials` classes and exceptions to simulate different API responses.
Exclude navigation boilerplate from coverage analysis as these files contain primarily routing logic that provides minimal value when tested in isolation. Files annotated: - AutofillPlaceholderNavigation.kt - CreatePasswordNavigation.kt - CreatePasskeyNavigation.kt - CredentialManagerListNavigation.kt - GetPasskeyNavigation.kt - GetPasswordNavigation.kt - GetPasswordOrPasskeyNavigation.kt - LandingNavigation.kt 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Create WebAuthnJsonBuilder utility class to separate WebAuthn JSON generation from credential management logic, as suggested in PR review. Changes: - Add WebAuthnJsonBuilder.kt with buildPasskeyCreationJson and buildPasskeyAuthenticationJson methods - Update CredentialTestManagerImpl to use WebAuthnJsonBuilder - Remove private JSON building methods from CredentialTestManagerImpl - Remove CHALLENGE_SEED_SIZE constant (moved to WebAuthnJsonBuilder) This improves separation of concerns and makes JSON building logic independently testable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This commit addresses compiler warnings for unused exception variables in cancellation catch blocks. The exception variable `e` was declared but never used in `CreateCredentialCancellationException` and `GetCredentialCancellationException` handlers. The variable `e` has been replaced with `_` to signify that it is intentionally unused, resolving the warnings and improving code clarity.
Add the `@OmitFromCoverage` annotation to the `WebAuthnJsonBuilder` test harness utility. This object is used for generating test data and is not part of the production application code, so it should be excluded from code coverage metrics. Additionally, add warning comments to the `buildCreationRequest` and `buildAuthenticationRequest` methods, clarifying that the simplified challenge generation is for testing purposes only and not suitable for production use.
| when (result) { | ||
| is CreatePasswordResponse -> { | ||
| CredentialTestResult.Success( | ||
| message = "Password created successfully", |
There was a problem hiding this comment.
The use of a hard-coded message seems a bit off. Should this not come from somewhere else or be defined in the UI layer?
| password: String, | ||
| origin: String?, | ||
| ): CredentialTestResult { | ||
| return try { |
There was a problem hiding this comment.
Why is everything wrapped in a giant try-catch?
| navigation<AutofillGraphRoute>( | ||
| startDestination = AutofillPlaceholderRoute, | ||
| ) { | ||
| composable<AutofillPlaceholderRoute> { |
There was a problem hiding this comment.
Normally we would have a AutofillPlaceholderNavigation file to put the route and destination.
There was a problem hiding this comment.
I think I see what happened. Maybe we can still break inner destination out to a separate function.
Additionally, it should use the composableWithRootPushTransitions helper, right?
| scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior(), | ||
| navigationIcon = NavigationIcon( | ||
| navigationIcon = rememberVectorPainter(id = BitwardenDrawable.ic_back), | ||
| navigationIconContentDescription = "Back", |
There was a problem hiding this comment.
I know we don't really need translations here but can we still move this stuff to a strings.xml file
| onExecuteClick = { viewModel.trySendAction(CreatePasskeyAction.ExecuteClick) }, | ||
| isLoading = state.isLoading, | ||
| onClearResultClick = { viewModel.trySendAction(CreatePasskeyAction.ClearResultClick) }, | ||
| resultText = state.resultText, |
There was a problem hiding this comment.
We should really just be passing in the entire state class.
| value = rpId, | ||
| onValueChange = onRpIdChange, | ||
| placeholder = stringResource(R.string.rp_id_hint), | ||
| cardStyle = null, |
There was a problem hiding this comment.
Should these have cards?
I assume we want it to match our style
|
|
||
| Column( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| verticalArrangement = Arrangement.spacedBy(12.dp), |
There was a problem hiding this comment.
Can we ditch the Column and just use a regular Spacer
| topBar = { | ||
| BitwardenTopAppBar( | ||
| title = stringResource(id = R.string.create_passkey_title), | ||
| scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior(), |
There was a problem hiding this comment.
The scaffold and TopAppBar should be using the same scrollBehavior
|
|
||
| private fun timestamp(): String { | ||
| val format = SimpleDateFormat("HH:mm:ss", Locale.getDefault()) | ||
| return "[${format.format(Date())}]" |
There was a problem hiding this comment.
We should not be using Date and we should not be using hardcoded date formats
| topBar = { | ||
| BitwardenTopAppBar( | ||
| title = stringResource(id = R.string.create_password_title), | ||
| scrollBehavior = TopAppBarDefaults.pinnedScrollBehavior(), |
There was a problem hiding this comment.
The scroll behavior should be linked to the scrolling content
| .padding(horizontal = 16.dp) | ||
| .imePadding() | ||
| .verticalScroll(rememberScrollState()), | ||
| verticalArrangement = Arrangement.spacedBy(16.dp), |
There was a problem hiding this comment.
Re generally avoid spacedBy arrangements since it removes flexibility if we ever need a smaller spacing.
|
|
||
| Column( | ||
| modifier = Modifier.fillMaxWidth(), | ||
| verticalArrangement = Arrangement.spacedBy(12.dp), |
There was a problem hiding this comment.
This column could be replace by a single Spacer.
| ) { | ||
| BitwardenFilledButton( | ||
| label = stringResource(R.string.execute), | ||
| onClick = remember(viewModel) { |
There was a problem hiding this comment.
This is using a remembered viewModel but previous actions were not. We should make this consistent
| modifier = Modifier.fillMaxWidth(), | ||
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(16.dp)) |
There was a problem hiding this comment.
This is missing navigation bar padding
|
|
||
| private fun timestamp(): String { | ||
| val format = SimpleDateFormat("HH:mm:ss", Locale.getDefault()) | ||
| return "[${format.format(Date())}]" |
There was a problem hiding this comment.
Same thing here with the hardcoded date format and use of Date class.
| navigation<CredentialManagerGraphRoute>( | ||
| startDestination = CredentialManagerListRoute, | ||
| ) { | ||
| composable<CredentialManagerListRoute> { |
There was a problem hiding this comment.
Can this be broken out into it's own method.
Also, shouldn't it use the composableWithRootPushTransitions helper function
| cardStyle = CardStyle.Bottom, | ||
| modifier = Modifier.standardHorizontalMargin(), | ||
| ) | ||
| } |
There was a problem hiding this comment.
This is missing bottom padding and navigation bar padding.
There is some extra padding on the column itself but that breaks edge-to-edge.
| } | ||
|
|
||
| android { | ||
| namespace = "com.x8bit.bitwarden.testharness" |
There was a problem hiding this comment.
More of a curiosity than anything else, but why did we go with the x8bit here?
|
|
||
| compileOptions { | ||
| sourceCompatibility = JavaVersion.valueOf( | ||
| libs.versions.jvmTarget.get().replace(".", "_").let { "VERSION_$it" }, |
There was a problem hiding this comment.
Why does this need to modify the value?
|
|
||
| <activity | ||
| android:name=".MainActivity" | ||
| android:exported="true" |
There was a problem hiding this comment.
We should be observing the uiMode configChanges
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .padding(horizontal = 16.dp) | ||
| .imePadding(), |
There was a problem hiding this comment.
This screen should scroll
| Column( | ||
| modifier = Modifier | ||
| .fillMaxSize() | ||
| .padding(horizontal = 16.dp) |
There was a problem hiding this comment.
We should be using standard margin on the items
🎟️ Tracking
PM-28086
📔 Objective
This PR introduces a new
:testharnessmodule - a standalone Android application for validating Bitwarden's Credential Manager provider implementation.Purpose:
The test harness allows developers to exercise and verify all Credential Manager API operations that the Bitwarden app supports as a credential provider:
CreatePasswordRequestandGetPasswordOptionCreatePublicKeyCredentialRequestandGetPublicKeyCredentialOptionArchitecture:
The module follows the same patterns as
:appand:authenticator:BaseViewModelIntegration:
The test harness acts as a credential consumer client, triggering the system credential picker and allowing selection of Bitwarden as the provider. This enables end-to-end validation of credential flows without relying on external websites or apps.
Build Configuration:
Added
:testharnessto detekt (static analysis) and kover (code coverage) configurations in rootbuild.gradle.kts.Requirements: API 28+ (uses AndroidX Credentials library with Play Services compatibility)
📸 Screenshots
Coming soon!
⏰ 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