Skip to content

[PM-34009] chore: Create KeychainServiceFacade#2507

Open
KatherineInCode wants to merge 45 commits intomainfrom
pm-34009/keychain-service-facade
Open

[PM-34009] chore: Create KeychainServiceFacade#2507
KatherineInCode wants to merge 45 commits intomainfrom
pm-34009/keychain-service-facade

Conversation

@KatherineInCode
Copy link
Copy Markdown
Contributor

@KatherineInCode KatherineInCode commented Mar 30, 2026

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-34009

📔 Objective

This consolidates logic that was spread between BitwardenShared.KeychainRepository, AuthenticatorShared.KeychainRepository, and AuthenticatorBridgeKit.KeychainStorage into one KeychainServiceFacade object, and updates the KeychainRepository objects accordingly. In the process, I also tried to write new tests in Swift Testing, and convert a lot of our bespoke mock objects into AutoMockable, and therefore also needed to update tests.

This effectively updates the architecture from this:

flowchart TD
    BSKRAT[getAccessToken]
    BSKRNL[getNeverLock]
    BSKRG[getValue]

    subgraph BitwardenShared.KeychainRepository
    direction TD
    BSKRAT-->BSKRG
    BSKRNL-->BSKRG
    end

    ASKRSK[getSecretKey]
    ASKRUAK[getUserAccessKey]
    ASKRG[getValue]
    subgraph AuthenticatorShared.KeychainRepository
    direction TD
    ASKRSK-->ASKRG
    ASKRUAK-->ASKRG
    end
    
    subgraph AuthenticatorBridgeKit.KeychainRepository
    direction TD
    ABKAK[getAuthenticatorKey]
    ABKALT[getAutoLogoutTime]
    end
    subgraph AuthenticatorBridgeKit.KeychainStorage
    ABKG[getValue]
    ABKAK-->ABKG
    ABKALT-->ABKG
    end

    subgraph BitwardenKit.KeychainService
    BKKSS[search]
    BSKRG-->BKKSS
    ASKRG-->BKKSS
    ABKG-->BKKSS
    end
Loading

To this:

flowchart TD
    subgraph BitwardenShared.KeychainRepository
    direction TD
    BSKRAT[getAccessToken]
    BSKRNL[getNeverLock]
    end

    subgraph AuthenticatorShared.KeychainRepository
    direction TD
    ASKRSK[getSecretKey]
    ASKRUAK[getUserAccessKey]
    end
    
    subgraph AuthenticatorBridgeKit.KeychainRepository
    direction TD
    ABKAK[getAuthenticatorKey]
    ABKALT[getAutoLogoutTime]
    end

    subgraph BitwardenKit.KeychainServiceFacade
    BKKSWG[getValue]
    BSKRAT-->BKKSWG
    BSKRNL-->BKKSWG
    ASKRSK-->BKKSWG
    ASKRUAK-->BKKSWG
    ABKAK-->BKKSWG
    ABKALT-->BKKSWG
    end

    subgraph BitwardenKit.KeychainService
    BKKSS[search]
    BKKSWG-->BKKSS
    end
Loading

†diagrams are just for some getters; set and delete and other properties all work similarly from an architectural perspective

@github-actions github-actions bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Mar 30, 2026
@KatherineInCode KatherineInCode changed the title [PM-34009] Create KeychainServiceFacade [PM-34009] chore: Create KeychainServiceFacade Mar 30, 2026
@KatherineInCode KatherineInCode added the hold This shouldn't be merged yet label Mar 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 30, 2026

Logo
Checkmarx One – Scan Summary & Details8ef0c8eb-870f-48ef-b154-d1a37dc07507

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

❌ Patch coverage is 96.65236% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.99%. Comparing base (571dd72) to head (dfca6a1).

Files with missing lines Patch % Lines
...ared/Core/Platform/Services/ServiceContainer.swift 0.00% 15 Missing ⚠️
...Shared/Core/Auth/Services/KeychainRepository.swift 87.30% 8 Missing ⚠️
...SharedKeychain/SharedKeychainRepositoryTests.swift 96.46% 4 Missing ⚠️
...latform/Models/Domain/Mocks/MockKeychainItem.swift 50.00% 3 Missing ⚠️
...d/Core/Auth/Services/KeychainRepositoryTests.swift 97.94% 3 Missing ⚠️
...eKit/SharedKeychain/SharedKeychainRepository.swift 92.59% 2 Missing ⚠️
...enKit/Core/Platform/Services/KeychainService.swift 80.00% 2 Missing ⚠️
...Core/Platform/Services/KeychainServiceFacade.swift 98.24% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2507      +/-   ##
==========================================
- Coverage   87.07%   85.99%   -1.09%     
==========================================
  Files        1859     2086     +227     
  Lines      164172   178692   +14520     
==========================================
+ Hits       142955   153668   +10713     
- Misses      21217    25024    +3807     

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

@github-actions github-actions bot added the t:tech-debt Change Type - Tech debt label Mar 30, 2026
extension MockKeychainItem: Equatable {
public convenience init(unformattedKey: String) {
public convenience init(
unformattedKey: String,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not alphabetized to put optional parameters at the end

}

public func getValue(for item: any KeychainItem) async throws -> String {
let foundItem = try await keychainService.search(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

While this could have gone in the guard, I feel like this is a little easier to read.

@KatherineInCode KatherineInCode added ai-review-vnext Request a Claude code review using the vNext workflow and removed hold This shouldn't be merged yet labels Apr 7, 2026
@KatherineInCode KatherineInCode marked this pull request as ready for review April 7, 2026 18:10
@KatherineInCode KatherineInCode requested review from a team and matt-livefront as code owners April 7, 2026 18:10
@github-actions github-actions bot added hold This shouldn't be merged yet and removed ai-review-vnext Request a Claude code review using the vNext workflow labels Apr 7, 2026
@KatherineInCode KatherineInCode added ai-review-vnext Request a Claude code review using the vNext workflow and removed hold This shouldn't be merged yet labels Apr 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR consolidates keychain interaction logic from three separate implementations (BitwardenShared.KeychainRepository, AuthenticatorShared.KeychainRepository, and AuthenticatorBridgeKit.SharedKeychainStorage) into a single KeychainServiceFacade in BitwardenKit. The facade correctly preserves all existing keychain behaviors including access control flags, protection levels, the update-then-add race condition prevention pattern, and per-context storage key formatting. Test coverage is thorough, with bespoke mocks replaced by AutoMockable-generated mocks and new Swift Testing tests for the facade itself.

Code Review Details

No findings identified. The refactoring is clean and well-tested.

  • Security-critical keychain behaviors (access groups, protection levels, access control flags) are correctly preserved through the facade
  • The KeychainNamespacing enum cleanly models the two different key formatting strategies (app-scoped with prefix vs shared bare keys)
  • The DefaultKeychainService.search() behavioral change (returning nil instead of throwing for errSecItemNotFound) is correctly handled by the facade's getValue method
  • Storage key formats are preserved exactly: bwKeyChainStorage for Password Manager, bwaKeychainStorage for Authenticator, and bare unformatted keys for shared keychain items

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

Labels

ai-review-vnext Request a Claude code review using the vNext workflow app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:tech-debt Change Type - Tech debt

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant