[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