[SSSD] [PATCH] nested groups unit test

Pavel Březina pbrezina at redhat.com
Fri Jan 31 13:37:36 UTC 2014


On 11/05/2013 12:22 PM, Jakub Hrozek wrote:
> On Thu, Jul 18, 2013 at 03:45:35PM +0200, Pavel Březina wrote:
>> On 07/18/2013 02:38 PM, Pavel Březina wrote:
>>> Hi,
>>> this is the first unit test for nested groups. It covers only the most
>>> basic situation when we are trying to resolve one group with no members.
>>>
>>> Even though it is only one test, the patch set is quite big. This is
>>> because it creates the possibility to mock providers related modules.
>>> Most of the patches are just a preparation for unit testing providers.
>>>
>>> Patches 1-5
>>> Moves the code around to reduce number of dependencies. (E.g. you do not
>>> want to load fail over when you are testing nested groups.)
>>>
>>> Patch 6
>>> Mocks basic SDAP interface.
>>>
>>> Patch 7
>>> Mocks sysdb objects - currently user and rfc2307bis group. You can
>>> decide what set of attributes the object should posses. For example,
>>> creating a user requires only basedn and name parameter, to construct
>>> originalDN and name attributes. The rest is provided by (attrname,
>>> value) pairs via variadic function.
>>>
>>> E.g.:
>>> mock_sysdb_user(mem_ctx, basedn, name, SYSDB_UIDNUM, uid, ...)
>>>
>>> get_attr_type() translates the sysdb attribute name to proper data type.
>>> This should be extended as needed.
>>>
>>> Patch 8
>>> Adds provider tests related common object files and cflags in makefile
>>>
>>> Patch 9
>>> New macro sss_will_return_always(fn, value). This can be used to mock
>>> function data in such way that any call of mock() will return the value.
>>>
>>> It was just pushed also to cmocka upstream as will_return_always().
>>>
>>> Patch 10-11
>>> Unit test.
>>>
>>> Patch 12
>>> Removes a noisy debug message.
>>>
>>> I would like to get this reviewed before I continue with more test
>>> cases, so the framework is tuned enough.
>>>
>>> Off topic:
>>> I also created new macro called fail_msg, which will make the test fail
>>> printing a message. I didn't use this macro in the end, but it made its
>>> way to cmocka upstream.
>>
>> Oops, I did some reorganization while writing this mail but sent
>> patches generated sooner, so the numbers does not match. Reordered
>> patches are attached.
>>
>>
>
> The patches no longer apply (not suprising after such a long time on the
> list..)
>
> I will add some quick comments inline without trying out the patches.
>
>>  From 9b370ed83ec5c85819781145de92c39977fd19eb Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Tue, 16 Jul 2013 11:21:02 +0200
>> Subject: [PATCH 01/12] sdap: move non async functions from sdap_async.c to
>>   sdap_utils.c
>>
>> ---
>>   Makefile.am                     |   1 +
>>   src/providers/ldap/sdap_async.c | 107 --------------------------------
>>   src/providers/ldap/sdap_utils.c | 131 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 132 insertions(+), 107 deletions(-)
>>   create mode 100644 src/providers/ldap/sdap_utils.c
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index 515843420834bbbd0e5b4c86c114294c89afa752..6cfc363cea9eae833a8a4bdff12309286475b89a 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> @@ -1450,6 +1450,7 @@ libsss_ldap_common_la_SOURCES = \
>>       src/providers/ldap/sdap_reinit.c \
>>       src/providers/ldap/sdap_dyndns.c \
>>       src/providers/ldap/sdap_refresh.c \
>> +    src/providers/ldap/sdap_utils.c \
>>       src/providers/ldap/sdap.c
>>   libsss_ldap_common_la_LDFLAGS = \
>>       -avoid-version
>> diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
>> index bdd3c3c8b2ed4cc7246c008ddbfe531b82d880bc..6d29c1a566f82c2ce0dead2ca8073ace15afbf56 100644
>> --- a/src/providers/ldap/sdap_async.c
>> +++ b/src/providers/ldap/sdap_async.c
>> @@ -23,26 +23,8 @@
>>   #include "util/util.h"
>>   #include "providers/ldap/sdap_async_private.h"
>>
>> -#define REALM_SEPARATOR '@'
>>   #define REPLY_REALLOC_INCREMENT 10
>>
>> -void make_realm_upper_case(const char *upn)
>
> This function seems to be called from a single location only. Can we
> either ditch it in favor of get_uppercase_realm() from usertools.c or
> make it static?
>
>> -{
>> -    char *c;
>> -
>> -    c = strchr(upn, REALM_SEPARATOR);
>> -    if (c == NULL) {
>> -        DEBUG(9, ("No realm delimiter found in upn [%s].\n", upn));
>> -        return;
>> -    }
>> -
>> -    while(*(++c) != '\0') {
>> -        c[0] = toupper(*c);
>> -    }
>> -
>> -    return;
>> -}
>> -
>
>>  From 4f76600299c31489c98f960d162ab7c1d2a29126 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 17 Jul 2013 13:20:43 +0200
>> Subject: [PATCH 02/12] sdap: move non async functions from
>>   sdap_async_connection.c to sdap_utils.c
>
> Looks good to me.
>
>>  From dab6d9221bafe0e18f599b88de10b7cc57a57b8e Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 17 Jul 2013 14:28:26 +0200
>> Subject: [PATCH 03/12] sdap: move sdap_get_id_specific_filter() to
>>   sdap_utils.c
>
> I think this makes sense.
>
> Since we started talking about LDAP provider refactoring on IRC
> recently, I would like to bring up the ldap_*.c vs. sdap_*.c confusion.
>
> In my opinion, we should separate libsss_ldap.la as discussed on #sssd
> the other day and keep its sources in sdap*.c. Only code specific to the
> initialization of the LDAP provider should be kept in ldap*.c
>
>
>>  From a97267f35a598e7f944cf993b37cd6b43db7a572 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 17 Jul 2013 13:50:34 +0200
>> Subject: [PATCH 04/12] ldap: move options related content from ldap_common.c
>>   to ldap_options.c
>>
>> ---
>>   src/providers/ldap/ldap_common.c                   | 886 --------------------
>>   .../ldap/{ldap_common.c => ldap_options.c}         | 921 ---------------------
>>   2 files changed, 1807 deletions(-)
>>   copy src/providers/ldap/{ldap_common.c => ldap_options.c} (53%)
>>
>> diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
>> index 5782c4615ddd3778d16e52986eae065c3c61b8e5..7a8bf93d2366a517e545f1faecdc3aefa2be36e7 100644
>> --- a/src/providers/ldap/ldap_common.c
>> +++ b/src/providers/ldap/ldap_common.c
>> @@ -39,892 +39,6 @@
>>   /* a fd the child process would log into */
>>   int ldap_child_debug_fd = -1;
>>
>> -int
>> -sdap_domain_destructor(void *mem)
>> -{
>> -    struct sdap_domain *dom =
>> -            talloc_get_type(mem, struct sdap_domain);
>> -    DLIST_REMOVE(*(dom->head), dom);
>> -    return 0;
>> -}
>> -
>> -struct sdap_domain *
>> -sdap_domain_get(struct sdap_options *opts,
>> -                struct sss_domain_info *dom)
>> -{
>> -    struct sdap_domain *sditer = NULL;
>> -
>> -    DLIST_FOR_EACH(sditer, opts->sdom) {
>> -        if (sditer->dom == dom) {
>> -            break;
>> -        }
>> -    }
>> -
>> -    return sditer;
>> -}
>> -
>> -errno_t
>> -sdap_domain_add(struct sdap_options *opts,
>> -                struct sss_domain_info *dom,
>> -                struct sdap_domain **_sdom)
>> -{
>> -    struct sdap_domain *sdom;
>> -
>> -    sdom = talloc_zero(opts, struct sdap_domain);
>> -    if (sdom == NULL) {
>> -        return ENOMEM;
>> -    }
>> -    sdom->dom = dom;
>> -    sdom->head = &opts->sdom;
>> -
>> -    if (opts->sdom) {
>> -        /* Only allow subdomains of the parent domain */
>> -        if (dom->parent == NULL ||
>> -            dom->parent != opts->sdom->dom) {
>> -            DEBUG(SSSDBG_OP_FAILURE, ("Domain %s is not a subdomain of %s\n",
>> -                  dom->name, opts->sdom->dom->name));
>> -            return EINVAL;
>> -        }
>> -    }
>> -
>> -    talloc_set_destructor((TALLOC_CTX *)sdom, sdap_domain_destructor);
>> -    DLIST_ADD_END(opts->sdom, sdom, struct sdap_domain *);
>> -
>> -    if (_sdom) *_sdom = sdom;
>> -    return EOK;
>> -}
>> -
>> -errno_t
>> -sdap_domain_subdom_add(struct sdap_id_ctx *sdap_id_ctx,
>> -                       struct sdap_domain *sdom_list,
>> -                       struct sss_domain_info *parent)
>> -{
>> -    struct sss_domain_info *dom;
>> -    struct sdap_domain *sdom, *sditer;
>> -    char *basedn;
>> -    errno_t ret;
>> -
>> -    for (dom = get_next_domain(parent, true);
>> -         dom && IS_SUBDOMAIN(dom); /* if we get back to a parent, stop */
>> -         dom = get_next_domain(dom, false)) {
>> -
>> -        DLIST_FOR_EACH(sditer, sdom_list) {
>> -            if (sditer->dom == dom) {
>> -                break;
>> -            }
>> -        }
>> -
>> -        if (sditer == NULL) {
>> -            /* New sdap domain */
>> -            DEBUG(SSSDBG_TRACE_FUNC, ("subdomain %s is a new one, will "
>> -                  "create a new sdap domain object\n", dom->name));
>> -
>> -            ret = sdap_domain_add(sdap_id_ctx->opts, dom, &sdom);
>> -            if (ret != EOK) {
>> -                DEBUG(SSSDBG_OP_FAILURE,
>> -                    ("Cannot add new sdap domain for domain %s [%d]: %s\n",
>> -                    parent->name, ret, strerror(ret)));
>> -                return ret;
>> -            }
>> -        } else {
>> -            sdom = sditer;
>> -        }
>> -
>> -        /* Convert the domain name into search base */
>> -        ret = domain_to_basedn(sdom, sdom->dom->name, &basedn);
>> -        if (ret != EOK) {
>> -            DEBUG(SSSDBG_OP_FAILURE,
>> -                ("Cannot convert domain name [%s] to base DN [%d]: %s\n",
>> -                dom->name, ret, strerror(ret)));
>> -            talloc_free(basedn);
>> -            return ret;
>> -        }
>> -
>> -        /* Update search bases */
>> -        talloc_zfree(sdom->search_bases);
>> -        sdom->search_bases = talloc_array(sdom, struct sdap_search_base *, 2);
>> -        if (sdom->search_bases == NULL) {
>> -            return ret;
>> -        }
>> -        sdom->search_bases[1] = NULL;
>> -
>> -        ret = sdap_create_search_base(sdom, basedn, LDAP_SCOPE_SUBTREE, NULL,
>> -                                      &sdom->search_bases[0]);
>> -        talloc_free(basedn);
>> -        if (ret) {
>> -            DEBUG(SSSDBG_OP_FAILURE, ("Cannot create new sdap search base\n"));
>> -            return ret;
>> -        }
>> -
>> -        sdom->user_search_bases = sdom->search_bases;
>> -        sdom->group_search_bases = sdom->search_bases;
>> -        sdom->netgroup_search_bases = sdom->search_bases;
>> -        sdom->sudo_search_bases = sdom->search_bases;
>> -        sdom->service_search_bases = sdom->search_bases;
>> -        sdom->autofs_search_bases = sdom->search_bases;
>> -    }
>> -
>> -    return EOK;
>> -}
>> -
>> -void
>> -sdap_domain_remove(struct sdap_options *opts,
>> -                   struct sss_domain_info *dom)
>> -{
>> -    struct sdap_domain *sdom;
>> -
>> -    sdom = sdap_domain_get(opts, dom);
>> -    if (sdom == NULL) return;
>> -
>> -    DLIST_REMOVE(*(sdom->head), sdom);
>> -}
>> -
>
> Would it make sense to separate sdap_domain_* functions to an sdap
> module instead?
>
> Moving ldap_get_$provider_options makes sense to me.
>
>>  From 3054f4e5359b3f46faa33545b1cd36d11ef646ba Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 17 Jul 2013 13:51:37 +0200
>> Subject: [PATCH 05/12] ldap_options.c: do not call krb5_try_kdcip when unit
>>   testing
>
> I agree with Lukas, I would prefer not to add ifdefs to the code unless
> needed.
>
>>  From d1e5b04f0cc7ece9bf7fccd564626e488d532951 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Tue, 26 Mar 2013 15:07:17 +0100
>> Subject: [PATCH 06/12] tests: mock SDAP
>
> Looks good to me.
>
>
>>  From e64bce82249f1b8be7bd5eb8bd4fd01d7a5fb5dd Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Tue, 26 Mar 2013 15:18:21 +0100
>> Subject: [PATCH 07/12] tests: mock sysdb users and groups
>
> I think the approach is fine for unit tests. I don't see us change the
> types of sysdb attributes frequently.
>
>
>>  From 7f909490572f3edf4c8f6540ee0b62bc5f226ea0 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 17 Jul 2013 13:58:54 +0200
>> Subject: [PATCH 08/12] tests: prepare makefile for provider related unit
>>   tests
>
> Looks good to me.
>
>
>>  From 7e09c2abfb6749b424cb2ea4e63ecf685ac82033 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Thu, 18 Jul 2013 13:24:26 +0200
>> Subject: [PATCH 09/12] tests: new macro sss_will_return_always
>
> Is this feature available in all cmocka releases (IIRC you wrote it) ?
>
>>  From 65da81b7fe5418f44ec743fc11520c5e68a85435 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Tue, 26 Mar 2013 15:06:44 +0100
>> Subject: [PATCH 10/12] tests: add confdb_path to sss_test_ctx
>
> Looks good to me.
>
>
>>  From 03e318c1319f8b3545195d7deee3d55793fc941a Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Wed, 17 Jul 2013 13:59:36 +0200
>> Subject: [PATCH 11/12] tests: nested groups unit test
>
> I only scrolled through this one, but in general looks good.
>
>
>>  From 53ff2f0812b6ae81642513aee693284ad134ba47 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
>> Date: Thu, 18 Jul 2013 14:05:05 +0200
>> Subject: [PATCH 12/12] tests: don't print debug message when test dir does
>>   not exist
>
> Looks good to me.

Rebased version with all issues addressed is attached.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sdap-move-non-async-functions-from-sdap_async.c-to-s.patch
Type: text/x-patch
Size: 9703 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sdap-move-non-async-functions-from-sdap_async_connec.patch
Type: text/x-patch
Size: 2372 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sdap-move-sdap_get_id_specific_filter-to-sdap_utils..patch
Type: text/x-patch
Size: 2501 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ldap-move-options-related-content-from-ldap_common.c.patch
Type: text/x-patch
Size: 56929 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-ldap-move-domain-related-content-from-ldap_common.c-.patch
Type: text/x-patch
Size: 13423 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-make-make_realm_upper_case-static.patch
Type: text/x-patch
Size: 2695 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-tests-add-confdb_path-to-sss_test_ctx.patch
Type: text/x-patch
Size: 1967 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-tests-mock-SDAP.patch
Type: text/x-patch
Size: 7408 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0009-tests-mock-sysdb-users-and-groups.patch
Type: text/x-patch
Size: 7968 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0010-tests-prepare-makefile-for-provider-related-unit-tes.patch
Type: text/x-patch
Size: 1147 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0011-tests-new-macro-sss_will_return_always.patch
Type: text/x-patch
Size: 851 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0012-tests-nested-groups-unit-test.patch
Type: text/x-patch
Size: 8567 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0013-tests-don-t-print-debug-message-when-test-dir-does-n.patch
Type: text/x-patch
Size: 1075 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140131/80be6846/attachment-0012.bin>


More information about the sssd-devel mailing list