[ARVADOS] created: 227589de13afa707bf19aea4db4a99fdc2b24d95

[Git user]

        at  227589de13afa707bf19aea4db4a99fdc2b24d95 (commit)


commit 227589de13afa707bf19aea4db4a99fdc2b24d95
Author: Tom Clegg <tom at curoverse.com>
Date:   Fri Mar 4 17:30:53 2016 -0500

    8079: Prevent users from looking up 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..59a05fa 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,22 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController
   end
 
   def find_object_by_uuid
-    @object = model_class.where(uuid: (params[:uuid] || params[:id])).first
+    @object = model_class.
+      where(uuid: (params[:uuid] || params[:id]),
+            api_token: current_api_client_authorization.andand.api_token).
+      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
--