Merge pull request #3267 from dbjorge/issue-3266

#3266 Fix bad Windows securable_resource functional spec assumptions for default file owners/groups

6 files changed, 133 insertions, 14 deletions

diff --git a/lib/chef/win32/api/security.rb b/lib/chef/win32/api/security.rb
index 95e7f4aab7..4c352a3554 100644
--- a/lib/chef/win32/api/security.rb
+++ b/lib/chef/win32/api/security.rb
@@ -284,6 +284,14 @@ class Chef
              :MaxTokenInfoClass
         ]
 
+        class TOKEN_OWNER < FFI::Struct
+          layout :Owner, :pointer
+        end
+
+        class TOKEN_PRIMARY_GROUP < FFI::Struct
+          layout :PrimaryGroup, :pointer
+        end
+
         # https://msdn.microsoft.com/en-us/library/windows/desktop/aa379572%28v=vs.85%29.aspx
         SECURITY_IMPERSONATION_LEVEL = enum :SECURITY_IMPERSONATION_LEVEL, [
              :SecurityAnonymous,
diff --git a/lib/chef/win32/security.rb b/lib/chef/win32/security.rb
index a5a845e55c..5c83180bc0 100644
--- a/lib/chef/win32/security.rb
+++ b/lib/chef/win32/security.rb
@@ -273,6 +273,36 @@ class Chef
         [ present.read_char != 0, acl.null? ? nil : ACL.new(acl, security_descriptor), defaulted.read_char != 0 ]
       end
 
+      def self.get_token_information_owner(token)
+        owner_result_size = FFI::MemoryPointer.new(:ulong)
+        if GetTokenInformation(token.handle.handle, :TokenOwner, nil, 0, owner_result_size)
+          raise "Expected ERROR_INSUFFICIENT_BUFFER from GetTokenInformation, and got no error!"
+        elsif FFI::LastError.error != ERROR_INSUFFICIENT_BUFFER
+          Chef::ReservedNames::Win32::Error.raise!
+        end
+        owner_result_storage = FFI::MemoryPointer.new owner_result_size.read_ulong
+        unless GetTokenInformation(token.handle.handle, :TokenOwner, owner_result_storage, owner_result_size.read_ulong, owner_result_size)
+          Chef::ReservedNames::Win32::Error.raise!
+        end
+        owner_result = TOKEN_OWNER.new owner_result_storage
+        SID.new(owner_result[:Owner], owner_result_storage)
+      end
+
+      def self.get_token_information_primary_group(token)
+        group_result_size = FFI::MemoryPointer.new(:ulong)
+        if GetTokenInformation(token.handle.handle, :TokenPrimaryGroup, nil, 0, group_result_size)
+          raise "Expected ERROR_INSUFFICIENT_BUFFER from GetTokenInformation, and got no error!"
+        elsif FFI::LastError.error != ERROR_INSUFFICIENT_BUFFER
+          Chef::ReservedNames::Win32::Error.raise!
+        end
+        group_result_storage = FFI::MemoryPointer.new group_result_size.read_ulong
+        unless GetTokenInformation(token.handle.handle, :TokenPrimaryGroup, group_result_storage, group_result_size.read_ulong, group_result_size)
+          Chef::ReservedNames::Win32::Error.raise!
+        end
+        group_result = TOKEN_PRIMARY_GROUP.new group_result_storage
+        SID.new(group_result[:PrimaryGroup], group_result_storage)
+      end
+
       def self.initialize_acl(acl_size)
         acl = FFI::MemoryPointer.new acl_size
         unless InitializeAcl(acl, acl_size, ACL_REVISION)
@@ -418,6 +448,10 @@ class Chef
         [ SecurityDescriptor.new(absolute_sd), SID.new(owner), SID.new(group), ACL.new(dacl), ACL.new(sacl) ]
       end
 
+      def self.open_current_process_token(desired_access = TOKEN_READ)
+        open_process_token(Chef::ReservedNames::Win32::Process.get_current_process, desired_access)
+      end
+
       def self.open_process_token(process, desired_access)
         process = process.handle if process.respond_to?(:handle)
         process = process.handle if process.respond_to?(:handle)
@@ -516,7 +550,7 @@ class Chef
 
       def self.with_privileges(*privilege_names)
         # Set privileges
-        token = open_process_token(Chef::ReservedNames::Win32::Process.get_current_process, TOKEN_READ | TOKEN_ADJUST_PRIVILEGES)
+        token = open_current_process_token(TOKEN_READ | TOKEN_ADJUST_PRIVILEGES)
         old_privileges = token.enable_privileges(*privilege_names)
 
         # Let the caller do their privileged stuff
@@ -536,7 +570,7 @@ class Chef
 
           true
         else
-          process_token = open_process_token(Chef::ReservedNames::Win32::Process.get_current_process, TOKEN_READ)
+          process_token = open_current_process_token(TOKEN_READ)
           elevation_result = FFI::Buffer.new(:ulong)
           elevation_result_size = FFI::MemoryPointer.new(:uint32)
           success = GetTokenInformation(process_token.handle.handle, :TokenElevation, elevation_result, 4, elevation_result_size)
diff --git a/lib/chef/win32/security/sid.rb b/lib/chef/win32/security/sid.rb
index 8e9407dc80..f8bd934876 100644
--- a/lib/chef/win32/security/sid.rb
+++ b/lib/chef/win32/security/sid.rb
@@ -203,6 +203,23 @@ class Chef
           SID.from_account("#{::ENV['USERDOMAIN']}\\#{::ENV['USERNAME']}")
         end
 
+        # See https://technet.microsoft.com/en-us/library/cc961992.aspx
+        # In practice, this is SID.Administrators if the current_user is an admin (even if not
+        # running elevated), and is current_user otherwise. On win2k3, it technically can be
+        # current_user in all cases if a certain group policy is set.
+        def self.default_security_object_owner
+          token = Chef::ReservedNames::Win32::Security.open_current_process_token
+          Chef::ReservedNames::Win32::Security.get_token_information_owner(token)
+        end
+
+        # See https://technet.microsoft.com/en-us/library/cc961996.aspx
+        # In practice, this seems to be SID.current_user for Microsoft Accounts, the current
+        # user's Domain Users group for domain accounts, and SID.None otherwise.
+        def self.default_security_object_group
+          token = Chef::ReservedNames::Win32::Security.open_current_process_token
+          Chef::ReservedNames::Win32::Security.get_token_information_primary_group(token)
+        end
+
         def self.admin_account_name
           @admin_account_name ||= begin
             admin_account_name = nil
diff --git a/spec/functional/win32/sid_spec.rb b/spec/functional/win32/sid_spec.rb
new file mode 100644
index 0000000000..1f5f66178a
--- /dev/null
+++ b/spec/functional/win32/sid_spec.rb
@@ -0,0 +1,55 @@
+#
+# Author:: Dan Bjorge (<dbjorge@gmail.com>)
+# Copyright:: Copyright (c) 2015 Dan Bjorge
+# License:: Apache License, Version 2.0
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+require 'spec_helper'
+if Chef::Platform.windows?
+  require 'chef/win32/security'
+end
+
+describe 'Chef::ReservedNames::Win32::SID', :windows_only do
+  if Chef::Platform.windows?
+    SID ||= Chef::ReservedNames::Win32::Security::SID
+  end
+
+  it 'should resolve default_security_object_group as a sane user group', :windows_not_domain_joined_only do
+    # Domain accounts: domain-specific Domain Users SID
+    # Microsoft Accounts: SID.current_user
+    # Else: SID.None
+    expect(SID.default_security_object_group).to eq(SID.None).or eq(SID.current_user)
+  end
+
+  context 'running as an elevated administrator user' do
+    it 'should resolve default_security_object_owner as the Administrators group' do
+      expect(SID.default_security_object_owner).to eq(SID.Administrators)
+    end
+  end
+
+  context 'running as a non-elevated administrator user' do
+    it 'should resolve default_security_object_owner as the current user' do
+      skip 'requires user support in mixlib-shellout, see security_spec.rb'
+      expect(SID.default_security_object_owner).to eq(SID.Administrators)
+    end
+  end
+
+  context 'running as a non-elevated, non-administrator user' do
+    it 'should resolve default_security_object_owner as the current user' do
+      skip 'requires user support in mixlib-shellout, see security_spec.rb'
+      expect(SID.default_security_object_owner).to eq(SID.current_user)
+    end
+  end
+end
diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb
index fe96c723b6..8b5e42e5b6 100644
--- a/spec/spec_helper.rb
+++ b/spec/spec_helper.rb
@@ -130,6 +130,7 @@ RSpec.configure do |config|
   config.filter_run_excluding :windows_powershell_dsc_only => true unless windows_powershell_dsc?
   config.filter_run_excluding :windows_powershell_no_dsc_only => true unless ! windows_powershell_dsc?
   config.filter_run_excluding :windows_domain_joined_only => true unless windows_domain_joined?
+  config.filter_run_excluding :windows_not_domain_joined_only => true if windows_domain_joined?
   config.filter_run_excluding :solaris_only => true unless solaris?
   config.filter_run_excluding :system_windows_service_gem_only => true unless system_windows_service_gem?
   config.filter_run_excluding :unix_only => true unless unix?
diff --git a/spec/support/shared/functional/securable_resource.rb b/spec/support/shared/functional/securable_resource.rb
index cd8c2a166b..bde508b14d 100644
--- a/spec/support/shared/functional/securable_resource.rb
+++ b/spec/support/shared/functional/securable_resource.rb
@@ -313,10 +313,10 @@ shared_examples_for "a securable resource without existing target" do
   context "on Windows", :windows_only do
     include_context "use Windows permissions"
 
-    it "sets owner to Administrators on create if owner is not specified" do
+    it "leaves owner as system default on create if owner is not specified" do
       expect(File.exist?(path)).to eq(false)
       resource.run_action(:create)
-      expect(descriptor.owner).to eq(SID.Administrators)
+      expect(descriptor.owner).to eq(SID.default_security_object_owner)
     end
 
     it "sets owner when owner is specified" do
@@ -336,22 +336,24 @@ shared_examples_for "a securable resource without existing target" do
     end
 
     it "leaves owner alone if owner is not specified and resource already exists" do
-      # Set owner to Guest so it's not the same as the current user (which is the default on create)
-      resource.owner 'Guest'
+      arbitrary_non_default_owner = SID.Guest
+      expect(arbitrary_non_default_owner).not_to eq(SID.default_security_object_owner)
+
+      resource.owner 'Guest' # Change to arbitrary_non_default_owner once issue #1508 is fixed
       resource.run_action(:create)
-      expect(descriptor.owner).to eq(SID.Guest)
+      expect(descriptor.owner).to eq(arbitrary_non_default_owner)
 
       new_resource = create_resource
       expect(new_resource.owner).to eq(nil)
       new_resource.run_action(:create)
-      expect(descriptor.owner).to eq(SID.Guest)
+      expect(descriptor.owner).to eq(arbitrary_non_default_owner)
     end
 
-    it "sets group to None on create if group is not specified" do
+    it "leaves group as system default on create if group is not specified" do
       expect(resource.group).to eq(nil)
       expect(File.exist?(path)).to eq(false)
       resource.run_action(:create)
-      expect(descriptor.group).to eq(SID.None)
+      expect(descriptor.group).to eq(SID.default_security_object_group)
     end
 
     it "sets group when group is specified" do
@@ -372,15 +374,17 @@ shared_examples_for "a securable resource without existing target" do
     end
 
     it "leaves group alone if group is not specified and resource already exists" do
-      # Set group to Everyone so it's not the default (None)
-      resource.group 'Everyone'
+      arbitrary_non_default_group = SID.Everyone
+      expect(arbitrary_non_default_group).not_to eq(SID.default_security_object_group)
+
+      resource.group 'Everyone' # Change to arbitrary_non_default_group once issue #1508 is fixed
       resource.run_action(:create)
-      expect(descriptor.group).to eq(SID.Everyone)
+      expect(descriptor.group).to eq(arbitrary_non_default_group)
 
       new_resource = create_resource
       expect(new_resource.group).to eq(nil)
       new_resource.run_action(:create)
-      expect(descriptor.group).to eq(SID.Everyone)
+      expect(descriptor.group).to eq(arbitrary_non_default_group)
     end
 
     describe "with rights and deny_rights attributes" do