From 3cfa71bff08564239e4fd2a4e9efed3e75054970 Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Fri, 15 Mar 2019 14:57:59 +1000 Subject: [PATCH] win_acl_inheritance - fix glob like paths (#53829) --- .../fragments/win_acl_inheritance-paths.yaml | 2 + .../modules/windows/win_acl_inheritance.ps1 | 12 +- .../win_acl_inheritance/defaults/main.yml | 2 +- .../library/test_get_acl.ps1 | 14 +- .../win_acl_inheritance/tasks/main.yml | 153 +++++++++--------- 5 files changed, 89 insertions(+), 94 deletions(-) create mode 100644 changelogs/fragments/win_acl_inheritance-paths.yaml diff --git a/changelogs/fragments/win_acl_inheritance-paths.yaml b/changelogs/fragments/win_acl_inheritance-paths.yaml new file mode 100644 index 00000000000..7cfcab663f7 --- /dev/null +++ b/changelogs/fragments/win_acl_inheritance-paths.yaml @@ -0,0 +1,2 @@ +bugfixes: +- win_acl_inheritance - Fix issues when using paths with glob like characters, e.g. ``[``, ``]`` diff --git a/lib/ansible/modules/windows/win_acl_inheritance.ps1 b/lib/ansible/modules/windows/win_acl_inheritance.ps1 index 981eb0489ed..13ea0a46fdd 100644 --- a/lib/ansible/modules/windows/win_acl_inheritance.ps1 +++ b/lib/ansible/modules/windows/win_acl_inheritance.ps1 @@ -16,12 +16,12 @@ $path = Get-AnsibleParam -obj $params "path" -type "path" -failifempty $true $state = Get-AnsibleParam -obj $params "state" -type "str" -default "absent" -validateSet "present","absent" -resultobj $result $reorganize = Get-AnsibleParam -obj $params "reorganize" -type "bool" -default $false -resultobj $result -If (-Not (Test-Path -Path $path)) { +If (-Not (Test-Path -LiteralPath $path)) { Fail-Json $result "$path file or directory does not exist on the host" } Try { - $objACL = Get-ACL -Path $path + $objACL = Get-ACL -LiteralPath $path # AreAccessRulesProtected - $false if inheritance is set ,$true if inheritance is not set $inheritanceDisabled = $objACL.AreAccessRulesProtected @@ -31,9 +31,9 @@ Try { If ($reorganize) { # it wont work without intermediate save, state would be the same - Set-ACL -Path $path -AclObject $objACL -WhatIf:$check_mode + Set-ACL -LiteralPath $path -AclObject $objACL -WhatIf:$check_mode $result.changed = $true - $objACL = Get-ACL -Path $path + $objACL = Get-ACL -LiteralPath $path # convert explicit ACE to inherited ACE ForEach($inheritedRule in $objACL.Access) { @@ -53,11 +53,11 @@ Try { } } - Set-ACL -Path $path -AclObject $objACL -WhatIf:$check_mode + Set-ACL -LiteralPath $path -AclObject $objACL -WhatIf:$check_mode $result.changed = $true } Elseif (($state -eq "absent") -And (-not $inheritanceDisabled)) { $objACL.SetAccessRuleProtection($True, $reorganize) - Set-ACL -Path $path -AclObject $objACL -WhatIf:$check_mode + Set-ACL -LiteralPath $path -AclObject $objACL -WhatIf:$check_mode $result.changed = $true } } Catch { diff --git a/test/integration/targets/win_acl_inheritance/defaults/main.yml b/test/integration/targets/win_acl_inheritance/defaults/main.yml index 325bfbe81d3..138063f4fe8 100644 --- a/test/integration/targets/win_acl_inheritance/defaults/main.yml +++ b/test/integration/targets/win_acl_inheritance/defaults/main.yml @@ -1 +1 @@ -test_win_acl_inheritance_path: C:\ansible\win_acl_inheritance +test_win_acl_inheritance_path: C:\ansible\win_acl_inheritance .ÅÑŚÌβŁÈ [$!@^&test(;)] diff --git a/test/integration/targets/win_acl_inheritance/library/test_get_acl.ps1 b/test/integration/targets/win_acl_inheritance/library/test_get_acl.ps1 index 6c73f96c0a9..c75d33ead85 100644 --- a/test/integration/targets/win_acl_inheritance/library/test_get_acl.ps1 +++ b/test/integration/targets/win_acl_inheritance/library/test_get_acl.ps1 @@ -13,29 +13,21 @@ $result = @{ changed = $false } -$acl = Get-Acl -Path $path +$acl = Get-Acl -LiteralPath $path $result.inherited = $acl.AreAccessRulesProtected -eq $false $user_details = @{} $acl.Access | ForEach-Object { - # Backslashes are the bane of my existance, convert to / to we can export to JSON - $user = $_.IdentityReference -replace '\\','/' + $user = $_.IdentityReference.Translate([System.Security.Principal.SecurityIdentifier]).Value if ($user_details.ContainsKey($user)) { $details = $user_details.$user } else { $details = @{ isinherited = $false - isnotinherited = $false } } - - if ($_.IsInherited) { - $details.isinherited = $true - } else { - $details.isnotinherited = $true - } - + $details.isinherited = $_.IsInherited $user_details.$user = $details } diff --git a/test/integration/targets/win_acl_inheritance/tasks/main.yml b/test/integration/targets/win_acl_inheritance/tasks/main.yml index d1fadc43a76..9b613276a39 100644 --- a/test/integration/targets/win_acl_inheritance/tasks/main.yml +++ b/test/integration/targets/win_acl_inheritance/tasks/main.yml @@ -1,24 +1,65 @@ --- - # Test setup -- name: remove test folder for baseline - win_file: - path: '{{test_win_acl_inheritance_path}}' - state: absent - +# Test setup +# Use single task to save in CI runtime - name: create test folders - win_file: - path: '{{test_win_acl_inheritance_path}}\folder' - state: directory + win_shell: | + $ErrorActionPreference = 'Stop' -- name: create test files - win_copy: - dest: '{{test_win_acl_inheritance_path}}\folder\file.txt' - content: a + $tmp_dir = '{{ test_win_acl_inheritance_path }}' + if (Test-Path -LiteralPath $tmp_dir) { + Remove-Item -LiteralPath $tmp_dir -Force -Recurse + } + New-Item -Path $tmp_dir -ItemType Directory > $null + + Add-Type -AssemblyName System.DirectoryServices.AccountManagement + $current_sid = ([System.DirectoryServices.AccountManagement.UserPrincipal]::Current).Sid + $system_sid = New-Object -TypeName System.Security.Principal.SecurityIdentifier -ArgumentList @([System.Security.Principal.WellKnownSidType]::LocalSystemSid, $null) + $everyone_sid = New-Object -TypeName System.Security.Principal.SecurityIdentifier -ArgumentList @([System.Security.Principal.WellKnownSidType]::WorldSid, $null) + + $sd = New-Object -TypeName System.Security.AccessControl.DirectorySecurity + $sd.SetAccessRuleProtection($true, $false) + $sd.AddAccessRule( + (New-Object -TypeName System.Security.AccessControl.FileSystemAccessRule -ArgumentList @( + $system_sid, + [System.Security.AccessControl.FileSystemRights]::FullControl, + [System.Security.AccessControl.InheritanceFlags]"ContainerInherit, ObjectInherit", + [System.Security.AccessControl.PropagationFlags]::None, + [System.Security.AccessControl.AccessControlType]::Allow + )) + ) + $sd.AddAccessRule( + (New-Object -TypeName System.Security.AccessControl.FileSystemAccessRule -ArgumentList @( + $current_sid, + [System.Security.AccessControl.FileSystemRights]::FullControl, + [System.Security.AccessControl.InheritanceFlags]"ContainerInherit, ObjectInherit", + [System.Security.AccessControl.PropagationFlags]::None, + [System.Security.AccessControl.AccessControlType]::Allow + )) + ) + $sd.AddAccessRule( + (New-Object -TypeName System.Security.AccessControl.FileSystemAccessRule -ArgumentList @( + $everyone_sid, + [System.Security.AccessControl.FileSystemRights]::Read, + [System.Security.AccessControl.InheritanceFlags]"ContainerInherit, ObjectInherit", + [System.Security.AccessControl.PropagationFlags]::None, + [System.Security.AccessControl.AccessControlType]::Allow + )) + ) + + Set-Acl -LiteralPath $tmp_dir -AclObject $sd + + New-Item -Path "$tmp_dir\folder" -ItemType Directory > $null + Set-Content -LiteralPath "$tmp_dir\folder\file.txt" -Value 'a' + + $system_sid.Value + $current_sid.Value + $everyone_sid.Value + register: test_sids # register the output SID values used for comparison tests below # Run tests - name: remove inheritance check win_acl_inheritance: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' reorganize: True state: absent register: remove_check @@ -26,7 +67,7 @@ - name: get actual remove inheritance check test_get_acl: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' register: actual_remove_check - name: assert remove inheritance check @@ -34,17 +75,20 @@ that: - remove_check is changed - actual_remove_check.inherited == True + - actual_remove_check.user_details[test_sids.stdout_lines[0]].isinherited == True + - actual_remove_check.user_details[test_sids.stdout_lines[1]].isinherited == True + - actual_remove_check.user_details[test_sids.stdout_lines[2]].isinherited == True - name: remove inheritance win_acl_inheritance: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' reorganize: True state: absent register: remove - name: get actual remove inheritance test_get_acl: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' register: actual_remove - name: assert remove inheritance @@ -52,44 +96,25 @@ that: - remove is changed - actual_remove.inherited == False - - actual_remove.user_details['BUILTIN/Administrators'].isinherited == False - - actual_remove.user_details['BUILTIN/Administrators'].isnotinherited == True - - actual_remove.user_details['BUILTIN/Users'].isinherited == False - - actual_remove.user_details['BUILTIN/Users'].isnotinherited == True - - actual_remove.user_details['CREATOR OWNER'].isinherited == False - - actual_remove.user_details['CREATOR OWNER'].isnotinherited == True - - actual_remove.user_details['NT AUTHORITY/SYSTEM'].isinherited == False - - actual_remove.user_details['NT AUTHORITY/SYSTEM'].isnotinherited == True + - actual_remove.user_details[test_sids.stdout_lines[0]].isinherited == False + - actual_remove.user_details[test_sids.stdout_lines[1]].isinherited == False + - actual_remove.user_details[test_sids.stdout_lines[2]].isinherited == False - name: remove inheritance again win_acl_inheritance: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' reorganize: True state: absent register: remove_again -- name: get actual remove inheritance again - test_get_acl: - path: '{{test_win_acl_inheritance_path}}\folder' - register: actual_remove_again - - name: assert remove inheritance again assert: that: - remove_again is not changed - - actual_remove_again.inherited == False - - actual_remove.user_details['BUILTIN/Administrators'].isinherited == False - - actual_remove.user_details['BUILTIN/Administrators'].isnotinherited == True - - actual_remove.user_details['BUILTIN/Users'].isinherited == False - - actual_remove.user_details['BUILTIN/Users'].isnotinherited == True - - actual_remove.user_details['CREATOR OWNER'].isinherited == False - - actual_remove.user_details['CREATOR OWNER'].isnotinherited == True - - actual_remove.user_details['NT AUTHORITY/SYSTEM'].isinherited == False - - actual_remove.user_details['NT AUTHORITY/SYSTEM'].isnotinherited == True - name: add inheritance check win_acl_inheritance: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' reorganize: True state: present register: add_check @@ -97,7 +122,7 @@ - name: get actual add inheritance check test_get_acl: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' register: actual_add_check - name: assert add inheritance check @@ -105,25 +130,20 @@ that: - add_check is changed - actual_add_check.inherited == False - - actual_add_check.user_details['BUILTIN/Administrators'].isinherited == False - - actual_add_check.user_details['BUILTIN/Administrators'].isnotinherited == True - - actual_add_check.user_details['BUILTIN/Users'].isinherited == False - - actual_add_check.user_details['BUILTIN/Users'].isnotinherited == True - - actual_add_check.user_details['CREATOR OWNER'].isinherited == False - - actual_add_check.user_details['CREATOR OWNER'].isnotinherited == True - - actual_add_check.user_details['NT AUTHORITY/SYSTEM'].isinherited == False - - actual_add_check.user_details['NT AUTHORITY/SYSTEM'].isnotinherited == True + - actual_add_check.user_details[test_sids.stdout_lines[0]].isinherited == False + - actual_add_check.user_details[test_sids.stdout_lines[1]].isinherited == False + - actual_add_check.user_details[test_sids.stdout_lines[2]].isinherited == False - name: add inheritance win_acl_inheritance: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' reorganize: True state: present register: add - name: get actual add inheritance test_get_acl: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' register: actual_add - name: assert add inheritance @@ -131,43 +151,24 @@ that: - add is changed - actual_add.inherited == True - - actual_add.user_details['BUILTIN/Administrators'].isinherited == True - - actual_add.user_details['BUILTIN/Administrators'].isnotinherited == False - - actual_add.user_details['BUILTIN/Users'].isinherited == True - - actual_add.user_details['BUILTIN/Users'].isnotinherited == True # Bug in win_acl_inheritance, resetting inheritance doubles up entries - - actual_add.user_details['CREATOR OWNER'].isinherited == True - - actual_add.user_details['CREATOR OWNER'].isnotinherited == False - - actual_add.user_details['NT AUTHORITY/SYSTEM'].isinherited == True - - actual_add.user_details['NT AUTHORITY/SYSTEM'].isnotinherited == False + - actual_add.user_details[test_sids.stdout_lines[0]].isinherited == True + - actual_add.user_details[test_sids.stdout_lines[1]].isinherited == True + - actual_add.user_details[test_sids.stdout_lines[2]].isinherited == True - name: add inheritance again win_acl_inheritance: - path: '{{test_win_acl_inheritance_path}}\folder' + path: '{{ test_win_acl_inheritance_path }}\folder' reorganize: True state: present register: add_again -- name: get actual add inheritance again - test_get_acl: - path: '{{test_win_acl_inheritance_path}}\folder' - register: actual_add_again - - name: assert add inheritance again assert: that: - add_again is not changed - - actual_add_again.inherited == True - - actual_add_again.user_details['BUILTIN/Administrators'].isinherited == True - - actual_add_again.user_details['BUILTIN/Administrators'].isnotinherited == False - - actual_add_again.user_details['BUILTIN/Users'].isinherited == True - - actual_add_again.user_details['BUILTIN/Users'].isnotinherited == True # Bug in win_acl_inheritance, resetting inheritance doubles up entries - - actual_add_again.user_details['CREATOR OWNER'].isinherited == True - - actual_add_again.user_details['CREATOR OWNER'].isnotinherited == False - - actual_add_again.user_details['NT AUTHORITY/SYSTEM'].isinherited == True - - actual_add_again.user_details['NT AUTHORITY/SYSTEM'].isnotinherited == False # Test cleanup - name: remove test folder win_file: - path: '{{test_win_acl_inheritance_path}}' + path: '{{ test_win_acl_inheritance_path }}' state: absent