Page MenuHomePhabricator

Implement function to encrypt android notification

Authored by marcin on Fri, May 19, 6:58 AM.
Referenced Files
F570617: D7872.id26862.diff
Sat, Jun 3, 1:28 PM
F570455: D7872.id26866.diff
Sat, Jun 3, 5:19 AM
Unknown Object (File)
Wed, May 31, 3:30 PM
Unknown Object (File)
Fri, May 19, 10:45 AM



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

69 ↗(On Diff #26669)

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

3 ↗(On Diff #26669)

Thanks for typing this!

7 ↗(On Diff #26669)

Can we make this read-only?

7 ↗(On Diff #26669)

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

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.Mon, May 22, 10:28 AM

Passing to your queue

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

Encrypt blob of fields in one pass

ashoat requested changes to this revision.Tue, May 23, 7:06 AM
ashoat added inline comments.
101–110 ↗(On Diff #26866)

Personally I think this is more clear:

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

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

7 ↗(On Diff #26866)

Still isn't read-only

This revision now requires changes to proceed.Tue, May 23, 7:06 AM
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]

 [2] 73│   const { id, badgeOnly, ...unencryptedPayload } =;
     87│   } catch (e) {
     88│     console.log('Notification encryption failed: ' + e);
 [1] 90│ = {
     92│       ...unencryptedPayload,
     93│       encryptionFailed: '1',
     94│     };
     95│     return encryptedNotification;
     96│   }

 [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.Wed, May 24, 9:29 AM