Page MenuHomePhabricator

Implement function to encrypt android notification
ClosedPublic

Authored by marcin on May 19 2023, 6:58 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 12:48 PM
Unknown Object (File)
Tue, Nov 5, 9:04 AM
Unknown Object (File)
Tue, Nov 5, 9:04 AM
Unknown Object (File)
Tue, Nov 5, 9:04 AM
Unknown Object (File)
Tue, Nov 5, 9:04 AM
Unknown Object (File)
Tue, Nov 5, 9:04 AM
Unknown Object (File)
Tue, Nov 5, 9:03 AM
Unknown Object (File)
Tue, Nov 5, 9:03 AM
Subscribers

Details

Summary

This differential implements function that can encrypt Android notification. Additionally it introduces type for Android notification. The type is very flexible (so is Android notification payload) but its main purpose is to
protect against uninformed developer that might not know that olm doesn't accept anything else than strings. Additionally it protects against attempts to encrypt notification id.

Test Plan

Use this funcion in prepareAndroidNotification. Log the result, examine its content.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/push/crypto.js
69 ↗(On Diff #26669)

Can we construct a new one instead of copying the input?

keyserver/src/push/types.js
3 ↗(On Diff #26669)

Thanks for typing this!

7 ↗(On Diff #26669)

Can we make this read-only?

keyserver/src/push/types.js
7 ↗(On Diff #26669)

I think so, but the code might be a bit more complicated.

keyserver/src/push/types.js
7 ↗(On Diff #26669)

I think it's probably worth it, but it's hard for me to say since I haven't seen the details

tomek added a reviewer: ashoat.
ashoat requested changes to this revision.May 22 2023, 10:28 AM

Passing to your queue

This revision now requires changes to proceed.May 22 2023, 10:28 AM

Encrypt blob of fields in one pass

ashoat requested changes to this revision.May 23 2023, 7:06 AM
ashoat added inline comments.
keyserver/src/push/crypto.js
101–110 ↗(On Diff #26866)

Personally I think this is more clear:

const { id, badgeOnly, ...unencryptedPayload } = notification.data;
const encryptedNotification = { data: { id, badgeOnly } };
126–130 ↗(On Diff #26866)

Personally I prefer to always have the existing data first when doing this pattern

keyserver/src/push/types.js
7 ↗(On Diff #26866)

Still isn't read-only

This revision now requires changes to proceed.May 23 2023, 7:06 AM
keyserver/src/push/crypto.js
126–130 ↗(On Diff #26866)

By changing the AndroidNotification type to be purely readonly, we can no longer introduce this change due to the flow error. It even explicitly advises to change the order of spreads:

Cannot spread object literal because Flow cannot determine a type for object literal [1]. rest of object pattern [2]
cannot be spread because the indexer string [3] may overwrite properties with explicit keys in a way that Flow cannot
track. Try spreading rest of object pattern [2] first or remove the indexer. [cannot-spread-indexer]

     src/push/crypto.js
 [2] 73│   const { id, badgeOnly, ...unencryptedPayload } = notification.data;
       :
     87│   } catch (e) {
     88│     console.log('Notification encryption failed: ' + e);
     89│
 [1] 90│     encryptedNotification.data = {
     91│       ...encryptedNotification.data,
     92│       ...unencryptedPayload,
     93│       encryptionFailed: '1',
     94│     };
     95│     return encryptedNotification;
     96│   }
     97│

     src/push/types.js
 [3]  7│     +[string]: string,

If it is a really important convention we can introduce it but we would probably have to refactor this function significantly.

  1. Make AndroidNotification type read-only
  2. Change order of assignments to increase readability

Makes sense, thanks for explaining @marcin!

This revision is now accepted and ready to land.May 24 2023, 9:29 AM