Page MenuHomePhabricator

[keyserver] Use the internal-ip package in the dev environment to ensure physical device / emulators can access uploads
ClosedPublic

Authored by rohan on Feb 4 2023, 3:28 PM.
Tags
None
Referenced Files
F3335983: D6587.diff
Thu, Nov 21, 1:21 PM
Unknown Object (File)
Mon, Nov 18, 7:36 PM
Unknown Object (File)
Mon, Nov 18, 7:35 PM
Unknown Object (File)
Mon, Nov 18, 7:35 PM
Unknown Object (File)
Mon, Nov 18, 7:35 PM
Unknown Object (File)
Mon, Nov 18, 7:35 PM
Unknown Object (File)
Mon, Nov 18, 7:35 PM
Unknown Object (File)
Mon, Nov 18, 7:35 PM
Subscribers

Details

Summary

The majority of the context is contained in the Linear task (https://linear.app/comm/issue/ENG-2924/use-the-internal-ip-package-in-the-dev-environment-to-ensure-physical). In short, the current shimUploadURI will detect what OS the user is on, and if it's iOS return localhost and if it's Android return 10.0.2.2. This works fine for the emulators, but makes testing uploads on physical devices difficult because we'd need to use a different method and get the accessing devices' IPv4 address to return in the upload url. By removing shimUploadURI and always using the IPv4 address (regardless of emulator or physical device), we keep the logic consistent between testing devices. Handling the logic to do this directly within getUploadURL allows all uploads when fetched to return to an address that is accessible across all dev environments.

Another note: I looked through D6266 prior to working on this, and while it looks like internal-ip version 7.0.0 worked fine within the keyserver, I chose to use version 4.3.0 in an effort to keep consistent. If we want to change to 7.0.0, there should only be four things that need to be done:

  1. Change the version in devDependencies
  2. Change the import from import ip from 'internal-ip to import { internalIpV4Sync } from 'internal-ip' to prevent SyntaxError: The requested module 'internal-ip' does not provide an export named 'default'
  3. Change the use of ip.v4.sync() to internalIpV4Sync() in getUploadURL.
  4. Run yarn dev in native and ensure that it still works (see @bartek's comment below)

Adding @ashoat as blocking since it involves a dependency

Depends on D6531

Test Plan

Ran yarn cleaninstall, restarted keyserver and tested across each of the clients:

  1. Web client - media is at the url: http://{IPv4}:3000/comm/upload/84155/1f243e6ee9d4d2b8
  2. iOS Emulator - media is at the url: http://{IPv4}:3000/comm/upload/84155/1f243e6ee9d4d2b8
  3. Android Emulator - media is at the url: http://{IPv4}:3000/comm/upload/84155/1f243e6ee9d4d2b8
  4. Physical iPhone build - media is at the url: http://{IPv4}:3000/comm/upload/84155/1f243e6ee9d4d2b8
  5. Physical Android build - (I don't have access to an Android device so I won't be able to do this one)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

lib/shared/messages/multimedia-message-spec.js
327

After removing shimUploadURI, a lot of the code / checks done in this file become redundant and there can be a lot of simplification. To make this diff easy to review, I've created a task to track actually cleaning this file: https://linear.app/comm/issue/ENG-2925/refactor-shimmediamessageinfo-in-multimedia-message-spec

rohan edited the summary of this revision. (Show Details)
rohan requested review of this revision.Feb 4 2023, 3:43 PM

Just a small note:

If we want to change to 7.0.0, there should only be three things that need to be done: [...]

  1. Run yarn dev in native and ensure that it still works (expo still uses the correct version of the dependency). It's likely that the "wrong" one might be hoisted.
keyserver/src/fetchers/upload-fetchers.js
87

nit, more a follow up task:
I'd make a single source of truth for the port and default fallback value and extract this to a separate helper function. Another usage is here. This way, if this ever changed, we'd need to edit only one place

  1. Run yarn dev in native and ensure that it still works (expo still uses the correct version of the dependency). It's likely that the "wrong" one might be hoisted.

Yeah that's a good idea, I'll add that into the summary

keyserver/src/fetchers/upload-fetchers.js
87
rohan edited the summary of this revision. (Show Details)
ashoat added inline comments.
keyserver/src/fetchers/upload-fetchers.js
90

I would construct ${basePath}upload/${id}/${secret} before the conditional to avoid copy-pasting it

lib/shared/messages/multimedia-message-spec.js
327

I would just clean it up in this diff personally. It seems like shimMediaMessageInfo can be deleted completely, no?

This revision is now accepted and ready to land.Feb 6 2023, 7:13 AM
keyserver/src/fetchers/upload-fetchers.js
90

Good point, I'll do this in the revision when taking care of shimMediaMessageInfo

lib/shared/messages/multimedia-message-spec.js
327

Yeah it can be removed entirely, it seems like it's purpose is now handled within getUploadURL

Remove shimMediaMessageInfo entirely. Ran git grep shimMediaMessageInfo to confirm that all uses of the function are modified and handled

lib/shared/messages/multimedia-message-spec.js
203 ↗(On Diff #22143)

Nit – I would remove this else block... the if has a return, so the else isn't necessary

204 ↗(On Diff #22143)

I think we can remove this TODO, right @atul?

lib/shared/messages/multimedia-message-spec.js
204 ↗(On Diff #22143)

Yes, sorry I should have remove that earlier.

lib/shared/messages/multimedia-message-spec.js
203 ↗(On Diff #22143)

Do you have a preference on merging the if statements or keeping the logic separate?

if (rawMessageInfo.type === messageTypes.IMAGES) {
  return rawMessageInfo;
}
if (hasMinCodeVersion(platformDetails, 158)) {
  return rawMessageInfo;
}

vs.

if (rawMessageInfo.type === messageTypes.IMAGES || hasMinCodeVersion(platformDetails, 158)) {
  return rawMessageInfo;
}
lib/shared/messages/multimedia-message-spec.js
203 ↗(On Diff #22143)

No strong preference. I would probably keep the former personally but I'm okay either way