Fix FileSystemAclExtensions.Create when passing a null FileSecurity (#61297)

* Make FileSecurity parameter nullable.

* Add missing ArgumentException message for FileMode.Append.

* Refactor tests to ensure FileSecurity is tested with all FileMode and FileSystemRights combinations. Separate special cases.

* Remove exception that throws when FileSecurity is null.
Ensure we have logic that can create a FileHandle when FileSecurity is null.
Fix bug where FileShare.Inheritable causes IOException because it is being unexpectedly passed to the P/Invoke (it should just be saved in the SECURITY_ATTRIBUTES struct).
Add documentation to mention this parameter as optional.
Ensure all exceptions match exactly what we have in .NET Framework, with simpler logic.

* Address suggestions

Co-authored-by: carlossanlop <[email protected]>

Author: carlossanlop

src/libraries/System.IO.FileSystem.AccessControl/ref/System.IO.FileSystem.AccessControl.cs

@@ -9,7 +9,7 @@ namespace System.IO
     public static partial class FileSystemAclExtensions
     {
         public static void Create(this System.IO.DirectoryInfo directoryInfo, System.Security.AccessControl.DirectorySecurity directorySecurity) { }
-        public static System.IO.FileStream Create(this System.IO.FileInfo fileInfo, System.IO.FileMode mode, System.Security.AccessControl.FileSystemRights rights, System.IO.FileShare share, int bufferSize, System.IO.FileOptions options, System.Security.AccessControl.FileSecurity fileSecurity) { throw null; }
+        public static System.IO.FileStream Create(this System.IO.FileInfo fileInfo, System.IO.FileMode mode, System.Security.AccessControl.FileSystemRights rights, System.IO.FileShare share, int bufferSize, System.IO.FileOptions options, System.Security.AccessControl.FileSecurity? fileSecurity) { throw null; }
         public static System.IO.DirectoryInfo CreateDirectory(this System.Security.AccessControl.DirectorySecurity directorySecurity, string path) { throw null; }
         public static System.Security.AccessControl.DirectorySecurity GetAccessControl(this System.IO.DirectoryInfo directoryInfo) { throw null; }
         public static System.Security.AccessControl.DirectorySecurity GetAccessControl(this System.IO.DirectoryInfo directoryInfo, System.Security.AccessControl.AccessControlSections includeSections) { throw null; }

src/libraries/System.IO.FileSystem.AccessControl/src/Resources/Strings.resx

@@ -122,6 +122,9 @@
   <data name="Arg_MustBeIdentityReferenceType" xml:space="preserve">
     <value>Type must be an IdentityReference, such as NTAccount or SecurityIdentifier.</value>
   </data>
+  <data name="Argument_InvalidAppendMode" xml:space="preserve">
+    <value>Append access can be requested only in write-only mode.</value>
+  </data>
   <data name="Argument_InvalidEnumValue" xml:space="preserve">
     <value>The value '{0}' is not valid for this usage of the type {1}.</value>
   </data>

src/libraries/System.IO.FileSystem.AccessControl/src/System/IO/FileSystemAclExtensions.cs

@@ -159,29 +159,24 @@ public static void Create(this DirectoryInfo directoryInfo, DirectorySecurity di
         /// <param name="share">One of the enumeration values for controlling the kind of access other FileStream objects can have to the same file.</param>
         /// <param name="bufferSize">The number of bytes buffered for reads and writes to the file.</param>
         /// <param name="options">One of the enumeration values that describes how to create or overwrite the file.</param>
-        /// <param name="fileSecurity">An object that determines the access control and audit security for the file.</param>
+        /// <param name="fileSecurity">An optional object that determines the access control and audit security for the file.</param>
         /// <returns>A file stream for the newly created file.</returns>
         /// <exception cref="ArgumentException">The <paramref name="rights" /> and <paramref name="mode" /> combination is invalid.</exception>
-        /// <exception cref="ArgumentNullException"><paramref name="fileInfo" /> or <paramref name="fileSecurity" /> is <see langword="null" />.</exception>
+        /// <exception cref="ArgumentNullException"><paramref name="fileInfo" /> is <see langword="null" />.</exception>
         /// <exception cref="ArgumentOutOfRangeException"><paramref name="mode" /> or <paramref name="share" /> are out of their legal enum range.
         ///-or-
         /// <paramref name="bufferSize" /> is not a positive number.</exception>
         /// <exception cref="DirectoryNotFoundException">Could not find a part of the path.</exception>
         /// <exception cref="IOException">An I/O error occurs.</exception>
         /// <exception cref="UnauthorizedAccessException">Access to the path is denied.</exception>
         /// <remarks>This extension method was added to .NET Core to bring the functionality that was provided by the `System.IO.FileStream.#ctor(System.String,System.IO.FileMode,System.Security.AccessControl.FileSystemRights,System.IO.FileShare,System.Int32,System.IO.FileOptions,System.Security.AccessControl.FileSecurity)` .NET Framework constructor.</remarks>
-        public static FileStream Create(this FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, int bufferSize, FileOptions options, FileSecurity fileSecurity)
+        public static FileStream Create(this FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, int bufferSize, FileOptions options, FileSecurity? fileSecurity)
         {
             if (fileInfo == null)
             {
                 throw new ArgumentNullException(nameof(fileInfo));
             }
 
-            if (fileSecurity == null)
-            {
-                throw new ArgumentNullException(nameof(fileSecurity));
-            }
-
             // don't include inheritable in our bounds check for share
             FileShare tempshare = share & ~FileShare.Inheritable;
 
@@ -200,18 +195,33 @@ public static FileStream Create(this FileInfo fileInfo, FileMode mode, FileSyste
                 throw new ArgumentOutOfRangeException(nameof(bufferSize), SR.ArgumentOutOfRange_NeedPosNum);
             }
 
-            // Do not allow using combinations of non-writing file system rights with writing file modes
+            // Do not combine writing modes with exclusively read rights
+            // Write contains AppendData, WriteAttributes, WriteData and WriteExtendedAttributes
             if ((rights & FileSystemRights.Write) == 0 &&
                 (mode == FileMode.Truncate || mode == FileMode.CreateNew || mode == FileMode.Create || mode == FileMode.Append))
             {
                 throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndFileSystemRightsCombo, mode, rights));
             }
 
+            // Additionally, append is disallowed if any read rights are provided
+            // ReadAndExecute contains ExecuteFile, ReadAttributes, ReadData, ReadExtendedAttributes and ReadPermissions
+            if ((rights & FileSystemRights.ReadAndExecute) != 0 && mode == FileMode.Append)
+            {
+                throw new ArgumentException(SR.Argument_InvalidAppendMode);
+            }
+
+            // Cannot truncate unless all of the write rights are provided
+            // Write contains AppendData, WriteAttributes, WriteData and WriteExtendedAttributes
+            if (mode == FileMode.Truncate && (rights & FileSystemRights.Write) != FileSystemRights.Write)
+            {
+                throw new ArgumentException(SR.Format(SR.Argument_InvalidFileModeAndFileSystemRightsCombo, mode, rights));
+            }
+
             SafeFileHandle handle = CreateFileHandle(fileInfo.FullName, mode, rights, share, options, fileSecurity);
 
             try
             {
-                return new FileStream(handle, GetFileStreamFileAccess(rights), bufferSize, (options & FileOptions.Asynchronous) != 0);
+                return new FileStream(handle, GetFileAccessFromRights(rights), bufferSize, (options & FileOptions.Asynchronous) != 0);
             }
             catch
             {
@@ -258,23 +268,48 @@ public static DirectoryInfo CreateDirectory(this DirectorySecurity directorySecu
 
         // In the context of a FileStream, the only ACCESS_MASK ACE rights we care about are reading/writing data and the generic read/write rights.
         // See: https://docs.microsoft.com/en-us/windows/win32/secauthz/access-mask
-        private static FileAccess GetFileStreamFileAccess(FileSystemRights rights)
+        private static FileAccess GetFileAccessFromRights(FileSystemRights rights)
         {
             FileAccess access = 0;
-            if ((rights & FileSystemRights.ReadData) != 0 || ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_READ) != 0)
+
+            if ((rights & FileSystemRights.FullControl) != 0 ||
+                (rights & FileSystemRights.Modify) != 0)
+            {
+                return FileAccess.ReadWrite;
+            }
+
+            if ((rights & FileSystemRights.ReadData) != 0 || // Same as ListDirectory
+                (rights & FileSystemRights.ReadExtendedAttributes) != 0 ||
+                (rights & FileSystemRights.ExecuteFile) != 0 || // Same as Traverse
+                (rights & FileSystemRights.ReadAttributes) != 0 ||
+                (rights & FileSystemRights.ReadPermissions) != 0 ||
+                (rights & FileSystemRights.TakeOwnership) != 0 ||
+                ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_READ) != 0)
             {
                 access = FileAccess.Read;
             }
 
-            if ((rights & FileSystemRights.WriteData) != 0 || ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_WRITE) != 0)
+            if ((rights & FileSystemRights.AppendData) != 0 || // Same as CreateDirectories
+                (rights & FileSystemRights.ChangePermissions) != 0 ||
+                (rights & FileSystemRights.Delete) != 0 ||
+                (rights & FileSystemRights.DeleteSubdirectoriesAndFiles) != 0 ||
+                (rights & FileSystemRights.WriteAttributes) != 0 ||
+                (rights & FileSystemRights.WriteData) != 0 || // Same as CreateFiles
+                (rights & FileSystemRights.WriteExtendedAttributes) != 0 ||
+                ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_WRITE) != 0)
+            {
+                access |= FileAccess.Write;
+            }
+
+            if (access == 0)
             {
-                access = access == FileAccess.Read ? FileAccess.ReadWrite : FileAccess.Write;
+                throw new ArgumentOutOfRangeException(nameof(rights));
             }
 
             return access;
         }
 
-        private static unsafe SafeFileHandle CreateFileHandle(string fullPath, FileMode mode, FileSystemRights rights, FileShare share, FileOptions options, FileSecurity security)
+        private static unsafe SafeFileHandle CreateFileHandle(string fullPath, FileMode mode, FileSystemRights rights, FileShare share, FileOptions options, FileSecurity? security)
         {
             Debug.Assert(fullPath != null);
 
@@ -291,23 +326,39 @@ private static unsafe SafeFileHandle CreateFileHandle(string fullPath, FileMode
 
             SafeFileHandle handle;
 
-            fixed (byte* pSecurityDescriptor = security.GetSecurityDescriptorBinaryForm())
+            var secAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES
             {
-                var secAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES
+                nLength = (uint)sizeof(Interop.Kernel32.SECURITY_ATTRIBUTES),
+                bInheritHandle = ((share & FileShare.Inheritable) != 0) ? Interop.BOOL.TRUE : Interop.BOOL.FALSE,
+            };
+
+            if (security != null)
+            {
+                fixed (byte* pSecurityDescriptor = security.GetSecurityDescriptorBinaryForm())
                 {
-                    nLength = (uint)sizeof(Interop.Kernel32.SECURITY_ATTRIBUTES),
-                    bInheritHandle = ((share & FileShare.Inheritable) != 0) ? Interop.BOOL.TRUE : Interop.BOOL.FALSE,
-                    lpSecurityDescriptor = (IntPtr)pSecurityDescriptor
-                };
+                    secAttrs.lpSecurityDescriptor = (IntPtr)pSecurityDescriptor;
+                    handle = CreateFileHandleInternal(fullPath, mode, rights, share, flagsAndAttributes, &secAttrs);
+                }
+            }
+            else
+            {
+                handle = CreateFileHandleInternal(fullPath, mode, rights, share, flagsAndAttributes, &secAttrs);
+            }
+
+            return handle;
 
+            static unsafe SafeFileHandle CreateFileHandleInternal(string fullPath, FileMode mode, FileSystemRights rights, FileShare share, int flagsAndAttributes, Interop.Kernel32.SECURITY_ATTRIBUTES* secAttrs)
+            {
+                SafeFileHandle handle;
                 using (DisableMediaInsertionPrompt.Create())
                 {
-                    handle = Interop.Kernel32.CreateFile(fullPath, (int)rights, share, &secAttrs, mode, flagsAndAttributes, IntPtr.Zero);
+                    // The Inheritable bit is only set in the SECURITY_ATTRIBUTES struct,
+                    // and should not be passed to the CreateFile P/Invoke.
+                    handle = Interop.Kernel32.CreateFile(fullPath, (int)rights, (share & ~FileShare.Inheritable), secAttrs, mode, flagsAndAttributes, IntPtr.Zero);
                     ValidateFileHandle(handle, fullPath);
                 }
+                return handle;
             }
-
-            return handle;
         }
 
         private static void ValidateFileHandle(SafeFileHandle handle, string fullPath)