fix: remove generic rights the way FileSystemSecurity removes other rights

The fix for #17 stopped adding Synchronize to an Allow rule with generic rights, which bypassed the exact match of FileSystemSecurity.RemoveAccessRule: -AccessRights GenericAll left a Synchronize-only entry behind when the entry also had Synchronize. RemoveRule now does what FileSystemSecurity does, without its validation: it removes a rule that matches an entry exactly as it is, and otherwise without Synchronize. It covers every mask that .NET rejects, not only the generic rights. The tests cover -RemoveSpecific, a Deny entry, a partial generic mask, and that nothing else is removed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: AI Assistant <ai@example.com>
7 days ago · ada006f5f0
3 changed files with 105 additions and 46 deletions
--- a/.memory-bank/progress.md
+++ b/.memory-bank/progress.md
@ -186,6 +186,8 @@ Numbered as agreed with the maintainer; each is documented on its page.
  location of nine cmdlets, found by the review), #88 (an object passed by
  position), #17 (an entry with `GenericAll`); `Docs/FAQ.md` answers the
  recurring questions.
+- Review of #17: generic rights are removed the way .NET removes other
+  rights, an exact match as it is, otherwise without `Synchronize`.
 - Open bugs: #5 (`-Attributes` matches all, `Get-ChildItem` any; needs a
  decision), #34 and #67 (writes owner and group), #41 (drive root), #82
  (`Size` type data), #90 (trailing space).
--- a/Class/FileSystemAccessRule2.RemoveFileSystemAccessRules.cs
+++ b/Class/FileSystemAccessRule2.RemoveFileSystemAccessRules.cs
@ -1,39 +1,55 @@
 using Alphaleonis.Win32.Filesystem;
 using System.Collections.Generic;
+using System.Linq;
 using System.Security.AccessControl;
+using System.Security.Principal;

 namespace Security2
 {
    public partial class FileSystemAccessRule2
    {
-        // Generic rights, such as GENERIC_ALL, appear in inherit-only entries. FileSystemSecurity.RemoveAccessRule
-        // rebuilds a rule that doesn't match exactly and rejects generic rights then, so such a rule is removed
-        // through ModifyAccessRule, which works on the access mask (#17).
-        private static bool HasGenericRights(FileSystemRights2 rights)
+        // Rights that FileSystemAccessRule.AccessMaskFromRights rejects, such as the generic rights, which Windows
+        // keeps in the inherit-only entries of folders (#17).
+        private static bool HasUnsupportedRights(int accessMask)
        {
-            return ((long)rights & 0xF0000000L) != 0;
+            return accessMask < 0 || accessMask > (int)FileSystemRights.FullControl;
        }

+        // FileSystemSecurity.RemoveAccessRule removes a rule that matches an entry exactly as it is, and otherwise
+        // rebuilds it without the Synchronize right, which fails for unsupported rights. For those, do the same
+        // without rebuilding the rule.
        private static void RemoveRule(FileSystemSecurity sd, FileSystemAccessRule ace, bool removeSpecific)
        {
-            if (HasGenericRights((FileSystemRights2)(int)ace.FileSystemRights))
-            {
-                bool modified;
-                sd.ModifyAccessRule(removeSpecific ? AccessControlModification.RemoveSpecific : AccessControlModification.Remove, ace, out modified);
-            }
-            else if (removeSpecific)
+            var accessMask = (int)ace.FileSystemRights;
+
+            if (!HasUnsupportedRights(accessMask))
            {
+                if (removeSpecific)
                    sd.RemoveAccessRuleSpecific(ace);
-            }
                else
-            {
                    sd.RemoveAccessRule(ace);
+
+                return;
            }
-        }

+            var sid = (SecurityIdentifier)ace.IdentityReference.Translate(typeof(SecurityIdentifier));
+            var exactMatch = sd.GetAccessRules(true, true, typeof(SecurityIdentifier))
+                .OfType<FileSystemAccessRule>()
+                .Any(rule => (int)rule.FileSystemRights == accessMask &&
+                    rule.IdentityReference == sid &&
+                    rule.AccessControlType == ace.AccessControlType);
+
+            var ruleToRemove = exactMatch ? ace : (FileSystemAccessRule)sd.AccessRuleFactory(sid,
+                accessMask & ~(int)FileSystemRights.Synchronize, false, ace.InheritanceFlags, ace.PropagationFlags, ace.AccessControlType);
+
+            // Like RemoveAccessRule, ignore whether an entry was changed: removing an entry that doesn't exist is not
+            // an error.
+            bool modified;
+            sd.ModifyAccessRule(removeSpecific ? AccessControlModification.RemoveSpecific : AccessControlModification.Remove, ruleToRemove, out modified);
+        }
        public static void RemoveFileSystemAccessRule(FileSystemInfo item, IdentityReference2 account, FileSystemRights2 rights, AccessControlType type, InheritanceFlags inheritanceFlags, PropagationFlags propagationFlags, bool removeSpecific = false)
        {
-            if (type == AccessControlType.Allow && !HasGenericRights(rights))
+            if (type == AccessControlType.Allow)
                rights = rights | FileSystemRights2.Synchronize;

            FileSystemAccessRule ace = null;
@ -122,18 +138,11 @@ namespace Security2

        public static FileSystemAccessRule2 RemoveFileSystemAccessRule(FileSystemSecurity2 sd, IdentityReference2 account, FileSystemRights2 rights, AccessControlType type, InheritanceFlags inheritanceFlags, PropagationFlags propagationFlags, bool removeSpecific = false)
        {
-            if (type == AccessControlType.Allow && !HasGenericRights(rights))
+            if (type == AccessControlType.Allow)
                rights = rights | FileSystemRights2.Synchronize;

            var ace = (FileSystemAccessRule)sd.SecurityDescriptor.AccessRuleFactory(account, (int)rights, false, inheritanceFlags, propagationFlags, type);
-            if (sd.IsFile)
-            {
-                RemoveRule(((FileSecurity)sd.SecurityDescriptor), ace, removeSpecific);
-            }
-            else
-            {
-                RemoveRule(((DirectorySecurity)sd.SecurityDescriptor), ace, removeSpecific);
-            }
+            RemoveRule(sd.SecurityDescriptor, ace, removeSpecific);

            return ace;
        }
--- a/Tests/Access.Tests.ps1
+++ b/Tests/Access.Tests.ps1
@ -208,6 +208,73 @@ Describe 'Remove-NTFSAccess' {
        }
    }

+    Context 'With a generic right' {
+        # Before 5.0.0, removing an entry with a generic right such as GENERIC_ALL failed with "The value '269484032' is
+        # not valid", because .NET rebuilds the rule and rejects generic rights (#17). Windows keeps generic rights in
+        # inherit-only entries of folders.
+        BeforeAll {
+            $guests = [System.Security.Principal.SecurityIdentifier]'S-1-5-32-546'
+
+            function New-GenericRightFolder {
+                [Diagnostics.CodeAnalysis.SuppressMessageAttribute(
+                    'PSUseShouldProcessForStateChangingFunctions', '', Justification = 'Test helper that only writes to the sandbox.'
+                )]
+                param ([string] $Entry)
+
+                $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Generic' -Directory
+                Assert-TestSandboxPath -Sandbox $sandbox -Path $folder
+                $acl = Get-Acl -LiteralPath $folder
+                $acl.SetSecurityDescriptorSddlForm(($acl.Sddl -replace 'D:(?<flags>[A-Z]*)', ('D:${flags}' + $Entry)))
+                Set-Acl -LiteralPath $folder -AclObject $acl
+                $folder
+            }
+
+            function Get-GuestsRule {
+                param ([string] $Path)
+
+                (Get-Acl -LiteralPath $Path).GetAccessRules($true, $false, [System.Security.Principal.SecurityIdentifier]) |
+                    Where-Object -Property IdentityReference -EQ -Value $guests
+            }
+        }
+
+        It 'Should remove an inherit-only entry that Get-NTFSAccess returned, and nothing else' -ForEach @(
+            @{ Case = 'Allow'; Entry = '(A;OICIIO;GA;;;BG)'; Specific = $false }
+            @{ Case = 'Allow with -RemoveSpecific'; Entry = '(A;OICIIO;GA;;;BG)'; Specific = $true }
+            @{ Case = 'Deny'; Entry = '(D;OICIIO;GA;;;BG)'; Specific = $false }
+        ) {
+            $folder = New-GenericRightFolder -Entry $Entry
+            $before = (Get-Acl -LiteralPath $folder).Sddl
+            $before | Should -Match ([regex]::Escape($Entry))
+
+            Get-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -ExcludeInherited |
+                Remove-NTFSAccess -RemoveSpecific:$Specific -ErrorVariable removeErrors -ErrorAction SilentlyContinue
+
+            $removeErrors | Should -BeNullOrEmpty
+            (Get-Acl -LiteralPath $folder).Sddl | Should -BeExactly $before.Replace($Entry, '')
+        }
+
+        It 'Should remove only the requested generic right from an entry with two' {
+            $folder = New-GenericRightFolder -Entry '(A;OICIIO;0x90000000;;;BG)'
+
+            Remove-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -AccessRights GenericRead -InheritanceFlags ContainerInherit, ObjectInherit -PropagationFlags InheritOnly -ErrorVariable removeErrors -ErrorAction SilentlyContinue
+
+            $removeErrors | Should -BeNullOrEmpty
+            $rule = Get-GuestsRule -Path $folder
+            $rule | Should -HaveCount 1
+            [int] $rule.FileSystemRights | Should -Be 0x10000000
+        }
+
+        # A rule that matches the entry exactly is removed as it is, with the Synchronize right that the module adds
+        # to an Allow rule; otherwise an entry with only Synchronize would be left behind.
+        It 'Should remove an entry with GenericAll and Synchronize when -AccessRights names GenericAll' {
+            $folder = New-GenericRightFolder -Entry '(A;OICIIO;0x10100000;;;BG)'
+
+            Remove-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -AccessRights GenericAll -InheritanceFlags ContainerInherit, ObjectInherit -PropagationFlags InheritOnly -ErrorVariable removeErrors -ErrorAction SilentlyContinue
+
+            $removeErrors | Should -BeNullOrEmpty
+            Get-GuestsRule -Path $folder | Should -BeNullOrEmpty
+        }
+    }
    Context 'With -RemoveSpecific' {
        BeforeEach {
            $removeFolder = New-TestSandboxItem -Sandbox $sandbox -Name 'RemoveSpecific' -Directory
@ -259,25 +326,6 @@ Describe 'Add-NTFSAccess' {
    }
 }

-Describe 'Remove-NTFSAccess with generic rights' {
-    # Before 5.0.0, removing an entry with a generic right such as GENERIC_ALL failed with "The value '269484032' is not
-    # valid", because .NET rebuilds the rule and rejects generic rights (#17). Windows keeps generic rights in
-    # inherit-only entries of folders.
-    It 'Should remove an inherit-only GenericAll entry that Get-NTFSAccess returned' {
-        $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'Generic' -Directory
-        Assert-TestSandboxPath -Sandbox $sandbox -Path $folder
-        $acl = Get-Acl -LiteralPath $folder
-        $acl.SetSecurityDescriptorSddlForm(($acl.Sddl -replace 'D:(?<flags>[A-Z]*)', 'D:${flags}(A;OICIIO;GA;;;S-1-5-32-546)'))
-        Set-Acl -LiteralPath $folder -AclObject $acl
-        (Get-Acl -LiteralPath $folder).Sddl | Should -Match '\(A;OICIIO;GA;;;BG\)'
-
-        Get-NTFSAccess -Path $folder -Account 'S-1-5-32-546' -ExcludeInherited |
-            Remove-NTFSAccess -ErrorVariable removeErrors -ErrorAction SilentlyContinue
-
-        $removeErrors | Should -BeNullOrEmpty
-        (Get-Acl -LiteralPath $folder).Sddl | Should -Not -Match ';;;BG\)'
-    }
-}
 Describe 'Security descriptor parameter sets' {
    BeforeAll {
        $folder = New-TestSandboxItem -Sandbox $sandbox -Name 'ParameterSets' -Directory