Minor fixes as per the review comments

- Using proper verbiage for constant SUBFOLDERS_AND_FILES_ONLY
- Source
https://metacpan.org/pod/Win32::Security::ACE#dbmAceFlags
- Minor DRY up and Fixes
- Ensured chefstyle

Signed-off-by: vijaymmali1990 <vijay.mali@msystechnologies.com>

7 files changed, 110 insertions, 102 deletions

diff --git a/lib/chef/file_access_control/windows.rb b/lib/chef/file_access_control/windows.rb
index 6937912849..a7cefdc6d7 100644
--- a/lib/chef/file_access_control/windows.rb
+++ b/lib/chef/file_access_control/windows.rb
@@ -90,10 +90,12 @@ class Chef
         target_acl.each do |target_ace|
           if target_ace.flags & INHERIT_ONLY_ACE == 0
             self_ace = target_ace.dup
+            # 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
+          # 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
@@ -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 bac4ab5450..5c3dd69c3e 100644
--- a/lib/chef/win32/api/security.rb
+++ b/lib/chef/win32/api/security.rb
@@ -115,7 +115,6 @@ class Chef
         STANDARD_RIGHTS_EXECUTE           = READ_CONTROL
         STANDARD_RIGHTS_ALL               = 0x001F0000
         SPECIFIC_RIGHTS_ALL               = 0x0000FFFF
-
         # Access System Security Right
         ACCESS_SYSTEM_SECURITY            = 0x01000000
         # File/Directory Specific Rights
@@ -142,6 +141,7 @@ class Chef
         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/lib/chef/win32/security/ace.rb b/lib/chef/win32/security/ace.rb
index ba81c44269..d593513983 100644
--- a/lib/chef/win32/security/ace.rb
+++ b/lib/chef/win32/security/ace.rb
@@ -113,7 +113,7 @@ class Chef
           struct[:AceType] = type
           struct[:AceFlags] = flags
           struct[:AceSize] = size_needed
-          struct[:Mask] = mask == Security::GENERIC_WRITE ? Security::WRITE : mask
+          struct[:Mask] = mask
           Chef::ReservedNames::Win32::Memory.memcpy(struct.pointer + struct.offset_of(:SidStart), sid.pointer, sid.size)
           ACE.new(struct.pointer)
         end
diff --git a/spec/functional/resource/link_spec.rb b/spec/functional/resource/link_spec.rb
index 4c8545e60b..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, flags = 0)
+          def allowed_acl(sid, expected_perms, _flags = 0)
             [ ACE.access_allowed(sid, expected_perms[:specific]) ]
           end
 
-          def denied_acl(sid, expected_perms, flags = 0)
+          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 c910e7c668..4fb08479e6 100644
--- a/spec/support/shared/functional/directory_resource.rb
+++ b/spec/support/shared/functional/directory_resource.rb
@@ -68,7 +68,7 @@ shared_examples_for "a directory resource" do
     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::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE))
+        acl << ACE.access_allowed(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::SUBFOLDERS_AND_FILES_ONLY))
       end
       acl
     end
@@ -76,7 +76,7 @@ shared_examples_for "a directory resource" do
     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::INHERIT_ONLY_ACE | Chef::ReservedNames::Win32::API::Security::CONTAINER_INHERIT_ACE | Chef::ReservedNames::Win32::API::Security::OBJECT_INHERIT_ACE))
+        acl << ACE.access_denied(sid, expected_perms[:generic], (Chef::ReservedNames::Win32::API::Security::SUBFOLDERS_AND_FILES_ONLY))
       end
       acl
     end
diff --git a/spec/support/shared/functional/file_resource.rb b/spec/support/shared/functional/file_resource.rb
index 8aa4ffb65e..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, flags = 0)
+  def allowed_acl(sid, expected_perms, _flags = 0)
     [ ACE.access_allowed(sid, expected_perms[:specific]) ]
   end
 
-  def denied_acl(sid, expected_perms, flags = 0)
+  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 0a7eac442d..18e7243453 100644
--- a/spec/support/shared/functional/securable_resource.rb
+++ b/spec/support/shared/functional/securable_resource.rb
@@ -364,102 +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))
-      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, write_flag))
-      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))
+    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 :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 :read deny_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 "correctly sets :read_execute deny_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 "correctly sets :write deny_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 "correctly sets :modify deny_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 "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