Repository: ansible/ansible
Pull request: win_share - Implement append paramtere for access rules
##### SUMMARY
Fixes: #59237

##### ISSUE TYPE
- Feature Pull Request

##### COMPONENT NAME
win_share.py
win_share.ps1

##### ADDITIONAL INFORMATION


Problem statement:
win_share: remove all other permissions of a share
<!--- Verify first that your issue is not already reported on GitHub -->
<!--- Also test if the latest release and devel branch are affected too -->
<!--- Complete *all* sections as described, this form is processed automatically -->

##### SUMMARY
I have a playbook the user: App_Urb on a share EAI. Everything is ok, but If I have another user on that share, playbook remove it

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
win_share

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```
ansible 2.8.1
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Jul 13 2018, 13:06:57) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]

```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```
[14:07:41]root@vsrvkermit playbook]# ansible-config dump --only-changed
DEFAULT_ROLES_PATH(/etc/ansible/ansible.cfg) = [u'/etc/ansible/roles', u'/datas/ansible/roles']

```

##### OS / ENVIRONMENT
Target is Windows 2012 R2

##### STEPS TO REPRODUCE
<!--- Describe exactly how to reproduce the problem, using a minimal test-case -->

<!--- Paste example playbooks or commands between quotes below -->
Case original:
```
C:\Windows\system32>net share EAI
Share name        EAI
Path              D:\Partage\EAI
Remark            Repertoire EAI
Maximum users     No limit
Users
Caching           Manual caching of documents
Permission        VSRVQSRM\Appli_Urba, FULL
                  PAM\EXP_TRT, READ
```
I manually added PAM\EXP_TRT with READ rules.

I run playbook like this:
```
- name: Add share EAI
  win_share:
    name: EAI
    description: Repertoire EAI
    path: D:\Partage\EAI
    list: no
    full: Appli_Urba
  when: ansible_distribution_version is version('6.2','>=')

```
This remove the created user

<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->
Keep original user, just add Appli_Urb if not exist

##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->

<!--- Paste verbatim command output between quotes -->
```
C:\Windows\system32>net share EAI
Share name        EAI
Path              D:\Partage\EAI
Remark            Repertoire EAI
Maximum users     No limit
Users
Caching           Manual caching of documents
Permission        VSRVQSRM\Appli_Urba, FULL
```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/windows/win_share.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_share.py)

If these files are inaccurate, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
cc @ShachafGoldstein @daBONDi @if-meaton @jborean93 @jhawkesworth @nitzmahone
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
I think this is just 'desired state' behaviour.  Think of the win_share module as describing what permissions are required on the share.  So you must tell it all the permissions you need.

You would need to modify playbook like this:

```yaml
- name: Add share EAI
  win_share:
    name: EAI
    description: Repertoire EAI
    path: D:\Partage\EAI
    list: no
    full: Appli_Urba
    read: PAM\EXP_TRT
  when: ansible_distribution_version is version('6.2','>=')
```

I think behaving like it does is desirable because the ansible configuration becomes 'single point of truth' regarding how exactly the share should be configured.  Obviously this means that you must capture all the permissions you need to set up in your playbook, not just new ones you wish to add.

The problem is PAM\EXP_TRT is a specifical rule needed on only 1 or 2 server. It won't be managed by Ansible.
Appli_Urba is a rule for ALL servers, so managed by Ansible

Is there any way to keep original rules ?
I can make a patch to add append to the module but it would take some time
you can use pure Powershell in the meantime to just add the permissions
You could also define host var or group var so that the correct settings are applied to the correct hosts.

Personally I'm not in favour of adding 'append' type options to modules but its true they exist for other modules.
Hello, the problem to me is this rule is a single rule (that can be managed for some projects). It's not an automation, and Single case don't have to be managed by automation system. I think **append** option could be a right solution, and make  **replace** option by default to don't break actual playbooks.

Commit message: Forgot the actual new parameter in the test

Patch to review:
diff --git a/changelogs/fragments/win_share-Implement-append-paramtere-for-access-rules.yml b/changelogs/fragments/win_share-Implement-append-paramtere-for-access-rules.yml
new file mode 100644
index 00000000000000..84405c18265879
--- /dev/null
+++ b/changelogs/fragments/win_share-Implement-append-paramtere-for-access-rules.yml
@@ -0,0 +1,2 @@
+minor_changes:
+  - "win_share - Implement append paramtere for access rules (https://github.com/ansible/ansible/issues/59237)"
\ No newline at end of file
diff --git a/lib/ansible/modules/windows/win_share.ps1 b/lib/ansible/modules/windows/win_share.ps1
index eadce970c0764b..eb9ba03d4bf771 100644
--- a/lib/ansible/modules/windows/win_share.ps1
+++ b/lib/ansible/modules/windows/win_share.ps1
@@ -47,6 +47,7 @@ $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "b
 
 $name = Get-AnsibleParam -obj $params -name "name" -type "str" -failifempty $true
 $state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "present" -validateset "present","absent"
+$appendRules = Get-AnsibleParam -obj $params -name "appendRules" -type "bool" -default $false
 
 if (-not (Get-Command -Name Get-SmbShare -ErrorAction SilentlyContinue)) {
     Fail-Json $result "The current host does not support the -SmbShare cmdlets required by this module. Please run on Server 2012 or Windows 8 and later"
@@ -151,65 +152,67 @@ If ($state -eq "absent") {
 
     # remove permissions
     $permissions = Get-SmbShareAccess -Name $name
-    ForEach ($permission in $permissions) {
-        If ($permission.AccessControlType -eq "Deny") {
-            $cim_count = 0
-            foreach ($count in $permissions) {
-                $cim_count++
-            }
-            # Don't remove the Deny entry for Everyone if there are no other permissions set (cim_count == 1)
-            if (-not ($permission.AccountName -eq 'Everyone' -and $cim_count -eq 1)) {
-                If (-not ($permissionDeny.Contains($permission.AccountName))) {
-                    if (-not $check_mode) {
-                        Unblock-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
-                    }
-                    $result.changed = $true
-                    $result.actions += "Unblock-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
-                } else {
-                    # Remove from the deny list as it already has the permissions
-                    $permissionDeny.remove($permission.AccountName) | Out-Null
+    if(-not $appendRules) {
+        ForEach ($permission in $permissions) {
+            If ($permission.AccessControlType -eq "Deny") {
+                $cim_count = 0
+                foreach ($count in $permissions) {
+                    $cim_count++
                 }
-            }
-        } ElseIf ($permission.AccessControlType -eq "Allow") {
-            If ($permission.AccessRight -eq "Full") {
-                If (-not ($permissionFull.Contains($permission.AccountName))) {
-                    if (-not $check_mode) {
-                        Revoke-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
+                # Don't remove the Deny entry for Everyone if there are no other permissions set (cim_count == 1)
+                if (-not ($permission.AccountName -eq 'Everyone' -and $cim_count -eq 1)) {
+                    If (-not ($permissionDeny.Contains($permission.AccountName))) {
+                        if (-not $check_mode) {
+                            Unblock-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
+                        }
+                        $result.changed = $true
+                        $result.actions += "Unblock-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
+                    } else {
+                        # Remove from the deny list as it already has the permissions
+                        $permissionDeny.remove($permission.AccountName) | Out-Null
                     }
-                    $result.changed = $true
-                    $result.actions += "Revoke-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
-
-                    Continue
                 }
-
-                # user got requested permissions
-                $permissionFull.remove($permission.AccountName) | Out-Null
-            } ElseIf ($permission.AccessRight -eq "Change") {
-                If (-not ($permissionChange.Contains($permission.AccountName))) {
-                    if (-not $check_mode) {
-                        Revoke-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
+            } ElseIf ($permission.AccessControlType -eq "Allow") {
+                If ($permission.AccessRight -eq "Full") {
+                    If (-not ($permissionFull.Contains($permission.AccountName))) {
+                        if (-not $check_mode) {
+                            Revoke-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
+                        }
+                        $result.changed = $true
+                        $result.actions += "Revoke-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
+
+                        Continue
                     }
-                    $result.changed = $true
-                    $result.actions += "Revoke-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
 
-                    Continue
-                }
+                    # user got requested permissions
+                    $permissionFull.remove($permission.AccountName) | Out-Null
+                } ElseIf ($permission.AccessRight -eq "Change") {
+                    If (-not ($permissionChange.Contains($permission.AccountName))) {
+                        if (-not $check_mode) {
+                            Revoke-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
+                        }
+                        $result.changed = $true
+                        $result.actions += "Revoke-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
+
+                        Continue
+                    }
 
-                # user got requested permissions
-                $permissionChange.remove($permission.AccountName) | Out-Null
-            } ElseIf ($permission.AccessRight -eq "Read") {
-                If (-not ($permissionRead.Contains($permission.AccountName))) {
-                    if (-not $check_mode) {
-                        Revoke-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
+                    # user got requested permissions
+                    $permissionChange.remove($permission.AccountName) | Out-Null
+                } ElseIf ($permission.AccessRight -eq "Read") {
+                    If (-not ($permissionRead.Contains($permission.AccountName))) {
+                        if (-not $check_mode) {
+                            Revoke-SmbShareAccess -Force -Name $name -AccountName $permission.AccountName | Out-Null
+                        }
+                        $result.changed = $true
+                        $result.actions += "Revoke-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
+
+                        Continue
                     }
-                    $result.changed = $true
-                    $result.actions += "Revoke-SmbShareAccess -Force -Name $name -AccountName $($permission.AccountName)"
 
-                    Continue
+                    # user got requested permissions
+                    $permissionRead.Remove($permission.AccountName) | Out-Null
                 }
-
-                # user got requested permissions
-                $permissionRead.Remove($permission.AccountName) | Out-Null
             }
         }
     }
diff --git a/lib/ansible/modules/windows/win_share.py b/lib/ansible/modules/windows/win_share.py
index 0a730f7879c47b..f0aff7b5565c8f 100644
--- a/lib/ansible/modules/windows/win_share.py
+++ b/lib/ansible/modules/windows/win_share.py
@@ -75,6 +75,11 @@
     type: bool
     default: no
     version_added: "2.4"
+  appendRules:
+    description: Sets whether to add or replace access control entries
+    type: bool
+    default: no
+    version_added: "2.9"
 author:
   - Hans-Joachim Kliemeck (@h0nIg)
   - David Baumann (@daBONDi)
diff --git a/test/integration/targets/win_share/tasks/tests.yml b/test/integration/targets/win_share/tasks/tests.yml
index c120491b28c67e..5608c62bd8073d 100644
--- a/test/integration/targets/win_share/tasks/tests.yml
+++ b/test/integration/targets/win_share/tasks/tests.yml
@@ -398,6 +398,30 @@
     - set_acl_actual_again.stdout_lines[2] == 'Change|Allow|BUILTIN\\Users'
     - set_acl_actual_again.stdout_lines[3] == 'Full|Allow|BUILTIN\\Administrators'
 
+- name: Append ACLs on share
+  win_share:
+    name: "{{test_win_share_name}}"
+    state: present
+    path: "{{test_win_share_path}}"
+    change: Remote Desktop Users
+    appendRules: yes
+  register: append_acl
+
+- name: get actual share ACLs
+  win_shell: foreach ($acl in Get-SmbShareAccess -Name '{{test_win_share_name}}') { Write-Host "$($acl.AccessRight)|$($acl.AccessControlType)|$($acl.AccountName)" }
+  register: append_acl_actual
+
+- name: assert set ACLs on share again
+  assert:
+    that:
+    - append_acl is changed
+    - append_acl_actual.stdout_lines|length == 5
+    - append_acl_actual.stdout_lines[0] == 'Full|Deny|BUILTIN\Remote Desktop Users'
+    - append_acl_actual.stdout_lines[1] == 'Read|Allow|BUILTIN\\Guests'
+    - append_acl_actual.stdout_lines[2] == 'Change|Allow|BUILTIN\\Users'
+    - append_acl_actual.stdout_lines[3] == 'Full|Allow|BUILTIN\\Administrators'
+    - append_acl_actual.stdout_lines[4] == 'Change|Allow|BUILTIN\\Remote Desktop Users'
+
 - name: remove share check
   win_share:
     name: "{{test_win_share_name}}"


Code review comments:
diff_hunk: @@ -0,0 +1,2 @@
+minor_changes:
+  - "win_share - Implement append paramtere for access rules (https://github.com/ansible/ansible/issues/59237)"
path: changelogs/fragments/win_share-Implement-append-paramtere-for-access-rules.yml
text: @user1:
parameter*

diff_hunk: @@ -47,6 +47,7 @@ $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "b
 
 $name = Get-AnsibleParam -obj $params -name "name" -type "str" -failifempty $true
 $state = Get-AnsibleParam -obj $params -name "state" -type "str" -default "present" -validateset "present","absent"
+$appendRules = Get-AnsibleParam -obj $params -name "appendRules" -type "bool" -default $false
path: lib/ansible/modules/windows/win_share.ps1
text: @user1:
We seem to have settled on using `action` as an option name for similar modules that need to add, remove, or set from a list. I would suggest we use `rule_action` with those 3 options and default to `set` to ensure backwards compatibility. You don't need to implement the `remove` if you don't want to but having `set` and `add` should satisfy the feature request.

@author:
done