Skip to content

Commit eba5293

Browse files
bendwyerbendwyer
authored
FileSystemAccessRule: No longer tries to write back owner from the ACL (#4)
- FileSystemAccessRule - Fixed an issue where the owner of ACL was written back resulting in an error "The security identifier is not allowed to be the owner of this object" (issue #3).
1 parent 432ccea commit eba5293

3 files changed

Lines changed: 84 additions & 21 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
55

66
## [Unreleased]
77

8+
### Fixed
9+
10+
- FileSystemAccessRule
11+
- Fixed an issue where the owner of ACL was written back resulting in an
12+
error "The security identifier is not allowed to be the owner of this
13+
object" ([issue #3](https://github.com/dsccommunity/FileSystemDsc/issues/3)).
14+
815
## [1.1.0] - 2020-03-09
916

1017
### Added

‎source/DSCResources/DSC_FileSystemAccessRule/DSC_FileSystemAccessRule.psm1‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,6 @@ $script:localizedData = Get-LocalizedData -DefaultUICulture 'en-US'
1313
1414
.PARAMETER Identity
1515
The identity to set permissions for.
16-
17-
.NOTES
18-
This function uses Get-Acl that was first introduced in Windows Powershell 5.1.
1916
#>
2017
function Get-TargetResource
2118
{
@@ -136,7 +133,7 @@ function Get-TargetResource
136133
$script:localizedData.PathExist -f $Identity
137134
)
138135

139-
$acl = Get-Acl -Path $Path
136+
$acl = Get-ACLAccess -Path $Path
140137
$accessRules = $acl.Access
141138

142139
<#
@@ -199,7 +196,7 @@ function Get-TargetResource
199196
Not used in Set-TargetResource.
200197
201198
.NOTES
202-
This function uses Get-Acl and Set-Acl that was first introduced in
199+
This function uses Set-Acl that was first introduced in
203200
Windows Powershell 5.1.
204201
#>
205202
function Set-TargetResource
@@ -261,7 +258,7 @@ function Set-TargetResource
261258
New-ObjectNotFoundException -Message $errorMessage
262259
}
263260

264-
$acl = Get-Acl -Path $Path
261+
$acl = Get-ACLAccess -Path $Path
265262

266263
if ($Ensure -eq 'Present')
267264
{
@@ -582,3 +579,34 @@ function Test-TargetResource
582579

583580
return $result
584581
}
582+
583+
<#
584+
.SYNOPSIS
585+
This function is wrapper for getting the DACL for the specified path.
586+
587+
.PARAMETER Path
588+
The path to the item that should have permissions set.
589+
590+
.NOTES
591+
"Well the limited features of Get-ACL means that you always read the full
592+
security descriptor including the owner whether you intended to or not.
593+
That means that when you come to write to the object based on a modified
594+
version of what you read, you are attempting to write back to the owner
595+
attribute.
596+
597+
The GetAccessControl('Access') method reads only the DACL so when you
598+
write it back you are not trying to write something you did not intend to."
599+
https://www.mickputley.net/2015/11/set-acl-security-identifier-is-not.html
600+
https://github.com/dsccommunity/FileSystemDsc/issues/3
601+
#>
602+
function Get-ACLAccess
603+
{
604+
[CmdletBinding()]
605+
param
606+
(
607+
[Parameter(Mandatory = $true)]
608+
[System.String]
609+
$Path
610+
)
611+
return (Get-Item -Path $Path).GetAccessControl('Access')
612+
}

‎tests/Unit/DSC_FileSystemAccessRule.Tests.ps1‎

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ try
5555
Context 'When the node does not belong to a cluster' {
5656
BeforeAll {
5757
Mock -CommandName Write-Warning
58-
Mock -CommandName Get-Acl
58+
Mock -CommandName Get-ACLAccess
5959

6060
Mock -CommandName Test-Path -MockWith {
6161
return $false
@@ -67,7 +67,7 @@ try
6767
It 'Should not throw and output the correct warning message' {
6868
{ $script:result = Get-TargetResource @mockGetTargetResourceParameters } | Should -Not -Throw
6969

70-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 0 -Scope It
70+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 0 -Scope It
7171
Assert-MockCalled -CommandName Test-Path -Times 1 -Exactly -Scope It
7272

7373
Assert-MockCalled -CommandName Get-CimInstance -ParameterFilter {
@@ -93,7 +93,7 @@ try
9393
Context 'When no cluster disk partition is found that uses the path' {
9494
BeforeAll {
9595
Mock -CommandName Write-Warning
96-
Mock -CommandName Get-Acl
96+
Mock -CommandName Get-ACLAccess
9797
Mock -CommandName Get-CimAssociatedInstance
9898
Mock -CommandName Test-Path -MockWith {
9999
return $false
@@ -121,7 +121,7 @@ try
121121
It 'Should not throw and output the correct warning message' {
122122
{ $script:result = Get-TargetResource @mockGetTargetResourceParameters } | Should -Not -Throw
123123

124-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 0 -Scope It
124+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 0 -Scope It
125125
Assert-MockCalled -CommandName Test-Path -Times 1 -Exactly -Scope It
126126

127127
Assert-MockCalled -CommandName Get-CimInstance -ParameterFilter {
@@ -158,7 +158,7 @@ try
158158
Context 'When the current node is not a possible cluster resource owner' {
159159
BeforeAll {
160160
Mock -CommandName Write-Warning
161-
Mock -CommandName Get-Acl
161+
Mock -CommandName Get-ACLAccess
162162
Mock -CommandName Test-Path -MockWith {
163163
return $false
164164
}
@@ -208,7 +208,7 @@ try
208208
It 'Should not throw and output the correct warning message' {
209209
{ $script:result = Get-TargetResource @mockGetTargetResourceParameters } | Should -Not -Throw
210210

211-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 0 -Scope It
211+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 0 -Scope It
212212
Assert-MockCalled -CommandName Test-Path -Times 1 -Exactly -Scope It
213213

214214
Assert-MockCalled -CommandName Get-CimInstance -ParameterFilter {
@@ -250,7 +250,7 @@ try
250250
Context 'When the configuration is present' {
251251
Context 'When the path exists' {
252252
BeforeAll {
253-
Mock -CommandName Get-Acl -MockWith {
253+
Mock -CommandName Get-ACLAccess -MockWith {
254254
return New-Object -TypeName PSObject |
255255
Add-Member -MemberType 'NoteProperty' -Name 'Access' -Value @(
256256
New-Object -TypeName PSObject |
@@ -269,7 +269,7 @@ try
269269
It 'Should get the access rules without throwing' {
270270
{ $script:result = Get-TargetResource @mockGetTargetResourceParameters } | Should -Not -Throw
271271

272-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 1 -Scope It
272+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 1 -Scope It
273273
}
274274

275275
It 'Should return the state as present' {
@@ -292,7 +292,7 @@ try
292292
Context 'When path is not found on the node, but the path is found in a cluster disk partition and the current node is a possible cluster resource owner' {
293293
BeforeAll {
294294
Mock -CommandName Write-Warning
295-
Mock -CommandName Get-Acl
295+
Mock -CommandName Get-ACLAccess
296296
Mock -CommandName Test-Path -MockWith {
297297
return $false
298298
}
@@ -347,7 +347,7 @@ try
347347
It 'Should not throw and should not output a warning message' {
348348
{ $script:result = Get-TargetResource @mockGetTargetResourceParameters } | Should -Not -Throw
349349

350-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 0 -Scope It
350+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 0 -Scope It
351351
Assert-MockCalled -CommandName Test-Path -Times 1 -Exactly -Scope It
352352

353353
Assert-MockCalled -CommandName Get-CimInstance -ParameterFilter {
@@ -659,7 +659,7 @@ try
659659
} -PassThru -Force
660660
}
661661

662-
Mock -CommandName Get-Acl -MockWith $mockGetAcl
662+
Mock -CommandName Get-ACLAccess -MockWith $mockGetAcl
663663
Mock -CommandName Set-Acl
664664
}
665665

@@ -710,7 +710,7 @@ try
710710
It 'Should call the correct methods and mocks to remove the permissions' {
711711
{ Set-TargetResource @mockSetTargetResourceParameters } | Should -Not -Throw
712712

713-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 1 -Scope It
713+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 1 -Scope It
714714

715715
$script:SetAccessRuleMethodCallCount | Should -Be 0
716716
$script:PurgeAccessRulesMethodCallCount | Should -Be 0
@@ -731,7 +731,7 @@ try
731731
It 'Should call the correct methods and mocks to remove the permissions' {
732732
{ Set-TargetResource @mockSetTargetResourceParameters } | Should -Not -Throw
733733

734-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 1 -Scope It
734+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 1 -Scope It
735735

736736
$script:SetAccessRuleMethodCallCount | Should -Be 0
737737
$script:PurgeAccessRulesMethodCallCount | Should -Be 1
@@ -750,7 +750,7 @@ try
750750
It 'Should call the correct methods and mocks to remove the permissions' {
751751
{ Set-TargetResource @mockSetTargetResourceParameters } | Should -Not -Throw
752752

753-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 1 -Scope It
753+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 1 -Scope It
754754

755755
$script:SetAccessRuleMethodCallCount | Should -Be 0
756756
$script:PurgeAccessRulesMethodCallCount | Should -Be 1
@@ -771,7 +771,7 @@ try
771771
It 'Should call the correct methods and mocks to set the permissions' {
772772
{ Set-TargetResource @mockSetTargetResourceParameters } | Should -Not -Throw
773773

774-
Assert-MockCalled -CommandName Get-Acl -Exactly -Times 1 -Scope It
774+
Assert-MockCalled -CommandName Get-ACLAccess -Exactly -Times 1 -Scope It
775775

776776
$script:SetAccessRuleMethodCallCount | Should -Be 1
777777
$script:PurgeAccessRulesMethodCallCount | Should -Be 0
@@ -799,6 +799,34 @@ try
799799
}
800800
}
801801
}
802+
803+
Describe 'DSC_FileSystemAccessRule\Get-ACLAccess' -Tag 'Helper' {
804+
BeforeAll {
805+
Mock -CommandName Get-Item -MockWith {
806+
return New-Object -TypeName PSObject |
807+
Add-Member -MemberType 'ScriptMethod' -Name 'GetAccessControl' -Value {
808+
return New-Object -TypeName PSObject |
809+
# Regression test for issue #3
810+
Add-Member -MemberType 'NoteProperty' -Name 'Owner' -Value $null -PassThru |
811+
Add-Member -MemberType 'NoteProperty' -Name 'Access' -Value @(
812+
New-Object -TypeName PSObject |
813+
Add-Member -MemberType 'NoteProperty' -Name 'IdentityReference' -Value $mockIdentity -PassThru |
814+
Add-Member -MemberType 'NoteProperty' -Name 'FileSystemRights' -Value $mockFileSystemRights -PassThru -Force
815+
) -PassThru -Force
816+
} -PassThru -Force
817+
}
818+
}
819+
820+
It 'Should return the correct access control list (ACL)' {
821+
$result = Get-ACLAccess -Path 'AnyPath'
822+
823+
$result.Access[0].IdentityReference | Should -Be $mockIdentity
824+
$result.Access[0].FileSystemRights | Should -Be $mockFileSystemRights
825+
826+
# Regression test for issue #3
827+
$result.Owner | Should -BeNullOrEmpty
828+
}
829+
}
802830
}
803831
}
804832
finally

0 commit comments

Comments
 (0)