On Tue, 2013-11-26 at 18:25 +0100, Jakub Hrozek wrote: > On Tue, Nov 26, 2013 at 05:28:18PM +0100, Pavel Reichl wrote: > > Hi Jakub, > > > > thanks for review. I have just a few little remarks, please see inline. > > > > On Tue, 2013-11-26 at 15:23 +0100, Jakub Hrozek wrote: > > > On Tue, Nov 19, 2013 at 10:14:49PM +0100, Pavel Reichl wrote: > > > > On Fri, 2013-11-15 at 21:01 +0100, Jakub Hrozek wrote: > > > > > On Fri, Nov 15, 2013 at 11:15:53AM +0100, Pavel Reichl wrote: > > > > > > Hello, > > > > > > > > > > > > First patch improves domain detection taking matched length into > > > > > > account > > > > > > as proposed and elaborated by Jakub. > > > > > > > > > > > > Second patch is a simple unit test testing > > > > > > sss_ldap_dn_in_search_bases > > > > > > and sdap_domain_get_by_dn. > > > > > > > > > > > > Pavel Reichl > > > > > > > > > > Hi Pavel, > > > > > > > > > > I haven't really read the full patches yet, just scrolled through > > > > > them, > > > > > so I have only one comment so far: > > > > > > > > > > > +++ b/src/tests/cmocka/test_search_bases.c > > > > > > @@ -0,0 +1,214 @@ > > > > > > +/* > > > > > > + SSSD > > > > > > + > > > > > > + find_uid - Utilities tests > > > > > > + > > > > > > + Authors: > > > > > > + Abhishek Singh <abhishekkumarsingh....@gmail.com> > > > > > > + > > > > > > + Copyright (C) 2013 Red Hat > > > > > > > > > > You should credit yourself :-) > > > > > > > > Hi Jakub, > > > > > > > > you are right, thanks for noticing. > > > > > > Hi, in general the patches are working well and looking good. I only have > > > a couple of comments, see inline. > > > > > > > From c7632a2310442cfc10708c5728bd63e6a8c916d2 Mon Sep 17 00:00:00 2001 > > > > From: Pavel Reichl <pavel.rei...@redhat.com> > > > > Date: Thu, 14 Nov 2013 21:34:51 +0000 > > > > Subject: [PATCH 1/2] SSSD: Improved domain detection > > > > > > > > A bit more elegant way of detection of what domain the group member > > > > belongs to > > > > > > > > Resolves: > > > > https://fedorahosted.org/sssd/ticket/2132 > > > > > > I only have code style comments about this patch. > > > > > > > + tmp_ctx = talloc_new(NULL); > > > > + if (tmp_ctx == NULL) > > > > + return NULL; > > > > > > We use brackets around single-line conditionals as well. > > > > OK, I have no trouble fixing this, but accordingly to > > http://www.freeipa.org/page/Coding_Style#IF_Statements > > I got an idea that statements like: > > > > if (foo) > > bar(); > > else > > baz(); > > > > are legal so I assumed that: > > > > if (foo) > > bar(); > > > > would be legal too. So it may be a good idea to clarify this in above > > mentioned document or maybe It's just my bad reading. :-) > > Per IPA developers not using braces is discouraged. I agree the wiki > page should be clarified. > _______________________________________________ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi, thanks for information. Improved patches are attached. PR
>From 669ba173b0daaef99fab6ea1a8d5e5eb71c2cea1 Mon Sep 17 00:00:00 2001 From: Pavel Reichl <pavel.rei...@redhat.com> Date: Thu, 14 Nov 2013 21:34:51 +0000 Subject: [PATCH 1/2] SSSD: Improved domain detection A bit more elegant way of detection of what domain the group member belongs to Resolves: https://fedorahosted.org/sssd/ticket/2132 --- src/providers/ldap/ldap_common.c | 39 ++++++++++++++++++++++++++++----------- src/util/sss_ldap.c | 28 +++++++++++++++++++++++----- src/util/sss_ldap.h | 6 ++++++ 3 files changed, 57 insertions(+), 16 deletions(-) diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c index e29a5219768df154608aad9740dd9ed44c14e22e..482271b8c22095116c7090e3ec70ed2a3c763b34 100644 --- a/src/providers/ldap/ldap_common.c +++ b/src/providers/ldap/ldap_common.c @@ -68,23 +68,40 @@ sdap_domain_get_by_dn(struct sdap_options *opts, const char *dn) { struct sdap_domain *sditer = NULL; - char *dc = NULL; + struct sdap_domain *sdmatch = NULL; + TALLOC_CTX *tmp_ctx = NULL; + int match_len; + int best_match_len = 0; - dc = strstr(dn, "dc="); - if (dc == NULL) { - dc = strstr(dn, "DC="); - if (dc == NULL) { - return NULL; - } + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return NULL; } DLIST_FOR_EACH(sditer, opts->sdom) { - if (strcasecmp(sditer->basedn, dc) == 0) { - return sditer; + if (sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, sditer->search_bases, + NULL, &match_len) + || sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, + sditer->user_search_bases, NULL, &match_len) + || sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, + sditer->group_search_bases, NULL, &match_len) + || sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, + sditer->netgroup_search_bases, NULL, &match_len) + || sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, + sditer->sudo_search_bases, NULL, &match_len) + || sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, + sditer->service_search_bases, NULL, &match_len) + || sss_ldap_dn_in_search_bases_len(tmp_ctx, dn, + sditer->autofs_search_bases, NULL, &match_len)) { + if (best_match_len < match_len) { + /*this is a longer match*/ + best_match_len = match_len; + sdmatch = sditer; + } } } - - return NULL; + talloc_free(tmp_ctx); + return sdmatch; } errno_t diff --git a/src/util/sss_ldap.c b/src/util/sss_ldap.c index 6d7b0907ca2fa48d9cff5257ab6bbba0ae7dd5c6..e1a05e8f60afb692ac95c99a443febac72a31187 100644 --- a/src/util/sss_ldap.c +++ b/src/util/sss_ldap.c @@ -470,10 +470,13 @@ int sss_ldap_init_recv(struct tevent_req *req, LDAP **ldap, int *sd) * _filter will contain combined filters from all possible search bases * or NULL if it should be empty */ -bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, - const char *dn, - struct sdap_search_base **search_bases, - char **_filter) + + +bool sss_ldap_dn_in_search_bases_len(TALLOC_CTX *mem_ctx, + const char *dn, + struct sdap_search_base **search_bases, + char **_filter, + int *_match_len) { struct sdap_search_base *base; int basedn_len, dn_len; @@ -484,6 +487,7 @@ bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, bool backslash_found = false; char *filter = NULL; bool ret = false; + int match_len; if (dn == NULL) { DEBUG(SSSDBG_FUNC_DATA, ("dn is NULL\n")); @@ -511,6 +515,7 @@ bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, if (!base_confirmed) { continue; } + match_len = basedn_len; switch (base->scope) { case LDAP_SCOPE_BASE: @@ -558,6 +563,9 @@ bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, * Append filter otherwise. */ ret = true; + if (_match_len) { + *_match_len = match_len; + } if (base->filter == NULL || _filter == NULL) { goto done; @@ -575,7 +583,8 @@ bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, if (filter != NULL) { *_filter = talloc_asprintf(mem_ctx, "(|%s)", filter); if (*_filter == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_asprintf_append() failed\n")); + DEBUG(SSSDBG_CRIT_FAILURE, + ("talloc_asprintf_append() failed\n")); ret = false; goto done; } @@ -589,6 +598,15 @@ done: return ret; } +bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, + const char *dn, + struct sdap_search_base **search_bases, + char **_filter) +{ + return sss_ldap_dn_in_search_bases_len(mem_ctx, dn, search_bases, _filter, + NULL); +} + char *sss_ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t flags) { char hex[9]; /* 4 bytes in hex + terminating zero */ diff --git a/src/util/sss_ldap.h b/src/util/sss_ldap.h index e5c30eb2115d422ef5a52cc5cd75c85be8fbe2d7..f298b2fbb30cf1532f8e94504ffb83ef73880b81 100644 --- a/src/util/sss_ldap.h +++ b/src/util/sss_ldap.h @@ -74,6 +74,12 @@ bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx, struct sdap_search_base **search_bases, char **_filter); +bool sss_ldap_dn_in_search_bases_len(TALLOC_CTX *mem_ctx, + const char *dn, + struct sdap_search_base **search_bases, + char **_filter, + int *_match_len); + char *sss_ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t flags); #endif /* __SSS_LDAP_H__ */ -- 1.8.3.1
>From cdd879924d048de33b604a0730369e05cae8a813 Mon Sep 17 00:00:00 2001 From: Pavel Reichl <pavel.rei...@redhat.com> Date: Thu, 14 Nov 2013 21:52:26 +0000 Subject: [PATCH 2/2] SSSD: Unit test - sss_ldap_dn_in_search_bases Unit test testing detection of the right domain when processing group with members from several domains Resolves: https://fedorahosted.org/sssd/ticket/2132 --- Makefile.am | 27 ++++- src/tests/cmocka/test_search_bases.c | 191 +++++++++++++++++++++++++++++++++++ 2 files changed, 217 insertions(+), 1 deletion(-) create mode 100644 src/tests/cmocka/test_search_bases.c diff --git a/Makefile.am b/Makefile.am index 992d5796ea3e729238a8d7664c128112291c40ab..ed155bf25c1d1adfe91a7c3c76cdbce62bf6ae59 100644 --- a/Makefile.am +++ b/Makefile.am @@ -154,7 +154,8 @@ if HAVE_CMOCKA fqnames-tests \ test_sss_idmap \ test_utils \ - ad_access_filter_tests + ad_access_filter_tests \ + test_search_bases endif check_PROGRAMS = \ @@ -1388,6 +1389,30 @@ test_utils_LDADD = \ $(SSSD_INTERNAL_LTLIBS) \ libsss_test_common.la +test_search_bases_SOURCES = \ + $(sssd_be_SOURCES) \ + src/util/sss_ldap.c \ + src/util/sss_krb5.c \ + src/util/find_uid.c \ + src/util/user_info_msg.c \ + src/tests/cmocka/test_search_bases.c +test_search_bases_CFLAGS = \ + $(AM_CFLAGS) \ + -DUNIT_TESTING +test_search_bases_LDADD = \ + $(PAM_LIBS) \ + $(CMOCKA_LIBS) \ + $(POPT_LIBS) \ + $(SSSD_LIBS) \ + $(CARES_LIBS) \ + $(KRB5_LIBS) \ + $(SSSD_INTERNAL_LTLIBS) \ + $(SYSTEMD_LOGIN_LIBS) \ + libsss_idmap.la \ + libsss_ldap_common.la \ + libsss_krb5_common.la \ + libsss_test_common.la + ad_access_filter_tests_SOURCES = \ $(sssd_be_SOURCES) \ src/util/sss_ldap.c \ diff --git a/src/tests/cmocka/test_search_bases.c b/src/tests/cmocka/test_search_bases.c new file mode 100644 index 0000000000000000000000000000000000000000..16f7160a20b7991e0518cc4df0cf4c0ba6275f57 --- /dev/null +++ b/src/tests/cmocka/test_search_bases.c @@ -0,0 +1,191 @@ +/* + Authors: + Pavel Reichl <prei...@redhat.com> + + Copyright (C) 2013 Red Hat + + SSSD tests - Search bases + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <stdarg.h> +#include <stdlib.h> +#include <stddef.h> +#include <setjmp.h> +#include <unistd.h> +#include <sys/types.h> +#include <cmocka.h> +#include <ldap.h> + +#include "util/find_uid.h" +#include "util/sss_ldap.h" +#include "tests/common.h" +#include "providers/ldap/ldap_common.h" +#include "providers/ldap/sdap.h" +#include "dhash.h" +#include "tests/common_check.h" + +enum sss_test_get_by_dn { + DN_NOT_IN_DOMS, /* dn is not in any domain */ + DN_IN_DOM1, /* dn is in the domain based on dns */ + DN_IN_DOM2, /* dn is in the domain based on dns2 */ +}; + +static struct sdap_search_base** generate_bases(TALLOC_CTX *mem_ctx, + const char** dns, size_t n) +{ + struct sdap_search_base **search_bases; + errno_t err; + int i; + + search_bases = talloc_array(mem_ctx, struct sdap_search_base *, n + 1); + assert_non_null(search_bases); + + for(i=0; i < n; ++i) { + err = sdap_create_search_base(mem_ctx, dns[i], LDAP_SCOPE_SUBTREE, + NULL, &search_bases[i]); + if (err != EOK) { + fprintf(stderr, "Failed to create search base\n"); + } + assert_int_equal(err, EOK); + } + search_bases[n] = NULL; + return search_bases; +} + +static bool do_test_search_bases(const char* dn, const char** dns, size_t n) +{ + TALLOC_CTX *tmp_ctx; + struct sdap_search_base **search_bases; + bool ret; + + tmp_ctx = talloc_new(NULL); + assert_non_null(tmp_ctx); + + search_bases = generate_bases(tmp_ctx, dns, n); + check_leaks_push(tmp_ctx); + ret = sss_ldap_dn_in_search_bases(tmp_ctx, dn, search_bases, NULL); + assert_true(check_leaks_pop(tmp_ctx) == true); + + talloc_free(tmp_ctx); + return ret; +} + +void test_search_bases_fail(void **state) +{ + const char *dn = "cn=user, dc=sub, dc=ad, dc=pb"; + const char *dns[] = {"dc=example, dc=com", "dc=subdom, dc=ad, dc=pb"}; + bool ret; + + ret = do_test_search_bases(dn, dns, 2); + assert_false(ret); +} + +void test_search_bases_success(void **state) +{ + const char *dn = "cn=user, dc=sub, dc=ad, dc=pb"; + const char *dns[] = {"", "dc=ad, dc=pb", "dc=sub, dc=ad, dc=pb"}; + bool ret; + + ret = do_test_search_bases(dn, dns, 3); + assert_true(ret); +} + +static void do_test_get_by_dn(const char *dn, const char **dns, size_t n, + const char **dns2, size_t n2, int expected_result) +{ + TALLOC_CTX *tmp_ctx; + struct sdap_options *opts; + struct sdap_domain *sdom; + struct sdap_domain *sdom2; + struct sdap_domain *res_sdom; + struct sdap_search_base **search_bases; + struct sdap_search_base **search_bases2; + tmp_ctx = talloc_new(NULL); + assert_non_null(tmp_ctx); + + search_bases = generate_bases(tmp_ctx, dns, n); + search_bases2 = generate_bases(tmp_ctx, dns2, n2); + sdom = talloc_zero(tmp_ctx, struct sdap_domain); + assert_non_null(sdom); + sdom2 = talloc_zero(tmp_ctx, struct sdap_domain); + assert_non_null(sdom2); + + sdom->search_bases = search_bases; + sdom->next = sdom2; + sdom->prev = NULL; + sdom2->search_bases = search_bases2; + sdom2->next = NULL; + sdom2->prev = sdom; + + opts = talloc(tmp_ctx, struct sdap_options); + assert_non_null(opts); + opts->sdom = sdom; + res_sdom = sdap_domain_get_by_dn(opts, dn); + + switch(expected_result) { + case DN_NOT_IN_DOMS: + assert_null(res_sdom); + break; + case DN_IN_DOM1: + assert_true(res_sdom == sdom); + break; + case DN_IN_DOM2: + assert_true(res_sdom == sdom2); + break; + } + + talloc_free(tmp_ctx); +} + +void test_get_by_dn(void **state) +{ + const char *dn = "cn=user, dc=sub, dc=ad, dc=pb"; + const char *dns[] = {"dc=ad, dc=pb"}; + const char *dns2[] = {"dc=sub, dc=ad, dc=pb"}; + + do_test_get_by_dn(dn, dns, 1, dns2, 1, DN_IN_DOM2); +} + +void test_get_by_dn2(void **state) +{ + const char *dn = "cn=user, dc=ad, dc=com"; + const char *dns[] = {"dc=ad, dc=com"}; + const char *dns2[] = {"dc=sub, dc=ad, dc=pb"}; + + do_test_get_by_dn(dn, dns, 1, dns2, 1, DN_IN_DOM1); +} + +void test_get_by_dn_fail(void **state) +{ + const char *dn = "cn=user, dc=sub, dc=example, dc=com"; + const char *dns[] = {"dc=ad, dc=pb"}; + const char *dns2[] = {"dc=sub, dc=ad, dc=pb"}; + + do_test_get_by_dn(dn, dns, 1, dns2, 1, DN_NOT_IN_DOMS); +} + +int main(void) +{ + const UnitTest tests[] = { + unit_test(test_search_bases_fail), + unit_test(test_search_bases_success), + unit_test(test_get_by_dn_fail), + unit_test(test_get_by_dn), + unit_test(test_get_by_dn2) + }; + + return run_tests(tests); +} -- 1.8.3.1
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
