[SSSD] [PATCH] nested groups unit test

Jakub Hrozek jhrozek at redhat.com
Tue Nov 5 11:22:56 UTC 2013


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.



More information about the sssd-devel mailing list