[ARVADOS] updated: 128506128f407047b3dc40e219cc9734afa7090a
git at public.curoverse.com
git at public.curoverse.com
Fri Aug 22 11:22:31 EDT 2014
Summary of changes:
.../api/app/controllers/application_controller.rb | 5 +-
.../arvados/v1/collections_controller.rb | 62 +--------------
.../controllers/arvados/v1/groups_controller.rb | 4 +
.../app/controllers/arvados/v1/jobs_controller.rb | 4 +-
services/api/app/helpers/collections_helper.rb | 8 --
services/api/app/models/arvados_model.rb | 2 +-
services/api/app/models/collection.rb | 89 ++++++++++++++++++----
services/api/app/models/link.rb | 3 +-
.../20140811184643_collection_use_regular_uuids.rb | 4 +-
services/api/lib/has_uuid.rb | 8 +-
.../arvados/v1/collections_controller_test.rb | 7 +-
11 files changed, 95 insertions(+), 101 deletions(-)
via 128506128f407047b3dc40e219cc9734afa7090a (commit)
from 86c4b60b5f9e8ab1f20eeee1da0afce3db57ab3b (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 128506128f407047b3dc40e219cc9734afa7090a
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date: Fri Aug 22 11:22:23 2014 -0400
3036: Move manifest_text validation into Collection model. Change
uuids_for_docker_image to find_all_for_docker_image which returns Collection
objects instead of uuids. Remove unused stripped_portable_data_hash helper.
Improved error messages. Tests pass.
diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 1934504..ad22acc 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -104,9 +104,8 @@ class ApplicationController < ActionController::Base
if e.respond_to? :backtrace and e.backtrace
logger.error e.backtrace.collect { |x| x + "\n" }.join('')
end
- if (@object and @object.respond_to? :errors and
- @object.errors and @object.errors.full_messages and
- not @object.errors.full_messages.empty?)
+ if (@object.respond_to? :errors and
+ @object.errors.andand.full_messages.andand.any?)
errors = @object.errors.full_messages
logger.error errors.inspect
else
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index d31e59a..23230c6 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -1,53 +1,9 @@
class Arvados::V1::CollectionsController < ApplicationController
def create
- if !resource_attrs[:manifest_text]
- return send_error("'manifest_text' attribute must be specified",
- status: :unprocessable_entity)
- end
-
if resource_attrs[:uuid] and (loc = Locator.parse(resource_attrs[:uuid]))
resource_attrs[:portable_data_hash] = loc.to_s
resource_attrs.delete :uuid
end
-
- # Check permissions on the collection manifest.
- # If any signature cannot be verified, return 403 Permission denied.
- api_token = current_api_client_authorization.andand.api_token
- signing_opts = {
- key: Rails.configuration.blob_signing_key,
- api_token: api_token,
- ttl: Rails.configuration.blob_signing_ttl,
- }
- resource_attrs[:manifest_text].lines.each do |entry|
- entry.split[1..-1].each do |tok|
- if /^[[:digit:]]+:[[:digit:]]+:/.match tok
- # This is a filename token, not a blob locator. Note that we
- # keep checking tokens after this, even though manifest
- # format dictates that all subsequent tokens will also be
- # filenames. Safety first!
- elsif Blob.verify_signature tok, signing_opts
- # OK.
- elsif Locator.parse(tok).andand.signature
- # Signature provided, but verify_signature did not like it.
- logger.warn "Invalid signature on locator #{tok}"
- raise ArvadosModel::PermissionDeniedError
- elsif Rails.configuration.permit_create_collection_with_unsigned_manifest
- # No signature provided, but we are running in insecure mode.
- logger.debug "Missing signature on locator #{tok} ignored"
- elsif Blob.new(tok).empty?
- # No signature provided -- but no data to protect, either.
- else
- logger.warn "Missing signature on locator #{tok}"
- raise ArvadosModel::PermissionDeniedError
- end
- end
- end
-
- # Remove any permission signatures from the manifest.
- munge_manifest_locators(resource_attrs[:manifest_text]) do |loc|
- loc.without_signature.to_s
- end
-
super
end
@@ -200,18 +156,6 @@ class Arvados::V1::CollectionsController < ApplicationController
render json: visited
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|
- if loc = Locator.parse(word.strip)
- " " + yield(loc)
- else
- " " + word
- end
- end
- end
-
protected
def find_objects_for_index
@@ -222,10 +166,6 @@ class Arvados::V1::CollectionsController < ApplicationController
super
end
- def munge_manifest_locators(manifest, &block)
- self.class.munge_manifest_locators(manifest, &block)
- end
-
def sign_manifests(*manifests)
if current_api_client_authorization
signing_opts = {
@@ -234,7 +174,7 @@ class Arvados::V1::CollectionsController < ApplicationController
ttl: Rails.configuration.blob_signing_ttl,
}
manifests.each do |text|
- munge_manifest_locators(text) do |loc|
+ Collection.munge_manifest_locators(text) do |loc|
Blob.sign_locator(loc.to_s, signing_opts)
end
end
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 7ee4e55..c1d414e 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -6,6 +6,8 @@ class Arvados::V1::GroupsController < ApplicationController
uuid: {
type: 'string', required: false, default: nil
},
+ # include_linked returns name links, which are obsolete, so
+ # remove it when clients have been migrated.
include_linked: {
type: 'boolean', required: false, default: false
},
@@ -34,6 +36,8 @@ class Arvados::V1::GroupsController < ApplicationController
def contents
# Set @objects:
+ # include_linked returns name links, which are obsolete, so
+ # remove it when clients have been migrated.
load_searchable_objects(owner_uuid: @object.andand.uuid,
include_linked: params[:include_linked])
sql = 'link_class=? and head_uuid in (?)'
diff --git a/services/api/app/controllers/arvados/v1/jobs_controller.rb b/services/api/app/controllers/arvados/v1/jobs_controller.rb
index 5045e07..fedaf27 100644
--- a/services/api/app/controllers/arvados/v1/jobs_controller.rb
+++ b/services/api/app/controllers/arvados/v1/jobs_controller.rb
@@ -214,9 +214,7 @@ class Arvados::V1::JobsController < ApplicationController
search_list = filter[2].is_a?(Enumerable) ? filter[2] : [filter[2]]
filter[2] = search_list.flat_map do |search_term|
image_search, image_tag = search_term.split(':', 2)
- Collection.uuids_for_docker_image(image_search, image_tag, @read_users).map do |uuid|
- Collection.find_by_uuid(uuid).portable_data_hash
- end
+ Collection.find_all_for_docker_image(image_search, image_tag, @read_users).map(&:portable_data_hash)
end
true
else
diff --git a/services/api/app/helpers/collections_helper.rb b/services/api/app/helpers/collections_helper.rb
index 020bdbe..3017985 100644
--- a/services/api/app/helpers/collections_helper.rb
+++ b/services/api/app/helpers/collections_helper.rb
@@ -1,10 +1,2 @@
module CollectionsHelper
- def stripped_portable_data_hash(uuid)
- m = /([a-f0-9]{32}(\+[0-9]+)?)(\+.*)?/.match(uuid)
- if m
- m[1]
- else
- nil
- end
- end
end
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index f95a35e..205f54a 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -240,7 +240,7 @@ class ArvadosModel < ActiveRecord::Base
rsc_class = ArvadosModel::resource_class_for_uuid owner_uuid
unless rsc_class == User or rsc_class == Group
- errors.add :owner_uuid, "can only be set to User or Group"
+ errors.add :owner_uuid, "must be set to User or Group"
raise PermissionDeniedError
end
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index c0c2b7e..f15d031 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -3,6 +3,8 @@ class Collection < ArvadosModel
include KindAndEtag
include CommonApiTemplate
+ before_validation :check_signatures
+ before_validation :strip_manifest_text
before_validation :set_portable_data_hash
validate :ensure_hash_matches_manifest_text
@@ -22,6 +24,59 @@ class Collection < ArvadosModel
})
end
+ def check_signatures
+ return false if self.manifest_text.nil?
+
+ return true if current_user.andand.is_admin
+
+ if self.manifest_text_changed?
+ # Check permissions on the collection manifest.
+ # If any signature cannot be verified, raise PermissionDeniedError
+ # which will return 403 Permission denied to the client.
+ api_token = current_api_client_authorization.andand.api_token
+ signing_opts = {
+ key: Rails.configuration.blob_signing_key,
+ api_token: api_token,
+ ttl: Rails.configuration.blob_signing_ttl,
+ }
+ self.manifest_text.lines.each do |entry|
+ entry.split[1..-1].each do |tok|
+ if /^[[:digit:]]+:[[:digit:]]+:/.match tok
+ # This is a filename token, not a blob locator. Note that we
+ # keep checking tokens after this, even though manifest
+ # format dictates that all subsequent tokens will also be
+ # filenames. Safety first!
+ elsif Blob.verify_signature tok, signing_opts
+ # OK.
+ elsif Locator.parse(tok).andand.signature
+ # Signature provided, but verify_signature did not like it.
+ logger.warn "Invalid signature on locator #{tok}"
+ raise ArvadosModel::PermissionDeniedError
+ elsif Rails.configuration.permit_create_collection_with_unsigned_manifest
+ # No signature provided, but we are running in insecure mode.
+ logger.debug "Missing signature on locator #{tok} ignored"
+ elsif Blob.new(tok).empty?
+ # No signature provided -- but no data to protect, either.
+ else
+ logger.warn "Missing signature on locator #{tok}"
+ raise ArvadosModel::PermissionDeniedError
+ end
+ end
+ end
+ end
+ true
+ 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|
+ loc.without_signature.to_s
+ end
+ end
+ true
+ 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}"
@@ -136,6 +191,18 @@ class Collection < ArvadosModel
end
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|
+ if loc = Locator.parse(word.strip)
+ " " + yield(loc)
+ else
+ " " + word
+ end
+ end
+ end
+
def self.normalize_uuid uuid
hash_part = nil
size_part = nil
@@ -152,7 +219,8 @@ class Collection < ArvadosModel
[hash_part, size_part].compact.join '+'
end
- def self.uuids_for_docker_image(search_term, search_tag=nil, readers=nil)
+ # Return array of Collection objects
+ def self.find_all_for_docker_image(search_term, search_tag=nil, readers=nil)
readers ||= [Thread.current[:user]]
base_search = Link.
readable_by(*readers).
@@ -167,7 +235,7 @@ class Collection < ArvadosModel
coll_match = readable_by(*readers).where(portable_data_hash: loc.to_s).limit(1).first
if coll_match and (coll_match.files.size == 1) and
(coll_match.files[0][1] =~ /^[0-9A-Fa-f]{64}\.tar$/)
- return [coll_match.uuid]
+ return [coll_match]
end
end
@@ -192,21 +260,14 @@ class Collection < ArvadosModel
# so that anything with an image timestamp is considered more recent than
# anything without; then we use the link's created_at as a tiebreaker.
uuid_timestamps = {}
- matches.find_each do |link|
- c = Collection.find_by_uuid(link.head_uuid)
- uuid_timestamps[c.uuid] =
- [(-link.properties["image_timestamp"].to_datetime.to_i rescue 0),
- -link.created_at.to_i]
+ matches.all.map do |link|
+ uuid_timestamps[link.head_uuid] = [(-link.properties["image_timestamp"].to_datetime.to_i rescue 0),
+ -link.created_at.to_i]
end
- uuid_timestamps.keys.sort_by { |uuid| uuid_timestamps[uuid] }
+ Collection.where('uuid in (?)', uuid_timestamps.keys).sort_by { |c| uuid_timestamps[c.uuid] }
end
def self.for_latest_docker_image(search_term, search_tag=nil, readers=nil)
- image_uuid = uuids_for_docker_image(search_term, search_tag, readers).first
- if image_uuid.nil?
- nil
- else
- find_by_uuid(image_uuid)
- end
+ find_all_for_docker_image(search_term, search_tag, readers).first
end
end
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index e319190..808489e 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -72,7 +72,8 @@ class Link < ArvadosModel
def name_links_are_obsolete
if link_class == 'name'
- errors.add('name', 'Name links are obsolete')
+ errors.add('name', 'Name links are obsolete')
+ false
else
true
end
diff --git a/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb b/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
index 5c21c0b..4f83eca 100644
--- a/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
+++ b/services/api/db/migrate/20140811184643_collection_use_regular_uuids.rb
@@ -3,7 +3,7 @@ class CollectionUseRegularUuids < ActiveRecord::Migration
add_column :collections, :name, :string
add_column :collections, :description, :string
add_column :collections, :properties, :text
- add_column :collections, :expire_time, :date
+ add_column :collections, :expires_at, :date
remove_column :collections, :locator
say_with_time "Step 1. Move manifest hashes into portable_data_hash field" do
@@ -175,6 +175,6 @@ where head_uuid like '________________________________+%' or tail_uuid like '___
end
def down
- # Not gonna happen.
+ raise ActiveRecord::IrreversibleMigration, "Can't downmigrate changes to collections and links without potentially losing data."
end
end
diff --git a/services/api/lib/has_uuid.rb b/services/api/lib/has_uuid.rb
index df175f8..06c7d0c 100644
--- a/services/api/lib/has_uuid.rb
+++ b/services/api/lib/has_uuid.rb
@@ -36,18 +36,18 @@ module HasUuid
if re[1] == self.class.uuid_prefix
return true
else
- self.errors.add(:uuid, "Matched uuid type '#{re[1]}', expected '#{self.class.uuid_prefix}'")
+ self.errors.add(:uuid, "type field is '#{re[1]}', expected '#{self.class.uuid_prefix}'")
return false
end
else
- self.errors.add(:uuid, "'#{self.uuid}' is not a valid Arvados UUID")
+ self.errors.add(:uuid, "not a valid Arvados uuid '#{self.uuid}'")
return false
end
else
if self.new_record?
- self.errors.add(:uuid, "Not permitted to specify uuid")
+ self.errors.add(:uuid, "assignment not permittid")
else
- self.errors.add(:uuid, "Not permitted to change uuid")
+ self.errors.add(:uuid, "change not permitted")
end
return false
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 78d3d74..65dfca5 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -102,9 +102,8 @@ class Arvados::V1::CollectionsControllerTest < ActionController::TestCase
assert_equal 99999, resp['offset']
end
- test "create with unsigned manifest" do
- permit_unsigned_manifests
- authorize_with :active
+ test "admin can create collection with unsigned manifest" do
+ authorize_with :admin
test_collection = {
manifest_text: <<-EOS
. d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo.txt
@@ -134,7 +133,7 @@ EOS
assert_response :success
assert_not_nil assigns(:object)
resp = JSON.parse(@response.body)
- assert_equal test_collection[:uuid], resp['uuid']
+ assert_equal test_collection[:portable_data_hash], resp['portable_data_hash']
# The manifest in the response will have had permission hints added.
# Remove any permission hints in the response before comparing it to the source.
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list