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 006eb90e12fd550e2110906a1b1c42cc64e291e8..69d329f02ad4c8d6d60356216e4d563de619ffcc 100644 (file)
--- 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 4d7f63005344019f2020ac75f59858cb635d4cb7..eb11ffd202293693b484757aea0fcdc2bcd6b21d 100644 (file)
--- 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 6bba194c24dac1f5460f5ce82fe90e87c202919c..2badea084337483d2d2387001d84cfa70f6d134b 100644 (file)
--- 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 (file)
index 0000000..d242b41
--- /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 (file)
index 0000000..2cb4063
--- /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