Ipauser query

[t-woerner]

ansible_freeipa_module.py: New PARAM_MAPPING and query support

New functions and methods have been added to reduce code complexity
and duplication in several modules, as well as to enable query mode
in the modules.

New functions:

    gen_member_add_del_lists
        Compute member add/del lists for all member params marked with
        "member": True in PARAM_MAPPING. Handles sync, add-only and
        remove-only modes based on action and state.

    gen_member_args_from_mapping
        Build a member args dict for compare_args_ipa() from
        PARAM_MAPPING. Replaces per-module gen_member_args() functions.

New static methods:

    gen_args_from_mapping
        Generate IPA command args dict from PARAM_MAPPING. Replaces
        per-module gen_args() functions.

    extract_params
        Extract parameter values from module params using PARAM_MAPPING.

    extract_params_from_entry
        Extract parameter values from a users/hosts dict entry using
        PARAM_MAPPING.

    build_query_param_settings
        Build query parameter settings from PARAM_MAPPING and
        QUERY_FIELDS for use with execute_query.

New methods:

    execute_query
        Execute query state for modules supporting state: query.

    _extract_query_fields
        Extract requested fields from a query result using the mapping.

The params_fail_used_invalid method has been extended to accept optional
params and param_mapping arguments for use with extracted params dicts.

PARAM_MAPPING is an ordered dict that maps Ansible parameter names to
their metadata. Each entry is a dict with optional keys:

    "api_name"      IPA attribute name if different from the Ansible
                    parameter name.
    "gen_args"      Set to False to exclude the parameter from
                    gen_args_from_mapping (used for members and
                    parameters handled separately).
    "member"        Set to True to include the parameter in
                    gen_member_add_del_lists and
                    gen_member_args_from_mapping.
    "module_param"  Set to True for top-level module parameters that
                    are not part of the entity (e.g. action, state,
                    query_param, users).
    "return_only"   Set to True for parameters that are only returned
                    by IPA and never sent (excluded from
                    gen_args_from_mapping).
    "nonempty_list" Set to True if an empty list value should not be
                    sent to IPA.
    "lowercase"     Set to True to lowercase values before comparison.
    "convert_to"    Conversion type for the parameter value
                    (e.g. "str", "int", "bool").
An empty dict {} means the Ansible name equals the IPA attribute name
and the parameter is included in gen_args_from_mapping with no special
handling.

For query support a QUERY_FIELDS dict is needed with "prefix" for the
grouping key in PKEY_ONLY results, "primary_key" for the IPA name
attribute and "base" for the list of essential field names returned by
default.

ipauser: Use PARAM_MAPPING and query state support

The ipauser module has been reworked to use the new PARAM_MAPPING added
to ansible_freeipa_module.

The member handling for manager, principal, certificate and certmapdata
has been simplified by using gen_member_add_del_lists. The member
entries in PARAM_MAPPING are now marked with "member": True. This
replaces the manual calls to gen_add_del_lists, gen_add_list and
gen_intersection_list across three separate action/state branches with
a single unified call.

The new query state allows to retrieve user information from IPA.
The query_param option controls which fields are returned: BASE for
essential fields, ALL for all fields, PKEY_ONLY for user names only,
or a list of specific field names.

Here is the updated documentation of the module:

    README-user.md

New tests for the query state can be found at:

    tests/user/test_user_query.yml

Summary by Sourcery

Add query-mode support to the ipauser module and refactor shared FreeIPA utilities to use a parameter mapping–based approach.

New Features:

• Introduce a query state to the ipauser module with a query_param option to control which user fields are returned, including support for BASE, ALL, PKEY_ONLY, and custom field lists.
• Provide generic parameter mapping and query execution helpers in ansible_freeipa_module to generate IPA command arguments and extract/query fields across modules.

Enhancements:

• Refactor ipauser to use a centralized PARAM_MAPPING and shared helpers for argument generation, member handling, and parameter extraction, reducing duplication and simplifying state handling.
• Extend parameter validation to work with mapped/extracted parameter dictionaries and support module-level query-only options.

Documentation:

• Update README-user.md with examples demonstrating how to query users with the new query state and query_param modes.

Tests:

• Add an integration-style playbook test to verify user query behavior, including querying non-existent users, all users, specific users, and various query_param combinations.

[sourcery-ai]

Hey - I've found 2 issues, and left some high level feedback:

• In convert_user_dates(), accessing val[-1] assumes a non-empty string; consider guarding against empty strings and non-string inputs (e.g., via isinstance(val, str) and len checks) to avoid runtime errors when dates are mis-specified or omitted.
• In the PARAM_MAPPING / query helpers, there is a mix of metadata keys like "type" and "type_cast" used in different places (e.g. extract_params vs _extract_query_fields); it would be clearer and less error-prone to standardize on a single key for type information or explicitly document the difference and enforce it consistently.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In convert_user_dates(), accessing val[-1] assumes a non-empty string; consider guarding against empty strings and non-string inputs (e.g., via isinstance(val, str) and len checks) to avoid runtime errors when dates are mis-specified or omitted.
- In the PARAM_MAPPING / query helpers, there is a mix of metadata keys like "type" and "type_cast" used in different places (e.g. extract_params vs _extract_query_fields); it would be clearer and less error-prone to standardize on a single key for type information or explicitly document the difference and enforce it consistently.

## Individual Comments

### Comment 1
<location path="plugins/modules/ipauser.py" line_range="854" />
<code_context>
+        val = user_params.get("date_field")
</code_context>
<issue_to_address>
**issue:** convert_user_dates can raise on empty strings when indexing val[-1]

`convert_user_dates` assumes any non-None date value is a non-empty string and does `val[-1]`. If a caller passes an empty string, this raises `IndexError`. Consider adding a guard like `if isinstance(val, str) and val and val[-1] != 'Z':` to handle empty or invalid input more safely.
</issue_to_address>

### Comment 2
<location path="plugins/module_utils/ansible_freeipa_module.py" line_range="1331" />
<code_context>
             self, name, datatype, allow_empty)

-    def params_fail_used_invalid(self, invalid_params, state, action=None):
+    def params_fail_used_invalid(self, invalid_params, state, action=None,
+                                 params=None, param_mapping=None):
         """
</code_context>
<issue_to_address>
**issue (complexity):** Consider keeping params_fail_used_invalid as a small, mapping-agnostic core method and moving the param_mapping-specific behavior into a thin wrapper helper for callers that need it.

The new `params_fail_used_invalid` signature is carrying mapping-specific logic that makes a core utility harder to reason about. You can keep the new behavior but push the mapping complexity into a dedicated wrapper, keeping the primary method simple and easier to reuse.

For example:

```python
def params_fail_used_invalid(self, invalid_params, state, action=None,
                             params=None):
    """
    Fail module execution if one of the invalid parameters is not None.

    Parameters
    ----------
    invalid_params:
        List of parameters that must value 'None'.
    state:
        State being tested.
    action:
        Action being tested (optional).
    params:
        Extracted params dict to check (optional, defaults to self.params).
    """
    if action is None:
        msg = "Argument '{0}' can not be used with state '{1}'"
    else:
        msg = "Argument '{0}' can not be used with action '{2}' and state '{1}'"

    _params = params if params is not None else self.params
    for param in invalid_params:
        if _params.get(param) is not None:
            self.fail_json(msg=msg.format(param, state, action))
```

Then add a thin helper that handles the `param_mapping` overlay and delegates to the simple core:

```python
def params_fail_used_invalid_with_mapping(self, invalid_params, state,
                                          action=None, params=None,
                                          param_mapping=None):
    """
    Like params_fail_used_invalid, but honours param_mapping["module_param"].
    """
    # Start from provided params or self.params
    effective = dict(params or self.params)

    if param_mapping:
        for name, meta in param_mapping.items():
            if meta.get("module_param"):
                # module_param values should always come from self.params
                if name in self.params:
                    effective[name] = self.params[name]

    self.params_fail_used_invalid(
        invalid_params,
        state,
        action=action,
        params=effective,
    )
```

Callers that need mapping-aware behavior (e.g. the user module) can use `params_fail_used_invalid_with_mapping`, while existing and future callers can continue using the simpler `params_fail_used_invalid` API. This keeps the commonly-used utility straightforward and localizes the “framework” logic to a dedicated helper.
</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.}

[rjeffman]

What do you think of having a "hidden" attribute to the parameter mapping that would "hide" a field from being queried?
I know we try to mimic CLI behavior, but Ansible may already mask some data.

[rjeffman]

Please, add max-public-methods=25 to [pylint.DESIGN] section of setup.cfg.

[varunmylaraiah]

@t-woerner
Thanks for the PR. I noticed a regression related to Bug #1747413. The user module appears to be throwing an error again when there are no changes to apply.

 PLAYBOOK: user_module.yml ******************************************************
 1 plays in user_module.yml
 
 PLAY [Playbook to ensure a user is present.] ***********************************
 
 TASK [Gathering Facts] *********************************************************
 task path: /root/user_module.yml:2
 ok: [master.ipadomain.test]
 
 TASK [ipauser] *****************************************************************
 task path: /root/user_module.yml:8
 fatal: [master.ipadomain.test]: FAILED! => {"changed": false, "msg": "user_mod: user1747413: no modifications to be performed"}
 
 PLAY RECAP *********************************************************************
 master.ipadomain.test      : ok=1    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0   

[rjeffman]

Without elements=<type>, Ansible will not coerce value into the proper type. As we handle this attribute, I don't think we need to advance ourselves into "fixing" this, and experiment before we actually find any issue.u
(This issue was spotted by an LLM.)

[rjeffman]

Here's another issue that must be addressed.

When executing the task:

  - ipauser:
      users: [{name: alice}, {name: bob}]
      state: absent
      preserve: true

The preserve attribute is ignored.

It still works using name instead of users:

  - ipauser:
      name: [alice, bob]
      state: absent
      preserve: true

[varunmylaraiah]

LGTM

[rjeffman]

LGTM.