fix: skip setting back an owner that did not change

Clear-NTFSAccess -DisableInheritance and Set-NTFSSecurityDescriptor took ownership of an item that the user owned already, left a DACL without the right to set an owner, and then reported a RestoreOwnerError for setting the same owner back. Skip the restore when the previous owner is the current user. The guards fail without the fix in all four configurations and also cover the owner that cannot be set back without the Restore privilege, the missing path of Get-ChildItem2, and the descriptor write that retries as owner.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: AI Assistant <ai@example.com>

CHANGELOG.md

@@ -113,6 +113,11 @@ The format is based on
 - Compare boxed privilege output values by their privilege and attributes;
   the object overload rejected privilege values and recursively compared
   an attributes enum instead
+- Fix `Clear-NTFSAccess -DisableInheritance` and `Set-NTFSSecurityDescriptor`,
+  which reported a `RestoreOwnerError` for an owner that had not changed:
+  after they took ownership of an item that the user owned already and left a
+  DACL without the right to set an owner, they failed to set the same owner
+  back
 
 - Fix `Get-Help`, which showed only the syntax: ship the help file
   `en-US\NTFSSecurity.dll-Help.xml` generated from the cmdlet documentation,

NTFSSecurity/BaseCmdlets.cs

@@ -192,6 +192,8 @@ namespace NTFSSecurity
         /// <summary>
         /// Takes ownership of the item, runs the action, and restores the previous owner on every exit path.
         /// A failure to restore the owner is written as a RestoreOwnerError and doesn't hide an error of the action.
+        /// An owner that the action did not change, because the current user owned the item already, isn't set again:
+        /// an action such as clearing the DACL can leave nobody the right to do so.
         /// </summary>
         /// <param name="item">The file or folder to take ownership of.</param>
         /// <param name="path">The path the user specified, used as the error target.</param>
@@ -199,8 +201,9 @@ namespace NTFSSecurity
         protected void InvokeAsOwner(Alphaleonis.Win32.Filesystem.FileSystemInfo item, string path, Action action)
         {
             var previousOwner = FileSystemOwner.GetOwner(item).Owner;
+            IdentityReference2 currentUser = System.Security.Principal.WindowsIdentity.GetCurrent().User;
 
-            FileSystemOwner.SetOwner(item, System.Security.Principal.WindowsIdentity.GetCurrent().User);
+            FileSystemOwner.SetOwner(item, currentUser);
 
             try
             {
@@ -208,13 +211,16 @@ namespace NTFSSecurity
             }
             finally
             {
-                try
+                if (previousOwner != currentUser)
                 {
-                    FileSystemOwner.SetOwner(item, previousOwner);
-                }
-                catch (Exception ex)
-                {
-                    WriteError(new ErrorRecord(ex, "RestoreOwnerError", ErrorCategory.WriteError, path));
+                    try
+                    {
+                        FileSystemOwner.SetOwner(item, previousOwner);
+                    }
+                    catch (Exception ex)
+                    {
+                        WriteError(new ErrorRecord(ex, "RestoreOwnerError", ErrorCategory.WriteError, path));
+                    }
                 }
             }
         }

NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs

@@ -88,13 +88,16 @@ namespace NTFSSecurity
         }
 
         // Like InvokeAsOwner, takes ownership for the write and sets the previous owner back on every exit path, but not
-        // after a successful write of a descriptor that sets the owner itself, which would undo that owner.
+        // after a successful write of a descriptor that sets the owner itself, which would undo that owner, and not when the
+        // current user owned the item already, so that nothing changed and a descriptor that leaves nobody the right to set
+        // an owner can't make it fail.
         private void WriteChangesAsOwner(FileSystemSecurity2 sd)
         {
             var setsOwner = (sd.ChangedSections & AccessControlSections.Owner) == AccessControlSections.Owner;
             var previousOwner = FileSystemOwner.GetOwner(sd.Item).Owner;
+            IdentityReference2 currentUser = System.Security.Principal.WindowsIdentity.GetCurrent().User;
 
-            FileSystemOwner.SetOwner(sd.Item, System.Security.Principal.WindowsIdentity.GetCurrent().User);
+            FileSystemOwner.SetOwner(sd.Item, currentUser);
 
             var written = false;
             try
@@ -104,7 +107,7 @@ namespace NTFSSecurity
             }
             finally
             {
-                if (!(written && setsOwner))
+                if (!(written && setsOwner) && previousOwner != currentUser)
                 {
                     try
                     {

Tests/PathErrors.Tests.ps1

@@ -35,6 +35,7 @@ BeforeDiscovery {
         @{ Command = 'Set-NTFSOwner'; Parameters = @{ Account = $currentUser }; ErrorId = 'ReadFileError'; Output = $false }
         @{ Command = 'Get-NTFSSecurityDescriptor'; Parameters = @{}; ErrorId = 'ReadFileError'; Output = $true }
         @{ Command = 'Get-Item2'; Parameters = @{}; ErrorId = 'FileNotFound'; Output = $true }
+        @{ Command = 'Get-ChildItem2'; Parameters = @{}; ErrorId = 'FileNotFound'; Output = $true }
         @{ Command = 'Get-FileHash2'; Parameters = @{}; ErrorId = 'ReadFileError'; Output = $true }
         @{ Command = 'Get-NTFSHardLink'; Parameters = @{}; ErrorId = 'FileNotFound'; Output = $true }
     )
@@ -259,6 +260,43 @@ Describe 'An item whose owner may not change its permissions' {
         @($acl.GetAccessRules($true, $false, $sidType)) | Should -BeNullOrEmpty
     }
 
+    # With the DACL cleared and protected, nobody keeps the right to set an owner, so setting the previous owner back
+    # fails. The user owned the item already, so there is no owner to set back.
+    It 'Clear-NTFSAccess -DisableInheritance should take ownership, clear and protect the DACL, and not set an unchanged owner back' {
+        $user = [Security.Principal.WindowsIdentity]::GetCurrent().User.Value
+        Set-TestOwner -Sandbox $sandbox -Path $file -Sid $user
+        Add-NTFSAccess -Path $file -Account 'S-1-1-0' -AccessRights ReadData
+        Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' }
+
+        Clear-NTFSAccess -Path $file -DisableInheritance -ErrorVariable changeErrors -ErrorAction SilentlyContinue
+
+        $changeErrors | Should -BeNullOrEmpty
+        $acl = Get-TestAcl -Path $file
+        $acl.GetOwner($sidType).Value | Should -Be $user
+        $acl.AreAccessRulesProtected | Should -BeTrue
+        @($acl.GetAccessRules($true, $true, $sidType)) | Should -BeNullOrEmpty
+    }
+
+    # Windows lets the owner of an item set another owner only with the Restore privilege, which the tests turn off, or
+    # with the right in the DACL, which the cleared DACL no longer holds. The cmdlet reports the owner it cannot set back.
+    It 'Clear-NTFSAccess -DisableInheritance should report RestoreOwnerError for a previous owner that it cannot set back' -Skip:(-not $holdsRestorePrivilege) {
+        $user = [Security.Principal.WindowsIdentity]::GetCurrent().User.Value
+        $owner | Should -Not -Be $user
+        Add-NTFSAccess -Path $file -Account 'S-1-1-0' -AccessRights ReadData
+        Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' }
+
+        Clear-NTFSAccess -Path $file -DisableInheritance -ErrorVariable changeErrors -ErrorAction SilentlyContinue
+
+        $changeErrors | Should -HaveCount 1
+        $changeErrors[0].FullyQualifiedErrorId | Should -BeLike 'RestoreOwnerError,*'
+        $changeErrors[0].CategoryInfo.Category | Should -Be 'WriteError'
+        $changeErrors[0].TargetObject | Should -Be $file
+        $acl = Get-TestAcl -Path $file
+        $acl.GetOwner($sidType).Value | Should -Be $user
+        $acl.AreAccessRulesProtected | Should -BeTrue
+        @($acl.GetAccessRules($true, $true, $sidType)) | Should -BeNullOrEmpty
+    }
+
     It 'Disable-NTFSAccessInheritance should take ownership, protect the DACL, and set the owner back' {
         Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' }
 

Tests/SecurityDescriptor.Tests.ps1

@@ -225,6 +225,44 @@ Describe 'Set-NTFSSecurityDescriptor' {
             $result[0].FullName | Should -Be $file
             $result[0].SecurityDescriptor.GetOwner($sidType).Value | Should -Be 'S-1-5-32-544'
         }
+
+        # With a cleared, protected DACL, nobody keeps the right to set an owner, so setting the previous owner back would
+        # fail. The user owned the item already, so there is no owner to set back.
+        It 'Should not report an owner that did not change when the write that took ownership leaves an empty DACL' {
+            $file = New-TestSandboxItem -Sandbox $sandbox -Name 'RetryUnchangedOwner'
+            Set-TestOwner -Sandbox $sandbox -Path $file -Sid $currentUser
+            Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ 'S-1-3-4' = 'ChangePermissions' }
+            $sd = Get-NTFSSecurityDescriptor -Path $file
+            Clear-NTFSAccess -SecurityDescriptor $sd -DisableInheritance -ErrorAction Stop
+
+            Set-NTFSSecurityDescriptor -SecurityDescriptor $sd -ErrorVariable setErrors -ErrorAction SilentlyContinue
+
+            $setErrors | Should -BeNullOrEmpty
+            $acl = Get-Acl -LiteralPath $file
+            $acl.GetOwner($sidType).Value | Should -Be $currentUser
+            $acl.AreAccessRulesProtected | Should -BeTrue
+            @($acl.GetAccessRules($true, $true, $sidType)) | Should -BeNullOrEmpty
+        }
+
+        # Without the Restore privilege, the user can't set an owner such as TrustedInstaller back. The cmdlet reports it
+        # after it wrote the descriptor.
+        It 'Should report RestoreOwnerError for a previous owner that it cannot set back after the write' -Skip:(-not $canAssignAnyOwner) {
+            $file = New-TestSandboxItem -Sandbox $sandbox -Name 'RetryRestoreDenied'
+            Add-TestDenyRule -Sandbox $sandbox -Path $file -Rights @{ $currentUser = 'ChangePermissions' }
+            Set-TestOwner -Sandbox $sandbox -Path $file -Sid $trustedInstaller
+            Get-RestorePrivilegeState | Should -Be 'Disabled'
+            $sd = Get-NTFSSecurityDescriptor -Path $file
+            Add-NTFSAccess -SecurityDescriptor $sd -Account 'Everyone' -AccessRights ReadData
+
+            Set-NTFSSecurityDescriptor -SecurityDescriptor $sd -ErrorVariable setErrors -ErrorAction SilentlyContinue
+
+            $setErrors | Should -HaveCount 1
+            $setErrors[0].FullyQualifiedErrorId | Should -BeLike 'RestoreOwnerError,*'
+            $setErrors[0].CategoryInfo.Category | Should -Be 'WriteError'
+            $setErrors[0].TargetObject.FullName | Should -Be $file
+            @(Get-EveryoneRule -Path $file) | Should -HaveCount 1
+            (Get-Acl -LiteralPath $file).GetOwner($sidType).Value | Should -Be $currentUser
+        }
     }
 
     Context 'A descriptor that cannot be written' {