[SSSD] [PATCH] - Improved detection of user domain
Pavel Reichl
preichl at redhat.com
Tue Nov 26 16:28:18 UTC 2013
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.cse at 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.reichl at 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. :-)
>
> >
> > 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)
>
> Can you reformat the first call so it's within 80 characters?
>
>
> [snip]
>
> > @@ -558,6 +563,8 @@ bool sss_ldap_dn_in_search_bases(TALLOC_CTX *mem_ctx,
> > * Append filter otherwise.
> > */
> > ret = true;
> > + if (_match_len)
> > + *_match_len = match_len;
>
> Same comment about brackets here.
>
> > From cfcd83aca098784d9ab1ca59f0a1ad8ef3145b62 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl <pavel.reichl at redhat.com>
> > Date: Thu, 14 Nov 2013 21:52:26 +0000
> > Subject: [PATCH] SSSD: Unit test - sss_ldap_dn_in_search_bases
>
> > +struct sdap_search_base** generate_bases(TALLOC_CTX *mem_ctx, const char** dns, size_t n)
>
> Can you make the helper functions static and wrap for 80 characters per
> line?
>
> > +{
> > + 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_true(err == EOK);
>
> cmocka has helper macros for comparing two integers:
> assert_int_equal(err, EOK);
>
> > + }
> > + search_bases[n] = NULL;
> > + return search_bases;
> > +}
> > +
> > +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);
>
> Can you check how we use check_leaks_push() and check_leaks_pop()
> elsewhere? I think it would be nice to assert that
> sss_ldap_dn_in_search_bases() does not allocate more memory that it
> should.
>
> > + ret = sss_ldap_dn_in_search_bases(tmp_ctx, dn, search_bases, NULL);
> > +
> > + 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);
> > +}
> > +
> > +void test_search_empty_bases_fail(void **sstate)
> > +{
> > + TALLOC_CTX *tmp_ctx;
> > + struct sdap_search_base **search_bases;
> > + const char *dn = "cn=user, dc=sub, dc=ad, dc=pb";
> > + errno_t err;
> > + bool ret;
> > +
> > + tmp_ctx = talloc_new(NULL);
> > + assert_non_null(tmp_ctx);
> > +
> > + search_bases = talloc_array(tmp_ctx, struct sdap_search_base*, 2);
> > + assert_non_null(search_bases);
> > +
> > + err = sdap_create_search_base(tmp_ctx, "", LDAP_SCOPE_SUBTREE, NULL,
>
> Search base "" is kind of a special case in LDAP, I think it can only be
> used to describe the special rootDSE entry and can't be used otherwise.
> Maybe we should patch sdap_create_search_base to only allow a base with
> strlen > 1.
>
> Was the test purpose to check a wrong search base or one that doesn't
> match at all?
Well I just thought it would be a good idea to test an edge case. I
wasn't really sure about correct behavior for this case.
I'll remove this test completely, OK?
>
> > + &search_bases[0]);
> > + assert_true(err == EOK);
> > + search_bases[1] = NULL;
> > +
> > + ret = sss_ldap_dn_in_search_bases(tmp_ctx, dn, search_bases, NULL);
> > + assert_false(ret);
> > +
> > + talloc_free(tmp_ctx);
> > +}
> > +
> > +
> > +/*
> > + * expected_result equal to 0 means that dn is not in any domain
> > + * expected_result equal to 1 means that dn is in the domain based on dns
> > + * expected_result equal to 2 means that dn is in the domain based on dns2
> > + */
>
> I was thinking about converting the above into an enum, but it's
> probably not worth it.
>
> > +
> > +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);
>
> Parts of the unit test checks for allocation failures and part does not.
> The code should choose one and stick with it (and checking allocation
> failures is safer, even though in non-deamons not completely necessary).
>
> > +
> > + search_bases = generate_bases(tmp_ctx, dns, n);
> > + search_bases2 = generate_bases(tmp_ctx, dns2, n2);
> > + sdom = talloc_zero(tmp_ctx, struct sdap_domain);
> > + sdom2 = talloc_zero(tmp_ctx, struct sdap_domain);
> > +
> > + 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);
> > + opts->sdom = sdom;
> > + res_sdom = sdap_domain_get_by_dn(opts, dn);
> > +
> > + switch(expected_result) {
> > + case 0:
> > + assert_null(res_sdom);
> > + break;
> > + case 1:
> > + assert_true(res_sdom == sdom);
> > + break;
> > + case 2:
> > + assert_true(res_sdom == sdom2);
> > + break;
> > + }
> > + talloc_free(tmp_ctx);
> > +}
> > +
>
> [snip]
>
> > +
> > +int main(void)
> > +{
> > + const UnitTest tests[] = {
> > + unit_test(test_search_bases_fail),
> > + unit_test(test_search_bases_success),
> > +#if 0
> > + unit_test(test_search_empty_bases_fail),
> > +#endif
>
> I don't like a test case that is commented out, we should either fix it
> or remove it.
I tried to address this issue above I suggest removing it.
>
> > + 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list