From c3181d581e7b143cebc03a16103bcfcac3536e50 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Wed, 3 Sep 2025 15:41:11 -0400 Subject: [PATCH] Apply suggestions from code review Co-authored-by: Todd Martin <106564991+trmartin4@users.noreply.github.com> --- src/Core/Platform/Push/README.md | 25 +++++++++++--------- src/Core/Platform/PushRegistration/README.md | 18 +++++++------- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/Core/Platform/Push/README.md b/src/Core/Platform/Push/README.md index eaaf37db59..68ed7f0598 100644 --- a/src/Core/Platform/Push/README.md +++ b/src/Core/Platform/Push/README.md @@ -29,7 +29,7 @@ await pushNotificationService.PushAsync(new PushNotification ## Extending If you want to extend this framework for sending your own notification type you do so by adding a -new enum member the [`PushType`](./PushType.cs) enum. Assign a number to it that is 1 above the next +new enum member to the [`PushType`](./PushType.cs) enum. Assign a number to it that is 1 above the next highest value. You must then annotate that enum member with a [`[NotificationInfo]`](./NotificationInfoAttribute.cs) attribute to inform others who the owning team and expected payload type are. Then you may inject @@ -40,30 +40,33 @@ You should NOT add tests for your specific notification type in any of the `IPus implementations. They do currently have tests for many of the notification types but those will eventually be deleted and no new ones need to be added. -Since notifications are relayed through our cloud instance for self hosted users (whom opt-in) it's +Since notifications are relayed through our cloud instance for self hosted users (if they opt in) it's important to try and keep the information in the notification payload minimal. It's generally best -to send a notification with various ID's that mean nothing to our cloud but can then be used to get +to send a notification with IDs for any entities involved, which mean nothing to our cloud but can then be used to get more detailed information once the notification is received on the device. ## Implementations -The implementation of push notifications scatters push notification requests to all `IPushEngine`'s +The implementation of push notifications scatters push notification requests to all `IPushEngine`s that have been registered in DI for the current application. In release builds, this service does -NOT await the underlying engines to make sure that the notification has gotten to its destination +NOT await the underlying engines to make sure that the notification has arrived at its destination before its returned task completes. ### Azure Notification Hub Used when the application is hosted by Bitwarden in the cloud. This sends the notification to the -configured Azure Notification Hub which we currently rely on it for sending the notification to our +configured Azure Notification Hub, which we currently rely on for sending notifications to: +- Our mobile clients, through the Notification Hub federation with mobile app notification systems, and +- Our clients configured to use Web Push (currently the Chrome Extension). + mobile clients and any clients configured to use Web Push (currently Chrome Extension). This implementation is always assumed to have available configuration when running in the cloud. ### Azure Queue -Used when the application is hosted by Bitwarden in the cloud. This sends notifications to a Azure -Queue, that queue is then consumed in our Notifications service and the notifications are then sent +Used when the application is hosted by Bitwarden in the cloud, to send the notification over web sockets (SignalR). This sends the notification to a Azure +Queue. That queue is then consumed in our Notifications service, where the notification is sent to a SignalR hub so that our clients connected through a persistent web socket to our notifications service get the notification. @@ -74,7 +77,7 @@ to a value. Used when the application is being self-hosted. This relays a notification from the self-hosted instance to a cloud instance. The notification is recieved by our cloud and then relayed to -Azure Notification Hub. This is because self-hosted instance aren't able to directly send +Azure Notification Hub. This is necessary because self-hosted instance aren't able to directly send notifications to mobile devices. This instance is registered in DI when `GlobalSettings:PushRelayBaseUri` and @@ -83,9 +86,9 @@ This instance is registered in DI when `GlobalSettings:PushRelayBaseUri` and ### Notifications API Used when the application is being self-hosted. This sends a API request to the self-hosted instance -of the Notifications service. The notifications service receives the request and then sends the +of the Notifications service. The Notifications service receives the request and then sends the notification through the SignalR hub. This is very similar to cloud using an Azure Queue but it -doesn't require the self-hosted infrastructure to run their own queuing infrastructure. +doesn't require the self-hosted customer to run their own queuing infrastructure. This instance is registered when `GlobalSettings:InternalIdentityKey` and `GlobalSettings:BaseServiceUri:InternalNotifications` are set. Both of these settings are usually diff --git a/src/Core/Platform/PushRegistration/README.md b/src/Core/Platform/PushRegistration/README.md index e7f4d162e3..407f37a6c2 100644 --- a/src/Core/Platform/PushRegistration/README.md +++ b/src/Core/Platform/PushRegistration/README.md @@ -20,23 +20,23 @@ so that we can keep push registration working correctly. ### Azure Notification Hub Used when the application is hosted by Bitwarden in the cloud. This registers the device and -associated metadata with Azure Notification Hub. This is necessary so that when a notification +associated metadata with Azure Notification Hub (ANH). This is necessary so that when a notification is sent ANH will be able to get the notification to that device. -Since Azure Notification Hubs has a limit on the amount of devices per hub we have begun to shard +Since Azure Notification Hub has a limit on the amount of devices per hub we have begun to shard devices across multiple hubs. Multiple hubs can be configured through configuration and each one can have a `RegistrationStartDate` and `RegistrationEndDate`. If the start date is `null` no devices will be registered against that given hub. A `null` end date is treated as no known expiry. The -creation of a device is pulled out of the devices ID and that date is used to find a hub that spans +creation date for a device is retrieved by the device's ID, and that date is used to find a hub that spans during it's creation date. -When we register a device with Azure Notificaiton Hub we include tags, which are data that can later +When we register a device with Azure Notification Hub we include tags, which are data that can later be used to specifically target that device with a notification. We send the ID of the user this -device belongs to, the type of the client (Web, Desktop, Mobile, etc), all the organization id's of -organizations the user is a confirmed member of, the ID of the self-hosted installation if this -device was relayed to us, and the device identifier which is a random guid generated on the device. -Most of this data is considered immutable after the creation of a device except for the -organization memberships of a user. If a user is added/removed from an organization it's important +device belongs to, the type of the client (Web, Desktop, Mobile, etc), all the organization IDs of +organizations of which the user is a confirmed member, the ID of the self-hosted installation if this +device was relayed to us, and the device identifier, which is a random GUID generated on the device. +Most of this data is considered immutable after the creation of a device, except for the +organization memberships of a user. If a user is added/removed from an organization, it is important that `CreateOrUpdateRegistrationAsync` is called with the new memberships. ### Relay