Skip to content

[PM-31929] Add deletion days restriction to Send Controls policy #7506

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mcamirault wants to merge 2 commits into
base: tools/pm-31884/send-access-controls-policy
Choose a base branch
from
Open

[PM-31929] Add deletion days restriction to Send Controls policy #7506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
761f257
[PM-31929] Add deletion days restriction to Send Controls policy
mcamirault Apr 20, 2026
0d66de0
Merge branch 'tools/pm-31884/send-access-controls-policy' into tools/…
mcamirault Apr 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/Core/AdminConsole/Models/Data/Organizations/Policies/SendControlsPolicyData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,6 @@ public class SendControlsPolicyData : IPolicyDataModel
[Display(Name = "AllowedDomains")]
[StringLength(1000)]
public string? AllowedDomains { get; set; }
[Display(Name = "DeletionDays")]
public int? DeletionDays { get; set; }
}
4 changes: 4 additions & 0 deletions ...nConsole/OrganizationFeatures/Policies/PolicyEventHandlers/SendControlsSyncPolicyEvent.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,10 @@ private async Task UpdateSendsByPolicyAsync(Policy postUpsertedPolicyState, Send
{
await sendRepository.UpdateManyDisabledAsync(disabled, true);
}
if (sendControlsPolicyData.DeletionDays != null)
{
await sendRepository.UpdateManyDeletionDatesByIdsAsync(sendIdsChunk, sendControlsPolicyData.DeletionDays.GetValueOrDefault(0));
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 20, 2026

Choose a reason for hiding this comment

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

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

}
Comment on lines +112 to +115
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 20, 2026

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: Deletion dates are rewritten even when the policy is being disabled.

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.

}
}
}
8 changes: 8 additions & 0 deletions src/Core/Tools/Repositories/ISendRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,12 @@ UpdateEncryptedDataForKeyRotation UpdateForKeyRotation(Guid userId,
/// <param name="ids">The IDs of the <see cref="Send"/>ss to load</param>
/// <returns></returns>
Task<ICollection<Send>> GetManyByIdsAsync(IEnumerable<Guid> ids);

/// <summary>
/// Update <see cref="Send"/> deletion dates in bulk by IDs
/// </summary>
/// <param name="ids">The IDs of the <see cref="Send"/>s to update</param>
/// <param name="deletionHours">The number of hours after the <see cref="Send"/>s' creation dates to set the deletion date</param>
/// <returns></returns>
Task UpdateManyDeletionDatesByIdsAsync(IEnumerable<Guid> ids, int deletionHours);
}
9 changes: 9 additions & 0 deletions src/Infrastructure.Dapper/Tools/Repositories/SendRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,15 @@ public async Task<ICollection<Send>> GetManyByIdsAsync(IEnumerable<Guid> ids)
return sends;
}

public async Task UpdateManyDeletionDatesByIdsAsync(IEnumerable<Guid> ids, int deletionHours)
{
using var connection = new SqlConnection(ConnectionString);
await connection.ExecuteAsync(
$"[{Schema}].[Send_UpdateDeletionDatesByIds]",
new { Ids = ids.ToGuidIdArrayTVP(), DeletionHours = deletionHours },
commandType: CommandType.StoredProcedure);
}

private async Task ProtectDataAndSaveAsync(Send send, Func<Task> saveTask)
{
if (send == null)
Expand Down
14 changes: 14 additions & 0 deletions src/Infrastructure.EntityFramework/Tools/Repositories/SendRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,20 @@ public async Task<IEnumerable<Guid>> GetIdsByOrganizationIdAsync(Guid organizati
return Mapper.Map<List<Guid>>(orgUserSendIds);
}

public async Task UpdateManyDeletionDatesByIdsAsync(IEnumerable<Guid> ids, int deletionHours)
{
using var scope = ServiceScopeFactory.CreateScope();
var dbContext = GetDatabaseContext(scope);
var sends = dbContext.Sends.Where(s => ids.Contains(s.Id));
await sends.ExecuteUpdateAsync(setters => setters
.SetProperty(s => s.DeletionDate, s => s.CreationDate.AddHours(deletionHours))
.SetProperty(s => s.RevisionDate, DateTime.UtcNow)
);
var userIds = await sends.Select(s => s.User.Id).ToArrayAsync() ?? [];
await dbContext.UserBumpManyAccountRevisionDatesAsync(userIds);
await dbContext.SaveChangesAsync();
}

public async Task<ICollection<Core.Tools.Entities.Send>> GetManyByIdsAsync(IEnumerable<Guid> ids)
{
using var scope = ServiceScopeFactory.CreateScope();
Expand Down
27 changes: 27 additions & 0 deletions src/Sql/dbo/Tools/Stored Procedures/Send_UpdateDeletionDatesByIds.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
CREATE PROCEDURE [dbo].[Send_UpdateDeletionDatesByIds]
@Ids AS [dbo].[GuidIdArray] READONLY,
@DeletionHours INT
AS
BEGIN
SET NOCOUNT ON

-- Set field
UPDATE
[dbo].[Send]
SET
[DeletionDate] = DATEADD(HOUR, @DeletionHours, [CreationDate]),
[RevisionDate] = GETUTCDATE()
WHERE
[Id] IN (SELECT * FROM @Ids)
Comment on lines +9 to +15
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 20, 2026

Choose a reason for hiding this comment

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

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


-- Bump account revision dates
EXEC [dbo].[User_BumpManyAccountRevisionDates]
Copy link
Copy Markdown
Contributor

@mkincaid-bw mkincaid-bw Apr 20, 2026

Choose a reason for hiding this comment

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

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

(
SELECT DISTINCT
UserId
FROM
[dbo].[Send]
WHERE
[Id] IN (SELECT * FROM @Ids)
)
END
46 changes: 46 additions & 0 deletions ...ole/OrganizationFeatures/Policies/PolicyEventHandlers/SendControlsSyncPolicyEventTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,4 +449,50 @@ await sutProvider.GetDependency<ISendRepository>()
.Received(1)
.UpdateManyDisabledAsync(Arg.Is<List<Guid>>(l => l.Count == 3 && l.Contains(nonCompliantSend1.Id) && l.Contains(nonCompliantSend2.Id) && l.Contains(nonCompliantSend3.Id)), true);
}

[Theory, BitAutoData]
public async Task ExecutePostUpsertSideEffectAsync_DeletionDateUpdatesSendDeletionDates(
[PolicyUpdate(PolicyType.SendControls, enabled: true)] PolicyUpdate policyUpdate,
[Policy(PolicyType.SendControls, enabled: true)] Policy postUpsertedPolicy,
[Policy(PolicyType.DisableSend, enabled: false)] Policy existingDisableSendPolicy,
[Policy(PolicyType.SendOptions, enabled: false)] Policy existingSendOptionsPolicy,
SutProvider<SendControlsSyncPolicyEvent> sutProvider)
{
postUpsertedPolicy.OrganizationId = policyUpdate.OrganizationId;
existingDisableSendPolicy.OrganizationId = policyUpdate.OrganizationId;
existingSendOptionsPolicy.OrganizationId = policyUpdate.OrganizationId;
postUpsertedPolicy.SetDataModel(new SendControlsPolicyData { DeletionDays = 48 });

sutProvider.GetDependency<IPolicyRepository>()
.GetByOrganizationIdTypeAsync(policyUpdate.OrganizationId, PolicyType.DisableSend)
.Returns(existingDisableSendPolicy);
sutProvider.GetDependency<IPolicyRepository>()
.GetByOrganizationIdTypeAsync(policyUpdate.OrganizationId, PolicyType.SendOptions)
.Returns(existingSendOptionsPolicy);

var send1 = new Send
{
Id = Guid.NewGuid(),
CreationDate = DateTime.UtcNow
};
var send2 = new Send
{
Id = Guid.NewGuid(),
CreationDate = DateTime.UtcNow
};
var sendIds = new List<Guid>([ send1.Id, send2.Id ]);
sutProvider.GetDependency<ISendRepository>()
.GetIdsByOrganizationIdAsync(policyUpdate.OrganizationId)
.Returns(sendIds);
sutProvider.GetDependency<ISendRepository>()
.GetManyByIdsAsync(Arg.Any<IEnumerable<Guid>>())
.Returns([ send1, send2 ]);

await sutProvider.Sut.ExecutePostUpsertSideEffectAsync(
new SavePolicyModel(policyUpdate), postUpsertedPolicy, null);

await sutProvider.GetDependency<ISendRepository>()
.Received(1)
.UpdateManyDeletionDatesByIdsAsync(Arg.Is<Guid[]>(l => l.Count() == 2), 48);
}
}
27 changes: 27 additions & 0 deletions util/Migrator/DbScripts/2026-04-20_01_SendUpdateDeletionDaysByIds.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
CREATE OR ALTER PROCEDURE [dbo].[Send_UpdateDeletionDatesByIds]
@Ids AS [dbo].[GuidIdArray] READONLY,
@DeletionHours INT
AS
BEGIN
SET NOCOUNT ON

-- Set field
UPDATE
[dbo].[Send]
SET
[DeletionDate] = DATEADD(HOUR, @DeletionHours, [CreationDate]),
[RevisionDate] = GETUTCDATE()
WHERE
[Id] IN (SELECT * FROM @Ids)

-- Bump account revision dates
EXEC [dbo].[User_BumpManyAccountRevisionDates]
(
SELECT DISTINCT
UserId
FROM
[dbo].[Send]
WHERE
[Id] IN (SELECT * FROM @Ids)
)
END
Loading