5824: Fix disposition=attachment handling.

Propagate disposition=attachment from Workbench to keep-web when
redirecting.

Include a filename in the Content-Disposition header if the request
URL contains "?", so UAs don't mistakenly include the query string as
part of the default filename.

diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 38b58a16500bbdf5808b11ad77791426f3451042..cadef383dd265e2eafb4d6e868509f886c4e64d4 100644 (file)
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -148,6 +148,7 @@ class CollectionsController < ApplicationController
         # to read the collection.
         opts[:query_token] = usable_token
       end
+      opts[:disposition] = params[:disposition]
       return redirect_to keep_web_url(params[:uuid], params[:file], opts)
     end
 
@@ -332,9 +333,18 @@ class CollectionsController < ApplicationController
     end
     uri.path += '_/'
     uri.path += CGI::escape(file)
-    if opts[:query_token]
-      uri.query = 'api_token=' + CGI::escape(opts[:query_token])
+
+    query_params = []
+    { query_token: 'api_token',
+      disposition: 'disposition' }.each do |opt, param|
+      if opts[opt]
+        query_params << param + '=' + CGI::escape(opts[opt])
+      end
     end
+    unless query_params.empty?
+      uri.query = query_params.join '&'
+    end
+
     uri.to_s
   end
 

diff --git a/apps/workbench/config/application.default.yml b/apps/workbench/config/application.default.yml
index 5504fd29a726383544c78d45aa38266fe7458500..0a9ee9f11663f5f830b19c89dd5a4650c3c804ff 100644 (file)
--- a/apps/workbench/config/application.default.yml
+++ b/apps/workbench/config/application.default.yml
@@ -230,6 +230,13 @@ common:
   # download facility.
   #
   # Examples:
-  # keep_web_url: https://%{uuid_or_pdh}.dl.zzzzz.your.domain
-  # keep_web_url: https://%{uuid_or_pdh}--dl.zzzzz.your.domain
+  # keep_web_url: https://%{uuid_or_pdh}.collections.uuid_prefix.arvadosapi.com
+  # keep_web_url: https://%{uuid_or_pdh}--collections.uuid_prefix.arvadosapi.com
+  #
+  # Example supporting only public data and collection-sharing links:
+  # keep_web_url: https://collections.uuid_prefix.arvadosapi.com/c=%{uuid_or_pdh}
+  #
+  # Example supporting only download/attachment: (using keep-web
+  # -attachment-only-host collections.uuid_prefix.arvadosapi.com):
+  # keep_web_url: https://collections.uuid_prefix.arvadosapi.com/c=%{uuid_or_pdh}
   keep_web_url: false

diff --git a/apps/workbench/test/integration_helper.rb b/apps/workbench/test/integration_helper.rb
index 5750a1b0c20a4b5a5c8f62092db0eb6039ed9862..1cda1275e11724074d2fd39e1c273e7613b74bd4 100644 (file)
--- a/apps/workbench/test/integration_helper.rb
+++ b/apps/workbench/test/integration_helper.rb
@@ -27,6 +27,7 @@ Capybara.register_driver :selenium_with_download do |app|
   profile['browser.download.folderList'] = 2 # "save to user-defined location"
   profile['browser.download.manager.showWhenStarting'] = false
   profile['browser.helperApps.alwaysAsk.force'] = false
+  profile['browser.helperApps.neverAsk.saveToDisk'] = 'text/plain,application/octet-stream'
   Capybara::Selenium::Driver.new app, profile: profile
 end
 

diff --git a/services/keep-web/handler.go b/services/keep-web/handler.go
index 3e38cc35cad611b5944d5c68221f43989daf4bb5..e8678fa761bc2f6292ebbd92ec11555ccefe910b 100644 (file)
--- a/services/keep-web/handler.go
+++ b/services/keep-web/handler.go
@@ -9,6 +9,7 @@ import (
        "net/http"
        "net/url"
        "os"
+       "strconv"
        "strings"
 
        "git.curoverse.com/arvados.git/sdk/go/arvadosclient"
@@ -304,8 +305,24 @@ func (h *handler) ServeHTTP(wOrig http.ResponseWriter, r *http.Request) {
        if rdr, ok := rdr.(keepclient.ReadCloserWithLen); ok {
                w.Header().Set("Content-Length", fmt.Sprintf("%d", rdr.Len()))
        }
+
+       disposition := "inline"
        if attachment {
-               w.Header().Set("Content-Disposition", "attachment")
+               disposition = "attachment"
+       }
+       if strings.ContainsRune(r.RequestURI, '?') {
+               // Help the UA realize that the filename is just
+               // "filename.txt", not
+               // "filename.txt?disposition=attachment".
+               //
+               // TODO(TC): Follow advice at RFC 6266 appendix D
+               if basenamePos < 0 {
+                       basenamePos = 0
+               }
+               disposition += "; filename=" + strconv.QuoteToASCII(filename[basenamePos:])
+       }
+       if disposition != "inline" {
+               w.Header().Set("Content-Disposition", disposition)
        }
 
        w.WriteHeader(http.StatusOK)