Page MenuHomePhabricator

Implement web notifs session createion with race condition handling
ClosedPublic

Authored by marcin on Jul 22 2024, 7:18 AM.
Tags
None
Referenced Files
F3339202: D12838.id43143.diff
Thu, Nov 21, 7:02 PM
F3338656: D12838.id43086.diff
Thu, Nov 21, 6:28 PM
F3338547: D12838.id42641.diff
Thu, Nov 21, 6:19 PM
F3332708: D12838.id43086.diff
Thu, Nov 21, 1:15 AM
F3332520: D12838.id42948.diff
Thu, Nov 21, 12:25 AM
Unknown Object (File)
Fri, Nov 15, 3:56 PM
Unknown Object (File)
Tue, Nov 12, 6:14 PM
Unknown Object (File)
Sun, Nov 10, 5:40 AM
Subscribers

Details

Summary

This differential enables notifs inbound session creation on web platforms with race condition handling

Test Plan
  1. Apply this patch: https://gist.github.com/marcinwasowicz/c3da26ab164d1044aa3b2870f3434acd
  2. Log in on web and native
  3. Send message from native to web
  4. Copy the content of encrypted notification addressed to web client
  5. Paste the content in service-worker code and pass it to decryptWebNotification function. Log the result
  6. Expect that logged result is decrypted notification

Now repeat the test plan above but FIRST send message to native. Tunnelbroker can send pushes to native so session will be created on the native side and notification sent to web will be type MESSAGE_TYPE_MESSAGE (1).

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Improvement: use the same functions to fetch and persist olm data regardless of notification type (keyserver/peer device)

kamil added inline comments.
web/push-notif/notif-crypto-utils.js
164–169 ↗(On Diff #42718)

why do we need to throw if we will never reach this branch?

202–209 ↗(On Diff #42718)

can you use validators here?

343–349 ↗(On Diff #42718)

In this case I would create only one variable with object

463–468 ↗(On Diff #42718)

if Flow forcing this, can you move this above and early-exit?

This revision is now accepted and ready to land.Jul 25 2024, 8:18 AM

Fix desktop notifications

web/push-notif/notif-crypto-utils.js
463–468 ↗(On Diff #42718)

I tried and unfortunately flow can't conclude that senderDeviceID is not nullish if we early exit.

Fix keyserver based notifs