Page MenuHomePhabricator

Implement sending E2EE notification from the client
ClosedPublic

Authored by marcin on Jul 25 2024, 9:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 1:18 PM
Unknown Object (File)
Tue, Oct 22, 8:33 AM
Unknown Object (File)
Tue, Oct 22, 8:32 AM
Unknown Object (File)
Tue, Oct 22, 8:32 AM
Unknown Object (File)
Tue, Oct 22, 8:32 AM
Unknown Object (File)
Fri, Oct 18, 5:36 PM
Unknown Object (File)
Mon, Oct 14, 12:15 PM
Unknown Object (File)
Sep 30 2024, 8:11 AM
Subscribers

Details

Summary

This differential implements sending e2ee notifications to clients when reducers return notifs message datas.

Test Plan
  1. Apply this patch: https://gist.github.com/marcinwasowicz/c9944e2ed8ed7bbd269017aff7a9a336
  2. Send messages back and forth between Android and iOS devices. Ensure each message comes with two duplicate notifications. One is from the keyserver and the other is from the TB.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 25 2024, 10:09 AM
Harbormaster failed remote builds in B30676: Diff 42781!
kamil added 1 blocking reviewer(s): tomek.

Accepting with some comments, you can re-request review if needed.

Might be good to get @tomek opinion too.

lib/handlers/db-ops-handler.react.js
79–84 ↗(On Diff #42781)

I am not sure what is the expected design here, but if we want to return notifs along with store operations (see my comment below in lib/types/store-ops-types.js) first make sure we change our own store before sending notifs - then this code should be put after await processDBStoreOperations(ops);.
If not and we don't care about the case when we send notif and potentially fail processing store ops - then I would extract notificationsMessageDatas from StoreOperations and put this code somewhere else.

cc. @tomek

lib/push/send-hooks.react.js
43 ↗(On Diff #42781)
61 ↗(On Diff #42781)
65 ↗(On Diff #42781)

you can create a task to fix this one on TB side - but probably not worth prioritizing this now

88–103 ↗(On Diff #42781)

I think those two can be merged

lib/types/store-ops-types.js
98 ↗(On Diff #42781)

this is not a store operation - it's not related to any store or DB and this it should be a separate param returned from baseReducer, however, it will require some changes in DBOpsHandler curious for @tomek's perspective

lib/utils/config.js
33 ↗(On Diff #42781)

It's okay to leave it now but this is a completely separate change and IMO should be introduced in a separate diff

lib/types/store-ops-types.js
98 ↗(On Diff #42781)

It is quite convenient to have this next to the store operations. We could start thinking about DBOpsHandler more like SideEffectHandler - then DB ops, P2P messages, and notifs are the side effects.

Introducing a new handler that will do something similar to what the existing one does, doesn't sound too beneficial. I guess we can introduce notificationsMessageDatas to DBOpsEntry instead of to StoreOperations. Maybe we should rename the handler and the entry to be more general?

tomek requested changes to this revision.Jul 31 2024, 5:48 AM
This revision now requires changes to proceed.Jul 31 2024, 5:48 AM

After discussing it with @kamil and @tomek we concluded that changes necessary to be done in this diff include:

  1. Sequentially awaiting db ops processing before notifs processing.
  2. Put MessageData directly in DBOpsEntry.
tomek added inline comments.
lib/push/send-hooks.react.js
130 ↗(On Diff #43009)

Should we keep this console log?

lib/types/db-ops-types.js
15 ↗(On Diff #43009)

I think we should use a higher-level abstraction. What matters to DBOpsEntry is that it contains some data for the notifs - it doesn't matter that it is a message data. So I think we can make this prop more generic.

This revision is now accepted and ready to land.Aug 1 2024, 1:14 AM
lib/push/send-hooks.react.js
130 ↗(On Diff #43009)

This is a debugging artifact. Will be removed.

Extract notificationsCreationData as separate DBOpsEntry type

tomek requested changes to this revision.Aug 26 2024, 5:45 AM

It is a really good idea to split this diff because it does a couple of unrelated things. Especially, the part about sending notifs requires some discussion and changes, while all the other parts could be accepted.

lib/handlers/db-ops-handler.react.js
36–53 ↗(On Diff #43554)

We should discuss with @kamil whether it is the best place for sending the notifs.

lib/push/send-hooks.react.js
147–163 ↗(On Diff #43554)

We can consider having specs for each platform in the future.

This revision now requires changes to proceed.Aug 26 2024, 5:45 AM

Remove logic in db-ops-handler

kamil added inline comments.
lib/push/send-hooks.react.js
34–96

those methods are used only once, and creating them feels like a boilerplate, I would prefer to see login in call sites or ideally create a spec for them but up to you

This revision is now accepted and ready to land.Aug 27 2024, 7:56 AM