[PM-29186] Remove 2FA user interface from premium method signatures

[trmartin4]

🎟️ Tracking

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

📔 Objective

During the refinement for upcoming changes to some of our 2FA setup logic, the Auth team discovered that there is a misleading signature on the CanAccessPremium() and HasPremiumFromOrganization() methods on UserService. These methods as written accepted an ITwoFactorProvidersUser, but the premium checks have evolved over time to be consumed at various other points in the code that have nothing to do with 2FA.

This PR proposes to change those signatures to accept a User instead. This does not break any existing behavior, because User implements ITwoFactorProvidersUser. The only thing that would break would be if other implementors of ITwoFactorProvidersUser - namely OrganizationUserUserDetails - needed to be used as input to CanAccessPremium() or HasPremiumFromOrganization(). However, a check of the code shows that this user representation does not consume those methods.

If we were to need to have an abstraction for an entity that has premium, we should create an IPremiumUser interface at that point to maintain the proper abstraction.

⏰ 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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.23%. Comparing base (267759d) to head (c09a973).
⚠️ Report is 17 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6684      +/-   ##
==========================================
+ Coverage   53.33%   57.23%   +3.89%     
==========================================
  Files        1909     1917       +8     
  Lines       85118    85460     +342     
  Branches     7648     7667      +19     
==========================================
+ Hits        45398    48913    +3515     
+ Misses      37965    34706    -3259     
- Partials     1755     1841      +86     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Checkmarx One – Scan Summary & Details – 171136f7-dd14-4035-9b06-1d22fff65218

New Issues (1)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 427	details Method at line 427 of /src/Api/Auth/Controllers/AccountsController.cs gets a parameter from a user request from model. This parameter value flow... ID: VU8sgdorwW4N9vJXTUl%2BCpjmMcY%3D Attack Vector

[claude]

Claude finished @trmartin4's task —— View job

---

PR Review: Remove 2FA user interface from premium method signatures

I've completed a comprehensive code review of this PR. Here are my findings:

---

🔒 Security Review

Status: ✅ No Security Concerns

The changes in this PR do not introduce any security vulnerabilities:

• Zero-Knowledge Principles: Not affected. The changes are purely about method signature refactoring and do not touch encryption, vault data, or user credentials.
• Data Exposure: No sensitive data logging or exposure introduced.
• Authentication/Authorization: No changes to auth flows or access control logic.
• Premium Access Logic: The underlying premium validation logic remains unchanged—only the interface contract was updated.

---

⚠️ Breaking Changes

Status: ✅ No Breaking Changes for Current Usage

Analysis:

1. Interface Narrowing is Safe: Changing from ITwoFactorProvidersUser to User is not a breaking change because:

   • User class implements ITwoFactorProvidersUser (User.cs:14)
   • All existing callers pass User instances, not other ITwoFactorProvidersUser implementations
   • No code passes OrganizationUserUserDetails to these methods

2. Removed Methods:

   • ✅ User.GetPremium() removed (was just return Premium;)
   • ✅ OrganizationUserUserDetails.GetPremium() removed
   • ✅ ITwoFactorProvidersUser.GetPremium() removed from interface

3. Updated Implementation (UserService.cs:1115, 1118):

   • Changed from user.GetPremium() to user.Premium - direct property access
   • More idiomatic C# code

---

📝 Code Quality

Status: ✅ Excellent Quality

Strengths:

1. Clear Intent: The PR correctly addresses the semantic mismatch between ITwoFactorProvidersUser and premium-related functionality
2. Proper Null Safety:
   • ITwoFactorProvidersUser.cs properly enabled nullable reference types (removed #nullable disable)
   • Added appropriate ? annotations for nullable types
3. Improved Documentation:
   • IUserService.cs:60-77 contain excellent XML comments explaining when to use each method
   • Clarifies that CanAccessPremium() is for "definitive" premium access checks
   • Clarifies that HasPremiumFromOrganization() is primarily for UI communication
4. Cleaner Code: Removed unnecessary GetPremium() wrapper methods in favor of direct property access

---

🤔 Minor Issue Found

Incomplete Documentation (IUserService.cs:75):

/// <param name="user"></param>

Should have a description like:

/// <param name="user">The user to check for organization-based premium access</param>
/// <returns>True if the user has premium access through any organization; false otherwise</returns>

---

🎨 Optional Suggestions

1. Consider Null Guard: While the current implementation checks userId.HasValue, adding an explicit null check for the user parameter would be more defensive:

   public async Task<bool> CanAccessPremium(User user)
   {
       ArgumentNullException.ThrowIfNull(user);
       var userId = user.GetUserId();
       // ... rest of method
   }

2. Consistency: The call changed from this.HasPremiumFromOrganization(user) to HasPremiumFromOrganization(user). Consider being consistent with the use of this. qualifier throughout the codebase.

---

📊 Summary

Aspect	Status	Notes
Security	✅ Pass	No security concerns
Breaking Changes	✅ Pass	Not a breaking change
Type Safety	✅ Pass	Improved with narrower types
Documentation	⚠️ Minor Issue	One param doc incomplete
Tests	✅ Pass	Existing tests cover functionality
Code Style	✅ Pass	Follows .NET conventions
Nullable References	✅ Pass	Properly implemented

---

🎯 Recommendation

APPROVE ✅

This is a well-executed refactoring that improves code semantics without introducing breaking changes or security issues. The single minor documentation gap is not a blocker but should be addressed.

Summary:

• ✅ No breaking changes - User implements ITwoFactorProvidersUser, all callers compatible
• ✅ Security review passed - no vulnerabilities introduced
• ✅ Type safety improved - narrower, more semantically correct types
• ✅ Existing tests compatible
• 📝 Minor: Complete XML documentation for HasPremiumFromOrganization parameter

The PR successfully achieves its goal of removing the misleading ITwoFactorProvidersUser interface from premium method signatures while maintaining backward compatibility and improving code clarity.

---