From 1f25024396c3b77770ec667d3c74218acd1b4d0b Mon Sep 17 00:00:00 2001
From: Thomas Woerner <twoerner@redhat.com>
Date: Thu, 16 Sep 2021 12:31:09 +0200
Subject: [PATCH] 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                              |   6 +-
 plugins/modules/ipagroup.py                  |  78 ++++----
 tests/group/test_group_external_nonposix.yml | 187 ++++++++++++++++++-
 3 files changed, 220 insertions(+), 51 deletions(-)

diff --git a/README-group.md b/README-group.md
index d94e3ccc..f845ef05 100644
--- a/README-group.md
+++ b/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
-`nonposix` | Create as a non-POSIX group. (bool) | no
-`external` | Allow adding external non-IPA members from trusted domains. (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. `nonposix`, `posix` and `external` are mutually exclusive. (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
diff --git a/plugins/modules/ipagroup.py b/plugins/modules/ipagroup.py
index bbc8e973..a502f935 100644
--- a/plugins/modules/ipagroup.py
+++ b/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 (
-            (posix is not None and posix is False)
-            or nonposix
-            or external
-        ):
+        if external is not None and external or posix is False:
             module.fail_json(
-                msg="Cannot change `POSIX` status of a group "
-                    "to `non-POSIX` or `external`.")
-    # Can't change an existing external group
+                msg="Cannot change `posix` group to `non-posix` or "
+                "`external`.")
+    # An external group can not be changed to nonposix or posix or nonexternal
     if is_external_group(res_find):
-        if (
-            posix
-            or (nonposix is not None and nonposix is False)
-            or (external is not None and external is False)
-        ):
+        if external is False or posix is not None:
             module.fail_json(
-                msg="Cannot change `external` status of group "
-                    "to `POSIX` or `non-external`.")
-
-
-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
+                msg="Cannot change `external` group to `posix` or "
+                "`non-posix`.")
 
 
 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,
-                                       posix, external)
+                check_objectclass_args(ansible_module, res_find, posix,
+                                       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,
-                                               nonposix, posix, external):
-                            if (
-                                posix
-                                or (nonposix is not None and not nonposix)
-                            ):
+                        # Also if it is a modification from nonposix to posix
+                        # or nonposix to external.
+                        if not compare_args_ipa(ansible_module, args,
+                                                res_find) or \
+                           (
+                               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
diff --git a/tests/group/test_group_external_nonposix.yml b/tests/group/test_group_external_nonposix.yml
index 55c92408..f647f7d3 100644
--- a/tests/group/test_group_external_nonposix.yml
+++ b/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
-- 
GitLab