clients [PM-29021] Add additional guards for phishing detection feature

🎟️ Tracking

📔 Objective

This pull request changes the access requirements for phishing blocker.

Previous access requirements:

User has premium through any source, personal or org

Update access requirements:

User has premium through personal subscription
User is a member of a Family organization that has the following permission:
- usersGetPremium
User is a member of an Enterprise organization that has the following permissions:
- usePhishingBlocker
- usersGetPremium

Note: If a user has access through a Family organization, the usePhishingBlocker flag should not affect access. The user should have access whether the organization's flag is enabled or disabled.

⏰ 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

Dec 04 '25 16:12 Banrion

Claude finished @Banrion's task —— View job

Code Review Complete

Todo List:

[x] Review PR changes using Bitwarden code review standards
[x] Analyze access control logic changes
[x] Evaluate security implications
[x] Assess test coverage
[x] Provide comprehensive feedback

Code Review for PR #17818: Phishing Detection Feature Access Control Updates

Overall Assessment

I've completed a thorough review of the phishing detection access control changes. The PR successfully transitions from "any premium source" to more restrictive access based on personal premium or specific organization types. However, I've identified 3 critical issues, 1 important concern, and 1 technical debt item that should be addressed before merge.

Critical Issues

Finding 1: Discrepancy between PR description and implementation

Location: apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts:152-156

The PR description states that Family organizations should have access, but the code also grants access to Enterprise organizations with usePhishingBlocker enabled. This is not mentioned in the PR description.

Questions:

Is Enterprise organization access intended? If yes, the PR description should be updated.
Should Teams (ProductTierType.Teams) or TeamsStarter (ProductTierType.TeamsStarter) organizations with usePhishingBlocker also have access?
Why is usePhishingBlocker only checked for Enterprise but not for Families?

Code in question:

// Check if the user is a member of a enterprise organization with phishing blocker enabled
if (
  org.productTierType === ProductTierType.Enterprise &&
  org.usePhishingBlocker
) {
  return true;
}

Finding 2: Inconsistent permission requirements between org types

Location: apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts:148-156

Family organizations only need usersGetPremium to grant access, while Enterprise organizations need both usersGetPremium AND usePhishingBlocker. This creates an asymmetric security model.

Security concern: If Family organizations don't check usePhishingBlocker, they might inadvertently grant access even when the feature is disabled at the org level.

Current logic:

if (org.canAccess && org.isMember && org.usersGetPremium) {
  // Family: passes here, no usePhishingBlocker check
  if (org.productTierType === ProductTierType.Families) {
    return true;
  }
  // Enterprise: requires additional usePhishingBlocker check
  if (
    org.productTierType === ProductTierType.Enterprise &&
    org.usePhishingBlocker
  ) {
    return true;
  }
}

Recommendation: Either:

Check usePhishingBlocker for Families too: org.productTierType === ProductTierType.Families && org.usePhishingBlocker
Document why Families don't need this check (if there's a business reason)

Finding 3: No validation that organization is enabled

Location: apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts:146

While org.canAccess checks enabled status for non-owners, relying on this getter is implicit. The security-critical nature of this feature warrants an explicit check for disabled organizations.

The canAccess getter:

get canAccess() {
  if (this.isOwner) {
    return true;  // Owners bypass enabled check!
  }
  return this.enabled && this.status === OrganizationUserStatusType.Confirmed;
}

Issue: If an organization is disabled, owners would still pass canAccess === true. The current code would then grant them phishing detection access based on their disabled org, which seems incorrect.

Test case to add: Org is disabled → user should NOT have access, even if they're an owner.

Important Concerns

Finding 4: Zero meaningful test coverage (20% patch coverage)

Location: apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts

The test file only includes a basic initialization test and has 2 TODO-commented tests that test the OLD logic (using hasPremiumFromAnySource$). The new access control logic (personal premium + org-based access) has zero test coverage.

Missing test scenarios:

Personal premium subscription → should have access
No personal premium, Family org with usersGetPremium → should have access
No personal premium, Family org without usersGetPremium → should NOT have access
No personal premium, Enterprise org with both flags → should have access
No personal premium, Enterprise org with only usersGetPremium → should NOT have access
No personal premium, Enterprise org with only usePhishingBlocker → should NOT have access
Disabled organization → should NOT have access
User status is Invited/Accepted → should NOT have access
Teams/TeamsStarter org with both flags → clarify expected behavior
Feature flag disabled → should NOT have access regardless of premium status

This is particularly concerning because:

Author noted "tests are difficult to add with current implementation"
20% coverage means 4 out of 5 new lines are untested
Access control logic is security-critical and prone to logic errors
No follow-up issue is linked for adding tests

Recommendation: Either:

Refactor the service to be testable and add comprehensive tests before merge
Create a blocking follow-up issue to add tests within the current sprint, and link it in the PR

Technical Debt

Finding 5: Static service with singleton pattern makes testing difficult

Location: apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts:49-203

The service uses static methods and static state (_tabUpdated$, _ignoredHostnames, _didInit), which makes unit testing extremely difficult. This is acknowledged in the code comment at line 195-197.

Impact:

Cannot mock dependencies effectively
Cannot test in isolation
Cannot test concurrent scenarios
Difficult to verify RxJS stream behavior

Code comment acknowledges this:

// Manually type cast to satisfy the listener signature due to the mixture
// of static and instance methods in this class. To be fixed when refactoring
// this class to be instance-based while providing a singleton instance in usage

Recommendation: File a technical debt issue to refactor this to an instance-based service with proper dependency injection, making it testable. This is blocking proper test coverage.

Questions Requiring Clarification

ProductTierType coverage: Should Teams or TeamsStarter organizations have access if they have the appropriate flags? These tiers support usePhishingBlocker based on the Organization model.
Families vs Enterprise consistency: Why do Families not require usePhishingBlocker check while Enterprise does?
PR scope mismatch: The PR description doesn't mention Enterprise access but the code implements it. Which is correct?
Test coverage plan: What's the timeline for adding comprehensive tests? Is there a follow-up issue?

Positive Observations

Observable patterns: Correctly uses RxJS Observable patterns with combineLatest and proper stream composition (ADR-0003 compliance)
Dependency injection: Properly adds OrganizationService to the constructor in both implementation and test files
Code documentation: Good inline comments explaining the three access paths (lines 124-128)
Feature flag gating: Maintains proper feature flag check before granting access

Recommendation

REQUEST CHANGES - The access control logic needs clarification on intended behavior, the permission inconsistency between org types should be resolved, and at minimum basic test coverage should be added to verify the security-critical access logic works as intended.

Files Reviewed

✅ apps/browser/src/background/main.background.ts - Correctly adds organizationService parameter
⚠️ apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.ts - Logic issues identified above
❌ apps/browser/src/dirt/phishing-detection/services/phishing-detection.service.spec.ts - Insufficient test coverage

Dec 04 '25 16:12 claude[bot]

Checkmarx One – Scan Summary & Details – 9b3c91b6-4007-4c2d-9e3b-69b8478f5603

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

Dec 04 '25 16:12 github-actions[bot]

Codecov Report

:x: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 41.73%. Comparing base (9f5dab0) to head (68b4d27). :warning: Report is 74 commits behind head on main. :white_check_mark: All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...g-detection/services/phishing-detection.service.ts	9.09%	10 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17818      +/-   ##
==========================================
+ Coverage   41.70%   41.73%   +0.02%     
==========================================
  Files        3571     3573       +2     
  Lines      103898   103691     -207     
  Branches    15618    15653      +35     
==========================================
- Hits        43335    43271      -64     
+ Misses      58724    58581     -143     
  Partials     1839     1839

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

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

Dec 04 '25 16:12 codecov[bot]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and all feature flags disabled.

✅ Fortunately, these BIT tests have passed! 🎉

Dec 09 '25 20:12 bw-ghapp[bot]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and the feature flag configuration used by vault.bitwarden.com.

✅ Fortunately, these BIT tests have passed! 🎉

Dec 09 '25 20:12 bw-ghapp[bot]

Closing since logic is captured in 17527

Dec 12 '25 16:12 Banrion