Ensure Access Controls on link resourceCOOL-614/tduffield/link-keep-access-controls

When a link resource needs to change its destination, there was a bug in
the logic flow that would cause the access controls on the resulting
symlink to revert to the default access controls. This commit closes
that hole by re-evaluating the current access controls after creating
the new symlink.

Signed-off-by: Tom Duffield <tom@chef.io>

2 files changed, 68 insertions, 1 deletions

diff --git a/lib/chef/provider/link.rb b/lib/chef/provider/link.rb
index 16d30319b3..5add453072 100644
--- a/lib/chef/provider/link.rb
+++ b/lib/chef/provider/link.rb
@@ -121,6 +121,12 @@ class Chef
               file_class.symlink(canonicalize(@new_resource.to), @new_resource.target_file)
               Chef::Log.debug("#{@new_resource} created #{@new_resource.link_type} link from #{@new_resource.target_file} -> #{@new_resource.to}")
               Chef::Log.info("#{@new_resource} created")
+              # file_class.symlink will create the link with default access controls.
+              # This means that the access controls of the file could be different
+              # than those captured during the initial evaluation of current_resource.
+              # We need to re-evaluate the current_resource to ensure that the desired
+              # access controls are applied.
+              ScanAccessControl.new(@new_resource, @current_resource).set_all!
             end
           elsif @new_resource.link_type == :hard
             converge_by("create hard link at #{@new_resource.target_file} to #{@new_resource.to}") do
diff --git a/spec/functional/resource/link_spec.rb b/spec/functional/resource/link_spec.rb
index de6976448e..256f350640 100644
--- a/spec/functional/resource/link_spec.rb
+++ b/spec/functional/resource/link_spec.rb
@@ -20,6 +20,7 @@ require "spec_helper"
 
 if windows?
   require "chef/win32/file" #probably need this in spec_helper
+  require "chef/win32/security"
 end
 
 describe Chef::Resource::Link do
@@ -62,6 +63,18 @@ describe Chef::Resource::Link do
     end
   end
 
+  def node
+    node = Chef::Node.new
+    node.consume_external_attrs(ohai.data, {})
+    node
+  end
+
+  def user(user)
+    usr = Chef::Resource.resource_for_node(:user, node).new(user, run_context)
+    usr.password("ComplexPass11!") if windows?
+    usr
+  end
+
   def cleanup_link(path)
     if windows? && File.directory?(path)
       # If the link target is a directory rm_rf doesn't work all the
@@ -108,6 +121,42 @@ describe Chef::Resource::Link do
     end
   end
 
+  let(:test_user) { windows? ? nil : ENV["USER"] }
+
+  def expected_owner
+    if windows?
+      get_sid(test_user)
+    else
+      test_user
+    end
+  end
+
+  def get_sid(value)
+    if value.kind_of?(String)
+      Chef::ReservedNames::Win32::Security::SID.from_account(value)
+    elsif value.kind_of?(Chef::ReservedNames::Win32::Security::SID)
+      value
+    else
+      raise "Must specify username or SID: #{value}"
+    end
+  end
+
+  def chown(file, user)
+    if windows?
+      Chef::ReservedNames::Win32::Security::SecurableObject.new(file).owner = get_sid(user)
+    else
+      File.lchown(Etc.getpwnam(user).uid, Etc.getpwnam(user).gid, file)
+    end
+  end
+
+  def owner(file)
+    if windows?
+      Chef::ReservedNames::Win32::Security::SecurableObject.new(file).security_descriptor.owner
+    else
+      Etc.getpwuid(File.lstat(file).uid).name
+    end
+  end
+
   def create_resource
     node = Chef::Node.new
     events = Chef::EventDispatch::Dispatcher.new
@@ -184,6 +233,7 @@ describe Chef::Resource::Link do
         it "links to the target file" do
           expect(symlink?(target_file)).to be_truthy
           expect(readlink(target_file)).to eq(canonicalize(to))
+          expect(owner(target_file)).to eq(expected_owner) unless test_user.nil?
         end
         it "marks the resource updated" do
           expect(resource).to be_updated
@@ -205,6 +255,7 @@ describe Chef::Resource::Link do
         it "leaves the file linked" do
           expect(symlink?(target_file)).to be_truthy
           expect(readlink(target_file)).to eq(canonicalize(to))
+          expect(owner(target_file)).to eq(expected_owner) unless test_user.nil?
         end
         it "does not mark the resource updated" do
           expect(resource).not_to be_updated
@@ -291,13 +342,23 @@ describe Chef::Resource::Link do
               expect(File.exists?(to)).to be_truthy
             end
           end
-          context "pointing somewhere else" do
+          context "pointing somewhere else", :requires_root_or_running_windows do
+            let(:test_user) { "test-link-user" }
+            before do
+              user(test_user).run_action(:create)
+            end
+            after do
+              user(test_user).run_action(:remove)
+            end
             before(:each) do
+              resource.owner(test_user)
               @other_target = File.join(test_file_dir, make_tmpname("other_spec"))
               File.open(@other_target, "w") { |file| file.write("eek") }
               symlink(@other_target, target_file)
+              chown(target_file, test_user)
               expect(symlink?(target_file)).to be_truthy
               expect(readlink(target_file)).to eq(canonicalize(@other_target))
+              expect(owner(target_file)).to eq(expected_owner)
             end
             after(:each) do
               File.delete(@other_target)