View uploaded files without needing to be logged in to the realm in the browser. #4089

agrawal-d · 2020-05-06T18:16:22Z

Closes #3303.

agrawal-d · 2020-05-06T18:40:05Z

@chrisbobbe @gnprice this PR is ready for a review.

chrisbobbe · 2020-05-06T18:42:01Z

@chrisbobbe @gnprice this PR is ready for a review.

Great, glad to have this so quickly! Looking now.

agrawal-d · 2020-05-06T18:53:29Z

@chrisbobbe I force pushed some non-trivial changes after you commented.

chrisbobbe · 2020-05-06T18:55:08Z

@chrisbobbe I force pushed some non-trivial changes after you commented.

Yep, thanks! I saw the update and I'm looking at the series up to 9c0b4bc now.

src/api/messages/getTemporaryFileUrl.js

agrawal-d · 2020-05-06T19:38:08Z

@chrisbobbe there were some errors in the code. I've pushed some changes. Apologies for the inconvenience. I've thoroughly tested now.

chrisbobbe · 2020-05-06T20:09:39Z

@chrisbobbe there were some errors in the code. I've pushed some changes. Apologies for the inconvenience. I've thoroughly tested now.

No problem! 🙂 I actually hadn't gotten very deep in my review when I took a break to eat lunch; back now.

chrisbobbe

Thanks, @agrawal-d! To me, this looks like it's on the right track.

There were quite a few details involved in the server-side implementation discussion (how to handle local vs. s3 backends, etc.), so I imagined this taking longer to figure out. But I think something like this is where we ended up, following @gnprice's comment at #3303 (comment).

A few comments below; I'm sure @gnprice will have more to add.

src/lightbox/LightboxActionSheet.js

src/utils/file.js

src/message/messagesActions.js

src/api/messages/getTemporaryFileUrl.js

src/api/index.js

At time of writing, this handles all embedded data in all routes in all API calls used by the mobile app. However, that won't be true forever. In particular, PR zulip#4089 introduces code for a new endpoint `getTemporaryFileUri` which contains an arbitrary filename. This endpoint will have to be handled differently -- either by calling `apiCall` directly, or by adding a bit more plumbing through `apiGet` and friends.

agrawal-d · 2020-05-07T08:42:23Z

@chrisbobbe Thanks for the review!

Also, if the browser decides it can't display it, it triggers what seems like the platform's preferred way to download a file.

You are right! Even on Android, if the browser can't handle a file, but some other app can, a dialog is shown to choose the app to open it with, apart from the browser.

For example -

For video files, I get a choice to open in VLC media player, the system video app, or in Firefox.
For music files, I get a choice to open in a music app, in VLC, or in Firefox.
For pdf files, I get a choice to open in GDrive PDF Viewer, or in Firefox.

So, I think it's best to let the browser handle all files, for an overall better experience. I've changed the PR to reflect this.

gnprice

Thanks! One quick comment below, but I'll leave Chris to continue to do the full review.

So, I think it's best to let the browser handle all files, for an overall better experience. I've changed the PR to reflect this.

Before seeing this I replied on a related inline comment thread:
#4089 (comment)
but in short that SGTM.

gnprice · 2020-05-08T00:06:58Z

src/message/messagesActions.js

+  // We don't want to generate temporary URLs to external websites.
+  if (!url.includes(auth.realm)) {
+    openLink(url);
+    return;
+  }
+
+  let tempUrl: string;
+  try {
+    tempUrl = getFullUrl((await getTemporaryFileUrl(auth, href)).url, auth.realm);
+  } catch (err) {
+    // If we failed to generate the temporary URL, we assume it's because the
+    // server doesn't suport it yet, and fallback to use the regular file
+    // URL.
+    openLink(url);
+    return;
+  }
+  openLink(tempUrl);


This logic is kind of tricky, especially around the try/catch.

A couple of things that I think would help:

Every control-flow path out of this code ends up calling openLink at the end. Make that explicit by having just one openLink call. Have a helper function that returns the URL we'll pass to openLink. That function will contain some control flow.

This also makes an opportunity to give that helper function a meaningful name, which I think will also add clarity.

Have the try block be as small as possible. As is, we'd end up in the catch if there's somehow an exception thrown from getFullUrl. That shouldn't happen, but ruling that out requires going and reading more code. Instead that call can be after the whole try/catch.

chrisbobbe

Thanks, @agrawal-d! A few comments, and I agree with Greg's, as well. This revision feels more streamlined, and closer to a pure fix for the annoying need-to-login issue; lots of people will be quite happy to see it working soon. 🙂

chrisbobbe · 2020-05-08T01:12:20Z

src/api/messages/getTemporaryFileUrl.js

+
+/**
+ * This endpoint, introduced in zulip/zulip@9e72fbe3f, returns a partial URL
+ * string without the realm domain included in it. This URL allows temporarily


Good to have these details in a jsdoc!

I'm struggling to find an example in our API bindings, but Greg guided me toward some conventions we use, when I was writing src/utils/zuilpVersion.js (you could take a look at that file):

Start with a terse single-line summary of what the thing is/does.

Then some summary, which gives relevant background, or places you might want to use the thing, or reasons to use it instead of something else, etc.

Good to include the details in * @param filePath, I think. Not sure what to do about auth; it's boring, since all the other bindings have it too. Maybe just an empty @param auth, with no explanation? Order matters, so I think there's an argument for putting something minimal before @param filePath.

I'm not sure (and it doesn't seem to be documented in the zulip.yaml file you link to) whether the backend implementation ended up giving us a short-lived URL that was

just completely random, or

based on a combination of the user (even their private API key?) and the requested document, with, I guess,
2a. some randomness on top of that, or
2b. completely determined by this info combined with a timestamp?

Possibly, the answer to this won't ever be important for the mobile app to be aware of, from a security perspective or otherwise, or these are considered implementation details by the server, so could change at any time. Assuming one of these is true, it seems appropriate for us to be intentionally vague about those details. The important piece is that the URL won't be valid after, I think, 60 seconds or so (edit: and a few other things, noted in my next comment).

I think there's even more detail about why we're using this endpoint, which would be good to include. See Greg's points here:

The goal basically is to get

(a) a plain old URL that can be passed to a browser (not a URL plus some cookies or other headers that need to be set)

(b) that gets the user the file immediately, without requiring a fresh login

(c) but doesn't include a secret with much wider powers (i.e. their API key), which would risk a nasty surprise if they innocently show someone else the file by sending them the link.

I think (a) is explained by "without sending auth headers", but let's get (b) and (c) in there too.

I'm not sure (and it doesn't seem to be documented in the zulip.yaml file you link to) whether the backend implementation ended up giving us a short-lived URL that was

just completely random, or

based on a combination of the user (even their private API key?) and the requested document, with, I guess,
2a. some randomness on top of that, or
2b. completely determined by this info combined with a timestamp?

I believe the implementation ended up using Django's django.core.signing, either its dumps function or the underlying TimestampSigner. There's docs here:
https://docs.djangoproject.com/en/3.0/topics/signing/
and the cryptographic concept it implements is an "HMAC":
https://en.wikipedia.org/wiki/HMAC

Basically we take the IDs of the user and the requested document, make a sentence saying "As of TIME, user USER can access file FILE", and then compute a signature. The URL contains that sentence and signature.

The signature is done with a secret key belonging to the server, and can be verified with the same secret key by just re-computing what the signature should be.

The HMAC is a common technique for webapps to hand the client some piece of data, especially this kind of access token, in what amounts to a transparent but tamper-proof envelope so the client can hand it back to the server later and the server can trust the contents. Just as it did for us here, this technique saves the server from having to store the tokens itself.

Assuming one of these is true, it seems appropriate for us to be intentionally vague about those details.

Yeah, I think those are implementation details the client needn't care about, and the other points you mentioned are more relevant.

chrisbobbe · 2020-05-08T01:29:11Z

src/api/messages/getTemporaryFileUrl.js

+|};
+
+/**
+ * This endpoint, introduced in zulip/zulip@9e72fbe3f, returns a partial URL


introduced in zulip/zulip@9e72fbe3f

Eventually, we'll move to including a value of zulip_feature_level (doc) here, and we'll even be able to use that for feature detection in using this feature, as Greg notes at #3303 (comment), following #4049, but I think this is fine for now; a commit ID is good because it's as specific as it's possible to be.

At time of writing, this handles all embedded data in all routes in all API calls used by the mobile app. However, that won't be true forever. In particular, PR zulip#4089 introduces code for a new endpoint `getTemporaryFileUri` which contains an arbitrary filename. This endpoint will have to be handled differently -- either by calling `apiCall` directly, or by adding a bit more plumbing through `apiGet` and friends.

gnprice

Thanks @agrawal-d ! Code LGTM now, with just a couple of small comments below.

I still need to try it out end-to-end -- or @chrisbobbe, if you've done so then also feel free to merge.

src/lightbox/downloadImage.js

src/api/messages/getTemporaryFileUrl.js

gnprice · 2020-06-04T20:58:13Z

OK, tried this end-to-end, in an emulator running Android 8.

Tried downloading an image, for Error: Download manager failed to download from (a user_uploads URL) #2115. Steps: Message list > tap an image to get to lightbox > overflow menu > Download image.

Result: got this error message:

That is a different message from the status quo! So something changed.

And, notably, it seems like the download actually worked. There was a notification from Download Manager:

And opening that notification, I get to choose between HTML Viewer and Chrome; and either way, it successfully shows the image. Here's Chrome:

Also the download shows up in my Downloads folder, in the Files app:

(there's two of them there because I tried the download twice.)

Anyway, that's totally progress, so worth merging, but there are still some things to fix. 🙂
Viewing uploaded files fails, or requires logging in again #3303 coming up...

gnprice · 2020-06-04T21:00:42Z

(Oh, and I was curious where that error message comes from, with its garbled syntax:

Download manager download failed, the file does not downloaded to destination.

Found it with grep -r 'downloaded to d' -- it's in the file node_modules/rn-fetch-blob/android/src/main/java/com/RNFetchBlob/RNFetchBlobReq.java, so from rn-fetch-blob.)

gnprice · 2020-06-04T21:09:40Z

Again in an emulator running Android 8, testing this end to end:

Viewing uploaded files fails, or requires logging in again #3303. Went to message list, found a message with an upload that wasn't an image. (To do that: in the webapp searched for has:attachment, and scrolled through history to find one that wasn't an image.) Specifically the file happened to be a PDF, with a .pdf extension on its name.

Hit the link. Got a prompt (a bottom sheet) asking if I wanted to use Chrome or WebView Browser Tester; I chose Chrome.

That opened a new screen with a browser view. That in turn gave me a permission prompt, for Chrome -- asking for file/storage permissions for Chrome. I approved.

Then a toast "Downloading..." appeared, and the browser view closed and I got the message list again. The download succeeded; I got a Chrome notification for it. Tapping the notification, the file opened up successfully in a PDF viewer.

So, I think that is a success!

The UX seems a bit suboptimal, in that I'd rather go straight to showing the PDF rather than have a few more taps and have to go through a notification. But I'm pretty used to seeing that flow, as an Android user, so I'm not sure there's an easy way for us to cause something nicer to happen.

gnprice · 2020-06-04T22:35:48Z

OK, and this time on iOS, in a simulated iPhone 8 running iOS 13.3.

Viewing uploaded files fails, or requires logging in again #3303. Same steps as on Android, up to tapping on the link in the message list.

Got a browser view. But this time, after a few seconds of loading, the result was an error:

That appears to be from S3 directly, because that's in the location bar at the top. The error message begins:

SignatureDoesNotMatch The request signature we calculated does not match the signature you provided. Check your key and signing method. [... then a bunch of details ...]

Ditto on a second try. I watched the timing closely this time, and it was only about a second between me tapping the link and the error message appearing. So it's not an expiration issue -- there's something else wrong.

Still worth merging because it fixes the issue for Android and this is no worse than the status quo on iOS -- but there remains something to fix and I think it doesn't resolve the issue Viewing uploaded files fails, or requires logging in again #3303.

gnprice · 2020-06-04T22:42:02Z

Same iOS environment, now testing #2115.

Error: Download manager failed to download from (a user_uploads URL) #2115. Same initial steps, up to hitting "Download image" in the overflow menu.

The permissions prompt has a confusing message, in this context:

We should fix that, as a followup.

Then the first time, the download failed. I think I spent over a minute at the permissions prompt, mostly because I was describing it here. Can we request permission before we get the temporary URL, rather than after? That'd avoid this issue.

Then the second time, it succeeded! At least, the toast said it did.

Going to the Files app, it's not there -- in particular it's not in the "Downloads" folder.

But going to the Photos app, it's there. Cool. I'm not sure which place I would expect it to land in, but either way, having it work at all is a major improvement 🙂

This JSDoc adds useful information about what it does, and it's limitations.

The endpoint GET `/user_uploads/{realm_id_str}/{filename}` allows for the generation of a temporary file URL that does not require authentication. This endpoint was added in zulip/zulip@9e72fbe3f. The goal basically is to get: (a) a plain old URL that can be passed to a browser (not a URL plus some cookies or other headers that need to be set). (b) that gets the user the file immediately, without requiring a fresh login. (c) but doesn't include a secret with much wider powers (i.e. their API key), which would risk a nasty surprise if they innocently show someone else the file by sending them the link. Commit-message-suggested-by: Greg Price <greg@zulipchat.com>

Using the new endpoint `/user_uploads/{realm_id_str}/{filename}` it generates a temporary URL that does not require auth to access. This allows the user to open the file in the browser without being logged in to the realm, or to directly download it without redirects to S3 or needing to send auth headers. If generation of the url fails, it returns `null`.

gnprice · 2020-06-05T00:52:32Z

Aha, and I may have spotted what's going wrong in the #3033 case on iOS!

From that error page, I hit the "share" icon and chose "Copy", to get the URL onto the clipboard. Then went and pasted it elsewhere (the compose box, as the handiest place.) Here it is:
https://zulip-uploads.s3.amazonaws.com/1230/-Opc2L055IYelpraPb1oRDeU/sagas.pdf?Signature=2mpNGb0ysKFpVN8bkkMVsulQSVE%253D&Expires=1591317724&AWSAccessKeyId=AKIAIEVMBCAT2WD3M5KQ

I went and pulled up the same upload in the webapp, to compare. (It works fine there.) Here's the URL I find in the location bar there:
https://zulip-uploads.s3.amazonaws.com/1230/-Opc2L055IYelpraPb1oRDeU/sagas.pdf?Signature=xipp%2FD69nk89xmkKha3cx6K%2FSSg%3D&Expires=1591317779&AWSAccessKeyId=AKIAIEVMBCAT2WD3M5KQ

I think the problem is that %253D. That's the percent-encoding of %3D, which is itself the percent-encoding of =. Note there's a %3D at the end of the Signature query-parameter in the successful URL. Both signatures look like base64, which very often ends with a = as padding.

So it seems like we're double-encoding the URL, and as a result the decoding of it has a signature ending in %3D instead of in = and the signature doesn't validate.

Anyway, about to merge, and then I'll file issues for the various followups.

Using the new helper tryGetFileDownloadURL, we now try to generate and use temporary authless URLs for realm uploaded files. We open files using the temporary URL in the browser, as opposed to downloading them directly, because 1. The browser can usually open several file types instantly and 2. If the browser can not handle displaying the file, a system dialog shows up, both on Android and iOS, showing platform specific ways to handle the file, such as opening in Google Drive, Saving to iCloud and other options based on installed apps, which is convenient for the user. [greg: On iOS this doesn't actually work yet, but the bug it runs into is unrelated to zulip#3303.] Fixes: zulip#3303

Using the helper `tryGetFileDownloadUrl`, we try to generate and use temporary authless URLs to download files from the lighbox. This fixes an issue where redirects to S3 caused RNFetchBlob to fail downloading the file. If we fail to download the file, which happens if we failed to generate the temporary URL because the server does not support it, we open it in the browser instead, from where the user can easily save it. Also makes changes in other places required due to this change. Fixes: zulip#2115

gnprice · 2020-06-05T01:55:32Z

Then the first time, the download failed. I think I spent over a minute at the permissions prompt, mostly because I was describing it here. Can we request permission before we get the temporary URL, rather than after?

I think the short answer to this question may be "no", sadly -- the API we're using, and similarly its latest version, doesn't have a way to request the permission before committing to a specific URL to try to save. That I think is in turn probably ultimately a consequence of Apple's (reasonable!) desire to push apps to show permissions prompts only right at the moment where there's a clear thing the app wants to use the permission to do.

Ah well. These temporary URLs last 60 seconds, and it probably won't be common for a user to hesitate at a permission prompt for that long, or close to it. And if they do, it's a one-time occurrence anyway.

gnprice · 2020-06-05T02:08:37Z

OK, and done filing those followups! See #4136, #4137, #4138, #4139.

agrawal-d changed the title ~~View files~~ Viewing uploaded files without needing to be logged in to the realm in the browser. May 6, 2020

agrawal-d force-pushed the view-files branch from 2495f70 to 657635e Compare May 6, 2020 18:33

agrawal-d requested review from gnprice and chrisbobbe May 6, 2020 18:39

agrawal-d force-pushed the view-files branch from 657635e to 9c0b4bc Compare May 6, 2020 18:46

agrawal-d changed the title ~~Viewing uploaded files without needing to be logged in to the realm in the browser.~~ View uploaded files without needing to be logged in to the realm in the browser. May 6, 2020

agrawal-d commented May 6, 2020

View reviewed changes

src/api/messages/getTemporaryFileUrl.js Outdated Show resolved Hide resolved

agrawal-d force-pushed the view-files branch 2 times, most recently from 1090b87 to 33c17e8 Compare May 6, 2020 19:37

chrisbobbe requested changes May 7, 2020

View reviewed changes

agrawal-d requested a review from chrisbobbe May 7, 2020 08:42

agrawal-d force-pushed the view-files branch from 33c17e8 to 52a41cf Compare May 7, 2020 08:45

gnprice reviewed May 8, 2020

View reviewed changes

chrisbobbe requested changes May 8, 2020

View reviewed changes

rk-for-zulip mentioned this pull request May 8, 2020

[NEEDS iOS TESTING] API errors in Sentry, v2 #4096

Open

agrawal-d force-pushed the view-files branch from 52a41cf to 66f4e28 Compare May 11, 2020 05:13

agrawal-d requested a review from gnprice May 11, 2020 05:13

agrawal-d force-pushed the view-files branch 2 times, most recently from dd72ae2 to 5ae7d53 Compare June 3, 2020 13:00

gnprice reviewed Jun 3, 2020

View reviewed changes

src/lightbox/downloadImage.js Outdated Show resolved Hide resolved

src/api/messages/getTemporaryFileUrl.js Outdated Show resolved Hide resolved

agrawal-d force-pushed the view-files branch 2 times, most recently from 991841d to 8c311d3 Compare June 4, 2020 07:44

agrawal-d added 3 commits June 4, 2020 17:36

url [nfc]: Add JSDoc for getFullUrl.

9025ac1

This JSDoc adds useful information about what it does, and it's limitations.

agrawal-d added 2 commits June 4, 2020 17:52

gnprice force-pushed the view-files branch from 8c311d3 to ed645fe Compare June 5, 2020 00:55

gnprice merged commit ed645fe into zulip:master Jun 5, 2020

gnprice mentioned this pull request Jun 5, 2020

[iOS] Review and reword permission usage text #4139

Closed

agrawal-d deleted the view-files branch June 5, 2020 04:35

chrisbobbe mentioned this pull request Apr 1, 2021

lightbox: Refactor sharing module in android which is used in lightbox. #4554

Merged

gnprice mentioned this pull request Jun 18, 2022

ios: On save from lightbox, request only the needed permission #5420

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

View uploaded files without needing to be logged in to the realm in the browser. #4089

View uploaded files without needing to be logged in to the realm in the browser. #4089

agrawal-d commented May 6, 2020 •

edited

Loading

agrawal-d commented May 6, 2020

chrisbobbe commented May 6, 2020

agrawal-d commented May 6, 2020

chrisbobbe commented May 6, 2020

agrawal-d commented May 6, 2020

chrisbobbe commented May 6, 2020

chrisbobbe left a comment

agrawal-d commented May 7, 2020

gnprice left a comment •

edited

Loading

gnprice May 8, 2020

chrisbobbe left a comment •

edited

Loading

chrisbobbe May 8, 2020

chrisbobbe May 8, 2020 •

edited

Loading

chrisbobbe May 8, 2020

gnprice May 13, 2020

chrisbobbe May 14, 2020

chrisbobbe May 8, 2020 •

edited

Loading

gnprice left a comment

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 5, 2020

gnprice commented Jun 5, 2020

gnprice commented Jun 5, 2020

View uploaded files without needing to be logged in to the realm in the browser. #4089

View uploaded files without needing to be logged in to the realm in the browser. #4089

Conversation

agrawal-d commented May 6, 2020 • edited Loading

agrawal-d commented May 6, 2020

chrisbobbe commented May 6, 2020

agrawal-d commented May 6, 2020

chrisbobbe commented May 6, 2020

agrawal-d commented May 6, 2020

chrisbobbe commented May 6, 2020

chrisbobbe left a comment

Choose a reason for hiding this comment

agrawal-d commented May 7, 2020

gnprice left a comment • edited Loading

Choose a reason for hiding this comment

gnprice May 8, 2020

Choose a reason for hiding this comment

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe May 8, 2020

Choose a reason for hiding this comment

chrisbobbe May 8, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe May 8, 2020

Choose a reason for hiding this comment

gnprice May 13, 2020

Choose a reason for hiding this comment

chrisbobbe May 14, 2020

Choose a reason for hiding this comment

chrisbobbe May 8, 2020 • edited Loading

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 4, 2020

gnprice commented Jun 5, 2020

gnprice commented Jun 5, 2020

gnprice commented Jun 5, 2020

agrawal-d commented May 6, 2020 •

edited

Loading

gnprice left a comment •

edited

Loading

chrisbobbe left a comment •

edited

Loading

chrisbobbe May 8, 2020 •

edited

Loading

chrisbobbe May 8, 2020 •

edited

Loading