[CHEF-3557] remove load_current_resource_attrs

Functionality was a duplicate of ScanAccessControl with some slightly
different behavior. Correct behavior is now implemented in
ScanAccessControl.

6 files changed, 165 insertions, 160 deletions

diff --git a/lib/chef/provider/file.rb b/lib/chef/provider/file.rb
index 2928a36f5c..14da43599a 100644
--- a/lib/chef/provider/file.rb
+++ b/lib/chef/provider/file.rb
@@ -138,41 +138,18 @@ class Chef
             end
           end
         end
-        load_current_resource_attrs
         setup_acl
-        
+
         @current_resource
       end
 
+      # TODO: remove me
       def load_current_resource_attrs
-        if Chef::Platform.windows?
-          # TODO: To work around CHEF-3554, add support for Windows
-          # equivalent, or implicit resource reporting won't work for
-          # Windows.
-          return
-        end
-
-        if ::File.exist?(@new_resource.path)
-          stat = ::File.stat(@new_resource.path)
-          @current_resource.owner(stat.uid)
-          @current_resource.mode(stat.mode & 07777)
-          @current_resource.group(stat.gid)
-
-          if @new_resource.group.nil?
-            @new_resource.group(@current_resource.group)
-          end 
-          if @new_resource.owner.nil?
-            @new_resource.owner(@current_resource.owner)
-          end
-          if @new_resource.mode.nil?
-            @new_resource.mode(@current_resource.mode)
-          end
-        end
       end
-      
+
       def setup_acl
-        @acl_scanner = ScanAccessControl.new(@new_resource, @current_resource)
-        @acl_scanner.set_all!
+        acl_scanner = ScanAccessControl.new(@new_resource, @current_resource)
+        acl_scanner.set_all!
       end
 
       def define_resource_requirements
@@ -236,10 +213,8 @@ class Chef
           return
         end
 
-        stat = ::File.stat(path)
-        @new_resource.owner(stat.uid)
-        @new_resource.mode(stat.mode & 07777)
-        @new_resource.group(stat.gid)
+        acl_scanner = ScanAccessControl.new(@new_resource, @new_resource)
+        acl_scanner.set_all!
       end
 
       def action_create
@@ -249,7 +224,7 @@ class Chef
           desc << " with content checksum #{short_cksum(new_resource_content_checksum)}" if new_resource.content
           description << desc
           description << diff_current_from_content(@new_resource.content) 
-          
+
           converge_by(description) do
             Chef::Log.info("entered create")
             ::File.open(@new_resource.path, "w+") {|f| f.write @new_resource.content }
diff --git a/lib/chef/provider/template.rb b/lib/chef/provider/template.rb
index 4dcf23cb4e..fbd5c600b9 100644
--- a/lib/chef/provider/template.rb
+++ b/lib/chef/provider/template.rb
@@ -51,14 +51,17 @@ class Chef
       def action_create
         render_with_context(template_location) do |rendered_template|
           rendered(rendered_template)
-          update = ::File.exist?(@new_resource.path)
-          if update && content_matches?
+          if file_already_exists? && content_matches?
             Chef::Log.debug("#{@new_resource} content has not changed.")
             set_all_access_controls
+            update_new_file_state(@new_resource.path)
           else
-            description = [] 
-            action_message = update ? "update #{@current_resource} from #{short_cksum(@current_resource.checksum)} to #{short_cksum(@new_resource.checksum)}" :
+            description = []
+            action_message = if file_already_exists?
+              "update #{@current_resource} from #{short_cksum(@current_resource.checksum)} to #{short_cksum(@new_resource.checksum)}"
+            else
               "create #{@new_resource}"
+            end
             description << action_message
             description << diff_current(rendered_template.path)
             converge_by(description) do
@@ -66,17 +69,10 @@ class Chef
               FileUtils.mv(rendered_template.path, @new_resource.path)
               Chef::Log.info("#{@new_resource} updated content")
               access_controls.set_all!
-              stat = ::File.stat(@new_resource.path)
-
-              # template depends on the checksum not changing, and updates it
-              # itself later in the code, so we cannot set it here, as we do with
-              # all other < File child provider classes
-              @new_resource.owner(stat.uid)
-              @new_resource.mode(stat.mode & 07777)
-              @new_resource.group(stat.gid)
+              update_new_file_state(@new_resource.path)
             end
           end
-        end  
+        end
       end
 
       def template_finder
@@ -107,6 +103,10 @@ class Chef
 
       private
 
+      def file_already_exists?
+        ::File.exist?(@new_resource.path)
+      end
+
       def render_with_context(template_location, &block)
         context = {}
         context.merge!(@new_resource.variables)
diff --git a/spec/functional/resource/file_spec.rb b/spec/functional/resource/file_spec.rb
index c25d8bae26..352f28c3da 100644
--- a/spec/functional/resource/file_spec.rb
+++ b/spec/functional/resource/file_spec.rb
@@ -72,7 +72,7 @@ describe Chef::Resource::File do
           # TODO: most stable way to specify?
           resource.owner.should == Etc.getpwuid(Process.uid).name
           resource.group.should == Etc.getgrgid(Process.gid).name
-          resource.mode.should == (0100666 - File.umask).to_s(8)
+          resource.mode.should == ((0100666 - File.umask) & 07777).to_s(8)
         end
       end
 
@@ -140,10 +140,11 @@ describe Chef::Resource::File do
       end
 
       context "and mode is specified as a String" do
-        let(:expected_mode) { "0440" }
+        let(:set_mode) { "0440" }
+        let(:expected_mode) { "440" }
 
         before do
-          resource.mode(expected_mode)
+          resource.mode(set_mode)
           resource.run_action(:create)
         end
 
@@ -153,14 +154,15 @@ describe Chef::Resource::File do
       end
 
       context "and mode is specified as an Integer" do
-        let(:expected_mode) { 00440 }
+        let(:set_mode) { 00440 }
 
+        let(:expected_mode) { "440" }
         before do
-          resource.mode(expected_mode)
+          resource.mode(set_mode)
           resource.run_action(:create)
         end
 
-        it "sets mode on the new resource as an integer" do
+        it "sets mode on the new resource as a String" do
           resource.mode.should == expected_mode
         end
       end
@@ -177,7 +179,7 @@ describe Chef::Resource::File do
           # TODO: most stable way to specify?
           current_resource.owner.should == Etc.getpwuid(Process.uid).name
           current_resource.group.should == Etc.getgrgid(Process.gid).name
-          current_resource.mode.should == (0100666 - File.umask).to_s(8)
+          current_resource.mode.should == ((0100666 - File.umask) & 07777).to_s(8)
         end
       end
 
@@ -236,7 +238,8 @@ describe Chef::Resource::File do
       end
 
       context "and mode is specified as a String" do
-        let(:expected_mode) { ((0100666 - File.umask) & 07777).to_s(8) }
+        let(:default_create_mode) { (0100666 - File.umask) }
+        let(:expected_mode) { (default_create_mode & 07777).to_s(8) }
 
         before do
           resource.mode(expected_mode)
@@ -248,13 +251,14 @@ describe Chef::Resource::File do
       end
 
       context "and mode is specified as an Integer" do
-        let(:expected_mode) { (0100666 - File.umask) & 07777 }
+        let(:set_mode) { (0100666 - File.umask) & 07777 }
+        let(:expected_mode) { set_mode.to_s(8) }
 
         before do
-          resource.mode(expected_mode)
+          resource.mode(set_mode)
         end
 
-        it "sets mode on the new resource as an integer" do
+        it "sets mode on the new resource as a String" do
           current_resource.mode.should == expected_mode
         end
       end
diff --git a/spec/unit/provider/directory_spec.rb b/spec/unit/provider/directory_spec.rb
index 835be07bd6..598bb46f0d 100644
--- a/spec/unit/provider/directory_spec.rb
+++ b/spec/unit/provider/directory_spec.rb
@@ -36,80 +36,97 @@ describe Chef::Provider::Directory do
     @directory = Chef::Provider::Directory.new(@new_resource, @run_context)
   end
 
-  context "load_current_resource_attrs", :unix_only do
-    it "should load the current resource based on the new resource" do
-      File.stub!(:exist?).and_return(true)
+
+  describe "scanning file security metadata on windows" do
+    before do
+    end
+
+    it "describes the directory's access rights" do
+      pending
+    end
+  end
+
+  describe "scanning file security metadata on unix" do
+    let(:mock_stat) do
       cstats = mock("stats")
       cstats.stub!(:uid).and_return(500)
       cstats.stub!(:gid).and_return(500)
       cstats.stub!(:mode).and_return(0755)
-      File.should_receive(:stat).twice.and_return(cstats)
-      @directory.load_current_resource
-      @directory.current_resource.path.should eql(@new_resource.path)
-      @directory.current_resource.owner.should eql(500)
-      @directory.current_resource.group.should eql(500)
-      @directory.current_resource.mode.should == 00755
+      cstats
     end
-  
-    it "should create a new directory on create, setting updated to true" do
-      @new_resource.path "/tmp/foo"
-  
-      File.should_receive(:exist?).exactly(3).and_return(false)
-      Dir.should_receive(:mkdir).with(@new_resource.path).once.and_return(true)
-  
-      @directory.should_receive(:set_all_access_controls)
-      @directory.stub!(:update_new_file_state)
-      @directory.run_action(:create)
-      @directory.new_resource.should be_updated
-    end
-  
-    it "should raise an exception if the parent directory does not exist and recursive is false" do 
-      @new_resource.path "/tmp/some/dir"
-      @new_resource.recursive false
-      lambda { @directory.run_action(:create) }.should raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist) 
-    end
-  
-    it "should create a new directory when parent directory does not exist if recursive is true and permissions are correct" do
-      @new_resource.path "/path/to/dir"
-      @new_resource.recursive true
-      File.should_receive(:exist?).with(@new_resource.path).ordered.and_return(false)
-      File.should_receive(:exist?).with(@new_resource.path).ordered.and_return(false)
-  
-      File.should_receive(:exist?).with('/path/to').ordered.and_return(false)
-      File.should_receive(:exist?).with('/path').ordered.and_return(true)
-      File.should_receive(:writable?).with('/path').ordered.and_return(true)
-      File.should_receive(:exist?).with(@new_resource.path).ordered.and_return(false)
-   
-      FileUtils.should_receive(:mkdir_p).with(@new_resource.path).and_return(true) 
-      @directory.should_receive(:set_all_access_controls)
-      @directory.stub!(:update_new_file_state)
-      @directory.run_action(:create)
-      @new_resource.should be_updated
+
+    it "describes the access mode as a String of octal integers" do
+      File.stub!(:exist?).and_return(true)
+      File.should_receive(:stat).and_return(mock_stat)
+      @directory.load_current_resource
+      @directory.current_resource.mode.should == "755"
     end
-   
-    # it "should raise an error when creating a directory recursively and permissions do not allow creation" do
-      
-    # end
-  
-    it "should raise an error when creating a directory when parent directory is a file" do
-      File.should_receive(:directory?).and_return(false)
-      Dir.should_not_receive(:mkdir).with(@new_resource.path)
-      lambda { @directory.run_action(:create) }.should raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist)
-      @directory.new_resource.should_not be_updated
+
+    context "when user and group are specified with UID/GID" do
+      it "describes the current owner and group as UID and GID" do
+        File.stub!(:exist?).and_return(true)
+        File.should_receive(:stat).and_return(mock_stat)
+        @directory.load_current_resource
+        @directory.current_resource.path.should eql(@new_resource.path)
+        @directory.current_resource.owner.should eql(500)
+        @directory.current_resource.group.should eql(500)
+      end
     end
-    
-    it "should not create the directory if it already exists" do
-      stub_file_cstats
-      @new_resource.path "/tmp/foo"
-      File.should_receive(:exist?).exactly(3).and_return(true)
-      Dir.should_not_receive(:mkdir).with(@new_resource.path)
-      @directory.should_receive(:set_all_access_controls)
-      @directory.run_action(:create)
+
+    context "when user/group are specified with user/group names" do
     end
   end
 
-  context "load_current_resource_attrs", :windows_only do
-    pending "CHEF-3557: Fix implicit resource change collection on Windows"
+  it "should create a new directory on create, setting updated to true" do
+    @new_resource.path "/tmp/foo"
+
+    File.should_receive(:exist?).exactly(2).and_return(false)
+    Dir.should_receive(:mkdir).with(@new_resource.path).once.and_return(true)
+
+    @directory.should_receive(:set_all_access_controls)
+    @directory.stub!(:update_new_file_state)
+    @directory.run_action(:create)
+    @directory.new_resource.should be_updated
+  end
+
+  it "should raise an exception if the parent directory does not exist and recursive is false" do 
+    @new_resource.path "/tmp/some/dir"
+    @new_resource.recursive false
+    lambda { @directory.run_action(:create) }.should raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist) 
+  end
+
+  it "should create a new directory when parent directory does not exist if recursive is true and permissions are correct" do
+    @new_resource.path "/path/to/dir"
+    @new_resource.recursive true
+    File.should_receive(:exist?).with(@new_resource.path).ordered.and_return(false)
+
+    File.should_receive(:exist?).with('/path/to').ordered.and_return(false)
+    File.should_receive(:exist?).with('/path').ordered.and_return(true)
+    File.should_receive(:writable?).with('/path').ordered.and_return(true)
+    File.should_receive(:exist?).with(@new_resource.path).ordered.and_return(false)
+
+    FileUtils.should_receive(:mkdir_p).with(@new_resource.path).and_return(true) 
+    @directory.should_receive(:set_all_access_controls)
+    @directory.stub!(:update_new_file_state)
+    @directory.run_action(:create)
+    @new_resource.should be_updated
+  end
+
+
+  it "should raise an error when creating a directory when parent directory is a file" do
+    File.should_receive(:directory?).and_return(false)
+    Dir.should_not_receive(:mkdir).with(@new_resource.path)
+    lambda { @directory.run_action(:create) }.should raise_error(Chef::Exceptions::EnclosingDirectoryDoesNotExist)
+    @directory.new_resource.should_not be_updated
+  end
+
+  it "should not create the directory if it already exists" do
+    stub_file_cstats
+    @new_resource.path "/tmp/foo"
+    File.should_receive(:exist?).exactly(2).and_return(true)
+    Dir.should_not_receive(:mkdir).with(@new_resource.path)
+    @directory.should_receive(:set_all_access_controls)
+    @directory.run_action(:create)
   end
 
   it "should delete the directory if it exists, and is writable with action_delete" do
diff --git a/spec/unit/provider/file_spec.rb b/spec/unit/provider/file_spec.rb
index 8c575e815e..3d41d326d3 100644
--- a/spec/unit/provider/file_spec.rb
+++ b/spec/unit/provider/file_spec.rb
@@ -53,19 +53,24 @@ describe Chef::Provider::File do
     @provider.current_resource.content.should eql(nil)
   end
 
-  context "load_current_resource_attrs", :unix_only do
+  describe "examining file security metadata on Unix" do
     it "should collect the current state of the file on the filesystem and populate current_resource" do
       # test setup
       stat_struct = mock("::File.stat", :mode => 0600, :uid => 0, :gid => 0, :mtime => 10000)
       ::File.should_receive(:stat).exactly(2).times.with(@resource.path).and_return(stat_struct)
 
       # test execution
+
+      Etc.should_receive(:getgrgid).with(0).and_return(mock("Group Ent", :name => "wheel"))
+      Etc.should_receive(:getpwuid).with(0).and_return(mock("User Ent", :name => "root"))
+
+      # test execution 
       @provider.load_current_resource
 
       # post-condition checks
-      @provider.current_resource.mode.should == 0600
-      @provider.current_resource.owner.should == 0
-      @provider.current_resource.group.should == 0
+      @provider.current_resource.mode.should == "600"
+      @provider.current_resource.owner.should == "root"
+      @provider.current_resource.group.should == "wheel"
     end
 
     it "should NOT update the new_resource state with the current_resourse state if new_resource state is already specified" do
@@ -86,42 +91,29 @@ describe Chef::Provider::File do
       @provider.new_resource.mode.should == 0644
     end
 
-    it "should update the new_resource state with the current_resource state if the new_resource state is not specified." do
-      # test setup
-      stat_struct = mock("::File.stat", :mode => 0600, :uid => 0, :gid => 0, :mtime => 10000)
-      ::File.should_receive(:stat).exactly(2).times.with(@resource.path).and_return(stat_struct)
-
-      @provider.new_resource.group(nil)
-      @provider.new_resource.owner(nil)
-      @provider.new_resource.mode(nil)
-
-      # test execution
-      @provider.load_current_resource
-
-      # post-condition checks
-      @provider.new_resource.group.should eql(@provider.current_resource.group)
-      @provider.new_resource.owner.should eql(@provider.current_resource.owner)
-      @provider.new_resource.mode.should eql(@provider.current_resource.mode)
-    end
+    context "when the new_resource does not specify the desired access control" do
+      it "records access control information in the new resource after modifying the file" do
+        # test setup
+        stat_struct = mock("::File.stat", :mode => 0600, :uid => 0, :gid => 0, :mtime => 10000) 
+        # called once in update_new_file_state and once in checksum
+        ::File.should_receive(:stat).twice.with(@provider.new_resource.path).and_return(stat_struct)  
+        ::File.should_receive(:directory?).once.with(@provider.new_resource.path).and_return(false)
 
-    it "should update the new_resource when attempting to set the new state" do
-      # test setup
-      stat_struct = mock("::File.stat", :mode => 0600, :uid => 0, :gid => 0, :mtime => 10000)
-      # called once in update_new_file_state and once in checksum
-      ::File.should_receive(:stat).with(@provider.new_resource.path).and_return(stat_struct)
-      ::File.should_receive(:directory?).once.with(@provider.new_resource.path).and_return(false)
+        Etc.should_receive(:getpwuid).with(0).and_return(mock("User Ent", :name => "root"))
+        Etc.should_receive(:getgrgid).with(0).and_return(mock("Group Ent", :name => "wheel"))
 
-      @provider.new_resource.group(nil)
-      @provider.new_resource.owner(nil)
-      @provider.new_resource.mode(nil)
+        @provider.new_resource.group(nil)
+        @provider.new_resource.owner(nil)
+        @provider.new_resource.mode(nil)
 
-      # test exectution
-      @provider.update_new_file_state
+        # test exectution
+        @provider.update_new_file_state
 
-      # post-condition checks
-      @provider.new_resource.group.should == 0
-      @provider.new_resource.owner.should == 0
-      @provider.new_resource.mode.should == 0600
+        # post-condition checks
+        @provider.new_resource.group.should == "wheel"
+        @provider.new_resource.owner.should == "root"
+        @provider.new_resource.mode.should == "600"
+      end
     end
   end
 
diff --git a/spec/unit/provider/template_spec.rb b/spec/unit/provider/template_spec.rb
index f5d30f0219..5f59f8d50e 100644
--- a/spec/unit/provider/template_spec.rb
+++ b/spec/unit/provider/template_spec.rb
@@ -47,16 +47,16 @@ describe Chef::Provider::Template do
                     else
                       Struct::Passwd.new("root", "x", 0, 0, "root", "/root", "/bin/bash")
                     end
-    group_struct = OpenStruct.new(:name => "root", :passwd => "x", :gid => 0)
+    group_struct = mock("Group Ent", :name => "root", :passwd => "x", :gid => 0)
     Etc.stub!(:getpwuid).and_return(passwd_struct)
     Etc.stub!(:getgrgid).and_return(group_struct)
   end
 
   describe "when creating the template" do
 
-    before do 
-
+    before do
     end
+
     after do
       FileUtils.rm(@rendered_file_location) if ::File.exist?(@rendered_file_location)
     end
@@ -115,6 +115,23 @@ describe Chef::Provider::Template do
         IO.read(@rendered_file_location).should == "slappiness is happiness"
         @resource.should be_updated_by_last_action
       end
+
+      context "and no access control settings are set on the resource" do
+        it "sets access control metadata on the new resource" do
+          @access_controls.stub!(:requires_changes?).and_return(false)
+          @access_controls.should_receive(:set_all!)
+          @node.normal[:slappiness] = "happiness"
+          @provider.should_receive(:backup)
+          @provider.run_action(:create)
+          IO.read(@rendered_file_location).should == "slappiness is happiness"
+          @resource.should be_updated_by_last_action
+
+          # Veracity of actual data checked in functional tests
+          @resource.owner.should be_a_kind_of(String)
+          @resource.group.should be_a_kind_of(String)
+          @resource.mode.should be_a_kind_of(String)
+        end
+      end
     end
 
     describe "when the target file has the wrong content" do