Generalize combined user and group lookup #8127
Conversation
Explanation Resolves: https://github.com/SSSD/sssd/issues/XXXX
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring to generalize the lookup of LDAP objects, particularly for user and group resolution within nested groups. The new sdap_get_and_multi_parse_generic functions provide a flexible way to handle multiple object types using different attribute maps, which simplifies the nested group processing logic considerably.
While the generalization is a good improvement, I've identified a few issues that should be addressed:
- A potential bug in objectClass matching that could lead to incorrect map selection.
- Two performance regressions in the nested group handling due to the removal of an IPA-specific optimization and fetching all attributes (
*) instead of specific ones. - A compilation error in the test mocks due to a mismatched function signature.
| struct tevent_req *sdap_get_and_multi_parse_generic_send(TALLOC_CTX *mem_ctx, | ||
| 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 *map, | ||
| int map_num_attrs, | ||
| int timeout, | ||
| bool allow_paging) | ||
| { | ||
| return test_req_succeed_send(mem_ctx, ev); | ||
| } |
There was a problem hiding this comment.
The signature of the mocked sdap_get_and_multi_parse_generic_send function does not match the actual function declaration in src/providers/ldap/sdap_async.h. This will cause a compilation error. Please correct the function signature to match the declaration.
struct tevent_req *sdap_get_and_multi_parse_generic_send(TALLOC_CTX *mem_ctx,
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)
{
return test_req_succeed_send(mem_ctx, ev);
}| if (strncasecmp(state->maps[mi].map[0].name, | ||
| vals[i]->bv_val, vals[i]->bv_len) == 0) { |
There was a problem hiding this comment.
The strncasecmp with bv_len can lead to incorrect prefix matches. For example, if an objectClass from the server is "user" and the map expects "userAccount", this will be considered a match, which is incorrect. You should also compare the lengths of the strings to ensure an exact, case-insensitive 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) {| sdap_nested_group_lookup_member_send(TALLOC_CTX *mem_ctx, | ||
| struct tevent_context *ev, | ||
| struct sdap_nested_group_ctx *group_ctx, | ||
| struct sdap_nested_group_member *member) |
There was a problem hiding this comment.
The previous implementation included an optimization for IPA schemas (SDAP_SCHEMA_IPA_V1) that would attempt to parse user information from the DN directly using sdap_nested_group_get_ipa_user, avoiding an LDAP lookup. This refactoring has removed that optimization, which will result in a performance regression for IPA environments as it will now always perform an LDAP lookup for every member. Was this intentional? If not, this optimization should be restored.
| attrs = talloc_array(state, const char *, 3); | ||
| if (attrs == NULL) { | ||
| ret = ENOMEM; | ||
| goto immediately; | ||
| } | ||
|
|
||
| attrs[0] = "objectClass"; | ||
| attrs[1] = group_ctx->opts->user_map[SDAP_AT_USER_NAME].name; | ||
| attrs[0] = "objectclass"; | ||
| attrs[1] = "*"; | ||
| attrs[2] = NULL; |
There was a problem hiding this comment.
The new implementation requests all attributes (*) for every member lookup. The previous implementation was more specific, requesting only objectClass and the user name attribute for user lookups. Fetching all attributes can be inefficient and increase network traffic, especially in environments with large user objects and deep group nesting. Consider constructing a specific attribute list based on the user and group maps to fetch only the necessary attributes.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| ret = sdap_get_generic_recv(subreq, state, &count, &group); | ||
| ret = sdap_get_and_multi_parse_generic_recv(subreq, state, &count, &member); | ||
| //ret = sdap_get_generic_recv(subreq, state, &count, &member); |
Check notice
Code scanning / CodeQL
Commented-out code Note
Hi Gemini,
|
/gemini see quoted message |
|
Hi Alexey, Thanks for reaching out and for the follow-up questions! Regarding your first question about the IPAv1 regression:
Regarding your second question about fetching all attributes (
|
@sumit-bose , can you comment on this one please?
/gemini |
|
Hi Ondrej, Thanks for the follow-up! You're right, my previous response explained the concept but didn't provide a direct code suggestion for the Regarding your use of Now, for the functional patch you requested for To address the performance issue of fetching all attributes (
Here is a code suggestion that implements these changes. This replaces the hardcoded // In src/providers/ldap/sdap_async_nested_groups.c
// Add this include at the top of the file, or where other includes are present
// #include "providers/ad/ad_opts.h"
// ... inside sdap_nested_group_lookup_member_send function
// (lines 1705-1739 in the right-hand side of DIFF 8)
size_t num_maps = 3; // Changed from 2 to 3
req = tevent_req_create(mem_ctx, &state,
struct sdap_nested_group_lookup_member_state);
if (req == NULL) {
DEBUG(SSSDBG_CRIT_FAILURE, "tevent_req_create() failed\n");
return NULL;
}
PROBE(SDAP_NESTED_GROUP_LOOKUP_USER_SEND);
/* Request specific attributes needed for parsing users, groups, and FSPs */
attrs = talloc_array(state, const char *,
1 + SDAP_OPTS_USER + SDAP_OPTS_GROUP + SDAP_OPTS_FSP + 1); // objectclass + all map attributes + NULL terminator
if (attrs == NULL) {
ret = ENOMEM;
goto immediately;
}
int attr_idx = 0;
attrs[attr_idx++] = "objectclass"; // Always request objectclass
// Collect attributes from user_map
for (int i = 0; i < SDAP_OPTS_USER; i++) {
if (group_ctx->opts->user_map[i].ldap_name != NULL) {
attrs[attr_idx++] = group_ctx->opts->user_map[i].ldap_name;
}
}
// Collect attributes from group_map
for (int i = 0; i < SDAP_OPTS_GROUP; i++) {
if (group_ctx->opts->group_map[i].ldap_name != NULL) {
attrs[attr_idx++] = group_ctx->opts->group_map[i].ldap_name;
}
}
// Collect attributes from ad_fsp_map
for (int i = 0; i < SDAP_OPTS_FSP; i++) {
if (ad_fsp_map[i].ldap_name != NULL) {
attrs[attr_idx++] = ad_fsp_map[i].ldap_name;
}
}
attrs[attr_idx] = NULL; // Null-terminate the array
maps = talloc_array(state, struct sdap_attr_map_info, num_maps + 1);
if (maps == NULL) {
DEBUG(SSSDBG_OP_FAILURE,
"Failed to allocate memory for attribute maps.\n");
ret = ENOMEM;
goto immediately;
}
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;
maps[2].map = ad_fsp_map; // Added ad_fsp_map
maps[2].num_attrs = SDAP_OPTS_FSP; // Added FSP attribute count
maps[3].map = NULL; // Null-terminate the maps array
maps[3].num_attrs = 0;
/* create filter */
base_filter = talloc_asprintf(state, "(|(objectclass=%s)(objectclass=%s)(objectclass=%s))",
group_ctx->opts->user_map[SDAP_OC_USER].name,
SYSDB_AD_FSP_CLASS,
group_ctx->opts->group_map[SDAP_OC_GROUP].name);
if (base_filter == NULL) {
ret = ENOMEM;
goto immediately;
}This suggestion provides a more precise list of attributes to fetch, which should resolve the performance regression related to fetching all attributes ( Let me know if you have any further questions or need additional adjustments! |
Hi, this is somewhat wrong because that LDAP Schema we are interested in did not change between the IPA versions, so sill The check can even be kept because although the call is now not specific to users anymore Btw, I didn't looked at the comments from Gemini so far because this is currently only a draft and I just would like to understand if it is already working for you before polishing it and e.g. actually use HTH bye, |
Would it be possible to give me RH-9 repo for this as my test machine is running RHEL-9? |
Hint: if one opens this same PR against sssd-2-9 branch then CI should build C9S package that should be installable on the latest RHEL9 |
RHEL-10 will do as well - unfortunately the given F41 repo is still too recent even for RH10 |
centos-stream-10 is built as part of CI for this PR: |
|
Hi, I opened #8131 with a backport to sssd-2.9 and the related C9S build can be found at https://copr.fedorainfracloud.org/coprs/g/sssd/pr8131/. bye, |
|
Sorry gents, it does not work for me: but I only see Similar error I get on RHEL-10. Looks like Centos-stream is not really compatible with RHEL of the same version (which is quite surprising) |
Ah, that's because 9.6 isn't latest, 9.7 is (but it is not yet released). https://kojihub.stream.centos.org/kojifiles/repos/c9s-build/latest/x86_64/pkglist - |
|
I tested this on my test machine, it only seems to work with @sumit-bose , can you add the |
pbrezina
force-pushed
the
master
branch
2 times, most recently
from
f5d64b3 to
b854636
Compare
|
/gemini can you suggest a patch to eliminate the 2nd parsing of objectclass - we identified it in sdap_get_and_multi_parse_generic_parse_entry() already so I guess we don't have to check twice |
|
Hi Ondrej, You've raised an excellent point about optimizing the objectClass parsing. You're right that the type of entry (user, group, or Foreign Security Principal) is already determined within To address this, I've prepared a patch that stores the index of the matched attribute map directly within the Here's the suggested patch: --- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -64,6 +64,7 @@
#define SYSDB_DOMAIN_ID_RANGE_CLASS "domainIDRange"
#define SYSDB_TRUSTED_AD_DOMAIN_RANGE_CLASS "TrustedADDomainRange"
#define SYSDB_CERTMAP_CLASS "certificateMappingRule"
+#define SYSDB_MATCHED_MAP_INDEX_STR "matchedMapIndex"
#define SYSDB_AD_FSP_CLASS "foreignSecurityPrincipal"
#define SYSDB_DN "dn"--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -2119,6 +2119,17 @@
"Matched objectclass [%s] on DN [%s], will use associated map\n",
state->maps[mi].map[0].name, dn);
map = state->maps[mi].map;
num_attrs = state->maps[mi].num_attrs;
+
+ // Store the index of the matched map for direct type determination by callers
+ char *index_str = talloc_asprintf(state, "%d", mi);
+ if (index_str == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ ret = sysdb_attrs_add_string(attrs, SYSDB_MATCHED_MAP_INDEX_STR, index_str);
+ talloc_free(index_str); // sysdb_attrs_add_string makes a copy
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, "Failed to add matched map index string.\n");
+ goto done;
+ }
break;
}
}--- a/src/providers/ldap/sdap_async_nested_groups.c
+++ b/src/providers/ldap/sdap_async_nested_groups.c
@@ -1863,39 +1863,59 @@
*_entry = talloc_steal(mem_ctx, state->member);
sysdb_attrs_get_string(state->member, SYSDB_ORIG_DN, &val);
- /* Figure out what we got here */
- ret = sysdb_attrs_get_string_array(state->member, SYSDB_OBJECTCLASS,
- mem_ctx->group_ctx, &val_list);
- if (ret == EOK) {
- struct sdap_attr_map *user_map, *group_map;
-
- user_map = mem_ctx->group_ctx->opts->user_map;
- group_map = mem_ctx->group_ctx->opts->group_map;
-
- if (string_in_list(SYSDB_AD_FSP_CLASS, discard_const(val_list), false)) {
-
- *_type = SDAP_NESTED_GROUP_DN_FSP;
-
- } else if (string_in_list(user_map[SDAP_OC_USER].name,
- discard_const(val_list), false)) {
- DEBUG(SSSDBG_TRACE_ALL, "%s is User\n", val);
- /* if we expected a group by a filter, we return NULL */
- if (*_type == SDAP_NESTED_GROUP_DN_GROUP )
- *_entry = NULL;
- *_type = SDAP_NESTED_GROUP_DN_USER;
-
- } else if (string_in_list(group_map[SDAP_OC_GROUP].name,
- discard_const(val_list), false)) {
- DEBUG(SSSDBG_TRACE_ALL, "%s is Group\n", val);
- if (*_type == SDAP_NESTED_GROUP_DN_USER )
- *_entry = NULL;
- *_type = SDAP_NESTED_GROUP_DN_GROUP;
-
- } else {
- DEBUG(SSSDBG_TRACE_ALL, "unexpected object %s??\n", val);
- *_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
- }
- talloc_free(val_list);
+ enum sdap_nested_group_dn_type found_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
+
+ /* Try to determine type directly from matched map index if available */
+ const char *matched_map_index_str = NULL;
+ ret = sysdb_attrs_get_string(state->member, SYSDB_MATCHED_MAP_INDEX_STR,
+ &matched_map_index_str);
+ if (ret == EOK && matched_map_index_str != NULL) {
+ int matched_map_index = atoi(matched_map_index_str);
+ if (matched_map_index == 0) { /* user_map */
+ found_type = SDAP_NESTED_GROUP_DN_USER;
+ } else if (matched_map_index == 1) { /* group_map */
+ found_type = SDAP_NESTED_GROUP_DN_GROUP;
+ } else if (matched_map_index == 2) { /* ad_fsp_map */
+ found_type = SDAP_NESTED_GROUP_DN_FSP;
+ } else {
+ DEBUG(SSSDBG_TRACE_ALL, "Unknown matched map index [%d] for DN [%s]\n",
+ matched_map_index, val);
+ found_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
+ }
} else {
- DEBUG(SSSDBG_TRACE_ALL, "can't find objectclass for %s??\n", val);
- *_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
+ /* Fallback to checking objectClass strings if index is not available */
+ DEBUG(SSSDBG_TRACE_ALL, "Matched map index not found, falling back to objectClass check for DN [%s]\n", val);
+ ret = sysdb_attrs_get_string_array(state->member, SYSDB_OBJECTCLASS,
+ mem_ctx->group_ctx, &val_list);
+ if (ret == EOK) {
+ struct sdap_attr_map *user_map, *group_map;
+
+ user_map = mem_ctx->group_ctx->opts->user_map;
+ group_map = mem_ctx->group_ctx->opts->group_map;
+
+ if (string_in_list(SYSDB_AD_FSP_CLASS, discard_const(val_list), false)) {
+ found_type = SDAP_NESTED_GROUP_DN_FSP;
+ } else if (string_in_list(user_map[SDAP_OC_USER].name,
+ discard_const(val_list), false)) {
+ DEBUG(SSSDBG_TRACE_ALL, "%s is User\n", val);
+ found_type = SDAP_NESTED_GROUP_DN_USER;
+ } else if (string_in_list(group_map[SDAP_OC_GROUP].name,
+ discard_const(val_list), false)) {
+ DEBUG(SSSDBG_TRACE_ALL, "%s is Group\n", val);
+ found_type = SDAP_NESTED_GROUP_DN_GROUP;
+ } else {
+ DEBUG(SSSDBG_TRACE_ALL, "unexpected object %s??\n", val);
+ found_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
+ }
+ talloc_free(val_list);
+ } else {
+ DEBUG(SSSDBG_TRACE_ALL, "can't find objectclass for %s??\n", val);
+ found_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
+ }
+ }
+
+ /* Apply filtering based on expected type */
+ if (found_type == SDAP_NESTED_GROUP_DN_USER && *_type == SDAP_NESTED_GROUP_DN_GROUP) {
+ *_entry = NULL;
+ } else if (found_type == SDAP_NESTED_GROUP_DN_GROUP && *_type == SDAP_NESTED_GROUP_DN_USER) {
+ *_entry = NULL;
+ }
+ *_type = found_type;
+
return ret;This patch introduces a new This approach avoids the redundant string comparisons for type determination, making the process more efficient as requested. Please let me know if this patch works for you or if you have any further questions! |
|
Ok that looks good, but what's the point of fallback check? I mean it seems to me that if we could not determine the index map in attributes, then we can safely assume the attribute is not parse-able, right? |
JFTR: gemini only reads comments if explicitly tagged via '/' |
|
/gemini see my reply above, instead of just |
|
Hi Ondrej, Thanks for the detailed follow-up questions and for pointing out the cumbersomeness of the string conversion. You've identified areas where we can definitely improve the clarity and efficiency of the code. Regarding your first question about the fallback check:
Regarding your second question about avoiding string conversion:
I've prepared a revised patch that implements this. It introduces a new structure, Here are the proposed changes: --- a/src/providers/ldap/sdap_async.h
+++ b/src/providers/ldap/sdap_async.h
@@ -262,6 +262,11 @@
TALLOC_CTX *mem_ctx, size_t *reply_count,
struct sysdb_attrs ***reply_list);
+struct sdap_parsed_entry_info {
+ struct sysdb_attrs *attrs;
+ int matched_map_index;
+};
+
struct tevent_req *
sdap_get_and_multi_parse_generic_send(TALLOC_CTX *memctx,
struct tevent_context *ev,
@@ -278,7 +283,7 @@
int timeout,
bool allow_paging);
int sdap_get_and_multi_parse_generic_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
size_t *reply_count,
- struct sysdb_attrs ***reply);
+ struct sdap_parsed_entry_info ***reply);
bool sdap_has_deref_support_ex(struct sdap_handle *sh,
struct sdap_options *opts,--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1999,7 +1999,8 @@
struct sdap_attr_map_info *maps;
size_t num_maps;
- struct sdap_reply sreply;
+ struct sdap_parsed_entry_info **parsed_entries;
+ size_t parsed_entries_count;
struct sdap_options *opts;
};
@@ -2119,17 +2120,20 @@
for (mi =0; mi < state->num_maps; mi++) {
map = NULL;
for (i = 0; vals[i]; i++) {
/* the objectclass is always the first name in the map */
if (strncasecmp(state->maps[mi].map[0].name,
vals[i]->bv_val, vals[i]->bv_len) == 0) {
/* it's an entry of the right type */
DEBUG(SSSDBG_TRACE_INTERNAL,
"Matched objectclass [%s] on DN [%s], will use associated map\n",
state->maps[mi].map[0].name, dn);
map = state->maps[mi].map;
num_attrs = state->maps[mi].num_attrs;
+ // The matched map index 'mi' is now stored directly in sdap_parsed_entry_info
+ // and no longer needs to be added as a string attribute.
break;
}
}
if (!map) {
DEBUG(SSSDBG_TRACE_INTERNAL,
"DN [%s] did not match the objectClass [%s]\n",
dn, state->maps[mi].map[0].name);
@@ -2150,11 +2154,29 @@
}
break;
}
ldap_value_free_len(vals);
/* If some mapped entry was found, add to to the reply */
if (attrs != NULL) {
- ret = add_to_reply(state, &state->sreply, attrs);
+ struct sdap_parsed_entry_info *entry_info;
+
+ entry_info = talloc(state, struct sdap_parsed_entry_info);
+ if (entry_info == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ entry_info->attrs = talloc_steal(entry_info, attrs);
+ entry_info->matched_map_index = mi; // Store the matched index directly
+
+ // Dynamically grow the array of parsed entries
+ struct sdap_parsed_entry_info **new_array;
+ size_t new_count = state->parsed_entries_count + 1;
+
+ new_array = talloc_realloc(state, state->parsed_entries,
+ struct sdap_parsed_entry_info *, new_count + 1);
+ if (new_array == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ state->parsed_entries = new_array;
+ state->parsed_entries[state->parsed_entries_count] = entry_info;
+ state->parsed_entries_count = new_count;
+ state->parsed_entries[state->parsed_entries_count] = NULL; /* Null terminate */
+
+ ret = EOK; // Ensure ret is EOK if successful
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, "add_to_reply failed.\n");
goto done;
}
}
ret = EOK;
@@ -2174,10 +2196,10 @@
int sdap_get_and_multi_parse_generic_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
size_t *reply_count,
- struct sysdb_attrs ***reply)
+ struct sdap_parsed_entry_info ***reply)
{
struct sdap_get_and_multi_parse_generic_state *state = tevent_req_data(req,
struct sdap_get_and_multi_parse_generic_state);
TEVENT_REQ_RETURN_ON_ERROR(req);
- *reply_count = state->sreply.reply_count;
- *reply = talloc_steal(mem_ctx, state->sreply.reply);
+ *reply_count = state->parsed_entries_count;
+ *reply = talloc_steal(mem_ctx, state->parsed_entries);
return EOK;
}--- a/src/providers/ldap/sdap_async_nested_groups.c
+++ b/src/providers/ldap/sdap_async_nested_groups.c
@@ -1685,7 +1685,7 @@
}
struct sdap_nested_group_lookup_member_state {
- struct sysdb_attrs *member;
+ struct sdap_parsed_entry_info *member_info;
};
static void sdap_nested_group_lookup_member_done(struct tevent_req *subreq);
@@ -1796,10 +1796,10 @@
struct tevent_req *req = NULL;
struct sysdb_attrs **member = NULL;
size_t count = 0;
errno_t ret;
req = tevent_req_callback_data(subreq, struct tevent_req);
state = tevent_req_data(req, struct sdap_nested_group_lookup_member_state);
- ret = sdap_get_and_multi_parse_generic_recv(subreq, state, &count, &member);
+ ret = sdap_get_and_multi_parse_generic_recv(subreq, state, &count, &parsed_entries);
//ret = sdap_get_generic_recv(subreq, state, &count, &member);
talloc_zfree(subreq);
if (ret == ENOENT) {
count = 0;
@@ -1815,10 +1815,10 @@
}
if (count == 1) {
- state->member = member[0];
+ state->member_info = talloc_steal(state, parsed_entries[0]);
} else if (count == 0) {
/* group not found */
- state->member = NULL;
+ state->member_info = NULL;
} else {
DEBUG(SSSDBG_OP_FAILURE,
"BASE search returned more than one records\n");
@@ -1837,64 +1837,46 @@
static errno_t
sdap_nested_group_lookup_recv(struct sdap_nested_group_single_state *mem_ctx,
struct tevent_req *req,
struct sysdb_attrs **_entry,
enum sdap_nested_group_dn_type *_type)
{
const char *val = NULL;
- const char **val_list = NULL;
errno_t ret = EOK;
struct sdap_nested_group_lookup_member_state *state = NULL;
+ enum sdap_nested_group_dn_type expected_type = *_type;
+ enum sdap_nested_group_dn_type found_type;
state = tevent_req_data(req, struct sdap_nested_group_lookup_member_state);
PROBE(SDAP_NESTED_GROUP_LOOKUP_UNKNOWN_RECV);
TEVENT_REQ_RETURN_ON_ERROR(req);
- if (state->member == NULL) {
+ if (state->member_info == NULL || state->member_info->attrs == NULL) {
*_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
*_entry = NULL;
return EOK;
}
- *_entry = talloc_steal(mem_ctx, state->member);
- sysdb_attrs_get_string(state->member, SYSDB_ORIG_DN, &val);
-
- /* Figure out what we got here */
- ret = sysdb_attrs_get_string_array(state->member, SYSDB_OBJECTCLASS,
- mem_ctx->group_ctx, &val_list);
- if (ret == EOK) {
- struct sdap_attr_map *user_map, *group_map;
-
- user_map = mem_ctx->group_ctx->opts->user_map;
- group_map = mem_ctx->group_ctx->opts->group_map;
-
- if (string_in_list(SYSDB_AD_FSP_CLASS, discard_const(val_list), false)) {
-
- *_type = SDAP_NESTED_GROUP_DN_FSP;
-
- } else if (string_in_list(user_map[SDAP_OC_USER].name,
- discard_const(val_list), false)) {
- DEBUG(SSSDBG_TRACE_ALL, "%s is User\n", val);
- /* if we expected a group by a filter, we return NULL */
- if (*_type == SDAP_NESTED_GROUP_DN_GROUP )
- *_entry = NULL;
- *_type = SDAP_NESTED_GROUP_DN_USER;
-
- } else if (string_in_list(group_map[SDAP_OC_GROUP].name,
- discard_const(val_list), false)) {
- DEBUG(SSSDBG_TRACE_ALL, "%s is Group\n", val);
- if (*_type == SDAP_NESTED_GROUP_DN_USER )
- *_entry = NULL;
- *_type = SDAP_NESTED_GROUP_DN_GROUP;
-
- } else {
- DEBUG(SSSDBG_TRACE_ALL, "unexpected object %s??\n", val);
- *_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
- }
- talloc_free(val_list);
- } else {
- DEBUG(SSSDBG_TRACE_ALL, "can't find objectclass for %s??\n", val);
- *_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
+ *_entry = talloc_steal(mem_ctx, state->member_info->attrs);
+ sysdb_attrs_get_string(state->member_info->attrs, SYSDB_ORIG_DN, &val);
+
+ int matched_map_index = state->member_info->matched_map_index;
+ if (matched_map_index == 0) { /* user_map */
+ found_type = SDAP_NESTED_GROUP_DN_USER;
+ } else if (matched_map_index == 1) { /* group_map */
+ found_type = SDAP_NESTED_GROUP_DN_GROUP;
+ } else if (matched_map_index == 2) { /* ad_fsp_map */
+ found_type = SDAP_NESTED_GROUP_DN_FSP;
+ } else {
+ DEBUG(SSSDBG_TRACE_ALL, "Unknown matched map index [%d] for DN [%s]\n",
+ matched_map_index, val);
+ found_type = SDAP_NESTED_GROUP_DN_UNKNOWN;
+ }
+
+ /* Apply filtering based on expected type */
+ if (found_type == SDAP_NESTED_GROUP_DN_USER && expected_type == SDAP_NESTED_GROUP_DN_GROUP) {
+ *_entry = NULL;
+ } else if (found_type == SDAP_NESTED_GROUP_DN_GROUP && expected_type == SDAP_NESTED_GROUP_DN_USER) {
+ *_entry = NULL;
}
+ *_type = found_type;
return ret;
}These changes provide a more robust and efficient way to determine the entry type by directly passing the matched map index, avoiding redundant string parsing. Please let me know if this revised approach works for you! |
|
Sumit, The changes Gemini proposed seems OK to me, shall I try to see how it works? |
3 participants
No description provided.