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 2eb79c090dcd69a4e6e4d4157b0ea0f0d2de5afc..cb232f0d0de4f36ccd3549224fe0beeea0aa31ae 100644 (file)
--- 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 5da9145a81e052b3ef5a471f672c7568399e428b..e45bdc494e566af5d278a29bf652fd8985eb5fcc 100644 (file)
--- 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