[PM-31885] Consolidate all Send policies to a single policy#7113
[PM-31885] Consolidate all Send policies to a single policy#7113
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7113 +/- ##
==========================================
+ Coverage 57.62% 57.68% +0.05%
==========================================
Files 2032 2037 +5
Lines 89517 89636 +119
Branches 7974 7988 +14
==========================================
+ Hits 51587 51704 +117
- Misses 36071 36072 +1
- Partials 1859 1860 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@claude please review |
|
Claude finished @harr1424's task in 5m 16s —— View job Code Review: PM-31885 Consolidate all Send policies to a single policy
SummaryThis PR introduces a new Overall, the architecture is sound. The bidirectional sync approach via Issues1.
|
…ntrols-Policy merge main
util/Migrator/DbScripts_transition/2026-02-28_00_CreateSendControlsPolicies.sql
Outdated
Show resolved
Hide resolved
• `IX_Policy_OrganizationId_Type` is a unique index Co-authored-by: mkincaid-bw <mkincaid@bitwarden.com>
…rden/server into tools/PM-31885-SendControls-Policy merge suggested changes to migration
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Nice use of the policy framework. General approach looks good to me.
...dminConsole/OrganizationFeatures/Policies/PolicyValidators/DisableSendSyncPolicyValidator.cs
Outdated
Show resolved
Hide resolved
| @@ -0,0 +1,54 @@ | |||
| #nullable enable | |||
There was a problem hiding this comment.
Not required - nullable is the default.
| policy.SetNewId(); | ||
| } | ||
|
|
||
| policy.Enabled = data.DisableSend || data.DisableHideEmail; |
There was a problem hiding this comment.
The configuration of a policy (in its data) is separate to its enabled/disabled state. I think the proper mapping would be: sendControlsPolicy.Enabled = disableSendPolicy.Enabled || SendOptionsPolicy.Enabled. (not the variable names you have here, example only)
This doesn't make a difference for DisableSend, because it's mapped from the Enabled state anyway, but it does for DisableHideEmail.
| var policy = sendControlsPolicy ?? new Policy | ||
| { | ||
| OrganizationId = policyUpdate.OrganizationId, | ||
| Type = PolicyType.SendControls, | ||
| }; |
There was a problem hiding this comment.
All the different policy objects floating around here are a bit confusing. Maybe handle this upfront:
var sendControlsPolicy = await policyRepository.GetByOrganizationIdTypeAsync(
policyUpdate.OrganizationId, PolicyType.SendControls) ?? new Policy
{
Id = CoreHelpers.GenerateComb(),
OrganizationId = policyUpdate.OrganizationId,
Type = PolicyType.SendControls,
};
So you can then just deal with this object without worrying whether it existed or not.
| /// Runs regardless of the pm-31885-send-controls feature flag to ensure SendControls | ||
| /// always stays current for when the flag is eventually enabled. | ||
| /// </summary> | ||
| public class DisableSendSyncPolicyValidator(IPolicyRepository policyRepository) : IOnPolicyPostUpdateEvent |
There was a problem hiding this comment.
The existing classes are also Validators, but this is not. A more accurate name could be something like
| public class DisableSendSyncPolicyValidator(IPolicyRepository policyRepository) : IOnPolicyPostUpdateEvent | |
| public class DisableSendSyncUpdateEvent(IPolicyRepository policyRepository) : IOnPolicyPostUpdateEvent |
| p.OrganizationId == policyUpdate.OrganizationId && | ||
| p.Type == PolicyType.SendControls && | ||
| p.Enabled == true && | ||
| (CoreHelpers.LoadClassFromJsonData<SendControlsPolicyData>(p.Data)!.DisableSend == true))); |
There was a problem hiding this comment.
Tests should also use the getter to access the data model.
| SELECT DISTINCT | ||
| COALESCE(ds.OrganizationId, so.OrganizationId) AS OrganizationId, | ||
| ds.Enabled AS DisableSendEnabled, | ||
| so.Enabled AS SendOptionsEnabled, | ||
| so.Data AS SendOptionsData | ||
| FROM | ||
| [dbo].[Policy] ds | ||
| LEFT JOIN | ||
| [dbo].[Policy] so | ||
| ON ds.OrganizationId = so.OrganizationId | ||
| AND so.Type = @SendOptionsType | ||
| WHERE | ||
| ds.Type = @DisableSendType | ||
| UNION | ||
| SELECT | ||
| so.OrganizationId, | ||
| NULL, | ||
| so.Enabled, | ||
| so.Data | ||
| FROM | ||
| [dbo].[Policy] so | ||
| WHERE | ||
| so.Type = @SendOptionsType | ||
| AND NOT EXISTS ( | ||
| SELECT | ||
| 1 | ||
| FROM | ||
| [dbo].[Policy] ds | ||
| WHERE | ||
| ds.OrganizationId = so.OrganizationId | ||
| AND ds.Type = @DisableSendType |
There was a problem hiding this comment.
dbops will be a better source of advice on this, but the design of this query is a little unusual to me. "select send options with left join on disable send, but also UNION with select disable send where there are no send options". Couldn't we just select send options UNION select disable send?
There was a problem hiding this comment.
@mkincaid-bw do you have any input on this?
There was a problem hiding this comment.
I was thinking about this further, and my suggestion isn't correct. UNION just concatenates rows to the previous result, which is not what you're after here. Your combined result is essentially a PIVOT operation, changing rows to columns. But I think it can be simplified to avoid this just by breaking it down into multiple CTEs.
Here's what I'm thinking (please note this would need to be verified, tested and adjusted to work with batching):
-- Step 1: Extract all DisableSend policies
WITH DisableSendPolicies AS (
SELECT
OrganizationId,
Enabled AS DisableSendEnabled
FROM [dbo].[Policy]
WHERE Type = @DisableSendType
),
-- Step 2: Extract all SendOptions policies
SendOptionsPolicies AS (
SELECT
OrganizationId,
Enabled AS SendOptionsEnabled,
Data AS SendOptionsData
FROM [dbo].[Policy]
WHERE Type = @SendOptionsType
),
-- Step 3: Combine both policy types (all orgs with either policy)
CombinedPolicies AS (
SELECT DISTINCT
COALESCE(ds.OrganizationId, so.OrganizationId) AS OrganizationId,
ds.DisableSendEnabled,
so.SendOptionsEnabled,
so.SendOptionsData
FROM DisableSendPolicies ds
FULL OUTER JOIN SendOptionsPolicies so
ON ds.OrganizationId = so.OrganizationId
),
-- Step 4: Filter out orgs that already have SendControls policy
OrgsNeedingMigration AS (
SELECT
OrganizationId,
DisableSendEnabled,
SendOptionsEnabled,
SendOptionsData
FROM CombinedPolicies cp
WHERE NOT EXISTS (
SELECT 1
FROM [dbo].[Policy] sc
WHERE sc.OrganizationId = cp.OrganizationId
AND sc.Type = @SendControlsType
)
)
-- Step 5: Insert new SendControls policies
-- ....etc| SELECT lower(hex(randomblob(4))) || '-' || lower(hex(randomblob(2))) || '-4' || | ||
| substr(lower(hex(randomblob(2))),2) || '-' || | ||
| substr('89ab',abs(random()) % 4 + 1, 1) || | ||
| substr(lower(hex(randomblob(2))),2) || '-' || lower(hex(randomblob(6))), |
There was a problem hiding this comment.
Again I will defer to dbops, but this doens't seem like the right way to generate guids.
There was a problem hiding this comment.
@mkincaid-bw Sqlite doesn't seem to have a function equivalent to NEWID() or UUID() etc. do you have any input on the approach used here? For context it's a migration script, I don't see a way to generate GUIDs in C# or something similar. I haven't been able to find any precedence for what we are trying to accomplish here.
| -- Build JSON: use ISJSON guard for SendOptions.Data | ||
| N'{"disableSend":' + | ||
| CASE WHEN ISNULL(combined.DisableSendEnabled, 0) = 1 | ||
| THEN N'true' ELSE N'false' END + | ||
| N',"disableHideEmail":' + | ||
| CASE WHEN combined.SendOptionsData IS NOT NULL | ||
| AND ISJSON(combined.SendOptionsData) = 1 | ||
| AND JSON_VALUE(combined.SendOptionsData, '$.disableHideEmail') = 'true' | ||
| THEN N'true' ELSE N'false' END + | ||
| N'}', |
There was a problem hiding this comment.
Could we use one of the json functions here, e.g. https://learn.microsoft.com/en-us/sql/t-sql/functions/json-object-transact-sql?view=sql-server-ver17 ?
There was a problem hiding this comment.
@mkincaid-bw I couldn't find any existing usage of JSON_OBJECT() in the solution, and see this was introduced in SQL Server 2022. Could this lead to any potential problems?
There was a problem hiding this comment.
Data migrations can be pretty high risk - definitely talk to dbops about this. They may want to run it manually to control/monitor performance if it is expected to take a while (especially with json parsing).
You should also write integration tests for this - see https://contributing.bitwarden.com/contributing/testing/database/#testing-data-migrations. Note that these tests are not permanent, but they give you confidence when merging your PR - especially in a non-trivial data migration script like this one, with separate implementations per database provider.
| if (!featureService.IsEnabled(FeatureFlagKeys.SendControls)) | ||
| { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Not sure if this guard is needed - you don't expect any updates if the feature flag is off, sure, but if you get one - wouldn't you still want to sync it?
Note that the feature flag state in clients can lag behind server (by whatever the refresh interval is), so it's possible that an admin edits the Send Controls policy on the front-end even though the feature flag has been turned off.
…rden/server into tools/PM-31885-SendControls-Policy merge main
- fix SQL syntax error - escape Sqlite format specifier - update migration IDs to match sorted filename - fix SQL syntax error
|
|
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-31885
📔 Objective
Consolidate all Send policies, including new policies, to a single policy.
Please see TBD for full context.
This PR introduces a new
SendControlspolicy, that is intended to absorb the existing Send policiesDisableSendandDisableHideEmailand serve as a container for upcoming Send related policies as part of PM-31883.Of note in this PR are multiple
PolicyValidatorsused to ensure policy statuses are synced between the old and new implementation across feature flag status changes or potential rollbacks.