automember: Verify condition keys

The automember_add_condition and automember_remove_condition commands
are not verifying condition keys in all cases. This is for example not
done in the removal case if a condition is not part of the automember
rule.

For consistent behaviour this needs to be done in the automember module
now. The condition keys are verified with the user and group aciattrs
returned by the API command json_metadata now.

Related: RHBZ#1976926

galaxy.yml

@@ -14,8 +14,6 @@ issues: "https://github.com/freeipa/ansible-freeipa/issues"
 readme: "README.md"
 license: "GPL-3.0-or-later"
 
-dependencies:
-
 tags:
   - "system"
   - "identity"

plugins/modules/ipaautomember.py

@@ -66,15 +66,33 @@ options:
     type: list
     elements: dict
     aliases: ["automemberexclusiveregex"]
+    options:
+      key:
+        description: The attribute of the regex
+        type: str
+        required: true
+      expression:
+        description: The expression of the regex
+        type: str
+        required: true
   inclusive:
     description: List of dictionaries containing the attribute and expression.
     type: list
     elements: dict
     aliases: ["automemberinclusiveregex"]
+    options:
+      key:
+        description: The attribute of the regex
+        type: str
+        required: true
+      expression:
+        description: The expression of the regex
+        type: str
+        required: true
   action:
-    description: Work on service or member level
+    description: Work on automember or member level
-    default: service
+    default: automember
-    choices: ["member", "service"]
+    choices: ["member", "automember"]
   state:
     description: State to ensure
     default: present
@@ -167,6 +185,15 @@ def transform_conditions(conditions):
     return transformed
 
 
+def check_condition_keys(ansible_module, conditions, aciattrs):
+    if conditions is None:
+        return
+    for condition in conditions:
+        if condition["key"] not in aciattrs:
+            ansible_module.fail_json(
+                msg="Invalid automember condition key '%s'" % condition["key"])
+
+
 def main():
     ansible_module = AnsibleModule(
         argument_spec=dict(
@@ -174,17 +201,27 @@ def main():
             ipaadmin_principal=dict(type="str", default="admin"),
             ipaadmin_password=dict(type="str", required=False, no_log=True),
 
-            inclusive=dict(type="list", aliases=[
+            inclusive=dict(type="list",
-                           "automemberinclusiveregex"], default=None),
+                           aliases=["automemberinclusiveregex"], default=None,
+                           options=dict(
+                               key=dict(type="str", required=True),
+                               expression=dict(type="str", required=True)
+                           ),
+                           elements="dict", required=False),
             exclusive=dict(type="list", aliases=[
-                           "automemberexclusiveregex"], default=None),
+                           "automemberexclusiveregex"], default=None,
+                           options=dict(
+                               key=dict(type="str", required=True),
+                               expression=dict(type="str", required=True)
+                           ),
+                           elements="dict", required=False),
             name=dict(type="list", aliases=["cn"],
                       default=None, required=True),
             description=dict(type="str", default=None),
             automember_type=dict(type='str', required=False,
                                  choices=['group', 'hostgroup']),
-            action=dict(type="str", default="service",
+            action=dict(type="str", default="automember",
-                        choices=["member", "service"]),
+                        choices=["member", "automember"]),
             state=dict(type="str", default="present",
                        choices=["present", "absent", "rebuild"]),
             users=dict(type="list", default=None),
@@ -246,11 +283,33 @@ def main():
             # Make sure automember rule exists
             res_find = find_automember(ansible_module, name, automember_type)
 
+            # Check inclusive and exclusive conditions
+            if inclusive is not None or exclusive is not None:
+                # automember_type is either "group" or "hostgorup"
+                if automember_type == "group":
+                    _type = "user"
+                elif automember_type == "hostgroup":
+                    _type = "host"
+                else:
+                    ansible_module.fail_json(
+                        msg="Bad automember type '%s'" % automember_type)
+
+                try:
+                    aciattrs = api_command(
+                        ansible_module, "json_metadata", to_text(_type), {}
+                    )['objects'][_type]['aciattrs']
+                except Exception as ex:
+                    ansible_module.fail_json(
+                        msg="%s: %s: %s" % ("json_metadata", _type, str(ex)))
+
+                check_condition_keys(ansible_module, inclusive, aciattrs)
+                check_condition_keys(ansible_module, exclusive, aciattrs)
+
             # Create command
             if state == 'present':
                 args = gen_args(description, automember_type)
 
-                if action == "service":
+                if action == "automember":
                     if res_find is not None:
                         if not compare_args_ipa(ansible_module,
                                                 args,
@@ -273,7 +332,8 @@ def main():
 
                 elif action == "member":
                     if res_find is None:
-                        ansible_module.fail_json(msg="No service '%s'" % name)
+                        ansible_module.fail_json(
+                            msg="No automember '%s'" % name)
 
                     inclusive_add = transform_conditions(inclusive or [])
                     inclusive_del = []
@@ -309,14 +369,15 @@ def main():
                                      condition_args])
 
             elif state == 'absent':
-                if action == "service":
+                if action == "automember":
                     if res_find is not None:
                         commands.append([name, 'automember_del',
                                          {'type': to_text(automember_type)}])
 
                 elif action == "member":
                     if res_find is None:
-                        ansible_module.fail_json(msg="No service '%s'" % name)
+                        ansible_module.fail_json(
+                            msg="No automember '%s'" % name)
 
                     if inclusive is not None:
                         for _inclusive in transform_conditions(inclusive):
@@ -355,7 +416,6 @@ def main():
         if ansible_module.check_mode:
             ansible_module.exit_json(changed=len(commands) > 0, **exit_args)
 
-        errors = []
         for name, command, args in commands:
             try:
                 if name is None:
@@ -372,16 +432,13 @@ def main():
             except Exception as ex:
                 ansible_module.fail_json(msg="%s: %s: %s" % (command, name,
                                                              str(ex)))
-            # Get all errors
+
-            if "failed" in result and len(result["failed"]) > 0:
+            # result["failed"] is used only for INCLUDE_RE, EXCLUDE_RE
-                for item in result["failed"]:
+            # if entries could not be added that are already there and
-                    failed_item = result["failed"][item]
+            # it entries could not be removed that are not there.
-                    for member_type in failed_item:
+            # All other issues like invalid attributes etc. are handled
-                        for member, failure in failed_item[member_type]:
+            # as exceptions. Therefore the error section is not here as
-                            errors.append("%s: %s %s: %s" % (
+            # in other modules.
-                                command, member_type, member, failure))
-        if len(errors) > 0:
-            ansible_module.fail_json(msg=", ".join(errors))
 
     except Exception as e:
         ansible_module.fail_json(msg=str(e))

plugins/modules/ipasudorule.py

@@ -53,7 +53,7 @@ options:
     required: false
     choices: ["all", ""]
     aliases: ["usercat"]
-  usergroup:
+  group:
     description: List of user groups assigned to the sudo rule.
     required: false
   runasgroupcategory:

requirements.txt

@@ -1 +0,0 @@
-ansible>=2.8.0

roles/ipabackup/library/ipabackup_get_backup_dir.py

@@ -0,0 +1,69 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+
+# Authors:
+#   Thomas Woerner <twoerner@redhat.com>
+#
+# Copyright (C) 2021  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+ANSIBLE_METADATA = {
+    'metadata_version': '1.0',
+    'supported_by': 'community',
+    'status': ['preview'],
+}
+
+DOCUMENTATION = '''
+---
+module: ipabackup_get_backup_dir
+short description:
+  Get IPA_BACKUP_DIR from ipaplatform
+description:
+  Get IPA_BACKUP_DIR from ipaplatform
+options:
+author:
+    - Thomas Woerner
+'''
+
+EXAMPLES = '''
+# Get IPA_BACKUP_DIR from ipaplatform
+- name: ipabackup_get_backup_dir:
+  register result
+'''
+
+RETURN = '''
+backup_dir:
+  description: IPA_BACKUP_DIR from ipaplatform
+  returned: always
+  type: str
+'''
+
+from ansible.module_utils.basic import AnsibleModule
+from ipaplatform.paths import paths
+
+
+def main():
+    module = AnsibleModule(
+        argument_spec=dict(),
+        supports_check_mode=True,
+    )
+
+    module.exit_json(changed=False,
+                     backup_dir=paths.IPA_BACKUP_DIR)
+
+
+if __name__ == '__main__':
+    main()

roles/ipabackup/tasks/get_ipabackup_dir.yml

@@ -1,12 +1,8 @@
 ---
-- name: Get IPA_BACKUP_DIR dir from ipaplatform
+- name: Get IPA_BACKUP_DIR from ipaplatform
-  command: "{{ ansible_python_interpreter | default('/usr/bin/python') }}"
+  ipabackup_get_backup_dir:
-  args:
+  register: result_ipabackup_get_backup_dir
-    stdin: |
-      from ipaplatform.paths import paths
-      print(paths.IPA_BACKUP_DIR)
-  register: result_ipaplatform_backup_dir
 
 - name: Set IPA backup dir
   set_fact:
-    ipabackup_dir: "{{ result_ipaplatform_backup_dir.stdout_lines | first }}"
+    ipabackup_dir: "{{ result_ipabackup_get_backup_dir.backup_dir }}"

roles/ipaserver/README.md

@@ -260,12 +260,12 @@ Certificate system Variables
 Variable | Description | Required
 -------- | ----------- | --------
 `ipaserver_external_ca` | Generate a CSR for the IPA CA certificate to be signed by an external CA. (bool, default: false) | no
-`ipaserver_external_ca_type` | Type of the external CA. (choice: generic,ms-cs) | no
+`ipaserver_external_ca_type` | Type of the external CA. (choice: generic, ms-cs) | no
 `ipaserver_external_ca_profile` | Specify the certificate profile/template to use at the external CA. (string) | no
 `ipaserver_external_cert_files` | Files containing the IPA CA certificates and the external CA certificate chains (list of string) | no
 `ipaserver_subject_base` | The certificate subject base (default O=<realm-name>). RDNs are in LDAP order (most specific RDN first). (string) | no
 `ipaserver_ca_subject` | The CA certificate subject DN (default CN=Certificate Authority,O=<realm-name>). RDNs are in LDAP order (most specific RDN first). (string) | no
-`ipaserver_ca_signing_algorithm` | Signing algorithm of the IPA CA certificate. (choice: SHA1withRSA,SHA256withRSA,SHA512withRSA) | no
+`ipaserver_ca_signing_algorithm` | Signing algorithm of the IPA CA certificate. (choice: SHA1withRSA, SHA256withRSA, SHA512withRSA) | no
 
 DNS Variables
 -------------
@@ -280,7 +280,7 @@ Variable | Description | Required
 `ipaserver_forwarders` | Add DNS forwarders to the DNS configuration. (list of strings) | no
 `ipaserver_no_forwarders` | Do not add any DNS forwarders. Root DNS servers will be used instead. (bool, default: false) | no
 `ipaserver_auto_forwarders` | Add DNS forwarders configured in /etc/resolv.conf to the list of forwarders used by IPA DNS. (bool, default: false) | no
-`ipaserver_forward_policy` | DNS forwarding policy for global forwarders specified using other options. (choice: first|only) | no
+`ipaserver_forward_policy` | DNS forwarding policy for global forwarders specified using other options. (choice: first, only) | no
 `ipaserver_no_dnssec_validation` | Disable DNSSEC validation on this server. (bool, default: false) | no
 
 AD trust Variables

tests/automember/test_automember.yml

@@ -164,6 +164,34 @@
     register: result
     failed_when: result.changed or result.failed
 
+  - name: Ensure testgroup group automember conditions fails on invalid inclusive key
+    ipaautomember:
+      ipaadmin_principal: admin
+      ipaadmin_password: SomeADMINpassword
+      name: testgroup
+      automember_type: group
+      inclusive:
+        - key: cns
+          expression: 'foo'
+      action: member
+    register: result
+    failed_when: result.changed or not result.failed or
+                 "Invalid automember condition key 'cns'" not in result.msg
+
+  - name: Ensure testgroup group automember conditions fails on invalid exlusive key
+    ipaautomember:
+      ipaadmin_principal: admin
+      ipaadmin_password: SomeADMINpassword
+      name: testgroup
+      automember_type: group
+      exclusive:
+        - key: cns
+          expression: 'foo'
+      action: member
+    register: result
+    failed_when: result.changed or not result.failed or
+                 "Invalid automember condition key 'cns'" not in result.msg
+
   - name: Ensure testhostgroup hostgroup automember rule is present
     ipaautomember:
       ipaadmin_password: SomeADMINpassword
@@ -282,6 +310,35 @@
     register: result
     failed_when: result.changed or result.failed
 
+
+  - name: Ensure testhostgroup hostgroup automember conditions fails on invalid inclusive key
+    ipaautomember:
+      ipaadmin_principal: admin
+      ipaadmin_password: SomeADMINpassword
+      name: testhostgroup
+      automember_type: hostgroup
+      inclusive:
+        - key: cns
+          expression: 'foo'
+      action: member
+    register: result
+    failed_when: result.changed or not result.failed or
+                 "Invalid automember condition key 'cns'" not in result.msg
+
+  - name: Ensure testhostgroup hostgroup automember conditions fails on invalid exlusive key
+    ipaautomember:
+      ipaadmin_principal: admin
+      ipaadmin_password: SomeADMINpassword
+      name: testhostgroup
+      automember_type: hostgroup
+      exclusive:
+        - key: cns
+          expression: 'foo'
+      action: member
+    register: result
+    failed_when: result.changed or not result.failed or
+                 "Invalid automember condition key 'cns'" not in result.msg
+
   # CLEANUP TEST ITEMS
 
   - name: Ensure group testgroup is absent

tests/pwpolicy/test_pwpolicy.yml

@@ -81,8 +81,7 @@
       ipaadmin_password: SomeADMINpassword
       state: absent
     register: result
-    ignore_errors: True
+    failed_when: not result.failed or "'global_policy' can not be made absent." not in result.msg
-    failed_when: (result is defined and result) or result.failed
 
   - name: Ensure absence of pwpolicies for group ops
     ipapwpolicy: