[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