[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.46%. Comparing base (267759d) to head (99338aa).
⚠️ Report is 38 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6684      +/-   ##
==========================================
+ Coverage   53.33%   57.46%   +4.13%     
==========================================
  Files        1909     1926      +17     
  Lines       85118    85711     +593     
  Branches     7648     7686      +38     
==========================================
+ Hits        45398    49258    +3860     
+ Misses      37965    34606    -3359     
- Partials     1755     1847      +92     

☔ 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 – 9f4737cd-20f3-46f8-9fc2-f1a52541be13

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

---

Code Review Complete

Tasks

• Analyze PR context and changes
• Review code changes for correctness and security
• Verify documentation and test coverage
• Provide comprehensive review feedback

---

Overall Assessment

This is an architecturally sound refactoring that removes a misleading interface abstraction. The premium checking logic has evolved beyond its original 2FA context, and this PR correctly narrows the method signatures to reflect their actual usage.

Finding Summary:

• 1 Critical Issue (test compilation failure)
• 2 Suggested Improvements (documentation, interface consistency)

---

Critical Issues

Finding 1: Test Compilation Failure ❌

The test file test/Core.Test/Auth/UserFeatures/TwoFactorAuth/TwoFactorIsEnabledQueryTests.cs contains a private test class TestTwoFactorProviderUser (lines 407-426) that implements ITwoFactorProvidersUser. This class still defines the GetPremium() method which was removed from the interface:

private class TestTwoFactorProviderUser : ITwoFactorProvidersUser
{
    // ... other members ...
    
    public bool GetPremium()  // ← This method no longer exists in the interface
    {
        return Premium;
    }
}

Impact: The build will fail with a compilation error stating that GetPremium() does not implement an interface member.

Fix: Remove the GetPremium() method from the TestTwoFactorProviderUser class at line 422-425. The Premium property can be accessed directly by tests if needed.

Why this was missed: The test class is private and only used within that test file, so it's easy to overlook during interface refactoring.

---

Suggested Improvements

Finding 2: Incomplete Null-Safety Migration ⛏️

The ITwoFactorProvidersUser.cs file was updated to remove #nullable disable and made nullable-aware, but there's an inconsistency:

1. The interface property TwoFactorProviders is now nullable: string? TwoFactorProviders { get; }
2. However, the implementing class OrganizationUserUserDetails (line 12 of src/Core/AdminConsole/Models/Data/Organizations/OrganizationUsers/OrganizationUserUserDetails.cs) is still marked with #nullable disable

Suggestion: While not strictly required for this PR, consider filing a follow-up issue to complete the null-safety migration for OrganizationUserUserDetails.cs since its interface is now null-safe. This creates technical debt where the interface contract differs from the implementation's nullability context.

Finding 3: Documentation Could Be More Prescriptive 🎨

The added XML documentation for CanAccessPremium and HasPremiumFromOrganization is good, but the guidance could be clearer about their intended use:

Current documentation (lines 68-76 of IUserService.cs):

/// <summary>
/// Checks if the user has inherited access to premium features through an organization.
///
/// This primarily serves as a means to communicate to the client when a user has inherited their premium status
/// through an organization. Feature gating logic probably should not be behind this check.
/// </summary>
Task<bool> HasPremiumFromOrganization(User user);

Suggestion: The documentation could be more explicit about when NOT to use HasPremiumFromOrganization:

/// <summary>
/// Checks if the user has inherited access to premium features through an organization.
///
/// This method is intended ONLY for UI/client communication purposes to display the source of premium access.
/// DO NOT use this method for feature gating - use <see cref="CanAccessPremium"/> instead.
/// </summary>

Why: The current wording "probably should not" is somewhat ambiguous. Making it more prescriptive ("DO NOT use") helps prevent future misuse.

---

Architecture & Security Assessment

Positive Findings:

• ✅ Architecturally sound: Removes misleading abstraction that conflated 2FA and premium concepts
• ✅ No breaking changes: Runtime behavior unchanged - User still implements ITwoFactorProvidersUser
• ✅ Proper abstraction level: Premium methods logically operate on full User entities
• ✅ Future-proofed: PR description correctly identifies that if premium abstraction is needed later, an IPremiumUser interface should be created
• ✅ No security implications: Pure refactoring with no changes to security logic
• ✅ Zero-knowledge principles: Not affected by this change

Code Quality:

• ✅ Comprehensive XML documentation added
• ✅ Clear, self-documenting method names
• ✅ Consistent usage patterns across all call sites
• ✅ Follows nullable reference type patterns (ADR 0024)

Test Coverage:

• ✅ Existing tests in UserServiceTests.cs for both premium methods
• ✅ All call sites verified to pass User objects, not OrganizationUserUserDetails
• ❌ One test helper class needs update (Finding 1)

---

Verification Steps Performed

1. ✅ Searched all usages of CanAccessPremium and HasPremiumFromOrganization - all pass User objects
2. ✅ Verified OrganizationUserUserDetails is never passed to these methods in the codebase
3. ✅ Confirmed GetPremium() is no longer referenced except in one test helper class
4. ✅ Validated that User.Premium property access is functionally equivalent to removed GetPremium() call
5. ✅ Checked interface implementors - only User, OrganizationUserUserDetails, and test classes implement ITwoFactorProvidersUser

---

Action Items for Author

1. REQUIRED: Update TestTwoFactorProviderUser class in test/Core.Test/Auth/UserFeatures/TwoFactorAuth/TwoFactorIsEnabledQueryTests.cs to remove the GetPremium() method at lines 422-425 (Finding 1)

2. SUGGESTED: Consider strengthening the documentation for HasPremiumFromOrganization to be more prescriptive about not using it for feature gating (Finding 3)

3. OPTIONAL: File a follow-up issue to complete null-safety migration for OrganizationUserUserDetails to match its now-nullable interface (Finding 2)

---

Summary

This PR successfully removes a misleading abstraction and clarifies the purpose of premium checking methods. The refactoring is well-justified and architecturally sound. One critical test compilation issue must be fixed before merge. The changes align with Bitwarden's code quality standards and security requirements.

Recommendation: Request changes (due to test compilation failure)

---

[enmande]

Thank you; this adds a ton of clarity, and the comments are fantastic.
Just proposing a couple of ⛏️ -level suggestions to make the XML comments between the two IUserService methods more similar, but totally not necessary.