Page MenuHomePhabricator
Differential D8697

Significant refactor of notification encryption code
ClosedPublic
Actions

Authored by marcin on Aug 1 2023, 6:52 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 8, 7:27 PM
Unknown Object (File)
Wed, May 8, 7:27 PM
Unknown Object (File)
Wed, May 8, 7:27 PM
Unknown Object (File)
Tue, May 7, 10:16 PM
Unknown Object (File)
Mon, May 6, 12:27 PM
Unknown Object (File)
Sat, May 4, 5:05 PM
Unknown Object (File)
Sat, May 4, 5:05 PM
Unknown Object (File)
Sat, May 4, 5:05 PM
View All 88 Files
Subscribers
ashoat

Details

Reviewers
tomek
bartek
atul
ashoat
Commits
rCOMMafe1c380344f: Significant refactor of notification encryption code
Summary

This differential does two important things:

  1. Changes notification encryption API so that we must pass device tokens along with cookieIDs and encrypted notifications carry thos device token and cookieIDs. This enables us to use thos methods with ease without need to handle independent arrays

ordering which is a brittle design.

  1. It uses the ability to conditionally skip olm session database update. Previously we used to encrypt two notifications: with and without messageInfos. Then we decided which one is going to be delivered. As a result the client hand to keep skipped olm

key for the notif that wasn't delivered. Now it is ensured it is not happening.

Test Plan
  1. Test that encrypted notifications/ rescinds and badge-only notifications work correctly.
  2. Ensure that each notification advances keyserver olm session version by one. It can be done by executing select * from olm_sessions;. This means that only one encryption is persisted to the db and it is the one that encrypts notification delivered to

the client.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Aug 1 2023, 6:52 AM
Herald added a subscriber: ashoat. · View Herald TranscriptAug 1 2023, 6:52 AM
marcin added a parent revision: D8695: Remove redundant warnings about notifications without message infos that exceed limits.Aug 1 2023, 6:54 AM
marcin added a child revision: D8589: Upload Android notification payload to blob service if its size exceeds limits.
marcin updated this revision to Diff 29436.Aug 1 2023, 6:58 AM

Rebase to update commit message

marcin added a child revision: D8698: Upload iOS notification payload to blob service if its size exceeds limits.Aug 1 2023, 7:05 AM
Harbormaster completed remote builds in B21457: Diff 29433.Aug 1 2023, 7:43 AM
Harbormaster completed remote builds in B21460: Diff 29436.Aug 1 2023, 8:23 AM
marcin requested review of this revision.Aug 1 2023, 8:23 AM
atul added a reviewer: ashoat.Aug 1 2023, 2:18 PM

(Preemptively adding @ashoat as reviewer)

atul resigned from this revision.Aug 1 2023, 2:21 PM

Don't have a ton of context here

ashoat resigned from this revision.Aug 1 2023, 6:23 PM

Not sure I need to be added here... maybe somebody else can give it a first stab, and add me if they feel it's necessary? The last several of @marcin's notifs diffs have been reviewed by @tomek / @bartek and I think things have generally gone well (outside of a few strange issues, but we haven't really identified any code / code review problems as causing those)

tomek added inline comments.Aug 2 2023, 3:28 AM
keyserver/src/push/crypto.js
19 ↗(On Diff #29436)

Are you going to replace the violated with exceeded later in the stack?

143 ↗(On Diff #29436)

Shouldn't we check resultPayload?

keyserver/src/push/rescind.js
244–247 ↗(On Diff #29436)

Shouldn't we map to an object that contains cookieID?

keyserver/src/push/send.js
671 ↗(On Diff #29436)

Are you sure that we should use this version?

marcin added inline comments.Aug 2 2023, 5:18 AM
keyserver/src/push/crypto.js
19 ↗(On Diff #29436)

It will be replaced - I just wanted to done refactoring is smaller parts.

143 ↗(On Diff #29436)

We should. We might run into a case when notification size exceeds limits exactly after encryptionFailed: 1 field is added.

keyserver/src/push/rescind.js
244–247 ↗(On Diff #29436)

Once we return from this function rescinds are directly passed to apnPush/fcmPush where cookieID is not necessary. There is not code that would make use of this field.

keyserver/src/push/send.js
671 ↗(On Diff #29436)

Encrypted notifications were introduced since codeVersion 223. This differential does not introduce changes that break this functionality.

marcin updated this revision to Diff 29482.Aug 2 2023, 5:43 AM
  1. Pass resultPayload to pauloadSizeValidator instead of unencryptedPayload.
  2. Rename payloadSizeViolated to payloadSizeExceeded and devicesWithSizeViolation to devicesWithExcessiveSize
Harbormaster completed remote builds in B21489: Diff 29482.Aug 2 2023, 6:01 AM
tomek accepted this revision.Aug 4 2023, 4:47 AM
tomek added inline comments.
keyserver/src/push/crypto.js
97 ↗(On Diff #29482)

Does it need to be optional?

107–109 ↗(On Diff #29482)

You can consider using a shorthand

19 ↗(On Diff #29436)

Thanks for replacing it! But there are still some usages of violated in this diff.

keyserver/src/push/send.js
735–737 ↗(On Diff #29482)

Do we need to map?

This revision is now accepted and ready to land.Aug 4 2023, 4:47 AM
marcin added inline comments.Aug 4 2023, 8:40 AM
keyserver/src/push/crypto.js
97 ↗(On Diff #29482)

The encryptAndroidNotificationPayload function is used for visible notifs, badge only notifs and rescinds. Only visible notifs may exceed push services limits so we don't want to bother including this parameter when encryption rescinds and badge only notifs.

19 ↗(On Diff #29436)

Those usages are dbPersistConditionViolated variables which are fields of the object returned from encryptAndUpdateOlmSession . In encryptAndUpdateOlmSession we can pass any condition that might prevent us from updating olm session in the database not necessarily related to payload size so I use generic violated. So those usages should stay this way.

keyserver/src/push/send.js
735–737 ↗(On Diff #29482)

Without map objects in resulting array contain more data than just deviceToken and cookieIDs so flow will complain since this array is further passed to function that expects only deviceToken and cookieIDs to be present.

marcin updated this revision to Diff 30075.Aug 18 2023, 7:11 AM

Rebase before landing

Harbormaster completed remote builds in B21894: Diff 30075.Aug 18 2023, 7:28 AM
marcin updated this revision to Diff 30082.Aug 18 2023, 7:54 AM

Rebase before landing

Harbormaster completed remote builds in B21899: Diff 30082.Aug 18 2023, 8:11 AM
Closed by commit rCOMMafe1c380344f: Significant refactor of notification encryption code (authored by marcin). · Explain WhyAug 18 2023, 8:13 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMafe1c380344f: Significant refactor of notification encryption code.

Revision Contents
Changeset List

PathSize
keyserver/
src/
push/
222 lines
29 lines
219 lines
14 lines

Diff 30084

View Options

keyserver/src/push/crypto.js

Loading...
View Options

keyserver/src/push/rescind.js

Loading...
View Options

keyserver/src/push/send.js

Loading...
View Options

keyserver/src/push/types.js

Loading...