[ARVADOS] created: fadf2b0fe1240f607fc91d65d6831621262afeb1
git at public.curoverse.com
git at public.curoverse.com
Sun May 11 03:03:35 EDT 2014
at fadf2b0fe1240f607fc91d65d6831621262afeb1 (commit)
commit fadf2b0fe1240f607fc91d65d6831621262afeb1
Author: Tom Clegg <tom at curoverse.com>
Date: Sun May 11 03:02:57 2014 -0400
2762: Rename assign_uuid to has_uuid to reflect expanded scope.
diff --git a/services/api/app/models/api_client.rb b/services/api/app/models/api_client.rb
index 9689074..75a800b 100644
--- a/services/api/app/models/api_client.rb
+++ b/services/api/app/models/api_client.rb
@@ -1,5 +1,5 @@
class ApiClient < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
has_many :api_client_authorizations
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 4faa0d0..79237b8 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -1,4 +1,5 @@
-require 'assign_uuid'
+require 'has_uuid'
+
class ArvadosModel < ActiveRecord::Base
self.abstract_class = true
diff --git a/services/api/app/models/authorized_key.rb b/services/api/app/models/authorized_key.rb
index a6bc065..5856e0c 100644
--- a/services/api/app/models/authorized_key.rb
+++ b/services/api/app/models/authorized_key.rb
@@ -1,5 +1,5 @@
class AuthorizedKey < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
before_create :permission_to_set_authorized_user_uuid
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 600c075..745f0bf 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -1,5 +1,5 @@
class Collection < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index eb11ffd..f055716 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -1,7 +1,7 @@
require 'can_be_an_owner'
class Group < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
include CanBeAnOwner
diff --git a/services/api/app/models/human.rb b/services/api/app/models/human.rb
index 3717f81..32f2906 100644
--- a/services/api/app/models/human.rb
+++ b/services/api/app/models/human.rb
@@ -1,5 +1,5 @@
class Human < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :properties, Hash
diff --git a/services/api/app/models/job.rb b/services/api/app/models/job.rb
index a239893..fbc5640 100644
--- a/services/api/app/models/job.rb
+++ b/services/api/app/models/job.rb
@@ -1,5 +1,5 @@
class Job < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :script_parameters, Hash
diff --git a/services/api/app/models/job_task.rb b/services/api/app/models/job_task.rb
index 7d568e9..d5d2edd 100644
--- a/services/api/app/models/job_task.rb
+++ b/services/api/app/models/job_task.rb
@@ -1,5 +1,5 @@
class JobTask < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :parameters, Hash
diff --git a/services/api/app/models/keep_disk.rb b/services/api/app/models/keep_disk.rb
index 77fc627..ee05ec2 100644
--- a/services/api/app/models/keep_disk.rb
+++ b/services/api/app/models/keep_disk.rb
@@ -1,5 +1,5 @@
class KeepDisk < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
before_validation :ensure_ping_secret
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 8e83a15..af39185 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -1,5 +1,5 @@
class Link < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :properties, Hash
diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index 66ba1d7..6921eca 100644
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -1,5 +1,5 @@
class Log < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :properties, Hash
diff --git a/services/api/app/models/node.rb b/services/api/app/models/node.rb
index 21d249b..512f0e0 100644
--- a/services/api/app/models/node.rb
+++ b/services/api/app/models/node.rb
@@ -1,5 +1,5 @@
class Node < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :info, Hash
diff --git a/services/api/app/models/pipeline_instance.rb b/services/api/app/models/pipeline_instance.rb
index e5bd464..9ce0c4d 100644
--- a/services/api/app/models/pipeline_instance.rb
+++ b/services/api/app/models/pipeline_instance.rb
@@ -1,5 +1,5 @@
class PipelineInstance < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :components, Hash
diff --git a/services/api/app/models/pipeline_template.rb b/services/api/app/models/pipeline_template.rb
index 3b099ed..cd0e5cb 100644
--- a/services/api/app/models/pipeline_template.rb
+++ b/services/api/app/models/pipeline_template.rb
@@ -1,5 +1,5 @@
class PipelineTemplate < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :components, Hash
diff --git a/services/api/app/models/repository.rb b/services/api/app/models/repository.rb
index ad4a84d..f159b48 100644
--- a/services/api/app/models/repository.rb
+++ b/services/api/app/models/repository.rb
@@ -1,5 +1,5 @@
class Repository < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
diff --git a/services/api/app/models/specimen.rb b/services/api/app/models/specimen.rb
index bcfcd7a..d39c612 100644
--- a/services/api/app/models/specimen.rb
+++ b/services/api/app/models/specimen.rb
@@ -1,5 +1,5 @@
class Specimen < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :properties, Hash
diff --git a/services/api/app/models/trait.rb b/services/api/app/models/trait.rb
index 85ab236..a59c007 100644
--- a/services/api/app/models/trait.rb
+++ b/services/api/app/models/trait.rb
@@ -1,5 +1,5 @@
class Trait < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
serialize :properties, Hash
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 2badea0..8743b92 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -1,7 +1,7 @@
require 'can_be_an_owner'
class User < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
include CanBeAnOwner
diff --git a/services/api/app/models/virtual_machine.rb b/services/api/app/models/virtual_machine.rb
index d2830cf..094591e 100644
--- a/services/api/app/models/virtual_machine.rb
+++ b/services/api/app/models/virtual_machine.rb
@@ -1,5 +1,5 @@
class VirtualMachine < ArvadosModel
- include AssignUuid
+ include HasUuid
include KindAndEtag
include CommonApiTemplate
diff --git a/services/api/config/initializers/assign_uuid.rb b/services/api/config/initializers/assign_uuid.rb
deleted file mode 100644
index d3835db..0000000
--- a/services/api/config/initializers/assign_uuid.rb
+++ /dev/null
@@ -1 +0,0 @@
-require 'assign_uuid'
diff --git a/services/api/lib/assign_uuid.rb b/services/api/lib/has_uuid.rb
similarity index 98%
rename from services/api/lib/assign_uuid.rb
rename to services/api/lib/has_uuid.rb
index 3e7c377..21369d1 100644
--- a/services/api/lib/assign_uuid.rb
+++ b/services/api/lib/has_uuid.rb
@@ -1,4 +1,4 @@
-module AssignUuid
+module HasUuid
def self.included(base)
base.extend(ClassMethods)
commit cf32ef421c037692e7ae896f695c61d8e3670f01
Author: Tom Clegg <tom at curoverse.com>
Date: Sun May 11 02:55:45 2014 -0400
2762: When deleting an object, delete permissions and fail if other
links would be left dangling.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 69d329f..4faa0d0 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -26,7 +26,7 @@ class ArvadosModel < ActiveRecord::Base
# Note: This only returns permission links. It does not account for
# permissions obtained via user.is_admin or
# user.uuid==object.owner_uuid.
- has_many :permissions, :foreign_key => :head_uuid, :class_name => 'Link', :primary_key => :uuid, :conditions => "link_class = 'permission'"
+ has_many :permissions, :foreign_key => :head_uuid, :class_name => 'Link', :primary_key => :uuid, :conditions => "link_class = 'permission'", dependent: :destroy
class PermissionDeniedError < StandardError
def http_status
@@ -236,6 +236,11 @@ class ArvadosModel < ActiveRecord::Base
return true
end
+ def destroy_permission_links
+ Link.destroy_all(['link_class=? and (head_uuid=? or tail_uuid=?)',
+ 'permission', uuid, uuid])
+ end
+
def ensure_permission_to_destroy
raise PermissionDeniedError unless permission_to_destroy
end
diff --git a/services/api/lib/assign_uuid.rb b/services/api/lib/assign_uuid.rb
index 50738aa..3e7c377 100644
--- a/services/api/lib/assign_uuid.rb
+++ b/services/api/lib/assign_uuid.rb
@@ -3,6 +3,9 @@ module AssignUuid
def self.included(base)
base.extend(ClassMethods)
base.before_create :assign_uuid
+ base.before_destroy :destroy_permission_links
+ base.has_many :links_via_head, class_name: 'Link', foreign_key: :head_uuid, primary_key: :uuid, conditions: "not (link_class = 'permission')", dependent: :restrict
+ base.has_many :links_via_tail, class_name: 'Link', foreign_key: :tail_uuid, primary_key: :uuid, conditions: "not (link_class = 'permission')", dependent: :restrict
end
module ClassMethods
diff --git a/services/api/test/unit/link_test.rb b/services/api/test/unit/link_test.rb
index 72d6017..10f2b5e 100644
--- a/services/api/test/unit/link_test.rb
+++ b/services/api/test/unit/link_test.rb
@@ -4,7 +4,7 @@ class LinkTest < ActiveSupport::TestCase
fixtures :all
setup do
- Thread.current[:user] = users(:active)
+ set_user_from_auth :admin_trustedclient
end
test 'name links with the same tail_uuid must be unique' do
@@ -45,4 +45,16 @@ class LinkTest < ActiveSupport::TestCase
assert a.invalid?, "invalid name was accepted as valid?"
end
end
+
+ test "cannot delete an object referenced by links" do
+ ob = Specimen.create
+ link = Link.create(tail_uuid: users(:active).uuid,
+ head_uuid: ob.uuid,
+ link_class: 'test',
+ name: 'test')
+ assert_raises(ActiveRecord::DeleteRestrictionError,
+ "should not delete #{ob.uuid} with link #{link.uuid}") do
+ ob.destroy
+ end
+ end
end
commit 34ac761cb1fd2b7b9438765d14ded85dfab0453b
Author: Tom Clegg <tom at curoverse.com>
Date: Sat May 10 14:55:57 2014 -0400
2762: Test assigning random non-existent owner_uuids to new objects.
diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb
index 2cb4063..df96f31 100644
--- a/services/api/test/unit/owner_test.rb
+++ b/services/api/test/unit/owner_test.rb
@@ -24,6 +24,20 @@ class OwnerTest < ActiveSupport::TestCase
assert Specimen.where(uuid: i.uuid).any?, "new item should really be in DB"
end
+ test "create object with non-existent #{o_class} owner" do
+ assert_raises(ActiveRecord::RecordInvalid,
+ "create should fail with random owner_uuid") do
+ i = Specimen.create!(owner_uuid: o_class.generate_uuid)
+ end
+
+ i = Specimen.create(owner_uuid: o_class.generate_uuid)
+ assert !i.valid?, "object with random owner_uuid should not be valid?"
+
+ i = Specimen.new(owner_uuid: o_class.generate_uuid)
+ assert !i.valid?, "new item should not pass validation"
+ assert !i.uuid, "new item should not have an ID"
+ 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
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