Merge branch 'stable'

Author: thallgren

lib/puppet/parameter.rb

@@ -295,6 +295,10 @@ def self.proxymethods(*values)
   #
   attr_accessor :parent
 
+  # @!attribute [rw] sensitive
+  #   @return [true, false] If this parameter has been tagged as sensitive.
+  attr_accessor :sensitive
+
   # Returns a string representation of the resource's containment path in
   # the catalog.
   # @return [String]
@@ -522,6 +526,22 @@ def to_s
     name.to_s
   end
 
+  # Formats the given string and conditionally redacts the provided interpolation variables, depending on if
+  # this property is sensitive.
+  #
+  # @note Because the default implementation of Puppet::Property#is_to_s returns the current value as-is, it
+  #   doesn't necessarily return a string. For the sake of sanity we just cast everything to a string for
+  #   interpolation so we don't introduce issues with unexpected property values.
+  #
+  # @see String#format
+  # @param fmt [String] The format string to interpolate.
+  # @param args [Array<String>] One or more strings to conditionally redact and interpolate into the format string.
+  #
+  # @return [String]
+  def format(fmt, *args)
+    fmt % args.map { |arg| @sensitive ? "[redacted]" : arg.to_s }
+  end
+
   # Produces a String with the value formatted for display to a human.
   # When the parameter value is a:
   #

lib/puppet/property.rb

@@ -55,10 +55,6 @@ class Puppet::Property < Puppet::Parameter
   #
   attr_writer :noop
 
-  # @!attribute [rw] sensitive
-  #   @return [true, false] If this property has been tagged as sensitive.
-  attr_accessor :sensitive
-
   class << self
     # @todo Figure out what this is used for. Can not find any logic in the puppet code base that
     #   reads or writes this attribute.
@@ -221,22 +217,6 @@ def change_to_s(current_value, newvalue)
     end
   end
 
-  # Formats the given string and conditionally redacts the provided interpolation variables, depending on if
-  # this property is sensitive.
-  #
-  # @note Because the default implementation of #is_to_s returns the current value as-is, it doesn't necessarily
-  #   return a string. For the sake of sanity we just cast everything to a string for interpolation so we don't
-  #   introduce issues with unexpected property values.
-  #
-  # @see String#format
-  # @param fmt [String] The format string to interpolate.
-  # @param args [Array<String>] One or more strings to conditionally redact and interpolate into the format string.
-  #
-  # @return [String]
-  def format(fmt, *args)
-    fmt % args.map { |arg| @sensitive ? "[redacted]" : arg.to_s }
-  end
-
   # Produces the name of the event to use to describe a change of this property's value.
   # The produced event name is either the event name configured for this property, or a generic
   # event based on the name of the property with suffix `_changed`, or if the property is

lib/puppet/type/file.rb

@@ -906,9 +906,10 @@ def set_sensitive_parameters(sensitive_parameters)
 
     # The source parameter isn't actually a property but works by injecting information into the
     # content property. In order to preserve the intended sensitive context we need to mark content
-    # as sensitive instead.
+    # as sensitive as well.
     if sensitive_parameters.include?(:source)
       sensitive_parameters.delete(:source)
+      parameter(:source).sensitive = true
       # The `source` parameter will generate the `content` property when the resource state is retrieved
       # but that's long after we've set the sensitive context. Force the early creation of the `content`
       # attribute so we can mark it as sensitive.

lib/puppet/type/file/data_sync.rb

@@ -34,14 +34,22 @@ def checksum_insync?(param, is, has_contents, &block)
 
       return true if ! resource.replace?
 
-      result = yield(is)
+      is_insync = yield(is)
 
-      if !result && Puppet[:show_diff] && resource.show_diff?
-        write_temporarily(param) do |path|
-          send resource[:loglevel], "\n" + diff(resource[:path], path)
+      if show_diff?(!is_insync)
+        if param.sensitive
+          send resource[:loglevel], "[diff redacted]"
+        else
+          write_temporarily(param) do |path|
+            send resource[:loglevel], "\n" + diff(resource[:path], path)
+          end
         end
       end
-      result
+      is_insync
+    end
+
+    def show_diff?(has_changes)
+      has_changes && Puppet[:show_diff] && resource.show_diff?
     end
 
     def date_matches?(checksum_type, current, desired)

spec/unit/parameter_spec.rb

@@ -190,4 +190,15 @@
       )).to eq("{'1' => 'foo', 'bar' => ['2', '3', '4'], 'baz' => {'quux' => 'two', 'qux' => '1'}}")
     end
   end
+
+  describe 'formatting messages' do
+    it "formats messages as-is when the parameter is not sensitive" do
+      expect(@parameter.format("hello %s", "world")).to eq("hello world")
+    end
+
+    it "formats messages with redacted values when the parameter is not sensitive" do
+      @parameter.sensitive = true
+      expect(@parameter.format("hello %s", "world")).to eq("hello [redacted]")
+    end
+  end
 end

spec/unit/property_spec.rb

@@ -50,17 +50,6 @@ class << self
     expect(property.to_s).to eq(property.name.to_s)
   end
 
-  describe 'formatting messages' do
-    it "formats messages as-is when the property is not sensitive" do
-      expect(property.format("hello %s", "world")).to eq("hello world")
-    end
-
-    it "formats messages with redacted values when the property is not sensitive" do
-      property.sensitive = true
-      expect(property.format("hello %s", "world")).to eq("hello [redacted]")
-    end
-  end
-
   describe "when returning the default event name" do
     it "should use the current 'should' value to pick the event name" do
       property.expects(:should).returns "myvalue"

spec/unit/type/file/content_spec.rb

@@ -184,25 +184,30 @@
           expect(content.respond_to?("diff")).to eq(false)
         end
 
-        [true, false].product([true, false]).each do |cfg, param|
-          describe "and Puppet[:show_diff] is #{cfg} and show_diff => #{param}" do
+        describe "showing the diff" do
+          it "doesn't show the diff when #show_diff? is false" do
+            content.expects(:show_diff?).returns false
+            content.expects(:diff).never
+            expect(content).not_to be_safe_insync("other content")
+          end
+
+          describe "and #show_diff? is true" do
             before do
-              Puppet[:show_diff] = cfg
-              resource.stubs(:show_diff?).returns param
+              content.expects(:show_diff?).returns true
               resource[:loglevel] = "debug"
             end
 
-            if cfg and param
-              it "should display a diff" do
-                content.expects(:diff).returns("my diff").once
-                content.expects(:debug).with("\nmy diff").once
-                expect(content).not_to be_safe_insync("other content")
-              end
-            else
-              it "should not display a diff" do
-                content.expects(:diff).never
-                expect(content).not_to be_safe_insync("other content")
-              end
+            it "prints the diff" do
+              content.expects(:diff).returns("my diff").once
+              content.expects(:debug).with("\nmy diff").once
+              expect(content).not_to be_safe_insync("other content")
+            end
+
+            it "redacts the diff when the property is sensitive" do
+              content.sensitive = true
+              content.expects(:diff).returns("my diff").never
+              content.expects(:debug).with("[diff redacted]").once
+              expect(content).not_to be_safe_insync("other content")
             end
           end
         end
@@ -308,6 +313,33 @@
     end
   end
 
+  describe "determining if a diff should be shown" do
+    let(:content) { described_class.new(:resource => resource) }
+
+    before do
+      Puppet[:show_diff] = true
+      resource[:show_diff] = true
+    end
+
+    it "is true if there are changes and the global and per-resource show_diff settings are true" do
+      expect(content.show_diff?(true)).to be_truthy
+    end
+
+    it "is false if there are no changes" do
+      expect(content.show_diff?(false)).to be_falsey
+    end
+
+    it "is false if show_diff is globally disabled" do
+      Puppet[:show_diff] = false
+      expect(content.show_diff?(false)).to be_falsey
+    end
+
+    it "is false if show_diff is disabled on the resource" do
+      resource[:show_diff] = false
+      expect(content.show_diff?(false)).to be_falsey
+    end
+  end
+
   describe "when changing the content" do
     let(:content) { described_class.new(:resource => resource) }
 

spec/unit/type/file_spec.rb

@@ -364,10 +364,11 @@
       expect(file[:ensure]).to eq(:link)
     end
 
-    describe "marking properties as sensitive" do
-      it "marks content and ensure as sensitive when source is sensitive" do
+    describe "marking parameters as sensitive" do
+      it "marks sensitive, content, and ensure as sensitive when source is sensitive" do
         resource = Puppet::Resource.new(:file, make_absolute("/tmp/foo"), :parameters => {:source => make_absolute('/tmp/bar')}, :sensitive_parameters => [:source])
         file = described_class.new(resource)
+        expect(file.parameter(:source).sensitive).to eq true
         expect(file.property(:content).sensitive).to eq true
         expect(file.property(:ensure).sensitive).to eq true
       end