[PM-31885] Consolidate all Send policies to a single policy#7113
[PM-31885] Consolidate all Send policies to a single policy#7113
Conversation
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7113 +/- ##
==========================================
+ Coverage 56.87% 61.02% +4.14%
==========================================
Files 2028 2033 +5
Lines 88827 88946 +119
Branches 7917 7935 +18
==========================================
+ Hits 50523 54276 +3753
+ Misses 36472 32756 -3716
- Partials 1832 1914 +82 ☔ 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?
| 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.
| -- 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.
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.
|
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.