From 024e430aececb9017466f4738753d5009124a245 Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Fri, 4 Mar 2016 17:30:53 -0500 Subject: [PATCH] 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". --- .../api_client_authorizations_controller.rb | 21 ++++-- ...i_client_authorizations_controller_test.rb | 74 +++++++++++-------- 2 files changed, 58 insertions(+), 37 deletions(-) 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 2eb79c090d..cb232f0d0d 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 5da9145a81..e45bdc494e 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 -- 2.30.2