Skip to content

[PM-13789] add credential manager provider for passwords #4110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 88 commits into
base: main
Choose a base branch
from

Conversation

Nailik
Copy link
Contributor

@Nailik Nailik commented Oct 17, 2024

🎟️ 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

Status

  • new BitwardenCredentialProviderService to handle incoming credential requests
  • new CredentialCompletionManager (from Fido2CompletionManager) to handle both passkeys and passwords
  • Overwrite Password/Username combination confirmation dialogs.

Working on:

  • Cleanup (formatting, comments etc)
  • fixing tests / adding tests

Found issues:

  • showFido2ErrorDialog doesn't always show the correct reason, however to stay consistent the showPaswordErrorDialog also shows always the same issue

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation or informed the documentation team

🦮 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

@CLAassistant
Copy link

CLAassistant commented Oct 17, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ Nailik
✅ SaintPatrck
❌ kilian.eller


kilian.eller seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-13789

@bitwarden-bot bitwarden-bot changed the title [DRAFT] add credential manager provider for passwords [PM-13789] [DRAFT] add credential manager provider for passwords Oct 17, 2024
@Nailik Nailik force-pushed the feature/add-password-credential-manager-provider branch 3 times, most recently from 4aa9f5a to 5a6e528 Compare October 20, 2024 01:03
@Nailik
Copy link
Contributor Author

Nailik commented Oct 21, 2024

I would like a check/pre-review from the code owners before starting to work on the tests.

Maybe this could be split into multiple smaller PRs as it got already a lot bigger than i thought it would be - and it's also a lot to review

@Nailik Nailik changed the title [PM-13789] [DRAFT] add credential manager provider for passwords [PM-13789] add credential manager provider for passwords Oct 22, 2024
@djsmith85 djsmith85 linked an issue Nov 11, 2024 that may be closed by this pull request
2 tasks
@Nailik
Copy link
Contributor Author

Nailik commented Jan 17, 2025

due to a lot of changes in the types it's currently not compiling

however i am not sure if i just waste my time here since i get no response or any info if this feature is planned ...

@SaintPatrck
Copy link
Contributor

Hi @Nailik

Apologies for the late response. We've been busy shoring up our native releases. We are interested in consuming these changes and do appreciate you taking the time to submit them.

As you stated, there has been refactoring that conflicts with the changes you submitted. There's potential for more refactoring once Google releases the next version of Credential Manager (1.5.0). I suggest holding this PR until the new CredMan library is available. As soon as time permits we will review and provide feedback.

@Nailik
Copy link
Contributor Author

Nailik commented Jan 21, 2025

@SaintPatrck perfect thank you.

@Nailik
Copy link
Contributor Author

Nailik commented Mar 25, 2025

androidx.credentials:credentials:1.5.0 was released on 15. Mar so i will resume working on this PR now.
Will take some time

@SaintPatrck
Copy link
Contributor

androidx.credentials:1.5.0 introduces breaking changes. I suggest holding off until we have properly refactored to accommodate the changes (see WIP #4919).

@Nailik
Copy link
Contributor Author

Nailik commented Mar 26, 2025

thx for letting me know

@Nailik
Copy link
Contributor Author

Nailik commented May 12, 2025

I am currently rewriting the whole PR.

Currently i am blocked because of an issue with #5101.

@SaintPatrck
Copy link
Contributor

Hi @Nailik

I'm glad to see you're still willing to pick this back up! It's very much appreciated. 🫶

You may want to build on #5177 since it includes changes that will make implementing password credentials a little cleaner.

@Nailik Nailik requested a review from SaintPatrck June 21, 2025 14:23
@Nailik
Copy link
Contributor Author

Nailik commented Jun 21, 2025

Hi @SaintPatrck
i think i added all required UnitTests, i'll start on creating password entries in a separate PR now.

Nailik added 4 commits June 24, 2025 10:20
…ider

# Conflicts:
#	app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/manager/intent/IntentManager.kt
…ider

# Conflicts:
#	app/src/main/kotlin/com/x8bit/bitwarden/data/credentials/builder/CredentialEntryBuilderImpl.kt
#	app/src/main/kotlin/com/x8bit/bitwarden/data/credentials/util/CredentialProviderIntentUtils.kt
@SaintPatrck
Copy link
Contributor

Hey @Nailik!

These updates look great. Everything is working as expected. I'm going to push a small commit to fix the build error and make a few formatting tweaks. Once the build checks complete I'll have another team member review the changes for anything I may have overlooked.

Thank you for your patience while we review, and your amazing contributions. 🫶

Copy link
Contributor

github-actions bot commented Jul 2, 2025

Logo
Checkmarx One – Scan Summary & Detailsa3291f8c-701a-4467-a78d-7292c6cb4738

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Privacy_Violation /app/src/test/kotlin/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CipherViewUtil.kt: 113
detailsMethod createMockLoginView at line 113 of /app/src/test/kotlin/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CipherViewUtil.kt sends user in...
ID: MrQdqzaL5PNjNc2DM2qdYgfkqSk%3D
Attack Vector

- BitwardenCredentialManagerImpl.kt: formatting.
- CredentialEntryBuilderExtensions.kt: fix compile error.
- CredentialEntryBuilderImpl.kt: use generic login icon for password credntial entry icon to match autofill items and better distinguish from passkeys.
- CredentialProviderIntentUtils.kt: invert build version check.
- provider.xml: Add public key credential capability so debug builds still handle passkeys.
- VaultItemListingViewModel.kt: formatting.
@SaintPatrck SaintPatrck force-pushed the feature/add-password-credential-manager-provider branch from e1b13e7 to 02cde31 Compare July 2, 2025 19:27
Copy link

codecov bot commented Jul 2, 2025

Codecov Report

Attention: Patch coverage is 90.94650% with 22 lines in your changes missing coverage. Please review.

Project coverage is 84.06%. Comparing base (6454dc1) to head (4e2634b).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...t/feature/itemlisting/VaultItemListingViewModel.kt 86.81% 7 Missing and 5 partials ⚠️
...dentials/manager/BitwardenCredentialManagerImpl.kt 83.87% 0 Missing and 5 partials ⚠️
.../credentials/builder/CredentialEntryBuilderImpl.kt 94.28% 0 Missing and 2 partials ⚠️
...manager/CredentialProviderCompletionManagerImpl.kt 92.00% 0 Missing and 2 partials ⚠️
...en/ui/platform/feature/rootnav/RootNavViewModel.kt 87.50% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4110      +/-   ##
==========================================
+ Coverage   84.02%   84.06%   +0.03%     
==========================================
  Files         698      698              
  Lines       52872    53076     +204     
  Branches     7198     7241      +43     
==========================================
+ Hits        44428    44617     +189     
- Misses       5970     5973       +3     
- Partials     2474     2486      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -1450,6 +1466,10 @@ class VaultItemListingViewModel @Inject constructor(
handleFido2AssertionResultReceive(action)
}

is VaultItemListingsAction.Internal.ProviderGetPasswordCredentialRequestReceive -> {
handleProviderGetPasswordCredentialRequestReceive(action)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we get some test coverage on this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, while writing the tests i found some issues regarding authentication if a credential has reprompt set. i also renamed some functions to show that it is regarding credentials and not only fido2 and updted some texts for the same reason (passkey ... -> credential ..)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No Credential Manager Password Prompt
5 participants