Sysaccount management

[t-woerner]

New sysaccount management module

There is a new sysaccount management module placed in the plugins folder:

    plugins/modules/ipasysaccount.py

The sysaccount module allows to ensure presence or absence of system
accounts.

Here is the documentation for the module:

    README-sysaccount.md

New sysaccount example playbooks:

    playbooks/sysaccount/sysaccount-absent.yml
    playbooks/sysaccount/sysaccount-disabled.yml
    playbooks/sysaccount/sysaccount-enabled.yml
    playbooks/sysaccount/sysaccount-present.yml
    playbooks/sysaccount/sysaccount-privileged.yml
    playbooks/sysaccount/sysaccount-unprivileged.yml

New tests for the module:

    tests/sysaccount/test_sysaccount.yml
    tests/sysaccount/test_sysaccount_client_context.yml

iparole: Add sysaccount member support

sysaccounts can now be used as a member for roles.

Example:

  - name: Ensure role my-app role has sysaccount member my-app
    iparole:
      name: my-app role
      sysaccount: my-app
      action: member

New tests for the module:

    tests/role/test_role_sysaccount_member.yml

Summary by Sourcery

Add support for FreeIPA system account management and integrate it with role assignments

New Features:

• Introduce ipasysaccount module to create, modify, delete, enable, disable, and manage privileges of system accounts
• Extend iparole module to handle sysaccount membership for roles

Documentation:

• Add README-sysaccount.md and playbook examples for sysaccount operations
• Update main README and README-role to include sysaccount module and parameters

Tests:

• Add end-to-end tests for ipasysaccount module in server and client contexts
• Add tests for sysaccount membership operations in iparole module

[sourcery-ai]

Hey there - I've reviewed your changes and found some issues that need to be addressed.

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location> `plugins/modules/ipasysaccount.py:248` </location>
<code_context>
+                    if not res_find["nsaccountlock"]:
+                        commands.append([name, "sysaccount_disable", {}])
+
+            else:
+                ansible_module.fail_json(msg="Unkown state '%s'" % state)
+
+        # Execute commands
</code_context>

<issue_to_address>
**nitpick (typo):** Typo in error message: 'Unkown' should be 'Unknown'.

Fixing the typo ensures error messages are clear and professional.

```suggestion
                ansible_module.fail_json(msg="Unknown state '%s'" % state)
```
</issue_to_address>

### Comment 2
<location> `plugins/modules/ipasysaccount.py:167` </location>
<code_context>
+        mutually_exclusive=[["random", "password"]]
+    )
+
+    ansible_module._ansible_debug = True
+
+    # Get parameters
</code_context>

<issue_to_address>
**🚨 issue (security):** Enabling debug mode by default may expose sensitive information.

Recommend setting debug mode to off by default and allowing it to be enabled via configuration to prevent accidental exposure of sensitive data in production.
</issue_to_address>

### Comment 3
<location> `README-role.md:259` </location>
<code_context>
 `hostgroup` | List of hostgroups to be assigned or not assigned to the role. | no
 `service` | List of services to be assigned or not assigned to the role. | no
+`sysaccount` | List of sysaccounts to be assigned or not assigned to the role. | no
 `action` | Work on role or member level. It can be on of `member` or `role` and defaults to `role`. | no
 `state` | The state to ensure. It can be one of `present`, `absent`, default: `present`. | no

</code_context>

<issue_to_address>
**issue (typo):** Typo: 'on of' should be 'one of'.

Please update the phrase to: 'It can be one of `member` or `role` and defaults to `role`.'

```suggestion
`action` | Work on role or member level. It can be one of `member` or `role` and defaults to `role`. | no
```
</issue_to_address>

### Comment 4
<location> `plugins/modules/ipasysaccount.py:206` </location>
<code_context>
+    changed = False
+    exit_args = {}
+
+    # Connect to IPA API
+    with ansible_module.ipa_connect():
+
</code_context>

<issue_to_address>
**issue (complexity):** Consider refactoring the main state-handling logic into separate handler functions and removing unused code to simplify and flatten the module's control flow.

```markdown
You can simplify the big `if/elif` tree by:

1. Removing unused bits:
   - Drop the PY2 `unicode` stub (it’s never used).
   - Drop `ansible_module._ansible_debug = True` unless you actually need it.
   - Remove `exit_args = {}` and just call `exit_json(changed=changed)`.

2. Mapping states to small handler functions. For example:

```python
# at module top
STATE_HANDLERS = {
    'present': handle_present,
    'absent':  handle_absent,
    'enabled': handle_enabled,
    'disabled':handle_disabled,
}
```

```python
# in main(), replace the big loop with:
with ansible_module.ipa_connect():
    commands = []
    handler = STATE_HANDLERS[state]
    for name in names:
        commands.extend(handler(ansible_module, name,
                                description, random,
                                privileged, password,
                                delete_continue))
    changed = ansible_module.execute_ipa_commands(commands)

ansible_module.exit_json(changed=changed)
```

3. Example handlers:

```python
def handle_present(mod, name, description, random, privileged, password, _):
    args = gen_args(description, random, privileged, password)
    existing = find_sysaccount(mod, name)
    if not existing:
        return [[name, 'sysaccount_add', args]]
    if not compare_args_ipa(mod, args, existing):
        return [[name, 'sysaccount_mod', args]]
    return []

def handle_absent(mod, name, *_args):
    if find_sysaccount(mod, name):
        return [[name, 'sysaccount_del', {'continue': _args[-1]}]]
    return []

def handle_enabled(mod, name, *_):
    res = find_sysaccount(mod, name)
    if res and res['nsaccountlock']:
        return [[name, 'sysaccount_enable', {}]]
    return []

def handle_disabled(mod, name, *_):
    res = find_sysaccount(mod, name)
    if res and not res['nsaccountlock']:
        return [[name, 'sysaccount_disable', {}]]
    return []
```

This preserves all behavior, drops dead code, and flattens the logic.
</issue_to_address>

### Comment 5
<location> `plugins/modules/ipasysaccount.py:189-192` </location>
<code_context>
    if state == "present":
        if len(names) != 1:
            ansible_module.fail_json(
                msg="Only one sysaccount can be added at a time.")

</code_context>

<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))

```suggestion
    if state == "present" and len(names) != 1:
        ansible_module.fail_json(
            msg="Only one sysaccount can be added at a time.")

```

<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.

Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>

### Comment 6
<location> `plugins/modules/ipasysaccount.py:238-240` </location>
<code_context>
                if res_find is not None:
                    if res_find["nsaccountlock"]:
                        commands.append([name, "sysaccount_enable", {}])

</code_context>

<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))

```suggestion
                if res_find is not None and res_find["nsaccountlock"]:
                    commands.append([name, "sysaccount_enable", {}])

```

<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.

Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>

### Comment 7
<location> `plugins/modules/ipasysaccount.py:243-245` </location>
<code_context>
                if res_find is not None:
                    if not res_find["nsaccountlock"]:
                        commands.append([name, "sysaccount_disable", {}])

</code_context>

<issue_to_address>
**suggestion (code-quality):** Merge nested if conditions ([`merge-nested-ifs`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/merge-nested-ifs))

```suggestion
                if res_find is not None and not res_find["nsaccountlock"]:
                    commands.append([name, "sysaccount_disable", {}])

```

<br/><details><summary>Explanation</summary>Too much nesting can make code difficult to understand, and this is especially
true in Python, where there are no brackets to help out with the delineation of
different nesting levels.

Reading deeply nested code is confusing, since you have to keep track of which
conditions relate to which levels. We therefore strive to reduce nesting where
possible, and the situation where two `if` conditions can be combined using
`and` is an easy win.
</details>
</issue_to_address>

### Comment 8
<location> `plugins/modules/ipasysaccount.py:146` </location>
<code_context>
def main():
    ansible_module = IPAAnsibleModule(
        argument_spec=dict(
            # general
            name=dict(type="list", elements="str", required=True,
                      aliases=["login"]),
            # present
            description=dict(required=False, type='str', default=None),
            random=dict(required=False, type='bool', default=None),
            privileged=dict(required=False, type='bool', default=None),
            password=dict(required=False, type='str',
                          aliases=["userpassword"], default=None),
            # state
            state=dict(type="str", default="present",
                       choices=["present", "absent", "enabled", "disabled"]),
        ),
        supports_check_mode=True,
        ipa_module_options=["delete_continue"],
        mutually_exclusive=[["random", "password"]]
    )

    ansible_module._ansible_debug = True

    # Get parameters

    # general
    names = ansible_module.params_get("name")

    # present
    description = ansible_module.params_get("description")
    random = ansible_module.params_get("random")
    privileged = ansible_module.params_get("privileged")
    password = ansible_module.params_get("password")

    delete_continue = ansible_module.params_get("delete_continue")

    # state
    state = ansible_module.params_get("state")

    # Check parameters

    invalid = []

    if state == "present":
        if len(names) != 1:
            ansible_module.fail_json(
                msg="Only one sysaccount can be added at a time.")

    if state in ["absent", "enabled", "disabled"]:
        if len(names) < 1:
            ansible_module.fail_json(msg="No name given.")
        invalid = ["description", "random", "privileged", "password"]

    ansible_module.params_fail_used_invalid(invalid, state)

    # Init

    changed = False
    exit_args = {}

    # Connect to IPA API
    with ansible_module.ipa_connect():

        commands = []
        for name in names:
            # Make sure sysaccount exists
            res_find = find_sysaccount(ansible_module, name)

            # Create command
            if state == "present":

                # Generate args
                args = gen_args(description, random, privileged, password)

                # Found the sysaccount
                if res_find is not None:
                    # For all settings is args, check if there are
                    # different settings in the find result.
                    # If yes: modify
                    if not compare_args_ipa(ansible_module, args,
                                            res_find):
                        commands.append([name, "sysaccount_mod", args])
                else:
                    commands.append([name, "sysaccount_add", args])

            elif state == "absent":
                if res_find is not None:
                    commands.append(
                        [name, "sysaccount_del", {"continue": delete_continue}]
                    )

            elif state == "enabled":
                if res_find is not None:
                    if res_find["nsaccountlock"]:
                        commands.append([name, "sysaccount_enable", {}])

            elif state == "disabled":
                if res_find is not None:
                    if not res_find["nsaccountlock"]:
                        commands.append([name, "sysaccount_disable", {}])

            else:
                ansible_module.fail_json(msg="Unkown state '%s'" % state)

        # Execute commands

        changed = ansible_module.execute_ipa_commands(commands)

    # Done

    ansible_module.exit_json(changed=changed, **exit_args)

</code_context>

<issue_to_address>
**issue (code-quality):** We've found these issues:

- Swap if/else branches ([`swap-if-else-branches`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/swap-if-else-branches/))
- Merge else clause's nested if statement into elif ([`merge-else-if-into-elif`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/merge-else-if-into-elif/))
- Replace interpolated string formatting with f-string ([`replace-interpolation-with-fstring`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/replace-interpolation-with-fstring/))
- Low code quality found in main - 18% ([`low-code-quality`](https://docs.sourcery.ai/Reference/Default-Rules/comments/low-code-quality/))

<br/><details><summary>Explanation</summary>


The quality score for this function is below the quality threshold of 25%.
This score is a combination of the method length, cognitive complexity and working memory.

How can you solve this?

It might be worth refactoring this function to make it shorter and more readable.

- Reduce the function length by extracting pieces of functionality out into
  their own functions. This is the most important thing you can do - ideally a
  function should be less than 10 lines.
- Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
  sits together within the function rather than being scattered.</details>
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[varunmylaraiah]

All downstream test suites have successfully passed with this PR

[varunmylaraiah]

Thanks for the PR. One thing I noticed is that the ‘random’ option is not returning the password

[varunmylaraiah]

LGTM

[rjeffman]

I suggest copying the task names from the example playbooks.

[rjeffman]

LGTM.

[rjeffman]

I'll merge it after CI runs.

[rjeffman]

Changes fixed tests. LGTM.