From: Tom Clegg Date: Sat, 10 May 2014 18:34:32 +0000 (-0400) Subject: 2762: Protect owner_uuid referential integrity when changing uuids and X-Git-Tag: 1.1.0~2608^2~9^2~8 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/e19b02f69914c086f979ec31eb603bb3b456cd15 2762: Protect owner_uuid referential integrity when changing uuids and deleting users and groups. --- diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index 006eb90e12..69d329f02a 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -14,7 +14,6 @@ class ArvadosModel < ActiveRecord::Base before_save :ensure_ownership_path_leads_to_user before_destroy :ensure_owner_uuid_is_permitted before_destroy :ensure_permission_to_destroy - before_create :update_modified_by_fields before_update :maybe_update_modified_by_fields after_create :log_create diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb index 4d7f630053..eb11ffd202 100644 --- a/services/api/app/models/group.rb +++ b/services/api/app/models/group.rb @@ -1,7 +1,10 @@ +require 'can_be_an_owner' + class Group < ArvadosModel include AssignUuid include KindAndEtag include CommonApiTemplate + include CanBeAnOwner api_accessible :user, extend: :common do |t| t.add :name diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb index 6bba194c24..2badea0843 100644 --- a/services/api/app/models/user.rb +++ b/services/api/app/models/user.rb @@ -1,7 +1,11 @@ +require 'can_be_an_owner' + class User < ArvadosModel include AssignUuid include KindAndEtag include CommonApiTemplate + include CanBeAnOwner + serialize :prefs, Hash has_many :api_client_authorizations before_update :prevent_privilege_escalation diff --git a/services/api/lib/can_be_an_owner.rb b/services/api/lib/can_be_an_owner.rb new file mode 100644 index 0000000000..d242b4135a --- /dev/null +++ b/services/api/lib/can_be_an_owner.rb @@ -0,0 +1,46 @@ +# Protect + +module CanBeAnOwner + + def self.included(base) + # Rails' "has_many" can prevent us from destroying the owner + # record when other objects refer to it. + ActiveRecord::Base.connection.tables.each do |t| + next if t == base.table_name + next if t == 'schema_migrations' + klass = t.classify.constantize + next unless klass and 'owner_uuid'.in?(klass.columns.collect(&:name)) + base.has_many(t.to_sym, + foreign_key: :owner_uuid, + primary_key: :uuid, + dependent: :restrict) + end + # We need custom protection for changing an owner's primary + # key. (Apart from this restriction, admins are allowed to change + # UUIDs.) + base.validate :restrict_uuid_change_breaking_associations + end + + protected + + def restrict_uuid_change_breaking_associations + return true if new_record? or not uuid_changed? + + # Check for objects that have my old uuid listed as their owner. + self.class.reflect_on_all_associations(:has_many).each do |assoc| + next unless assoc.foreign_key == :owner_uuid + if assoc.klass.where(owner_uuid: uuid_was).any? + errors.add(:uuid, + "cannot be changed on a #{self.class} that owns objects") + return false + end + end + + # if I owned myself before, I'll just continue to own myself with + # my new uuid. + if owner_uuid == uuid_was + self.owner_uuid = uuid + end + end + +end diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb new file mode 100644 index 0000000000..2cb40630dd --- /dev/null +++ b/services/api/test/unit/owner_test.rb @@ -0,0 +1,112 @@ +require 'test_helper' + +# Test referential integrity: ensure we cannot leave any object +# without owners by deleting a user or group. +# +# "o" is an owner. +# "i" is an item. + +class OwnerTest < ActiveSupport::TestCase + fixtures :users, :groups, :specimens + + setup do + set_user_from_auth :admin_trustedclient + end + + User.all + Group.all + [User, Group].each do |o_class| + test "create object with legit #{o_class} owner" do + o = o_class.create + i = Specimen.create(owner_uuid: o.uuid) + assert i.valid?, "new item should pass validation" + assert i.uuid, "new item should have an ID" + assert Specimen.where(uuid: i.uuid).any?, "new item should really be in DB" + end + + [User, Group].each do |new_o_class| + test "change owner from legit #{o_class} to legit #{new_o_class} owner" do + o = o_class.create + i = Specimen.create(owner_uuid: o.uuid) + new_o = o_class.create + assert(Specimen.where(uuid: i.uuid).any?, + "new item should really be in DB") + assert(i.update_attributes(owner_uuid: new_o.uuid), + "should change owner_uuid from #{o.uuid} to #{new_o.uuid}") + end + end + + test "delete #{o_class} that owns nothing" do + o = o_class.create + assert(o_class.where(uuid: o.uuid).any?, + "new #{o_class} should really be in DB") + assert(o.destroy, "should delete #{o_class} that owns nothing") + assert_equal(false, o_class.where(uuid: o.uuid).any?, + "#{o.uuid} should not be in DB after deleting") + end + + test "change uuid of #{o_class} that owns nothing" do + # (we're relying on our admin credentials here) + o = o_class.create + assert(o_class.where(uuid: o.uuid).any?, + "new #{o_class} should really be in DB") + old_uuid = o.uuid + new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9]) + assert(o.update_attributes(uuid: new_uuid), + "should change #{o_class} uuid from #{old_uuid} to #{new_uuid}") + assert_equal(false, o_class.where(uuid: old_uuid).any?, + "#{old_uuid} should disappear when renamed to #{new_uuid}") + end + end + + ['users(:active)', 'groups(:afolder)'].each do |ofixt| + test "delete #{ofixt} that owns other objects" do + o = eval ofixt + assert_equal(true, Specimen.where(owner_uuid: o.uuid).any?, + "need something to be owned by #{o.uuid} for this test") + + assert_raises(ActiveRecord::DeleteRestrictionError, + "should not delete #{ofixt} that owns objects") do + o.destroy + end + end + + test "change uuid of #{ofixt} that owns other objects" do + o = eval ofixt + assert_equal(true, Specimen.where(owner_uuid: o.uuid).any?, + "need something to be owned by #{o.uuid} for this test") + old_uuid = o.uuid + new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9]) + assert(!o.update_attributes(uuid: new_uuid), + "should not change uuid of #{ofixt} that owns objects") + end + end + + test "delete User that owns self" do + o = User.create + assert User.where(uuid: o.uuid).any?, "new User should really be in DB" + assert_equal(true, o.update_attributes(owner_uuid: o.uuid), + "setting owner to self should work") + assert(o.destroy, "should delete User that owns self") + assert_equal(false, User.where(uuid: o.uuid).any?, + "#{o.uuid} should not be in DB after deleting") + end + + test "change uuid of User that owns self" do + o = User.create + assert User.where(uuid: o.uuid).any?, "new User should really be in DB" + assert_equal(true, o.update_attributes(owner_uuid: o.uuid), + "setting owner to self should work") + old_uuid = o.uuid + new_uuid = o.uuid.sub(/..........$/, rand(2**256).to_s(36)[0..9]) + assert(o.update_attributes(uuid: new_uuid), + "should change uuid of User that owns self") + assert_equal(false, User.where(uuid: old_uuid).any?, + "#{old_uuid} should not be in DB after deleting") + assert_equal(true, User.where(uuid: new_uuid).any?, + "#{new_uuid} should be in DB after renaming") + assert_equal(new_uuid, User.where(uuid: new_uuid).first.owner_uuid, + "#{new_uuid} should be its own owner in DB after renaming") + end + +end