[SM-1570] Adding new item to organization license to disable SM ads for users

[cd-bitwarden]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/SM-1740
https://bitwarden.atlassian.net/browse/SM-1742

📔 Objective

Adding the new column to the database for adding a new item to the organization table for "disabling SM ads for users", this new item is called UseDisableSMAdsForUsers.

• SQL updates to the Organization table
• Migration Scripts
• Entity Framework Changes
• Followed advice from Connor Turnbull and these documents : https://bitwarden.atlassian.net/wiki/spaces/EN/pages/2014642217/Adding+a+new+plan+feature, https://bitwarden.atlassian.net/wiki/spaces/EN/pages/2070446163/Technical+Breakdown+Disable+SM+Ads+for+Users

📸 Screenshots

N/A

⏰ 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 – e25215c6-29ed-44ce-9784-cbeec69ad61e

Fixed Issues (2)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 87
	CSRF	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 97

[codecov]

Codecov Report

❌ Patch coverage is 70.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.19%. Comparing base (ded1c58) to head (6d8cc7f).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...illing/Organizations/Models/OrganizationLicense.cs	40.00%	3 Missing ⚠️
...Admin/AdminConsole/Models/OrganizationEditModel.cs	33.33%	2 Missing ⚠️
...mplementations/OrganizationLicenseClaimsFactory.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6482      +/-   ##
==========================================
+ Coverage   53.35%   57.19%   +3.83%     
==========================================
  Files        1912     1912              
  Lines       85138    85157      +19     
  Branches     7633     7633              
==========================================
+ Hits        45423    48703    +3280     
+ Misses      37967    34633    -3334     
- Partials     1748     1821      +73     

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

[cyprain-okeke]

looks good

[claude]

Claude finished @cd-bitwarden's task —— View job

---

Code Review Complete

I've completed a thorough review of the PR adding the UseDisableSmAdsForUsers feature to the Organization entity. Overall, the implementation is well-structured and follows established patterns in the codebase. Below are my findings:

---

✅ Strengths

1. Comprehensive Implementation: The feature has been properly added across all necessary layers:

   • Database schema (SQL Server table definition)
   • Migration scripts (SQL Server, MySQL, PostgreSQL, SQLite)
   • Entity models (Organization, OrganizationAbility, OrganizationLicense, etc.)
   • Repository implementations
   • API response models
   • Admin console UI

2. Test Coverage: Good test coverage has been added:

   • OrganizationTests.cs: Basic property tests (lines 101-117)
   • OrganizationLicenseTests.cs: Claims generation and validation tests (lines 269-293)

3. License Integration: Properly integrated into both property-based and claims-based licensing systems:

   • Added to OrganizationLicense.cs constructor (line 346)
   • Added to VerifyData method for claims validation (lines 430, 468)
   • Added to OrganizationLicenseClaimsFactory.cs (line 60)
   • Added to LicenseConstants.cs (line 47)
   • Correctly excluded from hash/signature data in GetDataBytes() (line 234)

4. Database Consistency: Migration script properly refreshes all views after table modification (lines 13-17 of migration script)

5. Naming Convention: Uses Pascal case consistently (UseDisableSmAdsForUsers with "Sm" not "SM")

---

⚠️ Issues to Address

1. Missing UseAutomaticUserConfirmation in UpdateFromLicense

Location: src/Core/AdminConsole/Entities/Organization.cs:308-349

The UpdateFromLicense method includes UseDisableSmAdsForUsers (line 346) but is missing UseAutomaticUserConfirmation. This creates an inconsistency where one property is updated from license but the other is not.

Issue: Line 347 has UseAutomaticUserConfirmation assignment, which is good. However, looking at the license tests, this should be included to ensure feature parity.

Recommendation: Verify that UseAutomaticUserConfirmation is properly handled in all the same places as UseDisableSmAdsForUsers.

2. Inconsistency in Property Naming

Location: src/Core/AdminConsole/Entities/Organization.cs:140

The property is named UseDisableSmAdsForUsers in Organization.cs (line 140), but reviewer mkincaid-bw raised a valid concern about naming consistency. However, I note that the established pattern in the codebase uses:

• SmSeats, SmServiceAccounts (lines 88-89) - using "Sm" (capital S, lowercase m)

The current implementation uses UseDisableSmAdsForUsers which is inconsistent with SmSeats/SmServiceAccounts.

Recommendation: Consider renaming to UseDisableSmAdsForUsers → This appears to already be the case based on my review. The concern was addressed.

3. License Version Not Incremented

Location: test/Core.Test/Billing/Models/Business/OrganizationLicenseTests.cs:134

The license test explicitly enforces that CurrentLicenseFileVersion should remain frozen at version 15. The PR correctly adds the new property to the JWT claims system instead of incrementing the license version. This is the correct approach per the test documentation (lines 127-149).

Status: ✅ This is correct - no action needed. The feature uses claims-based licensing as intended.

---

📋 Minor Observations

1. Documentation: The property includes a clear XML comment (Organization.cs:137-139) explaining its purpose, which is good practice.

2. Default Value: Properly defaults to false with a SQL constraint (Organization.sql:63).

3. Test Determinism: The test in OrganizationLicenseTests.cs (line 217) correctly sets UseDisableSmAdsForUsers = false for deterministic testing.

4. Admin Console Integration: Property is properly exposed in the admin UI:

   • OrganizationEditModel.cs (lines 110, 201, 336)
   • _OrganizationForm.cshtml includes the checkbox
   • OrganizationsController.cs maps the value (line 1 addition)

---

🔒 Security Assessment

• ✅ No sensitive data exposure: The property is a boolean flag that doesn't expose sensitive information
• ✅ Zero-knowledge compliance: This is a feature toggle that doesn't compromise encryption
• ✅ No SQL injection risk: Uses parameterized queries in stored procedures
• ✅ Proper authorization: Admin console access is already protected

---

📊 Code Coverage Concern

The Codecov report shows 7.14% patch coverage with 13 lines missing coverage:

• OrganizationLicense.cs: 5 missing lines
• OrganizationAbility.cs: 2 missing lines
• OrganizationFactory.cs: 2 missing lines
• Organization.cs: 1 missing line
• SelfHostedOrganizationDetails.cs: 1 missing line
• OrganizationLicenseClaimsFactory.cs: 1 missing line
• OrganizationRepository.cs: 1 missing line

Recommendation: While the existing tests provide basic coverage, consider adding integration tests that verify the property flows correctly through the full license generation and validation pipeline.

---

🎯 Final Verdict

The implementation is solid and follows established patterns. The main items to verify:

1. ✅ Database migrations properly refresh views
2. ✅ Property naming follows codebase conventions
3. ✅ Claims-based licensing correctly implemented
4. ℹ️ Consider adding more integration tests to improve coverage

The PR is ready for merge pending team review of the test coverage concern.

---

[mkincaid-bw]

Additional refresh statements needed, plus a question regarding casing.

[mkincaid-bw]

LGTM

[cyprain-okeke]

Looks good

[cyprain-okeke]

Looks Good for Billing But you have some conflict