[SSSD] [PATCH] - Improved detection of user domain

Jakub Hrozek jhrozek at redhat.com
Tue Nov 26 14:23:13 UTC 2013


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.

>  
>      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? 

> +                                  &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.

> +        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
> 



More information about the sssd-devel mailing list