[ARVADOS] updated: 8dbf8bd4d53c200ca81494556326c03abae74581
git at public.curoverse.com
git at public.curoverse.com
Mon May 5 15:23:50 EDT 2014
Summary of changes:
.../app/controllers/application_controller.rb | 85 ++++++++++----
.../app/controllers/collections_controller.rb | 76 +++++++++++-
apps/workbench/app/models/arvados_api_client.rb | 18 ++--
.../test/functional/collections_controller_test.rb | 130 ++++++++++++++++++++
4 files changed, 272 insertions(+), 37 deletions(-)
via 8dbf8bd4d53c200ca81494556326c03abae74581 (commit)
via 3398985fa7f0997c225b55efaa97dc000a8234fa (commit)
via 5ab27a2c4889609669f6b04bd9dfd2e403e43441 (commit)
via 256142bddd532e2834b4e7f79c3146009e23059e (commit)
via c47ada5e06a91a0283bd779d17ad7e6403e1c223 (commit)
from 77f1f9eb6f2d03b53c0bb4567ef07718b934e8ea (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit 8dbf8bd4d53c200ca81494556326c03abae74581
Merge: 77f1f9e 3398985
Author: Brett Smith <brett at curoverse.com>
Date: Mon May 5 15:20:45 2014 -0400
Merge branch '1904-workbench-reader-tokens'
Refs #1904. Closes #2663, #2736.
commit 3398985fa7f0997c225b55efaa97dc000a8234fa
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..d1a8de2 100644
--- 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
commit 5ab27a2c4889609669f6b04bd9dfd2e403e43441
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 256142bddd532e2834b4e7f79c3146009e23059e
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 c47ada5e06a91a0283bd779d17ad7e6403e1c223
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