[CHEF-3557] fix file security reporting for files on Unix

* windows still needs work
* directories still need work
* needs tests for the case that *only* security metadata is updated
  (correct content)

Conflicts:
	spec/functional/resource/file_spec.rb

9 files changed, 390 insertions, 210 deletions

diff --git a/lib/chef/provider/cookbook_file.rb b/lib/chef/provider/cookbook_file.rb
index c9e9f5fa56..46a4935844 100644
--- a/lib/chef/provider/cookbook_file.rb
+++ b/lib/chef/provider/cookbook_file.rb
@@ -48,6 +48,7 @@ class Chef
               tempfile.close
               FileUtils.cp(file_cache_location, tempfile.path)
             end
+            update_new_file_state
             Chef::Log.info("#{@new_resource} created file #{@new_resource.path}")
           end
         else
diff --git a/lib/chef/provider/remote_file.rb b/lib/chef/provider/remote_file.rb
index 62db2cd7dd..f985682441 100644
--- a/lib/chef/provider/remote_file.rb
+++ b/lib/chef/provider/remote_file.rb
@@ -73,6 +73,7 @@ class Chef
           end
         end
         set_all_access_controls
+        update_new_file_state
       end
 
       def current_resource_matches_target_checksum?
diff --git a/spec/functional/resource/cookbook_file_spec.rb b/spec/functional/resource/cookbook_file_spec.rb
index b2bee5d95b..d88dafec0c 100644
--- a/spec/functional/resource/cookbook_file_spec.rb
+++ b/spec/functional/resource/cookbook_file_spec.rb
@@ -32,6 +32,10 @@ describe Chef::Resource::CookbookFile do
     content
   end
 
+  let(:default_mode) { "600" }
+
+  it_behaves_like "a securable resource with reporting"
+
   def create_resource
     # set up cookbook collection for this run to use, based on our
     # spec data.
diff --git a/spec/functional/resource/directory_spec.rb b/spec/functional/resource/directory_spec.rb
index 5777a114fc..0576baab37 100644
--- a/spec/functional/resource/directory_spec.rb
+++ b/spec/functional/resource/directory_spec.rb
@@ -36,4 +36,6 @@ describe Chef::Resource::Directory do
 
   it_behaves_like "a directory resource"
 
+  it_behaves_like "a securable resource with reporting"
+
 end
diff --git a/spec/functional/resource/file_spec.rb b/spec/functional/resource/file_spec.rb
index 352f28c3da..2c18d07520 100644
--- a/spec/functional/resource/file_spec.rb
+++ b/spec/functional/resource/file_spec.rb
@@ -52,218 +52,11 @@ describe Chef::Resource::File do
     provider.current_resource
   end
 
-  it_behaves_like "a file resource"
-
-  describe "reading file security metadata for reporting", :focus => true do
-    context "when the target file doesn't exist" do
-      before do
-        resource.action(:create)
-      end
-
-      it "has empty values for file metadata in 'current_resource'" do
-        current_resource.owner.should be_nil
-        current_resource.group.should be_nil
-        current_resource.mode.should be_nil
-      end
-
-      context "and no security metadata is specified in new_resource" do
-        it "sets the metadata values on the new_resource as strings after creating" do
-          resource.run_action(:create)
-          # 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) & 07777).to_s(8)
-        end
-      end
-
-      context "and owner is specified with a String (username) in new_resource", :requires_root => true do
-
-        # TODO/bug: duplicated from the "securable resource" tests
-        let(:expected_user_name) { 'nobody' }
-
-        before do
-          resource.owner(expected_user_name)
-          resource.run_action(:create)
-        end
-
-        it "sets the owner on new_resource to the username (String) of the desired owner" do
-          resource.owner.should == expected_user_name
-        end
-
-      end
-
-      context "and owner is specified with an Integer (uid) in new_resource", :requires_root => true do
-
-        # TODO: duplicated from "securable resource"
-        let(:expected_user_name) { 'nobody' }
-        let(:expected_uid) { Etc.getpwnam(expected_user_name).uid }
-        let(:desired_gid) { 1337 }
-        let(:expected_gid) { 1337 }
-
-        before do
-          resource.owner(expected_uid)
-          resource.run_action(:create)
-        end
-
-        it "sets the owner on new_resource to the uid (Integer) of the desired owner" do
-          resource.owner.should == expected_uid
-        end
-      end
+  let(:default_mode) { ((0100666 - File.umask) & 07777).to_s(8) }
 
-      context "and group is specified with a String (group name)", :requires_root => true do
-
-        let(:expected_group_name) { Etc.getgrent.name }
-
-        before do
-          resource.group(expected_group_name)
-          resource.run_action(:create)
-        end
-
-        it "sets the group on new_resource to the group name (String) of the group" do
-          resource.group.should == expected_group_name
-        end
-
-      end
-
-      context "and group is specified with an Integer (gid)", :requires_root => true do
-        let(:expected_gid) { Etc.getgrent.gid }
-
-        before do
-          resource.group(expected_gid)
-          resource.run_action(:create)
-        end
-
-        it "sets the group on new_resource to the gid (Integer)" do
-          resource.group.should == expected_gid
-        end
-
-      end
-
-      context "and mode is specified as a String" do
-        let(:set_mode) { "0440" }
-        let(:expected_mode) { "440" }
-
-        before do
-          resource.mode(set_mode)
-          resource.run_action(:create)
-        end
-
-        it "sets mode on the new_resource as a String" do
-          resource.mode.should == expected_mode
-        end
-      end
-
-      context "and mode is specified as an Integer" do
-        let(:set_mode) { 00440 }
-
-        let(:expected_mode) { "440" }
-        before do
-          resource.mode(set_mode)
-          resource.run_action(:create)
-        end
-
-        it "sets mode on the new resource as a String" do
-          resource.mode.should == expected_mode
-        end
-      end
-    end
-
-    context "when the target file exists" do
-      before do
-        FileUtils.touch(resource.path)
-        resource.action(:create)
-      end
-
-      context "and no security metadata is specified in new_resource" do
-        it "sets the current values on current resource as strings" 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) & 07777).to_s(8)
-        end
-      end
-
-      context "and owner is specified with a String (username) in new_resource" do
-
-        let(:expected_user_name) { Etc.getpwuid(Process.uid).name }
-
-        before do
-          resource.owner(expected_user_name)
-        end
-
-        it "sets the owner on new_resource to the username (String) of the desired owner" do
-          current_resource.owner.should == expected_user_name
-        end
-
-      end
-
-      context "and owner is specified with an Integer (uid) in new_resource" do
-
-        let(:expected_uid) { Process.uid }
-
-        before do
-          resource.owner(expected_uid)
-        end
-
-        it "sets the owner on new_resource to the uid (Integer) of the desired owner" do
-          current_resource.owner.should == expected_uid
-        end
-      end
-
-      context "and group is specified with a String (group name)" do
-
-        let(:expected_group_name) { Etc.getgrgid(Process.gid).name }
-
-        before do
-          resource.group(expected_group_name)
-        end
-
-        it "sets the group on new_resource to the group name (String) of the group" do
-          current_resource.group.should == expected_group_name
-        end
-
-      end
-
-      context "and group is specified with an Integer (gid)" do
-        let(:expected_gid) { Process.gid }
-
-        before do
-          resource.group(expected_gid)
-        end
-
-        it "sets the group on new_resource to the gid (Integer)" do
-          current_resource.group.should == expected_gid
-        end
-
-      end
-
-      context "and mode is specified as a String" do
-        let(:default_create_mode) { (0100666 - File.umask) }
-        let(:expected_mode) { (default_create_mode & 07777).to_s(8) }
-
-        before do
-          resource.mode(expected_mode)
-        end
-
-        it "sets mode on the new_resource as a String" do
-          current_resource.mode.should == expected_mode
-        end
-      end
-
-      context "and mode is specified as an Integer" do
-        let(:set_mode) { (0100666 - File.umask) & 07777 }
-        let(:expected_mode) { set_mode.to_s(8) }
-
-        before do
-          resource.mode(set_mode)
-        end
+  it_behaves_like "a file resource"
 
-        it "sets mode on the new resource as a String" do
-          current_resource.mode.should == expected_mode
-        end
-      end
-    end
-  end
+  it_behaves_like "a securable resource with reporting"
 
   describe "when running action :touch" do
     context "and the target file does not exist" do
diff --git a/spec/functional/resource/remote_directory_spec.rb b/spec/functional/resource/remote_directory_spec.rb
index 49e84ce12d..12d6b2d03e 100644
--- a/spec/functional/resource/remote_directory_spec.rb
+++ b/spec/functional/resource/remote_directory_spec.rb
@@ -69,6 +69,8 @@ describe Chef::Resource::RemoteDirectory do
 
   it_behaves_like "a directory resource"
 
+  it_behaves_like "a securable resource with reporting"
+
   context "when creating the remote directory with purging disabled" do
 
     context "and the directory does not yet exist" do
diff --git a/spec/functional/resource/remote_file_spec.rb b/spec/functional/resource/remote_file_spec.rb
index 998255e720..619283d632 100644
--- a/spec/functional/resource/remote_file_spec.rb
+++ b/spec/functional/resource/remote_file_spec.rb
@@ -45,6 +45,8 @@ describe Chef::Resource::RemoteFile do
     create_resource
   end
 
+  let(:default_mode) { "600" }
+
   before(:all) do
     @server = TinyServer::Manager.new
     @server.start
@@ -62,4 +64,6 @@ describe Chef::Resource::RemoteFile do
   end
 
   it_behaves_like "a file resource"
+
+  it_behaves_like "a securable resource with reporting"
 end
diff --git a/spec/functional/resource/template_spec.rb b/spec/functional/resource/template_spec.rb
index 0dfdb17324..87a8a3db66 100644
--- a/spec/functional/resource/template_spec.rb
+++ b/spec/functional/resource/template_spec.rb
@@ -49,8 +49,12 @@ describe Chef::Resource::Template do
     create_resource
   end
 
+  let(:default_mode) { "600" }
+
   it_behaves_like "a file resource"
 
+  it_behaves_like "a securable resource with reporting"
+
   context "when the target file does not exist" do
     it "creates the template with the rendered content using the variable attribute when the :create action is run" do
       resource.source('openldap_variable_stuff.conf.erb')
diff --git a/spec/support/shared/functional/securable_resource_with_reporting.rb b/spec/support/shared/functional/securable_resource_with_reporting.rb
new file mode 100644
index 0000000000..f3d4fa6ec5
--- /dev/null
+++ b/spec/support/shared/functional/securable_resource_with_reporting.rb
@@ -0,0 +1,369 @@
+
+shared_examples_for "a securable resource with reporting" do
+
+  let(:current_resource) do
+    provider = resource.provider_for_action(resource.action)
+    provider.load_current_resource
+    provider.current_resource
+  end
+
+  # Default mode varies based on implementation. Providers that use a tempfile
+  # will default to 0600. Providers that use File.open will default to 0666 -
+  # umask
+  # let(:default_mode) { ((0100666 - File.umask) & 07777).to_s(8) }
+
+  describe "reading file security metadata for reporting on unix",:unix_only => true do
+    context "when the target file doesn't exist" do
+      before do
+        resource.action(:create)
+      end
+
+      it "has empty values for file metadata in 'current_resource'" do
+        current_resource.owner.should be_nil
+        current_resource.group.should be_nil
+        current_resource.mode.should be_nil
+      end
+
+      context "and no security metadata is specified in new_resource" do
+        it "sets the metadata values on the new_resource as strings after creating" do
+          resource.run_action(:create)
+          # 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 == default_mode
+        end
+      end
+
+      context "and owner is specified with a String (username) in new_resource", :requires_root => true do
+
+        # TODO/bug: duplicated from the "securable resource" tests
+        let(:expected_user_name) { 'nobody' }
+
+        before do
+          resource.owner(expected_user_name)
+          resource.run_action(:create)
+        end
+
+        it "sets the owner on new_resource to the username (String) of the desired owner" do
+          resource.owner.should == expected_user_name
+        end
+
+      end
+
+      context "and owner is specified with an Integer (uid) in new_resource", :requires_root => true do
+
+        # TODO: duplicated from "securable resource"
+        let(:expected_user_name) { 'nobody' }
+        let(:expected_uid) { Etc.getpwnam(expected_user_name).uid }
+        let(:desired_gid) { 1337 }
+        let(:expected_gid) { 1337 }
+
+        before do
+          resource.owner(expected_uid)
+          resource.run_action(:create)
+        end
+
+        it "sets the owner on new_resource to the uid (Integer) of the desired owner" do
+          resource.owner.should == expected_uid
+        end
+      end
+
+      context "and group is specified with a String (group name)", :requires_root => true do
+
+        let(:expected_group_name) { Etc.getgrent.name }
+
+        before do
+          resource.group(expected_group_name)
+          resource.run_action(:create)
+        end
+
+        it "sets the group on new_resource to the group name (String) of the group" do
+          resource.group.should == expected_group_name
+        end
+
+      end
+
+      context "and group is specified with an Integer (gid)", :requires_root => true do
+        let(:expected_gid) { Etc.getgrent.gid }
+
+        before do
+          resource.group(expected_gid)
+          resource.run_action(:create)
+        end
+
+        it "sets the group on new_resource to the gid (Integer)" do
+          resource.group.should == expected_gid
+        end
+
+      end
+
+      context "and mode is specified as a String" do
+        let(:set_mode) { "0440" }
+        let(:expected_mode) { "440" }
+
+        before do
+          resource.mode(set_mode)
+          resource.run_action(:create)
+        end
+
+        it "sets mode on the new_resource as a String" do
+          resource.mode.should == expected_mode
+        end
+      end
+
+      context "and mode is specified as an Integer" do
+        let(:set_mode) { 00440 }
+
+        let(:expected_mode) { "440" }
+        before do
+          resource.mode(set_mode)
+          resource.run_action(:create)
+        end
+
+        it "sets mode on the new resource as a String" do
+          resource.mode.should == expected_mode
+        end
+      end
+    end
+
+    context "when the target file exists" do
+      before do
+        FileUtils.touch(resource.path)
+        resource.action(:create)
+      end
+
+      context "and no security metadata is specified in new_resource" do
+        it "sets the current values on current resource as strings" 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) & 07777).to_s(8)
+        end
+      end
+
+      context "and owner is specified with a String (username) in new_resource" do
+
+        let(:expected_user_name) { Etc.getpwuid(Process.uid).name }
+
+        before do
+          resource.owner(expected_user_name)
+        end
+
+        it "sets the owner on new_resource to the username (String) of the desired owner" do
+          current_resource.owner.should == expected_user_name
+        end
+
+      end
+
+      context "and owner is specified with an Integer (uid) in new_resource" do
+
+        let(:expected_uid) { Process.uid }
+
+        before do
+          resource.owner(expected_uid)
+        end
+
+        it "sets the owner on new_resource to the uid (Integer) of the desired owner" do
+          current_resource.owner.should == expected_uid
+        end
+      end
+
+      context "and group is specified with a String (group name)" do
+
+        let(:expected_group_name) { Etc.getgrgid(Process.gid).name }
+
+        before do
+          resource.group(expected_group_name)
+        end
+
+        it "sets the group on new_resource to the group name (String) of the group" do
+          current_resource.group.should == expected_group_name
+        end
+
+      end
+
+      context "and group is specified with an Integer (gid)" do
+        let(:expected_gid) { Process.gid }
+
+        before do
+          resource.group(expected_gid)
+        end
+
+        it "sets the group on new_resource to the gid (Integer)" do
+          current_resource.group.should == expected_gid
+        end
+
+      end
+
+      context "and mode is specified as a String" do
+        let(:default_create_mode) { (0100666 - File.umask) }
+        let(:expected_mode) { (default_create_mode & 07777).to_s(8) }
+
+        before do
+          resource.mode(expected_mode)
+        end
+
+        it "sets mode on the new_resource as a String" do
+          current_resource.mode.should == expected_mode
+        end
+      end
+
+      context "and mode is specified as an Integer" do
+        let(:set_mode) { (0100666 - File.umask) & 07777 }
+        let(:expected_mode) { set_mode.to_s(8) }
+
+        before do
+          resource.mode(set_mode)
+        end
+
+        it "sets mode on the new resource as a String" do
+          current_resource.mode.should == expected_mode
+        end
+      end
+    end
+  end
+
+  describe "reading file security metadata for reporting on windows",:windows_only => true, :focus => true do
+
+    ALL_EXPANDED_PERMISSIONS = ["generic read",
+                                "generic write",
+                                "generic execute",
+                                "generic all",
+                                "delete",
+                                "read permissions",
+                                "change permissions",
+                                "take ownership",
+                                "synchronize",
+                                "access system security",
+                                "read data / list directory",
+                                "write data / add file",
+                                "append data / add subdirectory",
+                                "read extended attributes",
+                                "write extended attributes",
+                                "execute / traverse",
+                                "delete child",
+                                "read attributes",
+                                "write attributes"]
+
+
+    context "when the target file doesn't exist" do
+
+      # Windows reporting data should look like this (+/- ish):
+      # { "owner" => "bob", "checksum" => "ffff", "access control" => { "bob" => { "permissions" => ["perm1", "perm2", ...], "flags" => [] }}}
+
+
+      before do
+        resource.action(:create)
+      end
+
+      it "has empty values for file metadata in 'current_resource'" do
+        current_resource.owner.should be_nil
+        current_resource.expanded_rights.should be_nil
+      end
+
+      context "and no security metadata is specified in new_resource" do
+        it "sets the metadata values on the new_resource as strings after creating" do
+          resource.run_action(:create)
+          # todo: most stable way to specify?
+          resource.owner.should == etc.getpwuid(process.uid).name
+          resource.state[:expanded_rights].should == { "CURRENTUSER" => { "permissions" => ALL_EXPANDED_PERMISSIONS, "flags" => [] }}
+          resource.state[:expanded_deny_rights].should == {}
+          resource.state[:inherits].should be_true
+        end
+      end
+
+
+      context "and owner is specified with a string (username) in new_resource" do
+
+        # todo/bug: duplicated from the "securable resource" tests
+        let(:expected_user_name) { 'Guest' }
+
+        before do
+          resource.owner(expected_user_name)
+          resource.run_action(:create)
+        end
+
+        it "sets the owner on new_resource to the username (string) of the desired owner" do
+          resource.owner.should == expected_user_name
+        end
+
+      end
+
+      context "and owner is specified with a fully qualified domain user" do
+
+        # todo: duplicated from "securable resource"
+        let(:expected_user_name) { 'domain\user' }
+
+        before do
+          resource.owner(expected_user_name)
+          resource.run_action(:create)
+        end
+
+        it "sets the owner on new_resource to the fully qualified name of the desired owner" do
+          resource.owner.should == expected_user_name
+        end
+      end
+
+    end
+
+    context "when the target file exists" do
+      before do
+        fileutils.touch(resource.path)
+        resource.action(:create)
+      end
+
+      context "and no security metadata is specified in new_resource" do
+        it "sets the current values on current resource as strings" do
+          # todo: most stable way to specify?
+          current_resource.owner.should == etc.getpwuid(process.uid).name
+          current_resource.expanded_rights.should == { "CURRENTUSER" => ALL_EXPANDED_PERMISSIONS }
+        end
+      end
+
+      context "and owner is specified with a string (username) in new_resource" do
+
+        let(:expected_user_name) { etc.getpwuid(process.uid).name }
+
+        before do
+          resource.owner(expected_user_name)
+        end
+
+        it "sets the owner on current_resource to the username (string) of the desired owner" do
+          current_resource.owner.should == expected_user_name
+        end
+
+      end
+
+      context "and owner is specified as a fully qualified 'domain\\user' in new_resource" do
+
+        let(:expected_user_name) { 'domain\user' }
+
+        before do
+          resource.owner(expected_user_name)
+        end
+
+        it "sets the owner on current_resource to the fully qualified name of the desired owner" do
+          current_resource.owner.should == expected_uid
+        end
+      end
+
+      context "and access rights are specified on the new_resource" do
+        # TODO: before do blah
+
+        it "sets the expanded_rights on the current resource" do
+          pending
+        end
+      end
+
+      context "and no access rights are specified on the current resource" do
+        # TODO: before do blah
+
+        it "sets the expanded rights on the current resource" do
+          pending
+        end
+      end
+
+
+    end
+  end
+end