[ARVADOS] updated: 84d5db363e4c10881fdb5317b01f46bdc33c002a
git at public.curoverse.com
git at public.curoverse.com
Tue Nov 18 03:03:57 EST 2014
Summary of changes:
.../api/app/controllers/application_controller.rb | 6 ++++--
.../arvados/v1/collections_controller.rb | 6 +++---
services/api/app/models/collection.rb | 10 +++++++++-
.../arvados/v1/collections_controller_test.rb | 22 +++++++++++-----------
4 files changed, 27 insertions(+), 17 deletions(-)
discards e46d4846dada9abd01ddffb1a8e2400449dec48a (commit)
discards ee756b104380760276607fedec3f4d1d86c27376 (commit)
discards fdb2dcbf4bc98540453fe1fbd0e832c106d817aa (commit)
discards f330ec11af5d25ffd8464225023bf4169a6b6186 (commit)
via 84d5db363e4c10881fdb5317b01f46bdc33c002a (commit)
via dea90c956d96d06e39df0f6567c3bb6582a5b3dd (commit)
via 75a07d8175c9ee9a1c212cd1abe1c0d64a91cfc7 (commit)
This update added new revisions after undoing existing revisions. That is
to say, the old revision is not a strict subset of the new revision. This
situation occurs when you --force push a change and generate a repository
containing something like this:
* -- * -- B -- O -- O -- O (e46d4846dada9abd01ddffb1a8e2400449dec48a)
\
N -- N -- N (84d5db363e4c10881fdb5317b01f46bdc33c002a)
When this happens we assume that you've already had alert emails for all
of the O revisions, and so we here report only the revisions in the N
branch from the common base, B.
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 84d5db363e4c10881fdb5317b01f46bdc33c002a
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 03fda75..4f0364f 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -80,7 +80,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.
@@ -88,40 +87,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 dea90c956d96d06e39df0f6567c3bb6582a5b3dd
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 75a07d8175c9ee9a1c212cd1abe1c0d64a91cfc7
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/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 762bbd9..03fda75 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -1,9 +1,11 @@
module ApiTemplateOverride
def allowed_to_render?(fieldset, field, model, options)
+ return false if !super
if options[:select]
- return options[:select].include? field.to_s
+ options[:select].include? field.to_s
+ else
+ true
end
- super
end
end
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index f546a4a..7bc207f 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -16,7 +16,7 @@ class Arvados::V1::CollectionsController < ApplicationController
@object = {
uuid: c.portable_data_hash,
portable_data_hash: c.portable_data_hash,
- manifest_text: c.manifest_text,
+ manifest_text: c.signed_manifest_text,
}
end
else
@@ -26,16 +26,14 @@ class Arvados::V1::CollectionsController < ApplicationController
end
def show
- sign_manifests(@object[:manifest_text])
if @object.is_a? Collection
- render json: @object.as_api_response
+ super
else
render json: @object
end
end
def index
- sign_manifests(*@objects.map { |c| c[:manifest_text] })
super
end
@@ -183,7 +181,7 @@ class Arvados::V1::CollectionsController < ApplicationController
protected
- def apply_filters
+ def load_limit_offset_order_params *args
if action_name == 'index'
# Omit manifest_text from index results unless expressly selected.
@select ||= model_class.api_accessible_attributes(:user).
@@ -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..08c50d9 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -18,7 +18,15 @@ 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 self.attributes_required_columns
+ # If we don't list this explicitly, the params[:select] code gets
+ # confused by the way we expose signed_manifest_text as
+ # manifest_text in the API response, and never let clients select
+ # the manifest_text column.
+ super.merge('manifest_text' => ['manifest_text'])
end
def check_signatures
@@ -26,6 +34,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 +76,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 +90,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 +114,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 +141,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 +245,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..021918c 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -2,21 +2,19 @@ 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
end
+ def assert_signed_manifest manifest_text, label=''
+ assert_not_nil manifest_text, "#{label} manifest_text was nil"
+ manifest_text.scan(/ [[:xdigit:]]{32}\S*/) do |tok|
+ assert_match(/\+A[[:xdigit:]]+@[[:xdigit:]]{8}\b/, tok,
+ "Locator in #{label} manifest_text was not signed")
+ end
+ end
+
test "should get index" do
authorize_with :active
get :index
@@ -26,6 +24,14 @@ 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
+ assert_signed_manifest json_response['manifest_text'], 'foo_file'
+ end
+
test "index with manifest_text selected returns signed locators" do
columns = %w(uuid owner_uuid manifest_text)
authorize_with :active
@@ -36,13 +42,7 @@ class Arvados::V1::CollectionsControllerTest < ActionController::TestCase
json_response["items"].each do |coll|
assert_equal(columns, columns & coll.keys,
"Collections index did not respect selected columns")
- loc_regexp = / [[:xdigit:]]{32}\+\d+\S+/
- pos = 0
- while match = loc_regexp.match(coll["manifest_text"], pos)
- assert_match(/\+A[[:xdigit:]]+@[[:xdigit:]]{8}\b/, match.to_s,
- "Locator in manifest_text was not signed")
- pos = match.end(0)
- end
+ assert_signed_manifest coll['manifest_text'], coll['uuid']
end
end
@@ -126,7 +126,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]
}
@@ -134,6 +134,7 @@ EOS
assert_not_nil assigns(:object)
resp = assigns(:object)
assert_equal foo_collection[:portable_data_hash], resp['portable_data_hash']
+ assert_signed_manifest resp['manifest_text']
# The manifest in the response will have had permission hints added.
# Remove any permission hints in the response before comparing it to the source.
@@ -177,21 +178,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