Skip to content

[PM-29049] refactor: Validate PIN against pinProtectedUserKeyEnvelope#2863

Open
matt-livefront wants to merge 1 commit into
mainfrom
matt/PM-29049-validate-pin-for-envelope
Open

[PM-29049] refactor: Validate PIN against pinProtectedUserKeyEnvelope#2863
matt-livefront wants to merge 1 commit into
mainfrom
matt/PM-29049-validate-pin-for-envelope

Conversation

@matt-livefront

@matt-livefront matt-livefront commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

🎟️ Tracking

PM-29049

📔 Objective

Updates the SDK method used for validating PINs to validatePinProtectedUserKeyEnvelope(pin: pinProtectedUserKeyEnvelope:). After migrating to the pin protected user key envelope in #1876, PIN validation needs to also use the envelope in order to validate correctly.

@matt-livefront matt-livefront requested a review from a team as a code owner July 7, 2026 16:35
@matt-livefront matt-livefront added ai-review Request a Claude code review t:tech-debt Change Type - Tech debt labels Jul 7, 2026
@github-actions github-actions Bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context labels Jul 7, 2026
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the PIN validation refactor that switches DefaultAuthRepository.validatePin from the legacy validatePin(pin:pinProtectedUserKey:) SDK call to validatePinProtectedUserKeyEnvelope(pin:pinProtectedUserKeyEnvelope:), aligning PIN validation with the pin-protected user key envelope migration. Verified the removed AuthClientService.validatePin protocol method has no remaining production or generated-mock references, and that the sole production caller (UserVerificationHelper.verifyPin) already wraps the call in do/catch so the behavior change from try? to try on the state lookup is handled gracefully. Test updates correctly cover the valid, invalid, no-active-account (now throwing), and missing-envelope paths.

Code Review Details

No findings. The refactor is well-scoped, the type change compiles against the SDK envelope type, and test coverage matches the new control flow (the only throwing path, the state service lookup, is exercised by test_validatePin_noActiveAccount; the SDK envelope validation call is non-throwing so no propagation test is needed).

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.98%. Comparing base (72273fe) to head (05b6977).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2863      +/-   ##
==========================================
- Coverage   81.25%   78.98%   -2.27%     
==========================================
  Files        1027     1150     +123     
  Lines       66091    73421    +7330     
==========================================
+ Hits        53699    57994    +4295     
- Misses      12392    15427    +3035     

☔ View full report in Codecov by Harness.
📢 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.

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: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