[SM-1570] Adding DisableSMAdsForUsers - Front end changes to disable SM ads for users

[cd-bitwarden]

🎟️ Tracking

https://bitwarden.atlassian.net/jira/software/c/projects/SM/boards/74?assignee=625cb516fd06270069beaf5d&selectedIssue=SM-1741

📔 Objective

Removing ads when the organization has useDisableSMAdsForUsers set to true, if the orgUser is of type User.

📸 Screenshots

For a user when the useDisableSMAdsForUsers is set to True:

⏰ 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 – 883c7973-cff0-4b82-a0fd-f8560f1439c4

New Issues (2)

Checkmarx found the following issues in this Pull Request

Severity	Issue	Source File / Package	Checkmarx Insight
	CVE-2025-10890	Npm-electron-36.9.3	details Recommended version: 37.10.2 Description: Side-channel information leakage in V8 in Google Chrome prior to 140.0.7339.207 allowed a remote attacker to leak cross-origin data via a crafted H... Attack Vector: NETWORK Attack Complexity: LOW ID: XKpdUkPcPfSt8V05l2NnfHdGxDCeApigigSeiy8DFAY%3D Vulnerable Package
	CVE-2025-8129	Npm-koa-2.16.1	details Recommended version: 2.16.3 Description: A vulnerability, which was classified as problematic, was found in KoaJS Koa versions through 2.16.1 and versions 3.0.0-alpha0 through 3.0.0. Affec... Attack Vector: NETWORK Attack Complexity: LOW ID: g3LaCCdVdCgT5QqmndjuvLXOyZ%2Br%2Bk9DUvA%2BbK79lO4%3D Vulnerable Package

[codecov]

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.33%. Comparing base (69d5c53) to head (57bfce4).
⚠️ Report is 271 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...roduct-switcher/shared/product-switcher.service.ts	75.00%	0 Missing and 1 partial ⚠️
...n-console/models/response/organization.response.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #17000   +/-   ##
=======================================
  Coverage   40.32%   40.33%           
=======================================
  Files        3494     3494           
  Lines       99800    99807    +7     
  Branches    14954    14955    +1     
=======================================
+ Hits        40249    40254    +5     
- Misses      57845    57846    +1     
- Partials     1706     1707    +1     

☔ 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 @cd-bitwarden's task —— View job

---

Code Review Complete

Todo List

• Read all changed files
• Analyze PR diff to understand changes since last review
• Review main business logic in product-switcher.service.ts
• Review model changes for consistency
• Check test coverage
• Identify security and performance concerns
• Provide comprehensive feedback

---

Summary of Changes

This PR adds support for disabling Secrets Manager (SM) ads for regular users in organizations. The implementation:

• Adds a new useDisableSMAdsForUsers field throughout the organization model hierarchy (response → data → domain)
• Updates the product switcher logic to check if SM ads should be hidden based on this flag and user role
• Adds test coverage for the new field in the spec file

Critical Issues

1. ⚠️ Logic Error: Incomplete Role Check

Location: apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts:218-220

const shouldDisableSMAds = orgs.some(
  (org) => org.useDisableSMAdsForUsers && org.type === OrganizationUserType.User,
);

Issue: The logic only checks if the user is of type User, but according to the comment and PR objective, this should exclude Admin and Owner roles. However, the current implementation doesn't properly handle all organization user types.

Problem: The OrganizationUserType enum likely includes more than just User, Admin, and Owner (e.g., Custom). The current check will disable ads even for users with custom roles, which may not be the intended behavior.

Suggested Fix:

// Check if SM ads should be disabled for any organization
// SM ads are only disabled if the user is a regular User (not Admin, Owner, or Custom)
// in an organization that has useDisableSMAdsForUsers enabled
const shouldDisableSMAds = orgs.some(
  (org) => 
    org.useDisableSMAdsForUsers && 
    org.type === OrganizationUserType.User &&
    !org.isOwner && 
    !org.isAdmin
);

Alternatively, if the intent is to only show ads to Admins and Owners:

const shouldDisableSMAds = orgs.some(
  (org) => 
    org.useDisableSMAdsForUsers && 
    org.type !== OrganizationUserType.Owner &&
    org.type !== OrganizationUserType.Admin
);

2. ❌ Missing Unit Tests for Business Logic

Location: apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts

Issue: The new business logic in product-switcher.service.ts (lines 215-220 and 281-284) has no unit tests. The only test added is in organization.data.spec.ts, which just verifies serialization of the field.

Impact:

• Cannot verify the logic correctly filters SM ads based on user role
• Cannot verify behavior when multiple orgs exist with different settings
• Cannot verify edge cases (e.g., user in multiple orgs, some with flag enabled, some disabled)
• The codecov report shows 75% patch coverage with 2 lines missing - this is the business logic!

Required Tests:

1. SM ads hidden when useDisableSMAdsForUsers=true and user is regular User
2. SM ads shown when useDisableSMAdsForUsers=true but user is Admin
3. SM ads shown when useDisableSMAdsForUsers=true but user is Owner
4. SM ads shown when useDisableSMAdsForUsers=false (default behavior)
5. Behavior when user belongs to multiple organizations with different settings
6. Behavior when user already has SM access (should still show in bento section)

3. 🤔 Potential Logic Flaw: "Any Organization" Check

Location: apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts:218

Issue: The logic uses .some(), meaning if the user belongs to any organization that has useDisableSMAdsForUsers=true and they're a User in that org, ads are disabled globally across all organizations.

Scenario:

• User belongs to Org A (role: User, useDisableSMAdsForUsers=true)
• User belongs to Org B (role: Owner, useDisableSMAdsForUsers=false)
• Result: SM ads are hidden even though Org B wants to show them

Question: Is this the intended behavior? Should the logic be more granular (per-org basis) or is the global check deliberate?

Alternative Approach (if per-org is intended):
The logic would need to be more complex, potentially checking the specific organization context when determining whether to show SM ads, rather than a blanket hide.

Suggested Improvements

1. 🎨 Improve Code Documentation

Location: apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts:215-217

The comment is helpful but could be more precise:

// Check if SM ads should be disabled for any organization
// SM ads are only disabled if the user is a regular User (not Admin or Owner)
// in an organization that has useDisableSMAdsForUsers enabled

Suggestion: Clarify the global nature of this check:

// Check if SM ads should be disabled globally for this user.
// Ads are hidden if the user has a regular User role (not Admin or Owner)
// in ANY organization that has the useDisableSMAdsForUsers flag enabled.
// This is a global check - if true for any org, ads are hidden across all orgs.

2. 🎨 Extract Magic Constant

Location: apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts:218-220

Issue: The logic for determining which roles should see ads is embedded in the service logic.

Suggestion: Consider extracting this to a helper method or constant for better testability and reusability:

private shouldShowSMAdForUserRole(org: Organization): boolean {
  // SM ads should be shown unless explicitly disabled for regular users
  if (!org.useDisableSMAdsForUsers) {
    return true;
  }
  
  // If disabled, only show ads to Admins and Owners
  return org.isAdmin || org.isOwner;
}

// Then in the main logic:
const shouldDisableSMAds = orgs.some(org => !this.shouldShowSMAdForUserRole(org));

3. 🎨 Type Safety: Default Value

Location: libs/common/src/admin-console/models/response/organization.response.ts:85

Issue: useDisableSMAdsForUsers doesn't have a default value if the API doesn't return it.

Suggestion: Add a default value to prevent undefined behavior:

this.useDisableSMAdsForUsers = this.getResponseProperty("UseDisableSMAdsForUsers") ?? false;

Same for profile-organization.response.ts:135.

4. 💭 Consider Custom Permission Users

Location: apps/web/src/app/layouts/product-switcher/shared/product-switcher.service.ts:218-220

Issue: The logic doesn't account for OrganizationUserType.Custom users who might have varying permissions.

Question: Should Custom users see SM ads? The current logic would hide ads for Custom users if the flag is enabled, which might not align with their actual permissions.

5. 📝 Documentation: Field Purpose

Location: Multiple model files

Observation: The new field useDisableSMAdsForUsers lacks inline documentation explaining its purpose and behavior.

Suggestion: Add JSDoc comments:

/**
 * When enabled, hides Secrets Manager marketing/ads from regular Users
 * (does not apply to Admins, Owners, or users who already have SM access)
 */
useDisableSMAdsForUsers: boolean;

Security Implications

✅ No security concerns identified. The changes are client-side UI logic only and don't affect:

• Actual access control (users still can't access SM without proper permissions)
• Data encryption or transmission
• Authentication/authorization flows

The worst case is showing/hiding UI elements incorrectly, which is a UX issue, not a security issue.

Performance Considerations

✅ No significant performance concerns. The .some() iteration over organizations is efficient and will short-circuit on first match. Most users have a small number of organizations.

Minor note: The logic runs on every product switcher evaluation (which is reactive based on route changes and syncs), but this is negligible overhead.

Good Practices Observed

• Consistent field propagation through the entire model hierarchy
• Updated spec file with new field for serialization testing
• Clear inline comments explaining business logic
• Used RxJS Observable pattern consistent with ADR-0003
• No TypeScript enums added (following ADR-0025)

Action Items for Author

1. HIGH PRIORITY: Add comprehensive unit tests for the product switcher logic

   • Test all user role combinations
   • Test multiple organization scenarios
   • Achieve >90% coverage on new logic

2. HIGH PRIORITY: Clarify and fix the role-checking logic

   • Verify intended behavior for Custom role users
   • Use helper methods from Organization class (isAdmin, isOwner) for consistency
   • Confirm "any org" vs "per-org" logic is correct

3. MEDIUM PRIORITY: Add default values for useDisableSMAdsForUsers in response models

4. MEDIUM PRIORITY: Add JSDoc documentation to the new field explaining its purpose

5. LOW PRIORITY: Consider extracting role-checking logic to a helper method for better testability

6. CLARIFICATION NEEDED: Confirm the intended behavior when a user belongs to multiple organizations with different flag settings

Example Test Structure

describe('ProductSwitcherService - SM Ads', () => {
  it('should hide SM ads for regular User when useDisableSMAdsForUsers is true', () => {
    // Test implementation
  });

  it('should show SM ads for Admin when useDisableSMAdsForUsers is true', () => {
    // Test implementation
  });

  it('should show SM ads for Owner when useDisableSMAdsForUsers is true', () => {
    // Test implementation
  });

  it('should show SM ads when useDisableSMAdsForUsers is false', () => {
    // Test implementation
  });

  it('should hide SM ads globally if any org has flag enabled and user is regular User', () => {
    // Test implementation for multiple org scenario
  });

  it('should still show SM in bento section when user has access', () => {
    // Verify smOrg logic takes precedence
  });
});

---