From 6241cccaeba4413883699c360bde08e0e544a10e Mon Sep 17 00:00:00 2001 From: Tom Clegg Date: Tue, 20 Dec 2022 11:39:01 -0500 Subject: [PATCH] 18693: Add row locking. Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- .../app/controllers/application_controller.rb | 15 ++++-- .../api_client_authorizations_controller.rb | 8 +++- .../arvados/v1/collections_controller.rb | 10 ++-- .../arvados/v1/links_controller.rb | 30 +++++++----- services/api/app/models/link.rb | 46 ++++++++++++------- .../arvados/v1/nodes_controller_test.rb | 11 ----- services/api/test/unit/permission_test.rb | 16 ++++++- 7 files changed, 87 insertions(+), 49 deletions(-) diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb index 4625ef654c..88679f4f33 100644 --- a/services/api/app/controllers/application_controller.rb +++ b/services/api/app/controllers/application_controller.rb @@ -46,7 +46,8 @@ class ApplicationController < ActionController::Base before_action :load_limit_offset_order_params, only: [:index, :contents] before_action :load_select_param before_action(:find_object_by_uuid, - except: [:index, :create] + ERROR_ACTIONS) + except: [:index, :create, :update] + ERROR_ACTIONS) + before_action :find_object_for_update, only: [:update] before_action :load_where_param, only: [:index, :contents] before_action :load_filters_param, only: [:index, :contents] before_action :find_objects_for_index, :only => :index @@ -464,7 +465,11 @@ class ApplicationController < ActionController::Base controller_name end - def find_object_by_uuid + def find_object_for_update + find_object_by_uuid(with_lock: true) + end + + def find_object_by_uuid(with_lock: false) if params[:id] and params[:id].match(/\D/) params[:uuid] = params.delete :id end @@ -475,7 +480,11 @@ class ApplicationController < ActionController::Base @filters = [] @objects = nil find_objects_for_index - @object = @objects.first + if with_lock + @object = @objects.lock.first + else + @object = @objects.first + end end def nullable_attributes 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 22bcb6c1d5..8ff5520e37 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 @@ -128,7 +128,7 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController super end - def find_object_by_uuid + def find_object_by_uuid(with_lock: false) uuid_param = params[:uuid] || params[:id] if (uuid_param != current_api_client_authorization.andand.uuid && !Thread.current[:api_client].andand.is_trusted) @@ -140,7 +140,11 @@ class Arvados::V1::ApiClientAuthorizationsController < ApplicationController @where = {} @filters = [['uuid', '=', uuid_param]] find_objects_for_index - @object = @objects.first + query = @objects + if with_lock + query = query.lock + end + @object = query.first end def current_api_client_is_trusted diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb index c9b36e19ed..d4860cce15 100644 --- a/services/api/app/controllers/arvados/v1/collections_controller.rb +++ b/services/api/app/controllers/arvados/v1/collections_controller.rb @@ -60,7 +60,7 @@ class Arvados::V1::CollectionsController < ApplicationController super end - def find_object_by_uuid + def find_object_by_uuid(with_lock: false) if loc = Keep::Locator.parse(params[:id]) loc.strip_hints! @@ -81,7 +81,11 @@ class Arvados::V1::CollectionsController < ApplicationController # available lifetime. select_attrs = (@select || ["manifest_text"]) | ["portable_data_hash", "trash_at"] - if c = Collection. + model = Collection + if with_lock + model = model.lock + end + if c = model. readable_by(*@read_users, opts). where({ portable_data_hash: loc.to_s }). order("trash_at desc"). @@ -98,7 +102,7 @@ class Arvados::V1::CollectionsController < ApplicationController end end else - super + super(with_lock: with_lock) end end diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb index c9ebe03460..32d005874a 100644 --- a/services/api/app/controllers/arvados/v1/links_controller.rb +++ b/services/api/app/controllers/arvados/v1/links_controller.rb @@ -22,10 +22,12 @@ class Arvados::V1::LinksController < ApplicationController resource_attrs.delete :tail_kind if resource_attrs[:link_class] == 'permission' && Link::PermLevel[resource_attrs[:name]] - existing = Link.where(link_class: 'permission', - tail_uuid: resource_attrs[:tail_uuid], - head_uuid: resource_attrs[:head_uuid], - name: Link::PermLevel.keys).first + existing = Link. + lock. # select ... for update + where(link_class: 'permission', + tail_uuid: resource_attrs[:tail_uuid], + head_uuid: resource_attrs[:head_uuid], + name: Link::PermLevel.keys).first if existing @object = existing if Link::PermLevel[resource_attrs[:name]] > Link::PermLevel[existing.name] @@ -41,9 +43,11 @@ class Arvados::V1::LinksController < ApplicationController resource_attrs[:name] == 'can_login' && resource_attrs[:properties].respond_to?(:has_key?) && resource_attrs[:properties].has_key?(:username) - existing = Link.where(link_class: 'permission', - tail_uuid: resource_attrs[:tail_uuid], - head_uuid: resource_attrs[:head_uuid]). + existing = Link. + lock. # select ... for update + where(link_class: 'permission', + tail_uuid: resource_attrs[:tail_uuid], + head_uuid: resource_attrs[:head_uuid]). where('properties @> ?', SafeJSON.dump({'username' => resource_attrs[:properties][:username]})). first if existing @@ -70,7 +74,7 @@ class Arvados::V1::LinksController < ApplicationController protected - def find_object_by_uuid + def find_object_by_uuid(with_lock: false) if params[:id] && params[:id].match(/\D/) params[:uuid] = params.delete :id end @@ -81,7 +85,7 @@ class Arvados::V1::LinksController < ApplicationController .where(uuid: params[:uuid]) .first elsif !current_user - super + super(with_lock: with_lock) else # The usual permission-filtering index query is unnecessarily # inefficient, and doesn't match all permission links that @@ -89,11 +93,15 @@ class Arvados::V1::LinksController < ApplicationController # by UUID, then check whether (a) its tail_uuid is the current # user or (b) its head_uuid is an object the current_user # can_manage. - link = Link.unscoped.where(uuid: params[:uuid]).first + model = Link + if with_lock + model = model.lock + end + link = model.unscoped.where(uuid: params[:uuid]).first if link && link.link_class != 'permission' # Not a permission link. Re-fetch using generic # permission-filtering query. - super + super(with_lock: with_lock) elsif link && (current_user.uuid == link.tail_uuid || current_user.can?(manage: link.head_uuid)) # Permission granted. diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb index e8ac51f998..4d4c2832bb 100644 --- a/services/api/app/models/link.rb +++ b/services/api/app/models/link.rb @@ -15,6 +15,7 @@ class Link < ArvadosModel validate :permission_to_attach_to_objects before_update :restrict_alter_permissions before_update :apply_max_overlapping_permissions + before_create :apply_max_overlapping_permissions after_update :delete_overlapping_permissions after_update :call_update_permissions after_create :call_update_permissions @@ -40,10 +41,12 @@ class Link < ArvadosModel def apply_max_overlapping_permissions return if self.link_class != 'permission' || !PermLevel[self.name] - Link.where(link_class: 'permission', - tail_uuid: self.tail_uuid, - head_uuid: self.head_uuid, - name: PermLevel.keys). + Link. + lock. # select ... for update + where(link_class: 'permission', + tail_uuid: self.tail_uuid, + head_uuid: self.head_uuid, + name: PermLevel.keys). where('uuid <> ?', self.uuid).each do |other| if PermLevel[other.name] > PermLevel[self.name] self.name = other.name @@ -53,23 +56,32 @@ class Link < ArvadosModel def delete_overlapping_permissions return if self.link_class != 'permission' + redundant = nil if PermLevel[self.name] - Link.where(link_class: 'permission', - tail_uuid: self.tail_uuid, - head_uuid: self.head_uuid, - name: PermLevel.keys). - where('uuid <> ?', self.uuid). - delete_all + redundant = Link. + lock. # select ... for update + where(link_class: 'permission', + tail_uuid: self.tail_uuid, + head_uuid: self.head_uuid, + name: PermLevel.keys). + where('uuid <> ?', self.uuid) elsif self.name == 'can_login' && self.properties.respond_to?(:has_key?) && self.properties.has_key?('username') - Link.where(link_class: 'permission', - tail_uuid: self.tail_uuid, - head_uuid: self.head_uuid, - name: 'can_login'). - where('properties @> ?', SafeJSON.dump({'username' => self.properties['username']})). - where('uuid <> ?', self.uuid). - delete_all + redundant = Link. + lock. # select ... for update + where(link_class: 'permission', + tail_uuid: self.tail_uuid, + head_uuid: self.head_uuid, + name: 'can_login'). + where('properties @> ?', SafeJSON.dump({'username' => self.properties['username']})). + where('uuid <> ?', self.uuid) + end + if redundant + redundant.each do |link| + link.clear_permissions + end + redundant.delete_all end end diff --git a/services/api/test/functional/arvados/v1/nodes_controller_test.rb b/services/api/test/functional/arvados/v1/nodes_controller_test.rb index c61a57ecc8..47f6c5ff3f 100644 --- a/services/api/test/functional/arvados/v1/nodes_controller_test.rb +++ b/services/api/test/functional/arvados/v1/nodes_controller_test.rb @@ -211,17 +211,6 @@ class Arvados::V1::NodesControllerTest < ActionController::TestCase assert_response 403 end - test "job readable after updating other attributes" do - authorize_with :admin - post :update, params: { - id: nodes(:busy).uuid, - node: {last_ping_at: 1.second.ago}, - } - assert_response :success - assert_equal(jobs(:nearly_finished_job).uuid, json_response["job_uuid"], - "mismatched job UUID after ping update") - end - test "node should fail ping with invalid hostname config format" do Rails.configuration.Containers.SLURM.Managed.AssignNodeHostname = 'compute%04' # should end with "04d" post :ping, params: { diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb index efc43dfde5..db60b4e6e1 100644 --- a/services/api/test/unit/permission_test.rb +++ b/services/api/test/unit/permission_test.rb @@ -529,7 +529,13 @@ class PermissionTest < ActiveSupport::TestCase assert users(:active).can?(write: col.uuid) assert users(:active).can?(manage: col.uuid) - l3.destroy! + # Creating l3 should have automatically deleted l1 and upgraded to + # the max permission of {l1, l3}, i.e., can_manage (see #18693) so + # there should be no can_read link now. + refute Link.where(tail_uuid: l3.tail_uuid, + head_uuid: l3.head_uuid, + link_class: 'permission', + name: 'can_read').any? assert Collection.readable_by(users(:active)).where(uuid: col.uuid).first assert users(:active).can?(read: col.uuid) @@ -575,7 +581,13 @@ class PermissionTest < ActiveSupport::TestCase assert users(:active).can?(write: prj.uuid) assert users(:active).can?(manage: prj.uuid) - l3.destroy! + # Creating l3 should have automatically deleted l0 and upgraded to + # the max permission of {l0, l3}, i.e., can_manage (see #18693) so + # there should be no can_read link now. + refute Link.where(tail_uuid: l3.tail_uuid, + head_uuid: l3.head_uuid, + link_class: 'permission', + name: 'can_read').any? assert Group.readable_by(users(:active)).where(uuid: prj.uuid).first assert users(:active).can?(read: prj.uuid) -- 2.30.2