[ARVADOS] created: e19b02f69914c086f979ec31eb603bb3b456cd15
git at public.curoverse.com
git at public.curoverse.com
Sat May 10 14:48:22 EDT 2014
at e19b02f69914c086f979ec31eb603bb3b456cd15 (commit)
commit e19b02f69914c086f979ec31eb603bb3b456cd15
Author: Tom Clegg <tom at curoverse.com>
Date: Sat May 10 14:34:32 2014 -0400
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 006eb90..69d329f 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 4d7f630..eb11ffd 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 6bba194..2badea0 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 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
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
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list