organization status changed code changes #5113

cyprain-okeke · 2024-12-04T20:28:43Z

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

👍 (:+1:) or similar for great changes
📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
❓ (:question:) for questions
🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
🎨 (:art:) for suggestions / improvements
❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
⛏ (:pick:) for minor or nitpick changes

Signed-off-by: Cy Okeke <[email protected]>

codecov · 2024-12-04T20:39:34Z

Codecov Report

Attention: Patch coverage is 0% with 58 lines in your changes missing coverage. Please review.

Project coverage is 43.35%. Comparing base (21fcfcd) to head (819ef74).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ationHub/NotificationHubPushNotificationService.cs	0.00%	8 Missing ⚠️
...plementations/AzureQueuePushNotificationService.cs	0.00%	8 Missing ⚠️
...tations/NotificationsApiPushNotificationService.cs	0.00%	8 Missing ⚠️
...es/Implementations/RelayPushNotificationService.cs	0.00%	8 Missing ⚠️
...ices/Implementations/SubscriptionUpdatedHandler.cs	0.00%	7 Missing ⚠️
src/Notifications/HubHelpers.cs	0.00%	6 Missing ⚠️
...ervices/Implementations/PaymentSucceededHandler.cs	0.00%	4 Missing ⚠️
...ementations/MultiServicePushNotificationService.cs	0.00%	4 Missing ⚠️
...NoopImplementations/NoopPushNotificationService.cs	0.00%	3 Missing ⚠️
src/Core/Models/PushNotification.cs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5113      +/-   ##
==========================================
- Coverage   43.39%   43.35%   -0.04%     
==========================================
  Files        1456     1456              
  Lines       66412    66468      +56     
  Branches     6078     6078              
==========================================
  Hits        28817    28817              
- Misses      36304    36360      +56     
  Partials     1291     1291

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

github-actions · 2024-12-04T21:04:05Z

Checkmarx One – Scan Summary & Details – d9ffe0e0-3d7f-47a7-8021-80f7a1ebfac0

New Issues

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Billing/Controllers/PayPalController.cs: 66	Attack Vector
Privacy_Violation	/src/Core/NotificationHub/NotificationHubPushNotificationService.cs: 195	Attack Vector
Privacy_Violation	/src/Api/Vault/Models/Request/CipherRequestModel.cs: 169	Attack Vector
Privacy_Violation	/src/Api/Vault/Models/Request/CipherRequestModel.cs: 173	Attack Vector
Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 213	Attack Vector
Privacy_Violation	/src/Api/Vault/Models/Request/CipherRequestModel.cs: 202	Attack Vector
Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 220	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AuthRequestsController.cs: 75	Attack Vector
Log_Forging	/src/Api/Auth/Controllers/AuthRequestsController.cs: 87	Attack Vector
Missing_CSP_Header	/src/Core/MailTemplates/Handlebars/AdminConsole/DomainClaimedByOrganization.html.hbs: 5	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 70
	CSRF	/src/Api/Controllers/LicensesController.cs: 44
	CSRF	/src/Api/Billing/Controllers/OrganizationsController.cs: 52
	CSRF	/src/Billing/Controllers/RecoveryController.cs: 38
	CSRF	/src/Billing/Controllers/StripeController.cs: 164
	CSRF	/src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 372
	CSRF	/src/Api/AdminConsole/Controllers/PoliciesController.cs: 91
	CSRF	/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 46
	CSRF	/src/Api/AdminConsole/Public/Controllers/PoliciesController.cs: 65
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 470
	Privacy_Violation	/src/Core/Auth/Services/Implementations/AuthRequestService.cs: 208
	Log_Forging	/src/Billing/Controllers/RecoveryController.cs: 38
	Log_Forging	/src/Billing/Controllers/RecoveryController.cs: 38
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164

Signed-off-by: Cy Okeke <[email protected]>

…spended-org-status-updated-to-Active

Signed-off-by: Cy Okeke <[email protected]>

mzieniukbw · 2024-12-12T10:21:27Z

src/Billing/Services/Implementations/PaymentSucceededHandler.cs

+                    Enabled = true
+                };
+
+                await _pushNotificationService.SendPayloadToUserAsync(payload.UserId.ToString(),


I don't know why exactly we expose those methods, but non-selfhosted SignalR does not implement it https://github.com/bitwarden/server/blob/main/src/Core/Services/Implementations/AzureQueuePushNotificationService.cs#L211
In most cases we implement the method for specific notification type in IPushNotificationService and all it's implementations.

…spended-org-status-updated-to-Active

mzieniukbw · 2024-12-12T12:48:57Z

src/Core/NotificationHub/NotificationHubPushNotificationService.cs

+            Enabled = organization.Enabled
+        };
+
+        await SendPayloadToUserAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, true);


We need to use SendPayloadToOrganizationAsync when sending to organization

Last argument needs to be false, since we don't want to exclude current device.

Suggested change

await SendPayloadToUserAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, true);

await SendPayloadToOrganizationAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, false);

mzieniukbw · 2024-12-12T13:13:48Z

src/Core/Services/Implementations/AzureQueuePushNotificationService.cs

+        await PushOrganizationStatusAsync(organization, PushType.SyncOrganizationStatusChanged);
+    }
+
+    private async Task PushOrganizationStatusAsync(Organization organization, PushType type)
+    {


We don't need the private method. It does not have any benefit.

Suggested change

await PushOrganizationStatusAsync(organization, PushType.SyncOrganizationStatusChanged);

}

private async Task PushOrganizationStatusAsync(Organization organization, PushType type)

{

mzieniukbw · 2024-12-12T13:14:48Z

src/Core/Services/Implementations/AzureQueuePushNotificationService.cs

+        if (organization is not null)
+        {


do we need this check ? I don't think we should expect this to be null.

mzieniukbw · 2024-12-12T13:15:38Z

src/Core/Services/Implementations/AzureQueuePushNotificationService.cs

+                Enabled = organization.Enabled
+            };
+
+            await SendMessageAsync(type, message, true);


Last argument needs to be false, since we don't want to exclude current device.

Suggested change

await SendMessageAsync(type, message, true);

await SendMessageAsync(type, message, false);

mzieniukbw · 2024-12-12T13:17:04Z

src/Core/Services/Implementations/NotificationsApiPushNotificationService.cs

+        await PushOrganizationStatusAsync(organization, PushType.SyncOrganizationStatusChanged);
+    }
+
+    private async Task PushOrganizationStatusAsync(Organization organization, PushType type)
+    {


We don't need the private method. It does not have any benefit.

Suggested change

await PushOrganizationStatusAsync(organization, PushType.SyncOrganizationStatusChanged);

}

private async Task PushOrganizationStatusAsync(Organization organization, PushType type)

{

mzieniukbw · 2024-12-12T13:17:13Z

src/Core/Services/Implementations/NotificationsApiPushNotificationService.cs

+
+    private async Task PushOrganizationStatusAsync(Organization organization, PushType type)
+    {
+        if (organization is not null)


do we need this check ? I don't think we should expect this to be null.

mzieniukbw · 2024-12-12T13:17:28Z

src/Core/Services/Implementations/NotificationsApiPushNotificationService.cs

+                Enabled = organization.Enabled
+            };
+
+            await SendMessageAsync(type, message, true);


Last argument needs to be false, since we don't want to exclude current device.

Suggested change

await SendMessageAsync(type, message, true);

await SendMessageAsync(type, message, false);

mzieniukbw · 2024-12-12T13:17:50Z

src/Core/Services/Implementations/RelayPushNotificationService.cs

+            Enabled = organization.Enabled
+        };
+
+        await SendPayloadToOrganizationAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, true);


Last argument needs to be false, since we don't want to exclude current device.

Suggested change

await SendPayloadToOrganizationAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, true);

await SendPayloadToOrganizationAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, false);

mzieniukbw

LGTM

…spended-org-status-updated-to-Active

organization status changed code changes

ee5b042

Signed-off-by: Cy Okeke <[email protected]>

cyprain-okeke requested a review from a team as a code owner December 4, 2024 20:28

jonashendrickx previously approved these changes Dec 6, 2024

View reviewed changes

Add the push notification to subscriptionUpdated

82d650a

Signed-off-by: Cy Okeke <[email protected]>

cyprain-okeke dismissed jonashendrickx’s stale review via 82d650a December 9, 2024 08:20

cyprain-okeke requested a review from jonashendrickx December 9, 2024 08:21

cyprain-okeke and others added 3 commits December 9, 2024 09:21

Merge branch 'main' into pm-14302-Suspended-icon-remains-after-the-su…

c6286b7

…spended-org-status-updated-to-Active

send notification using the SendPayloadToUser

635ffc1

Signed-off-by: Cy Okeke <[email protected]>

Change the implementation to send userId

6871f4b

mzieniukbw requested changes Dec 12, 2024

View reviewed changes

Added new implementation for orgstatus sync

61d6e36

cyprain-okeke requested a review from mzieniukbw December 12, 2024 12:34

Merge branch 'main' into pm-14302-Suspended-icon-remains-after-the-su…

f33f93d

…spended-org-status-updated-to-Active

mzieniukbw requested changes Dec 12, 2024

View reviewed changes

refactor the code and remove private methods

3539f7d

cyprain-okeke requested a review from mzieniukbw December 12, 2024 14:03

mzieniukbw approved these changes Dec 12, 2024

View reviewed changes

cturnbull-bitwarden approved these changes Dec 12, 2024

View reviewed changes

bitwarden-devops-bot temporarily deployed to EU-QA Cloud December 16, 2024 13:36 Inactive

Merge branch 'main' into pm-14302-Suspended-icon-remains-after-the-su…

4151ffa

…spended-org-status-updated-to-Active

bitwarden-devops-bot temporarily deployed to EU-QA Cloud December 16, 2024 16:26 Inactive

jonashendrickx approved these changes Dec 17, 2024

View reviewed changes

bitwarden-devops-bot temporarily deployed to US-QA Cloud December 18, 2024 12:48 Inactive

Merge branch 'main' into pm-14302-Suspended-icon-remains-after-the-su…

819ef74

…spended-org-status-updated-to-Active

cyprain-okeke merged commit 322a074 into main Dec 18, 2024
52 checks passed

cyprain-okeke deleted the pm-14302-Suspended-icon-remains-after-the-suspended-org-status-updated-to-Active branch December 18, 2024 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

organization status changed code changes #5113

organization status changed code changes #5113

cyprain-okeke commented Dec 4, 2024

codecov bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

mzieniukbw Dec 12, 2024 •

edited

Loading

mzieniukbw Dec 12, 2024

mzieniukbw Dec 12, 2024

mzieniukbw Dec 12, 2024

mzieniukbw Dec 12, 2024

mzieniukbw Dec 12, 2024

mzieniukbw Dec 12, 2024

mzieniukbw Dec 12, 2024

mzieniukbw Dec 12, 2024

mzieniukbw left a comment

	await SendPayloadToUserAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, true);
	await SendPayloadToOrganizationAsync(organization.Id, PushType.SyncOrganizationStatusChanged, message, false);

	await SendMessageAsync(type, message, true);
	await SendMessageAsync(type, message, false);

organization status changed code changes #5113

organization status changed code changes #5113

Conversation

cyprain-okeke commented Dec 4, 2024

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

github-actions bot commented Dec 4, 2024 • edited Loading

New Issues

Fixed Issues

mzieniukbw Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzieniukbw left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2024 •

edited

Loading

github-actions bot commented Dec 4, 2024 •

edited

Loading

mzieniukbw Dec 12, 2024 •

edited

Loading