Page MenuHomePhabricator

Enable encrypted notifications to have keyservevrID or senderDeviceID
ClosedPublic

Authored by marcin on Jun 11 2024, 10:28 AM.
Tags
None
Referenced Files
F3150225: D12395.diff
Mon, Nov 4, 10:43 PM
Unknown Object (File)
Sun, Nov 3, 10:29 AM
Unknown Object (File)
Sat, Nov 2, 4:12 PM
Unknown Object (File)
Fri, Nov 1, 7:00 PM
Unknown Object (File)
Tue, Oct 29, 8:24 AM
Unknown Object (File)
Fri, Oct 25, 11:28 AM
Unknown Object (File)
Thu, Oct 24, 8:06 PM
Unknown Object (File)
Sat, Oct 19, 10:08 PM
Subscribers

Details

Summary

This differential enables encrypted notifications to have either keyserverID or senderDeviceID field. Theoretically we could have only senderDeviceID but this has disadvantages. Firstly we would need to handle old clients. Secondly we will need keyserverID anyway since thin threads have different mechanism to calculate unreadCount so we need a way to distuinguish whether a notif comes from keyserver or peer device.

Test Plan
  1. Flow
  2. Test all notifs types for all platforms and large notifs as well.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/types/notif-types.js
103 ↗(On Diff #41212)

It looks strange but I hand to unwind AndroidVisualNotificationPayloadBase here to make AndroidVisualNotification work. For some reason without unwinding flow would complain that AndroidVisualNotification doesn't have fields that it clearly has.

web/push-notif/notif-crypto-utils.js
68 ↗(On Diff #41212)

Soon we will modify this to enforce either senderDeviceID or keyserverID. Not just keyserverID.

114 ↗(On Diff #41212)

For some reason if I used keyserver directly flow would think that it is void | string even with the invariant above.

tomek added inline comments.
keyserver/src/push/crypto.js
283–293 ↗(On Diff #41212)

Is there a reason for keeping these ifs separated?

lib/types/notif-types.js
29 ↗(On Diff #41212)

It's quite surprising to see that a both keyserverID and senderDeviceID are valid values for SenderDeviceID - it feels like senderDeviceID should cover all the options because its name is too similar to the type name. Also, it's strange that SenderDeviceID is an object instead of a string. How about renaming the type name to e.g. SenderDeviceDescriptor?

This revision is now accepted and ready to land.Jun 13 2024, 8:19 AM
lib/types/notif-types.js
103 ↗(On Diff #41212)

I don't think this structure makes sense anymore. If you're just adding a single +id?: string, to all of them, why not inline it?

Perhaps if you inline +id?: string,, can you re-integrate AndroidVisualNotificationPayloadBase?

And finally, does adding $ReadOnly wrappers fix the "unwinding" issue? Remember that read-only status is wiped when spreading a type, so it has to be re-applied at the new type, eg.:

type SpreadType = { +something: string };
type OtherType = $ReadOnly<{
  ...SpreadType,
  +blah: string,
}>

If you don't include $ReadOnly there, then something will not be read-only.

lib/types/notif-types.js
103 ↗(On Diff #41212)

We discussed this in our 1:1 and I was not able to reproduce the issues @marcin had which forced him to the take the "unwinding" approach here. We can return this to using a type spread of AndroidVisualNotificationPayloadBase: https://gist.github.com/Ashoat/bcb8778735d721eecea0f0ffbdfd2447

kamil requested changes to this revision.Jun 19 2024, 2:56 PM

Back to your queue to address inline comments

web/push-notif/notif-crypto-utils.js
114 ↗(On Diff #41212)

I think you can add another invariant here in line 114 to avoid creating new const and satisfy Flow - I saw this pattern in the codebase

This revision now requires changes to proceed.Jun 19 2024, 2:56 PM
lib/types/notif-types.js
29 ↗(On Diff #41212)

This is explained in diff summary. senderDeviceID will not suffice because of older clients that expect keyservervID. Introducing union in type is faster than implementing migrations and checks against code versions. It is an object only as a type. Later in the code it will be spread so that notification have a single string as a key in their payload.

keyserver/src/push/crypto.js
283–293 ↗(On Diff #41212)

Without the second if flow complains as follows:

Cannot return object literal because in type argument  `R` [1]: Either property `badgeOnly` is missing in  object type [2] but exists in  object literal [3] in property `data`. Or property `badge` is missing in  object type [2] but exists in  object literal [3] in property `data`. Or property `rescind` is missing in  object type [4] but exists in  object literal [3] in property `data`. Or property `badge` is missing in  object type [4] but exists in  object literal [3] in property `data`.Flow(incompatible-return)

Looks like it cannot determine if I am returning correct type or some mixture of fields from both types of the union.

  1. Use invariant instead of new const
  2. Rename to SenderDeviceDescriptor
  3. Make types elegant.
This revision is now accepted and ready to land.Jun 21 2024, 1:23 AM

Fix a bug: wrong order of spreads caused notif to miss keyserverID