[PM-24148] add credential manager provider for create passwords #5579
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Nailik
requested review from
a team,
brian-livefront and
david-livefront
as code owners
Contributor
Author
|
@SaintPatrck fyi i already worked on creating password entries via credential manager but there are still some doings. This is only to prevent others from working on this simultaneously. |
Contributor
|
Sounds good. Can you convert this to a draft until you're ready for an initial review? |
|
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
Nailik
force-pushed
the
feature/add-password-credential-manager-provider-create
branch
from
607b7e7 to
1a61ce7
Compare
…universal Credential intent and functions
…le password requests
… fully implemented)
|
|
||
| val password = createPasswordCredentialRequest | ||
| ?.password | ||
| .orEmpty() |
Collaborator
There was a problem hiding this comment.
So the username and password is allowed to be empty?
Contributor
Author
There was a problem hiding this comment.
password is null for fido2 requests. and for fido2 requests there might be no username. that's why both could be empty.
| ) | ||
| return true | ||
| } | ||
| return false |
Collaborator
There was a problem hiding this comment.
Can we make this a single top-level return?
| when (providerRequest.callingRequest) { | ||
| is CreatePublicKeyCredentialRequest -> registerFido2CredentialToCipher( | ||
| cipherView, | ||
| providerRequest, |
Collaborator
There was a problem hiding this comment.
Can we add some names here?
| verifyActivityResultIsSetAndFinishedAfter { | ||
| PendingIntentHandler.setCreateCredentialResponse( | ||
| any(), | ||
| any<CreatePasswordResponse>(), |
Collaborator
There was a problem hiding this comment.
Can we add names here
Remove the `PasswordRegisterResult` sealed class from the credentials data model. This class is no longer used in the codebase. Its removal cleans up the project structure by eliminating dead code, including the `Success` object and `Error` sealed hierarchy (specifically `InternalError`).
Specify the return type `List<CreateEntry>` for the `toCreatePasskeyEntry` extension function on `List<UserState.Account>`. This change adds type clarity to the function signature in `CredentialProviderProcessorImpl`, making the contract of the function explicit rather than inferred.
Refactor the handling of `SpecialCircumstance.ProviderCreateCredential` in `RootNavViewModel` to improve readability and flow control.
The logic previously relied on a chain of safe calls (`?.let`) and the elvis operator (`?:`), which made the conditional branching less clear. This update introduces local variables for the request components and replaces the chaining with a structured `when` expression to explicitly handle public key and password credential requests.
Specific changes:
- Extract `createCredentialRequest`, `createPublicKeyCredentialRequest`, and `createPasswordCredentialRequest` into local variables for better readability.
- Replace the `?.let { ... } ?: ...` chain with a `when` block that checks for non-null `publicKeyRequest` and `passwordRequest` separately.
- Maintain the existing `IllegalStateException` for cases where neither request type is present.
…nsions.kt` Refactor the `rpName` variable assignment to improve code readability and adherence to Kotlin coding conventions. The elvis operator logic for determining the Relying Party (RP) name has been split across multiple lines. This visual separation clarifies the fallback mechanism, where `callingAppInfo.packageName` is used if `attestationOptions?.relyingParty?.name` is null.
Update the `hasValidationErrors` method in `VaultAddEditViewModel.kt` to use an expression body syntax. This refactoring streamlines the conditional logic by returning the result of the `if-else` block directly, rather than using multiple explicit `return` statements. This change improves code readability and conciseness without altering the underlying validation behavior.
Update `VaultItemListingViewModel` and `CredentialProviderCompletionManagerTest` to use named arguments in function calls, improving code readability. - In `VaultItemListingViewModel`, explicitly name the `cipherView` and `providerRequest` arguments when calling `registerFido2CredentialToCipher`. - In `CredentialProviderCompletionManagerTest`, explicitly name the `intent` and `response` arguments when verifying `PendingIntentHandler.setCreateCredentialResponse`.
Refactor the credential result model to align naming conventions and improve class hierarchy. This change renames `RegisterCredentialResult` to `CreateCredentialResult` and groups success states under a sealed subclass. - Rename `RegisterCredentialResult` to `CreateCredentialResult` throughout the codebase. - Refactor the class structure to nest success states under `CreateCredentialResult.Success`. - Map `SuccessFido2` to `CreateCredentialResult.Success.Fido2CredentialRegistered`. - Map `SuccessPassword` to `CreateCredentialResult.Success.PasswordCreated`. - Update `CredentialProviderCompletionManager` and its implementations to handle the new `CreateCredentialResult` types. - Update usage in `VaultAddEditViewModel` and `VaultItemListingViewModel`. - Update all corresponding unit tests to reflect the class renaming and structure changes.
Expand test coverage for `CredentialProviderProcessor` in `CredentialProviderProcessorTest.kt`, focusing on the `processCreateCredentialRequest` method. - Rename the existing error handling test to `processCreateCredentialRequest should invoke callback with error on password create request when userState is null` to explicitly state the test condition. - Add `processCreateCredentialRequest should invoke callback with result on password create request with valid userState` to verify the success flow. - Add `processCreateCredentialRequest should generate correct password entries based on state` to validate that entries are correctly populated with `lastUsedTime` and `biometricPromptData` based on the cipher availability and Android version. - Add tests to ensure biometric data is excluded on pre-Android V devices and when the vault is locked.
Update the `BitwardenOverwriteCredentialConfirmationDialog` composable signature to require a non-nullable `title`. - Change the `title` parameter type from `String?` to `String` to enforce the presence of a title in the overwrite confirmation dialog.
Remove the `getCreatePasswordCredentialRequestOrNull` extension function from `ProviderCreateCredentialRequestExtensions.kt`. This function was no longer being used in the codebase and its removal cleans up the utility class. The corresponding import for `androidx.credentials.CreatePasswordRequest` was also removed.
Update the dialog state initialization in `VaultItemListingViewModel` to correctly check for the presence of a public key credential request. The condition for showing the loading dialog state has been narrowed. Previously, it checked if `providerCreateCredentialRequest` was not null. Now, it explicitly checks if `providerCreateCredentialRequest.createPublicKeyCredentialRequest` is not null before setting the state to `Loading`. This ensures the loading dialog only appears when a public key credential request specifically is active, preventing potential incorrect state display.
Add test coverage for the navigation logic when a `ProviderCreateCredentialRequest` containing a `CreatePasswordRequest` occurs while the user's vault is unlocked. - Update `RootNavViewModelTest` to verify that the ViewModel emits `RootNavState.VaultUnlockedForCreatePasswordRequest` with the extracted username, password, and URI when processing this request type. - Update `RootNavScreenTest` to ensure that the `VaultUnlockedForCreatePasswordRequest` state triggers the correct navigation sequence: first to `VaultUnlockedGraphRoute`, and then to `VaultAddEditRoute` configured for adding a login.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavViewModelTest.kt
Fixed
Show fixed
Hide fixed
Resolve a variable name shadowing issue in `RootNavViewModelTest` to improve code clarity and prevent potential ambiguity during test execution. The local variables `username`, `password`, and `packageName` were previously shadowing properties of the mocked objects within the `ProviderCreateCredentialRequest` block. This change renames these local test variables to `mockUsername`, `mockPassword`, and `mockPackageName` to clearly distinguish between the test data and the object properties being mocked or asserted against.
app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/rootnav/RootNavViewModelTest.kt
Dismissed
Show dismissed
Hide dismissed
| ) | ||
|
|
||
| else -> | ||
| showCredentialManagerErrorDialog( |
Collaborator
There was a problem hiding this comment.
Can we wrap these in curly braces since they are multiple lines.
|
|
||
| is CreatePasswordRequest -> observeVaultData() | ||
|
|
||
| else -> mutableStateFlow.update { |
Collaborator
There was a problem hiding this comment.
This guy should also have curly braces
Update the `when` expressions inside `VaultItemListingViewModel` to wrap their branches in explicit block bodies `{ ... }`.
This change improves code consistency and readability within the `registerCredentialToCipher` and `handleCreateCredentialRequestReceive` functions by ensuring all `when` branches, including single-line calls and multiline lambdas, utilize braces. This formatting adjustment makes the structure uniform across different request handling scenarios.
david-livefront
approved these changes
Nov 19, 2025
SaintPatrck
approved these changes
Nov 19, 2025
Contributor
Author
|
Very nice, thank you guys! |
Nailik
deleted the
feature/add-password-credential-manager-provider-create
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
#4100
📔 Objective
Currently there is only an implementation to provide or save Fido2 credentials via the CredentialManager.
The objective is to add the possibility to save and create Passwords.
Credential Manager usage: https://developer.android.com/identity/sign-in/credential-manager
Credential Provider to provide Credential Manager: https://developer.android.com/identity/sign-in/credential-provider
Example Project: https://github.com/android/identity-samples/tree/main/CredentialManager
Save was already done in #4110 , therefore this PR is for creating or updating password entries.
📸 Screenshots
Screen_recording_20251113_135054.mp4