[PM-20041] Deleting Notifications when Task is completed #5896

nick-livefront · 2025-05-28T20:44:18Z

🎟️ Tracking

PM-20041
Client PR: bitwarden/clients#14980

📔 Objective

When a security task is marked as complete, all notifications associated with that task should be marked as deleted.

Add GetActiveByTaskIdAsync query to find all non-deleted notifications associated with a task id.
Add MarkNotificationsForTaskAsDeletedCommand to mark all notifications as deleted for that task.
- This largely follows the patterns in MarkNotificationDeletedCommand. Because a single user can complete a task and then delete notifications for any user, this omits the authorization checks that are in MarkNotificationDeletedCommand.
  ❓ If there should be some authorization checks outside of the SecurityTasks checks, let me know.

📸 Screenshots

mark-notification-as-deleted.mov

…n the task is completed

codecov · 2025-05-28T20:50:15Z

Codecov Report

Attention: Patch coverage is 7.14286% with 65 lines in your changes missing coverage. Please review.

Project coverage is 51.01%. Comparing base (812fe94) to head (bde0aa3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ationCenter/Repositories/NotificationRepository.cs	0.00%	38 Missing ⚠️
...mmands/MarkNotificationsForTaskAsDeletedCommand.cs	0.00%	15 Missing ⚠️
...ationCenter/Repositories/NotificationRepository.cs	0.00%	12 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5896      +/-   ##
==========================================
+ Coverage   47.73%   51.01%   +3.28%     
==========================================
  Files        1665     1666       +1     
  Lines       75014    75084      +70     
  Branches     6755     6761       +6     
==========================================
+ Hits        35806    38304    +2498     
+ Misses      37743    35260    -2483     
- Partials     1465     1520      +55

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

github-actions · 2025-05-28T20:54:00Z

Checkmarx One – Scan Summary & Details – 92f7a257-59fc-4ab5-831e-0edd23105b8e

New Issues (3)

Checkmarx found the following issues in this Pull Request

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Api/Controllers/CollectionsController.cs: 143	details Method Post at line 143 of /src/Api/Controllers/CollectionsController.cs gets a parameter from a user request from orgId. This parameter value fl... `ID: W5s%2FSNe54CPP1CxzYuVqrn9v0%2Fk%3D` Attack Vector
CSRF	/src/Api/AdminConsole/Controllers/GroupsController.cs: 135	details Method Post at line 135 of /src/Api/AdminConsole/Controllers/GroupsController.cs gets a parameter from a user request from orgId. This parameter ... `ID: Qs417oLwYMuJ1g8cUCMHpbUJ9d0%3D` Attack Vector
Missing_CSP_Header	/src/Core/MailTemplates/Handlebars/Layouts/Full.html.hbs: 164	details A Content Security Policy is not explicitly defined within the web-application. `ID: iDkz8rv3w1QoR%2BKANf%2FBdTS52Xc%3D` Attack Vector

Fixed Issues (2)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	~~CSRF~~	/src/Billing/Controllers/PayPalController.cs: 52
	~~CSRF~~	/src/Api/SecretsManager/Controllers/SecretsController.cs: 201

src/Core/NotificationCenter/Repositories/INotificationRepository.cs

src/Core/Vault/Commands/MarkNotificationsForTaskAsDeletedCommand.cs

…e queries and insertions from the C#

withinfocus

I am confused about the notification spread related to the task.

src/Core/Vault/Commands/MarkNotificationsForTaskAsDeletedCommand.cs

src/Core/NotificationCenter/Repositories/INotificationRepository.cs

src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_MarkAsDeletedByTask.sql

The userId from the notification will be used

…/pm-20041/mark-task-complete

withinfocus

Makes sense to me now. Thanks for working through this with me.

Testing will still be important to see the performance and effect across several users' "inboxes" so let's be sure to include that in testing notes.

shane-melton

Really great work on this! 🚀 I only noticed a comment in the sprocs that is now out of date after some back and forth in the other discussions on the PR.

shane-melton · 2025-06-04T00:00:42Z

src/Sql/NotificationCenter/dbo/Stored Procedures/Notification_MarkAsDeletedByTask.sql

+BEGIN
+    SET NOCOUNT ON;
+
+    -- Collect NotificationIds as they are altered


⛏️ Tiny nit, but we're collecting UserIds here

shane-melton · 2025-06-04T00:01:07Z

util/Migrator/DbScripts/2025-05-30_00_Notification_MarkAsDeletedByTask.sql

+BEGIN
+    SET NOCOUNT ON;
+
+    -- Collect NotificationIds as they are altered


⛏️ Tiny nit, but we're collecting UserIds here

withinfocus · 2025-06-04T13:25:43Z

src/Core/NotificationCenter/Repositories/INotificationRepository.cs

+    /// </summary>
+    /// <param name="taskId">The unique identifier of the task.</param>
+    /// <returns>
+    /// A collection of users ids for the notifications that are now marked as deleted.


If we are ⛏️ing, the different forms of "ID" triggered my OCD but I chose not to mention it 😜 so fix those if you'd like.

I'm all for it 963c7e9

…/pm-20041/mark-task-complete

withinfocus

Deferring to your team for final approval.

sonarqubecloud · 2025-06-04T14:50:43Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
33.7% Duplication on New Code

See analysis details on SonarQube Cloud

nick-livefront added 3 commits May 28, 2025 15:33

mark all notifications associated with a security task as deleted whe…

0e277ad

…n the task is completed

fix spelling

31c7848

formatting

089fba6

nick-livefront requested review from a team as code owners May 28, 2025 20:44

nick-livefront requested a review from gbubemismith May 28, 2025 20:44

nick-livefront mentioned this pull request May 28, 2025

[PM-20041] Marking Task as complete bitwarden/clients#14980

Open

withinfocus reviewed May 28, 2025

View reviewed changes

nick-livefront added 2 commits May 28, 2025 21:47

refactor "Active" to "NonDeleted"

f553fd7

refactor "Active" to "NonDeleted" for stored procedure

d15e769

nick-livefront requested a review from shane-melton May 29, 2025 14:57

nick-livefront added 2 commits May 29, 2025 11:04

only send notifications per user for each notification

d295805

move notification status updates into the DB layer to save on multipl…

928bad3

…e queries and insertions from the C#

nick-livefront requested a review from withinfocus May 29, 2025 21:03

withinfocus requested changes May 30, 2025

View reviewed changes

nick-livefront added 3 commits May 31, 2025 20:15

Only return UserIds from db layer

8d40d66

omit userId from MarkTaskAsCompletedCommand query.

06f9f15

The userId from the notification will be used

Merge branch 'main' of https://github.com/bitwarden/server into vault…

f0b42cc

…/pm-20041/mark-task-complete

This was referenced Jun 2, 2025

Refactor PendingSecurityTasks to RefreshSecurityTasks #5903

Draft

Refactor PendingSecurityTasks to RefreshSecurityTasks bitwarden/clients#15021

Draft

nick-livefront requested a review from withinfocus June 2, 2025 02:10

withinfocus previously approved these changes Jun 3, 2025

View reviewed changes

shane-melton requested changes Jun 4, 2025

View reviewed changes

withinfocus reviewed Jun 4, 2025

View reviewed changes

nick-livefront added 3 commits June 4, 2025 09:40

update UserIds

9850fdc

consistency in comments regarding taskId and UserId

963c7e9

Merge branch 'main' of https://github.com/bitwarden/server into vault…

bde0aa3

…/pm-20041/mark-task-complete

nick-livefront dismissed withinfocus’s stale review via bde0aa3 June 4, 2025 14:45

nick-livefront requested review from withinfocus and shane-melton June 4, 2025 14:48

withinfocus approved these changes Jun 4, 2025

View reviewed changes

[PM-20041] Deleting Notifications when Task is completed #5896

Are you sure you want to change the base?

[PM-20041] Deleting Notifications when Task is completed #5896

Conversation

nick-livefront commented May 28, 2025 • edited by jira bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

📸 Screenshots

Uh oh!

codecov bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

shane-melton left a comment

Choose a reason for hiding this comment

Uh oh!

shane-melton Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

nick-livefront Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

shane-melton Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

nick-livefront Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

withinfocus Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

nick-livefront Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

withinfocus left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 4, 2025

Quality Gate passed

Uh oh!

Uh oh!

nick-livefront commented May 28, 2025 •

edited by jira bot

Loading

codecov bot commented May 28, 2025 •

edited

Loading

github-actions bot commented May 28, 2025 •

edited

Loading