Skip to content

[PM-26177] feat: Add device auth key to keychain repository#2293

Merged
KatherineInCode merged 18 commits intomainfrom
iinuwa/dak-1-add-device-key-to-keychain-repository
Feb 12, 2026
Merged

[PM-26177] feat: Add device auth key to keychain repository#2293
KatherineInCode merged 18 commits intomainfrom
iinuwa/dak-1-add-device-key-to-keychain-repository

Conversation

@iinuwa
Copy link
Contributor

@iinuwa iinuwa commented Jan 28, 2026

🎟️ Tracking

PM-26177

📔 Objective

Adds the device auth key and its metadata to the keychain repository. These will be used in later PRs for Device Auth Key implementation.

⏰ 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

@github-actions github-actions bot added the app:password-manager Bitwarden Password Manager app context label Jan 28, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 28, 2026

Logo
Checkmarx One – Scan Summary & Details5594658c-83f0-4bf9-8d53-cd1c4f123822

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

@KatherineInCode KatherineInCode changed the title Add device auth key to keychain repository [PM-26177] (1/8) [PM-26177] Add device auth key to keychain repository Feb 3, 2026
@KatherineInCode KatherineInCode added the ai-review Request a Claude code review label Feb 3, 2026
@KatherineInCode KatherineInCode marked this pull request as ready for review February 3, 2026 20:04
@claude
Copy link

claude bot commented Feb 3, 2026

Claude finished @KatherineInCode's task in 4m 24s —— View job


Code Review Summary

PR: PM-26177 feat: Add device auth key to keychain repository

Overall Assessment: This PR is well-structured and ready to merge. No new issues found.

Changes Reviewed

File Type Assessment
DeviceAuthKeyMetadata.swift New model Clean
DeviceAuthKeyRecord.swift New model Clean
DeviceAuthKeychainRepository.swift New protocol Clean
KeychainRepository.swift Extended implementation Clean
KeychainRepository+DeviceAuthTests.swift New tests Clean
DeviceAuthKeyMetadata+Fixtures.swift Test fixture Clean
DeviceAuthKeyRecord+Fixtures.swift Test fixture Clean
MockKeychainService.swift Test helper update Clean

Security Review

  • Device auth key (DeviceAuthKeyRecord) is correctly protected with .biometryCurrentSet access control, requiring biometric authentication before the key material can be accessed. This properly satisfies WebAuthn user verification requirements.
  • Metadata (DeviceAuthKeyMetadata) is appropriately stored without biometric protection, since it must be accessible for credential matching before authentication.
  • Both items use kSecAttrAccessibleWhenUnlockedThisDeviceOnly, preventing backup/transfer of these keys.
  • Key material fields use EncString (encrypted strings), providing an additional encryption layer beyond keychain at-rest encryption.
  • Device auth keys are correctly excluded from deleteItems(for:), consistent with how deviceKey is handled for re-login flows.

Architecture and Correctness

  • The consistency model is sound: metadata is set last and deleted first, acting as a commit indicator for the two-part storage operation.
  • The generic getValue<T: Codable> and setValue<T: Codable> methods are a clean addition that follows the existing pattern while reducing boilerplate for structured keychain data.
  • Error handling correctly returns nil for both keyNotFound and osStatusError(errSecItemNotFound), consistent with the getDeviceKey pattern in the codebase.

Test Coverage

Tests cover all key paths: successful get/set/delete, not-found scenarios (both error types), invalid data deserialization, and access control failures. The deletion ordering is verified to ensure metadata-first deletion.

No inline comments warranted -- previously identified issues (parameter ordering, type mismatches) have all been resolved in subsequent commits.

@codecov
Copy link

codecov bot commented Feb 3, 2026

Codecov Report

❌ Patch coverage is 97.73585% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.12%. Comparing base (0283b1f) to head (503e5d8).
⚠️ Report is 15 commits behind head on main.

Files with missing lines Patch % Lines
.../Services/KeychainRepository+DeviceAuthTests.swift 97.38% 4 Missing ⚠️
...Shared/Core/Auth/Services/KeychainRepository.swift 96.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2293      +/-   ##
==========================================
+ Coverage   86.04%   86.12%   +0.08%     
==========================================
  Files        1775     1781       +6     
  Lines      155124   156144    +1020     
==========================================
+ Hits       133472   134484    +1012     
- Misses      21652    21660       +8     

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

@KatherineInCode KatherineInCode changed the title [PM-26177] Add device auth key to keychain repository [PM-26177] feat: Add device auth key to keychain repository Feb 3, 2026
@KatherineInCode
Copy link
Contributor

⛏️ We usually order parameters and properties alphabetically. https://github.com/bitwarden/ios/pull/2293/changes#diff-8edd01974a95f4935502b37481cc70f61b9d3c948e5ae15c79b91e36c418b138R88 https://github.com/bitwarden/ios/pull/2293/changes#diff-8edd01974a95f4935502b37481cc70f61b9d3c948e5ae15c79b91e36c418b138R49

I thought we went through and alphabetized everything, and the links aren't working well. What did we miss?

Copy link
Member

@fedemkr fedemkr left a comment

Choose a reason for hiding this comment

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

Looks good, just a few improvements.

@andrebispo5
Copy link
Contributor

⛏️ We usually order parameters and properties alphabetically. https://github.com/bitwarden/ios/pull/2293/changes#diff-8edd01974a95f4935502b37481cc70f61b9d3c948e5ae15c79b91e36c418b138R88 https://github.com/bitwarden/ios/pull/2293/changes#diff-8edd01974a95f4935502b37481cc70f61b9d3c948e5ae15c79b91e36c418b138R49

I thought we went through and alphabetized everything, and the links aren't working well. What did we miss?

Weird they open fine on my end, it is userName and userDisplayName.

@KatherineInCode
Copy link
Contributor

Weird they open fine on my end, it is userName and userDisplayName.

Yeah, I have no idea. But good catch! We can swing back around on those.

Copy link
Member

@fedemkr fedemkr left a comment

Choose a reason for hiding this comment

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

Looks good, just one small ⛏️

Comment on lines +349 to +351
let value = try JSONDecoder.defaultDecoder.decode(T.self, from: jsonData)

return value
Copy link
Member

Choose a reason for hiding this comment

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

⛏️ Pretty minor but could we just return directly here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, I've pushed the change

@KatherineInCode KatherineInCode merged commit ff9b240 into main Feb 12, 2026
23 checks passed
@KatherineInCode KatherineInCode deleted the iinuwa/dak-1-add-device-key-to-keychain-repository branch February 12, 2026 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review app:password-manager Bitwarden Password Manager app context t:feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants