From b98b74cef6135af29a3152def0960baa948745be Mon Sep 17 00:00:00 2001 From: Maciej Zieniuk <167752252+mzieniukbw@users.noreply.github.com> Date: Wed, 12 Feb 2025 17:31:03 +0100 Subject: [PATCH] [PM-10600] Push notification with full notification center content (#5086) * PM-10600: Notification push notification * PM-10600: Sending to specific client types for relay push notifications * PM-10600: Sending to specific client types for other clients * PM-10600: Send push notification on notification creation * PM-10600: Explicit group names * PM-10600: Id typos * PM-10600: Revert global push notifications * PM-10600: Added DeviceType claim * PM-10600: Sent to organization typo * PM-10600: UT coverage * PM-10600: Small refactor, UTs coverage * PM-10600: UTs coverage * PM-10600: Startup fix * PM-10600: Test fix * PM-10600: Required attribute, organization group for push notification fix * PM-10600: UT coverage * PM-10600: Fix Mobile devices not registering to organization push notifications We only register devices for organization push notifications when the organization is being created. This does not work, since we have a use case (Notification Center) of delivering notifications to all users of organization. This fixes it, by adding the organization id tag when device registers for push notifications. * PM-10600: Unit Test coverage for NotificationHubPushRegistrationService Fixed IFeatureService substitute mocking for Android tests. Added user part of organization test with organizationId tags expectation. * PM-10600: Unit Tests fix to NotificationHubPushRegistrationService after merge conflict * PM-10600: Organization push notifications not sending to mobile device from self-hosted. Self-hosted instance uses relay to register the mobile device against Bitwarden Cloud Api. Only the self-hosted server knows client's organization membership, which means it needs to pass in the organization id's information to the relay. Similarly, for Bitwarden Cloud, the organizaton id will come directly from the server. * PM-10600: Fix self-hosted organization notification not being received by mobile device. When mobile device registers on self-hosted through the relay, every single id, like user id, device id and now organization id needs to be prefixed with the installation id. This have been missing in the PushController that handles this for organization id. * PM-10600: Broken NotificationsController integration test Device type is now part of JWT access token, so the notification center results in the integration test are now scoped to client type web and all. * PM-10600: Merge conflicts fix * merge conflict fix * PM-10600: Push notification with full notification center content. Notification Center push notification now includes all the fields. --- src/Core/Models/PushNotification.cs | 12 ++++++-- .../Commands/CreateNotificationCommand.cs | 2 +- .../Entities/Notification.cs | 5 ++-- .../NotificationHubPushNotificationService.cs | 11 +++++-- .../AzureQueuePushNotificationService.cs | 11 +++++-- .../Push/Services/IPushNotificationService.cs | 2 +- .../MultiServicePushNotificationService.cs | 4 +-- .../Services/NoopPushNotificationService.cs | 2 +- ...NotificationsApiPushNotificationService.cs | 11 +++++-- .../Services/RelayPushNotificationService.cs | 11 +++++-- src/Notifications/HubHelpers.cs | 2 +- .../Commands/CreateNotificationCommandTest.cs | 2 +- ...ficationHubPushNotificationServiceTests.cs | 29 +++++++++++-------- .../AzureQueuePushNotificationServiceTests.cs | 22 +++++++++----- ...ultiServicePushNotificationServiceTests.cs | 6 ++-- 15 files changed, 85 insertions(+), 47 deletions(-) diff --git a/src/Core/Models/PushNotification.cs b/src/Core/Models/PushNotification.cs index fd27ced6c5..a6e8852e95 100644 --- a/src/Core/Models/PushNotification.cs +++ b/src/Core/Models/PushNotification.cs @@ -1,4 +1,5 @@ using Bit.Core.Enums; +using Bit.Core.NotificationCenter.Enums; namespace Bit.Core.Models; @@ -45,14 +46,21 @@ public class SyncSendPushNotification public DateTime RevisionDate { get; set; } } -public class SyncNotificationPushNotification +#nullable enable +public class NotificationPushNotification { public Guid Id { get; set; } + public Priority Priority { get; set; } + public bool Global { get; set; } + public ClientType ClientType { get; set; } public Guid? UserId { get; set; } public Guid? OrganizationId { get; set; } - public ClientType ClientType { get; set; } + public string? Title { get; set; } + public string? Body { get; set; } + public DateTime CreationDate { get; set; } public DateTime RevisionDate { get; set; } } +#nullable disable public class AuthRequestPushNotification { diff --git a/src/Core/NotificationCenter/Commands/CreateNotificationCommand.cs b/src/Core/NotificationCenter/Commands/CreateNotificationCommand.cs index f378a3688a..3fddafcdc7 100644 --- a/src/Core/NotificationCenter/Commands/CreateNotificationCommand.cs +++ b/src/Core/NotificationCenter/Commands/CreateNotificationCommand.cs @@ -37,7 +37,7 @@ public class CreateNotificationCommand : ICreateNotificationCommand var newNotification = await _notificationRepository.CreateAsync(notification); - await _pushNotificationService.PushSyncNotificationAsync(newNotification); + await _pushNotificationService.PushNotificationAsync(newNotification); return newNotification; } diff --git a/src/Core/NotificationCenter/Entities/Notification.cs b/src/Core/NotificationCenter/Entities/Notification.cs index aba456efbe..ad43299f55 100644 --- a/src/Core/NotificationCenter/Entities/Notification.cs +++ b/src/Core/NotificationCenter/Entities/Notification.cs @@ -15,9 +15,8 @@ public class Notification : ITableObject public ClientType ClientType { get; set; } public Guid? UserId { get; set; } public Guid? OrganizationId { get; set; } - [MaxLength(256)] - public string? Title { get; set; } - public string? Body { get; set; } + [MaxLength(256)] public string? Title { get; set; } + [MaxLength(3000)] public string? Body { get; set; } public DateTime CreationDate { get; set; } public DateTime RevisionDate { get; set; } public Guid? TaskId { get; set; } diff --git a/src/Core/NotificationHub/NotificationHubPushNotificationService.cs b/src/Core/NotificationHub/NotificationHubPushNotificationService.cs index ed44e69218..d1c7749d9f 100644 --- a/src/Core/NotificationHub/NotificationHubPushNotificationService.cs +++ b/src/Core/NotificationHub/NotificationHubPushNotificationService.cs @@ -181,14 +181,19 @@ public class NotificationHubPushNotificationService : IPushNotificationService await PushAuthRequestAsync(authRequest, PushType.AuthRequestResponse); } - public async Task PushSyncNotificationAsync(Notification notification) + public async Task PushNotificationAsync(Notification notification) { - var message = new SyncNotificationPushNotification + var message = new NotificationPushNotification { Id = notification.Id, + Priority = notification.Priority, + Global = notification.Global, + ClientType = notification.ClientType, UserId = notification.UserId, OrganizationId = notification.OrganizationId, - ClientType = notification.ClientType, + Title = notification.Title, + Body = notification.Body, + CreationDate = notification.CreationDate, RevisionDate = notification.RevisionDate }; diff --git a/src/Core/Platform/Push/Services/AzureQueuePushNotificationService.cs b/src/Core/Platform/Push/Services/AzureQueuePushNotificationService.cs index d3509c5437..a25a017192 100644 --- a/src/Core/Platform/Push/Services/AzureQueuePushNotificationService.cs +++ b/src/Core/Platform/Push/Services/AzureQueuePushNotificationService.cs @@ -165,14 +165,19 @@ public class AzureQueuePushNotificationService : IPushNotificationService await PushSendAsync(send, PushType.SyncSendDelete); } - public async Task PushSyncNotificationAsync(Notification notification) + public async Task PushNotificationAsync(Notification notification) { - var message = new SyncNotificationPushNotification + var message = new NotificationPushNotification { Id = notification.Id, + Priority = notification.Priority, + Global = notification.Global, + ClientType = notification.ClientType, UserId = notification.UserId, OrganizationId = notification.OrganizationId, - ClientType = notification.ClientType, + Title = notification.Title, + Body = notification.Body, + CreationDate = notification.CreationDate, RevisionDate = notification.RevisionDate }; diff --git a/src/Core/Platform/Push/Services/IPushNotificationService.cs b/src/Core/Platform/Push/Services/IPushNotificationService.cs index 5e1ab7067e..7f2b6c90fe 100644 --- a/src/Core/Platform/Push/Services/IPushNotificationService.cs +++ b/src/Core/Platform/Push/Services/IPushNotificationService.cs @@ -24,7 +24,7 @@ public interface IPushNotificationService Task PushSyncSendCreateAsync(Send send); Task PushSyncSendUpdateAsync(Send send); Task PushSyncSendDeleteAsync(Send send); - Task PushSyncNotificationAsync(Notification notification); + Task PushNotificationAsync(Notification notification); Task PushAuthRequestAsync(AuthRequest authRequest); Task PushAuthRequestResponseAsync(AuthRequest authRequest); Task PushSyncOrganizationStatusAsync(Organization organization); diff --git a/src/Core/Platform/Push/Services/MultiServicePushNotificationService.cs b/src/Core/Platform/Push/Services/MultiServicePushNotificationService.cs index 4ad81e223b..db3107b6c3 100644 --- a/src/Core/Platform/Push/Services/MultiServicePushNotificationService.cs +++ b/src/Core/Platform/Push/Services/MultiServicePushNotificationService.cs @@ -144,9 +144,9 @@ public class MultiServicePushNotificationService : IPushNotificationService return Task.CompletedTask; } - public Task PushSyncNotificationAsync(Notification notification) + public Task PushNotificationAsync(Notification notification) { - PushToServices((s) => s.PushSyncNotificationAsync(notification)); + PushToServices((s) => s.PushNotificationAsync(notification)); return Task.CompletedTask; } diff --git a/src/Core/Platform/Push/Services/NoopPushNotificationService.cs b/src/Core/Platform/Push/Services/NoopPushNotificationService.cs index 463a2fde88..fb4121179f 100644 --- a/src/Core/Platform/Push/Services/NoopPushNotificationService.cs +++ b/src/Core/Platform/Push/Services/NoopPushNotificationService.cs @@ -113,5 +113,5 @@ public class NoopPushNotificationService : IPushNotificationService return Task.FromResult(0); } - public Task PushSyncNotificationAsync(Notification notification) => Task.CompletedTask; + public Task PushNotificationAsync(Notification notification) => Task.CompletedTask; } diff --git a/src/Core/Platform/Push/Services/NotificationsApiPushNotificationService.cs b/src/Core/Platform/Push/Services/NotificationsApiPushNotificationService.cs index 5c6b46f63e..fb3814fd64 100644 --- a/src/Core/Platform/Push/Services/NotificationsApiPushNotificationService.cs +++ b/src/Core/Platform/Push/Services/NotificationsApiPushNotificationService.cs @@ -184,14 +184,19 @@ public class NotificationsApiPushNotificationService : BaseIdentityClientService await PushSendAsync(send, PushType.SyncSendDelete); } - public async Task PushSyncNotificationAsync(Notification notification) + public async Task PushNotificationAsync(Notification notification) { - var message = new SyncNotificationPushNotification + var message = new NotificationPushNotification { Id = notification.Id, + Priority = notification.Priority, + Global = notification.Global, + ClientType = notification.ClientType, UserId = notification.UserId, OrganizationId = notification.OrganizationId, - ClientType = notification.ClientType, + Title = notification.Title, + Body = notification.Body, + CreationDate = notification.CreationDate, RevisionDate = notification.RevisionDate }; diff --git a/src/Core/Platform/Push/Services/RelayPushNotificationService.cs b/src/Core/Platform/Push/Services/RelayPushNotificationService.cs index f51ab004a6..4d99f04768 100644 --- a/src/Core/Platform/Push/Services/RelayPushNotificationService.cs +++ b/src/Core/Platform/Push/Services/RelayPushNotificationService.cs @@ -191,14 +191,19 @@ public class RelayPushNotificationService : BaseIdentityClientService, IPushNoti await SendPayloadToUserAsync(authRequest.UserId, type, message, true); } - public async Task PushSyncNotificationAsync(Notification notification) + public async Task PushNotificationAsync(Notification notification) { - var message = new SyncNotificationPushNotification + var message = new NotificationPushNotification { Id = notification.Id, + Priority = notification.Priority, + Global = notification.Global, + ClientType = notification.ClientType, UserId = notification.UserId, OrganizationId = notification.OrganizationId, - ClientType = notification.ClientType, + Title = notification.Title, + Body = notification.Body, + CreationDate = notification.CreationDate, RevisionDate = notification.RevisionDate }; diff --git a/src/Notifications/HubHelpers.cs b/src/Notifications/HubHelpers.cs index 6d17ca9955..9b0164fdc5 100644 --- a/src/Notifications/HubHelpers.cs +++ b/src/Notifications/HubHelpers.cs @@ -105,7 +105,7 @@ public static class HubHelpers break; case PushType.SyncNotification: var syncNotification = - JsonSerializer.Deserialize>( + JsonSerializer.Deserialize>( notificationJson, _deserializerOptions); if (syncNotification.Payload.UserId.HasValue) { diff --git a/test/Core.Test/NotificationCenter/Commands/CreateNotificationCommandTest.cs b/test/Core.Test/NotificationCenter/Commands/CreateNotificationCommandTest.cs index a51feb6a73..41efce82ab 100644 --- a/test/Core.Test/NotificationCenter/Commands/CreateNotificationCommandTest.cs +++ b/test/Core.Test/NotificationCenter/Commands/CreateNotificationCommandTest.cs @@ -58,6 +58,6 @@ public class CreateNotificationCommandTest Assert.Equal(notification.CreationDate, notification.RevisionDate); await sutProvider.GetDependency() .Received(1) - .PushSyncNotificationAsync(newNotification); + .PushNotificationAsync(newNotification); } } diff --git a/test/Core.Test/NotificationHub/NotificationHubPushNotificationServiceTests.cs b/test/Core.Test/NotificationHub/NotificationHubPushNotificationServiceTests.cs index dc391b9801..f1cfdc9f85 100644 --- a/test/Core.Test/NotificationHub/NotificationHubPushNotificationServiceTests.cs +++ b/test/Core.Test/NotificationHub/NotificationHubPushNotificationServiceTests.cs @@ -20,10 +20,10 @@ public class NotificationHubPushNotificationServiceTests [Theory] [BitAutoData] [NotificationCustomize] - public async void PushSyncNotificationAsync_Global_NotSent( + public async void PushNotificationAsync_Global_NotSent( SutProvider sutProvider, Notification notification) { - await sutProvider.Sut.PushSyncNotificationAsync(notification); + await sutProvider.Sut.PushNotificationAsync(notification); await sutProvider.GetDependency() .Received(0) @@ -39,7 +39,7 @@ public class NotificationHubPushNotificationServiceTests [BitAutoData(false)] [BitAutoData(true)] [NotificationCustomize(false)] - public async void PushSyncNotificationAsync_UserIdProvidedClientTypeAll_SentToUser( + public async void PushNotificationAsync_UserIdProvidedClientTypeAll_SentToUser( bool organizationIdNull, SutProvider sutProvider, Notification notification) { @@ -51,7 +51,7 @@ public class NotificationHubPushNotificationServiceTests notification.ClientType = ClientType.All; var expectedSyncNotification = ToSyncNotificationPushNotification(notification); - await sutProvider.Sut.PushSyncNotificationAsync(notification); + await sutProvider.Sut.PushNotificationAsync(notification); await AssertSendTemplateNotificationAsync(sutProvider, PushType.SyncNotification, expectedSyncNotification, $"(template:payload_userId:{notification.UserId})"); @@ -70,7 +70,7 @@ public class NotificationHubPushNotificationServiceTests [BitAutoData(true, ClientType.Web)] [BitAutoData(true, ClientType.Mobile)] [NotificationCustomize(false)] - public async void PushSyncNotificationAsync_UserIdProvidedClientTypeNotAll_SentToUser(bool organizationIdNull, + public async void PushNotificationAsync_UserIdProvidedClientTypeNotAll_SentToUser(bool organizationIdNull, ClientType clientType, SutProvider sutProvider, Notification notification) { @@ -82,7 +82,7 @@ public class NotificationHubPushNotificationServiceTests notification.ClientType = clientType; var expectedSyncNotification = ToSyncNotificationPushNotification(notification); - await sutProvider.Sut.PushSyncNotificationAsync(notification); + await sutProvider.Sut.PushNotificationAsync(notification); await AssertSendTemplateNotificationAsync(sutProvider, PushType.SyncNotification, expectedSyncNotification, $"(template:payload_userId:{notification.UserId} && clientType:{clientType})"); @@ -94,14 +94,14 @@ public class NotificationHubPushNotificationServiceTests [Theory] [BitAutoData] [NotificationCustomize(false)] - public async void PushSyncNotificationAsync_UserIdNullOrganizationIdProvidedClientTypeAll_SentToOrganization( + public async void PushNotificationAsync_UserIdNullOrganizationIdProvidedClientTypeAll_SentToOrganization( SutProvider sutProvider, Notification notification) { notification.UserId = null; notification.ClientType = ClientType.All; var expectedSyncNotification = ToSyncNotificationPushNotification(notification); - await sutProvider.Sut.PushSyncNotificationAsync(notification); + await sutProvider.Sut.PushNotificationAsync(notification); await AssertSendTemplateNotificationAsync(sutProvider, PushType.SyncNotification, expectedSyncNotification, $"(template:payload && organizationId:{notification.OrganizationId})"); @@ -116,7 +116,7 @@ public class NotificationHubPushNotificationServiceTests [BitAutoData(ClientType.Web)] [BitAutoData(ClientType.Mobile)] [NotificationCustomize(false)] - public async void PushSyncNotificationAsync_UserIdNullOrganizationIdProvidedClientTypeNotAll_SentToOrganization( + public async void PushNotificationAsync_UserIdNullOrganizationIdProvidedClientTypeNotAll_SentToOrganization( ClientType clientType, SutProvider sutProvider, Notification notification) { @@ -125,7 +125,7 @@ public class NotificationHubPushNotificationServiceTests var expectedSyncNotification = ToSyncNotificationPushNotification(notification); - await sutProvider.Sut.PushSyncNotificationAsync(notification); + await sutProvider.Sut.PushNotificationAsync(notification); await AssertSendTemplateNotificationAsync(sutProvider, PushType.SyncNotification, expectedSyncNotification, $"(template:payload && organizationId:{notification.OrganizationId} && clientType:{clientType})"); @@ -206,13 +206,18 @@ public class NotificationHubPushNotificationServiceTests .UpsertAsync(Arg.Any()); } - private static SyncNotificationPushNotification ToSyncNotificationPushNotification(Notification notification) => + private static NotificationPushNotification ToSyncNotificationPushNotification(Notification notification) => new() { Id = notification.Id, + Priority = notification.Priority, + Global = notification.Global, + ClientType = notification.ClientType, UserId = notification.UserId, OrganizationId = notification.OrganizationId, - ClientType = notification.ClientType, + Title = notification.Title, + Body = notification.Body, + CreationDate = notification.CreationDate, RevisionDate = notification.RevisionDate }; diff --git a/test/Core.Test/Platform/Push/Services/AzureQueuePushNotificationServiceTests.cs b/test/Core.Test/Platform/Push/Services/AzureQueuePushNotificationServiceTests.cs index 7aa053ec6d..a84a76152a 100644 --- a/test/Core.Test/Platform/Push/Services/AzureQueuePushNotificationServiceTests.cs +++ b/test/Core.Test/Platform/Push/Services/AzureQueuePushNotificationServiceTests.cs @@ -24,7 +24,7 @@ public class AzureQueuePushNotificationServiceTests [BitAutoData] [NotificationCustomize] [CurrentContextCustomize] - public async void PushSyncNotificationAsync_Notification_Sent( + public async void PushNotificationAsync_Notification_Sent( SutProvider sutProvider, Notification notification, Guid deviceIdentifier, ICurrentContext currentContext) { @@ -32,7 +32,7 @@ public class AzureQueuePushNotificationServiceTests sutProvider.GetDependency().HttpContext!.RequestServices .GetService(Arg.Any()).Returns(currentContext); - await sutProvider.Sut.PushSyncNotificationAsync(notification); + await sutProvider.Sut.PushNotificationAsync(notification); await sutProvider.GetDependency().Received(1) .SendMessageAsync(Arg.Is(message => @@ -43,23 +43,29 @@ public class AzureQueuePushNotificationServiceTests private static bool MatchMessage(PushType pushType, string message, IEquatable expectedPayloadEquatable, string contextId) { - var pushNotificationData = - JsonSerializer.Deserialize>(message); + var pushNotificationData = JsonSerializer.Deserialize>(message); return pushNotificationData != null && pushNotificationData.Type == pushType && expectedPayloadEquatable.Equals(pushNotificationData.Payload) && pushNotificationData.ContextId == contextId; } - private class SyncNotificationEquals(Notification notification) : IEquatable + private class SyncNotificationEquals(Notification notification) : IEquatable { - public bool Equals(SyncNotificationPushNotification? other) + public bool Equals(NotificationPushNotification? other) { return other != null && other.Id == notification.Id && - other.UserId == notification.UserId && - other.OrganizationId == notification.OrganizationId && + other.Priority == notification.Priority && + other.Global == notification.Global && other.ClientType == notification.ClientType && + other.UserId.HasValue == notification.UserId.HasValue && + other.UserId == notification.UserId && + other.OrganizationId.HasValue == notification.OrganizationId.HasValue && + other.OrganizationId == notification.OrganizationId && + other.Title == notification.Title && + other.Body == notification.Body && + other.CreationDate == notification.CreationDate && other.RevisionDate == notification.RevisionDate; } } diff --git a/test/Core.Test/Platform/Push/Services/MultiServicePushNotificationServiceTests.cs b/test/Core.Test/Platform/Push/Services/MultiServicePushNotificationServiceTests.cs index 35997f80e9..edbd297708 100644 --- a/test/Core.Test/Platform/Push/Services/MultiServicePushNotificationServiceTests.cs +++ b/test/Core.Test/Platform/Push/Services/MultiServicePushNotificationServiceTests.cs @@ -15,15 +15,15 @@ public class MultiServicePushNotificationServiceTests [Theory] [BitAutoData] [NotificationCustomize] - public async Task PushSyncNotificationAsync_Notification_Sent( + public async Task PushNotificationAsync_Notification_Sent( SutProvider sutProvider, Notification notification) { - await sutProvider.Sut.PushSyncNotificationAsync(notification); + await sutProvider.Sut.PushNotificationAsync(notification); await sutProvider.GetDependency>() .First() .Received(1) - .PushSyncNotificationAsync(notification); + .PushNotificationAsync(notification); } [Theory]