[ARVADOS] updated: 76e20e694fbe708e17e57a2e0b1c36aca6e8d7d0
git at public.curoverse.com
git at public.curoverse.com
Sat May 3 21:31:34 EDT 2014
Summary of changes:
doc/api/methods.html.textile.liquid | 10 ++-
.../api/app/controllers/application_controller.rb | 24 +++++-
.../app/controllers/arvados/v1/jobs_controller.rb | 2 +-
.../arvados/v1/repositories_controller.rb | 6 ++
.../controllers/arvados/v1/schema_controller.rb | 10 ++
services/api/app/models/arvados_model.rb | 93 ++++++++++++++------
services/api/app/models/link.rb | 2 +-
services/api/app/models/node.rb | 2 +-
services/api/lib/current_api_client.rb | 4 +-
services/api/lib/load_param.rb | 44 +++++++++-
services/api/test/fixtures/api_clients.yml | 2 +
services/api/test/fixtures/authorized_keys.yml | 15 +++
services/api/test/fixtures/groups.yml | 20 ++++
services/api/test/fixtures/links.yml | 28 ++++++
.../arvados/v1/repositories_controller_test.rb | 42 +++++++++
services/api/test/integration/select_test.rb | 82 +++++++++++++++++
services/api/test/test_helper.rb | 13 ++-
services/api/test/unit/group_test.rb | 60 ++++++++++++-
services/api/test/unit/log_test.rb | 7 --
services/api/test/unit/permission_test.rb | 17 ++++
20 files changed, 430 insertions(+), 53 deletions(-)
create mode 100644 services/api/test/fixtures/authorized_keys.yml
create mode 100644 services/api/test/integration/select_test.rb
create mode 100644 services/api/test/unit/permission_test.rb
via 76e20e694fbe708e17e57a2e0b1c36aca6e8d7d0 (commit)
via 988726079e8e2f8ce4b49115c10a8a1d22040972 (commit)
via 4e05647f7a5b3971771c5a928634c6b2a41aa591 (commit)
via 6c8ba53502c29dc9174291c04f3c7bc84777f9cc (commit)
via 307cf5007269a069b8c80dc14da134ec145cd292 (commit)
via 1d5a33d2af1e39041177681c8d66007b40b20df4 (commit)
via dca6cfe9750d8d1be4f3b63895b8cb73cc6c4cfe (commit)
via 0ed6856e2dd4c057e34ba5b2079cef0158ebc53f (commit)
via de54cdcea8dca015e3b08bb23f7221faa4814ef0 (commit)
via 0df7a1c38affbc50a9c7d8834f9822e398860d91 (commit)
via b4a9aaa3d68129d6e948dde46cee63cd66597aba (commit)
via fe78b629dc39298c5990a093e73b7c6231f387b0 (commit)
via 7a73008bcd015300461c34ce852d660f8244e11f (commit)
from 10d03fd3b09ec9f2eaea62cc665a1022f3744b89 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit 76e20e694fbe708e17e57a2e0b1c36aca6e8d7d0
Merge: 4e05647 9887260
Author: Tom Clegg <tom at curoverse.com>
Date: Sat May 3 21:25:19 2014 -0400
Merge branch 'master' into 2640-folder-api
diff --cc services/api/test/fixtures/links.yml
index 304e2d1,7d27f17..1385467
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@@ -233,9 -233,23 +233,23 @@@ foo_repository_readable_by_spectator
tail_uuid: zzzzz-tpzed-l1s2piq4t4mps8r
link_class: permission
name: can_read
- head_uuid: zzzzz-2x53u-382brsig8rp3666
+ head_uuid: zzzzz-s0uqq-382brsig8rp3666
properties: {}
+ foo_repository_writable_by_active:
+ uuid: zzzzz-o0j2j-8tdfjd8g0s4rn1k
+ owner_uuid: zzzzz-tpzed-000000000000000
+ created_at: 2014-01-24 20:42:26 -0800
+ modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+ modified_by_user_uuid: zzzzz-tpzed-000000000000000
+ modified_at: 2014-01-24 20:42:26 -0800
+ updated_at: 2014-01-24 20:42:26 -0800
+ tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ link_class: permission
+ name: can_write
+ head_uuid: zzzzz-2x53u-382brsig8rp3666
+ properties: {}
+
miniadmin_user_is_a_testusergroup_admin:
uuid: zzzzz-o0j2j-38vvkciz7qc12j9
owner_uuid: zzzzz-tpzed-000000000000000
commit 4e05647f7a5b3971771c5a928634c6b2a41aa591
Author: Tom Clegg <tom at curoverse.com>
Date: Sat May 3 21:08:18 2014 -0400
Prevent ownership cycles.
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 4f06f05..0b577d1 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -9,8 +9,10 @@ class ArvadosModel < ActiveRecord::Base
attr_protected :modified_by_client_uuid
attr_protected :modified_at
after_initialize :log_start_state
- before_create :ensure_permission_to_create
- before_update :ensure_permission_to_update
+ before_save :ensure_permission_to_save
+ before_save :ensure_owner_uuid_is_permitted
+ 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
@@ -130,16 +132,69 @@ class ArvadosModel < ActiveRecord::Base
protected
- def ensure_permission_to_create
- raise PermissionDeniedError unless permission_to_create
+ def ensure_ownership_path_leads_to_user
+ if new_record? or owner_uuid_changed?
+ uuid_in_path = {owner_uuid => true, uuid => true}
+ x = owner_uuid
+ while (owner_class = self.class.resource_class_for_uuid(x)) != User
+ begin
+ if x == uuid
+ # Test for cycles with the new version, not the DB contents
+ x = owner_uuid
+ else
+ x = owner_class.find_by_uuid(x).owner_uuid
+ end
+ rescue ActiveRecord::RecordNotFound => e
+ errors.add :owner_uuid, "is not owned by any user: #{e}"
+ return false
+ end
+ if uuid_in_path[x]
+ if x == owner_uuid
+ errors.add :owner_uuid, "would create an ownership cycle"
+ else
+ errors.add :owner_uuid, "has an ownership cycle"
+ end
+ return false
+ end
+ uuid_in_path[x] = true
+ end
+ end
+ true
end
- def permission_to_create
- current_user.andand.is_active
+ def ensure_owner_uuid_is_permitted
+ return false if !current_user
+ self.owner_uuid ||= current_user.uuid
+ if self.owner_uuid_changed?
+ if current_user.uuid == self.owner_uuid or
+ current_user.can? write: self.owner_uuid
+ # current_user is, or has :write permission on, the new owner
+ else
+ logger.warn "User #{current_user.uuid} tried to change owner_uuid of #{self.class.to_s} #{self.uuid} to #{self.owner_uuid} but does not have permission to write to #{self.owner_uuid}"
+ return false
+ end
+ end
+ if new_record?
+ return true
+ elsif current_user.uuid == self.owner_uuid_was or
+ current_user.uuid == self.uuid or
+ current_user.can? write: self.owner_uuid_was
+ # current user is, or has :write permission on, the previous owner
+ return true
+ else
+ logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} but does not have permission to write #{self.owner_uuid_was}"
+ raise PermissionDeniedError
+ end
+ end
+
+ def ensure_permission_to_save
+ unless (new_record? ? permission_to_create : permission_to_update)
+ raise PermissionDeniedError
+ end
end
- def ensure_permission_to_update
- raise PermissionDeniedError unless permission_to_update
+ def permission_to_create
+ current_user.andand.is_active
end
def permission_to_update
@@ -156,24 +211,7 @@ class ArvadosModel < ActiveRecord::Base
logger.warn "User #{current_user.uuid} tried to change uuid of #{self.class.to_s} #{self.uuid_was} to #{self.uuid}"
return false
end
- if self.owner_uuid_changed?
- if current_user.uuid == self.owner_uuid or
- current_user.can? write: self.owner_uuid
- # current_user is, or has :write permission on, the new owner
- else
- logger.warn "User #{current_user.uuid} tried to change owner_uuid of #{self.class.to_s} #{self.uuid} to #{self.owner_uuid} but does not have permission to write to #{self.owner_uuid}"
- return false
- end
- end
- if current_user.uuid == self.owner_uuid_was or
- current_user.uuid == self.uuid or
- current_user.can? write: self.owner_uuid_was
- # current user is, or has :write permission on, the previous owner
- return true
- else
- logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} but does not have permission to write #{self.owner_uuid_was}"
- return false
- end
+ return true
end
def ensure_permission_to_destroy
diff --git a/services/api/app/models/node.rb b/services/api/app/models/node.rb
index b88d4a5..21d249b 100644
--- a/services/api/app/models/node.rb
+++ b/services/api/app/models/node.rb
@@ -119,7 +119,7 @@ class Node < ArvadosModel
end
def start!(ping_url_method)
- ensure_permission_to_update
+ ensure_permission_to_save
ping_url = ping_url_method.call({ id: self.uuid, ping_secret: self.info[:ping_secret] })
if (Rails.configuration.compute_node_ec2run_args and
Rails.configuration.compute_node_ami)
diff --git a/services/api/lib/current_api_client.rb b/services/api/lib/current_api_client.rb
index 8e7d7fc..f851c58 100644
--- a/services/api/lib/current_api_client.rb
+++ b/services/api/lib/current_api_client.rb
@@ -44,7 +44,9 @@ module CurrentApiClient
def system_user
if not $system_user
real_current_user = Thread.current[:user]
- Thread.current[:user] = User.new(is_admin: true, is_active: true)
+ Thread.current[:user] = User.new(is_admin: true,
+ is_active: true,
+ uuid: system_user_uuid)
$system_user = User.where('uuid=?', system_user_uuid).first
if !$system_user
$system_user = User.new(uuid: system_user_uuid,
diff --git a/services/api/test/fixtures/api_clients.yml b/services/api/test/fixtures/api_clients.yml
index beb061c..79bddf0 100644
--- a/services/api/test/fixtures/api_clients.yml
+++ b/services/api/test/fixtures/api_clients.yml
@@ -2,12 +2,14 @@
trusted_workbench:
uuid: zzzzz-ozdt8-teyxzyd8qllg11h
+ owner_uuid: zzzzz-tpzed-000000000000000
name: Official Workbench
url_prefix: https://official-workbench.local/
is_trusted: true
untrusted:
uuid: zzzzz-ozdt8-obw7foaks3qjyej
+ owner_uuid: zzzzz-tpzed-000000000000000
name: Untrusted
url_prefix: https://untrusted.local/
is_trusted: false
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 947d762..96db93c 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -67,3 +67,23 @@ asubfolder:
name: A Subfolder
description: "Test folder belonging to active user's first test folder"
group_class: folder
+
+bad_group_has_ownership_cycle_a:
+ uuid: zzzzz-j7d0g-cx2al9cqkmsf1hs
+ owner_uuid: zzzzz-j7d0g-0077nzts8c178lw
+ modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+ modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ created_at: 2014-05-03 18:50:08 -0400
+ modified_at: 2014-05-03 18:50:08 -0400
+ updated_at: 2014-05-03 18:50:08 -0400
+ name: Owned by bad group b
+
+bad_group_has_ownership_cycle_b:
+ uuid: zzzzz-j7d0g-0077nzts8c178lw
+ owner_uuid: zzzzz-j7d0g-cx2al9cqkmsf1hs
+ modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+ modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ created_at: 2014-05-03 18:50:08 -0400
+ modified_at: 2014-05-03 18:50:08 -0400
+ updated_at: 2014-05-03 18:50:08 -0400
+ name: Owned by bad group a
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index 87997df..304e2d1 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -345,3 +345,17 @@ foo_collection_tag:
link_class: tag
name: foo_tag
properties: {}
+
+active_user_can_manage_bad_group_cx2al9cqkmsf1hs:
+ uuid: zzzzz-o0j2j-ezv55ahzc9lvjwe
+ owner_uuid: zzzzz-tpzed-000000000000000
+ created_at: 2014-05-03 18:50:08 -0400
+ modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+ modified_by_user_uuid: zzzzz-tpzed-000000000000000
+ modified_at: 2014-05-03 18:50:08 -0400
+ updated_at: 2014-05-03 18:50:08 -0400
+ tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+ link_class: permission
+ name: can_manage
+ head_uuid: zzzzz-j7d0g-cx2al9cqkmsf1hs
+ properties: {}
diff --git a/services/api/test/test_helper.rb b/services/api/test/test_helper.rb
index 869d87f..0e8c56b 100644
--- a/services/api/test/test_helper.rb
+++ b/services/api/test/test_helper.rb
@@ -33,6 +33,13 @@ class ActiveSupport::TestCase
Thread.current[:user] = nil
end
+ def set_user_from_auth(auth_name)
+ client_auth = api_client_authorizations(auth_name)
+ Thread.current[:api_client_authorization] = client_auth
+ Thread.current[:api_client] = client_auth.api_client
+ Thread.current[:user] = client_auth.user
+ end
+
def expect_json
self.request.headers["Accept"] = "text/json"
end
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 778eb0c..6b419ad 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -1,7 +1,61 @@
require 'test_helper'
class GroupTest < ActiveSupport::TestCase
- # test "the truth" do
- # assert true
- # end
+
+ test "cannot set owner_uuid to object with existing ownership cycle" do
+ set_user_from_auth :active_trustedclient
+
+ # First make sure we have lots of permission on the bad group
+ g = groups(:bad_group_has_ownership_cycle_b)
+ g.name += " xyz"
+ assert g.save, "active user should be able to modify group #{g.uuid}"
+
+ # Use the group as the owner of a new object
+ s = Specimen.
+ create(owner_uuid: groups(:bad_group_has_ownership_cycle_b).uuid)
+ assert s.valid?, "ownership should pass validation"
+ assert_equal false, s.save, "should not save object with #{g.uuid} as owner"
+
+ # Use the group as the new owner of an existing object
+ s = specimens(:in_afolder)
+ s.owner_uuid = groups(:bad_group_has_ownership_cycle_b).uuid
+ assert s.valid?, "ownership should pass validation"
+ assert_equal false, s.save, "should not save object with #{g.uuid} as owner"
+ end
+
+ test "cannot create a new ownership cycle" do
+ set_user_from_auth :active_trustedclient
+
+ g_foo = Group.create(name: "foo")
+ g_foo.save!
+
+ g_bar = Group.create(name: "bar")
+ g_bar.save!
+
+ g_foo.owner_uuid = g_bar.uuid
+ assert g_foo.save, lambda { g_foo.errors.messages }
+ g_bar.owner_uuid = g_foo.uuid
+ assert g_bar.valid?, "ownership cycle should not prevent validation"
+ assert_equal false, g_bar.save, "should not create an ownership loop"
+ assert g_bar.errors.messages[:owner_uuid].join(" ").match(/ownership cycle/)
+ end
+
+ test "cannot create a single-object ownership cycle" do
+ set_user_from_auth :active_trustedclient
+
+ g_foo = Group.create(name: "foo")
+ assert g_foo.save
+
+ # Ensure I have permission to manage this group even when its owner changes
+ perm_link = Link.create(tail_uuid: users(:active).uuid,
+ head_uuid: g_foo.uuid,
+ link_class: 'permission',
+ name: 'can_manage')
+ assert perm_link.save
+
+ g_foo.owner_uuid = g_foo.uuid
+ assert_equal false, g_foo.save, "should not create an ownership loop"
+ assert g_foo.errors.messages[:owner_uuid].join(" ").match(/ownership cycle/)
+ end
+
end
diff --git a/services/api/test/unit/log_test.rb b/services/api/test/unit/log_test.rb
index be8498d..4fc273b 100644
--- a/services/api/test/unit/log_test.rb
+++ b/services/api/test/unit/log_test.rb
@@ -65,13 +65,6 @@ class LogTest < ActiveSupport::TestCase
end
end
- def set_user_from_auth(auth_name)
- client_auth = api_client_authorizations(auth_name)
- Thread.current[:api_client_authorization] = client_auth
- Thread.current[:api_client] = client_auth.api_client
- Thread.current[:user] = client_auth.user
- end
-
test "creating a user makes a log" do
set_user_from_auth :admin_trustedclient
u = User.new(first_name: "Log", last_name: "Test")
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list