Hello Lukas, thanks for comments please see replies inline, new
patches
are attached.
On Fri, 2014-05-16 at 15:34 +0200, Lukas Slebodnik wrote:
> On (13/05/14 00:07), Pavel Reichl wrote:
> >Hello,
> >
> >please see attached patches solving:
> >
> >https://fedorahosted.org/sssd/ticket/2294
> >
> >Thanks,
> >
> >Pavel Reichl
>
> >From 17d8d7c4e53da2291daebfdbbf90d3448df9e765 Mon Sep 17 00:00:00 2001
> >From: Pavel Reichl <preichl(a)redhat.com>
> >Date: Mon, 12 May 2014 15:00:26 +0000
> >Subject: [PATCH 2/3] SDAP: Make nesting_level = 0 to ignore nested groups
> >
> >Make ldap_group_nesting_level = 0 to ignore group nesting entirely.
> >
> >Resolves:
> >https://fedorahosted.org/sssd/ticket/2294
> >---
> > src/providers/ldap/sdap_async_groups.c | 94 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 94 insertions(+)
> >
> >diff --git a/src/providers/ldap/sdap_async_groups.c
b/src/providers/ldap/sdap_async_groups.c
> >index
4240bb585d57a80199d040ac44891ee1b6429e05..b956e55bbf4f60e411a6bcf9f9ad79cf977f7144 100644
> >--- a/src/providers/ldap/sdap_async_groups.c
> >+++ b/src/providers/ldap/sdap_async_groups.c
> >@@ -28,6 +28,86 @@
> > #include "providers/ldap/sdap_idmap.h"
> > #include "providers/ad/ad_common.h"
> >
> >+static errno_t sdap_get_entry_attrs_dn(TALLOC_CTX *mem_ctx,
> >+ struct sysdb_ctx *ctx,
> >+ const char *dn_str,
> >+ const char **attrs,
> >+ struct ldb_message ***_msgs,
> >+ size_t *_num_msgs)
> >+{
> >+ TALLOC_CTX *tmp_ctx;
> >+ struct ldb_dn *base_dn;
> >+ struct ldb_message **msgs;
> >+ size_t num_msgs;
> >+ int ret;
> >+
> >+ tmp_ctx = talloc_new(NULL);
> >+ if (tmp_ctx == NULL) {
> >+ return ENOMEM;
> >+ }
> >+
> >+ base_dn = sysdb_dn(ctx, tmp_ctx, dn_str);
> >+ if (base_dn == NULL) {
> >+ ret = ENOMEM;
> >+ goto done;
> >+ }
> >+
> >+ DEBUG(SSSDBG_TRACE_ALL, "Searching cache for [%s].\n", dn_str);
> >+ ret = sysdb_search_entry(tmp_ctx, ctx, base_dn, LDB_SCOPE_BASE,
"", attrs,
> >+ &num_msgs, &msgs);
> >+ if (ret != EOK) {
> >+ DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_entry failed: [%d]:
%s\n",
> >+ ret, strerror(ret));
> >+ goto done;
> >+ }
> >+
> >+ *_msgs = talloc_steal(mem_ctx, msgs);
> >+ *_num_msgs = num_msgs;
> >+
> >+done:
> >+ talloc_zfree(tmp_ctx);
> >+ return ret;
> >+}
> >+
> >+static errno_t sdap_is_member_type_group(struct sysdb_ctx *sctx,
> >+ const char* sysdb_member_dn,
> >+ bool *isGroup)
> ^^^^^^^^^^^^^^
>
http://www.freeipa.org/page/Coding_Style#Naming
> HIGHLY RECOMMENDED: Use low case multi word underscore separated notation for
> naming variables.
>
> We have also another "SSSD rule" that output parameter start with
underscore
Fixed.
> >+{
> >+ struct ldb_message **msgs;
> >+ size_t num_msgs;
> >+ const char *attrs[] = {SYSDB_OBJECTCLASS, NULL};
> >+ TALLOC_CTX *tmp_ctx;
> >+ errno_t ret;
> >+ const char *object_class;
> >+
> >+ tmp_ctx = talloc_new(NULL);
> >+ if (tmp_ctx == NULL) {
> >+ return ENOMEM;
> >+ }
> >+ ret = sdap_get_entry_attrs_dn(tmp_ctx, sctx, sysdb_member_dn, attrs,
&msgs,
> >+ &num_msgs);
> >+ if (ret == EOK) {
> >+ if (num_msgs != 1) {
> >+ DEBUG(SSSDBG_OP_FAILURE,
> >+ "Expected 1 value for %s, got %zu\n", attrs[0],
num_msgs);
> >+ ret = EINVAL;
> >+ goto done;
> >+ }
> >+
> >+ object_class = ldb_msg_find_attr_as_string(msgs[0],
> >+ SYSDB_OBJECTCLASS, NULL);
> >+ if (strcmp(object_class, SYSDB_GROUP_CLASS) == 0) {
> >+ *isGroup = true;
> >+ } else {
> >+ *isGroup = false;
> >+ }
> >+ }
> >+
> >+done:
> >+ talloc_free(tmp_ctx);
> >+ return ret;
> >+}
> >+
> > /* ==Group-Parsing Routines=============================================== */
> >
> > static int sdap_find_entry_by_origDN(TALLOC_CTX *memctx,
> >@@ -1211,6 +1291,10 @@ sdap_process_group_members_2307bis(struct tevent_req
*req,
> > char *strdn;
> > int ret;
> > int i;
> >+ int nesting_level;
> >+ bool isMemberGroup = false;
> >+
> >+ nesting_level = dp_opt_get_int(state->opts->basic,
SDAP_NESTING_LEVEL);
> >
> > for (i=0; i < memberel->num_values; i++) {
> > member_dn = (char *)memberel->values[i].data;
> >@@ -1220,7 +1304,17 @@ sdap_process_group_members_2307bis(struct tevent_req
*req,
> > state->dom,
> > member_dn,
> > &strdn);
> >+
> > if (ret == EOK) {
> >+ if (nesting_level == 0) {
> >+ /* Ignore group members which are groups themselves. */
> >+ ret = sdap_is_member_type_group(state->sysdb, strdn,
> >+ &isMemberGroup);
> I think you reimplemented already existing sysdb functions.
> I think that aim is to find out if "strdn" is group.
>
> strdn is already "sysdb DN" which can be used for function sysdb_search_*
well actually strdn is pointer to char, which can't be directly used by
sysdb_search_*, because they expect ldb_dn.
> You can obtain groupname from dn with function sysdb_group_dn_name
> hint: test_sysdb_group_dn_name
> and then you can find group with the function sysdb_search_group_by_name
You are right, but AFIK sysdb_group_dn_nane just returns RDN from DN,
which will cause problems:
Thank you for pasting next two ldb DNs. They are vere helpful
From highlighted parts of DN, it is obvious which one is group.
This fact will simplify your patches.
LS
Side note:
You wrote two functions with prefix "sdap_", but they are not related to LDAP
(sdap is abbreviation of sss LDAP). They are "sysdb" functions with
confusing name.
LS