[PM-31929] Add deletion days restriction to Send Controls policy#7506
[PM-31929] Add deletion days restriction to Send Controls policy#7506mcamirault wants to merge 2 commits intotools/pm-31884/send-access-controls-policyfrom
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR adds a Code Review Details
Additional minor observations not posted as inline comments:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| if (sendControlsPolicyData.DeletionDays != null) | ||
| { | ||
| await sendRepository.UpdateManyDeletionDatesByIdsAsync(sendIdsChunk, sendControlsPolicyData.DeletionDays.GetValueOrDefault(0)); |
There was a problem hiding this comment.
❌ CRITICAL: Units mismatch — DeletionDays is passed directly as deletionHours.
Details and fix
The policy field is SendControlsPolicyData.DeletionDays (int?), but the repository method is declared as UpdateManyDeletionDatesByIdsAsync(IEnumerable<Guid> ids, int deletionHours) and the stored procedure / EF implementation both do DATEADD(HOUR, @DeletionHours, [CreationDate]) / CreationDate.AddHours(deletionHours).
Tracing the call chain: an admin who sets DeletionDays = 7 ends up with sends whose DeletionDate = CreationDate + 7 hours, i.e., ~7 hours instead of 7 days. This is off by a factor of 24.
The test case even asserts the broken behavior directly: DeletionDays = 48 is asserted to be passed as 48 to a method that interprets it as hours.
Either convert at the call site:
await sendRepository.UpdateManyDeletionDatesByIdsAsync(
sendIdsChunk,
sendControlsPolicyData.DeletionDays.GetValueOrDefault(0) * 24);Or rename the policy field/column to match the unit actually stored (e.g., DeletionHours), and update the DB column name, migration file (2026-04-20_01_SendUpdateDeletionDaysByIds.sql), SendControlsPolicyRequirement, and all callers / UI consumers accordingly.
Recommended: change the SQL/EF implementations to use DATEADD(DAY, @DeletionDays, ...) / .AddDays(...) and rename the parameter to @DeletionDays / deletionDays. That keeps the policy unit semantics consistent with the field name and with how admins will likely reason about Send retention.
| if (sendControlsPolicyData.DeletionDays != null) | ||
| { | ||
| await sendRepository.UpdateManyDeletionDatesByIdsAsync(sendIdsChunk, sendControlsPolicyData.DeletionDays.GetValueOrDefault(0)); | ||
| } |
There was a problem hiding this comment.
Details and fix
The existing enable/disable branch above intentionally checks postUpsertedPolicyState.Enabled — the comment on line 94 notes "If the policy is disabled then we want to re-enable any Sends that were previously disabled." The new DeletionDays branch ignores that guard. If an admin disables the SendControls policy (or its previously-enforced DeletionDays value), every Send in the org still gets DeletionDate reset to CreationDate + DeletionDays. That contradicts the semantics of disabling the policy.
Suggested fix:
if (postUpsertedPolicyState.Enabled && sendControlsPolicyData.DeletionDays != null)
{
await sendRepository.UpdateManyDeletionDatesByIdsAsync(
sendIdsChunk,
sendControlsPolicyData.DeletionDays.Value);
}Consider also whether only changes to DeletionDays (compared to previousPolicyState) should trigger this update, to avoid rewriting deletion dates on every unrelated save.
| UPDATE | ||
| [dbo].[Send] | ||
| SET | ||
| [DeletionDate] = DATEADD(HOUR, @DeletionHours, [CreationDate]), | ||
| [RevisionDate] = GETUTCDATE() | ||
| WHERE | ||
| [Id] IN (SELECT * FROM @Ids) |
There was a problem hiding this comment.
❓ QUESTION: Should user-set deletion dates that are earlier than CreationDate + DeletionDays be preserved?
Details
This proc unconditionally overwrites DeletionDate for every Send in the chunk. If a user created a Send with a deliberately short deletion (e.g., CreationDate + 1 day while the policy is DeletionDays = 7), this will extend its lifetime to 7 days — the opposite of what a retention-max policy usually intends.
If DeletionDays is meant as a maximum retention cap, consider:
UPDATE [dbo].[Send]
SET
[DeletionDate] = DATEADD(DAY, @DeletionDays, [CreationDate]),
[RevisionDate] = GETUTCDATE()
WHERE
[Id] IN (SELECT * FROM @Ids)
AND [DeletionDate] > DATEADD(DAY, @DeletionDays, [CreationDate])(mirror the same guard in the EF implementation).
If it is instead intended to force all Sends to exactly CreationDate + DeletionDays, please confirm that's the product requirement — that behavior effectively resets users' chosen deletion dates whenever any SendControls policy setting is saved.
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
jrmccannon
left a comment
jrmccannon
left a comment
There was a problem hiding this comment.
The name of the variable as DeletionHours but using Days is confusing.
Should it be hours or days?
| [Id] IN (SELECT * FROM @Ids) | ||
|
|
||
| -- Bump account revision dates | ||
| EXEC [dbo].[User_BumpManyAccountRevisionDates] |
There was a problem hiding this comment.
This does not work as expected and oddly, does not generate an error, despite being invalid SQL syntax. Instead you need to do something like this:
-- Near the top of stored proc
DECLARE @UserIds [dbo].[GuidIdArray]
--rest of stored procedure
INSERT INTO @UserIds
SELECT DISTINCT
UserId
FROM
[dbo].[Send]
WHERE
[Id] IN (SELECT * FROM @Ids)
AND [UserId] IS NOT NULL
EXEC [dbo].[User_BumpManyAccountRevisionDates] @UserIds…pm-31929/send-deletion-days-policy
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## tools/pm-31884/send-access-controls-policy #7506 +/- ##
==============================================================================
- Coverage 14.30% 14.30% -0.01%
==============================================================================
Files 1278 1278
Lines 55670 55694 +24
Branches 4317 4319 +2
==============================================================================
Hits 7966 7966
- Misses 47564 47588 +24
Partials 140 140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ails in SyncController (#7506) (#7529) - Added support for retrieving confirmed accepted policies and organization user details based on the feature flag 'PoliciesInAcceptedState'. - Updated SyncResponseModel to include new properties for these details. - Enhanced SyncControllerTests to verify behavior with the feature flag enabled and disabled.
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31929
📔 Objective
This PR adds a field to the Send Controls policy that allows specifying a deletion date restriction on Sends created in an organization
📸 Screenshots
N/A