Repository: ansible/ansible
Pull request: Windows: Add multi-domain forest Support
##### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
replaces #65042 - as i am still learning the git workflow sorry. 
Add  multi-domain forest support for win_domain_group_membership by defining the domain of the member if needed: 

```
- name: Add a domain user/group from another Domain in the multi-domain forest to a domain group
  win_domain_group_membership:
    domain_server: DomainAAA.cloud
    name: GroupinDomainAAA
    members:
      - DomainBBB.cloud\UserInDomainBBB
    state: Present
```
Fixes #59829
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->

##### ISSUE TYPE
<!--- Pick one below and delete the rest -->
- Feature Pull Request


##### COMPONENT NAME
<!--- Write the short name of the module, plugin, task or feature below -->
win_domain_group_membership

##### ADDITIONAL INFORMATION
<!--- Include additional information to help people understand the change here -->
<!--- A step-by-step reproduction of the problem is helpful if there is no related issue -->

<!--- Paste verbatim command output below, e.g. before and after your change -->
```paste below

```


Problem statement:
win_domain_group_membership
<!--- 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
win_domain_group_membership does not support cross domain membership.
This module needs extra parameters so that both group and members have server specified.  At the moment the users and groups have to be in the same domain for the module to work!  We have a multi-domain forest and need the ability to add users to groups from the other domains

$user = Get-ADUser fredb -Server fabricam.com
$group = Get-ADGroup "Box Permanent user group" -Server contoso.com
Add-ADGroupMember -Identity $Group -Members $User -Server contoso.com

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
win_domain_group_membership

##### ANSIBLE VERSION
<!--- Paste verbatim output from "ansible --version" between quotes -->
```paste below
ansible 2.8.1
  config file = /etc/ansible/ansible.cfg
  ansible python module location = /usr/lib/python2.7/site-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.5 (default, Jun 11 2019, 12:19:05) [GCC 4.8.5 20150623 (Red Hat 4.8.5-36)]

```

##### CONFIGURATION
<!--- Paste verbatim output from "ansible-config dump --only-changed" between quotes -->
```paste below

```

##### OS / ENVIRONMENT
<!--- Provide all relevant information below, e.g. target OS versions, network device firmware, etc. -->
Target Active Directory. Host Windows Server 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 -->
```yaml
---
-
  hosts: all
  ignore_errors: true
  vars:
    basic_user_group:
      - "group1"
      - "group2"
    userId:
      - "fredb"

  tasks:
    - name: Set fact basic_user_group
      set_fact:
        box_groups: "{{ basic_user_group }}"
      when: user_profile == "basic"

    - name: Add Username to GroupName
      win_domain_group_membership:
        name: "{{ basic_user_group }}"
        domain_server: "contoso.com"
        members:
          - "{{ userId }}"
        state: present
```
<!--- HINT: You can paste gist.github.com links for larger files -->

##### EXPECTED RESULTS
<!--- Describe what you expected to happen when running the steps above -->


##### ACTUAL RESULTS
<!--- Describe what actually happened. If possible run with extra verbosity (-vvvv) -->
The full traceback is:
Cannot find an object with identity: 'System.Object[]' under: 'DC=xxx,DC=xxxx,DC=net'.
At line:51 char:19
+ $members_before = Get-AdGroupMember -Identity $name @extra_args
+                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : ObjectNotFound: (System.Object[]:ADGroup) [Get-ADGroupMember], ADIdentityNotFoundException
    + FullyQualifiedErrorId : ActiveDirectoryCmdlet:Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException,Microsoft.ActiveDirectory.Management.Commands.GetADGroupMember

ScriptStackTrace:
at <ScriptBlock>, <No file>: line 51

Microsoft.ActiveDirectory.Management.ADIdentityNotFoundException: Cannot find an object with identity: 'System.Object[]' under: 'DC=xxx,DC=xxxx,DC=net'.
   at Microsoft.ActiveDirectory.Management.Commands.ADFactoryUtil.GetObjectFromIdentitySearcher(ADObjectSearcher searcher, ADEntity identityObj, String searchRoot, AttributeSetRequest attrs, CmdletSessionInfo cmdletSessionInfo, String[]& warningMessages)
   at Microsoft.ActiveDirectory.Management.Commands.ADFactory`1.GetDirectoryObjectFromIdentity(T identityObj, String searchRoot, Boolean showDeleted)
   at Microsoft.ActiveDirectory.Management.Commands.GetADGroupMember.GetADGroupMemberProcessCSRoutine()
   at Microsoft.ActiveDirectory.Management.CmdletSubroutinePipeline.Invoke()
   at Microsoft.ActiveDirectory.Management.Commands.ADCmdletBase`1.ProcessRecord()
fatal: [xxxxxxxxxxxxxxx.xxx.xxxxxxxxxxx.xxx]: FAILED! => {
    "changed": false,
    "msg": "Unhandled exception while executing module: Cannot find an object with identity: 'System.Object[]' under: 'DC=xxx,DC=xxxx,DC=net'."
}
<!--- Paste verbatim command output between quotes -->
```paste below

```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/modules/windows/win_domain_group_membership.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_domain_group_membership.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 @if-meaton @jborean93 @jhawkesworth @jiuka @nitzmahone
[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: notify --->
Any updates please
This is IMHO not a _Bug Report_ but a _Feature Idea_. As I do not have access to a multi-domain forest i can not test or verify this. Maybe using GC could help.
This seems related to #57404 that was fixed in https://github.com/ansible/ansible/pull/58943

This was never backported to 2.8 so will only be in 2.9

you can add a request on the weekly agenda to have this backported but since it's a feature and not a bug I'm not sure it will be approved
Hi Marius,

Thanks for the update.  In my opinion this is a bug!  The module crashes
because of the cross-domain request. Every single AD that I have ever
worked has been multi-domain.  Multi-domain forests are the norm in
companies that use Microsoft products on premises.  As it stands we cannot
use the module and will have to defer to using script.  I am not sure what
GC is.  But again to have 'domain' in the title of the module is misleading.

Regards

David Piscopo

On Fri, Aug 2, 2019 at 4:08 PM Marius Rieder <notifications@github.com>
wrote:

> This is IMHO not a *Bug Report* but a *Feature Idea*. As I do not have
> access to a multi-domain forest i can not test or verify this. Maybe using
> GC could help.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/ansible/ansible/issues/59829?email_source=notifications&email_token=AA6YHU5YBXLMVH6XXTN7KO3QCPTQLA5CNFSM4IIBEZ3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3M75FA#issuecomment-517602964>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA6YHU2ZWJ4KATS7IGSUFFTQCPTQLANCNFSM4IIBEZ3A>
> .
>

Commit message: Add multi-domain forest Support
cloned extra_args so there is no check for credentials needed.
Fixed Formatting
added missing extra_args to pure state

Patch to review:
diff --git a/lib/ansible/modules/windows/win_domain_group_membership.ps1 b/lib/ansible/modules/windows/win_domain_group_membership.ps1
index 89f6c854fa1d65..c53866f61b96b6 100644
--- a/lib/ansible/modules/windows/win_domain_group_membership.ps1
+++ b/lib/ansible/modules/windows/win_domain_group_membership.ps1
@@ -39,6 +39,8 @@ if ($null -ne $domain_server) {
     $extra_args.Server = $domain_server
 }
 
+$name = Get-ADGroup $name @extra_args
+
 $result = @{
     changed = $false
     added = [System.Collections.Generic.List`1[String]]@()
@@ -52,7 +54,14 @@ $members_before = Get-AdGroupMember -Identity $name @extra_args
 $pure_members = [System.Collections.Generic.List`1[String]]@()
 
 foreach ($member in $members) {
-    $group_member = Get-ADObject -Filter "SamAccountName -eq '$member' -and $ad_object_class_filter" -Properties objectSid, sAMAccountName @extra_args
+    $extra_member_args = $extra_args.Clone()
+    if ($member -match "\\"){
+        $extra_member_args.Server = $member.Split("\")[0]
+        $member = $member.Split("\")[1]
+    } else {
+        $extra_member_args = $extra_args
+    }
+    $group_member = Get-ADObject -Filter "SamAccountName -eq '$member' -and $ad_object_class_filter" -Properties objectSid, sAMAccountName @extra_member_args
     if (!$group_member) {
         Fail-Json -obj $result "Could not find domain user, group, service account or computer named $member"
     }
@@ -70,11 +79,11 @@ foreach ($member in $members) {
     }
 
     if ($state -in @("present", "pure") -and !$user_in_group) {
-        Add-ADGroupMember -Identity $name -Members $group_member -WhatIf:$check_mode @extra_args
+        Add-ADPrincipalGroupMembership -Identity $group_member -MemberOf $name -WhatIf:$check_mode @extra_member_args
         $result.added.Add($group_member.SamAccountName)
         $result.changed = $true
     } elseif ($state -eq "absent" -and $user_in_group) {
-        Remove-ADGroupMember -Identity $name -Members $group_member -WhatIf:$check_mode @extra_args -Confirm:$False
+        Remove-ADPrincipalGroupMembership -Identity $group_member -MemberOf $name -WhatIf:$check_mode -Confirm:$False @extra_member_args
         $result.removed.Add($group_member.SamAccountName)
         $result.changed = $true
     }
@@ -94,7 +103,7 @@ if ($state -eq "pure") {
         }
 
         if ($user_to_remove) {
-            Remove-ADGroupMember -Identity $name -Members $current_member -WhatIf:$check_mode @extra_args -Confirm:$False
+            Remove-ADPrincipalGroupMembership -Identity $current_member -MemberOf $name -WhatIf:$check_mode -Confirm:$False
             $result.removed.Add($current_member.SamAccountName)
             $result.changed = $true
         }
diff --git a/lib/ansible/modules/windows/win_domain_group_membership.py b/lib/ansible/modules/windows/win_domain_group_membership.py
index ec07039875dcf4..58ee10560b5868 100644
--- a/lib/ansible/modules/windows/win_domain_group_membership.py
+++ b/lib/ansible/modules/windows/win_domain_group_membership.py
@@ -27,6 +27,7 @@
       - A list of members to ensure are present/absent from the group.
       - The given names must be a SamAccountName of a user, group, service account, or computer.
       - For computers, you must add "$" after the name; for example, to add "Mycomputer" to a group, use "Mycomputer$" as the member.
+      - If the Object is part of another domain in a multi-domain forest, you must add the domain and "\" in front of the name.
     type: list
     required: yes
   state:
@@ -91,6 +92,15 @@
     members:
       - DESKTOP$
     state: present
+
+- name: Add a domain user/group from another Domain in the multi-domain forest to a domain group
+  win_domain_group_membership:
+    domain_server: DomainAAA.cloud
+    name: GroupinDomainAAA
+    members:
+      - DomainBBB.cloud\UserInDomainBBB
+    state: Present
+
 '''
 
 RETURN = r'''


Code review comments:
diff_hunk: @@ -39,6 +39,8 @@ if ($null -ne $domain_server) {
     $extra_args.Server = $domain_server
 }
 
+$name = Get-ADGroup $name @extra_args
path: lib/ansible/modules/windows/win_domain_group_membership.ps1
text: @user1:
Probably better not to override the `$name` argument as that's an input from the module options. I would recommend you do `$group` or something to differentiate between them. Also would be nice if you added the parameter name i.e. `-Identity $name`

@author:
Changed $name to $ADGroup so it is better to distinguish between the Group Name and the Object

diff_hunk: @@ -52,7 +54,14 @@ $members_before = Get-AdGroupMember -Identity $name @extra_args
 $pure_members = [System.Collections.Generic.List`1[String]]@()
 
 foreach ($member in $members) {
-    $group_member = Get-ADObject -Filter "SamAccountName -eq '$member' -and $ad_object_class_filter" -Properties objectSid, sAMAccountName @extra_args
+    $extra_member_args = $extra_args.Clone()
+    if ($member -match "\\"){
+        $extra_member_args.Server = $member.Split("\")[0]
+        $member = $member.Split("\")[1]
+    } else {
+        $extra_member_args = $extra_args
path: lib/ansible/modules/windows/win_domain_group_membership.ps1
text: @user1:
Not needed as you have already cloned `$extra_args` to `$extra_member_args`.

@author:
i removed the part and tested it successfully

diff_hunk: @@ -27,6 +27,7 @@
       - A list of members to ensure are present/absent from the group.
       - The given names must be a SamAccountName of a user, group, service account, or computer.
       - For computers, you must add "$" after the name; for example, to add "Mycomputer" to a group, use "Mycomputer$" as the member.
+      - If the Object is part of another domain in a multi-domain forest, you must add the domain and "\" in front of the name.
path: lib/ansible/modules/windows/win_domain_group_membership.py
text: @user1:
I don't believe Object should be capitalised, might be better to change it to member object as well.

@author:
Thanks for pointing that out. changed to member object