Arvados

New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/055723f58e163f0b5e49c1e8b92fd1bebe81873e
Test run: developer-tests-workbench2: #99

Added new way of downloading files

New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/7b26514e0a1cbe9700d944097b80ec2f46b3d95d
Test run: developer-tests-workbench2: #114

fixed types

The "Download" and "Open in new tab" menu items need to use different links. Neither link should use the /t=.../ form for tokens.

The "Download" menu item should have "download" anchor attribute set, and open a download dialog in the browser. The download link should use the base URL from Services.WebDAVDownload.ExternalURL.

The "Open in new tab" menu item should not have the "download" anchor attribute. The link should use the base URL from Services.WebDAV.ExternalURL.

The "Copy to clipboard" should not use an open redirect. The redirect could be used to easily steal someone's token. The link should be something like:

https://workbench2.example.com/collections/x2b8c-4zz18-9i4shmy31jxxfbe/download/hello.txt

Visiting this link should redirect the user to the file using the same link as "Open in new tab (but opening in the current tab, not a new one).

If the user arrives at Workbench2 without a token, the path should be stored in localstorage, and then restored after successful login.

New version: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/9525ed95bef2a8de63b48a0682c342465d29bae9
Test run: developer-tests-workbench2: #126

Removed download attribute, fixed redirectTo

So I started writing comments but I realized I didn't know what to tell you so I just went and started hacking on it. I discovered some other infrastructure issues in the process (you can see the conversation in the development gitter).

Take a look at these changes. I didn't run tests so I don't know if anything else needs to be updated.

16812-token-appears-in-the-download-URL @ arvados-workbench2|ae94f4d8463ff6350329e802cb902c8dad96a710

16812: Handoff token using query param

Need to pass the token to keep-web without it being "sticky" in the
URL bar. Using a query param accomplishes this, because keep-web
knows to strip the api_token query parameter and respond with redirect
and a cookie which the browser can use to fetch the file safely.

Also distinguish between KeepWebService (now the download service) and
KeepWebInlineService (the one that will serve content that can be
displayed inline in the browser if it is safe to do so).

Here's another way to try to pass the token without exposing it in the URL:

<form action="https://keep_web_url/c=.../file.txt" method="get" target="_blank">
  <input type="hidden" id="api_token" name="api_token" value="xyz" />
  <input type="submit" class="link-button">Open in new tab</input>
</form>

New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/ffc9666a04b0cef18a15571c1a0f4fdc41e87b75
Test run: developer-tests-workbench2: #128

Fixed tests and redirect

Let me try to clarify a few things.

The API that we are talking to is the WebDAV API, which is documented here:

https://doc.arvados.org/v2.1/api/keep-webdav.html

This documentation page is new (added on Friday) so I couldn't give it to you earlier.

There are two endpoints. The "Services.WebDAV" endpoint (keepWebInlineServiceUrl) and "Services.WebDAVDownload" (keepWebServiceUrl) endpoint.

The "inline" endpoint may respond such that a browser can render the file inline.

The "download" endpoint responds so that the browser will always invokes the download behavior.

When using "open in new tab" we want the "inline" endpoint and using "download" we want the "download" endpoint.

When sending the user to the download endpoint, the ?disposition=attachement is redundant. (That parameter does change the behavior if you send the user to the "inline" endpoint).

Finally, although the way the react context menu behaves makes it difficult or impossible to right-click copy links, the link (with the token) still shows up in the browser. Instead of using an anchor tag, it should be possible to use a form to provide the token as a hidden parameter, as in my example in #note-15. Then the token won't be visible to the user.

Also, I noticed in src/services/collection-service.ts:extendFileURL it is splitting the API token and only taking the secret portion. This is wrong. It should be using the complete token, but it should be URL-encoded.

I know we have already gone back and forth on this a couple of times, I don't want you to get frustrated so if you would prefer I can also take over the branch.

New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/67d77b206ebee7c776bd4d7dbc112ebb0c7ba800
Test run: developer-tests-workbench2: #129

Code cleanup

I think you committed "open-in-new-tab.actions.ts" by accident.

Otherwise LGTM.

Make sure the image link is using the "inline" webdav URL, not the "download" one.

New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/55c77cf6945551bbb2500c213be48db84315b445
Test run: developer-tests-workbench2: #215

Fixed img preview for collection items

Reviewing arvados-workbench2|55c77cf - branch 16812-images-issue-fix

• The test suite is named after a different component
• Instead of “test.com” as a fake domain please use “example.com” as we do in other tests.
• Testing this fix on arvbox doesn’t work, the request fails with status 401 (unauthorized)
  • Example URL on “master” branch: https://10.1.1.7:9004/c=xupkx-4zz18-l4r1mbaajnbu1qz/t=v2/xupkx-gj3su-ru0tjgov29ijmyy/5otoq9c65yl6uef63ojpsd35bgj2gvmgzsmjowyz1ii85qz4ic/Sculptor-150sec-2.jpg (fails with 404).
  • Same URL on this branch: https://10.1.1.7:9004/c=xupkx-4zz18-l4r1mbaajnbu1qz/Sculptor-150sec-2.jpg (fails with 401 — token is missing).
• What I think is happening here is that the v2 token needs to be url-encoded, so its ‘/‘ chars don’t get interpreted as path separators.

New version first commit: https://dev.arvados.org/projects/arvados/repository/arvados-workbench2/revisions/a1abf5536a6d65f6ede59748cfbb9afceb249a83
Test run: developer-tests-workbench2: #216

Added token as query param

• Could you please replace the “test.com” domain usage with “example.com” on the tests? Example.com is meant to be used on tests/examples (https://tools.ietf.org/html/rfc2606#section-3), and “test.com” is a real existing domain.
• Continuing with the tests: I think for readability purposes the mocked file URL should have more correct (format wise) data. Example:
  • Instead of c=test-hash for the collection’s uuid, it should use c=zzzzz-4zz18-0123456789abcde (https://doc.arvados.org/v2.1/api/methods/collections.html)
  • Instead of t=test-token/test-token2/test-token3 for the token, it should use something like v2/zzzzz-gj3su-0123456789abcde/xxxxxxtokenxxxxx
  • The tests will work the same, but for code maintainability I think is convenient.
• I tried it and it worked ok on Firefox but not Chrome nor Safari.
  • I think Chrome is behaving more strictly that others and recently enabled a cross site protection for set-cookies headers. (https://blog.chromium.org/2019/10/developers-get-ready-for-new.html)
  • Couldn’t confirm if Safari has the same issue, but it’s probably not that important
  • On the other hand, if I test the commit just before the one that broke this feature, it works fine (tested it against ce8i5) on Chrome too. This is the last commit before the preview stopped working: 970a9e9dcd2a444d02181c4df3f205f7e0a8ebeb — it was using the v1 token on the file path. I think this got caught by the original fix on this story, to avoid sharing URLs with embedded tokens, but the URL used for the preview doesn’t get shared so we may want to keep using it in the url's path.
• I manually tried to get the image passing the /t=v2%Fce8i5-gj3su-0123456789abcde%2Fxxxxxxtokenxxxxx (with slashes encoded as %2F) path part on the URL and it didn’t work, we should confirm with Peter or Tom if keep-web supports it.

As a reference, it seems that the SameSite=Lax attribute will be eventually enabled by all browsers: https://web.dev/samesite-cookie-recipes/

Tom fixed keep-web so that it adds SameSite=None; Secure attributes to the set-cookie header. With that, I can confirm that Chrome is happy.

So, after making the test cosmetic improvements, it will LGTM.