[ARVADOS] created: e46d4846dada9abd01ddffb1a8e2400449dec48a

git at public.curoverse.com git at public.curoverse.com
Mon Nov 17 15:30:58 EST 2014


        at  e46d4846dada9abd01ddffb1a8e2400449dec48a (commit)


commit e46d4846dada9abd01ddffb1a8e2400449dec48a
Author: Tom Clegg <tom at curoverse.com>
Date:   Mon Nov 17 15:17:59 2014 -0500

    4552: Tidy up ensure_unique_name block.

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 762bbd9..ed7e21e 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -78,7 +78,6 @@ class ApplicationController < ActionController::Base
 
   def create
     @object = model_class.new resource_attrs
-    retry_save = true
 
     if @object.respond_to? :name and params[:ensure_unique_name]
       # Record the original name.  See below.
@@ -86,40 +85,35 @@ class ApplicationController < ActionController::Base
       counter = 1
     end
 
-    while retry_save
-      begin
-        retry_save = false
-        @object.save!
-      rescue ActiveRecord::RecordNotUnique => rn
-        # Dig into the error to determine if it is specifically calling out a
-        # (owner_uuid, name) uniqueness violation.  In this specific case, and
-        # the client requested a unique name with ensure_unique_name==true,
-        # update the name field and try to save again.  Loop as necessary to
-        # discover a unique name.  It is necessary to handle name choosing at
-        # this level (as opposed to the client) to ensure that record creation
-        # never fails due to a race condition.
-        if rn.original_exception.is_a? PG::UniqueViolation
-          # Unfortunately ActiveRecord doesn't abstract out any of the
-          # necessary information to figure out if this the error is actually
-          # the specific case where we want to apply the ensure_unique_name
-          # behavior, so the following code is specialized to Postgres.
-          err = rn.original_exception
-          detail = err.result.error_field(PG::Result::PG_DIAG_MESSAGE_DETAIL)
-          if /^Key \(owner_uuid, name\)=\([a-z0-9]{5}-[a-z0-9]{5}-[a-z0-9]{15}, .*?\) already exists\./.match detail
-            logger.error "params[:ensure_unique_name] is #{params[:ensure_unique_name]}"
-            if params[:ensure_unique_name]
-              counter += 1
-              @object.uuid = nil
-              @object.name = "#{name_stem} (#{counter})"
-              retry_save = true
-            end
-          end
-        end
-        if not retry_save
-          raise
-        end
-      end
-    end
+    begin
+      @object.save!
+    rescue ActiveRecord::RecordNotUnique => rn
+      raise unless params[:ensure_unique_name]
+
+      # Dig into the error to determine if it is specifically calling out a
+      # (owner_uuid, name) uniqueness violation.  In this specific case, and
+      # the client requested a unique name with ensure_unique_name==true,
+      # update the name field and try to save again.  Loop as necessary to
+      # discover a unique name.  It is necessary to handle name choosing at
+      # this level (as opposed to the client) to ensure that record creation
+      # never fails due to a race condition.
+      raise unless rn.original_exception.is_a? PG::UniqueViolation
+
+      # Unfortunately ActiveRecord doesn't abstract out any of the
+      # necessary information to figure out if this the error is actually
+      # the specific case where we want to apply the ensure_unique_name
+      # behavior, so the following code is specialized to Postgres.
+      err = rn.original_exception
+      detail = err.result.error_field(PG::Result::PG_DIAG_MESSAGE_DETAIL)
+      raise unless /^Key \(owner_uuid, name\)=\([a-z0-9]{5}-[a-z0-9]{5}-[a-z0-9]{15}, .*?\) already exists\./.match detail
+
+      # OK, this exception really is just a unique name constraint
+      # violation, and we've been asked to ensure_unique_name.
+      counter += 1
+      @object.uuid = nil
+      @object.name = "#{name_stem} (#{counter})"
+      redo
+    end while false
     show
   end
 

commit ee756b104380760276607fedec3f4d1d86c27376
Author: Tom Clegg <tom at curoverse.com>
Date:   Mon Nov 17 14:20:14 2014 -0500

    4552: Test signing in collections.show.

diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index df51f56..2e306fc 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -16,6 +16,17 @@ class Arvados::V1::CollectionsControllerTest < ActionController::TestCase
            "basic Collections index included manifest_text")
   end
 
+  test "collections.get returns signed locators" do
+    permit_unsigned_manifests
+    authorize_with :active
+    get :show, {id: collections(:foo_file).uuid}
+    assert_response :success
+    json_response['manifest_text'].scan(/ [[:xdigit:]]{32}\S*/) do |tok|
+      assert_match(/\+A[[:xdigit:]]+@[[:xdigit:]]{8}\b/, tok,
+                   "Locator in manifest_text was not signed")
+    end
+  end
+
   test "index with manifest_text selected returns signed locators" do
     columns = %w(uuid owner_uuid manifest_text)
     authorize_with :active
@@ -116,7 +127,7 @@ EOS
 
       foo_collection = collections(:foo_file)
 
-      # Get foo_file using it's portable data has
+      # Get foo_file using its portable data hash
       get :show, {
         id: foo_collection[:portable_data_hash]
       }

commit fdb2dcbf4bc98540453fe1fbd0e832c106d817aa
Author: Tom Clegg <tom at curoverse.com>
Date:   Mon Nov 17 13:57:51 2014 -0500

    4552: Use faster database_cleaner strategy.

diff --git a/services/api/test/integration/websocket_test.rb b/services/api/test/integration/websocket_test.rb
index 7780ccb..d5808d8 100644
--- a/services/api/test/integration/websocket_test.rb
+++ b/services/api/test/integration/websocket_test.rb
@@ -3,7 +3,7 @@ require 'websocket_runner'
 require 'oj'
 require 'database_cleaner'
 
-DatabaseCleaner.strategy = :truncation
+DatabaseCleaner.strategy = :deletion
 
 class WebsocketTest < ActionDispatch::IntegrationTest
   self.use_transactional_fixtures = false

commit f330ec11af5d25ffd8464225023bf4169a6b6186
Author: Tom Clegg <tom at curoverse.com>
Date:   Mon Nov 17 13:48:30 2014 -0500

    4552: Fix conflict between ensure_unique_name and signed manifests.

diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index f546a4a..74ce1e9 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -26,7 +26,6 @@ class Arvados::V1::CollectionsController < ApplicationController
   end
 
   def show
-    sign_manifests(@object[:manifest_text])
     if @object.is_a? Collection
       render json: @object.as_api_response
     else
@@ -35,7 +34,6 @@ class Arvados::V1::CollectionsController < ApplicationController
   end
 
   def index
-    sign_manifests(*@objects.map { |c| c[:manifest_text] })
     super
   end
 
@@ -191,19 +189,4 @@ class Arvados::V1::CollectionsController < ApplicationController
     end
     super
   end
-
-  def sign_manifests(*manifests)
-    if current_api_client_authorization
-      signing_opts = {
-        key: Rails.configuration.blob_signing_key,
-        api_token: current_api_client_authorization.api_token,
-        ttl: Rails.configuration.blob_signing_ttl,
-      }
-      manifests.each do |text|
-        Collection.munge_manifest_locators(text) do |loc|
-          Blob.sign_locator(loc.to_s, signing_opts)
-        end
-      end
-    end
-  end
 end
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index accd2cc..4afa073 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -18,7 +18,7 @@ class Collection < ArvadosModel
     t.add :description
     t.add :properties
     t.add :portable_data_hash
-    t.add :manifest_text
+    t.add :signed_manifest_text, as: :manifest_text
   end
 
   def check_signatures
@@ -26,6 +26,13 @@ class Collection < ArvadosModel
 
     return true if current_user.andand.is_admin
 
+    # Provided the manifest_text hasn't changed materially since an
+    # earlier validation, it's safe to pass this validation on
+    # subsequent passes without checking any signatures. This is
+    # important because the signatures have probably been stripped off
+    # by the time we get to a second validation pass!
+    return true if @signatures_checked and @signatures_checked == compute_pdh
+
     if self.manifest_text_changed?
       # Check permissions on the collection manifest.
       # If any signature cannot be verified, raise PermissionDeniedError
@@ -61,13 +68,13 @@ class Collection < ArvadosModel
         end
       end
     end
-    true
+    @signatures_checked = compute_pdh
   end
 
   def strip_manifest_text
     if self.manifest_text_changed?
       # Remove any permission signatures from the manifest.
-      Collection.munge_manifest_locators(self[:manifest_text]) do |loc|
+      self.class.munge_manifest_locators!(self[:manifest_text]) do |loc|
         loc.without_signature.to_s
       end
     end
@@ -75,16 +82,20 @@ class Collection < ArvadosModel
   end
 
   def set_portable_data_hash
-    if (self.portable_data_hash.nil? or (self.portable_data_hash == "") or (manifest_text_changed? and !portable_data_hash_changed?))
-      self.portable_data_hash = "#{Digest::MD5.hexdigest(manifest_text)}+#{manifest_text.length}"
+    if (portable_data_hash.nil? or
+        portable_data_hash == "" or
+        (manifest_text_changed? and !portable_data_hash_changed?))
+      @need_pdh_validation = false
+      self.portable_data_hash = compute_pdh
     elsif portable_data_hash_changed?
+      @need_pdh_validation = true
       begin
         loc = Keep::Locator.parse!(self.portable_data_hash)
         loc.strip_hints!
         if loc.size
           self.portable_data_hash = loc.to_s
         else
-          self.portable_data_hash = "#{loc.hash}+#{self.manifest_text.length}"
+          self.portable_data_hash = "#{loc.hash}+#{portable_manifest_text.bytesize}"
         end
       rescue ArgumentError => e
         errors.add(:portable_data_hash, "#{e}")
@@ -95,15 +106,15 @@ class Collection < ArvadosModel
   end
 
   def ensure_hash_matches_manifest_text
-    if manifest_text_changed? or portable_data_hash_changed?
-      computed_hash = "#{Digest::MD5.hexdigest(manifest_text)}+#{manifest_text.length}"
-      unless computed_hash == portable_data_hash
-        logger.debug "(computed) '#{computed_hash}' != '#{portable_data_hash}' (provided)"
-        errors.add(:portable_data_hash, "does not match hash of manifest_text")
-        return false
-      end
+    return true unless manifest_text_changed? or portable_data_hash_changed?
+    # No need verify it if :set_portable_data_hash just computed it!
+    return true if not @need_pdh_validation
+    expect_pdh = compute_pdh
+    if expect_pdh != portable_data_hash
+      errors.add(:portable_data_hash,
+                 "does not match computed hash #{expect_pdh}")
+      return false
     end
-    true
   end
 
   def redundancy_status
@@ -122,7 +133,27 @@ class Collection < ArvadosModel
     end
   end
 
-  def self.munge_manifest_locators(manifest)
+  def signed_manifest_text
+    if has_attribute? :manifest_text
+      token ||= current_api_client_authorization.andand.api_token
+      @signed_manifest_text = self.class.sign_manifest manifest_text, token
+    end
+  end
+
+  def self.sign_manifest manifest, token
+    signing_opts = {
+      key: Rails.configuration.blob_signing_key,
+      api_token: token,
+      ttl: Rails.configuration.blob_signing_ttl,
+    }
+    m = manifest.dup
+    munge_manifest_locators!(m) do |loc|
+      Blob.sign_locator(loc.to_s, signing_opts)
+    end
+    return m
+  end
+
+  def self.munge_manifest_locators! manifest
     # Given a manifest text and a block, yield each locator,
     # and replace it with whatever the block returns.
     manifest.andand.gsub!(/ [[:xdigit:]]{32}(\+[[:digit:]]+)?(\+\S+)/) do |word|
@@ -206,4 +237,20 @@ class Collection < ArvadosModel
   def self.for_latest_docker_image(search_term, search_tag=nil, readers=nil)
     find_all_for_docker_image(search_term, search_tag, readers).first
   end
+
+  protected
+  def portable_manifest_text
+    portable_manifest = self[:manifest_text].dup
+    self.class.munge_manifest_locators!(portable_manifest) do |loc|
+      loc.hash + '+' + loc.size.to_s
+    end
+    portable_manifest
+  end
+
+  def compute_pdh
+    portable_manifest = portable_manifest_text
+    (Digest::MD5.hexdigest(portable_manifest) +
+     '+' +
+     portable_manifest.bytesize.to_s)
+  end
 end
diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index e5b17dd..df51f56 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -2,16 +2,6 @@ require 'test_helper'
 
 class Arvados::V1::CollectionsControllerTest < ActionController::TestCase
 
-  setup do
-    # Unless otherwise specified in the test, we want normal/secure behavior.
-    permit_unsigned_manifests false
-  end
-
-  teardown do
-    # Reset to secure behavior after each test.
-    permit_unsigned_manifests false
-  end
-
   def permit_unsigned_manifests isok=true
     # Set security model for the life of a test.
     Rails.configuration.permit_create_collection_with_unsigned_manifest = isok
@@ -177,21 +167,25 @@ EOS
            "Expected 'duplicate key' error in #{response_errors.first}")
   end
 
-  test "create succeeds with duplicate name with ensure_unique_name" do
-    permit_unsigned_manifests
-    authorize_with :active
-    manifest_text = ". d41d8cd98f00b204e9800998ecf8427e 0:0:foo.txt\n"
-    post :create, {
-      collection: {
-        owner_uuid: users(:active).uuid,
-        manifest_text: manifest_text,
-        portable_data_hash: "d30fe8ae534397864cb96c544f4cf102+47",
-        name: "owned_by_active"
-      },
-      ensure_unique_name: true
-    }
-    assert_response :success
-    assert_equal 'owned_by_active (2)', json_response['name']
+  [false, true].each do |unsigned|
+    test "create with duplicate name, ensure_unique_name, unsigned=#{unsigned}" do
+      permit_unsigned_manifests unsigned
+      authorize_with :active
+      manifest_text = ". acbd18db4cc2f85cedef654fccc4a4d8+3 0:0:foo.txt\n"
+      if !unsigned
+        manifest_text = Collection.sign_manifest manifest_text, api_token(:active)
+      end
+      post :create, {
+        collection: {
+          owner_uuid: users(:active).uuid,
+          manifest_text: manifest_text,
+          name: "owned_by_active"
+        },
+        ensure_unique_name: true
+      }
+      assert_response :success
+      assert_equal 'owned_by_active (2)', json_response['name']
+    end
   end
 
   test "create with owner_uuid set to group i can_manage" do

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list