[native] Encrypt files during media mission
ClosedPublic
Actions

Authored by bartek on Mar 29 2023, 7:04 AM.

Details

Reviewers

atul
marcin
ashoat

Commits

rCOMMc298aa5173e1: [native] Encrypt files during media mission

Summary

This is final diff that allows sending encrypted messages on native. The shouldEncryptMedia boolean has been introduced as a feature flag. Now it's hardcoded to false, but we want to enable it only for Comm community (ENG-3526) - this will be done in another diff.

The whole function becomes kind of spaghetti - a refactor follow-up wouldn't be a bad idea

Depends on D7234, D7235

Test Plan

Sending multimedia works as before when the flag is false.
Set it to true, then send both photos and videos. Ensure both work as expected

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Mar 29 2023, 7:04 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptMar 29 2023, 7:04 AM

Harbormaster completed remote builds in B17782: Diff 24333.Mar 29 2023, 7:19 AM

bartek published this revision for review.Mar 30 2023, 4:50 AM

bartek edited the summary of this revision. (Show Details)

The whole function becomes kind of spaghetti - a refactor follow-up wouldn't be a bad idea

Ah dang, InputStateContainer is already one of the most difficult to understand places in the codebase for me. Strongly agree we should refactor at some point, but probably not a priority at the moment.

This revision is now accepted and ready to land.Mar 30 2023, 8:14 AM

bartek added a parent revision: D7230: [lib] Add option to force-create multimedia-type message.Mar 30 2023, 9:36 AM

ashoat accepted this revision.Mar 30 2023, 3:20 PM

ashoat added inline comments.

native/input/input-state-container.react.js
163 ↗	(On Diff #24333)
817 ↗	(On Diff #24333)	Does this mean that we will have `encryptionKey: undefined` in the `uploads.extras` MySQL column for non-encrypted uploads? If so, can we avoid that?
895 ↗	(On Diff #24333)	It might be possible to do something like this instead: let payload: UpdateMultimediaMessageMediaPayload; if (updateMediaPayload.mediaUpdate.type === 'encrypted_video') { payload = { ...updateMediaPayload, mediaUpdate: { ...updateMediaPayload.mediaUpdate, thumbnailID, ...thumbnailSourcePayload, }, }; } else { // Same as above, for Flow payload = { ...updateMediaPayload, mediaUpdate: { ...updateMediaPayload.mediaUpdate, thumbnailID, ...thumbnailSourcePayload, }, }; } However, because we don't do the same thing on lines 850 and 862, it's likely that Flow won't be able to conclude that `payload` really is `UpdateMultimediaMessageMediaPayload`. This would probably require a lot of boilerplate to type correctly...

bartek added inline comments.Mar 31 2023, 3:25 AM

native/input/input-state-container.react.js
817 ↗	(On Diff #24333)	We're already avoiding that here
895 ↗	(On Diff #24333)	This if-else fixed it, but I got rid of `thumbnailSourcePayload` and put these directly. It's less concise but solved Flow complaints