This differential implements Android notification encryption before sending them.
Details
Send notifications to Android device. On each notification issue query select * from olm_sessions;. Ensure that version column of session associated with current user increments its value on each
notification.
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
keyserver/src/push/send.js | ||
---|---|---|
834–860 ↗ | (On Diff #26671) | Same feedback as on iOS diffs – we should merge these so we only have to do a single pass |
keyserver/src/push/send.js | ||
---|---|---|
791 ↗ | (On Diff #26671) | Why do we check if codeVersion > 0? |
826–827 ↗ | (On Diff #26671) | Is it a good idea to encrypt both of these? Is the session state affected by that? |
keyserver/src/push/utils.js | ||
137–143 ↗ | (On Diff #26671) | Is it guaranteed that device tokes match notifications? Can we make the API safer by merging these into one object with deviceToken and notification fields? (and add an array of these as a parameter) |
keyserver/src/session/cookies.js | ||
834 ↗ | (On Diff #26671) | Why 10? |
keyserver/src/push/send.js | ||
---|---|---|
791 ↗ | (On Diff #26671) | This is an artifact from debugging. I forgot to set it to FUTURE_CODE_VERSION. |
keyserver/src/push/utils.js | ||
137–143 ↗ | (On Diff #26671) |
It is guaranted since:
I think we can |
keyserver/src/session/cookies.js | ||
834 ↗ | (On Diff #26671) | Artifact from debugging. I forgot to set it to FUTURE_CODE_VERSION. |
keyserver/src/push/send.js | ||
---|---|---|
826–827 ↗ | (On Diff #26671) |
We must encrypt both since it is the only way to get to know if adding messageInfos does not overflow fcm push limit.
It is but not in a dangerous way. Only one of those will be sent to client, and the other can be viewed as skipped messages. The client can handle skipped messages by advancing its ratchet keys. Every ratchet-encrypted message contains a header with the counter, so that the recipent can advance their keys if they received messages no. 1, 2 and 5 but didn't receive message 3 and 4. It can therefore decrypt 5 when it arrives and 3, 4 in case they happen to arrive later. |
- Use FUTURE_CODE_VERSION in everu codeVersion comparison
- Ona pass instead of three when evaluating size of notification with message infos.
- Change types and refactor so that fcmPush taks an array of pairs of devicetokens and notifications, so that we are sure notifications are sent to the device token associated with cookie id they were encrypted with
keyserver/src/push/send.js | ||
---|---|---|
245 ↗ | (On Diff #26869) | |
266–270 ↗ | (On Diff #26869) | Shorthand |
268 ↗ | (On Diff #26869) | We appear to rely on the assumption that cookieIDs and deviceTokens will have the same length, but I'm not sure that this is guaranteed in D7815. If for some reason two cookieIDs exist for the same deviceToken (should not be possible), this assumption will be broken. Furthermore, I could imagine a future developer updating getDevicesByPlatform and breaking this assumption. The assumption is not clear in the code. We have two options here:
{ cookieIDs: Set<string>, deviceTokens: Set<string> } With the following: Array<{ cookieID: string, deviceToken: string }> |
1271–1277 ↗ | (On Diff #26869) | await is an important keyword, and we should always be clear about it, and make sure it's visible. We always make sure that await appears as the first keyword on the right side of an assigment, eg.: const leftSide = await rightSide; We can rewrite this code to follow this rule. |
1273 ↗ | (On Diff #26869) | Your use of await here is very bad. You're using it inside a loop, forcing every single invocation of the loop to await one-at-a-time. This has come up before – I urge you to make sure you understand async / await and Promises going forward, so that you are able to write performant async code without needing reviewers to rewrite it for you |
Refactor to create delivery promise from android badhe notifications encryption and notification transport
(don't have much context here, feel free to re-add me if there's anything specific I can be helpful w/)
keyserver/src/push/send.js | ||
---|---|---|
776 ↗ | (On Diff #26980) | We should probably prefer $ReadOnlyArray. If we need mutability, we can use a shorthand AndroidNotification[] |
864 ↗ | (On Diff #26980) | This is really strange. We're iterating over cookies but not use them. Is it a mistake? We're returning the same notif multiple times. |
keyserver/src/push/send.js | ||
---|---|---|
864 ↗ | (On Diff #26980) | I thought this was weird on my initial read too, but after looking at it more closely I think it's correct. The caller of prepareAndroidNotification expects it to return one result for each cookieID. This is because it later pairs each notification with the corresponding deviceToken for that cookieID based on the notification's order in the array. This is arguably a brittle design. We could reconsider the approach, and instead pass the devices array (containing pairs of deviceTokens and cookieIDs) into prepareAndroidNotification, instead of just the cookieIDs. Then we could make sure that this function returns the format we want (notifications paired with deviceTokens). I think that design would be more readable (less weird lines like the one @tomek pointed out), and less brittle (if a future developer changes something, they are less likely to break an assumption). @marcin, what do you think? If we make this change, we should make it for both iOS and Android, assuming it applies for both. |
keyserver/src/push/send.js | ||
---|---|---|
776 ↗ | (On Diff #26980) | Sure - I will replace it with $ReadOnlyArray. Mutability is not necessary here. |
864 ↗ | (On Diff #26980) |
Looking at changes made in this differential it can be concluded that this is intentional. It was introduced in order to meet requirements of new typing. New typing here is that send{APNs/Android}Notification requires the array of pairs of deviceToken and notification to ensure that each encrypted notification is delivered to the particular deviceToken associated with cookieID used to encrypt notif (olm session related to this cookieID, to be precise). Therefore in order to meet this typing we have to repeat the same notification for each deviceToken in case notification is not encrypted.
I agree that this code might be of better quality and change suggested by @ashoat should be made. However the fact that this snippet of code was enough to the reviewer to request changes is concerning for me. Looking at this differential holistically was enough to figure out that it is not a mistake, but rather a consequence of types we agreed on. Additionally, it is worth to note that this differential already had many cycles during which we detected and addressed much more serious issues. It could be a flag for the reviewer that it is unlikely there is a mistake here. The fact that code quality is not perfect here could well be handled by a simple comment with suggestion on how to beautify this code. This team has a strong culture of not landing a diff with unaddressed comments. We should be taking advantage of this good convention to speed-up the process of delivering features where we know the code was tested and works correctly but there are some minor issues left about code quality.
I agree we shouldn't leave discrepancy between platforms and apply new approach to both. That said I will introduce those changes to iOS diff. However I would pursue landing the entire stack, apart from this diff, once I am back to work on Monday in order not to block entire goal, other people who might be interested in some parts of this stack and my future work regarding e2e rescinds and large notif download. |
keyserver/src/push/send.js | ||
---|---|---|
864 ↗ | (On Diff #26980) | I think I should add, that I expressed my concerns before I noticed there has been a change in reviewers and believed the stack will be blocked until June. |
Change typing so that notifications are packaged with relevant device token in advance