[web] Add component to display encrypted media
ClosedPublic
Actions

Authored by bartek on Apr 3 2023, 3:14 AM.

Details

Reviewers

atul
marcin
inka
• kuba
tomek

Group Reviewers

Restricted Owners Package

(Owns No Changed Paths)

Commits

rCOMM13387b3d37e5: [web] Add component to display encrypted media

Summary

This diff adds a <EncryptedMultimedia> component that is able to decrypt and display encrypted image or video.
A loading indicator is displayed during decryption and error mark on error.
This required slight CSS adjustments, but they don't affect existing multimedia

Depends on D7268

Test Plan

Existing non-encrypted media is not affected. New component is tested along with subsequent diffs.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

bartek created this revision.Apr 3 2023, 3:14 AM

bartek held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptApr 3 2023, 3:14 AM

bartek added a child revision: D7280: [web] Modify MultimediaModal to support encrypted media.Apr 3 2023, 3:17 AM

Harbormaster completed remote builds in B17927: Diff 24557.Apr 3 2023, 3:30 AM

bartek published this revision for review.Apr 3 2023, 4:56 AM

Looks reasonable to me, but would be great if @ashoat OR @tomek could take another look here since (I think) they have more context.

web/media/encrypted-multimedia.react.js
30–45 ↗	(On Diff #24557)	Instead of defining and then invoking can we use IIFE pattern (https://developer.mozilla.org/en-US/docs/Glossary/IIFE) here? Let me know if I'm missing something.

bartek added inline comments.Apr 4 2023, 12:19 AM

web/media/encrypted-multimedia.react.js
30–45 ↗	(On Diff #24557)	I consider IIFE less readable, but I'm not opposed to changing this

tomek accepted this revision.Apr 4 2023, 12:35 AM

tomek added inline comments.

web/media/encrypted-multimedia.react.js
4 ↗	(On Diff #24557)	Why do we import it like this? Does it have some side effects?
16 ↗	(On Diff #24557)	Do we have to send this metadata next to an encrypted payload? Ideally, we should avoid leaking this info by e.g. including it inside the encrypted message. Only after decrypting we would learn whether this is photo or image e.g. based on a flag / type field.
30–45 ↗	(On Diff #24557)	Does it mean that we will decrypt it every time this component mounts?
web/media/media.css
64 ↗	(On Diff #24557)	Isn't it just `margin: auto`?

This revision is now accepted and ready to land.Apr 4 2023, 12:35 AM

bartek added inline comments.Apr 4 2023, 1:36 AM

web/media/encrypted-multimedia.react.js
4 ↗	(On Diff #24557)	Good catch - it's a leftover from previous component that I tried to use as a loading indicator
16 ↗	(On Diff #24557)	That's basically how the whole approach is implemented - this `EncryptedMediaType` is the last missing part of ENG-386 - we need to pass the type along with encryption key as a part of multimedia messages. Given existing media handling implementation, your approach would be much more difficult to achieve. However, I believe that your concern will be solved by e2e encrypting message contents.
30–45 ↗	(On Diff #24557)	Yes, in contrast to native, decryption on web is very fast and I decided not to add cache here. It could be easily introduced if needed. Some more context: https://linear.app/comm/issue/ENG-389#comment-4eae04a3
web/media/media.css
64 ↗	(On Diff #24557)	¯\_(ツ)_/¯ I copy-pasted this from above, but you're right - both options do work

tomek added inline comments.Apr 4 2023, 4:45 AM

web/media/encrypted-multimedia.react.js
16 ↗	(On Diff #24557)	Ok, that makes sense.

atul added inline comments.Apr 4 2023, 9:54 AM

web/media/encrypted-multimedia.react.js
30–45 ↗	(On Diff #24557)	I consider IIFE less readable, but I'm not opposed to changing this Fair, now that you mention it the "invoking" `()` can often be hard to "find" with all the braces and indentation. This is probably better.