Merge branch 'ticket/2.6.next/5838' into 2.6.next

* ticket/2.6.next/5838:
  (#5838) Support paths as part of file bucket requests.
  (#5838) Improve the quality of file bucket specs.
  (#5838) Make file bucket dipper efficient when saving a file that already exists
  (#5838) Implemented the "head" method for FileBucketFile::File terminus.
  (#5838) Reworked file dipper spec to perform less stubbing.
  (#5838) Added support for HEAD requests to the indirector.
  (#5838) Refactored error handling logic into find_in_cache.
  (#5838) Refactored Puppet::Network::Rights#fail_on_deny

10 files changed, 143 insertions, 216 deletions

diff --git a/lib/puppet/file_bucket/dipper.rb b/lib/puppet/file_bucket/dipper.rb
index dbfcdcd43..f4bef28a8 100644
--- a/lib/puppet/file_bucket/dipper.rb
+++ b/lib/puppet/file_bucket/dipper.rb
@@ -33,10 +33,15 @@ class Puppet::FileBucket::Dipper
     raise(ArgumentError, "File #{file} does not exist") unless ::File.exist?(file)
     contents = ::File.read(file)
     begin
-      file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path, :path => absolutize_path(file) )
+      file_bucket_file = Puppet::FileBucket::File.new(contents, :bucket_path => @local_path)
       dest_path = "#{@rest_path}#{file_bucket_file.name}"
 
-      file_bucket_file.save(dest_path)
+      # Make a HEAD request for the file so that we don't waste time
+      # uploading it if it already exists in the bucket.
+      unless Puppet::FileBucket::File.head("#{@rest_path}#{file_bucket_file.checksum_type}/#{file_bucket_file.checksum_data}")
+        file_bucket_file.save(dest_path)
+      end
+
       return file_bucket_file.checksum_data
     rescue => detail
       puts detail.backtrace if Puppet[:trace]
diff --git a/lib/puppet/file_bucket/file.rb b/lib/puppet/file_bucket/file.rb
index 96fd8e225..08c0329f1 100644
--- a/lib/puppet/file_bucket/file.rb
+++ b/lib/puppet/file_bucket/file.rb
@@ -1,10 +1,9 @@
 require 'puppet/file_bucket'
 require 'puppet/indirector'
 require 'puppet/util/checksums'
+require 'digest/md5'
 
 class Puppet::FileBucket::File
-  include Puppet::Util::Checksums
-
   # This class handles the abstract notion of a file in a filebucket.
   # There are mechanisms to save and load this file locally and remotely in puppet/indirector/filebucketfile/*
   # There is a compatibility class that emulates pre-indirector filebuckets in Puppet::FileBucket::Dipper
@@ -12,71 +11,27 @@ class Puppet::FileBucket::File
   require 'puppet/file_bucket/file/indirection_hooks'
   indirects :file_bucket_file, :terminus_class => :file, :extend => Puppet::FileBucket::File::IndirectionHooks
 
-  attr :path, true
-  attr :paths, true
-  attr :contents, true
-  attr :checksum_type
-  attr :bucket_path, true
-
-  def self.default_checksum_type
-    "md5"
-  end
+  attr :contents
+  attr :bucket_path
 
   def initialize( contents, options = {} )
-    @bucket_path   = options[:bucket_path]
-    @path          = options[:path]
-    @paths         = options[:paths] || []
-
-    @checksum      = options[:checksum]
-    @checksum_type = options[:checksum_type]
-
-    self.contents  = contents
-
-    yield(self) if block_given?
-
-    validate!
-  end
+    raise ArgumentError if !contents.is_a?(String)
+    @contents  = contents
 
-  def validate!
-    validate_checksum_type!(checksum_type)
-    validate_checksum!(checksum) if checksum
+    @bucket_path = options.delete(:bucket_path)
+    raise ArgumentError if options != {}
   end
 
-  def contents=(str)
-    raise "You may not change the contents of a FileBucket File" if @contents
-    validate_content!(str)
-    @contents = str
+  def checksum_type
+    'md5'
   end
 
   def checksum
-    return @checksum if @checksum
-    @checksum = calculate_checksum if contents
-    @checksum
-  end
-
-  def checksum=(checksum)
-    validate_checksum!(checksum)
-    @checksum = checksum
-  end
-
-  def checksum_type=( new_checksum_type )
-    @checksum = nil
-    @checksum_type = new_checksum_type
-  end
-
-  def checksum_type
-    unless @checksum_type
-      if @checksum
-        @checksum_type = sumtype(checksum)
-      else
-        @checksum_type = self.class.default_checksum_type
-      end
-    end
-    @checksum_type
+    "{#{checksum_type}}#{checksum_data}"
   end
 
   def checksum_data
-    sumdata(checksum)
+    @checksum_data ||= Digest::MD5.hexdigest(contents)
   end
 
   def to_s
@@ -84,18 +39,7 @@ class Puppet::FileBucket::File
   end
 
   def name
-    [checksum_type, checksum_data, path].compact.join('/')
-  end
-
-  def name=(name)
-    data = name.split('/',3)
-    self.path = data.pop
-    @checksum_type = nil
-    self.checksum = "{#{data[0]}}#{data[1]}"
-  end
-
-  def conflict_check?
-    true
+    "#{checksum_type}/#{checksum_data}"
   end
 
   def self.from_s( contents )
@@ -103,34 +47,10 @@ class Puppet::FileBucket::File
   end
 
   def to_pson
-    hash = { "contents" => contents }
-    hash["path"] = @path if @path
-    hash.to_pson
+    { "contents" => contents }.to_pson
   end
 
   def self.from_pson( pson )
-    self.new( pson["contents"], :path => pson["path"] )
-  end
-
-  private
-
-  def calculate_checksum
-    "{#{checksum_type}}" + send(checksum_type, contents)
-  end
-
-  def validate_content!(content)
-    raise ArgumentError, "Contents must be a string" if content and ! content.is_a?(String)
-  end
-
-  def validate_checksum!(new_checksum)
-    newtype = sumtype(new_checksum)
-
-    unless sumdata(new_checksum) == (calc_sum = send(newtype, contents))
-      raise Puppet::Error, "Checksum #{new_checksum} does not match contents #{calc_sum}"
-    end
-  end
-
-  def validate_checksum_type!(type)
-    raise ArgumentError, "Invalid checksum type #{type}" unless respond_to?(type)
+    self.new( pson["contents"] )
   end
 end
diff --git a/lib/puppet/indirector.rb b/lib/puppet/indirector.rb
index 5b737578b..e6472f4d9 100644
--- a/lib/puppet/indirector.rb
+++ b/lib/puppet/indirector.rb
@@ -50,6 +50,10 @@ module Puppet::Indirector
       indirection.find(*args)
     end
 
+    def head(*args)
+      indirection.head(*args)
+    end
+
     def destroy(*args)
       indirection.destroy(*args)
     end
diff --git a/lib/puppet/indirector/file_bucket_file/file.rb b/lib/puppet/indirector/file_bucket_file/file.rb
index 9d9cee793..8bea2d767 100644
--- a/lib/puppet/indirector/file_bucket_file/file.rb
+++ b/lib/puppet/indirector/file_bucket_file/file.rb
@@ -14,25 +14,31 @@ module Puppet::FileBucketFile
     end
 
     def find( request )
-      checksum, path = request_to_checksum_and_path( request )
-      file = find_by_checksum( checksum, request.options )
+      checksum = request_to_checksum( request )
+      file_path = path_for(request.options[:bucket_path], checksum, 'contents')
 
-      if file && request.options[:diff_with]
+      return nil unless ::File.exists?(file_path)
+
+      if request.options[:diff_with]
         hash_protocol = sumtype(checksum)
-        file2 = find_by_checksum( "{#{hash_protocol}}#{request.options[:diff_with]}", request.options )
-        raise "could not find diff_with #{request.options[:diff_with]}" unless file2
-        return `diff #{path_for(file).inspect}/contents #{path_for(file2).inspect}/contents`
+        file2_path = path_for(request.options[:bucket_path], request.options[:diff_with], 'contents')
+        raise "could not find diff_with #{request.options[:diff_with]}" unless ::File.exists?(file2_path)
+        return `diff #{file_path.inspect} #{file2_path.inspect}`
+      else
+        contents = ::File.read file_path
+        Puppet.info "FileBucket read #{checksum}"
+        model.new(contents)
       end
+    end
 
-      file
+    def head(request)
+      checksum = request_to_checksum(request)
+      file_path = path_for(request.options[:bucket_path], checksum, 'contents')
+      ::File.exists?(file_path)
     end
 
     def save( request )
-      checksum, path = request_to_checksum_and_path( request )
-
       instance = request.instance
-      instance.checksum = checksum if checksum
-      instance.path = path if path
 
       save_to_disk(instance)
       instance.to_s
@@ -40,66 +46,41 @@ module Puppet::FileBucketFile
 
     private
 
-    def find_by_checksum( checksum, options )
-      model.new( nil, :checksum => checksum ) do |bucket_file|
-        bucket_file.bucket_path = options[:bucket_path]
-        filename = contents_path_for( bucket_file )
-
-        return nil if ! ::File.exist? filename
-
-        begin
-          contents = ::File.read filename
-          Puppet.info "FileBucket read #{bucket_file.checksum}"
-        rescue RuntimeError => e
-          raise Puppet::Error, "file could not be read: #{e.message}"
-        end
-
-        if ::File.exist?(paths_path_for( bucket_file) )
-          ::File.open(paths_path_for( bucket_file) ) do |f|
-            bucket_file.paths = f.readlines.map { |l| l.chomp }
-          end
-        end
-
-        bucket_file.contents = contents
-      end
-    end
-
     def save_to_disk( bucket_file )
-      # If the file already exists, just return the md5 sum.
-      if ::File.exist?(contents_path_for( bucket_file) )
+      filename = path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents')
+      dirname = path_for(bucket_file.bucket_path, bucket_file.checksum_data)
+
+      # If the file already exists, do nothing.
+      if ::File.exist?(filename)
         verify_identical_file!(bucket_file)
       else
         # Make the directories if necessary.
-        unless ::File.directory?( path_for( bucket_file) )
+        unless ::File.directory?(dirname)
           Puppet::Util.withumask(0007) do
-            ::FileUtils.mkdir_p( path_for( bucket_file) )
+            ::FileUtils.mkdir_p(dirname)
           end
         end
 
-        Puppet.info "FileBucket adding #{bucket_file.path} as #{bucket_file.checksum}"
+        Puppet.info "FileBucket adding #{bucket_file.checksum}"
 
         # Write the file to disk.
         Puppet::Util.withumask(0007) do
-          ::File.open(contents_path_for(bucket_file), ::File::WRONLY|::File::CREAT, 0440) do |of|
+          ::File.open(filename, ::File::WRONLY|::File::CREAT, 0440) do |of|
             of.print bucket_file.contents
           end
         end
       end
-
-      save_path_to_paths_file(bucket_file)
-      bucket_file.checksum_data
     end
 
-    def request_to_checksum_and_path( request )
-      return [request.key, nil] if checksum?(request.key)
-
-      checksum_type, checksum, path = request.key.split(/\//, 3)
-      return(checksum_type.to_s == "" ? nil : [ "{#{checksum_type}}#{checksum}", path ])
+    def request_to_checksum( request )
+      checksum_type, checksum, path = request.key.split(/\//, 3) # Note: we ignore path if present.
+      raise "Unsupported checksum type #{checksum_type.inspect}" if checksum_type != 'md5'
+      raise "Invalid checksum #{checksum.inspect}" if checksum !~ /^[0-9a-f]{32}$/
+      checksum
     end
 
-    def path_for(bucket_file, subfile = nil)
-      bucket_path = bucket_file.bucket_path || Puppet[:bucketdir]
-      digest      = bucket_file.checksum_data
+    def path_for(bucket_path, digest, subfile = nil)
+      bucket_path ||= Puppet[:bucketdir]
 
       dir     = ::File.join(digest[0..7].split(""))
       basedir = ::File.join(bucket_path, dir, digest)
@@ -108,48 +89,18 @@ module Puppet::FileBucketFile
       ::File.join(basedir, subfile)
     end
 
-    def contents_path_for(bucket_file)
-      path_for(bucket_file, "contents")
-    end
-
-    def paths_path_for(bucket_file)
-      path_for(bucket_file, "paths")
-    end
-
-    def content_check?
-      true
-    end
-
     # If conflict_check is enabled, verify that the passed text is
     # the same as the text in our file.
     def verify_identical_file!(bucket_file)
-      return unless content_check?
-      disk_contents = ::File.read(contents_path_for(bucket_file))
+      disk_contents = ::File.read(path_for(bucket_file.bucket_path, bucket_file.checksum_data, 'contents'))
 
       # If the contents don't match, then we've found a conflict.
       # Unlikely, but quite bad.
       if disk_contents != bucket_file.contents
-        raise Puppet::FileBucket::BucketError, "Got passed new contents for sum #{bucket_file.checksum}", caller
+        raise Puppet::FileBucket::BucketError, "Got passed new contents for sum #{bucket_file.checksum}"
       else
-        Puppet.info "FileBucket got a duplicate file #{bucket_file.path} (#{bucket_file.checksum})"
+        Puppet.info "FileBucket got a duplicate file #{bucket_file.checksum}"
       end
     end
-
-    def save_path_to_paths_file(bucket_file)
-      return unless bucket_file.path
-
-      # check for dupes
-      if ::File.exist?(paths_path_for( bucket_file) )
-        ::File.open(paths_path_for( bucket_file) ) do |f|
-          return if f.readlines.collect { |l| l.chomp }.include?(bucket_file.path)
-        end
-      end
-
-      # if it's a new file, or if our path isn't in the file yet, add it
-      ::File.open(paths_path_for(bucket_file), ::File::WRONLY|::File::CREAT|::File::APPEND) do |of|
-        of.puts bucket_file.path
-      end
-    end
-
   end
 end
diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index a010c4e40..ec147ec69 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -180,13 +180,8 @@ class Puppet::Indirector::Indirection
     request = request(:find, key, *args)
     terminus = prepare(request)
 
-    begin
-      if result = find_in_cache(request)
-        return result
-      end
-    rescue => detail
-      puts detail.backtrace if Puppet[:trace]
-      Puppet.err "Cached #{self.name} for #{request.key} failed: #{detail}"
+    if result = find_in_cache(request)
+      return result
     end
 
     # Otherwise, return the result from the terminus, caching if appropriate.
@@ -203,6 +198,17 @@ class Puppet::Indirector::Indirection
     nil
   end
 
+  # Search for an instance in the appropriate terminus, and return a
+  # boolean indicating whether the instance was found.
+  def head(key, *args)
+    request = request(:head, key, *args)
+    terminus = prepare(request)
+
+    # Look in the cache first, then in the terminus.  Force the result
+    # to be a boolean.
+    !!(find_in_cache(request) || terminus.head(request))
+  end
+
   def find_in_cache(request)
     # See if our instance is in the cache and up to date.
     return nil unless cache? and ! request.ignore_cache? and cached = cache.find(request)
@@ -213,6 +219,10 @@ class Puppet::Indirector::Indirection
 
     Puppet.debug "Using cached #{self.name} for #{request.key}"
     cached
+  rescue => detail
+    puts detail.backtrace if Puppet[:trace]
+    Puppet.err "Cached #{self.name} for #{request.key} failed: #{detail}"
+    nil
   end
 
   # Remove something via the terminus.
diff --git a/lib/puppet/indirector/rest.rb b/lib/puppet/indirector/rest.rb
index eb41ff3b1..e50dc68ae 100644
--- a/lib/puppet/indirector/rest.rb
+++ b/lib/puppet/indirector/rest.rb
@@ -53,11 +53,15 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
       end
     else
       # Raise the http error if we didn't get a 'success' of some kind.
-      message = "Error #{response.code} on SERVER: #{(response.body||'').empty? ? response.message : uncompress_body(response)}"
-      raise Net::HTTPError.new(message, response)
+      raise convert_to_http_error(response)
     end
   end
 
+  def convert_to_http_error(response)
+    message = "Error #{response.code} on SERVER: #{(response.body||'').empty? ? response.message : uncompress_body(response)}"
+    Net::HTTPError.new(message, response)
+  end
+
   # Provide appropriate headers.
   def headers
     add_accept_encoding({"Accept" => model.supported_formats.join(", ")})
@@ -73,6 +77,19 @@ class Puppet::Indirector::REST < Puppet::Indirector::Terminus
     result
   end
 
+  def head(request)
+    response = network(request).head(indirection2uri(request), headers)
+    case response.code
+    when "404"
+      return false
+    when /^2/
+      return true
+    else
+      # Raise the http error if we didn't get a 'success' of some kind.
+      raise convert_to_http_error(response)
+    end
+  end
+
   def search(request)
     unless result = deserialize(network(request).get(indirection2uri(request), headers), true)
       return []
diff --git a/lib/puppet/network/http/api/v1.rb b/lib/puppet/network/http/api/v1.rb
index dd4612a14..8aa1f0ee1 100644
--- a/lib/puppet/network/http/api/v1.rb
+++ b/lib/puppet/network/http/api/v1.rb
@@ -13,6 +13,9 @@ module Puppet::Network::HTTP::API::V1
     },
     "DELETE" => {
       :singular => :destroy
+    },
+    "HEAD" => {
+      :singular => :head
     }
   }
 
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index f22498b70..9e9356b2f 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -116,6 +116,17 @@ module Puppet::Network::HTTP::Handler
     end
   end
 
+  # Execute our head.
+  def do_head(indirection_request, request, response)
+    unless indirection_request.model.head(indirection_request.key, indirection_request.to_hash)
+      Puppet.info("Could not find #{indirection_request.indirection_name} for '#{indirection_request.key}'")
+      return do_exception(response, "Could not find #{indirection_request.indirection_name} #{indirection_request.key}", 404)
+    end
+
+    # No need to set a response because no response is expected from a
+    # HEAD request.  All we need to do is not die.
+  end
+
   # Execute our search.
   def do_search(indirection_request, request, response)
     result = indirection_request.model.search(indirection_request.key, indirection_request.to_hash)
diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/network/rest_authconfig.rb
index 7abe06956..7a6147a82 100644
--- a/lib/puppet/network/rest_authconfig.rb
+++ b/lib/puppet/network/rest_authconfig.rb
@@ -38,14 +38,10 @@ module Puppet
       # fail_on_deny could as well be called in the XMLRPC context
       # with a ClientRequest.
 
-            @rights.fail_on_deny(
-        build_uri(request),
-        
-                  :node => request.node,
-                  :ip => request.ip,
-                  :method => request.method,
-                  :environment => request.environment,
-                  :authenticated => request.authenticated)
+      if authorization_failure_exception = @rights.is_request_forbidden_and_why?(request)
+        Puppet.warning("Denying access: #{authorization_failure_exception}")
+        raise authorization_failure_exception
+      end
     end
 
     def initialize(file = nil, parsenow = true)
@@ -88,9 +84,5 @@ module Puppet
       end
       @rights.restrict_authenticated(acl[:acl], acl[:authenticated]) unless acl[:authenticated].nil?
     end
-
-    def build_uri(request)
-      "/#{request.indirection_name}/#{request.key}"
-    end
   end
 end
diff --git a/lib/puppet/network/rights.rb b/lib/puppet/network/rights.rb
index e3cd3179a..00ee04f8d 100755
--- a/lib/puppet/network/rights.rb
+++ b/lib/puppet/network/rights.rb
@@ -26,19 +26,34 @@ class Rights
 
   # Check that name is allowed or not
   def allowed?(name, *args)
-    begin
-      fail_on_deny(name, :node => args[0], :ip => args[1])
-    rescue AuthorizationError
-      return false
-    rescue ArgumentError
-      # the namespace contract says we should raise this error
-      # if we didn't find the right acl
-      raise
+    !is_forbidden_and_why?(name, :node => args[0], :ip => args[1])
+  end
+
+  def is_request_forbidden_and_why?(request)
+    methods_to_check = if request.method == :head
+                         # :head is ok if either :find or :save is ok.
+                         [:find, :save]
+                       else
+                         [request.method]
+                       end
+    authorization_failure_exceptions = methods_to_check.map do |method|
+      is_forbidden_and_why?("/#{request.indirection_name}/#{request.key}",
+                            :node => request.node,
+                            :ip => request.ip,
+                            :method => method,
+                            :environment => request.environment,
+                            :authenticated => request.authenticated)
+    end
+    if authorization_failure_exceptions.include? nil
+      # One of the methods we checked is ok, therefore this request is ok.
+      nil
+    else
+      # Just need to return any of the failure exceptions.
+      authorization_failure_exceptions.first
     end
-    true
   end
 
-  def fail_on_deny(name, args = {})
+  def is_forbidden_and_why?(name, args = {})
     res = :nomatch
     right = @rights.find do |acl|
       found = false
@@ -49,7 +64,7 @@ class Rights
         args[:match] = match
         if (res = acl.allowed?(args[:node], args[:ip], args)) != :dunno
           # return early if we're allowed
-          return if res
+          return nil if res
           # we matched, select this acl
           found = true
         end
@@ -70,13 +85,12 @@ class Rights
         error.file = right.file
         error.line = right.line
       end
-      Puppet.warning("Denying access: #{error}")
     else
       # there were no rights allowing/denying name
       # if name is not a path, let's throw
-      error = ArgumentError.new "Unknown namespace right '#{name}'"
+      raise ArgumentError.new "Unknown namespace right '#{name}'"
     end
-    raise error
+    error
   end
 
   def initialize