Page MenuHomePhabricator

Handle client response to initial notifications encrypted message request
ClosedPublic

Authored by marcin on May 18 2023, 5:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Nov 25, 11:19 AM
Unknown Object (File)
Sat, Nov 23, 9:13 PM
Unknown Object (File)
Wed, Nov 13, 3:24 PM
Unknown Object (File)
Thu, Nov 7, 6:06 AM
Unknown Object (File)
Thu, Nov 7, 6:06 AM
Unknown Object (File)
Thu, Nov 7, 6:05 AM
Unknown Object (File)
Thu, Nov 7, 6:05 AM
Unknown Object (File)
Thu, Nov 7, 6:05 AM
Subscribers

Details

Summary

This differential handles client response to initial notifications encrypted message request by creating new olm session for notifs.

Test Plan
  1. Log out.
  2. Comment the code in log in panel that creates olm session for notifs.
  3. Log in.
  4. Query select * from olm_sessions and ensure there is a new entry for notifs

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Added @atul as blocking reviewer for entire substack since he implemented very similar diffs for SignedIdentityKeysBlob.

tomek added inline comments.
keyserver/src/socket/session-utils.js
136–137 ↗(On Diff #26634)

I know this approach was used earlier, but isn't it just tString(serverRequestTypes.INITIAL_NOTIFICATIONS_ENCRYPTED_MESSAGE)?

245–248 ↗(On Diff #26634)

It would be better if it was verified using tComb validator

253 ↗(On Diff #26634)

What is the meaning of this value?

256–258 ↗(On Diff #26634)

Is it safe to ignore the exception? Would the client ever know that something went wrong?

keyserver/src/socket/session-utils.js
136–137 ↗(On Diff #26634)

I am not sure if that is what you were thinking about but an attempt to use it in the following way:

tShape({
   type: t.String(serverRequestTypes.INITIAL_NOTIFICATIONS_ENCRYPTED_MESSAGE),
   initialNotificationsEncryptedMessage: t.String,
 }),

results in flow error:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ src/socket/session-utils.js:136:13

Cannot call t.String because a call signature declaring the expected parameter / return type is missing in
TIrreducible [1]. [prop-missing]

     src/socket/session-utils.js
     133│     signedIdentityKeysBlob: signedIdentityKeysBlobValidator,
     134│   }),
     135│   tShape({
     136│     type: t.String(serverRequestTypes.INITIAL_NOTIFICATIONS_ENCRYPTED_MESSAGE),
     137│     initialNotificationsEncryptedMessage: t.String,
     138│   }),
     139│ ]);

     ../lib/flow-typed/npm/tcomb_v3.x.x.js
 [1] 110│     +String: TIrreducible<string>,
Found 1 error
253 ↗(On Diff #26634)

The keyserver has to olm accounts: one for notifications and the other for communication with identity service. createOlmSession can be used to create session for both of this accounts. This argument informs which olm account we will use to create olm session. In this particular case we will use olm accounts for e2e notifications.

256–258 ↗(On Diff #26634)

I think it si for three reasones:

  1. It has already been used for other requests and proved to work.
  2. If it fails, the keyserver will re-attempt to retrieve this data from the client.
  3. If this fails due to some permanent db failure we will be informed by prepareEncryptedIOSNotifications function that we are constantly missing olm session for a client that has code version high enough to handle notification encryption.
  4. The client code in this stack is implemented in a way that it is aware of a case when notifications are delivered unencrypted. When this issue: https://linear.app/comm/issue/ENG-2670/c-equivalent-of-dev-flag#comment-45f70230 is resolved staff members will be informed that their notifications are delivered unencrypted.

Until the linear issue mentioned above is not resolved we don't really have a proper way to handle such failure on the client.

Use tcomb to validate request

This revision is now accepted and ready to land.May 22 2023, 9:50 AM
This revision is now accepted and ready to land.May 31 2023, 8:38 AM