[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