[ARVADOS] updated: 780d51f24750e22e515f8d7d0f0229958a63f3b8
git at public.curoverse.com
git at public.curoverse.com
Wed Apr 30 10:09:11 EDT 2014
Summary of changes:
sdk/ruby/lib/arvados.rb | 36 ++++--
.../api/app/controllers/application_controller.rb | 133 +++++++++-----------
.../app/controllers/arvados/v1/nodes_controller.rb | 2 +-
.../api/app/models/api_client_authorization.rb | 12 ++
services/api/app/models/node.rb | 2 +-
.../functional/arvados/v1/nodes_controller_test.rb | 9 ++
.../api/test/integration/login_workflow_test.rb | 25 ++++
.../api/test/integration/reader_tokens_test.rb | 47 +++++--
services/api/test/test_helper.rb | 2 -
9 files changed, 170 insertions(+), 98 deletions(-)
create mode 100644 services/api/test/integration/login_workflow_test.rb
via 780d51f24750e22e515f8d7d0f0229958a63f3b8 (commit)
via 86e039faff6afefb4b24b09f2402ab9a242f9093 (commit)
via 840e7d7f96f763ae139545dca5d6dfa5a54f6cc6 (commit)
via 21cc5d3556498596bf679e3cef70edfdc01236c0 (commit)
via 1864415fc83013e28d6d06f0fd0efe2d13c10013 (commit)
via 8adde5132926e8f9cc1b01d79f9307614cc6021e (commit)
via 8eaad00b025167a7505ba11ad6a05b52a43c2399 (commit)
via f845c0645d9136a1e4ad993ecf34a156367e73b7 (commit)
via 7bc20af4935fee1caa566e86a074022f0b60d166 (commit)
from 5902a68d3f8672c38949d4a57f12435f40923a23 (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 780d51f24750e22e515f8d7d0f0229958a63f3b8
Merge: 86e039f 840e7d7
Author: Brett Smith <brett at curoverse.com>
Date: Wed Apr 30 10:06:19 2014 -0400
Merge branch 'master' into 1904-api-reader-tokens-wip
Conflicts:
services/api/app/controllers/application_controller.rb
diff --cc services/api/app/controllers/application_controller.rb
index c170979,d40c6de..1b1f025
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@@ -2,18 -2,18 +2,19 @@@ class ApplicationController < ActionCon
include CurrentApiClient
include ThemesForRails::ActionController
+ ERROR_ACTIONS = [:render_error, :render_not_found]
+
respond_to :json
protect_from_forgery
- around_filter :thread_with_auth_info, :except => [:render_error, :render_not_found]
-
+ around_filter :thread_with_auth_info, except: ERROR_ACTIONS
+ before_filter :respond_with_json_by_default
before_filter :remote_ip
- before_filter :require_auth_scope, :except => :render_not_found
- before_filter :catch_redirect_hint
+ before_filter :load_read_auths
+ before_filter :require_auth_scope, except: ERROR_ACTIONS
- before_filter :find_object_by_uuid, :except => [:index, :create,
- :render_error,
- :render_not_found]
+ before_filter :catch_redirect_hint
+ before_filter(:find_object_by_uuid,
+ except: [:index, :create] + ERROR_ACTIONS)
before_filter :load_limit_offset_order_params, only: [:index, :owned_items]
before_filter :load_where_param, only: [:index, :owned_items]
before_filter :load_filters_param, only: [:index, :owned_items]
commit 86e039faff6afefb4b24b09f2402ab9a242f9093
Author: Brett Smith <brett at curoverse.com>
Date: Wed Apr 30 10:05:29 2014 -0400
api: Only load reader tokens in scope.
A security improvement suggested by review. Refs #1904.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 75dec10..c170979 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -3,21 +3,15 @@ class ApplicationController < ActionController::Base
include ThemesForRails::ActionController
ERROR_ACTIONS = [:render_error, :render_not_found]
- READ_ACTIONS = [:index, :owned_items, :show]
respond_to :json
protect_from_forgery
around_filter :thread_with_auth_info, except: ERROR_ACTIONS
before_filter :remote_ip
- before_filter :load_reader_tokens
-
- # The next two methods should be complementary.
- # Almost all methods should require either a login or read access.
- before_filter :require_login, except: READ_ACTIONS + ERROR_ACTIONS
- before_filter :require_read_access, only: READ_ACTIONS
+ before_filter :load_read_auths
before_filter :require_auth_scope, except: ERROR_ACTIONS
- before_filter :catch_redirect_hint
+ before_filter :catch_redirect_hint
before_filter(:find_object_by_uuid,
except: [:index, :create] + ERROR_ACTIONS)
before_filter :load_limit_offset_order_params, only: [:index, :owned_items]
@@ -396,42 +390,16 @@ class ApplicationController < ActionController::Base
end
# Authentication
- def require_login
- if not current_user
- respond_need_login
- false
- end
- end
-
- def require_read_access
- if @read_auths.empty?
- respond_need_login
- false
- end
- end
-
- def respond_need_login
- respond_to do |format|
- format.json {
- render :json => { errors: ['Not logged in'] }.to_json, status: 401
- }
- format.html {
- redirect_to '/auth/joshid'
- }
- end
- end
-
- def admin_required
- unless current_user and current_user.is_admin
- render :json => { errors: ['Forbidden'] }.to_json, status: 403
- end
- end
-
- def load_reader_tokens
+ def load_read_auths
@read_auths = []
+ if current_api_client_authorization
+ @read_auths << current_api_client_authorization
+ end
+ # Load reader tokens if this is a read request.
# If there are too many reader tokens, assume the request is malicious
# and ignore it.
- if params[:reader_tokens] and params[:reader_tokens].size < 100
+ if request.get? and params[:reader_tokens] and
+ params[:reader_tokens].size < 100
@read_auths += ApiClientAuthorization
.includes(:user)
.where('api_token IN (?) AND
@@ -439,32 +407,37 @@ class ApplicationController < ActionController::Base
params[:reader_tokens])
.all
end
- @have_scopes = @read_auths.flat_map do |auth|
- auth.scopes.map do |scope|
- if scope == 'all'
- 'GET /'
- elsif scope.start_with? 'GET '
- scope
- else
- nil
- end
+ @read_auths.select! { |auth| auth.scopes_allow_request? request }
+ @read_users = @read_auths.map { |auth| auth.user }.uniq
+ end
+
+ def require_login
+ if not current_user
+ respond_to do |format|
+ format.json {
+ render :json => { errors: ['Not logged in'] }.to_json, status: 401
+ }
+ format.html {
+ redirect_to '/auth/joshid'
+ }
end
- end.compact
- if current_api_client_authorization
- @read_auths << current_api_client_authorization
- @have_scopes += current_api_client_authorization.scopes
+ false
+ end
+ end
+
+ def admin_required
+ unless current_user and current_user.is_admin
+ render :json => { errors: ['Forbidden'] }.to_json, status: 403
end
- @read_users = @read_auths.map { |auth| auth.user }.uniq
end
def require_auth_scope
- need_scope = "#{request.method} #{request.path}"
- @have_scopes.each do |have_scope|
- return if (have_scope == 'all') or (have_scope == need_scope) or
- ((have_scope.end_with? '/') and (need_scope.start_with? have_scope))
+ if @read_auths.empty?
+ if require_login != false
+ render :json => { errors: ['Forbidden'] }.to_json, status: 403
+ end
+ false
end
- render :json => { errors: ['Forbidden'] }.to_json, status: 403
- false
end
def thread_with_auth_info
diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb
index 3b73f40..2488d83 100644
--- a/services/api/app/models/api_client_authorization.rb
+++ b/services/api/app/models/api_client_authorization.rb
@@ -62,6 +62,18 @@ class ApiClientAuthorization < ArvadosModel
end
def modified_at=(x) end
+ def scopes_allow?(req_s)
+ scopes.each do |scope|
+ return true if (scope == 'all') or (scope == req_s) or
+ ((scope.end_with? '/') and (req_s.start_with? scope))
+ end
+ false
+ end
+
+ def scopes_allow_request?(request)
+ scopes_allow? [request.method, request.path].join(' ')
+ end
+
def logged_attributes
attrs = attributes.dup
attrs.delete('api_token')
diff --git a/services/api/test/integration/reader_tokens_test.rb b/services/api/test/integration/reader_tokens_test.rb
index 9f6a090..6ed8461 100644
--- a/services/api/test/integration/reader_tokens_test.rb
+++ b/services/api/test/integration/reader_tokens_test.rb
@@ -77,4 +77,9 @@ class Arvados::V1::ReaderTokensTest < ActionController::IntegrationTest
get_specimens(:active_noscope, :expired)
assert_response 403
end
+
+ test "reader tokens grant no permissions outside their scope" do
+ refute_includes(get_specimen_uuids(:active, :admin_vm), spectator_specimen,
+ "scoped reader token granted permissions out of scope")
+ end
end
commit 21cc5d3556498596bf679e3cef70edfdc01236c0
Author: Brett Smith <brett at curoverse.com>
Date: Tue Apr 29 17:41:48 2014 -0400
api: Accept JSON-formatted reader token array.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index c04fc62..75dec10 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -558,28 +558,38 @@ class ApplicationController < ActionController::Base
end
end
- def self.accept_attribute_as_json(attr, force_class=nil)
- before_filter lambda { accept_attribute_as_json attr, force_class }
+ def load_json_value(hash, key, must_be_class=nil)
+ if hash[key].is_a? String
+ hash[key] = Oj.load(hash[key], symbol_keys: false)
+ if must_be_class and !hash[key].is_a? must_be_class
+ raise TypeError.new("parameter #{key.to_s} must be a #{must_be_class.to_s}")
+ end
+ end
+ end
+
+ def self.accept_attribute_as_json(attr, must_be_class=nil)
+ before_filter lambda { accept_attribute_as_json attr, must_be_class }
end
accept_attribute_as_json :properties, Hash
accept_attribute_as_json :info, Hash
- def accept_attribute_as_json(attr, force_class)
+ def accept_attribute_as_json(attr, must_be_class)
if params[resource_name] and resource_attrs.is_a? Hash
- if resource_attrs[attr].is_a? String
- resource_attrs[attr] = Oj.load(resource_attrs[attr],
- symbol_keys: false)
- if force_class and !resource_attrs[attr].is_a? force_class
- raise TypeError.new("#{resource_name}[#{attr.to_s}] must be a #{force_class.to_s}")
- end
- elsif resource_attrs[attr].is_a? Hash
+ if resource_attrs[attr].is_a? Hash
# Convert symbol keys to strings (in hashes provided by
# resource_attrs)
resource_attrs[attr] = resource_attrs[attr].
with_indifferent_access.to_hash
+ else
+ load_json_value(resource_attrs, attr, must_be_class)
end
end
end
+ def self.accept_param_as_json(key, must_be_class=nil)
+ prepend_before_filter lambda { load_json_value(params, key, must_be_class) }
+ end
+ accept_param_as_json :reader_tokens, Array
+
def render_list
@object_list = {
:kind => "arvados##{(@response_resource_name || resource_name).camelize(:lower)}List",
diff --git a/services/api/test/integration/reader_tokens_test.rb b/services/api/test/integration/reader_tokens_test.rb
index 88607e2..9f6a090 100644
--- a/services/api/test/integration/reader_tokens_test.rb
+++ b/services/api/test/integration/reader_tokens_test.rb
@@ -7,20 +7,34 @@ class Arvados::V1::ReaderTokensTest < ActionController::IntegrationTest
specimens(:owned_by_spectator).uuid
end
- def get_specimens(main_auth, read_auth)
+ def get_specimens(main_auth, read_auth, formatter=:to_a)
params = {}
- params[:reader_tokens] = [api_token(read_auth)] if read_auth
+ params[:reader_tokens] = [api_token(read_auth)].send(formatter) if read_auth
headers = {}
headers.merge!(auth(main_auth)) if main_auth
get('/arvados/v1/specimens', params, headers)
end
- def get_specimen_uuids(main_auth, read_auth)
- get_specimens(main_auth, read_auth)
+ def get_specimen_uuids(main_auth, read_auth, formatter=:to_a)
+ get_specimens(main_auth, read_auth, formatter)
assert_response :success
json_response['items'].map { |spec| spec['uuid'] }
end
+ def assert_post_denied(main_auth, read_auth, formatter=:to_a)
+ if main_auth
+ headers = auth(main_auth)
+ expected = 403
+ else
+ headers = {}
+ expected = 401
+ end
+ post('/arvados/v1/specimens.json',
+ {specimen: {}, reader_tokens: [api_token(read_auth)].send(formatter)},
+ headers)
+ assert_response expected
+ end
+
test "active user can't see spectator specimen" do
# Other tests in this suite assume that the active user doesn't
# have read permission to the owned_by_spectator specimen.
@@ -37,17 +51,17 @@ class Arvados::V1::ReaderTokensTest < ActionController::IntegrationTest
spectator_specimen, "did not find spectator specimen")
end
+ test "#{main_auth} auth with JSON read token #{read_auth} can read" do
+ assert_includes(get_specimen_uuids(main_auth, read_auth, :to_json),
+ spectator_specimen, "did not find spectator specimen")
+ end
+
test "#{main_auth} auth with reader token #{read_auth} can't write" do
- if main_auth
- headers = auth(main_auth)
- expected = 403
- else
- headers = {}
- expected = 401
- end
- post('/arvados/v1/specimens.json',
- {specimen: {}, reader_tokens: [api_token(read_auth)]}, headers)
- assert_response expected
+ assert_post_denied(main_auth, read_auth)
+ end
+
+ test "#{main_auth} auth with JSON read token #{read_auth} can't write" do
+ assert_post_denied(main_auth, read_auth, :to_json)
end
end
end
commit 1864415fc83013e28d6d06f0fd0efe2d13c10013
Author: Brett Smith <brett at curoverse.com>
Date: Tue Apr 29 16:49:30 2014 -0400
api: Remove extraneous IntegrationTest include.
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index e2f2522..3e81ff7 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -43,8 +43,6 @@ class ActiveSupport::TestCase
end
class ActionDispatch::IntegrationTest
- include ArvadosTestSupport
-
teardown do
Thread.current[:api_client_ip_address] = nil
Thread.current[:api_client_authorization] = nil
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list