group: Fix nonposix, posix and external handling and conversions

The nonposix, posix and external parameters need to be mutually
exclusive. external was missing in this list. Only one of the three
parameters can be used.

external can not be set to no/false. This results in an error now.

if nonposix is used, posix is set as not nonposix. The nonposix
parameter is not used within the code anymore..

New tests have been added to tests the addition of users with for
nonposix and posix groups. The tests for the external group is not
active due to the need of an AD.

Fixes: #528 (Error creating nonposix group)

README-group.md

@@ -157,9 +157,9 @@ Variable | Description | Required
 `name` \| `cn` | The list of group name strings. | no
 `description` | The group description string. | no
 `gid` \| `gidnumber` | The GID integer. | no
-`posix` | Create a non-POSIX group or change a non-POSIX to a posix group. (bool) | no
+`posix` | Create a non-POSIX group or change a non-POSIX to a posix group. `nonposix`, `posix` and `external` are mutually exclusive. (bool) | no
-`nonposix` | Create as a non-POSIX group. (bool) | no
+`nonposix` | Create as a non-POSIX group. `nonposix`, `posix` and `external` are mutually exclusive. (bool) | no
-`external` | Allow adding external non-IPA members from trusted domains. (bool) | no
+`external` | Allow adding external non-IPA members from trusted domains. `nonposix`, `posix` and `external` are mutually exclusive. (bool) | no
 `nomembers` | Suppress processing of membership attributes. (bool) | no
 `user` | List of user name strings assigned to this group. | no
 `group` | List of group name strings assigned to this group. | no

plugins/modules/ipagroup.py

@@ -232,43 +232,25 @@ def is_external_group(res_find):
 
 
 def is_posix_group(res_find):
-    """Verify if the result group is an external group."""
+    """Verify if the result group is an posix group."""
     return res_find and 'posixgroup' in res_find['objectclass']
 
 
-def check_objectclass_args(module, res_find, nonposix, posix, external):
+def check_objectclass_args(module, res_find, posix, external):
+    # Only a nonposix group can be changed to posix or external
+
+    # A posix group can not be changed to nonposix or external
     if is_posix_group(res_find):
-        if (
+        if external is not None and external or posix is False:
-            (posix is not None and posix is False)
-            or nonposix
-            or external
-        ):
             module.fail_json(
-                msg="Cannot change `POSIX` status of a group "
+                msg="Cannot change `posix` group to `non-posix` or "
-                    "to `non-POSIX` or `external`.")
+                "`external`.")
-    # Can't change an existing external group
+    # An external group can not be changed to nonposix or posix or nonexternal
     if is_external_group(res_find):
-        if (
+        if external is False or posix is not None:
-            posix
-            or (nonposix is not None and nonposix is False)
-            or (external is not None and external is False)
-        ):
             module.fail_json(
-                msg="Cannot change `external` status of group "
+                msg="Cannot change `external` group to `posix` or "
-                    "to `POSIX` or `non-external`.")
+                "`non-posix`.")
-
-
-def should_modify_group(module, res_find, args, nonposix, posix, external):
-    if not compare_args_ipa(module, args, res_find):
-        return True
-    if any([posix, nonposix]):
-        set_posix = posix or (nonposix is not None and not nonposix)
-        if set_posix and not is_posix_group(res_find):
-            return True
-    if not is_external_group(res_find) and external:
-        if not is_posix_group(res_find):
-            return True
-    return False
 
 
 def main():
@@ -301,7 +283,9 @@ def main():
             state=dict(type="str", default="present",
                        choices=["present", "absent"]),
         ),
-        mutually_exclusive=[['posix', 'nonposix']],
+        # It does not make sense to set posix, nonposix or external at the
+        # same time
+        mutually_exclusive=[['posix', 'nonposix', 'external']],
         supports_check_mode=True,
     )
 
@@ -358,11 +342,19 @@ def main():
                     msg="Argument '%s' can not be used with state '%s'" %
                     (x, state))
 
+    if external is False:
+        ansible_module.fail_json(
+            msg="group can not be non-external")
+
     # Init
 
     changed = False
     exit_args = {}
 
+    # If nonposix is used, set posix as not nonposix
+    if nonposix is not None:
+        posix = not nonposix
+
     # Connect to IPA API
     with ansible_module.ipa_connect():
 
@@ -391,8 +383,8 @@ def main():
             # Create command
             if state == "present":
                 # Can't change an existing posix group
-                check_objectclass_args(ansible_module, res_find, nonposix,
+                check_objectclass_args(ansible_module, res_find, posix,
-                                       posix, external)
+                                       external)
 
                 # Generate args
                 args = gen_args(description, gid, nomembers)
@@ -400,21 +392,25 @@ def main():
                 if action == "group":
                     # Found the group
                     if res_find is not None:
-                        # For all settings is args, check if there are
+                        # For all settings in args, check if there are
                         # different settings in the find result.
                         # If yes: modify
-                        if should_modify_group(ansible_module, res_find, args,
+                        # Also if it is a modification from nonposix to posix
-                                               nonposix, posix, external):
+                        # or nonposix to external.
-                            if (
+                        if not compare_args_ipa(ansible_module, args,
-                                posix
+                                                res_find) or \
-                                or (nonposix is not None and not nonposix)
+                           (
-                            ):
+                               not is_posix_group(res_find) and
+                               not is_external_group(res_find) and
+                               (posix or external)
+                           ):
+                            if posix:
                                 args['posix'] = True
                             if external:
                                 args['external'] = True
                             commands.append([name, "group_mod", args])
                     else:
-                        if nonposix or (posix is not None and not posix):
+                        if posix is not None and not posix:
                             args['nonposix'] = True
                         if external:
                             args['external'] = True

tests/group/test_group_external_nonposix.yml

@@ -14,6 +14,28 @@
         - posixgroup
         state: absent
 
+    - name: Ensure test users testuser1, testuser2 and testuser3 are absent
+      ipauser:
+        ipaadmin_password: SomeADMINpassword
+        name: testuser1,testuser2,testuser3
+        state: absent
+
+    - name: Ensure test users testuser1..testuser3 are present
+      ipauser:
+        ipaadmin_password: SomeADMINpassword
+        users:
+        - name: testuser1
+          first: testuser1
+          last: Last
+        - name: testuser2
+          first: testuser2
+          last: Last
+        - name: testuser3
+          first: testuser3
+          last: Last
+      register: result
+      failed_when: not result.changed or result.failed
+
     - name: Add nonposix group.
       ipagroup:
         ipaadmin_password: SomeADMINpassword
@@ -52,7 +74,7 @@
         name: extgroup
         external: no
       register: result
-      failed_when: not result.failed or "Cannot change `external` status of group" not in result.msg
+      failed_when: not result.failed or "group can not be non-external" not in result.msg
 
     - name: Set external group to be posix.
       ipagroup:
@@ -60,7 +82,7 @@
         name: extgroup
         posix: yes
       register: result
-      failed_when: not result.failed or "Cannot change `external` status of group" not in result.msg
+      failed_when: not result.failed or "Cannot change `external` group" not in result.msg
 
     - name: Add nonposix group.
       ipagroup:
@@ -92,23 +114,23 @@
         name: posixgroup
         external: yes
       register: result
-      failed_when: not result.failed or "Cannot change `POSIX` status of a group" not in result.msg
+      failed_when: not result.failed or "Cannot change `posix` group" not in result.msg
 
-    - name: Set posix group to be non-POSIX.
+    - name: Set posix group to be non-posix.
       ipagroup:
         ipaadmin_password: SomeADMINpassword
         name: posixgroup
         posix: no
       register: result
-      failed_when: not result.failed or "Cannot change `POSIX` status of a group" not in result.msg
+      failed_when: not result.failed or "Cannot change `posix` group" not in result.msg
 
-    - name: Set posix group to be non-POSIX.
+    - name: Set posix group to be non-posix.
       ipagroup:
         ipaadmin_password: SomeADMINpassword
         name: posixgroup
         nonposix: yes
       register: result
-      failed_when: not result.failed or "Cannot change `POSIX` status of a group" not in result.msg
+      failed_when: not result.failed or "Cannot change `posix` group" not in result.msg
 
     - name: Add nonposix group.
       ipagroup:
@@ -126,8 +148,159 @@
       register: result
       failed_when: result.failed or result.changed
 
+
+    # NONPOSIX MEMBER TEST
+
+    - name: Ensure users testuser1, testuser2 and testuser3 are present in group nonposixgroup
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: nonposixgroup
+        nonposix: yes
+        user:
+        - testuser1
+        - testuser2
+        - testuser3
+      register: result
+      failed_when: not result.changed or result.failed
+
+    - name: Ensure users testuser1, testuser2 and testuser3 are present in group nonposixgroup again
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: nonposixgroup
+        nonposix: yes
+        user:
+        - testuser1
+        - testuser2
+        - testuser3
+      register: result
+      failed_when: result.changed or result.failed
+
+
+    # POSIX MEMBER TEST
+
+    - name: Ensure users testuser1, testuser2 and testuser3 are present in group posixgroup
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: posixgroup
+        posix: yes
+        user:
+        - testuser1
+        - testuser2
+        - testuser3
+      register: result
+      failed_when: not result.changed or result.failed
+
+    - name: Ensure users testuser1, testuser2 and testuser3 are present in group posixgroup again
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: posixgroup
+        posix: yes
+        user:
+        - testuser1
+        - testuser2
+        - testuser3
+      register: result
+      failed_when: result.changed or result.failed
+
+    # EXTERNAL MEMBER TEST (REQUIRES AD)
+
+    - block:
+
+      - name: Ensure users testuser1, testuser2 and testuser3 are present in group externalgroup
+        ipagroup:
+          ipaadmin_password: SomeADMINpassword
+          name: externalgroup
+          external: yes
+          user:
+          - testuser1
+          - testuser2
+          - testuser3
+        register: result
+        failed_when: not result.changed or result.failed
+
+      - name: Ensure users testuser1, testuser2 and testuser3 are present in group externalgroup again
+        ipagroup:
+          ipaadmin_password: SomeADMINpassword
+          name: externalgroup
+          external: yes
+          user:
+          - testuser1
+          - testuser2
+          - testuser3
+        register: result
+        failed_when: result.changed or result.failed
+
+      when: trust_test_is_supported | default(false)
+
+    # CONVERT NONPOSIX TO POSIX GROUP WITH USERS
+
+    - name: Ensure nonposix group nonposixgroup as posix
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: nonposixgroup
+        posix: yes
+      register: result
+      failed_when: not result.changed or result.failed
+
+    - name: Ensure nonposix group nonposixgroup as posix, again
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: nonposixgroup
+        posix: yes
+      register: result
+      failed_when: result.changed or result.failed
+
+    - name: Ensure nonposix group nonposixgroup (now posix) has users still
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: nonposixgroup
+        posix: yes
+        user:
+        - testuser1
+        - testuser2
+        - testuser3
+      register: result
+      failed_when: result.changed or result.failed
+
+    # FAIL ON COMBINATIONS OF NONPOSIX, POSIX AND EXTERNAL
+
+    - name: Fail to ensure group as nonposix and posix
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: posixgroup
+        nonposix: yes
+        posix: yes
+      register: result
+      failed_when: not result.failed or "parameters are mutually exclusive" not in result.msg
+
+    - name: Fail to ensure group as nonposix and external
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: posixgroup
+        nonposix: yes
+        external: yes
+      register: result
+      failed_when: not result.failed or "parameters are mutually exclusive" not in result.msg
+
+    - name: Fail to ensure group as posix and external
+      ipagroup:
+        ipaadmin_password: SomeADMINpassword
+        name: posixgroup
+        posix: yes
+        external: yes
+      register: result
+      failed_when: not result.failed or "parameters are mutually exclusive" not in result.msg
+
+    # CLEANUP
+
     - name: Remove testing groups.
       ipagroup:
         ipaadmin_password: SomeADMINpassword
         name: extgroup,nonposixgroup,posixgroup
         state: absent
+
+    - name: Ensure test users testuser1, testuser2 and testuser3 are absent
+      ipauser:
+        ipaadmin_password: SomeADMINpassword
+        name: testuser1,testuser2,testuser3
+        state: absent