[PM-41863] Fix redundant Send event logging - #8327
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8327 +/- ##
=======================================
Coverage 63.99% 63.99%
=======================================
Files 2471 2471
Lines 105800 105816 +16
Branches 9596 9599 +3
=======================================
+ Hits 67705 67716 +11
- Misses 35745 35749 +4
- Partials 2350 2351 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR threads a Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Create/edit/delete events have no org context and can come from a request with no | ||
| // Device-Type header at all, e.g. DeleteSendsJob's scheduled expiration cleanup, which runs | ||
| // with no HTTP request and thus no CurrentContext.DeviceType. Report Server instead of unknown. | ||
| var deviceType = _currentContext.DeviceType; | ||
| if (deviceType == null && organizationContext == null) | ||
| { | ||
| deviceType = DeviceType.Server; | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: organizationContext == null also matches user-initiated create/edit/delete, so a client request without a Device-Type value is recorded as Server.
Details
CurrentContext.DeviceType is populated only from the Device-Type header or the devicetype JWT claim, and that claim is issued only when a device is attached to the token (BaseRequestValidator.cs:614). A user API key / client-credentials token carries neither, so a genuine POST /sends or DELETE /sends/{id} from such a caller now lands in the audit log attributed to Server rather than staying unattributed.
Elsewhere in this file the Server device type keys off an explicit system-actor signal (systemUser is EventSystemUser.SCIM) rather than the absence of a header. Would gating on something that positively identifies the non-HTTP caller — e.g. _currentContext.HttpContext is null, or an EventSystemUser passed by DeleteSendsJob — better match the intent described in the comment, and keep real client actions out of the Server bucket?
This is also worth calling out in the PR description: the objective covers only the redundant event, but this changes recorded DeviceType for every Send create/edit/delete event.
There was a problem hiding this comment.
❌ Incorrect
A user API key / client-credentials token carries neither, so a genuine POST /sends or DELETE /sends/{id} from such a caller now lands in the audit log attributed to Server rather than staying unattributed.
This is not true.
This is also worth calling out in the PR description: the objective covers only the redundant event, but this changes recorded DeviceType for every Send create/edit/delete event.
This is also not true.
The changes introduced in this PR do not attribute server to any client initiated Send Event.
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41863
📔 Objective
This PR eliminates a redundant Send event log that occurred when working with File type Sends. The root cause was that
SaveSendAsync()was being called twice as part of a two-stage process unique to File type Sends.