[ARVADOS] created: 7ba3707abe8370a7ba7dd373be1fd7e2dbebedd7

[git at public.curoverse.com]

        at  7ba3707abe8370a7ba7dd373be1fd7e2dbebedd7 (commit)


commit 7ba3707abe8370a7ba7dd373be1fd7e2dbebedd7
Author: Brett Smith <brett at curoverse.com>
Date:   Thu Nov 6 11:59:00 2014 -0500

    4408: Workbench Collection sharing allows file downloads again.
    
    Earlier we refactored our API token loading code to load User.current
    to check that it was valid.  This approach doesn't work when we're
    presenting shared Collections, because the token usually won't be
    scoped to get user information.  Skip the check for this specific
    case.

diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 27e388b..0737c31 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -399,12 +399,17 @@ class ApplicationController < ActionController::Base
     false  # For convenience to return from callbacks
   end
 
-  def using_specific_api_token(api_token)
+  def using_specific_api_token(api_token, opts={})
     start_values = {}
     [:arvados_api_token, :user].each do |key|
       start_values[key] = Thread.current[key]
     end
-    load_api_token(api_token)
+    if opts.fetch(:load_user, true)
+      load_api_token(api_token)
+    else
+      Thread.current[:arvados_api_token] = api_token
+      Thread.current[:user] = nil
+    end
     begin
       yield
     ensure
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index e869824..5ddf93c 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -1,3 +1,5 @@
+require "arvados/keep"
+
 class CollectionsController < ApplicationController
   include ActionController::Live
 
@@ -298,7 +300,9 @@ class CollectionsController < ApplicationController
     most_specific_error = [401]
     token_list.each do |api_token|
       begin
-        using_specific_api_token(api_token) do
+        # We can't load the corresponding user, because the token may not
+        # be scoped for that.
+        using_specific_api_token(api_token, load_user: false) do
           yield
           return api_token
         end
diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index e621010..6c64ac9 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -86,14 +86,26 @@ class CollectionsControllerTest < ActionController::TestCase
 
   test "viewing collection files with a reader token" do
     params = collection_params(:foo_file)
-    params[:reader_token] =
-      api_fixture('api_client_authorizations')['active']['api_token']
+    params[:reader_token] = api_fixture("api_client_authorizations",
+                                        "active_all_collections", "api_token")
     get(:show_file_links, params)
     assert_response :success
     assert_equal([['.', 'foo', 3]], assigns(:object).files)
     assert_no_session
   end
 
+  test "fetching collection file with reader token" do
+    expected = stub_file_content
+    params = collection_params(:foo_file, "foo")
+    params[:reader_token] = api_fixture("api_client_authorizations",
+                                        "active_all_collections", "api_token")
+    get(:show_file, params)
+    assert_response :success
+    assert_equal(expected, @response.body,
+                 "failed to fetch a Collection file with a reader token")
+    assert_no_session
+  end
+
   test "reader token Collection links end with trailing slash" do
     # Testing the fix for #2937.
     session = session_for(:active_trustedclient)
diff --git a/apps/workbench/test/integration/collections_test.rb b/apps/workbench/test/integration/collections_test.rb
index 3abbf6f..e2c5d84 100644
--- a/apps/workbench/test/integration/collections_test.rb
+++ b/apps/workbench/test/integration/collections_test.rb
@@ -35,6 +35,8 @@ class CollectionsTest < ActionDispatch::IntegrationTest
   end
 
   test "can download an entire collection with a reader token" do
+    CollectionsController.any_instance.
+      stubs(:file_enumerator).returns(["foo\n", "file\n"])
     uuid = api_fixture('collections')['foo_file']['uuid']
     token = api_fixture('api_client_authorizations')['active_all_collections']['api_token']
     url_head = "/collections/download/#{uuid}/#{token}/"
@@ -43,9 +45,8 @@ class CollectionsTest < ActionDispatch::IntegrationTest
     # a very blunt approach.
     assert_no_match(/<\s*meta[^>]+\bnofollow\b/i, page.html,
                     "wget prohibited from recursing the collection page")
-    # TODO: When we can test against a Keep server, actually follow links
-    # and check their contents, rather than testing the href directly
-    # (this is too closely tied to implementation details).
+    # Look at all the links that wget would recurse through using our
+    # recommended options, and check that it's exactly the file list.
     hrefs = page.all('a').map do |anchor|
       link = anchor[:href] || ''
       if link.start_with? url_head
@@ -58,6 +59,10 @@ class CollectionsTest < ActionDispatch::IntegrationTest
     end
     assert_equal(['foo'], hrefs.compact.sort,
                  "download page did provide strictly file links")
+    within "#collection_files" do
+      click_link "foo"
+      assert_equal("foo\nfile\n", page.html)
+    end
   end
 
   test "can view empty collection" do

-----------------------------------------------------------------------


hooks/post-receive
--