[ARVADOS] updated: e3c2f8706634a3e1e6e4ecc485dadbf5e82dd2a7
Git user
git at public.curoverse.com
Mon Mar 14 15:17:49 EDT 2016
Summary of changes:
.../v1/api_client_authorizations_controller.rb | 65 ++++++++++-----
.../api/app/models/api_client_authorization.rb | 5 +-
.../api_client_authorizations_controller_test.rb | 92 +++++++++++++++-------
3 files changed, 112 insertions(+), 50 deletions(-)
via e3c2f8706634a3e1e6e4ecc485dadbf5e82dd2a7 (commit)
via e9d82ff8d9fbbc517a7b00647e87a4665a30dfc7 (commit)
via 56a38ef78f2a75c8f0809c21167851772062eb64 (commit)
via b8669d79af51d1d28d6c054b88efe0c02db029c7 (commit)
via 024e430aececb9017466f4738753d5009124a245 (commit)
from aae62bde2b57761995499add1a5799173da2d64d (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 e3c2f8706634a3e1e6e4ecc485dadbf5e82dd2a7
Merge: aae62bd e9d82ff
Author: Tom Clegg <tom at curoverse.com>
Date: Mon Mar 14 15:06:02 2016 -0400
Merge branch '8079-lookup-token-uuid' closes #8079
commit e9d82ff8d9fbbc517a7b00647e87a4665a30dfc7
Author: Tom Clegg <tom at curoverse.com>
Date: Mon Mar 14 15:04:12 2016 -0400
8079: Allow where(api_token: foo) and disallow where(api_token: ["contains", "f"])
diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
index 0040ee0..56d0d85 100644
--- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
+++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
@@ -61,7 +61,13 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
end
if @where
wanted_scopes << @where['scopes']
- @where.select! { |attr, val| attr == 'uuid' }
+ @where.select! { |attr, val|
+ # "where":{"uuid":"zzzzz-zzzzz-zzzzzzzzzzzzzzz"} is OK but
+ # "where":{"api_client_id":1} is not supported
+ # "where":{"uuid":["contains","-"]} is not supported
+ # "where":{"uuid":["uuid1","uuid2","uuid3"]} is not supported
+ val.is_a?(String) && (attr == 'uuid' || attr == 'api_token')
+ }
end
@objects = model_class.
includes(:user, :api_client).
commit 56a38ef78f2a75c8f0809c21167851772062eb64
Author: Tom Clegg <tom at curoverse.com>
Date: Sat Mar 12 17:32:17 2016 -0500
8079: Tidy up and document current_api_client_is_trusted.
diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
index cb232f0..0040ee0 100644
--- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
+++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
@@ -74,30 +74,46 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
end
def find_object_by_uuid
- conditions = {
- uuid: (params[:uuid] || params[:id]),
- user_id: current_user.id,
- }
- unless Thread.current[:api_client].andand.is_trusted
- conditions[:api_token] = current_api_client_authorization.andand.api_token
+ uuid_param = params[:uuid] || params[:id]
+ if (uuid_param != current_api_client_authorization.andand.uuid and
+ not Thread.current[:api_client].andand.is_trusted)
+ return forbidden
end
- @object = model_class.where(conditions).first
+ @limit = 1
+ @offset = 0
+ @orders = []
+ @where = {}
+ @filters = [['uuid', '=', uuid_param]]
+ find_objects_for_index
+ @object = @objects.first
end
def current_api_client_is_trusted
- unless Thread.current[:api_client].andand.is_trusted
- if %w[show update destroy].include? params['action']
- if @object.andand['api_token'] == current_api_client_authorization.andand.api_token
- return true
- end
- elsif params["action"] == "index" and @objects.andand.size == 1
- filters = @filters.map{|f|f.first}.uniq
- if [['uuid'], ['api_token']].include? filters
- return true if @objects.first['api_token'] == current_api_client_authorization.andand.api_token
- end
- end
- send_error('Forbidden: this API client cannot manipulate other clients\' access tokens.',
- status: 403)
+ if Thread.current[:api_client].andand.is_trusted
+ return true
+ end
+ # A non-trusted client can do a search for its own token if it
+ # explicitly restricts the search to its own UUID or api_token.
+ # Any other kind of query must return 403, even if it matches only
+ # the current token, because that's currently how Workbench knows
+ # (after searching on scopes) the difference between "the token
+ # I'm using now *is* the only sharing token for this collection"
+ # (403) and "my token is trusted, and there is one sharing token
+ # for this collection" (200).
+ #
+ # The @filters test here also prevents a non-trusted token from
+ # filtering on its own scopes, and discovering whether any _other_
+ # equally scoped tokens exist (403=yes, 200=no).
+ if (@objects.andand.count == 1 and
+ @objects.first.uuid == current_api_client_authorization.andand.uuid and
+ (@filters.map(&:first) & %w(uuid api_token)).any?)
+ return true
end
+ forbidden
+ end
+
+ def forbidden
+ send_error('Forbidden: this API client cannot manipulate other clients\' access tokens.',
+ status: 403)
end
end
diff --git a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
index 1fb94ab..192e6b9 100644
--- a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
@@ -72,9 +72,9 @@ class Arvados::V1::ApiClientAuthorizationsControllerTest < ActionController::Tes
[:admin, :admin, 200, 200, 1],
[:active, :active, 200, 200, 1],
# cannot look up other tokens (even for same user) if not trustedclient
- [:admin, :active, 404, 403],
- [:admin, :admin_vm, 404, 403],
- [:active, :admin, 404, 403],
+ [:admin, :active, 403, 403],
+ [:admin, :admin_vm, 403, 403],
+ [:active, :admin, 403, 403],
# cannot look up other tokens for other users, regardless of trustedclient
[:admin_trustedclient, :active, 404, 200, 0],
[:active_trustedclient, :admin, 404, 200, 0],
commit b8669d79af51d1d28d6c054b88efe0c02db029c7
Author: Tom Clegg <tom at curoverse.com>
Date: Sun Mar 6 14:50:22 2016 -0500
8079: Prevent users from changing their own token UUIDs.
diff --git a/services/api/app/models/api_client_authorization.rb b/services/api/app/models/api_client_authorization.rb
index c587e58..499a61b 100644
--- a/services/api/app/models/api_client_authorization.rb
+++ b/services/api/app/models/api_client_authorization.rb
@@ -82,8 +82,9 @@ class ApiClientAuthorization < ArvadosModel
def permission_to_update
(permission_to_create and
- not self.user_id_changed? and
- not self.owner_uuid_changed?)
+ not uuid_changed? and
+ not user_id_changed? and
+ not owner_uuid_changed?)
end
def log_update
diff --git a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
index e45bdc4..1fb94ab 100644
--- a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
@@ -126,4 +126,22 @@ class Arvados::V1::ApiClientAuthorizationsControllerTest < ActionController::Tes
end
end
end
+
+ test "scoped token cannot change its own scopes" do
+ authorize_with :admin_vm
+ put :update, {
+ id: api_client_authorizations(:admin_vm).uuid,
+ api_client_authorization: {scopes: ['all']},
+ }
+ assert_response 403
+ end
+
+ test "token cannot change its own uuid" do
+ authorize_with :admin
+ put :update, {
+ id: api_client_authorizations(:admin).uuid,
+ api_client_authorization: {uuid: 'zzzzz-gj3su-zzzzzzzzzzzzzzz'},
+ }
+ assert_response 403
+ end
end
commit 024e430aececb9017466f4738753d5009124a245
Author: Tom Clegg <tom at curoverse.com>
Date: Fri Mar 4 17:30:53 2016 -0500
8079: Prevent users from looking up other users' tokens by UUID.
Previous code was allowing any user logging in through a "trusted
client" (typically Workbench) to retrieve the secret token for any
ApiClientAuthorization whose UUID is known. This won't be acceptable
when Container records start including those UUIDs.
Also added permission for any user to update (e.g., change expiration)
and delete their current token, even if the token wasn't assigned
through a "trusted client".
diff --git a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
index 2eb79c0..cb232f0 100644
--- a/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
+++ b/services/api/app/controllers/arvados/v1/api_client_authorizations_controller.rb
@@ -49,14 +49,14 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
def find_objects_for_index
# Here we are deliberately less helpful about searching for client
# authorizations. We look up tokens belonging to the current user
- # and filter by exact matches on api_token and scopes.
+ # and filter by exact matches on uuid, api_token, and scopes.
wanted_scopes = []
if @filters
wanted_scopes.concat(@filters.map { |attr, operator, operand|
((attr == 'scopes') and (operator == '=')) ? operand : nil
})
@filters.select! { |attr, operator, operand|
- ((attr == 'uuid') and (operator == '=')) || ((attr == 'api_token') and (operator == '='))
+ operator == '=' && (attr == 'uuid' || attr == 'api_token')
}
end
if @where
@@ -74,21 +74,26 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
end
def find_object_by_uuid
- @object = model_class.where(uuid: (params[:uuid] || params[:id])).first
+ conditions = {
+ uuid: (params[:uuid] || params[:id]),
+ user_id: current_user.id,
+ }
+ unless Thread.current[:api_client].andand.is_trusted
+ conditions[:api_token] = current_api_client_authorization.andand.api_token
+ end
+ @object = model_class.where(conditions).first
end
def current_api_client_is_trusted
unless Thread.current[:api_client].andand.is_trusted
- if params["action"] == "show"
- if @object and @object['api_token'] == current_api_client_authorization.andand.api_token
+ if %w[show update destroy].include? params['action']
+ if @object.andand['api_token'] == current_api_client_authorization.andand.api_token
return true
end
elsif params["action"] == "index" and @objects.andand.size == 1
filters = @filters.map{|f|f.first}.uniq
- if ['uuid'] == filters
+ if [['uuid'], ['api_token']].include? filters
return true if @objects.first['api_token'] == current_api_client_authorization.andand.api_token
- elsif ['api_token'] == filters
- return true if @objects.first[:user_id] = current_user.id
end
end
send_error('Forbidden: this API client cannot manipulate other clients\' access tokens.',
diff --git a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
index 5da9145..e45bdc4 100644
--- a/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb
@@ -68,46 +68,62 @@ class Arvados::V1::ApiClientAuthorizationsControllerTest < ActionController::Tes
end
end
- [
- [:admin, :admin, 200],
- [:admin, :active, 403],
- [:admin, :admin_vm, 403], # this belongs to the user of current session, but we can't get it by uuid
- [:admin_trustedclient, :active, 200],
- ].each do |user, token, status|
- test "as user #{user} get #{token} token and expect #{status}" do
+ [# anyone can look up the token they're currently using
+ [:admin, :admin, 200, 200, 1],
+ [:active, :active, 200, 200, 1],
+ # cannot look up other tokens (even for same user) if not trustedclient
+ [:admin, :active, 404, 403],
+ [:admin, :admin_vm, 404, 403],
+ [:active, :admin, 404, 403],
+ # cannot look up other tokens for other users, regardless of trustedclient
+ [:admin_trustedclient, :active, 404, 200, 0],
+ [:active_trustedclient, :admin, 404, 200, 0],
+ ].each do |user, token, expect_get_response, expect_list_response, expect_list_items|
+ test "using '#{user}', get '#{token}' by uuid" do
authorize_with user
- get :show, {id: api_client_authorizations(token).uuid}
- assert_response status
+ get :show, {
+ id: api_client_authorizations(token).uuid,
+ }
+ assert_response expect_get_response
+ end
+
+ test "using '#{user}', update '#{token}' by uuid" do
+ authorize_with user
+ put :update, {
+ id: api_client_authorizations(token).uuid,
+ api_client_authorization: {},
+ }
+ assert_response expect_get_response
end
- end
- [
- [:admin, :admin, 200],
- [:admin, :active, 403],
- [:admin, :admin_vm, 403], # this belongs to the user of current session, but we can't list it by uuid
- [:admin_trustedclient, :active, 200],
- ].each do |user, token, status|
- test "as user #{user} list #{token} token using uuid and expect #{status}" do
+ test "using '#{user}', delete '#{token}' by uuid" do
+ authorize_with user
+ post :destroy, {
+ id: api_client_authorizations(token).uuid,
+ }
+ assert_response expect_get_response
+ end
+
+ test "using '#{user}', list '#{token}' by uuid" do
authorize_with user
get :index, {
- filters: [['uuid','=',api_client_authorizations(token).uuid]]
+ filters: [['uuid','=',api_client_authorizations(token).uuid]],
}
- assert_response status
+ assert_response expect_list_response
+ if expect_list_items
+ assert_equal assigns(:objects).length, expect_list_items
+ end
end
- end
- [
- [:admin, :admin, 200],
- [:admin, :active, 403],
- [:admin, :admin_vm, 200], # this belongs to the user of current session, and can be listed by token
- [:admin_trustedclient, :active, 200],
- ].each do |user, token, status|
- test "as user #{user} list #{token} token using token and expect #{status}" do
+ test "using '#{user}', list '#{token}' by token" do
authorize_with user
get :index, {
- filters: [['api_token','=',api_client_authorizations(token).api_token]]
+ filters: [['api_token','=',api_client_authorizations(token).api_token]],
}
- assert_response status
+ assert_response expect_list_response
+ if expect_list_items
+ assert_equal assigns(:objects).length, expect_list_items
+ end
end
end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list