ipapermission: Fix attrs and drop privilege handling

The attrs handling was not complete and did not support to ensure presence
or absence of attributes with action:member.

The includedattrs and excludedattrs parameters have not been added with
this change as the use of attrs will automatically set includedattrs and
excludedattrs. The includedattrs and excludedattrs parameters are only
usable for managed permissions and duplicating attrs.

The permission module may not handle privileges. An IPA internal only API
has been used for this. The prvilege variable and all related code paths
have been removed.

Fixes: #424 ([Permission Handling] Not able to add additional attributes
             with existing attributes)
Fixes: #425 ([Permission Handling] Not able to add member privilege while
             adding permission)

README-permission.md

@@ -43,7 +43,7 @@ Example playbook to make sure permission "MyPermission" is present:
 
 ```yaml
 ---
-- name: Playbook to create an IPA permission.
+- name: Playbook to handle IPA permissions
   hosts: ipaserver
   become: yes
 
@@ -56,39 +56,61 @@ Example playbook to make sure permission "MyPermission" is present:
       right: all
 ```
 
-Example playbook to make sure permission "MyPermission" member "privilege" with value "User Administrators" is present:
+
+Example playbook to ensure permission "MyPermission" is present with attr carlicense:
 
 ```yaml
 ---
-- name: Permission add privilege to a permission
+- name: Playbook to handle IPA permissions
   hosts: ipaserver
-  become: true
+  become: yes
 
   tasks:
-  - name: Ensure permission MyPermission is present with the User Administrators privilege present
+  - name: Ensure permission "MyPermission" is present with attr carlicense
     ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: MyPermission
-      privilege: "User Administrators"
+      object_type: host
+      right: all
+      attrs:
+      - carlicense
+```
+
+
+Example playbook to ensure attr gecos is present in permission "MyPermission":
+
+```yaml
+---
+- name: Playbook to handle IPA permissions
+  hosts: ipaserver
+  become: yes
+
+  tasks:
+  - name: Ensure attr gecos is present in permission "MyPermission"
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: MyPermission
+      attrs:
+      - gecos
       action: member
 ```
 
 
-Example playbook to make sure permission "MyPermission" member "privilege" with value "User Administrators" is absent:
+Example playbook to ensure attr gecos is absent in permission "MyPermission":
-
 
 ```yaml
 ---
-- name: Permission remove privilege from a permission
+- name: Playbook to handle IPA permissions
   hosts: ipaserver
-  become: true
+  become: yes
 
   tasks:
-  - name: Ensure permission MyPermission is present without the User Administrators privilege
+  - name: Ensure attr gecos is present in permission "MyPermission"
     ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: MyPermission
-      privilege: "User Administrators"
+      attrs:
+      - gecos
       action: member
       state: absent
 ```
@@ -98,27 +120,30 @@ Example playbook to make sure permission "MyPermission" is absent:
 
 ```yaml
 ---
-- name: Playbook to manage IPA permission.
+- name: Playbook to handle IPA permissions
   hosts: ipaserver
   become: yes
 
   tasks:
-  - ipapermission:
+  - name: Ensure permission "MyPermission" is absent
+    ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: MyPermission
       state: absent
 ```
 
+
 Example playbook to make sure permission "MyPermission" is renamed to "MyNewPermission":
 
 ```yaml
 ---
-- name: Playbook to manage IPA permission.
+- name: Playbook to handle IPA permissions
   hosts: ipaserver
   become: yes
 
   tasks:
-  - ipapermission:
+  - name: Eure permission "MyPermission" is renamed to "MyNewPermission
+    ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: MyPermission
       rename: MyNewPermission
@@ -126,8 +151,6 @@ Example playbook to make sure permission "MyPermission" is renamed to "MyNewPerm
 ```
 
 
-
-
 Variables
 ---------
 
@@ -140,7 +163,7 @@ Variable | Description | Required
 `ipaadmin_password` | The admin password is a string and is required if there is no admin ticket available on the node | no
 `name` \| `cn` | The permission name string. | yes
 `right` \| `ipapermright` | Rights to grant. It can be a list of one or more of `read`, `search`, `compare`, `write`, `add`, `delete`, and `all` default: `all` | no
-`attrs` | All attributes to which the permission applies | no
+`attrs` | All attributes to which the permission applies. | no
 `bindtype` \| `ipapermbindruletype` | Bind rule type. It can be one of `permission`, `all`, `self`, or `anonymous` defaults to `permission` for new permissions. Bind rule type `self` can only be used on IPA versions 4.8.7 or up.| no
 `subtree` \| `ipapermlocation` | Subtree to apply permissions to | no
 `filter` \| `extratargetfilter` | Extra target filter | no
@@ -153,10 +176,12 @@ Variable | Description | Required
 `object_type` | Type of IPA object (sets subtree and objectClass targetfilter) | no
 `no_members` | Suppress processing of membership | no
 `rename` | Rename the permission object | no
-`privilege` | Member Privilege of Permission | no
 `action` | Work on permission or member level. It can be on of `member` or `permission` and defaults to `permission`. | no
 `state` | The state to ensure. It can be one of `present`, `absent`, or `renamed` default: `present`. | no
 
+The `includedattrs` and `excludedattrs` variables are only usable for managed permisions and are not exposed by the module. Using `attrs` for managed permissions will result in the automatic generation of `includedattrs` and `excludedattrs` in the IPA server.
+
+
 Authors
 =======
 

plugins/modules/ipapermission.py

@@ -102,10 +102,6 @@ options:
   rename:
     description: Rename the permission object
     required: false
-  privilege:
-    description: Member Privilege of Permission
-    required: false
-    type: list
   action:
     description: Work on permission or member privilege level.
     choices: ["permission", "member"]
@@ -126,19 +122,6 @@ EXAMPLES = """
     bindtype: permission
     object_type: host
 
-# Ensure permission "NAME" member privilege VALUE is present
-- ipapermission:
-    name: "Add Automember Rebuild Membership Task"
-    privilege: "Automember Task Administrator"
-    action: member
-
-# Ensure permission "NAME" member privilege VALUE is absent
-- ipapermission:
-    name: "Add Automember Rebuild Membership Task"
-    privilege: "IPA Masters Readers"
-    action: member
-    state: absent
-
 # Ensure permission NAME is absent
 - ipapermission:
     name: "Removed Permission Name"
@@ -152,8 +135,7 @@ RETURN = """
 from ansible.module_utils.basic import AnsibleModule
 from ansible.module_utils.ansible_freeipa_module import \
     temp_kinit, temp_kdestroy, valid_creds, api_connect, api_command, \
-    compare_args_ipa, module_params_get, gen_add_del_lists, \
+    compare_args_ipa, module_params_get, api_check_ipa_version
-    api_check_ipa_version
 import six
 
 if six.PY3:
@@ -207,13 +189,6 @@ def gen_args(right, attrs, bindtype, subtree,
     return _args
 
 
-def gen_member_args(privilege):
-    _args = {}
-    if privilege is not None:
-        _args["privilege"] = privilege
-    return _args
-
-
 def main():
     ansible_module = AnsibleModule(
         argument_spec=dict(
@@ -252,7 +227,6 @@ def main():
                              required=False),
             no_members=dict(type=bool, default=None, require=False),
             rename=dict(type="str", default=None, required=False),
-            privilege=dict(type="list", default=None, required=False),
 
             action=dict(type="str", default="permission",
                         choices=["member", "permission"]),
@@ -289,7 +263,6 @@ def main():
     object_type = module_params_get(ansible_module, "object_type")
     no_members = module_params_get(ansible_module, "no_members")
     rename = module_params_get(ansible_module, "rename")
-    privilege = module_params_get(ansible_module, "privilege")
     action = module_params_get(ansible_module, "action")
 
     # state
@@ -304,10 +277,12 @@ def main():
             ansible_module.fail_json(
                 msg="Only one permission can be added at a time.")
         if action == "member":
-            invalid = ["right", "attrs", "bindtype", "subtree",
+            invalid = ["right", "bindtype", "subtree",
                        "extra_target_filter", "rawfilter", "target",
                        "targetto", "targetfrom", "memberof", "targetgroup",
                        "object_type", "rename"]
+        else:
+            invalid = ["rename"]
 
     if state == "renamed":
         if len(names) != 1:
@@ -315,7 +290,7 @@ def main():
                 msg="Only one permission can be renamed at a time.")
         if action == "member":
             ansible_module.fail_json(
-                msg="Member Privileges cannot be renamed")
+                msg="Member action can not be used with state 'renamed'")
         invalid = ["right", "attrs", "bindtype", "subtree",
                    "extra_target_filter", "rawfilter", "target", "targetto",
                    "targetfrom", "memberof", "targetgroup", "object_type",
@@ -324,12 +299,13 @@ def main():
     if state == "absent":
         if len(names) < 1:
             ansible_module.fail_json(msg="No name given.")
-        invalid = ["right", "attrs", "bindtype", "subtree",
+        invalid = ["right",
+                   "bindtype", "subtree",
                    "extra_target_filter", "rawfilter", "target", "targetto",
                    "targetfrom", "memberof", "targetgroup", "object_type",
                    "no_members", "rename"]
-        if action == "permission":
+        if action != "member":
-            invalid.append("privilege")
+            invalid += ["attrs"]
 
     for x in invalid:
         if vars()[x] is not None:
@@ -366,11 +342,6 @@ def main():
                                 targetto, targetfrom, memberof, targetgroup,
                                 object_type, no_members, rename)
 
-                no_members_value = False
-
-                if no_members is not None:
-                    no_members_value = no_members
-
                 if action == "permission":
                     # Found the permission
                     if res_find is not None:
@@ -383,41 +354,18 @@ def main():
                     else:
                         commands.append([name, "permission_add", args])
 
-                    member_args = gen_member_args(privilege)
-                    if not compare_args_ipa(ansible_module, member_args,
-                                            res_find):
-
-                        # Generate addition and removal lists
-                        privilege_add, privilege_del = gen_add_del_lists(
-                                privilege, res_find.get("member_privilege"))
-
-                        # Add members
-                        if len(privilege_add) > 0:
-                            commands.append([name, "permission_add_member",
-                                             {
-                                                 "privilege": privilege_add,
-                                                 "no_members": no_members_value
-                                             }])
-                        # Remove members
-                        if len(privilege_del) > 0:
-                            commands.append([name, "permission_remove_member",
-                                             {
-                                                 "privilege": privilege_del,
-                                                 "no_members": no_members_value
-                                             }])
                 elif action == "member":
                     if res_find is None:
                         ansible_module.fail_json(
                             msg="No permission '%s'" % name)
 
-                    if privilege is None:
+                    # attrs
-                        ansible_module.fail_json(msg="No privilege given")
+                    if attrs is not None:
+                        _attrs = list(set(list(res_find["attrs"]) + attrs))
+                        if len(_attrs) > len(res_find["attrs"]):
+                            commands.append([name, "permission_mod",
+                                             {"attrs": _attrs}])
 
-                    commands.append([name, "permission_add_member",
-                                     {
-                                         "privilege": privilege,
-                                         "no_members": no_members_value
-                                     }])
                 else:
                     ansible_module.fail_json(
                         msg="Unknown action '%s'" % action)
@@ -455,13 +403,20 @@ def main():
                         ansible_module.fail_json(
                             msg="No permission '%s'" % name)
 
-                    if privilege is None:
+                    # attrs
-                        ansible_module.fail_json(msg="No privilege given")
+                    if attrs is not None:
+                        # New attribute list (remove given ones from find
+                        # result)
+                        # Make list with unique entries
+                        _attrs = list(set(res_find["attrs"]) - set(attrs))
+                        if len(_attrs) < 1:
+                            ansible_module.fail_json(
+                                msg="At minimum one attribute is needed.")
 
-                    commands.append([name, "permission_remove_member",
+                        # Entries New number of attributes is smaller
-                                     {
+                        if len(_attrs) < len(res_find["attrs"]):
-                                         "privilege": privilege,
+                            commands.append([name, "permission_mod",
-                                     }])
+                                             {"attrs": _attrs}])
 
             else:
                 ansible_module.fail_json(msg="Unknown state '%s'" % state)

tests/permission/test_permission.yml

@@ -37,41 +37,127 @@
     register: result
     failed_when: result.changed or result.failed
 
-  - name: Ensure permission perm-test-1 member User Administrators privilege is present
+  - name: Ensure permission perm-test-1 is present with attr carlicense
     ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: perm-test-1
-      privilege: "User Administrators"
+      attrs:
+      - carlicense
+    register: result
+    failed_when: not result.changed or result.failed
+
+  - name: Ensure permission perm-test-1 is present with attr carlicense again
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: perm-test-1
+      attrs:
+      - carlicense
+    register: result
+    failed_when: result.changed or result.failed
+
+  - name: Ensure permission perm-test-1 is present with attr carlicense and displayname
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: perm-test-1
+      attrs:
+      - carlicense
+      - displayname
+    register: result
+    failed_when: not result.changed or result.failed
+
+  - name: Ensure permission perm-test-1 is present with attr carlicense and displayname again
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: perm-test-1
+      attrs:
+      - carlicense
+      - displayname
+    register: result
+    failed_when: result.changed or result.failed
+
+  - name: Ensure attr gecos is present in permission perm-test-1
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: perm-test-1
+      attrs:
+      - gecos
       action: member
     register: result
     failed_when: not result.changed or result.failed
 
-  - name: Ensure permission perm-test-1 member User Administrators privilege is present again
+  - name: Ensure attr gecos is present in permission perm-test-1 again
     ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: perm-test-1
-      privilege: "User Administrators"
+      attrs:
+      - gecos
       action: member
     register: result
     failed_when: result.changed or result.failed
 
-  - name: Ensure permission perm-test-1 member User Administrators privilege is absent
+  - name: Ensure attr gecos is absent in permission perm-test-1
     ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: perm-test-1
-      privilege: "User Administrators"
+      attrs:
+      - gecos
       action: member
       state: absent
     register: result
     failed_when: not result.changed or result.failed
 
-  # NOTE: We use the "User Administrators" Privilege here since we don't have a module
+  - name: Ensure attr gecos is absent in permission perm-test-1 again
-  # to make one. A test privilege should be used in the future.
-  - name: Ensure permission perm-test-1 member User Administrators privilege is absent again
     ipapermission:
       ipaadmin_password: SomeADMINpassword
       name: perm-test-1
-      privilege: "User Administrators"
+      attrs:
+      - gecos
+      action: member
+      state: absent
+    register: result
+    failed_when: result.changed or result.failed
+
+  - name: Ensure attributes carlicense and displayname are present in permission "System{{':'}} Update DNS Entries"
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: "System: Update DNS Entries"
+      attrs:
+      - carlicense
+      - displayname
+      action: member
+    register: result
+    failed_when: not result.changed or result.failed
+
+  - name: Ensure attributes carlicense and displayname are present in permission "System{{':'}} Update DNS Entries" again
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: "System: Update DNS Entries"
+      attrs:
+      - carlicense
+      - displayname
+      action: member
+    register: result
+    failed_when: result.changed or result.failed
+
+  - name: Ensure attributes carlicense and displayname are present in permission "System{{':'}} Update DNS Entries"
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: "System: Update DNS Entries"
+      attrs:
+      - carlicense
+      - displayname
+      action: member
+      state: absent
+    register: result
+    failed_when: not result.changed or result.failed
+
+  - name: Ensure attributes carlicense and displayname are present in permission "System{{':'}} Update DNS Entries" again
+    ipapermission:
+      ipaadmin_password: SomeADMINpassword
+      name: "System: Update DNS Entries"
+      attrs:
+      - carlicense
+      - displayname
       action: member
       state: absent
     register: result