[ARVADOS] created: f7dc278e7a8819ec17b082f540d57e1af50f3d2d
git at public.curoverse.com
git at public.curoverse.com
Mon May 5 13:04:58 EDT 2014
at f7dc278e7a8819ec17b082f540d57e1af50f3d2d (commit)
commit f7dc278e7a8819ec17b082f540d57e1af50f3d2d
Author: Brett Smith <brett at curoverse.com>
Date: Mon May 5 12:55:29 2014 -0400
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 eae7a77..a64ac11 100644
--- 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 ea42b15..06cc3a8 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -88,6 +88,48 @@ 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
commit a19f34a2e948d1e8945ca7262abafeb632bd41ae
Author: Brett Smith <brett at curoverse.com>
Date: Mon May 5 10:54:53 2014 -0400
workbench: Add initial reader tokens support.
The API server recently gained support for reader tokens, which
optionally supplement the user's permissions for read-only
operations. This commit extends Workbench to pass on reader tokens
for general index and show operations.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index d2b90c5..f9de62d 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -1,12 +1,17 @@
class ApplicationController < ActionController::Base
respond_to :html, :json, :js
protect_from_forgery
+
+ ERROR_ACTIONS = [:render_error, :render_not_found]
+
around_filter :thread_clear
- around_filter :thread_with_mandatory_api_token, :except => [:render_exception, :render_not_found]
+ around_filter(:thread_with_mandatory_api_token,
+ except: [:index, :show] + ERROR_ACTIONS)
around_filter :thread_with_optional_api_token
- before_filter :find_object_by_uuid, :except => [:index, :render_exception, :render_not_found]
- before_filter :check_user_agreements, :except => [:render_exception, :render_not_found]
- before_filter :check_user_notifications, :except => [:render_exception, :render_not_found]
+ 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
theme :select_theme
begin
@@ -183,7 +188,56 @@ class ApplicationController < ActionController::Base
end
protected
-
+
+ def redirect_to_login
+ respond_to do |f|
+ f.html {
+ if request.method == 'GET'
+ redirect_to $arvados_api_client.arvados_login_url(return_to: request.url)
+ else
+ flash[:error] = "Either you are not logged in, or your session has timed out. I can't automatically log you in and re-attempt this request."
+ redirect_to :back
+ end
+ }
+ f.json {
+ @errors = ['You do not seem to be logged in. You did not supply an API token with this request, and your session (if any) has timed out.']
+ self.render_error status: 422
+ }
+ end
+ 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|
+ start_values[key] = Thread.current[key]
+ end
+ Thread.current[:arvados_api_token] = api_token
+ Thread.current[:user] = nil
+ begin
+ yield
+ ensure
+ start_values.each_key { |key| Thread.current[key] = start_values[key] }
+ end
+ end
+
def find_object_by_uuid
if params[:id] and params[:id].match /\D/
params[:uuid] = params.delete :id
@@ -242,20 +296,7 @@ class ApplicationController < ActionController::Base
end
if try_redirect_to_login
unless login_optional
- respond_to do |f|
- f.html {
- if request.method == 'GET'
- redirect_to $arvados_api_client.arvados_login_url(return_to: request.url)
- else
- flash[:error] = "Either you are not logged in, or your session has timed out. I can't automatically log you in and re-attempt this request."
- redirect_to :back
- end
- }
- f.json {
- @errors = ['You do not seem to be logged in. You did not supply an API token with this request, and your session (if any) has timed out.']
- self.render_error status: 422
- }
- end
+ redirect_to_login
else
# login is optional for this route so go on to the regular controller
Thread.current[:arvados_api_token] = nil
diff --git a/apps/workbench/app/models/arvados_api_client.rb b/apps/workbench/app/models/arvados_api_client.rb
index efe8b7e..b6aeeca 100644
--- a/apps/workbench/app/models/arvados_api_client.rb
+++ b/apps/workbench/app/models/arvados_api_client.rb
@@ -26,16 +26,16 @@ class ArvadosApiClient
end
end
- api_token = Thread.current[:arvados_api_token]
- api_token ||= ''
-
resources_kind = class_kind(resources_kind).pluralize if resources_kind.is_a? Class
url = "#{self.arvados_v1_base}/#{resources_kind}#{action}"
# Clean up /arvados/v1/../../discovery/v1 to /discovery/v1
url.sub! '/arvados/v1/../../', '/'
- query = {"api_token" => api_token}
+ query = {
+ 'api_token' => Thread.current[:arvados_api_token] || '',
+ 'reader_tokens' => (Thread.current[:reader_tokens] || []).to_json,
+ }
if !data.nil?
data.each do |k,v|
if v.is_a? String or v.nil?
diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index d82ba6b..ea42b15 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -15,6 +15,24 @@ class CollectionsControllerTest < ActionController::TestCase
[token, params[:uuid], params[:file]].join('/')
end
+ def assert_hash_includes(actual_hash, expected_hash, msg=nil)
+ expected_hash.each do |key, value|
+ assert_equal(value, actual_hash[key], msg)
+ end
+ end
+
+ def assert_no_session
+ assert_hash_includes(session, {arvados_api_token: nil},
+ "session includes unexpected API token")
+ end
+
+ def assert_session_for_auth(client_auth)
+ api_token =
+ api_fixture('api_client_authorizations')[client_auth.to_s]['api_token']
+ assert_hash_includes(session, {arvados_api_token: api_token},
+ "session token does not belong to #{client_auth}")
+ end
+
# Mock the collection file reader to avoid external calls and return
# a predictable string.
CollectionsController.class_eval do
@@ -31,6 +49,32 @@ class CollectionsControllerTest < ActionController::TestCase
assert_equal([['.', 'foo', 3]], assigns(:object).files)
end
+ test "viewing a collection with a reader token" do
+ params = collection_params(:foo_file)
+ params[:reader_tokens] =
+ [api_fixture('api_client_authorizations')['active']['api_token']]
+ get(:show, params)
+ assert_response :success
+ 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)
+ assert_response :success
+ 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
params = collection_params(:foo_file, 'foo')
sess = session_for(:active)
commit e57c5d374a6fbca3999a97a995c366e2abb5f1bb
Author: Brett Smith <brett at curoverse.com>
Date: Wed Apr 30 10:38:08 2014 -0400
workbench: Add Collections controller tests.
Working to establish a baseline of behavior before I go mucking with
this.
diff --git a/apps/workbench/app/controllers/collections_controller.rb b/apps/workbench/app/controllers/collections_controller.rb
index 3089a1e..eae7a77 100644
--- a/apps/workbench/app/controllers/collections_controller.rb
+++ b/apps/workbench/app/controllers/collections_controller.rb
@@ -95,7 +95,7 @@ class CollectionsController < ApplicationController
Rack::Mime::MIME_TYPES[ext] || 'application/octet-stream'
self.response.headers['Content-Length'] = params[:size] if params[:size]
self.response.headers['Content-Disposition'] = params[:disposition] if params[:disposition]
- self.response_body = FileStreamer.new opts
+ self.response_body = file_enumerator opts
end
def show
@@ -161,6 +161,11 @@ class CollectionsController < ApplicationController
end
protected
+
+ def file_enumerator(opts)
+ FileStreamer.new opts
+ end
+
class FileStreamer
def initialize(opts={})
@opts = opts
diff --git a/apps/workbench/test/functional/collections_controller_test.rb b/apps/workbench/test/functional/collections_controller_test.rb
index f4a02c1..d82ba6b 100644
--- a/apps/workbench/test/functional/collections_controller_test.rb
+++ b/apps/workbench/test/functional/collections_controller_test.rb
@@ -1,4 +1,49 @@
require 'test_helper'
class CollectionsControllerTest < ActionController::TestCase
+ def collection_params(collection_name, file_name=nil)
+ uuid = api_fixture('collections')[collection_name.to_s]['uuid']
+ params = {uuid: uuid, id: uuid}
+ params[:file] = file_name if file_name
+ params
+ end
+
+ def expected_contents(params, token)
+ unless token.is_a? String
+ token = params[:api_token] || token[:arvados_api_token]
+ end
+ [token, params[:uuid], params[:file]].join('/')
+ end
+
+ # Mock the collection file reader to avoid external calls and return
+ # a predictable string.
+ CollectionsController.class_eval do
+ def file_enumerator(opts)
+ [[opts[:arvados_api_token], opts[:uuid], opts[:file]].join('/')]
+ end
+ end
+
+ test "viewing a collection" do
+ params = collection_params(:foo_file)
+ sess = session_for(:active)
+ get(:show, params, sess)
+ assert_response :success
+ assert_equal([['.', 'foo', 3]], assigns(:object).files)
+ end
+
+ test "getting a file from Keep" do
+ params = collection_params(:foo_file, 'foo')
+ sess = session_for(:active)
+ get(:show_file, params, sess)
+ assert_response :success
+ assert_equal(expected_contents(params, sess), @response.body,
+ "failed to get a correct file from Keep")
+ end
+
+ test "can't get a file from Keep without permission" do
+ params = collection_params(:foo_file, 'foo')
+ sess = session_for(:spectator)
+ get(:show_file, params, sess)
+ assert_includes([403, 422], @response.code.to_i)
+ end
end
commit 9e8d7a2d3a65593213c050b1398d17477829ffd6
Author: Brett Smith <brett at curoverse.com>
Date: Thu May 1 10:42:33 2014 -0400
workbench: Remove trailing whitespace.
diff --git a/apps/workbench/app/controllers/application_controller.rb b/apps/workbench/app/controllers/application_controller.rb
index 41d5566..d2b90c5 100644
--- a/apps/workbench/app/controllers/application_controller.rb
+++ b/apps/workbench/app/controllers/application_controller.rb
@@ -281,7 +281,7 @@ class ApplicationController < ActionController::Base
yield
else
# We skipped thread_with_mandatory_api_token. Use the optional version.
- thread_with_api_token(true) do
+ thread_with_api_token(true) do
yield
end
end
@@ -334,7 +334,7 @@ class ApplicationController < ActionController::Base
@@notification_tests = []
@@notification_tests.push lambda { |controller, current_user|
- AuthorizedKey.limit(1).where(authorized_user_uuid: current_user.uuid).each do
+ AuthorizedKey.limit(1).where(authorized_user_uuid: current_user.uuid).each do
return nil
end
return lambda { |view|
@@ -374,7 +374,7 @@ class ApplicationController < ActionController::Base
@notifications = []
if current_user
- @showallalerts = false
+ @showallalerts = false
@@notification_tests.each do |t|
a = t.call(self, current_user)
if a
diff --git a/apps/workbench/app/models/arvados_api_client.rb b/apps/workbench/app/models/arvados_api_client.rb
index cf14106..efe8b7e 100644
--- a/apps/workbench/app/models/arvados_api_client.rb
+++ b/apps/workbench/app/models/arvados_api_client.rb
@@ -15,7 +15,7 @@ class ArvadosApiClient
profile_checkpoint
@@client_mtx.synchronize do
- if not @@api_client
+ if not @@api_client
@@api_client = HTTPClient.new
if Rails.configuration.arvados_insecure_https
@@api_client.ssl_config.verify_mode = OpenSSL::SSL::VERIFY_NONE
@@ -54,11 +54,11 @@ class ArvadosApiClient
if @@profiling_enabled
query["_profile"] = "true"
end
-
+
header = {"Accept" => "application/json"}
profile_checkpoint { "Prepare request #{url} #{query[:uuid]} #{query[:where]}" }
- msg = @@api_client.post(url,
+ msg = @@api_client.post(url,
query,
header: header)
profile_checkpoint 'API transaction'
@@ -68,7 +68,7 @@ class ArvadosApiClient
end
json = msg.content
-
+
begin
resp = Oj.load(json, :symbol_keys => true)
rescue Oj::ParseError
@@ -102,7 +102,7 @@ class ArvadosApiClient
if limit
(class << ary; self; end).class_eval { attr_accessor :limit }
ary.limit = limit
- end
+ end
ary
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list