diff --git a/lib/puppet/provider/file/windows.rb b/lib/puppet/provider/file/windows.rb
index 235a3025f..b3475ebe1 100644
--- a/lib/puppet/provider/file/windows.rb
+++ b/lib/puppet/provider/file/windows.rb
@@ -1,92 +1,88 @@
 Puppet::Type.type(:file).provide :windows do
   desc "Uses Microsoft Windows functionality to manage file ownership and permissions."
 
   confine :operatingsystem => :windows
 
   include Puppet::Util::Warnings
 
   if Puppet.features.microsoft_windows?
     require 'puppet/util/windows'
     require 'puppet/util/adsi'
     include Puppet::Util::Windows::Security
   end
 
   # Determine if the account is valid, and if so, return the UID
   def name2id(value)
     Puppet::Util::Windows::Security.name_to_sid(value)
   end
 
   # If it's a valid SID, get the name. Otherwise, it's already a name,
   # so just return it.
   def id2name(id)
-    begin
-      if Puppet::Util::Windows::Security.string_to_sid_ptr(id)
-        return Puppet::Util::Windows::Security.sid_to_name(id)
-      end
-    rescue Puppet::Util::Windows::Error => e
-      raise unless e.code == ERROR_INVALID_SID_STRUCTURE
+    if Puppet::Util::Windows::Security.valid_sid?(id)
+      Puppet::Util::Windows::Security.sid_to_name(id)
+    else
+      id
     end
-
-    id
   end
 
   # We use users and groups interchangeably, so use the same methods for both
   # (the type expects different methods, so we have to oblige).
   alias :uid2name :id2name
   alias :gid2name :id2name
 
   alias :name2gid :name2id
   alias :name2uid :name2id
 
   def owner
     return :absent unless resource.exist?
     get_owner(resource[:path])
   end
 
   def owner=(should)
     begin
       set_owner(should, resource[:path])
     rescue => detail
       raise Puppet::Error, "Failed to set owner to '#{should}': #{detail}"
     end
   end
 
   def group
     return :absent unless resource.exist?
     get_group(resource[:path])
   end
 
   def group=(should)
     begin
       set_group(should, resource[:path])
     rescue => detail
       raise Puppet::Error, "Failed to set group to '#{should}': #{detail}"
     end
   end
 
   def mode
     if resource.exist?
       mode = get_mode(resource[:path])
       mode ? mode.to_s(8) : :absent
     else
       :absent
     end
   end
 
   def mode=(value)
     begin
       set_mode(value.to_i(8), resource[:path])
     rescue => detail
       error = Puppet::Error.new("failed to set mode #{mode} on #{resource[:path]}: #{detail.message}")
       error.set_backtrace detail.backtrace
       raise error
     end
     :file_changed
   end
 
   def validate
     if [:owner, :group, :mode].any?{|p| resource[p]} and !supports_acl?(resource[:path])
       resource.fail("Can only manage owner, group, and mode on filesystems that support Windows ACLs, such as NTFS")
     end
   end
 end
diff --git a/lib/puppet/util/windows/sid.rb b/lib/puppet/util/windows/sid.rb
index 2c15755bb..cd321eacb 100644
--- a/lib/puppet/util/windows/sid.rb
+++ b/lib/puppet/util/windows/sid.rb
@@ -1,89 +1,96 @@
 require 'puppet/util/windows'
 
 module Puppet::Util::Windows
   module SID
     require 'windows/security'
     include ::Windows::Security
 
     require 'windows/memory'
     include ::Windows::Memory
 
     require 'windows/msvcrt/string'
     include ::Windows::MSVCRT::String
 
     # missing from Windows::Error
     ERROR_NONE_MAPPED           = 1332
     ERROR_INVALID_SID_STRUCTURE = 1337
 
     # Convert an account name, e.g. 'Administrators' into a SID string,
     # e.g. 'S-1-5-32-544'. The name can be specified as 'Administrators',
     # 'BUILTIN\Administrators', or 'S-1-5-32-544', and will return the
     # SID. Returns nil if the account doesn't exist.
     def name_to_sid(name)
       # Apparently, we accept a symbol..
       name = name.to_s if name
 
       # if it's in SID string form, return it, otherwise, lookup sid
       is_sid = Win32::Security::SID.string_to_sid(name) rescue nil
 
       is_sid ? name : Win32::Security::SID.new(name).to_s
     rescue
       nil
     end
 
     # Convert a SID string, e.g. "S-1-5-32-544" to a name,
     # e.g. 'BUILTIN\Administrators'. Returns nil if an account
     # for that SID does not exist.
     def sid_to_name(value)
       sid = Win32::Security::SID.new(Win32::Security::SID.string_to_sid(value))
 
       if sid.domain and sid.domain.length > 0
         "#{sid.domain}\\#{sid.account}"
       else
         sid.account
       end
     rescue
       nil
     end
 
     # Convert a SID pointer to a SID string, e.g. "S-1-5-32-544".
     def sid_ptr_to_string(psid)
       sid_buf = 0.chr * 256
       str_ptr = 0.chr * 4
 
       raise Puppet::Util::Windows::Error.new("Invalid SID") unless IsValidSid(psid)
 
       raise Puppet::Util::Windows::Error.new("Failed to convert binary SID") unless ConvertSidToStringSid(psid, str_ptr)
 
       begin
         strncpy(sid_buf, str_ptr.unpack('L')[0], sid_buf.size - 1)
         sid_buf[sid_buf.size - 1] = 0.chr
         return sid_buf.strip
       ensure
         LocalFree(str_ptr.unpack('L')[0])
       end
     end
 
     # Convert a SID string, e.g. "S-1-5-32-544" to a pointer (containing the
     # address of the binary SID structure). The returned value can be used in
     # Win32 APIs that expect a PSID, e.g. IsValidSid. The account for this
     # SID may or may not exist.
     def string_to_sid_ptr(string, &block)
       sid_buf = 0.chr * 80
       string_addr = [string].pack('p*').unpack('L')[0]
 
       raise Puppet::Util::Windows::Error.new("Failed to convert string SID: #{string}") unless ConvertStringSidToSid(string_addr, sid_buf)
 
       sid_ptr = sid_buf.unpack('L')[0]
       begin
-        if block_given?
-          yield sid_ptr
-        else
-          true
-        end
+        yield sid_ptr
       ensure
         LocalFree(sid_ptr)
       end
     end
+
+    # Return true if the string is a valid SID, e.g. "S-1-5-32-544", false otherwise.
+    def valid_sid?(string)
+      string_to_sid_ptr(string) { |ptr| true }
+    rescue Puppet::Util::Windows::Error => e
+      if e.code == ERROR_INVALID_SID_STRUCTURE
+        false
+      else
+        raise
+      end
+    end
   end
 end
diff --git a/spec/integration/util/windows/security_spec.rb b/spec/integration/util/windows/security_spec.rb
index c6b5c0cc4..c2bd538ea 100755
--- a/spec/integration/util/windows/security_spec.rb
+++ b/spec/integration/util/windows/security_spec.rb
@@ -1,631 +1,611 @@
 #!/usr/bin/env ruby
 require 'spec_helper'
 
 require 'puppet/util/adsi'
 
 if Puppet.features.microsoft_windows?
   class WindowsSecurityTester
     require 'puppet/util/windows/security'
     include Puppet::Util::Windows::Security
   end
 end
 
 describe "Puppet::Util::Windows::Security", :if => Puppet.features.microsoft_windows? do
   include PuppetSpec::Files
 
   before :all do
     @sids = {
       :current_user => Puppet::Util::Windows::Security.name_to_sid(Sys::Admin.get_login),
       :admin => Puppet::Util::Windows::Security.name_to_sid("Administrator"),
       :guest => Puppet::Util::Windows::Security.name_to_sid("Guest"),
       :users => Win32::Security::SID::BuiltinUsers,
       :power_users => Win32::Security::SID::PowerUsers,
     }
   end
 
   let (:sids) { @sids }
   let (:winsec) { WindowsSecurityTester.new }
 
   shared_examples_for "only child owner" do
     it "should allow child owner" do
       check_child_owner
     end
 
     it "should deny parent owner" do
       lambda { check_parent_owner }.should raise_error(Errno::EACCES)
     end
 
     it "should deny group" do
       lambda { check_group }.should raise_error(Errno::EACCES)
     end
 
     it "should deny other" do
       lambda { check_other }.should raise_error(Errno::EACCES)
     end
   end
 
   shared_examples_for "a securable object" do
     describe "on a volume that doesn't support ACLs" do
       [:owner, :group, :mode].each do |p|
         it "should return nil #{p}" do
           winsec.stubs(:supports_acl?).returns false
 
           winsec.send("get_#{p}", path).should be_nil
         end
       end
     end
 
     describe "on a volume that supports ACLs" do
       describe "for a normal user" do
         before :each do
           Puppet.features.stubs(:root?).returns(false)
         end
 
         after :each do
           winsec.set_mode(WindowsSecurityTester::S_IRWXU, parent)
           winsec.set_mode(WindowsSecurityTester::S_IRWXU, path) if File.exists?(path)
         end
 
         describe "#supports_acl?" do
           %w[c:/ c:\\ c:/windows/system32 \\\\localhost\\C$ \\\\127.0.0.1\\C$\\foo].each do |path|
             it "should accept #{path}" do
               winsec.should be_supports_acl(path)
             end
           end
 
           it "should raise an exception if it cannot get volume information" do
             expect {
               winsec.supports_acl?('foobar')
             }.to raise_error(Puppet::Error, /Failed to get volume information/)
           end
         end
 
         describe "#owner=" do
           it "should allow setting to the current user" do
             winsec.set_owner(sids[:current_user], path)
           end
 
           it "should raise an exception when setting to a different user" do
             lambda { winsec.set_owner(sids[:guest], path) }.should raise_error(Puppet::Error, /This security ID may not be assigned as the owner of this object./)
           end
         end
 
         describe "#owner" do
           it "it should not be empty" do
             winsec.get_owner(path).should_not be_empty
           end
 
           it "should raise an exception if an invalid path is provided" do
             lambda { winsec.get_owner("c:\\doesnotexist.txt") }.should raise_error(Puppet::Error, /The system cannot find the file specified./)
           end
         end
 
         describe "#group=" do
           it "should allow setting to a group the current owner is a member of" do
             winsec.set_group(sids[:users], path)
           end
 
           # Unlike unix, if the user has permission to WRITE_OWNER, which the file owner has by default,
           # then they can set the primary group to a group that the user does not belong to.
           it "should allow setting to a group the current owner is not a member of" do
             winsec.set_group(sids[:power_users], path)
           end
         end
 
         describe "#group" do
           it "should not be empty" do
             winsec.get_group(path).should_not be_empty
           end
 
           it "should raise an exception if an invalid path is provided" do
             lambda { winsec.get_group("c:\\doesnotexist.txt") }.should raise_error(Puppet::Error, /The system cannot find the file specified./)
           end
         end
 
         describe "#mode=" do
           (0000..0700).step(0100) do |mode|
             it "should enforce mode #{mode.to_s(8)}" do
               winsec.set_mode(mode, path)
 
               check_access(mode, path)
             end
           end
 
           it "should round-trip all 128 modes that do not require deny ACEs" do
             0.upto(1).each do |s|
               0.upto(7).each do |u|
                 0.upto(u).each do |g|
                   0.upto(g).each do |o|
                     # if user is superset of group, and group superset of other, then
                     # no deny ace is required, and mode can be converted to win32
                     # access mask, and back to mode without loss of information
                     # (provided the owner and group are not the same)
                     next if ((u & g) != g) or ((g & o) != o)
 
                     mode = (s << 9 | u << 6 | g << 3 | o << 0)
                     winsec.set_mode(mode, path)
                     winsec.get_mode(path).to_s(8).should == mode.to_s(8)
                   end
                 end
               end
             end
           end
 
           describe "for modes that require deny aces" do
             it "should map everyone to group and owner" do
               winsec.set_mode(0426, path)
               winsec.get_mode(path).to_s(8).should == "666"
             end
 
             it "should combine user and group modes when owner and group sids are equal" do
               winsec.set_group(winsec.get_owner(path), path)
 
               winsec.set_mode(0410, path)
               winsec.get_mode(path).to_s(8).should == "550"
             end
           end
 
           describe "for read-only objects" do
             before :each do
               winsec.add_attributes(path, WindowsSecurityTester::FILE_ATTRIBUTE_READONLY)
               (winsec.get_attributes(path) & WindowsSecurityTester::FILE_ATTRIBUTE_READONLY).should be_nonzero
             end
 
             it "should make them writable if any sid has write permission" do
               winsec.set_mode(WindowsSecurityTester::S_IWUSR, path)
               (winsec.get_attributes(path) & WindowsSecurityTester::FILE_ATTRIBUTE_READONLY).should == 0
             end
 
             it "should leave them read-only if no sid has write permission" do
               winsec.set_mode(WindowsSecurityTester::S_IRUSR | WindowsSecurityTester::S_IXGRP, path)
               (winsec.get_attributes(path) & WindowsSecurityTester::FILE_ATTRIBUTE_READONLY).should be_nonzero
             end
           end
 
           it "should raise an exception if an invalid path is provided" do
             lambda { winsec.set_mode(sids[:guest], "c:\\doesnotexist.txt") }.should raise_error(Puppet::Error, /The system cannot find the file specified./)
           end
         end
 
         describe "#mode" do
           it "should report when extra aces are encounted" do
             winsec.set_acl(path, true) do |acl|
               (544..547).each do |rid|
                 winsec.add_access_allowed_ace(acl, WindowsSecurityTester::STANDARD_RIGHTS_ALL, "S-1-5-32-#{rid}")
               end
             end
             mode = winsec.get_mode(path)
             (mode & WindowsSecurityTester::S_IEXTRA).should_not == 0
           end
 
           it "should warn if a deny ace is encountered" do
             winsec.set_acl(path) do |acl|
               winsec.add_access_denied_ace(acl, WindowsSecurityTester::FILE_GENERIC_WRITE, sids[:guest])
               winsec.add_access_allowed_ace(acl, WindowsSecurityTester::STANDARD_RIGHTS_ALL | WindowsSecurityTester::SPECIFIC_RIGHTS_ALL, sids[:current_user])
             end
 
             Puppet.expects(:warning).with("Unsupported access control entry type: 0x1")
 
             winsec.get_mode(path)
           end
 
           it "should skip inherit-only ace" do
             winsec.set_acl(path) do |acl|
               winsec.add_access_allowed_ace(acl, WindowsSecurityTester::STANDARD_RIGHTS_ALL | WindowsSecurityTester::SPECIFIC_RIGHTS_ALL, sids[:current_user])
               winsec.add_access_allowed_ace(acl, WindowsSecurityTester::FILE_GENERIC_READ, Win32::Security::SID::Everyone, WindowsSecurityTester::INHERIT_ONLY_ACE | WindowsSecurityTester::OBJECT_INHERIT_ACE)
             end
 
             (winsec.get_mode(path) & WindowsSecurityTester::S_IRWXO).should == 0
           end
 
           it "should raise an exception if an invalid path is provided" do
             lambda { winsec.get_mode("c:\\doesnotexist.txt") }.should raise_error(Puppet::Error, /The system cannot find the file specified./)
           end
         end
 
         describe "inherited access control entries" do
           it "should be absent when the access control list is protected" do
             winsec.set_mode(WindowsSecurityTester::S_IRWXU, path)
             (winsec.get_mode(path) & WindowsSecurityTester::S_IEXTRA).should == 0
           end
 
           it "should be present when the access control list is unprotected" do
             # add a bunch of aces to the parent with permission to add children
             allow = WindowsSecurityTester::STANDARD_RIGHTS_ALL | WindowsSecurityTester::SPECIFIC_RIGHTS_ALL
             inherit = WindowsSecurityTester::OBJECT_INHERIT_ACE | WindowsSecurityTester::CONTAINER_INHERIT_ACE
 
             winsec.set_acl(parent, true) do |acl|
               winsec.add_access_allowed_ace(acl, allow, "S-1-1-0", inherit) # everyone
 
               (544..547).each do |rid|
                 winsec.add_access_allowed_ace(acl, WindowsSecurityTester::STANDARD_RIGHTS_ALL, "S-1-5-32-#{rid}", inherit)
               end
             end
 
             # unprotect child, it should inherit from parent
             winsec.set_mode(WindowsSecurityTester::S_IRWXU, path, false)
             (winsec.get_mode(path) & WindowsSecurityTester::S_IEXTRA).should == WindowsSecurityTester::S_IEXTRA
           end
         end
       end
 
       describe "for an administrator", :if => Puppet.features.root? do
         before :each do
           winsec.set_mode(WindowsSecurityTester::S_IRWXU | WindowsSecurityTester::S_IRWXG, path)
           winsec.set_group(sids[:guest], path)
           winsec.set_owner(sids[:guest], path)
           lambda { File.open(path, 'r') }.should raise_error(Errno::EACCES)
         end
 
         after :each do
           if File.exists?(path)
             winsec.set_owner(sids[:current_user], path)
             winsec.set_mode(WindowsSecurityTester::S_IRWXU, path)
           end
         end
 
         describe "#owner=" do
           it "should accept a user sid" do
             winsec.set_owner(sids[:admin], path)
             winsec.get_owner(path).should == sids[:admin]
           end
 
           it "should accept a group sid" do
             winsec.set_owner(sids[:power_users], path)
             winsec.get_owner(path).should == sids[:power_users]
           end
 
           it "should raise an exception if an invalid sid is provided" do
             lambda { winsec.set_owner("foobar", path) }.should raise_error(Puppet::Error, /Failed to convert string SID/)
           end
 
           it "should raise an exception if an invalid path is provided" do
             lambda { winsec.set_owner(sids[:guest], "c:\\doesnotexist.txt") }.should raise_error(Puppet::Error, /The system cannot find the file specified./)
           end
         end
 
         describe "#group=" do
           it "should accept a group sid" do
             winsec.set_group(sids[:power_users], path)
             winsec.get_group(path).should == sids[:power_users]
           end
 
           it "should accept a user sid" do
             winsec.set_group(sids[:admin], path)
             winsec.get_group(path).should == sids[:admin]
           end
 
           it "should allow owner and group to be the same sid" do
             winsec.set_mode(0610, path)
             winsec.set_owner(sids[:power_users], path)
             winsec.set_group(sids[:power_users], path)
 
             winsec.get_owner(path).should == sids[:power_users]
             winsec.get_group(path).should == sids[:power_users]
             # note group execute permission added to user ace, and then group rwx value
             # reflected to match
             winsec.get_mode(path).to_s(8).should == "770"
           end
 
           it "should raise an exception if an invalid sid is provided" do
             lambda { winsec.set_group("foobar", path) }.should raise_error(Puppet::Error, /Failed to convert string SID/)
           end
 
           it "should raise an exception if an invalid path is provided" do
             lambda { winsec.set_group(sids[:guest], "c:\\doesnotexist.txt") }.should raise_error(Puppet::Error, /The system cannot find the file specified./)
           end
         end
 
         describe "when the sid is NULL" do
           it "should retrieve an empty owner sid"
           it "should retrieve an empty group sid"
         end
 
         describe "when the sid refers to a deleted trustee" do
           it "should retrieve the user sid" do
             sid = nil
             user = Puppet::Util::ADSI::User.create("delete_me_user")
             user.commit
             begin
               sid = Sys::Admin::get_user(user.name).sid
               winsec.set_owner(sid, path)
               winsec.set_mode(WindowsSecurityTester::S_IRWXU, path)
             ensure
               Puppet::Util::ADSI::User.delete(user.name)
             end
 
             winsec.get_owner(path).should == sid
             winsec.get_mode(path).should == WindowsSecurityTester::S_IRWXU
           end
 
           it "should retrieve the group sid" do
             sid = nil
             group = Puppet::Util::ADSI::Group.create("delete_me_group")
             group.commit
             begin
               sid = Sys::Admin::get_group(group.name).sid
               winsec.set_group(sid, path)
               winsec.set_mode(WindowsSecurityTester::S_IRWXG, path)
             ensure
               Puppet::Util::ADSI::Group.delete(group.name)
             end
             winsec.get_group(path).should == sid
             winsec.get_mode(path).should == WindowsSecurityTester::S_IRWXG
           end
         end
 
         describe "#mode" do
           it "should deny all access when the DACL is empty" do
             winsec.set_acl(path, true) { |acl| }
 
             winsec.get_mode(path).should == 0
           end
 
           # REMIND: ruby crashes when trying to set a NULL DACL
           # it "should allow all when it is nil" do
           #   winsec.set_owner(sids[:current_user], path)
           #   winsec.open_file(path, WindowsSecurityTester::READ_CONTROL | WindowsSecurityTester::WRITE_DAC) do |handle|
           #     winsec.set_security_info(handle, WindowsSecurityTester::DACL_SECURITY_INFORMATION | WindowsSecurityTester::PROTECTED_DACL_SECURITY_INFORMATION, nil)
           #   end
           #   winsec.get_mode(path).to_s(8).should == "777"
           # end
         end
 
-        describe "#string_to_sid_ptr" do
-          it "should raise an error if an invalid SID is specified" do
-            expect do
-              winsec.string_to_sid_ptr('foobar')
-            end.to raise_error(Puppet::Util::Windows::Error) { |error| error.code.should == 1337 }
-          end
-
-          it "should yield if a block is given" do
-            yielded = nil
-            winsec.string_to_sid_ptr('S-1-1-0') do |sid|
-              yielded = sid
-            end
-            yielded.should_not be_nil
-          end
-
-          it "should allow no block to be specified" do
-            winsec.string_to_sid_ptr('S-1-1-0').should be_true
-          end
-        end
-
         describe "when the parent directory" do
           before :each do
             winsec.set_owner(sids[:current_user], parent)
             winsec.set_owner(sids[:current_user], path)
             winsec.set_mode(0777, path, false)
           end
 
           def check_child_owner
             winsec.set_group(sids[:guest], parent)
             winsec.set_owner(sids[:guest], parent)
 
             check_delete(path)
           end
 
         def check_parent_owner
           winsec.set_group(sids[:guest], path)
           winsec.set_owner(sids[:guest], path)
 
           check_delete(path)
         end
 
         def check_group
           winsec.set_group(sids[:current_user], path)
           winsec.set_owner(sids[:guest], path)
 
           winsec.set_owner(sids[:guest], parent)
 
           check_delete(path)
         end
 
         def check_other
           winsec.set_group(sids[:guest], path)
           winsec.set_owner(sids[:guest], path)
 
           winsec.set_owner(sids[:guest], parent)
 
           check_delete(path)
         end
 
         describe "is writable and executable" do
           describe "and sticky bit is set" do
             before :each do
               winsec.set_mode(01777, parent)
             end
 
             it "should allow child owner" do
               check_child_owner
             end
 
             it "should allow parent owner" do
               check_parent_owner
             end
 
             it "should deny group" do
               lambda { check_group }.should raise_error(Errno::EACCES)
             end
 
             it "should deny other" do
               lambda { check_other }.should raise_error(Errno::EACCES)
             end
           end
 
           describe "and sticky bit is not set" do
             before :each do
               winsec.set_mode(0777, parent)
             end
 
             it "should allow child owner" do
               check_child_owner
             end
 
             it "should allow parent owner" do
               check_parent_owner
             end
 
             it "should allow group" do
               check_group
             end
 
             it "should allow other" do
               check_other
             end
           end
         end
 
         describe "is not writable" do
           before :each do
             winsec.set_mode(0555, parent)
           end
 
           it_behaves_like "only child owner"
         end
 
         describe "is not executable" do
           before :each do
             winsec.set_mode(0666, parent)
           end
 
           it_behaves_like "only child owner"
         end
       end
     end
   end
   end
 
   describe "file" do
     let (:parent) do
       tmpdir('win_sec_test_file')
     end
 
     let (:path) do
       path = File.join(parent, 'childfile')
       File.new(path, 'w').close
       path
     end
 
     it_behaves_like "a securable object" do
       def check_access(mode, path)
         if (mode & WindowsSecurityTester::S_IRUSR).nonzero?
           check_read(path)
         else
           lambda { check_read(path) }.should raise_error(Errno::EACCES)
         end
 
         if (mode & WindowsSecurityTester::S_IWUSR).nonzero?
           check_write(path)
         else
           lambda { check_write(path) }.should raise_error(Errno::EACCES)
         end
 
         if (mode & WindowsSecurityTester::S_IXUSR).nonzero?
           lambda { check_execute(path) }.should raise_error(Errno::ENOEXEC)
         else
           lambda { check_execute(path) }.should raise_error(Errno::EACCES)
         end
       end
 
       def check_read(path)
         File.open(path, 'r').close
       end
 
       def check_write(path)
         File.open(path, 'w').close
       end
 
       def check_execute(path)
         Kernel.exec(path)
       end
 
       def check_delete(path)
         File.delete(path)
       end
     end
 
     describe "locked files" do
       let (:explorer) { File.join(Dir::WINDOWS, "explorer.exe") }
 
       it "should get the owner" do
         winsec.get_owner(explorer).should match /^S-1-5-/
       end
 
       it "should get the group" do
         winsec.get_group(explorer).should match /^S-1-5-/
       end
 
       it "should get the mode" do
         winsec.get_mode(explorer).should == (WindowsSecurityTester::S_IRWXU | WindowsSecurityTester::S_IRWXG | WindowsSecurityTester::S_IEXTRA)
       end
     end
   end
 
   describe "directory" do
     let (:parent) do
       tmpdir('win_sec_test_dir')
     end
 
     let (:path) do
       path = File.join(parent, 'childdir')
       Dir.mkdir(path)
       path
     end
 
     it_behaves_like "a securable object" do
       def check_access(mode, path)
         if (mode & WindowsSecurityTester::S_IRUSR).nonzero?
           check_read(path)
         else
           lambda { check_read(path) }.should raise_error(Errno::EACCES)
         end
 
         if (mode & WindowsSecurityTester::S_IWUSR).nonzero?
           check_write(path)
         else
           lambda { check_write(path) }.should raise_error(Errno::EACCES)
         end
 
         if (mode & WindowsSecurityTester::S_IXUSR).nonzero?
           check_execute(path)
         else
           lambda { check_execute(path) }.should raise_error(Errno::EACCES)
         end
       end
 
       def check_read(path)
         Dir.entries(path)
       end
 
       def check_write(path)
         Dir.mkdir(File.join(path, "subdir"))
       end
 
       def check_execute(path)
         Dir.chdir(path) {}
       end
 
       def check_delete(path)
         Dir.rmdir(path)
       end
     end
 
     describe "inheritable aces" do
       it "should be applied to child objects" do
         mode640 = WindowsSecurityTester::S_IRUSR | WindowsSecurityTester::S_IWUSR | WindowsSecurityTester::S_IRGRP
         winsec.set_mode(mode640, path)
 
         newfile = File.join(path, "newfile.txt")
         File.new(newfile, "w").close
 
         newdir = File.join(path, "newdir")
         Dir.mkdir(newdir)
 
         [newfile, newdir].each do |p|
           winsec.get_mode(p).to_s(8).should == mode640.to_s(8)
         end
       end
     end
   end
 end
diff --git a/spec/unit/provider/file/windows_spec.rb b/spec/unit/provider/file/windows_spec.rb
index aec62cb61..201baf531 100755
--- a/spec/unit/provider/file/windows_spec.rb
+++ b/spec/unit/provider/file/windows_spec.rb
@@ -1,153 +1,154 @@
 #!/usr/bin/env rspec
 
 require 'spec_helper'
 if Puppet.features.microsoft_windows?
   require 'puppet/util/windows'
   class WindowsSecurity
     extend Puppet::Util::Windows::Security
   end
 end
 
 describe Puppet::Type.type(:file).provider(:windows), :if => Puppet.features.microsoft_windows? do
   include PuppetSpec::Files
 
   let(:path) { tmpfile('windows_file_spec') }
   let(:resource) { Puppet::Type.type(:file).new :path => path, :mode => 0777, :provider => described_class.name }
   let(:provider) { resource.provider }
   let(:sid)      { 'S-1-1-50' }
   let(:account)  { 'quinn' }
 
   describe "#mode" do
     it "should return a string with the higher-order bits stripped away" do
       FileUtils.touch(path)
       WindowsSecurity.set_mode(0644, path)
 
       provider.mode.should == '644'
     end
 
     it "should return absent if the file doesn't exist" do
       provider.mode.should == :absent
     end
   end
 
   describe "#mode=" do
     it "should chmod the file to the specified value" do
       FileUtils.touch(path)
       WindowsSecurity.set_mode(0644, path)
 
       provider.mode = '0755'
 
       provider.mode.should == '755'
     end
 
     it "should pass along any errors encountered" do
       expect do
         provider.mode = '644'
       end.to raise_error(Puppet::Error, /failed to set mode/)
     end
   end
 
   describe "#id2name" do
     it "should return the name of the user identified by the sid" do
-      Puppet::Util::Windows::Security.expects(:string_to_sid_ptr).with(sid).returns(true)
+      Puppet::Util::Windows::Security.expects(:valid_sid?).with(sid).returns(true)
       Puppet::Util::Windows::Security.expects(:sid_to_name).with(sid).returns(account)
 
       provider.id2name(sid).should == account
     end
 
     it "should return the argument if it's already a name" do
-      Puppet::Util::Windows::Security.expects(:string_to_sid_ptr).with(account).returns(false)
+      Puppet::Util::Windows::Security.expects(:valid_sid?).with(account).returns(false)
+      Puppet::Util::Windows::Security.expects(:sid_to_name).never
 
       provider.id2name(account).should == account
     end
 
     it "should return nil if the user doesn't exist" do
-      Puppet::Util::Windows::Security.expects(:string_to_sid_ptr).with(sid).returns(true)
+      Puppet::Util::Windows::Security.expects(:valid_sid?).with(sid).returns(true)
       Puppet::Util::Windows::Security.expects(:sid_to_name).with(sid).returns(nil)
 
       provider.id2name(sid).should == nil
     end
   end
 
   describe "#name2id" do
     it "should delegate to name_to_sid" do
       Puppet::Util::Windows::Security.expects(:name_to_sid).with(account).returns(sid)
 
       provider.name2id(account).should == sid
     end
   end
 
   describe "#owner" do
     it "should return the sid of the owner if the file does exist" do
       FileUtils.touch(resource[:path])
       provider.stubs(:get_owner).with(resource[:path]).returns(sid)
 
       provider.owner.should == sid
     end
 
     it "should return absent if the file doesn't exist" do
       provider.owner.should == :absent
     end
   end
 
   describe "#owner=" do
     it "should set the owner to the specified value" do
       provider.expects(:set_owner).with(sid, resource[:path])
       provider.owner = sid
     end
 
     it "should propagate any errors encountered when setting the owner" do
       provider.stubs(:set_owner).raises(ArgumentError)
 
       expect {
         provider.owner = sid
       }.to raise_error(Puppet::Error, /Failed to set owner/)
     end
   end
 
   describe "#group" do
     it "should return the sid of the group if the file does exist" do
       FileUtils.touch(resource[:path])
       provider.stubs(:get_group).with(resource[:path]).returns(sid)
 
       provider.group.should == sid
     end
 
     it "should return absent if the file doesn't exist" do
       provider.group.should == :absent
     end
   end
 
   describe "#group=" do
     it "should set the group to the specified value" do
       provider.expects(:set_group).with(sid, resource[:path])
       provider.group = sid
     end
 
     it "should propagate any errors encountered when setting the group" do
       provider.stubs(:set_group).raises(ArgumentError)
 
       expect {
         provider.group = sid
       }.to raise_error(Puppet::Error, /Failed to set group/)
     end
   end
 
   describe "when validating" do
     {:owner => 'foo', :group => 'foo', :mode => 0777}.each do |k,v|
       it "should fail if the filesystem doesn't support ACLs and we're managing #{k}" do
         described_class.any_instance.stubs(:supports_acl?).returns false
 
         expect {
           Puppet::Type.type(:file).new :path => path, k => v
         }.to raise_error(Puppet::Error, /Can only manage owner, group, and mode on filesystems that support Windows ACLs, such as NTFS/)
       end
     end
 
     it "should not fail if the filesystem doesn't support ACLs and we're not managing permissions" do
       described_class.any_instance.stubs(:supports_acl?).returns false
 
       Puppet::Type.type(:file).new :path => path
     end
   end
 end
diff --git a/spec/unit/util/windows/sid_spec.rb b/spec/unit/util/windows/sid_spec.rb
index 232adc64c..d53e93ae6 100755
--- a/spec/unit/util/windows/sid_spec.rb
+++ b/spec/unit/util/windows/sid_spec.rb
@@ -1,84 +1,100 @@
 #!/usr/bin/env ruby
 require 'spec_helper'
 
 describe "Puppet::Util::Windows::SID", :if => Puppet.features.microsoft_windows? do
   if Puppet.features.microsoft_windows?
+    require 'puppet/util/windows'
     class SIDTester
       include Puppet::Util::Windows::SID
     end
   end
 
   let(:subject)      { SIDTester.new }
   let(:sid)          { Win32::Security::SID::LocalSystem }
   let(:invalid_sid)  { 'bogus' }
   let(:unknown_sid)  { 'S-0-0-0' }
   let(:unknown_name) { 'chewbacca' }
 
   context "#name_to_sid" do
     it "should return nil if the account does not exist" do
       subject.name_to_sid(unknown_name).should be_nil
     end
 
     it "should accept unqualified account name" do
       subject.name_to_sid('SYSTEM').should == sid
     end
 
     it "should be case-insensitive" do
       subject.name_to_sid('SYSTEM').should == subject.name_to_sid('system')
     end
 
     it "should accept domain qualified account names" do
       subject.name_to_sid('NT AUTHORITY\SYSTEM').should == sid
     end
 
     it "should be the identity function for any sid" do
       subject.name_to_sid(sid).should == sid
     end
   end
 
   context "#sid_to_name" do
     it "should return nil if given a sid for an account that doesn't exist" do
       subject.sid_to_name(unknown_sid).should be_nil
     end
 
     it "should accept a sid" do
       subject.sid_to_name(sid).should == "NT AUTHORITY\\SYSTEM"
     end
   end
 
   context "#sid_ptr_to_string" do
     it "should raise if given an invalid sid" do
       expect {
         subject.sid_ptr_to_string(nil)
       }.to raise_error(Puppet::Error, /Invalid SID/)
     end
 
     it "should yield a valid sid pointer" do
       string = nil
       subject.string_to_sid_ptr(sid) do |ptr|
         string = subject.sid_ptr_to_string(ptr)
       end
       string.should == sid
     end
   end
 
   context "#string_to_sid_ptr" do
-    it "should accept well known SID" do
-      subject.string_to_sid_ptr(sid).should be_true
-    end
-
     it "should yield sid_ptr" do
       ptr = nil
       subject.string_to_sid_ptr(sid) do |p|
         ptr = p
       end
       ptr.should_not be_nil
     end
 
     it "should raise on an invalid sid" do
       expect {
         subject.string_to_sid_ptr(invalid_sid)
       }.to raise_error(Puppet::Error, /Failed to convert string SID/)
     end
   end
+
+  context "#valid_sid?" do
+    it "should return true for a valid SID" do
+      subject.valid_sid?(sid).should be_true
+    end
+
+    it "should return false for an invalid SID" do
+      subject.valid_sid?(invalid_sid).should be_false
+    end
+
+    it "should raise if the conversion fails" do
+      subject.expects(:string_to_sid_ptr).with(sid).
+        raises(Puppet::Util::Windows::Error.new("Failed to convert string SID: #{sid}", Windows::Error::ERROR_ACCESS_DENIED))
+
+      expect {
+        subject.string_to_sid_ptr(sid) {|ptr| }
+      }.to raise_error(Puppet::Util::Windows::Error, /Failed to convert string SID: #{sid}/)
+    end
+  end
 end