[lib][keyserver] Attach MIME and encryption key to uploads
ClosedPublic
Actions

Authored by bartek on Mar 24 2023, 3:50 AM.

Details

Reviewers

atul
marcin
ashoat

Commits

rCOMMf5921700453c: [lib][keyserver] Attach MIME and encryption key to uploads

Summary

Part of ENG-3393

In order to upload encrypted media, we need to store encryption key on the keyserver.
Also, MIME type can no longer be detected on keyserver side, so we need to send it along with the media

This diff modifies the upload_multimedia endpoint to support providing these values. They're validated, but not used anywhere yet.

MIME type is already available when uploading, it only needed to be exposed.

Test Plan

Modified native input-state-container to provide example encryption key. Added console.log on keyserver side to see if both values are passed correctly.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

bartek created this revision.Mar 24 2023, 3:50 AM

bartek held this revision as a draft.

Herald added a subscriber: tomek. · View Herald TranscriptMar 24 2023, 3:50 AM

bartek added a child revision: D7167: [keyserver] Process and store encrypted media.Mar 24 2023, 4:04 AM

Harbormaster completed remote builds in B17574: Diff 24062.Mar 24 2023, 4:05 AM

bartek published this revision for review.Mar 24 2023, 4:13 AM

ashoat added inline comments.Mar 24 2023, 11:00 AM

keyserver/src/uploads/uploads.js
66 ↗	(On Diff #24062)	In what scenarios is `files.length !== 1`? Is it concerning that we don't have `mimeType` in that scenario?
lib/actions/upload-actions.js
16–20 ↗	(On Diff #24062)	Can we make this `$ReadOnly`?
46 ↗	(On Diff #24062)	What's the point of having `encryption_key` here instead of the same `encryptionKey`?

Passing back to you with question about multiple files being uploaded to the same endpoint at once. I don't think we actually ever do this, so I'm wondering if we can simply throw an exception if we get multiple files in the same upload

keyserver/src/uploads/uploads.js
61 ↗	(On Diff #24062)	Doesn't this need to be `encryption_key` since that's what you set in `stringExtras` below? Or am I missing something?

This revision now requires changes to proceed.Mar 24 2023, 11:41 AM

bartek added inline comments.Mar 24 2023, 10:41 PM

keyserver/src/uploads/uploads.js
66 ↗	(On Diff #24062)	Looks like this endpoint supports only single upload but receives an array. I just followed the pattern of the existing code (you can unwind e.g. 20 lines above)
lib/actions/upload-actions.js
16–20 ↗	(On Diff #24062)	Isn't `Shape` a `$ReadOnly` already? export type Shape<O> = $ReadOnly<$Rest<O, { ... }>>;
46 ↗	(On Diff #24062)	Ahh, my bad, a leftover from a broken rebase 🤦‍♂️ Thanks for catching it!

Fix encryption_key -> encryptionKey

Harbormaster completed remote builds in B17614: Diff 24108.Mar 24 2023, 11:06 PM

ashoat accepted this revision.Mar 25 2023, 2:51 AM

ashoat added inline comments.

keyserver/src/uploads/uploads.js
66 ↗	(On Diff #24062)	What's strange to me is that we don't throw a `ServerError` if more than one file is included in an upload. I can't tell if that's because of my own oversight from back when I wrote this code around 3 years ago, or if there is still some place that uploads multiple files at once (maybe web?)
lib/actions/upload-actions.js
16–20 ↗	(On Diff #24062)	So it is! Sorry I missed that