Generalize combined user and group lookup - backport 2.9

[sumit-bose]

No description provided.

[gemini-code-assist]

Code Review

This pull request introduces a generalized mechanism for looking up users and groups in a single LDAP search, which is a great simplification. The implementation looks mostly solid, but I've found a few issues related to correctness and memory management that should be addressed. Additionally, a critical issue in the test mocks will prevent the test suite from compiling.

[gemini-code-assist]

The signature of the mock function sdap_get_and_multi_parse_generic_send does not match the actual function declaration in src/providers/ldap/sdap_async.h. This will cause a compilation failure in the test suite. The mock function seems to be a copy-paste of sdap_get_generic_send's mock.

struct tevent_req *sdap_get_and_multi_parse_generic_send(TALLOC_CTX *memctx,
                                         struct tevent_context *ev,
                                         struct sdap_options *opts,
                                         struct sdap_handle *sh,
                                         const char *search_base,
                                         int scope,
                                         const char *filter,
                                         const char **attrs,
                                         struct sdap_attr_map_info *maps,
                                         size_t num_maps,
                                         int attrsonly,
                                         LDAPControl **serverctrls,
                                         LDAPControl **clientctrls,
                                         int sizelimit,
                                         int timeout,
                                         bool allow_paging)

[gemini-code-assist]

The strncasecmp call compares up to bv_len characters but doesn't ensure that the lengths of both strings are equal. This can lead to incorrect partial matches. For example, if map[0].name is "groupOfNames" and vals[i]->bv_val is "group" with bv_len of 5, the comparison would succeed, leading to incorrect object type identification.

You should check that the lengths are equal before comparing the content to ensure an exact match.

            if (strlen(state->maps[mi].map[0].name) == vals[i]->bv_len &&
                strncasecmp(state->maps[mi].map[0].name,
                            vals[i]->bv_val, vals[i]->bv_len) == 0) {

[gemini-code-assist]

The num_maps is hardcoded to 2, which only includes user and group maps. However, the LDAP filter is constructed to also search for Foreign Security Principals (foreignSecurityPrincipal). Without providing the ad_fsp_map in the maps array, FSP objects will not be parsed correctly, and the logic to identify and ignore them will fail.

You should conditionally increase num_maps for AD schemas and include ad_fsp_map. This will require making ad_fsp_map and SDAP_OPTS_FSP available, for example by adding extern declarations at the top of the file.

Example of how to fix this in sdap_nested_group_lookup_member_send:

// At top of file:
// #include "providers/ad/ad_opts.h" // Or extern declarations

// In sdap_nested_group_lookup_member_send:
size_t num_maps = 2;
if (group_ctx->opts->schema_type == SDAP_SCHEMA_AD) {
    num_maps = 3;
}

maps = talloc_array(state, struct sdap_attr_map_info, num_maps + 1);
if (maps == NULL) {
    // ... error handling
}

maps[0].map = group_ctx->opts->user_map;
maps[0].num_attrs = SDAP_OPTS_USER;
maps[1].map = group_ctx->opts->group_map;
maps[1].num_attrs = SDAP_OPTS_GROUP;

if (num_maps == 3) {
    maps[2].map = ad_fsp_map;
    maps[2].num_attrs = SDAP_OPTS_FSP;
}

maps[num_maps].map = NULL;
maps[num_maps].num_attrs = 0;

[gemini-code-assist]

The val_list is allocated on mem_ctx->group_ctx, which is a long-lived context. While talloc_free(val_list) is called later, it's better practice to allocate temporary data on a shorter-lived context. The mem_ctx parameter itself (struct sdap_nested_group_single_state *) is a more appropriate context for this temporary allocation.

                                       mem_ctx, &val_list);