Merge pull request #8168 from MsysTechnologiesllc/Vijay/MSYS-958_write_permissions_does_not_work_properly_on_windows

Fix for write permissions were not working properly on windows

6 files changed, 125 insertions, 88 deletions

diff --git a/lib/chef/file_access_control/windows.rb b/lib/chef/file_access_control/windows.rb
index 2c6b69c257..a7cefdc6d7 100644
--- a/lib/chef/file_access_control/windows.rb
+++ b/lib/chef/file_access_control/windows.rb
@@ -90,11 +90,13 @@ class Chef
         target_acl.each do |target_ace|
           if target_ace.flags & INHERIT_ONLY_ACE == 0
             self_ace = target_ace.dup
-            self_ace.flags = 0
+            # We need flag value which is already being set in case of WRITE permissions as 3, so we will not be overwriting it with the hard coded value.
+            self_ace.flags = 0 unless target_ace.mask == Chef::ReservedNames::Win32::API::Security::WRITE
             self_ace.mask = securable_object.predict_rights_mask(target_ace.mask)
             new_target_acl << self_ace
           end
-          if target_ace.flags & (CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE) != 0
+          # As there is no inheritence needed in case of WRITE permissions.
+          if target_ace.mask != Chef::ReservedNames::Win32::API::Security::WRITE && target_ace.flags & (CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE) != 0
             children_ace = target_ace.dup
             children_ace.flags |= INHERIT_ONLY_ACE
             new_target_acl << children_ace
@@ -220,7 +222,7 @@ class Chef
           when :read_execute
             mask |= GENERIC_READ | GENERIC_EXECUTE
           when :write
-            mask |= GENERIC_WRITE
+            mask |= WRITE
           else
             # Otherwise, assume it's an integer specifying the actual flags
             mask |= permission
diff --git a/lib/chef/win32/api/security.rb b/lib/chef/win32/api/security.rb
index 277e85a26b..5c3dd69c3e 100644
--- a/lib/chef/win32/api/security.rb
+++ b/lib/chef/win32/api/security.rb
@@ -140,6 +140,8 @@ class Chef
           FILE_READ_EA | SYNCHRONIZE
         FILE_GENERIC_WRITE         = STANDARD_RIGHTS_WRITE | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA | SYNCHRONIZE
         FILE_GENERIC_EXECUTE       = STANDARD_RIGHTS_EXECUTE | FILE_READ_ATTRIBUTES | FILE_EXECUTE | SYNCHRONIZE
+        WRITE                      = FILE_WRITE_DATA | FILE_APPEND_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA
+        SUBFOLDERS_AND_FILES_ONLY  = INHERIT_ONLY_ACE | CONTAINER_INHERIT_ACE | OBJECT_INHERIT_ACE
         # Access Token Rights (for OpenProcessToken)
         # Access Rights for Access-Token Objects (used in OpenProcessToken)
         TOKEN_ASSIGN_PRIMARY = 0x0001
diff --git a/spec/functional/resource/link_spec.rb b/spec/functional/resource/link_spec.rb
index 4464b6ed69..d86a904098 100644
--- a/spec/functional/resource/link_spec.rb
+++ b/spec/functional/resource/link_spec.rb
@@ -417,11 +417,11 @@ describe Chef::Resource::Link do
 
         it_behaves_like "a securable resource without existing target" do
           let(:path) { target_file }
-          def allowed_acl(sid, expected_perms)
+          def allowed_acl(sid, expected_perms, _flags = 0)
             [ ACE.access_allowed(sid, expected_perms[:specific]) ]
           end
 
-          def denied_acl(sid, expected_perms)
+          def denied_acl(sid, expected_perms, _flags = 0)
             [ ACE.access_denied(sid, expected_perms[:specific]) ]
           end
 
diff --git a/spec/support/shared/functional/directory_resource.rb b/spec/support/shared/functional/directory_resource.rb
index 5e5e2bb360..4fb08479e6 100644
--- a/spec/support/shared/functional/directory_resource.rb
+++ b/spec/support/shared/functional/directory_resource.rb
@@ -65,18 +65,20 @@ shared_examples_for "a directory resource" do
     end
 
     # Set up the context for security tests
-    def allowed_acl(sid, expected_perms)
-      [
-       ACE.access_allowed(sid, expected_perms[:specific]),
-       ACE.access_allowed(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE)),
-      ]
+    def allowed_acl(sid, expected_perms, flags = 0)
+      acl = [ ACE.access_allowed(sid, expected_perms[:specific], flags) ]
+      if expected_perms[:generic]
+        acl << ACE.access_allowed(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::SUBFOLDERS_AND_FILES_ONLY))
+      end
+      acl
     end
 
-    def denied_acl(sid, expected_perms)
-      [
-       ACE.access_denied(sid, expected_perms[:specific]),
-       ACE.access_denied(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE)),
-      ]
+    def denied_acl(sid, expected_perms, flags = 0)
+      acl = [ ACE.access_denied(sid, expected_perms[:specific], flags) ]
+      if expected_perms[:generic]
+        acl << ACE.access_denied(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::SUBFOLDERS_AND_FILES_ONLY))
+      end
+      acl
     end
 
     def parent_inheritable_acls
diff --git a/spec/support/shared/functional/file_resource.rb b/spec/support/shared/functional/file_resource.rb
index 8ae5db6a57..db947614b3 100644
--- a/spec/support/shared/functional/file_resource.rb
+++ b/spec/support/shared/functional/file_resource.rb
@@ -899,11 +899,11 @@ shared_examples_for "a configured file resource" do
   end
 
   # Set up the context for security tests
-  def allowed_acl(sid, expected_perms)
+  def allowed_acl(sid, expected_perms, _flags = 0)
     [ ACE.access_allowed(sid, expected_perms[:specific]) ]
   end
 
-  def denied_acl(sid, expected_perms)
+  def denied_acl(sid, expected_perms, _flags = 0)
     [ ACE.access_denied(sid, expected_perms[:specific]) ]
   end
 
diff --git a/spec/support/shared/functional/securable_resource.rb b/spec/support/shared/functional/securable_resource.rb
index 2abae030c2..18e7243453 100644
--- a/spec/support/shared/functional/securable_resource.rb
+++ b/spec/support/shared/functional/securable_resource.rb
@@ -117,8 +117,7 @@ shared_context "use Windows permissions", :windows_only do
 
   let(:expected_write_perms) do
     {
-      generic: Chef::ReservedNames::Win32::API::Security::GENERIC_WRITE,
-      specific: Chef::ReservedNames::Win32::API::Security::FILE_GENERIC_WRITE,
+      specific: Chef::ReservedNames::Win32::API::Security::WRITE,
     }
   end
 
@@ -136,6 +135,8 @@ shared_context "use Windows permissions", :windows_only do
     }
   end
 
+  let (:write_flag) { 3 }
+
   RSpec::Matchers.define :have_expected_properties do |mask, type, flags|
     match do |ace|
       ace.mask == mask &&
@@ -363,78 +364,108 @@ shared_examples_for "a securable resource without existing target" do
       expect(descriptor.group).to eq(arbitrary_non_default_group)
     end
 
-    describe "with rights and deny_rights attributes" do
-
-      it "correctly sets :read rights" do
-        resource.rights(:read, "Guest")
-        resource.run_action(:create)
-        expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_read_perms))
+    describe "#allowed_acl" do
+      context "correctly sets" do
+
+        it ":read rights" do
+          resource.rights(:read, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_read_perms))
+        end
+
+        it ":read_execute rights" do
+          resource.rights(:read_execute, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_read_execute_perms))
+        end
+
+        it ":write rights" do
+          resource.rights(:write, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_write_perms, write_flag))
+        end
+
+        it ":modify rights" do
+          resource.rights(:modify, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_modify_perms))
+        end
+
+        it ":full_control rights" do
+          resource.rights(:full_control, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_full_control_perms))
+        end
+
+        it "multiple rights" do
+          resource.rights(:read, "Everyone")
+          resource.rights(:modify, "Guest")
+          resource.run_action(:create)
+
+          expect(explicit_aces).to eq(
+            allowed_acl(SID.Everyone, expected_read_perms) +
+            allowed_acl(SID.Guest, expected_modify_perms)
+          )
+        end
       end
+    end
 
-      it "correctly sets :read_execute rights" do
-        resource.rights(:read_execute, "Guest")
-        resource.run_action(:create)
-        expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_read_execute_perms))
-      end
-
-      it "correctly sets :write rights" do
-        resource.rights(:write, "Guest")
-        resource.run_action(:create)
-        expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_write_perms))
-      end
-
-      it "correctly sets :modify rights" do
-        resource.rights(:modify, "Guest")
-        resource.run_action(:create)
-        expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_modify_perms))
-      end
-
-      it "correctly sets :full_control rights" do
-        resource.rights(:full_control, "Guest")
-        resource.run_action(:create)
-        expect(explicit_aces).to eq(allowed_acl(SID.Guest, expected_full_control_perms))
-      end
-
-      it "correctly sets deny_rights" do
-        # deny is an ACE with full rights, but is a deny type ace, not an allow type
-        resource.deny_rights(:full_control, "Guest")
-        resource.run_action(:create)
-        expect(explicit_aces).to eq(denied_acl(SID.Guest, expected_full_control_perms))
-      end
-
-      it "Sets multiple rights" do
-        resource.rights(:read, "Everyone")
-        resource.rights(:modify, "Guest")
-        resource.run_action(:create)
-
-        expect(explicit_aces).to eq(
-          allowed_acl(SID.Everyone, expected_read_perms) +
-          allowed_acl(SID.Guest, expected_modify_perms)
-        )
-      end
-
-      it "Sets deny_rights ahead of rights" do
-        resource.rights(:read, "Everyone")
-        resource.deny_rights(:modify, "Guest")
-        resource.run_action(:create)
-
-        expect(explicit_aces).to eq(
-          denied_acl(SID.Guest, expected_modify_perms) +
-          allowed_acl(SID.Everyone, expected_read_perms)
-        )
-      end
-
-      it "Sets deny_rights ahead of rights when specified in reverse order" do
-        resource.deny_rights(:modify, "Guest")
-        resource.rights(:read, "Everyone")
-        resource.run_action(:create)
-
-        expect(explicit_aces).to eq(
-          denied_acl(SID.Guest, expected_modify_perms) +
-          allowed_acl(SID.Everyone, expected_read_perms)
-        )
+    describe "#denied_acl" do
+      context "correctly sets" do
+
+        it ":read rights" do
+          resource.deny_rights(:read, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(denied_acl(SID.Guest, expected_read_perms))
+        end
+
+        it ":read_execute rights" do
+          resource.deny_rights(:read_execute, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(denied_acl(SID.Guest, expected_read_execute_perms))
+        end
+
+        it ":write rights" do
+          resource.deny_rights(:write, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(denied_acl(SID.Guest, expected_write_perms, write_flag))
+        end
+
+        it ":modify rights" do
+          resource.deny_rights(:modify, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(denied_acl(SID.Guest, expected_modify_perms))
+        end
+
+        it ":full_control rights" do
+          # deny is an ACE with full rights, but is a deny type ace, not an allow type
+          resource.deny_rights(:full_control, "Guest")
+          resource.run_action(:create)
+          expect(explicit_aces).to eq(denied_acl(SID.Guest, expected_full_control_perms))
+        end
+
+        it "deny_rights ahead of rights" do
+          resource.rights(:read, "Everyone")
+          resource.deny_rights(:modify, "Guest")
+          resource.run_action(:create)
+
+          expect(explicit_aces).to eq(
+            denied_acl(SID.Guest, expected_modify_perms) +
+            allowed_acl(SID.Everyone, expected_read_perms)
+          )
+        end
+
+        it "deny_rights ahead of rights when specified in reverse order" do
+          resource.deny_rights(:modify, "Guest")
+          resource.rights(:read, "Everyone")
+          resource.run_action(:create)
+
+          expect(explicit_aces).to eq(
+            denied_acl(SID.Guest, expected_modify_perms) +
+            allowed_acl(SID.Everyone, expected_read_perms)
+          )
+        end
       end
-
     end
 
     context "with a mode attribute" do