[PM-36158] - Fix org cipher push notifications to fan out per authorized user#7522
[PM-36158] - Fix org cipher push notifications to fan out per authorized user#7522jaasen-livefront wants to merge 10 commits intomainfrom
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the move of org-cipher push fan-out into Code Review DetailsNo new findings. The four prior review threads (silent drop on null Minor observations (not blocking):
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7522 +/- ##
===========================================
- Coverage 59.13% 14.39% -44.75%
===========================================
Files 2077 1280 -797
Lines 91848 55613 -36235
Branches 8175 4314 -3861
===========================================
- Hits 54315 8004 -46311
- Misses 35601 47469 +11868
+ Partials 1932 140 -1792 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
|
| INNER JOIN [dbo].[OrganizationUser] OU ON OU.[Id] = GU.[OrganizationUserId] | ||
| WHERE CG.[CollectionId] IN (SELECT [Id] FROM @CollectionIds) | ||
| AND OU.[Status] = 2 -- Confirmed | ||
| END |
There was a problem hiding this comment.
This query covers direct CollectionUser and group-based access, but it misses users who have access to all org ciphers implicitly, (OrganizationUser.Type of Owner or Admin) and users with AccessAll = true
-- Users with org-level access (admins, owners, AccessAll)
UNION
SELECT DISTINCT OU.[UserId]
FROM [dbo].[OrganizationUser] OU
INNER JOIN [dbo].[CollectionCipher] CC ON CC.[CollectionId] IN (SELECT [Id] FROM @CollectionIds)
INNER JOIN [dbo].[Collection] COL ON COL.[Id] = CC.[CollectionId]
WHERE OU.[OrganizationId] = COL.[OrganizationId]
AND OU.[Status] = 2
AND (OU.[AccessAll] = 1 OR OU.[Type] IN (0, 1))There was a problem hiding this comment.
The same change would be needed on the EF side of things in src/Infrastructure.EntityFramework/Repositories/CollectionCipherRepository.cs
There was a problem hiding this comment.
@nick-livefront Good point. AccessAll was removed but we should definitely automatically send this to admins and owners.
There was a problem hiding this comment.
Nice catch on AccessAll. Your current fix will allow any admin or owner to receive notifications, it still should be restricted to if they have access. AllowAdminAccessToAllCollectionItems should be checked to see if they have that setting enabled for any owner to admin to access all items.
There was a problem hiding this comment.
@nick-livefront Ah yes ofc. Fixed. e7d235d
There was a problem hiding this comment.
The EF implementation needs the fix as well 😄
| return storageBytesRemaining; | ||
| } | ||
|
|
||
| private async Task<ICollection<Guid>> GetCollectionIdsForPushAsync(Cipher cipher) |
There was a problem hiding this comment.
🎨 This should be nullable, given it can return null
| private async Task<ICollection<Guid>> GetCollectionIdsForPushAsync(Cipher cipher) | |
| private async Task<ICollection<Guid?>> GetCollectionIdsForPushAsync(Cipher cipher) |
There was a problem hiding this comment.
@nick-livefront Good suggestion. The file is #nullable disable but this is a good opportunity to clean that up. I've done so along with ICipherService.
| // Org cipher fan-out is handled upstream in MultiServicePushNotificationService, | ||
| // which resolves per-user access and calls PushAsync directly. | ||
| if (cipher.UserId.HasValue) |
There was a problem hiding this comment.
If this is true, then PushCipherAsync can be deleted from IPushEngine. It only exists because of the different behavior of the engines but since this PR is making it so there isn't a difference it shouldn't need to exist anymore.
There was a problem hiding this comment.
This is not the place to add this kind of logic. You should likely create your own service that has methods like PushSyncCipherCreateAsync, PushSyncCipherUpdateAsync, PushSyncCipherDeleteAsync and those do all the logic you have here then it can call IPushNotificationService.PushAsync then PushCipherAsync and associated tests can be deleted.
This new service is also the place where this needs to be feature flagged. This has the possibility of new load to the database when notifications are being sent, new amount of notifications being sent and new load to the server because clients are now reacting to notifications they did not get before by doing API calls.
There was a problem hiding this comment.
Great idea. Added a service as well as feature flag on/off handling.
2f0aab4
3 participants
🎟️ Tracking
PM-35168
📔 Objective
This PR refactors org cipher push notification delivery to be per-user and fixes several related regressions.
Previously, org cipher push notifications were either silently dropped (NotificationHub / Relay) or broadcast to an org-level SignalR group (Notifications API / AzureQueue) without respecting per-user collection access. This PR moves org cipher fan-out into
MultiServicePushNotificationService, which resolves the exact set of users who can access the cipher and delivers a targetedUser-scoped push to each one.Changes
Org cipher push fan-out moved to
MultiServicePushNotificationServicePushCipherAsyncis called for an org cipher, the service:collectionIdsor falling back toGetCollectionIdsByCipherIdAsyncif none were supplied).GetUserIdsByCollectionIdsAsyncto find all confirmed org members with access to those collections (via direct assignment or group membership).User-targetedPushAsynccall per user, concurrently viaTask.WhenAll.Org cipher handling removed from individual push engines
AzureQueuePushEngine,NotificationsApiPushEngine,RelayPushEngine, andNotificationHubPushEngineno longer handle org ciphers inPushCipherAsync; that responsibility now lives exclusively inMultiServicePushNotificationService.HubHelpersno longer broadcasts cipher notifications to an org-level SignalR group; per-user fan-out upstream means the engine only seesUserId-targeted messages.CipherServicefixesSaveAsyncandSaveDetailsAsyncnow correctly forwardcollectionIdstoPushSyncCipherCreateAsync(previously always passednull).DeleteAsyncnow callsPushCipherAsync(cipher, PushType.SyncLoginDelete, collectionIds)directly, replacing the oldPushSyncCipherDeleteAsync(cipher)call that did not carry collection context.GetCollectionIdsForPushAsyncretrieves collection IDs for org ciphers before push, returningnullfor personal ciphers.New repository surface
ICollectionCipherRepository.GetCollectionIdsByCipherIdAsync(Guid cipherId)— returns the collection IDs a specific cipher belongs to (targeted single-cipher lookup, avoids the O(org) query).ICollectionCipherRepository.GetUserIdsByCollectionIdsAsync(IEnumerable<Guid> collectionIds)— returns distinct confirmed user IDs with direct or group-based access to the given collections.ICollectionCipherRepositoryis now registered as a noop singleton inAddPushto satisfy DI in push-only service compositions.Database
CollectionCipher_ReadCollectionIdsByCipherIdCollectionCipher_ReadUserIdsByCollectionIdsutil/Migrator/DbScripts/.Tests
AzureQueuePushEngineTests: Replaced parameterized personal/org test cases with separate_PersonalCipher_SendsExpectedResponseand_OrgCipher_DoesNotSendMessagevariants to clearly document the new engine contract.MultiServicePushNotificationServiceTests: Added tests for personal cipher delegation, org cipher per-user fan-out, and the null-collection-ID fallback resolution path.CipherServiceTests: Updated fiveDeleteAsync_*assertions fromPushSyncCipherDeleteAsynctoPushCipherAsyncto match the production call shape.Behavior With Feature Flag OFF (
OrgCipherPushFanout = false)NotificationTarget.Organization) only to non-mobile engines (SignalR/Azure Queue/NotificationsApi).Behavior With Feature Flag ON (
OrgCipherPushFanout = true)NotificationTarget.User) to all engines (web, desktop, and mobile).📸 Screenshots
N/A — no UI changes.