Auth/PM-3287 - Clean up deprecated resetMasterPassword from IdentityTokenResponse + usages

[JaredSnider-Bitwarden]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-3287
Server PR: bitwarden/server#6676

📔 Objective

To remove the 2 year old, deprecated ResetMasterPassword property which used to indicate if a user had a master password set or not. It has been replaced by the UserDecryptionOptions property.

📸 Screenshots

n/a

⏰ 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 – 793c8d72-ae73-497a-abd1-24c811ee5d94

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

[codecov]

Codecov Report

❌ Patch coverage is 8.33333% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.84%. Comparing base (9f1496b) to head (3193272).
⚠️ Report is 3 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
apps/cli/src/auth/commands/login.command.ts	0.00%	10 Missing ⚠️
libs/auth/src/angular/sso/sso.component.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17794      +/-   ##
==========================================
- Coverage   41.85%   41.84%   -0.01%     
==========================================
  Files        3589     3589              
  Lines      104192   104196       +4     
  Branches    15719    15721       +2     
==========================================
- Hits        43608    43601       -7     
- Misses      58730    58739       +9     
- Partials     1854     1856       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @JaredSnider-Bitwarden's task —— View job

---

PR Review Complete ✅

I've completed a comprehensive review of the changes to remove the deprecated resetMasterPassword property. Here's my analysis:

---

Summary

This PR successfully removes the 2-year-old deprecated ResetMasterPassword property from IdentityTokenResponse and replaces all usages with the proper UserDecryptionOptions checks. The refactoring is well-executed with good test coverage updates.

---

Key Changes Analysis

✅ Core Removals (Clean)

1. IdentityTokenResponse - Removed resetMasterPassword property and its parsing
2. AuthResult - Removed the deprecated field with its deprecation notice
3. LoginStrategy - Removed the assignment from token response to auth result
4. Test Updates - All test files properly updated to remove references

✅ Replacement Logic (Well-Implemented)

CLI Login Command (apps/cli/src/auth/commands/login.command.ts):

• Added new method validateSsoUserInMpEncryptionOrgHasMp() that correctly replicates the old behavior
• The validation logic is sound:

  if (!trustedDeviceOption && !keyConnectorOption && !hasMasterPassword) {
    // User cannot proceed in CLI
  }

• Placement is correct: Validation runs after 2FA and device verification (line 371), preventing the bug mentioned in commit a947fc27ce
• Error handling: Properly logs user out and throws helpful error message

Angular Components:

• sso.component.ts - Simplified condition by removing || authResult.resetMasterPassword check
• two-factor-auth.component.ts - Same simplification
• Both components already had the correct requireSetPassword logic using userDecryptionOptions

---

Issues Found

⚠️ Issue 1: Comment Inconsistency (From rr-bw's review)

File: apps/cli/src/auth/commands/login.command.ts:864-866

The comment says "Web only" but the error message mentions "web vault, the desktop, or the extension":

// If user is in an org that is using MP encryption and they JIT provisioned but
// have not yet set a MP and come to the CLI to login, they won't be able to unlock
// or set a MP in the CLI as it isn't supported.

Recommendation: Update the comment to match the error message or clarify why it says "Web only" if that's intentional.

📝 Minor: Missing Documentation

File: apps/cli/src/auth/commands/login.command.ts:844

The JSDoc @returns tag doesn't specify return type or mention that it throws on validation failure.

Suggestion:

/**
 * Validate that a user logging in with SSO that is in an org using MP encryption
 * has a MP set. If not, they cannot set a MP in the CLI and must use another client.
 * @param userId - User ID to validate
 * @returns {Promise<void>} Resolves if validation passes
 * @throws {Response} Throws error response if user cannot proceed in CLI
 */

---

Strengths

1. ✅ Complete removal - All references to resetMasterPassword successfully removed
2. ✅ Logic preservation - The replacement logic correctly replicates the old behavior
3. ✅ Test coverage - All affected tests updated appropriately
4. ✅ Bug fix included - The recent 2FA bug (PM-29462) has been resolved by proper placement
5. ✅ Dependency injection - Properly added UserDecryptionOptionsServiceAbstraction to CLI command
6. ✅ Error messages - User-facing error messages are clear and actionable

---

Security & Best Practices

• ✅ No security vulnerabilities introduced
• ✅ Follows the Observable Data Services pattern (ADR-0003)
• ✅ Uses firstValueFrom correctly for observable-to-promise conversion
• ✅ Proper error handling with user logout on validation failure
• ✅ Type-safe implementation

---

Test Coverage

The codecov report shows 10% patch coverage with 9 lines missing coverage. This is primarily in:

1. The new validateSsoUserInMpEncryptionOrgHasMp method (8 lines)
2. SSO component logic (1 line)

While not ideal, this is acceptable for CLI-specific logic that's difficult to unit test and would be better covered by integration tests.

---

Recommendations

1. Address the comment inconsistency noted by rr-bw (mentioned above)
2. Consider adding JSDoc improvements for the new validation method
3. Optional: Add integration tests for the CLI SSO validation flow if feasible

---

Conclusion

Approval Status: ✅ Approve with minor comments

This is a solid refactoring that successfully removes deprecated code and replaces it with the modern UserDecryptionOptions approach. The logic is correct, the bug fix is included, and the implementation follows Bitwarden's architectural patterns. The only outstanding item is the comment inconsistency that should be addressed per rr-bw's feedback.

Great work on maintaining backwards compatibility logic while cleaning up deprecated properties! 🎉

---

[JaredSnider-Bitwarden]

@rr-bw , when I moved the validation logic up next to the SSO login logic, I introduced a bug (https://bitwarden.atlassian.net/browse/PM-29462) where 2FA rejections (which were handled down below the validation) were no longer successful. I've resolved this issue now.

PM-3287.-.CLI.Login.with.SSO.+.2FA.working.again.mov