workbench: Reader tokens show collection files.

Fundamentally, this commit aims to support reader tokens when showing
a file from a Collection.  Because this requires changing the token
handling generally, I also took this opportunity to improve error
reporting by checking the request's validity against the API server
before we pipe out to arv-get to render the file.

As a consequence, Workbench now returns a 404 if you request a file
using a token that does not have permission to read the collection.
Maybe this is not the best result, but previously it returned a 422,
so I think this counts as an improvement.

diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index eae7a77071dec213dfc28d01111bb4d68e724da5..a64ac11da32392a4b50ee3b99d3b9ddd9985e44b 100644 (file)
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -1,6 +1,7 @@
 class CollectionsController < ApplicationController
-  skip_before_filter :find_object_by_uuid, :only => [:provenance]
-  skip_before_filter :check_user_agreements, :only => [:show_file]
+  skip_around_filter :thread_with_mandatory_api_token, only: [:show_file]
+  skip_before_filter :find_object_by_uuid, only: [:provenance, :show_file]
+  skip_before_filter :check_user_agreements, only: [:show_file]
 
   def show_pane_list
     %w(Files Attributes Metadata Provenance_graph Used_by JSON API)
@@ -87,10 +88,21 @@ class CollectionsController < ApplicationController
   end
 
   def show_file
-    opts = params.merge(arvados_api_token: Thread.current[:arvados_api_token])
-    if r = params[:file].match(/(\.\w+)/)
-      ext = r[1]
+    # We pipe from arv-get to send the file to the user.  Before we start it,
+    # we ask the API server if the file actually exists.  This serves two
+    # purposes: it lets us return a useful status code for common errors, and
+    # helps us figure out which token to provide to arv-get.
+    coll = nil
+    usable_token = find_usable_token do
+      coll = Collection.find(params[:uuid])
     end
+    if usable_token.nil?
+      return  # Response already rendered.
+    elsif params[:file].nil? or not file_in_collection?(coll, params[:file])
+      return render_not_found
+    end
+    opts = params.merge(arvados_api_token: usable_token)
+    ext = File.extname(params[:file])
     self.response.headers['Content-Type'] =
       Rack::Mime::MIME_TYPES[ext] || 'application/octet-stream'
     self.response.headers['Content-Length'] = params[:size] if params[:size]
@@ -162,6 +174,53 @@ class CollectionsController < ApplicationController
 
   protected
 
+  def find_usable_token
+    # Iterate over every token available to make it the current token and
+    # yield the given block.
+    # If the block succeeds, return the token it used.
+    # Otherwise, render an error response based on the most specific
+    # error we encounter, and return nil.
+    read_tokens = [Thread.current[:arvados_api_token]].compact
+    if params[:reader_tokens].is_a? Array
+      read_tokens += params[:reader_tokens]
+    end
+    most_specific_error = [401]
+    read_tokens.each do |api_token|
+      using_specific_api_token(api_token) do
+        begin
+          yield
+          return api_token
+        rescue ArvadosApiClient::NotLoggedInException => error
+          status = 401
+        rescue => error
+          status = (error.message =~ /\[API: (\d+)\]$/) ? $1.to_i : nil
+          raise unless [401, 403, 404].include?(status)
+        end
+        if status >= most_specific_error.first
+          most_specific_error = [status, error]
+        end
+      end
+    end
+    case most_specific_error.shift
+    when 401, 403
+      redirect_to_login
+    when 404
+      render_not_found(*most_specific_error)
+    end
+    return nil
+  end
+
+  def file_in_collection?(collection, filename)
+    def normalized_path(part_list)
+      File.join(part_list).sub(%r{^\./}, '')
+    end
+    target = normalized_path([filename])
+    collection.files.each do |file_spec|
+      return true if (normalized_path(file_spec[0, 2]) == target)
+    end
+    false
+  end
+
   def file_enumerator(opts)
     FileStreamer.new opts
   end

diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index ea42b158eb21d4cd26ecb3db94953d44e5e6f49b..d1a8de226d2ab1bf96bd0221486fed24b4b9d263 100644 (file)
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -88,6 +88,47 @@ class CollectionsControllerTest < ActionController::TestCase
     params = collection_params(:foo_file, 'foo')
     sess = session_for(:spectator)
     get(:show_file, params, sess)
-    assert_includes([403, 422], @response.code.to_i)
+    assert_includes([403, 404], @response.code.to_i)
+  end
+
+  test "trying to get a nonexistent file from Keep returns a 404" do
+    params = collection_params(:foo_file, 'gone')
+    sess = session_for(:admin)
+    get(:show_file, params, sess)
+    assert_response 404
+  end
+
+  test "getting a file from Keep with a good reader token" do
+    params = collection_params(:foo_file, 'foo')
+    read_token = api_fixture('api_client_authorizations')['active']['api_token']
+    params[:reader_tokens] = [read_token]
+    get(:show_file, params)
+    assert_response :success
+    assert_equal(expected_contents(params, read_token), @response.body,
+                 "failed to get a correct file from Keep using a reader token")
+    assert_not_equal(read_token, session[:arvados_api_token],
+                     "using a reader token set the session's API token")
+  end
+
+  test "trying to get from Keep with an unscoped reader token prompts login" do
+    params = collection_params(:foo_file, 'foo')
+    read_token =
+      api_fixture('api_client_authorizations')['active_noscope']['api_token']
+    params[:reader_tokens] = [read_token]
+    get(:show_file, params)
+    assert_response :redirect
+  end
+
+  test "can get a file with an unpermissioned auth but in-scope reader token" do
+    params = collection_params(:foo_file, 'foo')
+    sess = session_for(:expired)
+    read_token = api_fixture('api_client_authorizations')['active']['api_token']
+    params[:reader_tokens] = [read_token]
+    get(:show_file, params, sess)
+    assert_response :success
+    assert_equal(expected_contents(params, read_token), @response.body,
+                 "failed to get a correct file from Keep using a reader token")
+    assert_not_equal(read_token, session[:arvados_api_token],
+                     "using a reader token set the session's API token")
   end
 end