Page MenuHomePhabricator

Implement utility function on the keyserver to upload notification payload to blob service
ClosedPublic

Authored by marcin on Jul 18 2023, 6:49 AM.
Tags
None
Referenced Files
F3389444: D8529.diff
Fri, Nov 29, 7:08 PM
Unknown Object (File)
Wed, Nov 27, 2:06 AM
Unknown Object (File)
Wed, Nov 27, 1:48 AM
Unknown Object (File)
Sat, Nov 23, 2:31 PM
Unknown Object (File)
Sat, Nov 23, 12:51 PM
Unknown Object (File)
Sat, Nov 23, 12:51 PM
Unknown Object (File)
Sat, Nov 23, 9:59 AM
Unknown Object (File)
Sat, Nov 23, 9:41 AM
Subscribers

Details

Summary

This differential introduces utility function on the keyserver that AES-encrypts notification payload and uploads it to the keyserver. Changes in eslint configuration that are necessary for native node fetch API usage are introduced as
well.

Test Plan
  1. Launch blob service locally.
  2. Modify blob-service.js file in facts directory to point to local IP address
  3. Modify send.js file to execute this method on the stringified Android notification payload and log output to the console.
  4. Send notification to Android device. Examine logs from keyserver and blob service terminals.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/.eslintrc.json
6 ↗(On Diff #28774)

Even the latest version of globals (see the last comment) does not define Blob as globally available in node, so we have to add it manually to use it on the keyserver without ESLint complaining.

keyserver/src/push/utils.js
393 ↗(On Diff #28774)

new TextEncoder().encode() transforms string into Uint8Array of correct UTF8 byte stream.

465 ↗(On Diff #28774)

encryptionKey obtained from generateKey is not necessarily correct UTF8 byte stream so we have to choose different encoding. I experimented with using TextDecoder.decode() and I was unable to decrypt data on the native due to invalid encryption key. Only the approach above allowed me to use AESCryptoModule with the encryption key sent from the keyserver.

package.json
56 ↗(On Diff #28774)

ESLint uses the following repository to get methods and variables globally available in node environment: https://github.com/sindresorhus/globals. Unfortunately it is only the latest version of this package (13.20.0) that defines fetch and FormData as node globals. Updating ESLint itself wouldn't help since the latest version of ESLint (8.45.0) expects ^13.19.0 version of globals (https://github.com/eslint/eslint/blob/v8.45.0/package.json#L85) that leaves possibility that we will end up with version 13.19.0 that is still lacks fetch and FormData definitions.

keyserver/src/push/utils.js
393 ↗(On Diff #28774)

Lucky you, having TextEncoder available ;)
I had to write ArrayBuffer <-> any-string-repr conversions myself in native

397–402 ↗(On Diff #28774)

IIRC we have a convention not to await inside other calls

443 ↗(On Diff #28774)

For multimedia uploads, I rely on results of data_exists before starting the UPLOAD_BLOB call.

In your case, you could potentially call both in parallel using Promise.all. What do you think? Calling the upload endpoint while given blob hash already exists, results in HTTP 409 Conflict. However, you had to ignore the data_existsresult from the ASSIGN_HOLDER call because its value would rely on request processing order.

465 ↗(On Diff #28774)

Generally, bytes stored as strings should be either base64 or hex-encoded to avoid UTF-8 issues you mentioned.

What about storing it as HEX string? See uintArrayToHexString() in lib/media/data-utils.js. I used this to store multimedia encryption keys

Blob upload looks good

This revision is now accepted and ready to land.Jul 25 2023, 3:01 AM
keyserver/src/push/utils.js
364 ↗(On Diff #28959)

Should we use the built-in fetch for this instead of node-fetch, and deprecate node-fetch? I know this feels like a follow-up task, but I think too often we leave "cruft" like this in the codebase and create follow-up tasks that never get addressed. This seems like a great opportunity for a quick "red diff" that should only take an hour, and will let us delete an outdated package

ashoat added inline comments.
keyserver/src/push/utils.js
364 ↗(On Diff #28959)

It might be helpful to get @michal's help for testing here, since it appears the only uses of node-fetch are for Windows notifs

After a short discussion with @tomek we decided to land this differential since it keeps working correctly even if isolated from the rest of the stack and @tomek needs it for the development of his work on invite links.

keyserver/src/push/utils.js
465 ↗(On Diff #28774)

We could do this but I chose Base64 since native platforms have API for straightforward Base64 encoding and decoding. Using HEX would require us to write some custom encoding/decoding.