On Mon, Sep 21, 2015 at 03:29:20PM +0200, Lukas Slebodnik wrote:
> On (17/09/15 09:38), Jakub Hrozek wrote:
> >On Thu, Sep 17, 2015 at 09:35:11AM +0200, Jakub Hrozek wrote:
> >> > After just only a month since the nack, attached are updated patches.
I
> >> > would prefer to only apply the first one to downstream, since the
others
> >> > are just optimizations and tests.
> >>
> >> CI runs revealed the tests didn't link correctly on Debian. New patches
> >> are attached.
> >
> >Ugh, those were from a wrong branch. Sorry about the spam.
>
> Attached patches fixed the issue and just one user was stored to
> sysdb cache
> >From e07541b9b61778fcd3377388fe105de15898cb16 Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek(a)redhat.com>
> >Date: Wed, 16 Sep 2015 15:28:54 +0200
> >Subject: [PATCH 3/4] LDAP: Move sdap_create_search_base from ldap to sdap code
[...]
> >+ if (!ldb_dn_validate(ldn)) {
> >+ DEBUG(SSSDBG_CRIT_FAILURE,
> >+ "Invalid base DN [%s]\n",
> >+ unparsed_base);
> ^^
> I know that this was also in old file,
> But You can fix extra spaces as part of this change.
The message actually fits to one line.
>
> >+ ret = EINVAL;
> >+ goto done;
> >+ }
> >+
> >+ base->scope = scope;
> >+ base->filter = filter;
> >+
> >+ *_base = talloc_steal(mem_ctx, base);
> >+ ret = EOK;
> >+done:
> >+ talloc_free(tmp_ctx);
> >+ return ret;
> >+}
> >+
> > static errno_t sdap_set_search_base(struct sdap_options *opts,
> > struct sdap_domain *sdom,
> > enum sdap_basic_opt class,
>
> >From 07b62ed6210f25a6b826896c97c8e75e9a6a9867 Mon Sep 17 00:00:00 2001
> >From: Jakub Hrozek <jhrozek(a)redhat.com>
> >Date: Fri, 4 Sep 2015 18:45:45 +0200
> >Subject: [PATCH 4/4] LDAP: Filter out multiple entries when searching
> > overlapping domains
[...]
> >+size_t sdap_copy_objects_in_dom(struct sdap_options *opts,
> >+ struct sysdb_attrs **dom_objects,
> >+ size_t offset,
> >+ struct sss_domain_info *dom,
> >+ struct sysdb_attrs **all_objects,
> >+ size_t count,
> >+ bool filter)
> The name of the function can be confusing
> It says "copy objects in domain" but objects are not just copied
> but also stolen from previous talloc context.
Renamed, the name now says steal and not copy.
> >diff --git a/src/providers/ldap/sdap_async_initgroups.c
b/src/providers/ldap/sdap_async_initgroups.c
> >index
ffb8f7e1f5db5564f45c4f5b7aff46139e64537d..ed0d029f03e5299420ce98423c5abc2fee9b48de 100644
> >--- a/src/providers/ldap/sdap_async_initgroups.c
> >+++ b/src/providers/ldap/sdap_async_initgroups.c
> >@@ -2900,6 +2900,7 @@ static void sdap_get_initgr_user(struct tevent_req
*subreq)
> > expected_basedn_len = strlen(expected_basedn);
> >
> > for (c = 0; c < count; c++) {
> >+ /* FIXME - convert to the new stuff?? */
> Please file also a ticket.
https://fedorahosted.org/sssd/ticket/2798
Sorry about the fixme, it wasn't supposed to be sent to the list :-)
>
> > ret = sysdb_attrs_get_string(usr_attrs[c], SYSDB_ORIG_DN,
&orig_dn);
> > if (ret != EOK) {
> > DEBUG(SSSDBG_OP_FAILURE, "sysdb_attrs_get_string
failed.\n");
> >diff --git a/src/providers/ldap/sdap_async_users.c
b/src/providers/ldap/sdap_async_users.c
> >index
e38f4cd1610e62aa2cf9f4add3a5f7ad5290e748..1d8008731fb2cc8252e3f18d83e05e79c6b3e9bd 100644
> >--- a/src/providers/ldap/sdap_async_users.c
> >+++ b/src/providers/ldap/sdap_async_users.c
> >@@ -617,6 +617,9 @@ struct sdap_search_user_state {
> > };
> >
> > static errno_t sdap_search_user_next_base(struct tevent_req *req);
> >+static void sdap_search_user_copy_batch(struct sdap_search_user_state *state,
> >+ struct sysdb_attrs **users,
> >+ size_t count);
> > static void sdap_search_user_process(struct tevent_req *subreq);
> >
> > struct tevent_req *sdap_search_user_send(TALLOC_CTX *memctx,
> >@@ -728,7 +731,7 @@ static void sdap_search_user_process(struct tevent_req
*subreq)
> > struct sdap_search_user_state *state = tevent_req_data(req,
> > struct sdap_search_user_state);
> > int ret;
> >- size_t count, i;
> >+ size_t count;
> > struct sysdb_attrs **users;
> > bool next_base = false;
> >
> >@@ -762,16 +765,7 @@ static void sdap_search_user_process(struct tevent_req
*subreq)
> > return;
> > }
> >
> >- /* Copy the new users into the list
> >- * They're already allocated on 'state'
> >- */
> >- for (i = 0; i < count; i++) {
> >- state->users[state->count + i] =
> >- talloc_steal(state->users, users[i]);
> >- }
> >-
> >- state->count += count;
> >- state->users[state->count] = NULL;
> >+ sdap_search_user_copy_batch(state, users, count);
> > }
> >
> > if (next_base) {
> >@@ -798,6 +792,25 @@ static void sdap_search_user_process(struct tevent_req
*subreq)
> > tevent_req_done(req);
> > }
> >
> >+static void sdap_search_user_copy_batch(struct sdap_search_user_state *state,
> >+ struct sysdb_attrs **users,
> >+ size_t count)
> >+{
> >+ size_t copied;
> >+ bool filter;
> >+
> >+ /* Always copy all objects for wildcard lookups. */
> >+ filter = state->lookup_type == SDAP_LOOKUP_SINGLE ? true : false;
> >+
> >+ copied = sdap_copy_objects_in_dom(state->opts,
> >+ state->users,
> >+ state->count,
> >+ state->dom,
> >+ users, count, filter);
> >+
> >+ state->count += copied;
> >+ state->users[state->count] = NULL;
> >+}
> >
> > int sdap_search_user_recv(TALLOC_CTX *memctx, struct tevent_req *req,
> > char **higher_usn, struct sysdb_attrs ***users,
> >diff --git a/src/tests/cmocka/test_sdap.c b/src/tests/cmocka/test_sdap.c
> >index
75fc34504bc35e4e4a5fc11bc9d645e78bc2da72..2e11085df29769f970573ec47351951430cec1a8 100644
> >--- a/src/tests/cmocka/test_sdap.c
> >+++ b/src/tests/cmocka/test_sdap.c
> >@@ -941,6 +941,184 @@ static void test_sdap_inherit_option_user(void **state)
> >
talloc_free(test_ctx->child_sdap_opts->user_map[SDAP_AT_USER_PRINC].name);
> > }
> >
> >+struct copy_dom_obj_test_ctx {
> >+ struct sdap_options *opts;
> >+
> >+ struct sss_domain_info *parent;
> >+ struct sss_domain_info *child;
> >+
> >+ struct sdap_domain *parent_sd;
> >+ struct sdap_domain *child_sd;
> >+
> >+ struct sysdb_attrs **ldap_objects;
> >+ struct sysdb_attrs **dom_objects;
> >+};
> >+
> >+static struct sysdb_attrs *test_obj(TALLOC_CTX *mem_ctx,
> >+ const char *name,
> >+ const char *basedn)
> >+{
> >+ errno_t ret;
> >+ const char *orig_dn;
> >+ struct sysdb_attrs *obj;
> >+
> >+ obj = sysdb_new_attrs(mem_ctx);
> >+ assert_non_null(obj);
> >+
> >+ orig_dn = talloc_asprintf(obj, "CN=%s,%s", name, basedn);
> >+ assert_non_null(orig_dn);
> >+
> >+ ret = sysdb_attrs_add_string(obj, SYSDB_ORIG_DN, orig_dn);
> >+ assert_int_equal(ret, EOK);
> >+
> >+ ret = sysdb_attrs_add_string(obj, SYSDB_NAME, name);
> >+ assert_int_equal(ret, EOK);
> >+
> >+ return obj;
> >+}
> >+
> >+static struct sdap_domain *create_sdap_domain(struct sdap_options *opts,
> >+ struct sss_domain_info *dom)
> >+{
> >+ errno_t ret;
> >+ struct sdap_domain *sdom;
> >+
> >+ ret = sdap_domain_add(opts, dom, &sdom);
> >+ assert_int_equal(ret, EOK);
> >+
> >+ sdom->search_bases = talloc_array(sdom, struct sdap_search_base *, 2);
> >+ assert_non_null(sdom->search_bases);
> >+ sdom->search_bases[1] = NULL;
> >+
> >+ ret = sdap_create_search_base(sdom, sdom->basedn,
> >+ LDAP_SCOPE_SUBTREE,
> >+ NULL,
> >+ &sdom->search_bases[0]);
> >+ assert_int_equal(ret, EOK);
> >+
> >+ return sdom;
> >+}
> >+
> >+static int sdap_copy_objects_in_dom_setup(void **state)
> >+{
> >+ struct copy_dom_obj_test_ctx *test_ctx;
> >+
> >+ test_ctx = talloc_zero(NULL,
> >+ struct copy_dom_obj_test_ctx);
> >+ assert_non_null(test_ctx);
> >+
> >+ test_ctx->opts = talloc_zero(test_ctx, struct sdap_options);
> >+ assert_non_null(test_ctx->opts);
> >+
> >+ test_ctx->parent = named_domain(test_ctx, "win.trust.test",
NULL);
> >+ assert_non_null(test_ctx->parent);
> >+
> >+ test_ctx->child = named_domain(test_ctx,
"child.win.trust.test",
> >+ test_ctx->parent);
> >+ assert_non_null(test_ctx->child);
> >+
> >+ test_ctx->parent_sd = create_sdap_domain(test_ctx->opts,
> >+ test_ctx->parent);
> >+ assert_non_null(test_ctx->parent_sd);
> >+
> >+ test_ctx->child_sd = create_sdap_domain(test_ctx->opts,
> >+ test_ctx->child);
> ^
> copy&paste issue :-)
Fixed
>
> >+ assert_non_null(test_ctx->child_sd);
> >+
> >+ /* These two objects were 'returned by LDAP' */
> >+ test_ctx->ldap_objects = talloc_zero_array(test_ctx,
> >+ struct sysdb_attrs *, 2);
> >+ assert_non_null(test_ctx->ldap_objects);
> >+
> >+ test_ctx->ldap_objects[0] = test_obj(test_ctx->ldap_objects,
"parent",
> >+ test_ctx->parent_sd->basedn);
> >+ assert_non_null(test_ctx->ldap_objects[0]);
> >+
> >+ test_ctx->ldap_objects[1] = test_obj(test_ctx->ldap_objects,
"child",
> >+ test_ctx->child_sd->basedn);
> >+ assert_non_null(test_ctx->ldap_objects[1]);
> >+
> >+ /* This is the array we'll filter to */
> >+ test_ctx->dom_objects = talloc_zero_array(test_ctx,
> >+ struct sysdb_attrs *, 2);
> >+ assert_non_null(test_ctx->dom_objects);
> >+
> >+ *state = test_ctx;
> >+ return 0;
> >+}
> >+
> >+static int sdap_copy_objects_in_dom_teardown(void **state)
> >+{
> >+ struct copy_dom_obj_test_ctx *test_ctx = talloc_get_type_abort(*state,
> >+ struct copy_dom_obj_test_ctx);
> >+
> >+ talloc_free(test_ctx);
> >+ return 0;
> >+}
> >+
> >+static void test_sdap_copy_objects_in_dom(void **state)
> >+{
> >+ struct copy_dom_obj_test_ctx *test_ctx = talloc_get_type_abort(*state,
> >+ struct copy_dom_obj_test_ctx);
> >+ size_t count;
> >+
> >+ assert_true(talloc_parent(test_ctx->ldap_objects[0]) == \
> >+ test_ctx->ldap_objects);
> >+ assert_true(talloc_parent(test_ctx->ldap_objects[1]) == \
> >+ test_ctx->ldap_objects);
> >+
> It's better to use assert_ptr_equal for this purpose
> So in cese of error the pointers will be printed to standart error.
Fixed.
>
> The same pattern is also on other places.
I didn't realize we have this function in cmocka. Feel free to send a
patch, so far I only fixed the calls these patches add.
From 571b46e3a9fe477ce3948f06906993c9d9c028be Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Tue, 21 Jul 2015 21:00:27 +0200
Subject: [PATCH 1/4] LDAP: imposing sizelimit=1 for single-entry searches
breaks overlapping domains