Details

Reviewers

tomek
inka

Commits

rCOMM7c887f023e95: Utilities to idempotently create content and notif sessions

Summary

This differential modifies utility function that creates content session to create both content and notif sessions idempotently

Test Plan

Apply this stash: https://gist.github.com/marcinwasowicz/047b0f1b00079562b2547ebe3a8c18ad. This stash does some of the work https://linear.app/comm/project/send-encrypted-notification-from-the-client-to-the-tunnelbroker-03fc019ed3ee/issues is supposed to do. Message reducer is modified to return MessageData array and db-ops-handler greedily consumes MessageData array to create and send encrypted notifications through TB.
Send text message from a client (any platform).
Ensure that the first time you send message there are logs indicating that session are created with particular device ids (thread members device ids).
Ensure that each time you see in the console logged payloads of encrypted notifications. Examine payloads. Ensure that they match relevant flow types. No notification will be displayed on the receiving client since they can't decrypt the notif yet.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Jul 4 2024, 9:33 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJul 4 2024, 9:33 AM

marcin added a parent revision: D12672: Implement utility functions to check if we have content or notif session with device.Jul 4 2024, 9:34 AM

marcin edited the test plan for this revision. (Show Details)Jul 4 2024, 9:43 AM

marcin mentioned this in D12672: Implement utility functions to check if we have content or notif session with device.

marcin mentioned this in D12671: Implement notification encryption on web.

marcin mentioned this in D12670: Implement notification encryption on native.

marcin mentioned this in D12669: Implement peer notifications session creation as outbound on web and add notifs outbound session creator to OlmAPI.

marcin mentioned this in D12668: Implement peer notifications session creation as outbound on native.

Harbormaster completed remote builds in B30152: Diff 42043.Jul 4 2024, 9:54 AM

marcin requested review of this revision.Jul 4 2024, 9:54 AM

I am afraid that there is a vulnerability in this diff. Basically it is possible that two createOlmSessionWithPeer promises will execute concurrently. This might lead to corrupted state. In order to mitigate that I think it is best expose this method as a context. The context would internally keep a map like [(userID, deviceID)] : Promise mapping from particular deviceID and userID to currently executing promise (if any).

marcin added a child revision: D12676: Migrate notifications crypto account to MMKV on native.Jul 5 2024, 11:22 AM

marcin mentioned this in D12677: Implement method to query identity service from the NSE.Jul 5 2024, 11:29 AM

In D12673#358848, @marcin wrote:

I am afraid that there is a vulnerability in this diff. Basically it is possible that two createOlmSessionWithPeer promises will execute concurrently. This might lead to corrupted state. In order to mitigate that I think it is best expose this method as a context. The context would internally keep a map like [(userID, deviceID)] : Promise mapping from particular deviceID and userID to currently executing promise (if any).

Sounds like something that should be fixed.

lib/utils/crypto-utils.js
161–171 ↗	(On Diff #42043)	It is possible that `hasNotifsSession` is true here - should we create the notifs session regardless?

This revision now requires changes to proceed.Jul 10 2024, 5:01 AM

marcin added inline comments.Jul 12 2024, 2:31 AM

lib/utils/crypto-utils.js
161–171 ↗	(On Diff #42043)	Perhaps I am missing something but I think that if we reach these lines then we are in the case that there are neither notifs nor content session. If statement at liner 126 should guarantee this

tomek added inline comments.Jul 15 2024, 1:14 AM

lib/utils/crypto-utils.js
161–171 ↗	(On Diff #42043)	If `hasContentSession = false` and `hasNotifsSession = true`, then we won't enter the `if` at 126 line. We also won't enter the `if` at 152. So it seems possible to reach this code with the session present.

Fix race condition by exposing olm session creation via context

Remove TODO

Harbormaster completed remote builds in B30384: Diff 42347.Jul 16 2024, 7:27 AM

Harbormaster completed remote builds in B30385: Diff 42348.Jul 16 2024, 7:35 AM

This diff looks confusing to me. We're introducing a new context PeerOlmSessionCreatorContext but it doesn't look like we're using it.

lib/components/peer-olm-session-creator-provider.react.js
35–38 ↗	(On Diff #42348)	This looks fragile. We have a function that accepts two parameters but keep promises in an object indexed by only one of them. What if someone calls this method twice with the same `deviceID` and with different `userID`s? We can solve this issue in two ways: 1. we can index by two properties, or 2. we can remove `userID` parameter and get it from e.g. a selector. Not sure which is better, but in the second solution we should probably also clear the `runningPromises` ref. Or maybe, `userID` here means the ID of a user possessing a device with `deviceID`. If that's correct, we're fine with the current solution.

This revision is now accepted and ready to land.Jul 17 2024, 4:37 AM

Use the API in send-utils

Harbormaster completed remote builds in B30620: Diff 42715.Jul 23 2024, 11:21 AM

Address review

Harbormaster failed remote builds in B30634: Diff 42730!Jul 24 2024, 6:08 AM

Harbormaster completed remote builds in B30634: Diff 42730.Jul 24 2024, 9:14 AM

Use query to check notifications session for multiple devices at once and initilize crypto account in send utils instead of each time before encryption. it is huge performance improvement.
Fix context cycle

Harbormaster failed remote builds in B30675: Diff 42780!Jul 25 2024, 9:56 AM

Update phab

Harbormaster failed remote builds in B30794: Diff 42939!Jul 30 2024, 4:22 AM

In D12673#361660, @tomek wrote:

This diff looks confusing to me. We're introducing a new context PeerOlmSessionCreatorContext but it doesn't look like we're using it.

@marcin can you please respond to this? What is the point of this new PeerOlmSessionCreatorContext? Please explain things like this in diff descriptions, and please make sure you always document any offline discussions in response to diff comments on that diff.

In D12673#366331, @ashoat wrote:

In D12673#361660, @tomek wrote:

This diff looks confusing to me. We're introducing a new context PeerOlmSessionCreatorContext but it doesn't look like we're using it.

@marcin can you please respond to this? What is the point of this new PeerOlmSessionCreatorContext? Please explain things like this in diff descriptions, and please make sure you always document any offline discussions in response to diff comments on that diff.

@marcin can you please respond to this?

I responded by replacing usages of createOlmSessionWithPeer with context usages.

What is the point of this new PeerOlmSessionCreatorContext?

Everything was explained here and reviewers were aware of this.

Please explain things like this in diff descriptions, and please make sure you always document any offline discussions in response to diff comments on that diff.

There was no undocumented offline discussion regarding this diff.

Thanks @marcin! Can you link the diff where the context is used? Appreciate your explanation above!