Conversation
BTreston
added
ai-review
|
Claude Code is reviewing this pull request... If this comment does not update with results, check the Actions log. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8469 +/- ##
==========================================
+ Coverage 65.43% 71.05% +5.62%
==========================================
Files 2527 2527
Lines 108488 108508 +20
Branches 9871 9875 +4
==========================================
+ Hits 70988 77103 +6115
+ Misses 35150 28928 -6222
- Partials 2350 2477 +127 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| { | ||
| OrganizationId = organizationId, | ||
| UserId = e.send.UserId, | ||
| ActingUserId = _currentContext?.UserId, |
There was a problem hiding this comment.
♻️ DEBT: ProviderId is not populated, so these rows are invisible in the provider event log.
Details and fix
Every other organization-scoped log method in this file sets ProviderId = await GetProviderIdAsync(orgId) — LogPolicyEventAsync (line 278), CreateLogCipherEvent (line 166), LogCollectionEventsAsync, LogGroupEventsAsync. That field is what Event_ReadPageByProviderId filters on (IEventRepository.GetManyByProviderAsync).
When a provider user saves the SendControls policy for a managed client organization, the Policy_Updated row gets the provider attribution but these Send_PolicyDisabled / Send_PolicyEnabled rows do not, so the provider's event log shows the policy change with none of the resulting Send changes.
OrganizationId = organizationId,
UserId = e.send.UserId,
ActingUserId = _currentContext?.UserId,
ProviderId = providerId, // resolved once before the Select via GetProviderIdAsync(organizationId)If the omission is deliberate — keeping member-scoped Send rows out of provider-visible logs — a short comment here would keep the next reader from treating it as an oversight.
There was a problem hiding this comment.
I would check to see if these events should turn up in the Provider event log. @BTreston
| .Where(s => !s.Disabled && postUpsertedPolicyState.Enabled && !IsIgnored(s) && SendIsNonCompliant(s, sendControlsPolicyData)) | ||
| .ToList(); | ||
| var toEnable = sendsChunk | ||
| .Where(s => s.Disabled && !postUpsertedPolicyState.Enabled) |
There was a problem hiding this comment.
❓ QUESTION: Relaxing the policy without disabling it never restores Sends the policy itself disabled — is manual re-enable the intended recovery?
Details and fix
toEnable requires !postUpsertedPolicyState.Enabled, so re-enabling only happens when SendControls is switched off entirely. Consider an org that enables SendControls with DisableSend = true (every member Send gets Disabled = 1), then clears DisableSend but keeps the policy enabled for DisableHideEmail. Those Sends are now compliant, but the sweep leaves them disabled — and the legacy path does not help either, since DisableSendSyncPolicyEvent writes the SendControls row through IPolicyRepository.UpsertAsync and runs no sweep at all. Previously the else enabled.Add(send.Id) branch restored them.
ExecutePostUpsertSideEffectAsync_DoesNotReEnableOwnerDisabledCompliantSend documents the owner-disabled case as intentional, and the two are indistinguishable without recording why a Send was disabled. Owners can still re-enable their own Sends (nothing in NonAnonymousSendCommand blocks it), so this is recoverable rather than broken — but the relaxed-policy case has no test and is not mentioned in the description, so flagging it in case it was not weighed.
| WHERE | ||
| [OrganizationId] = @OrganizationId | ||
| AND [UserId] IS NOT NULL | ||
| AND [Status] IN (1, 2) -- 1 = Accepted, 2 = Confirmed |
There was a problem hiding this comment.
Should we include staged here?
There was a problem hiding this comment.
No. Staged users are less than invited. @sven-bitwarden can confirm.
| @@ -95,34 +97,32 @@ private async Task UpdateSendsByPolicyAsync(Policy postUpsertedPolicyState, Send | |||
| var orgOwnerAndAdminUserIds = (await orgUserRepository.GetManyByMinimumRoleAsync(postUpsertedPolicyState.OrganizationId, Core.Enums.OrganizationUserType.Admin)).Select(oud => oud.GetUserId()); | |||
| foreach (var sendIdsChunk in orgSendIds.Chunk(50)) | |||
There was a problem hiding this comment.
Which system has the limiting that causes the 50 record chunking?
| { | ||
| OrganizationId = organizationId, | ||
| UserId = e.send.UserId, | ||
| ActingUserId = _currentContext?.UserId, |
There was a problem hiding this comment.
I would check to see if these events should turn up in the Provider event log. @BTreston
This branch has not been deployed
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-43734
📔 Objective
Saving the SendControls organization policy sets
Disabled = 0over every Send owned byevery user who has an OrganizationUser row in that organization, enabling sends that should remain disabled. (i.e. sends of revoked members or personal sends that were manually disabled by its owner.)
📸 Screenshots