Skip to content

[PM-20508] Centralize passkey credential entry creation #5033

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 1 commit into
base: main
Choose a base branch
from

Conversation

SaintPatrck
Copy link
Contributor

@SaintPatrck SaintPatrck commented Apr 11, 2025

🎟️ Tracking

Resolves PM-20176
Resolves PM-20127

📔 Objective

Move the responsibility of building credential entries from Fido2ProviderProcessor to Fido2CredentialManager. This change centralizes the credential handling and simplifies the code in Fido2ProviderProcessor by delegating this task.

Key changes:

  • Fido2CredentialManager:
    • Implemented getCredentialEntries() to handle the retrieval and creation of CredentialEntry objects.
    • Now uses the VaultRepository to fetch and decrypt credential data.
    • Uses EnvironmentRepository to get base icon url.
    • Now uses Glide for network image loading (currently disabled).
    • Includes logic to handle biometric prompts.
  • Fido2ProviderProcessor:
    • Removed the logic for building CredentialEntry.
    • Now relies on Fido2CredentialManager to provide the credential entries.
    • Updated logic in handleFido2GetCredentialsRequest to delegate credential retrieval to manager.
  • Fido2CompletionManager:
    • Updated to use CredentialEntry.
    • Updated logic to use new GetFido2CredentialsResult.
  • VaultItemListingViewModel:
    • Updated logic in handleFido2GetCredentialsRequest to delegate credential retrieval to manager.
  • Tests:
    • Updated unit tests to reflect the changes in Fido2ProviderProcessor, VaultItemListingScreenTest, and Fido2CompletionManager.
  • Dependencies:
    • Removed redundant dependencies.
  • Cleanup:
    • Removed unnecessary code and comments.
  • Module dependency:
    • Added dispatcher and environment modules dependencies to Fido2Provider module.
  • LocalManagerProvider:
    • Fido2CompletionManager doesn't need IntentManager anymore.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) 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

@SaintPatrck SaintPatrck marked this pull request as draft April 11, 2025 16:41
Copy link
Contributor

github-actions bot commented Apr 11, 2025

Logo
Checkmarx One – Scan Summary & Details1af3b989-72ca-4338-9e79-73a1f04e3298

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
MEDIUM Privacy_Violation /app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CipherViewUtil.kt: 69
detailsMethod Lambda at line 69 of /app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CipherViewUtil.kt sends user information outside...
ID: ozvY2R9FGey1yPaPp9mkWxWU8x4%3D
Attack Vector
MEDIUM Privacy_Violation /app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CipherViewUtil.kt: 69
detailsMethod Lambda at line 69 of /app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CipherViewUtil.kt sends user information outside...
ID: OYqiJjYGwzrwEh%2FGToa9Nyqc7kg%3D
Attack Vector
Fixed Issues (8)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 420
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 292
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 336
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 611
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 514
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 565
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 441
MEDIUM Use_of_Hardcoded_Password /app/src/test/java/com/x8bit/bitwarden/data/platform/datasource/network/ssl/CertificateManagerTest.kt: 465

Copy link

codecov bot commented Apr 11, 2025

Codecov Report

Attention: Patch coverage is 87.30159% with 16 lines in your changes missing coverage. Please review.

Project coverage is 83.57%. Comparing base (3131196) to head (09c1c95).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
...tofill/fido2/manager/Fido2CredentialManagerImpl.kt 87.91% 4 Missing and 7 partials ⚠️
...t/feature/itemlisting/VaultItemListingViewModel.kt 57.14% 1 Missing and 2 partials ⚠️
...i/auth/feature/vaultunlock/VaultUnlockViewModel.kt 77.77% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5033      +/-   ##
==========================================
- Coverage   83.85%   83.57%   -0.28%     
==========================================
  Files         596      620      +24     
  Lines       48855    49479     +624     
  Branches     6711     6747      +36     
==========================================
+ Hits        40966    41354     +388     
- Misses       5618     5840     +222     
- Partials     2271     2285      +14     

☔ 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.

@SaintPatrck SaintPatrck force-pushed the passkeys/favicons branch 3 times, most recently from 68f8116 to e4e9836 Compare April 21, 2025 20:40
@SaintPatrck SaintPatrck changed the title [PM-20176] Display cipher favicon in passkey credential entry [PM-20508] Centralize passkey credential entry creation Apr 22, 2025
@SaintPatrck SaintPatrck marked this pull request as ready for review April 22, 2025 18:55
Move the responsibility of building credential entries from `Fido2ProviderProcessor` to `Fido2CredentialManager`. This change centralizes the credential handling and simplifies the code in `Fido2ProviderProcessor` by delegating this task.

Key changes:

- **Fido2CredentialManager:**
    - Implemented `getCredentialEntries()` to handle the retrieval and creation of `CredentialEntry` objects.
    - Now uses the `VaultRepository` to fetch and decrypt credential data.
    - Uses `EnvironmentRepository` to get base icon url.
    - Now uses Glide for network image loading (currently disabled).
    - Includes logic to handle biometric prompts.
- **Fido2ProviderProcessor:**
    - Removed the logic for building `CredentialEntry`.
    - Now relies on `Fido2CredentialManager` to provide the credential entries.
    - Updated logic in `handleFido2GetCredentialsRequest` to delegate credential retrieval to manager.
- **Fido2CompletionManager:**
    - Updated to use `CredentialEntry`.
    - Updated logic to use new `GetFido2CredentialsResult`.
- **VaultItemListingViewModel:**
    - Updated logic in `handleFido2GetCredentialsRequest` to delegate credential retrieval to manager.
- **Tests:**
    - Updated unit tests to reflect the changes in `Fido2ProviderProcessor`, `VaultItemListingScreenTest`, and `Fido2CompletionManager`.
- **Dependencies:**
    - Removed redundant dependencies.
- **Cleanup:**
    - Removed unnecessary code and comments.
- **Module dependency:**
    - Added dispatcher and environment modules dependencies to Fido2Provider module.
- **LocalManagerProvider:**
   - `Fido2CompletionManager` doesn't need `IntentManager` anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant