group - Fix setting members on builtin group (#136)

Implement fix for when trying to set group members on a group that is
Builtin to the domain controller. This reverts the logic back to how it
was before v1.6.0 so that setting these members continues to work.

changelogs/fragments/130-group-member.yml

@@ -0,0 +1,4 @@
+bugfixes:
+  - >-
+    microsoft.ad.group - Fix setting group members of Builtin groups of a domain controller -
+    https://github.com/ansible-collections/microsoft.ad/issues/130

plugins/module_utils/_ADObject.psm1

@@ -769,6 +769,7 @@ Function Invoke-AnsibleADObject {
         StateRequired - Set to 'present' or 'absent' if this needs to be defined for either state
         DNLookup - Whether each value needs to be looked up to get the DN
         IsRawAttribute - Whether the attribute is a raw LDAP attribute name and not a parameter name
+        SupportsReplace - Whether the IsRawAttribute supports setting through -Replace or needs -Add/-Remove
         New - Called when the option is to be set on the New-AD* cmdlet splat
         Set - Called when the option is to be set on the Set-AD* cmdlet splat
 
@@ -998,6 +999,11 @@ Function Invoke-AnsibleADObject {
                 $option.type = 'raw'
             }
 
+            if (-not $propInfo.PSObject.Properties.Match('SupportsReplace')) {
+                $propInfo.PSObject.Properties.Add(
+                    [System.Management.Automation.PSNoteProperty]::new('SupportsReplace', $true))
+            }
+
             $spec.options[$ansibleOption] = $option
 
             if ($propInfo.Attribute) {
@@ -1362,10 +1368,25 @@ Function Invoke-AnsibleADObject {
                     if ($res.Changed) {
                         if ($propInfo.IsRawAttribute) {
                             if ($newValue) {
-                                if (-not $setParams.ContainsKey('Replace')) {
-                                    $setParams['Replace'] = @{}
+                                $toSet = @(
+                                    if ($propInfo.SupportsReplace) {
+                                        @{ Prop = 'Replace'; Value = $newValue }
+                                    }
+                                    else {
+                                        @{ Prop = 'Add'; Value = $res.ToAdd }
+                                        @{ Prop = 'Remove'; Value = $res.ToRemove }
+                                    }
+                                )
+
+                                foreach ($setInfo in $toSet) {
+                                    if (-not $setInfo.Value) {
+                                        continue
+                                    }
+                                    if (-not $setParams.ContainsKey($setInfo.Prop)) {
+                                        $setParams[$setInfo.Prop] = @{}
+                                    }
+                                    $setParams[$setInfo.Prop][$propInfo.Attribute] = $setInfo.Value
                                 }
-                                $setParams['Replace'][$propInfo.Attribute] = $newValue
                             }
                             else {
                                 if (-not $setParams.ContainsKey('Clear')) {

plugins/modules/group.ps1

@@ -34,6 +34,11 @@ $setParams = @{
             Attribute = 'member'
             DNLookup = $true
             IsRawAttribute = $true
+            # If the group is part of the CN=Builtin groups, it cannot
+            # use -Replace. This ensures it always uses -Add/-Remove when
+            # setting a changed value to handle this.
+            # https://github.com/ansible-collections/microsoft.ad/issues/130
+            SupportsReplace = $false
         }
         [PSCustomObject]@{
             Name = 'sam_account_name'

tests/integration/targets/group/tasks/tests.yml

@@ -67,6 +67,50 @@
     that:
     - not create_group_again is changed
 
+- name: add member to builtin group
+  group:
+    name: Administrators
+    path: CN=Builtin,{{ setup_domain_info.output[0].defaultNamingContext }}
+    members:
+      add:
+      - '{{ create_group.distinguished_name }}'
+  register: add_member_to_builtin
+
+- name: get result of add member to builtin group
+  object_info:
+    identity: '{{ add_member_to_builtin.object_guid }}'
+    properties:
+    - member
+  register: add_member_to_builtin_actual
+
+- name: assert add member to builtin group
+  assert:
+    that:
+    - add_member_to_builtin is changed
+    - create_group.distinguished_name in add_member_to_builtin_actual.objects[0].member
+
+- name: remove member on builtin group
+  group:
+    name: Administrators
+    path: CN=Builtin,{{ setup_domain_info.output[0].defaultNamingContext }}
+    members:
+      remove:
+      - '{{ create_group.distinguished_name }}'
+  register: remove_member_from_builtin
+
+- name: get result of add member to builtin group
+  object_info:
+    identity: '{{ add_member_to_builtin.object_guid }}'
+    properties:
+    - member
+  register: remove_member_from_builtin_actual
+
+- name: assert remove member to builtin group
+  assert:
+    that:
+    - remove_member_from_builtin is changed
+    - create_group.distinguished_name not in remove_member_from_builtin_actual.objects[0].member
+
 - name: create ou to store group members
   ou:
     name: MyOU