2765: Tear out general Workbench reader_tokens support.

Refs #2765.  This approach is not tenable in its current form, and the
callbacks are making further development confusing, so I'm getting rid
of them.

Conflicts:
	apps/workbench/test/functional/collections_controller_test.rb

diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index b62838f655e23a54033d28298c265849c32f2398..ade586c47422b88d848612a9adf4a19330eae1bd 100644 (file)
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -7,12 +7,10 @@ class ApplicationController < ActionController::Base
   ERROR_ACTIONS = [:render_error, :render_not_found]
 
   around_filter :thread_clear
-  around_filter(:thread_with_mandatory_api_token,
-                except: [:index, :show] + ERROR_ACTIONS)
+  around_filter :thread_with_mandatory_api_token, except: ERROR_ACTIONS
   around_filter :thread_with_optional_api_token
   before_filter :check_user_agreements, except: ERROR_ACTIONS
   before_filter :check_user_notifications, except: ERROR_ACTIONS
-  around_filter :using_reader_tokens, only: [:index, :show]
   before_filter :find_object_by_uuid, except: [:index] + ERROR_ACTIONS
   before_filter :check_my_folders, :except => ERROR_ACTIONS
   theme :select_theme
@@ -214,23 +212,6 @@ class ApplicationController < ActionController::Base
     false  # For convenience to return from callbacks
   end
 
-  def using_reader_tokens(login_optional=false)
-    if params[:reader_tokens].is_a?(Array) and params[:reader_tokens].any?
-      Thread.current[:reader_tokens] = params[:reader_tokens]
-    end
-    begin
-      yield
-    rescue ArvadosApiClient::NotLoggedInException
-      if login_optional
-        raise
-      else
-        return redirect_to_login
-      end
-    ensure
-      Thread.current[:reader_tokens] = nil
-    end
-  end
-
   def using_specific_api_token(api_token)
     start_values = {}
     [:arvados_api_token, :user].each do |key|

diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 370c681f074c4c9225d63dd52678734d8773d526..43ff754da8ab07c5db0b5b653ad337f5337b7cd1 100644 (file)
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -95,7 +95,8 @@ class CollectionsController < ApplicationController
     # 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
+    tokens = [Thread.current[:arvados_api_token], params[:reader_token]].compact
+    usable_token = find_usable_token(tokens) do
       coll = Collection.find(params[:uuid])
     end
     if usable_token.nil?
@@ -148,18 +149,14 @@ class CollectionsController < ApplicationController
 
   protected
 
-  def find_usable_token
-    # Iterate over every token available to make it the current token and
+  def find_usable_token(token_list)
+    # Iterate over every given token 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|
+    token_list.each do |api_token|
       using_specific_api_token(api_token) do
         begin
           yield

diff --git a/apps/workbench/config/routes.rb b/apps/workbench/config/routes.rb
index 2255856dbf5b0838bcf36db6327d206b64b90a13..30ac241333a0dcf6dbb90cd6f09fe1b42772dc58 100644 (file)
--- a/apps/workbench/config/routes.rb
+++ b/apps/workbench/config/routes.rb
@@ -45,6 +45,8 @@ ArvadosWorkbench::Application.routes.draw do
     post 'set_persistent', on: :member
   end
   get '/collections/:uuid/*file' => 'collections#show_file', :format => false
+  get('/collections/download/:uuid/:reader_token/*file' => 'collections#show_file',
+      format: false)
   resources :folders do
     match 'remove/:item_uuid', on: :member, via: :delete, action: :remove_item
   end

diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index 4f934504ce28855809802c79d056283004604860..b31e718e4d058fcd8fbd357122e8b90586b0118e 100644 (file)
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -88,29 +88,15 @@ class CollectionsControllerTest < ActionController::TestCase
                     "controller did not find related log")
   end
 
-  test "viewing a collection with a reader token" do
+  test "viewing collection files with a reader token" do
+    skip  # Need a new route+view for this.
     params = collection_params(:foo_file)
-    params[:reader_tokens] =
-      [api_fixture('api_client_authorizations')['active']['api_token']]
-    show_collection(params)
-    assert_equal([['.', 'foo', 3]], assigns(:object).files)
-    assert_no_session
-  end
-
-  test "viewing the index with a reader token" do
-    params = {reader_tokens:
-      [api_fixture('api_client_authorizations')['spectator']['api_token']]
-    }
-    get(:index, params)
+    params[:reader_token] =
+      api_fixture('api_client_authorizations')['active']['api_token']
+    get(:show, params)
     assert_response :success
+    assert_equal([['.', 'foo', 3]], assigns(:object).files)
     assert_no_session
-    listed_collections = assigns(:collections).map { |c| c.uuid }
-    assert_includes(listed_collections,
-                    api_fixture('collections')['bar_file']['uuid'],
-                    "spectator reader token didn't list bar file")
-    refute_includes(listed_collections,
-                    api_fixture('collections')['foo_file']['uuid'],
-                    "spectator reader token listed foo file")
   end
 
   test "getting a file from Keep" do
@@ -139,7 +125,7 @@ class CollectionsControllerTest < ActionController::TestCase
   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]
+    params[:reader_token] = read_token
     get(:show_file, params)
     assert_response :success
     assert_equal(expected_contents(params, read_token), @response.body,
@@ -150,9 +136,8 @@ class CollectionsControllerTest < ActionController::TestCase
 
   test "trying to get from Keep with an unscoped reader token prompts login" do
     params = collection_params(:foo_file, 'foo')
-    read_token =
+    params[:reader_token] =
       api_fixture('api_client_authorizations')['active_noscope']['api_token']
-    params[:reader_tokens] = [read_token]
     get(:show_file, params)
     assert_response :redirect
   end
@@ -161,7 +146,7 @@ class CollectionsControllerTest < ActionController::TestCase
     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]
+    params[:reader_token] = read_token
     get(:show_file, params, sess)
     assert_response :success
     assert_equal(expected_contents(params, read_token), @response.body,

diff --git a/apps/workbench/test/test_helper.rb b/apps/workbench/test/test_helper.rb
index c1eed5cd7bf6b56412749c7e4a7170d643fd4075..e833f970c02d211905cd504a4d7a25a13a25a016 100644 (file)
--- a/apps/workbench/test/test_helper.rb
+++ b/apps/workbench/test/test_helper.rb
@@ -36,6 +36,7 @@ class ActiveSupport::TestCase
 
   def teardown
     Thread.current[:arvados_api_token] = nil
+    Thread.current[:reader_tokens] = nil
     super
   end
 end