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

[Banrion]

🎟️ Tracking

PM-29021

📔 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 permissions
  • usePhishingBlocker
  • usersGetPremium

⏰ 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

[claude]

Claude finished @Banrion's task —— View job

---

Code Review Complete

I've conducted a comprehensive review of the phishing detection access control changes following Bitwarden engineering standards.

---

📋 Review Summary

Overall Assessment: 🟡 REQUEST CHANGES

This PR correctly implements the new access control requirements (personal premium OR Family org with specific permissions), but has critical gaps in test coverage for security-sensitive code and a potential missing permission check.

---

🔴 Critical Issues

1. Missing Test Coverage for Security-Critical Logic

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

Issue: The new access control logic has NO test coverage despite being security-critical code that gates premium feature access. Codecov reports only 20% patch coverage with 4 lines missing coverage.

Why this is critical:

• Access control bugs could grant premium features to non-paying users (revenue loss)
• Or deny features to paying users (support burden, user frustration)
• Complex boolean logic with multiple conditions is error-prone without test verification
• Future changes to org permissions could break this without tests catching it

Required test cases:

1. ✅ User with personal premium subscription → should have access
2. ✅ User in Family org with all required permissions → should have access
3. ❌ User in Family org missing usePhishingBlocker → should NOT have access
4. ❌ User in Family org missing usersGetPremium → should NOT have access
5. ❌ User in Teams org with all permissions → should NOT have access (wrong tier)
6. ❌ User in Enterprise org with all permissions → should NOT have access (wrong tier)
7. ❌ User in disabled Family org → should NOT have access (canAccess check fails)
8. ❌ User with unconfirmed status in Family org → should NOT have access (canAccess fails)
9. ❌ Feature flag disabled → should NOT have access regardless of premium
10. ❌ User with no premium and no orgs → should NOT have access
11. ✅ User with multiple orgs where only one Family org qualifies → should have access

Recommendation: Add comprehensive test coverage before merge. I understand tests are planned for the constructor refactoring, but security-critical access control logic should have tests immediately, not deferred to future work.

---

2. Potentially Missing org.usePasswordManager Check

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

Issue: The code checks org.usePhishingBlocker, org.usersGetPremium, org.canAccess, and org.productTierType === Families, but does NOT verify org.usePasswordManager is enabled.

Context: Many other features in the codebase verify org.usePasswordManager before granting password manager-related features. Phishing detection is a password manager security feature.

Questions:

• Does PM-29021 specification explicitly exclude the usePasswordManager check?
• Can a Family organization have phishing blocker enabled without password manager enabled?
• Is this an oversight or intentional based on product requirements?

Recommendation: Verify with product requirements whether org.usePasswordManager check is needed. If yes, add it to line 149-153:

const hasAccessThroughFamilyOrg =
  organizations?.some(
    (org) =>
      org.canAccess &&
      org.usePasswordManager &&  // Add this?
      org.usePhishingBlocker &&
      org.usersGetPremium &&
      org.productTierType === ProductTierType.Families,
  ) ?? false;

---

🟡 Important Considerations

3. Observable Initialization Race Condition

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

Question: Does combineLatest([hasPremiumPersonally$, organizations$]) ensure both services are fully initialized before emitting?

Context: The code waits for BOTH observables to emit their first value. If either service is initialized asynchronously in main.background.ts, there could be a delay before phishing detection activates.

Scenarios:

• If billing service emits but org service hasn't: no access check completed
• If org service emits but billing service hasn't: no access check completed
• Only when BOTH emit: access check completes

Question: Is there any scenario during browser startup where one service is ready before the other, causing phishing detection to be delayed? This would be particularly important if a user navigates to a phishing site immediately after browser startup.

---

🟢 Positive Aspects

What's working well:

• ✅ Clear, detailed comments explaining access requirements (lines 124-130)
• ✅ Proper RxJS patterns using combineLatest and switchMap
• ✅ Follows Observable Data Services architecture (ADR-0003)
• ✅ Defensive null handling with ?? false operator
• ✅ Correct use of org.canAccess which includes both enabled and status === Confirmed checks
• ✅ Clean variable naming (hasAccessThroughFamilyOrg)
• ✅ Comment references PM-29021 requirements for exclusion of Teams/Enterprise orgs
• ✅ No TypeScript enum violations (ProductTierType is legacy)

---

🔵 Technical Debt

Commented Test Cases Need Resolution

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

Issue: The test file contains commented-out test cases that reference the old hasPremiumFromAnySource$ API.

Recommendation: Either:

1. Update these test cases to use the new API and enable them
2. Remove them entirely if they'll be replaced during constructor refactoring
3. Add a clear TODO comment referencing the follow-up work: TODO [PM-XXXXX]: Implement comprehensive access control tests during constructor refactoring

---

📊 Verdict

The access control logic appears correct and follows good RxJS patterns, but:

🔴 Blockers:

1. Missing critical test coverage for security-sensitive access control code
2. Needs product clarification on whether org.usePasswordManager check is required

🟡 Should address:
3. Clarify observable initialization timing to rule out race conditions

Recommendation: Address test coverage and verify the usePasswordManager requirement before merge. The lack of tests for security-critical logic poses too much risk for production deployment.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 123d9c37-01c3-4142-8c56-f21ead05aedc

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

[codecov]

Codecov Report

❌ Patch coverage is 20.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.70%. Comparing base (9f5dab0) to head (21183b6).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...g-detection/services/phishing-detection.service.ts	20.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17818      +/-   ##
==========================================
- Coverage   41.70%   41.70%   -0.01%     
==========================================
  Files        3571     3571              
  Lines      103898   103901       +3     
  Branches    15618    15621       +3     
==========================================
+ Hits        43335    43336       +1     
- Misses      58724    58726       +2     
  Partials     1839     1839              

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

[bw-ghapp]

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! 🎉

[bw-ghapp]

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! 🎉