Page MenuHomePhabricator

Implement olm session updater that has encrypting functionality
ClosedPublic

Authored by marcin on May 11 2023, 7:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 27, 11:02 PM
Unknown Object (File)
Fri, Oct 25, 8:04 PM
Unknown Object (File)
Thu, Oct 24, 9:18 AM
Unknown Object (File)
Fri, Oct 18, 12:16 AM
Unknown Object (File)
Fri, Oct 18, 12:16 AM
Unknown Object (File)
Fri, Oct 18, 12:16 AM
Unknown Object (File)
Fri, Oct 18, 12:16 AM
Unknown Object (File)
Fri, Oct 18, 12:16 AM
Subscribers

Details

Summary

This differential implements updater that can use olm session to encrypt data.

Test Plan
  1. Apply those changes to NotificationService.mm: https://gist.github.com/marcinwasowicz/0a43950fb202f03b3c5b92fe17dd5272
  2. Build the app
  3. Log in
  4. Pull from the db cookie id of the currently logged in user.
  5. Apply those changes to the send.js in the keyserver: https://gist.github.com/marcinwasowicz/9bc13a92310a9b91a59d73a2557b62a9
  6. Add your cookie id.
  7. Send the notif.
  8. Ensure it is displayed and examine the logs

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat added inline comments.
keyserver/src/updaters/olm-session-updater.js
12 ↗(On Diff #26405)

Why is this different from olmAccountUpdateRetryDelay?

27 ↗(On Diff #26405)

Can we please put FROM on its own line?

This revision is now accepted and ready to land.May 12 2023, 9:25 AM
marcin added inline comments.
keyserver/src/updaters/olm-session-updater.js
11 ↗(On Diff #26405)

In this particular case I think it is not wise to introduce such a strict constraint on a retry number.
In the case of olm account we have only one account that can be concurrently updated per entire keyserver. Additionally updates are not expected to occur frequently. In the case of olm sessions however we expect frequent concurrent updates. Imagine a case of 10 people group conversation. If 9 of them send a message at the same time we might end up with up to 9 concurrent updates to the same sessions. Having const maxOlmSessionUpdateRetriesCount = 5; we will always experience a failure in 4 of them. One solution might be to parametrize this method with retry count and set it to the number of people participating in a conversation when this function is called. However I think it is quite cumbersome. I would rather drop a retry count constraint and start a time counter when this method begins. Each while loop round I would check current timestampt an calculate how much time has passed since the method was called. Limit would be put only on the total time we can spend working inside this method.

12 ↗(On Diff #26405)

My intention to use smaller constant was that we expect olm sessions to be updated more frequently than olm account. Additionally there will be more concurrent updates at the same time. Therefore it is wise to wait shorter between subsequent retries.

keyserver/src/updaters/olm-session-updater.js
11 ↗(On Diff #26405)

Can you create a follow-up task for this?

keyserver/src/updaters/olm-session-updater.js
11 ↗(On Diff #26405)

I would rather do it before landing this stack. I wouldn't expect this change to take more than 15-30 minutes.

keyserver/src/updaters/olm-session-updater.js
18 ↗(On Diff #26405)

Doesn't need to be $ReadOnly

Allow to pass discionary of messages to encrypt. This API is more robust

This revision is now accepted and ready to land.May 16 2023, 8:38 AM
keyserver/src/updaters/olm-session-updater.js
1

Can we add a newline here?

keyserver/src/updaters/olm-session-updater.js
18 ↗(On Diff #26650)

If we're going to do encryption of all fields in one pass, should we simplify this function so that it only takes one message?

keyserver/src/updaters/olm-session-updater.js
18 ↗(On Diff #26650)

My opinion on this is that it would be better to have this function taking one message if it was designed to deal specifically with notifications. However signature of this function indicates that it is supposed to work with both types of sessions. I am not sure what kind of scenarios are going to utilise content session, but if they need possibility to encrypt a couple of messages at once if is beneficial to have such API.

keyserver/src/updaters/olm-session-updater.js
18 ↗(On Diff #26650)

Okay, we can leave as-is