From 09476833720e338ad384dcfd3c2a0478cbfc32aa Mon Sep 17 00:00:00 2001 From: Raimund Andree Date: Tue, 6 Oct 2026 12:36:44 +0000 Subject: [PATCH] fix: address the review of 5.0.0-rc3 - Set-NTFSSecurityDescriptor -Verbose names the sections that it writes, or says that it writes nothing for an unchanged descriptor; its page says "since it was read or last written" (review F-02). - The pages of Enable-NTFSAccessInheritance, Disable-NTFSAccessInheritance, and Set-NTFSInheritance get the #34 note, like the other fixed cmdlets (review F-06). - Set-TestOwner throws its own error when icacls fails, also when the caller uses -ErrorAction Stop in Windows PowerShell (review F-07). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant --- CHANGELOG.md | 7 +++--- Docs/Cmdlets/Disable-NTFSAccessInheritance.md | 2 ++ Docs/Cmdlets/Enable-NTFSAccessInheritance.md | 2 ++ Docs/Cmdlets/Set-NTFSInheritance.md | 2 ++ Docs/Cmdlets/Set-NTFSSecurityDescriptor.md | 2 +- .../SetSecurityDescriptor.cs | 11 +++++++++ NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml | 5 +++- Tests/SecurityDescriptor.Tests.ps1 | 23 +++++++++++++++++++ Tests/TestHelpers.Tests.ps1 | 9 ++++++++ Tests/TestHelpers.psm1 | 3 +++ 10 files changed, 61 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a0d1e7..dc58999 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,9 +68,10 @@ The format is based on - Write only the sections of a security descriptor that changed since it was read in `Set-NTFSSecurityDescriptor`, such as the DACL after `Add-NTFSAccess -SecurityDescriptor`; a descriptor without changes writes - nothing. The cmdlet wrote every section that `Get-NTFSSecurityDescriptor` - had read, also an unchanged owner, which failed with error 1307 where the - account may not assign that owner + nothing, and `-Verbose` names the sections that the cmdlet writes. The + cmdlet wrote every section that `Get-NTFSSecurityDescriptor` had read, + also an unchanged owner, which failed with error 1307 where the account + may not assign that owner ([#34](https://github.com/raandree/NTFSSecurity/issues/34)) ### Deprecated diff --git a/Docs/Cmdlets/Disable-NTFSAccessInheritance.md b/Docs/Cmdlets/Disable-NTFSAccessInheritance.md index 0a8ca64..7a7b694 100644 --- a/Docs/Cmdlets/Disable-NTFSAccessInheritance.md +++ b/Docs/Cmdlets/Disable-NTFSAccessInheritance.md @@ -172,6 +172,8 @@ Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. +In the `Path` parameter set, the cmdlet writes only the DACL of the item and leaves its owner, its group, and its SACL as they are. Before 5.0.0, it could also write the owner back, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. + ## RELATED LINKS [Enable-NTFSAccessInheritance](Enable-NTFSAccessInheritance.md) diff --git a/Docs/Cmdlets/Enable-NTFSAccessInheritance.md b/Docs/Cmdlets/Enable-NTFSAccessInheritance.md index 6f38c39..3448101 100644 --- a/Docs/Cmdlets/Enable-NTFSAccessInheritance.md +++ b/Docs/Cmdlets/Enable-NTFSAccessInheritance.md @@ -171,6 +171,8 @@ Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. +In the `Path` parameter set, the cmdlet writes only the DACL of the item and leaves its owner, its group, and its SACL as they are. Before 5.0.0, it could also write the owner back, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. + ## RELATED LINKS [Disable-NTFSAccessInheritance](Disable-NTFSAccessInheritance.md) diff --git a/Docs/Cmdlets/Set-NTFSInheritance.md b/Docs/Cmdlets/Set-NTFSInheritance.md index e3667c6..cb4bfa1 100644 --- a/Docs/Cmdlets/Set-NTFSInheritance.md +++ b/Docs/Cmdlets/Set-NTFSInheritance.md @@ -198,6 +198,8 @@ Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. +In the `Path` parameter set, the cmdlet writes only the section that it changes, the DACL or the SACL, and leaves the owner and the group of the item as they are. Before 5.0.0, a change of the access inheritance could also write the owner back, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. + ## RELATED LINKS [Get-NTFSInheritance](Get-NTFSInheritance.md) diff --git a/Docs/Cmdlets/Set-NTFSSecurityDescriptor.md b/Docs/Cmdlets/Set-NTFSSecurityDescriptor.md index 14fa9f3..f7c37e0 100644 --- a/Docs/Cmdlets/Set-NTFSSecurityDescriptor.md +++ b/Docs/Cmdlets/Set-NTFSSecurityDescriptor.md @@ -21,7 +21,7 @@ Set-NTFSSecurityDescriptor [-SecurityDescriptor] [-PassT The `Set-NTFSSecurityDescriptor` cmdlet writes a `Security2.FileSystemSecurity2` object to the file system. It is the final step of the security descriptor workflow: `Get-NTFSSecurityDescriptor` reads a descriptor into memory, cmdlets such as `Add-NTFSAccess`, `Remove-NTFSAccess`, `Set-NTFSOwner`, and `Disable-NTFSAccessInheritance` change that copy through their `-SecurityDescriptor` parameter, and this cmdlet applies all of those changes in a single write. -Each descriptor remembers the item it was read from, and the cmdlet writes it back to exactly that item. There is no parameter that redirects the write to a different path. The cmdlet writes only the sections of the descriptor that changed since it was read, such as the DACL after `Add-NTFSAccess`, and leaves the other sections of the item as they are, so a descriptor that you did not change writes nothing. Before 5.0.0, the cmdlet wrote every section that it had read, also an unchanged owner, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. +Each descriptor remembers the item it was read from, and the cmdlet writes it back to exactly that item. There is no parameter that redirects the write to a different path. The cmdlet writes only the sections of the descriptor that changed since it was read or last written, such as the DACL after `Add-NTFSAccess`, and leaves the other sections of the item as they are, so a descriptor that you did not change writes nothing. With `-Verbose`, the cmdlet names the sections that it writes, or says that it writes nothing. Before 5.0.0, the cmdlet wrote every section that it had read, also an unchanged owner, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. The cmdlet produces no output unless you use `-PassThru`, which reads the item again after the write and returns a new `FileSystemSecurity2` object that reflects what is now stored on disk. Descriptors can be passed as an array or through the pipeline, and each one is processed on its own. diff --git a/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs b/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs index fb6a3bc..1fa10f4 100644 --- a/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs +++ b/NTFSSecurity/SecurityDescriptorCmdlets/SetSecurityDescriptor.cs @@ -1,6 +1,7 @@ using Security2; using System; using System.Management.Automation; +using System.Security.AccessControl; namespace NTFSSecurity { @@ -40,6 +41,16 @@ namespace NTFSSecurity try { // Only the changed sections, so that an unchanged owner, for example, isn't written back (#34) + var changedSections = sd.ChangedSections; + if (changedSections == AccessControlSections.None) + { + WriteVerbose(string.Format("No section of the security descriptor of '{0}' changed since it was read or last written; nothing is written", sd.FullName)); + } + else + { + WriteVerbose(string.Format("Writing the changed sections of the security descriptor of '{0}': {1}", sd.FullName, changedSections)); + } + sd.WriteChanges(); if (passThru) diff --git a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml index 87bb3ff..64cb669 100644 --- a/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml +++ b/NTFSSecurity/en-US/NTFSSecurity.dll-Help.xml @@ -2427,6 +2427,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd A path that does not exist produces a non-terminating error and the cmdlet continues with the remaining paths. Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + In the `Path` parameter set, the cmdlet writes only the DACL of the item and leaves its owner, its group, and its SACL as they are. Before 5.0.0, it could also write the owner back, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. @@ -3044,6 +3045,7 @@ PS C:\> Get-Privileges | Where-Object { $_.Privilege -in 'Backup', 'Restore', A path that does not exist produces a non-terminating error and the cmdlet continues with the remaining paths. Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + In the `Path` parameter set, the cmdlet writes only the DACL of the item and leaves its owner, its group, and its SACL as they are. Before 5.0.0, it could also write the owner back, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. @@ -9557,6 +9559,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd Before 5.0.0, omitting `-AccessInheritanceEnabled` or `-AuditInheritanceEnabled` could fail with the error "Nullable object must have a value". Before 5.0.0, the cmdlet enabled the privileges even when `EnablePrivileges` was `$false`, and left them enabled. Before 5.0.0, `-PassThru` returned the unchanged state of an item also when the change failed, and stopped the command when the item could not be read. + In the `Path` parameter set, the cmdlet writes only the section that it changes, the DACL or the SACL, and leaves the owner and the group of the item as they are. Before 5.0.0, a change of the access inheritance could also write the owner back, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. @@ -9875,7 +9878,7 @@ PS C:\> Set-NTFSSecurityDescriptor -SecurityDescriptor $sd The `Set-NTFSSecurityDescriptor` cmdlet writes a `Security2.FileSystemSecurity2` object to the file system. It is the final step of the security descriptor workflow: `Get-NTFSSecurityDescriptor` reads a descriptor into memory, cmdlets such as `Add-NTFSAccess`, `Remove-NTFSAccess`, `Set-NTFSOwner`, and `Disable-NTFSAccessInheritance` change that copy through their `-SecurityDescriptor` parameter, and this cmdlet applies all of those changes in a single write. - Each descriptor remembers the item it was read from, and the cmdlet writes it back to exactly that item. There is no parameter that redirects the write to a different path. The cmdlet writes only the sections of the descriptor that changed since it was read, such as the DACL after `Add-NTFSAccess`, and leaves the other sections of the item as they are, so a descriptor that you did not change writes nothing. Before 5.0.0, the cmdlet wrote every section that it had read, also an unchanged owner, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. + Each descriptor remembers the item it was read from, and the cmdlet writes it back to exactly that item. There is no parameter that redirects the write to a different path. The cmdlet writes only the sections of the descriptor that changed since it was read or last written, such as the DACL after `Add-NTFSAccess`, and leaves the other sections of the item as they are, so a descriptor that you did not change writes nothing. With `-Verbose`, the cmdlet names the sections that it writes, or says that it writes nothing. Before 5.0.0, the cmdlet wrote every section that it had read, also an unchanged owner, which failed with error 1307, "This security ID may not be assigned as the owner of this object", when the account may not assign that owner, such as on some file servers. The cmdlet produces no output unless you use `-PassThru`, which reads the item again after the write and returns a new `FileSystemSecurity2` object that reflects what is now stored on disk. Descriptors can be passed as an array or through the pipeline, and each one is processed on its own. When the write fails because access is denied, the cmdlet takes ownership of the item with the account of the current session, writes the descriptor, and restores the previous owner. If that fails as well, it writes a non-terminating error and continues with the next descriptor. Windows checks each section separately: changing the access control list requires the Change Permissions right on the item, changing the owner requires the Take Ownership right or the Take Ownership privilege, assigning ownership to another account requires the Restore privilege, and writing audit entries requires the Security privilege. diff --git a/Tests/SecurityDescriptor.Tests.ps1 b/Tests/SecurityDescriptor.Tests.ps1 index 1426344..d23d26d 100644 --- a/Tests/SecurityDescriptor.Tests.ps1 +++ b/Tests/SecurityDescriptor.Tests.ps1 @@ -157,4 +157,27 @@ Describe 'Set-NTFSSecurityDescriptor' { (Get-Acl -LiteralPath $file).GetOwner($sidType).Value | Should -Be $trustedInstaller } } + + Context 'With -Verbose' { + It 'Should name the sections that it writes' { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'VerboseChanged' + Assert-TestSandboxPath -Sandbox $sandbox -Path $file + $sd = Get-NTFSSecurityDescriptor -Path $file + Add-NTFSAccess -SecurityDescriptor $sd -Account 'Everyone' -AccessRights ReadData + + $messages = Set-NTFSSecurityDescriptor -SecurityDescriptor $sd -Verbose 4>&1 + + $messages.Message | Should -Contain "Writing the changed sections of the security descriptor of '$($sd.FullName)': Access" + } + + It 'Should say that it writes nothing for an unchanged descriptor' { + $file = New-TestSandboxItem -Sandbox $sandbox -Name 'VerboseUnchanged' + $sd = Get-NTFSSecurityDescriptor -Path $file + + $messages = Set-NTFSSecurityDescriptor -SecurityDescriptor $sd -Verbose 4>&1 + + $messages.Message | + Should -Contain "No section of the security descriptor of '$($sd.FullName)' changed since it was read or last written; nothing is written" + } + } } diff --git a/Tests/TestHelpers.Tests.ps1 b/Tests/TestHelpers.Tests.ps1 index fdf7598..355dff4 100644 --- a/Tests/TestHelpers.Tests.ps1 +++ b/Tests/TestHelpers.Tests.ps1 @@ -159,6 +159,15 @@ Describe 'Test helpers' { { Set-TestOwner -Sandbox $sandbox -Path "$sandbox-Other\File.txt" -Sid $trustedInstaller } | Should -Throw -ExpectedMessage 'Refusing to change*' } + + # icacls reports a failure on stderr, which Windows PowerShell turns into a terminating error of its own when + # the caller uses -ErrorAction Stop. + It 'Should throw its own error when icacls fails, also with -ErrorAction Stop' { + $missing = Join-Path -Path $sandbox -ChildPath 'Missing.txt' + + { Set-TestOwner -Sandbox $sandbox -Path $missing -Sid $trustedInstaller -ErrorAction Stop } | + Should -Throw -ExpectedMessage 'icacls could not make*' + } } Context 'Test-IsElevated and Test-PrivilegeHeld' { diff --git a/Tests/TestHelpers.psm1 b/Tests/TestHelpers.psm1 index 21e50c5..4ccdfdb 100644 --- a/Tests/TestHelpers.psm1 +++ b/Tests/TestHelpers.psm1 @@ -293,6 +293,9 @@ function Set-TestOwner { ) Assert-TestSandboxPath -Sandbox $Sandbox -Path $Path + # icacls reports a failure on stderr, which Windows PowerShell turns into a terminating error when the caller uses + # ErrorAction Stop; the exit code decides instead. + $ErrorActionPreference = 'Continue' # icacls resolves a relative path against the working folder of the process, not the location of PowerShell. $location = (Get-Location -PSProvider FileSystem).ProviderPath $fullName = [IO.Path]::GetFullPath([IO.Path]::Combine($location, $Path))