Skip to content

[PM-32187] Add Send Type restriction to Send Controls policy#7504

Open
mcamirault wants to merge 2 commits intotools/pm-31884/send-access-controls-policyfrom
tools/pm-32187/restrict-send-type-policy
Open

[PM-32187] Add Send Type restriction to Send Controls policy#7504
mcamirault wants to merge 2 commits intotools/pm-31884/send-access-controls-policyfrom
tools/pm-32187/restrict-send-type-policy

Conversation

@mcamirault
Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-32187

📔 Objective

This PR adds a field to the Send Controls policy that can be used to restrict which types of Sends can be created

📸 Screenshots

N/A

@mcamirault mcamirault requested a review from a team as a code owner April 20, 2026 02:59
@mcamirault mcamirault added the ai-review Request a Claude code review label Apr 20, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 20, 2026

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR adds a RestrictSendType field to the Send Controls policy so admins can limit which Send types are allowed, and wires the new check into SendControlsSyncPolicyEvent.UpdateSendsByPolicyAsync. The follow-up commit 9e682196b correctly fixes the parenthesis scoping flagged in the previous review by moving the RestrictSendType clause inside the postUpsertedPolicyState.Enabled guard, so re-enable behavior is preserved when the policy is disabled. A new unit test exercises the enabled path where non-matching Sends are disabled.

Code Review Details

No new findings. The previously flagged CRITICAL issue in src/Core/AdminConsole/OrganizationFeatures/Policies/PolicyEventHandlers/SendControlsSyncPolicyEvent.cs has been resolved by commit 9e682196b.

Comment on lines +100 to +101
(sendControlsPolicyData.WhoCanAccess == SendWhoCanAccessType.SpecificPeople && !SendValidationService.SendAllEmailsHaveAllowedDomains(send.Emails, sendControlsPolicyData.AllowedDomains))) ||
(sendControlsPolicyData.RestrictSendType != null && send.Type != sendControlsPolicyData.RestrictSendType))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CRITICAL: RestrictSendType check is outside the postUpsertedPolicyState.Enabled guard, breaking the re-enable behavior when the policy is disabled.

Details and fix

Counting parentheses, the current condition parses as:

(postUpsertedPolicyState.Enabled && (DisableSend || ... || AllowedDomains check))
||
(RestrictSendType != null && send.Type != RestrictSendType)

The new disjunct on line 101 is outside the Enabled guard. This regresses the intent documented on line 94 ("If the policy is disabled then we want to re-enable any Sends that were previously disabled").

Repro scenario:

  1. Admin enables SendControls with RestrictSendType = TextFile Sends get disabled (correct).
  2. Admin later disables the SendControls policy. Policy data still contains RestrictSendType = Text.
  3. Handler runs: postUpsertedPolicyState.Enabled == false, but RestrictSendType != null and send.Type != RestrictSendType for File Sends, so those File Sends are added to disabled (line 103) and UpdateManyDisabledAsync(..., true) is called, leaving them disabled instead of re-enabling them.

Fix — move the new clause inside the Enabled guard (same group as the other checks):

Suggested change
(sendControlsPolicyData.WhoCanAccess == SendWhoCanAccessType.SpecificPeople && !SendValidationService.SendAllEmailsHaveAllowedDomains(send.Emails, sendControlsPolicyData.AllowedDomains))) ||
(sendControlsPolicyData.RestrictSendType != null && send.Type != sendControlsPolicyData.RestrictSendType))
(sendControlsPolicyData.WhoCanAccess == SendWhoCanAccessType.SpecificPeople && !SendValidationService.SendAllEmailsHaveAllowedDomains(send.Emails, sendControlsPolicyData.AllowedDomains)) ||
(sendControlsPolicyData.RestrictSendType != null && send.Type != sendControlsPolicyData.RestrictSendType)))

Also consider adding a test that sets RestrictSendType with postUpsertedPolicy.Enabled == false and asserts non-matching Sends are re-enabled (not disabled) — the current new test only exercises the enabled path.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 20, 2026

Logo
Checkmarx One – Scan Summary & Details764d7e56-3392-45bd-9225-d0a5151aae67


New Issues (1) Checkmarx found the following issues in this Pull Request
# Severity Issue Source File / Package Checkmarx Insight
1 MEDIUM CSRF src/Api/AdminConsole/Controllers/GroupsController.cs: 275
detailsMethod at line 275 of /src/Api/AdminConsole/Controllers/GroupsController.cs gets a parameter from a user request from orgUserId. This parameter ...
Attack Vector

Fixed Issues (2) Great job! The following issues were fixed in this Pull Request
Severity Issue Source File / Package
MEDIUM CSRF src/Api/AdminConsole/Controllers/GroupsController.cs: 289
MEDIUM CSRF src/Api/Vault/Controllers/CiphersController.cs: 287

@mcamirault mcamirault force-pushed the tools/pm-32187/restrict-send-type-policy branch from 95a40df to 72a8b71 Compare April 23, 2026 14:19
@sonarqubecloud
Copy link
Copy Markdown

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.08%. Comparing base (44dd3cd) to head (9e68219).

Additional details and impacted files
@@                              Coverage Diff                               @@
##           tools/pm-31884/send-access-controls-policy    #7504      +/-   ##
==============================================================================
- Coverage                                       63.58%   59.08%   -4.50%     
==============================================================================
  Files                                            2077     2078       +1     
  Lines                                           91970    91823     -147     
  Branches                                         8192     8168      -24     
==============================================================================
- Hits                                            58482    54257    -4225     
- Misses                                          31465    35629    +4164     
+ Partials                                         2023     1937      -86     

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants