On Mon, 2014-05-19 at 08:08 +0200, Lukas Slebodnik wrote:
> On (17/05/14 22:52), Pavel Reichl wrote:
> >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
>
> >dn: name=ub,cn=users,cn=default,cn=sysdb
> ^^^^^^^^
> >dn: name=ub,cn=groups,cn=default,cn=sysdb
> ^^^^^^^^^
> From highlighted parts of DN, it is obvious which one is group.
> This fact will simplify your patches.
Well its obvious to me:-), not sure hoe to code general solution without
doing something overly complex.
>
> 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.
Please be sure to see patch attached in my 2nd mail. I have merged these
two functions into one saving some boilerplate code.
This function utilizes sysdb_search_entry and I find it too specific
to
be moved to sysdb.h. This is the reason why it is a part of sdap module
and hence I thought it is mandatory to use sdap prefix...?
In response to your first mail I have tried your approach and although
it is working, I find my solution a bit more elegant.