[ARVADOS] created: 5902a68d3f8672c38949d4a57f12435f40923a23
git at public.curoverse.com
git at public.curoverse.com
Tue Apr 29 14:33:21 EDT 2014
at 5902a68d3f8672c38949d4a57f12435f40923a23 (commit)
commit 5902a68d3f8672c38949d4a57f12435f40923a23
Author: Brett Smith <brett at curoverse.com>
Date: Tue Apr 29 14:30:53 2014 -0400
api: Introduce reader_tokens for extra access.
reader_tokens are API tokens whose read permissions will be added to
the primary API token's. For actions that only read resources, like
:index and :show, you can now omit the API token as long as you
provide at least one reader_token.
This allows users to get access to one-off resources without
interrupting their primary session. For example, Workbench will use
it to access Collections shared via secret link.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 4c30960..c04fc62 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -2,25 +2,31 @@ class ApplicationController < ActionController::Base
include CurrentApiClient
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 => [:render_error, :render_not_found]
-
+ around_filter :thread_with_auth_info, except: ERROR_ACTIONS
before_filter :remote_ip
- before_filter :require_auth_scope, :except => :render_not_found
+ 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 :require_auth_scope, except: ERROR_ACTIONS
before_filter :catch_redirect_hint
- before_filter :find_object_by_uuid, :except => [:index, :create,
- :render_error,
- :render_not_found]
+ 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]
before_filter :find_objects_for_index, :only => :index
before_filter :reload_object_before_update, :only => :update
- before_filter :render_404_if_no_object, except: [:index, :create,
- :render_error,
- :render_not_found]
+ before_filter(:render_404_if_no_object,
+ except: [:index, :create] + ERROR_ACTIONS)
theme :select_theme
@@ -91,7 +97,7 @@ class ApplicationController < ActionController::Base
when 'ApiClientAuthorization'
# Do not want.
else
- @objects = klass.readable_by(current_user)
+ @objects = klass.readable_by(*@read_users)
cond_sql = "#{klass.table_name}.owner_uuid = ?"
cond_params = [@object.uuid]
if params[:include_linked]
@@ -248,7 +254,7 @@ class ApplicationController < ActionController::Base
end
def find_objects_for_index
- @objects ||= model_class.readable_by(current_user)
+ @objects ||= model_class.readable_by(*@read_users)
apply_where_limit_order_params
end
@@ -391,32 +397,74 @@ class ApplicationController < ActionController::Base
# Authentication
def require_login
- if current_user
- true
- else
- respond_to do |format|
- format.json {
- render :json => { errors: ['Not logged in'] }.to_json, status: 401
- }
- format.html {
- redirect_to '/auth/joshid'
- }
- end
+ 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
+ @read_auths = []
+ # 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
+ @read_auths += ApiClientAuthorization
+ .includes(:user)
+ .where('api_token IN (?) AND
+ (expires_at IS NULL OR expires_at > CURRENT_TIMESTAMP)',
+ 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
+ end
+ end.compact
+ if current_api_client_authorization
+ @read_auths << current_api_client_authorization
+ @have_scopes += current_api_client_authorization.scopes
+ end
+ @read_users = @read_auths.map { |auth| auth.user }.uniq
+ end
+
def require_auth_scope
- return false unless require_login
- unless current_api_client_auth_has_scope("#{request.method} #{request.path}")
- render :json => { errors: ['Forbidden'] }.to_json, status: 403
+ 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))
end
+ render :json => { errors: ['Forbidden'] }.to_json, status: 403
+ false
end
def thread_with_auth_info
diff --git a/services/api/app/controllers/arvados/v1/keep_disks_controller.rb b/services/api/app/controllers/arvados/v1/keep_disks_controller.rb
index 47018d4..a4eb9a1 100644
--- a/services/api/app/controllers/arvados/v1/keep_disks_controller.rb
+++ b/services/api/app/controllers/arvados/v1/keep_disks_controller.rb
@@ -1,4 +1,5 @@
class Arvados::V1::KeepDisksController < ApplicationController
+ skip_before_filter :require_login, :only => :ping
skip_before_filter :require_auth_scope, :only => :ping
def self._ping_requires_parameters
diff --git a/services/api/app/controllers/arvados/v1/nodes_controller.rb b/services/api/app/controllers/arvados/v1/nodes_controller.rb
index d7a477d..8005a0e 100644
--- a/services/api/app/controllers/arvados/v1/nodes_controller.rb
+++ b/services/api/app/controllers/arvados/v1/nodes_controller.rb
@@ -1,4 +1,5 @@
class Arvados::V1::NodesController < ApplicationController
+ skip_before_filter :require_login, :only => :ping
skip_before_filter :require_auth_scope, :only => :ping
skip_before_filter :find_object_by_uuid, :only => :ping
skip_before_filter :render_404_if_no_object, :only => :ping
diff --git a/services/api/app/controllers/arvados/v1/schema_controller.rb b/services/api/app/controllers/arvados/v1/schema_controller.rb
index 625519e..6db8422 100644
--- a/services/api/app/controllers/arvados/v1/schema_controller.rb
+++ b/services/api/app/controllers/arvados/v1/schema_controller.rb
@@ -2,6 +2,7 @@ class Arvados::V1::SchemaController < ApplicationController
skip_before_filter :find_objects_for_index
skip_before_filter :find_object_by_uuid
skip_before_filter :render_404_if_no_object
+ skip_before_filter :require_read_access
skip_before_filter :require_auth_scope
def index
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index e42194b..f7da9df 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -72,19 +72,26 @@ class ArvadosModel < ActiveRecord::Base
# end
# end
- def self.readable_by user
- uuid_list = [user.uuid, *user.groups_i_can(:read)]
+ def self.readable_by(*users_list)
+ users_list.compact!
+ user_uuids = users_list.map { |u| u.uuid }
+ uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
sanitized_uuid_list = uuid_list.
collect { |uuid| sanitize(uuid) }.join(', ')
- or_references_me = ''
- if self == Link and user
- or_references_me = "OR (#{table_name}.link_class in (#{sanitize 'permission'}, #{sanitize 'resources'}) AND #{sanitize user.uuid} IN (#{table_name}.head_uuid, #{table_name}.tail_uuid))"
+ sql_conds = []
+ sql_params = []
+ if users_list.select { |u| u.is_admin }.empty?
+ sql_conds += ["#{table_name}.owner_uuid in (?)",
+ "#{table_name}.uuid in (?)",
+ "permissions.head_uuid IS NOT NULL"]
+ sql_params += [uuid_list, user_uuids]
+ if self == Link and users_list.any?
+ sql_conds += ["(#{table_name}.link_class in (#{sanitize 'permission'}, #{sanitize 'resources'}) AND (#{table_name}.head_uuid IN (?) OR #{table_name}.tail_uuid IN (?)))"]
+ sql_params += [user_uuids, user_uuids]
+ end
end
- joins("LEFT JOIN links permissions ON permissions.head_uuid in (#{table_name}.owner_uuid, #{table_name}.uuid) AND permissions.tail_uuid in (#{sanitized_uuid_list}) AND permissions.link_class='permission'").
- where("?=? OR #{table_name}.owner_uuid in (?) OR #{table_name}.uuid=? OR permissions.head_uuid IS NOT NULL #{or_references_me}",
- true, user.is_admin,
- uuid_list,
- user.uuid)
+ joins("LEFT JOIN links permissions ON permissions.head_uuid in (#{table_name}.owner_uuid, #{table_name}.uuid) AND permissions.tail_uuid in (#{sanitized_uuid_list}) AND permissions.link_class='permission'")
+ .where(sql_conds.join(' OR '), *sql_params)
end
def logged_attributes
@@ -251,7 +258,7 @@ class ArvadosModel < ActiveRecord::Base
self.class.kind
end
- def self.readable_by (u)
+ def self.readable_by (*u)
self
end
diff --git a/services/api/lib/current_api_client.rb b/services/api/lib/current_api_client.rb
index 0803d54..8e7d7fc 100644
--- a/services/api/lib/current_api_client.rb
+++ b/services/api/lib/current_api_client.rb
@@ -29,19 +29,6 @@ module CurrentApiClient
Thread.current[:api_client_ip_address]
end
- # Is the current API client authorization scoped for the request?
- def current_api_client_auth_has_scope(req_s)
- (current_api_client_authorization.andand.scopes || []).select { |scope|
- if scope == 'all'
- true
- elsif scope.end_with? '/'
- req_s.start_with? scope
- else
- req_s == scope
- end
- }.any?
- end
-
def system_user_uuid
[Server::Application.config.uuid_prefix,
User.uuid_prefix,
diff --git a/services/api/test/fixtures/api_client_authorizations.yml b/services/api/test/fixtures/api_client_authorizations.yml
index f772c4f..9901ec4 100644
--- a/services/api/test/fixtures/api_client_authorizations.yml
+++ b/services/api/test/fixtures/api_client_authorizations.yml
@@ -36,6 +36,13 @@ active_trustedclient:
api_token: 27bnddk6x2nmq00a1e3gq43n9tsl5v87a3faqar2ijj8tud5en
expires_at: 2038-01-01 00:00:00
+active_noscope:
+ api_client: untrusted
+ user: active
+ api_token: activenoscopeabcdefghijklmnopqrstuvwxyz12345678901
+ expires_at: 2038-01-01 00:00:00
+ scopes: []
+
admin_vm:
api_client: untrusted
user: admin
@@ -79,6 +86,14 @@ spectator:
api_token: zw2f4gwx8hw8cjre7yp6v1zylhrhn3m5gvjq73rtpwhmknrybu
expires_at: 2038-01-01 00:00:00
+spectator_specimens:
+ api_client: untrusted
+ user: spectator
+ api_token: spectatorspecimensabcdefghijklmnopqrstuvwxyz123245
+ expires_at: 2038-01-01 00:00:00
+ scopes: ["GET /arvados/v1/specimens", "GET /arvados/v1/specimens/",
+ "POST /arvados/v1/specimens"]
+
inactive:
api_client: untrusted
user: inactive
diff --git a/services/api/test/integration/reader_tokens_test.rb b/services/api/test/integration/reader_tokens_test.rb
new file mode 100644
index 0000000..88607e2
--- /dev/null
+++ b/services/api/test/integration/reader_tokens_test.rb
@@ -0,0 +1,66 @@
+require 'test_helper'
+
+class Arvados::V1::ReaderTokensTest < ActionController::IntegrationTest
+ fixtures :all
+
+ def spectator_specimen
+ specimens(:owned_by_spectator).uuid
+ end
+
+ def get_specimens(main_auth, read_auth)
+ params = {}
+ params[:reader_tokens] = [api_token(read_auth)] 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)
+ assert_response :success
+ json_response['items'].map { |spec| spec['uuid'] }
+ 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.
+ # This test checks that this assumption still holds.
+ refute_includes(get_specimen_uuids(:active, nil), spectator_specimen,
+ ["active user can read the owned_by_spectator specimen",
+ "other tests will return false positives"].join(" - "))
+ end
+
+ [nil, :active_noscope].each do |main_auth|
+ [:spectator, :spectator_specimens].each do |read_auth|
+ test "#{main_auth} auth with reader token #{read_auth} can read" do
+ assert_includes(get_specimen_uuids(main_auth, read_auth),
+ 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
+ end
+ end
+ end
+
+ test "scopes are still limited with reader tokens" do
+ get('/arvados/v1/collections',
+ {reader_tokens: [api_token(:spectator_specimens)]},
+ auth(:active_noscope))
+ assert_response 403
+ end
+
+ test "reader tokens grant no permissions when expired" do
+ get_specimens(:active_noscope, :expired)
+ assert_response 403
+ end
+end
commit 2391023e1f860aa830cec1e82ea00a3313b3313a
Author: Brett Smith <brett at curoverse.com>
Date: Tue Apr 29 10:20:28 2014 -0400
api: More consistency in test helper methods.
I keep accidentally rewriting the methods in ArvadosTestHelper, so I'm
putting them in one module loaded for all tests so I can use them
consistently.
diff --git a/services/api/test/integration/api_client_authorizations_scopes_test.rb b/services/api/test/integration/api_client_authorizations_scopes_test.rb
index ba91670..20f83dc 100644
--- a/services/api/test/integration/api_client_authorizations_scopes_test.rb
+++ b/services/api/test/integration/api_client_authorizations_scopes_test.rb
@@ -7,84 +7,65 @@ require 'test_helper'
class Arvados::V1::ApiTokensScopeTest < ActionController::IntegrationTest
fixtures :all
- def setup
- @token = {}
- end
-
- def auth_with(name)
- @token = {api_token: api_client_authorizations(name).api_token}
- end
-
def v1_url(*parts)
(['arvados', 'v1'] + parts).join('/')
end
- def request_with_auth(method, path, params={})
- send(method, path, @token.merge(params))
- end
-
- def get_with_auth(*args)
- request_with_auth(:get_via_redirect, *args)
- end
-
- def post_with_auth(*args)
- request_with_auth(:post_via_redirect, *args)
- end
-
test "user list token can only list users" do
- auth_with :active_userlist
- get_with_auth v1_url('users')
+ get_args = [{}, auth(:active_userlist)]
+ get(v1_url('users'), *get_args)
assert_response :success
- get_with_auth v1_url('users', '') # Add trailing slash.
+ get(v1_url('users', ''), *get_args) # Add trailing slash.
assert_response :success
- get_with_auth v1_url('users', 'current')
+ get(v1_url('users', 'current'), *get_args)
assert_response 403
- get_with_auth v1_url('virtual_machines')
+ get(v1_url('virtual_machines'), *get_args)
assert_response 403
end
test "specimens token can see exactly owned specimens" do
- auth_with :active_specimens
- get_with_auth v1_url('specimens')
+ get_args = [{}, auth(:active_specimens)]
+ get(v1_url('specimens'), *get_args)
assert_response 403
- get_with_auth v1_url('specimens', specimens(:owned_by_active_user).uuid)
+ get(v1_url('specimens', specimens(:owned_by_active_user).uuid), *get_args)
assert_response :success
- get_with_auth v1_url('specimens', specimens(:owned_by_spectator).uuid)
+ get(v1_url('specimens', specimens(:owned_by_spectator).uuid), *get_args)
assert_includes(403..404, @response.status)
end
test "token with multiple scopes can use them all" do
def get_token_count
- get_with_auth v1_url('api_client_authorizations')
+ get(v1_url('api_client_authorizations'), {}, auth(:active_apitokens))
assert_response :success
token_count = JSON.parse(@response.body)['items_available']
assert_not_nil(token_count, "could not find token count")
token_count
end
- auth_with :active_apitokens
# Test the GET scope.
token_count = get_token_count
# Test the POST scope.
- post_with_auth(v1_url('api_client_authorizations'),
- api_client_authorization: {user_id: users(:active).id})
+ post(v1_url('api_client_authorizations'),
+ {api_client_authorization: {user_id: users(:active).id}},
+ auth(:active_apitokens))
assert_response :success
assert_equal(token_count + 1, get_token_count,
"token count suggests POST was not accepted")
# Test other requests are denied.
- get_with_auth v1_url('api_client_authorizations',
- api_client_authorizations(:active_apitokens).uuid)
+ get(v1_url('api_client_authorizations',
+ api_client_authorizations(:active_apitokens).uuid),
+ {}, auth(:active_apitokens))
assert_response 403
end
test "token without scope has no access" do
# Logs are good for this test, because logs have relatively
# few access controls enforced at the model level.
- auth_with :admin_noscope
- get_with_auth v1_url('logs')
+ req_args = [{}, auth(:admin_noscope)]
+ get(v1_url('logs'), *req_args)
assert_response 403
- get_with_auth v1_url('logs', logs(:log1).uuid)
+ get(v1_url('logs', logs(:log1).uuid), *req_args)
assert_response 403
- post_with_auth(v1_url('logs'), log: {})
+ post(v1_url('logs'), *req_args)
assert_response 403
end
@@ -94,10 +75,11 @@ class Arvados::V1::ApiTokensScopeTest < ActionController::IntegrationTest
def vm_logins_url(name)
v1_url('virtual_machines', virtual_machines(name).uuid, 'logins')
end
- auth_with :admin_vm
- get_with_auth vm_logins_url(:testvm)
+ get_args = [{}, auth(:admin_vm)]
+ get(vm_logins_url(:testvm), *get_args)
assert_response :success
- get_with_auth vm_logins_url(:testvm2)
- assert(@response.status >= 400, "getting testvm2 logins should have failed")
+ get(vm_logins_url(:testvm2), *get_args)
+ assert_includes(400..419, @response.status,
+ "getting testvm2 logins should have failed")
end
end
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 286cf66..e2f2522 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -2,6 +2,20 @@ ENV["RAILS_ENV"] = "test"
require File.expand_path('../../config/environment', __FILE__)
require 'rails/test_help'
+module ArvadosTestSupport
+ def json_response
+ @json_response ||= ActiveSupport::JSON.decode @response.body
+ end
+
+ def api_token(api_client_auth_name)
+ api_client_authorizations(api_client_auth_name).api_token
+ end
+
+ def auth(api_client_auth_name)
+ {'HTTP_AUTHORIZATION' => "OAuth2 #{api_token(api_client_auth_name)}"}
+ end
+end
+
class ActiveSupport::TestCase
# Setup all fixtures in test/fixtures/*.(yml|csv) for all tests in alphabetical order.
#
@@ -9,6 +23,8 @@ class ActiveSupport::TestCase
# -- they do not yet inherit this setting
fixtures :all
+ include ArvadosTestSupport
+
teardown do
Thread.current[:api_client_ip_address] = nil
Thread.current[:api_client_authorization] = nil
@@ -21,18 +37,13 @@ class ActiveSupport::TestCase
self.request.headers["Accept"] = "text/json"
end
- def json_response
- @json_response ||= ActiveSupport::JSON.decode @response.body
- end
-
def authorize_with(api_client_auth_name)
- self.request.env['HTTP_AUTHORIZATION'] = "OAuth2 #{api_client_authorizations(api_client_auth_name).api_token}"
+ self.request.env['HTTP_AUTHORIZATION'] = "OAuth2 #{api_token(api_client_auth_name)}"
end
-
- # Add more helper methods to be used by all tests here...
end
class ActionDispatch::IntegrationTest
+ include ArvadosTestSupport
teardown do
Thread.current[:api_client_ip_address] = nil
@@ -41,10 +52,6 @@ class ActionDispatch::IntegrationTest
Thread.current[:api_client] = nil
Thread.current[:user] = nil
end
-
- def auth auth_fixture
- {'HTTP_AUTHORIZATION' => "OAuth2 #{api_client_authorizations(auth_fixture).api_token}"}
- end
end
# Ensure permissions are computed from the test fixtures.
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list