Clear pinProtectedUserKeyEnvelopeValue on user removal in mock service

[quexten]

🎟️ Tracking

📔 Objective

Looks like we forgot to clear the new pin envelope in a test. Adding it for consistency.

📸 Screenshots

⏰ 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]

Checkmarx One – Scan Summary & Details – df92814d-fcf7-4920-bf41-f0bc2f67cac0

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

[claude]

Claude finished @quexten's task —— View job

---

Code Review Complete

Todo List:

• Evaluate PR title and description
• Read changed files
• Search for related implementation files
• Run comprehensive code review
• Post findings

---

Summary

This PR adds a missing line to MockStateService.clearPins() to clear pinProtectedUserKeyEnvelopeValue, matching the implementation in the actual StateService. The fix prevents potential account bricking on user key rotation by ensuring all PIN-related data is properly cleared.

Critical Finding

Finding 1: The actual implementation already contains this fix ❌

Looking at /home/runner/work/ios/ios/BitwardenShared/Core/Platform/Services/StateService.swift:1534-1540, the production clearPins() implementation already includes the line that clears the PIN envelope:

func clearPins() async throws {
    let userId = try getActiveAccountUserId()
    accountVolatileData[userId]?.pinProtectedUserKey = nil
    appSettingsStore.setEncryptedPin(nil, userId: userId)
    appSettingsStore.setPinProtectedUserKey(key: nil, userId: userId)
    appSettingsStore.setPinProtectedUserKeyEnvelope(key: nil, userId: userId) // ← Already here
}

This PR only updates the mock/test helper version (MockStateService.swift:124-130) to match the production implementation. While this is good for test consistency, the actual bug described in the PR objective does not exist in production code—it was only missing in the mock.

PR Title & Description Quality

Finding 2: PR description is misleading 💭

The PR objective states:

"On a user key rotation, this may lead to an undecryptable / bricked account until clearing ios local data because the value is still present."

This description suggests a production bug that could brick user accounts. However, the actual production StateService.clearPins() implementation already properly clears pinProtectedUserKeyEnvelopeValue (line 1539). Only the test mock was missing this line.

Suggested improvements:

Revised PR Description

Title: Update MockStateService.clearPins() to match production implementation

Objective:
The MockStateService.clearPins() method was missing the line to clear pinProtectedUserKeyEnvelopeValue, making it inconsistent with the actual StateService implementation. While this doesn't affect production behavior, it could lead to incorrect test behavior where PIN data isn't fully cleared in mock scenarios.

This updates the mock to properly clear all three PIN-related values:

• pinProtectedUserKeyValue
• encryptedPinByUserId
• pinProtectedUserKeyEnvelopeValue (added)

Good Practices Observed

• Fix correctly mirrors the production implementation pattern
• Maintains consistency between mock and actual service interfaces

Recommendation

The change itself is correct and should be merged to maintain test/mock consistency. However, the PR description should be updated to clarify that:

1. This is a test-only fix (no production impact)
2. The actual production code already handles this correctly
3. There is no risk of bricked accounts from the missing mock implementation

---

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.23%. Comparing base (e8d7c67) to head (fa06bba).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2139   +/-   ##
=======================================
  Coverage   85.23%   85.23%           
=======================================
  Files        1715     1715           
  Lines      145524   145525    +1     
=======================================
+ Hits       124042   124043    +1     
  Misses      21482    21482           

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