Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add file creation method that takes an ACL #42099

Merged
carlossanlop merged 17 commits into from
Oct 29, 2019
Merged

Add file creation method that takes an ACL #42099

carlossanlop merged 17 commits into from
Oct 29, 2019

Conversation

@carlossanlop
Copy link

@carlossanlop carlossanlop commented Oct 24, 2019 •
edited
Loading

Approved API Proposal: #41614
Related change for directory creation method that takes an ACL: #41834 [merged and ported to 3.1 Prev2]

Description

We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact

Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

  • Potential security hole as files can be accessed between creation and modification.
  • Porting difficulties as there isn't a 1-1 API replacement
  • Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
  • This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.

This change is expected to be backported to 3.1.

@carlossanlop carlossanlop added this to the 5.0 milestone Oct 24, 2019
@carlossanlop carlossanlop self-assigned this Oct 24, 2019
@carlossanlop
Copy link
Author

carlossanlop commented Oct 25, 2019

The unit tests will fail with my latest commit. Some expected exceptions were different than expected, so I'm investigating.

@carlossanlop carlossanlop requested a review from jkotas October 29, 2019 17:27
@carlossanlop
Copy link
Author

carlossanlop commented Oct 29, 2019

@JeremyKuhne @danmosemsft @stephentoub @jkotas I have addressed the latest comments/suggestions and the build passed. Would you mind taking another look?

@carlossanlop carlossanlop marked this pull request as ready for review October 29, 2019 19:19
danmoseley
danmoseley reviewed Oct 29, 2019
View reviewed changes
src/System.IO.FileSystem.AccessControl/src/System/IO/FileSystemAclExtensions.netcoreapp.cs
}
if ((rights & FileSystemRights.WriteData) != 0 || ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_WRITE) != 0)
{
access = access == FileAccess.Read ? FileAccess.ReadWrite : FileAccess.Write;
Copy link
Member

@danmoseley danmoseley Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: our style would be to parenthesize
access = (access == FileAccess.Read) ? FileAccess.ReadWrite : FileAccess.Write;

danmoseley
danmoseley reviewed Oct 29, 2019
View reviewed changes
src/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

Assert.Throws<ArgumentNullException>("fileInfo", () =>
{
if (PlatformDetection.IsFullFramework)
Copy link
Member

@danmoseley danmoseley Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, there's a bunch of repetition among these tests, you could do

private void FileInfoCreate(info, mode, rights, share, buffer, options, security)
{
                if (PlatformDetection.IsFullFramework)
                {
                    FileSystemAclExtensions.Create(....);
                }
                else
                {
                    info.Create(...);
                }
}

Then your test would be just eg

        [Fact]
        public void FileInfo_Create_NullFileSecurity()
        {
            FileInfo info = new FileInfo("path");

            Assert.Throws<ArgumentNullException>("fileSecurity", () => FileInfoCreate(info, FileMode.Create, FileSystemRights.WriteData, FileShare.Read, DefaultBufferSize, FileOptions.None, null);
        }

Copy link
Member

@danmoseley danmoseley Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or - another idea. You could have a separate source file that's only included when building tests for .NET Framework which defines your new extension method and just passes it through to the .NET Framework method. That might be nice because the tests themselves don't need to know it exists. They just call the .NET Core method.

If it's worth doing, you coudl do it later.

Copy link
Member

@danmoseley danmoseley Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally you want tests to be short and sweet, because that makes it easier to see what changes between them (what the test cases are)

Copy link
Author

@carlossanlop carlossanlop Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally understood, @danmosemsft . Would it be ok if I address this in a separate PR so I get this merged today?

Copy link
Member

@danmoseley danmoseley Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call

danmoseley
danmoseley approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@danmoseley danmoseley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to defer any minor things until after the port.

@carlossanlop carlossanlop merged commit 508cbc4 into dotnet:master Oct 29, 2019
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Oct 29, 2019
@carlossanlop @dotnet-bot
Add file creation method that takes an ACL (dotnet/corefx#42099)
b639da3 
Approved API Proposal: #41614
Related change for directory creation method that takes an ACL: #41834 -merged and ported to 3.1 Prev2

Description
We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact
Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

Potential security hole as files can be accessed between creation and modification.
Porting difficulties as there isn't a 1-1 API replacement
Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.
This change is expected to be backported to 3.1.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/coreclr that referenced this pull request Oct 30, 2019
@carlossanlop @dotnet-bot
Add file creation method that takes an ACL (dotnet/corefx#42099)
43c4668 
Approved API Proposal: #41614
Related change for directory creation method that takes an ACL: #41834 -merged and ported to 3.1 Prev2

Description
We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact
Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

Potential security hole as files can be accessed between creation and modification.
Porting difficulties as there isn't a 1-1 API replacement
Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.
This change is expected to be backported to 3.1.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Oct 30, 2019
@carlossanlop @dotnet-bot
Add file creation method that takes an ACL (dotnet/corefx#42099)
eff34cc 
Approved API Proposal: #41614
Related change for directory creation method that takes an ACL: #41834 -merged and ported to 3.1 Prev2

Description
We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact
Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

Potential security hole as files can be accessed between creation and modification.
Porting difficulties as there isn't a 1-1 API replacement
Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.
This change is expected to be backported to 3.1.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/coreclr that referenced this pull request Oct 30, 2019
@carlossanlop @jkotas
Add file creation method that takes an ACL (dotnet/corefx#42099)
be9323f 
Approved API Proposal: #41614
Related change for directory creation method that takes an ACL: #41834 -merged and ported to 3.1 Prev2

Description
We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact
Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

Potential security hole as files can be accessed between creation and modification.
Porting difficulties as there isn't a 1-1 API replacement
Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.
This change is expected to be backported to 3.1.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
vargaz pushed a commit to mono/mono that referenced this pull request Oct 30, 2019
@Dotnet-GitSync-Bot @vargaz
Mirror changes from mono/coreclr,corefx (#17609)
3695d87 
* Revert removal of SuppressGCTransition from SystemNative_GetTimestamp() (#27473)

* Revert removal of SuppressGCTransition from SystemNative_GetTimestamp()

* Insert GC_POLL before statement with unmanaged call.

* JIT test for insertion of GCPoll

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Add file creation method that takes an ACL (dotnet/corefx#42099)

Approved API Proposal: #41614
Related change for directory creation method that takes an ACL: #41834 -merged and ported to 3.1 Prev2

Description
We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact
Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

Potential security hole as files can be accessed between creation and modification.
Porting difficulties as there isn't a 1-1 API replacement
Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.
This change is expected to be backported to 3.1.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Oct 30, 2019
@carlossanlop @jkotas
Add file creation method that takes an ACL (dotnet/corefx#42099)
28e5c53 
Approved API Proposal: #41614
Related change for directory creation method that takes an ACL: #41834 -merged and ported to 3.1 Prev2

Description
We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact
Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

Potential security hole as files can be accessed between creation and modification.
Porting difficulties as there isn't a 1-1 API replacement
Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.
This change is expected to be backported to 3.1.

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@carlossanlop carlossanlop mentioned this pull request Oct 30, 2019
Merged
danmoseley pushed a commit that referenced this pull request Oct 30, 2019
@carlossanlop @danmoseley
Add file creation method that takes an ACL (#42099) (#42228)
0f547d7
@carlossanlop carlossanlop mentioned this pull request Jan 9, 2020
Open
@carlossanlop carlossanlop mentioned this pull request Feb 1, 2020
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@carlossanlop
Add file creation method that takes an ACL (dotnet/corefx#42099)
70e8131 
Approved API Proposal: dotnet/corefx#41614
Related change for directory creation method that takes an ACL: dotnet/corefx#41834 -merged and ported to 3.1 Prev2

Description
We have extension methods in System.IO.FileSystem.AclExtensions that let the user get and set ACLs for existing files, but we do not have methods that create files with predefined ACLs.
.NET ACL (Access Control List) support is Windows specific. This change will reside inside the System.IO.FileSystem.AccessControl assembly.

Customer impact
Before this change, customers had to create a file or filestream, then set its ACLs. This presents a few problems:

Potential security hole as files can be accessed between creation and modification.
Porting difficulties as there isn't a 1-1 API replacement
Stability issues with background processes (file filters) can prevent modifying ACLs right after creation (typically surfaces as a security exception).
This change addresses those problems by adding a new extension method that allows creating a file and ensuring the provided ACLs are set during creation.
This change is expected to be backported to 3.1.

Commit migrated from dotnet/corefx@508cbc4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ericstj ericstj Awaiting requested review from ericstj

@stephentoub stephentoub Awaiting requested review from stephentoub

@jkotas jkotas Awaiting requested review from jkotas

2 more reviewers

@danmoseley danmoseley danmoseley approved these changes

@JeremyKuhne JeremyKuhne JeremyKuhne approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@carlossanlop carlossanlop

Labels

area-System.IO

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

6 participants

@carlossanlop @danmoseley @stephentoub @jkotas @JeremyKuhne @ericstj