[SSSD] [PATCHES] ldap: refactor - reuse sss_utc_to_time_t()

Jakub Hrozek jhrozek at redhat.com
Fri Mar 20 17:44:58 UTC 2015


On Fri, Mar 20, 2015 at 04:58:38PM +0100, Pavel Reichl wrote:
> On 03/20/2015 03:26 PM, Jakub Hrozek wrote:
> >On Fri, Mar 20, 2015 at 01:13:35PM +0100, Pavel Reichl wrote:
> >>>I wonder why didn't you create a single pwexpire test instead? The file
> >>>names are confusing, I would expect tests of authentication and acess
> >>>control.
> >>The functionality I wanted to test was in 2 separate units (ldap_auth,c,
> >>sdap_access.c), but for those units there were no tests. So I created 2 new
> >>test units (test_ldap_auth,c, test_sdap_access.c) in hope that more
> >>functions from these units will be tested. (code coverage is currently 0.0 %
> >>for both modules (https://jhrozek.fedorapeople.org/sssd-coverage/html/providers/ldap/index.html)
> >I think this is OK as long as there's nothing specific to the expiration
> >tests in the test sources and the sources can be re-used for 'real' auth
> >and access checks.
> >
> >(btw I'm not sure how realistic is to have auth and access /unit/ tests,
> >I looked at the sources when you wrote the SSH key expiration patches,
> >but it seemed to me integration tests could be a better option)
> >
> >So I'm fine both ways.
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> Please see updated patch set.
> Thanks!
> 
> ci passed:
> 
> http://sssd-ci.duckdns.org/logs/job/11/31/summary.html
> 

> From 4236caecdc9981d10f777e90a65aa60cd92445df Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Fri, 6 Mar 2015 04:29:24 -0500
> Subject: [PATCH 1/3] TESTS: test expiration

Sorry, more nitpicks. I should have sent them earlier, but I didn't
notice them before.

> 
> ---
>  Makefile.am                           |  44 ++++++++++++
>  src/providers/ldap/sdap_access.c      |   2 +-
>  src/tests/cmocka/test_expire_common.c | 130 ++++++++++++++++++++++++++++++++++
>  src/tests/cmocka/test_expire_common.h |  39 ++++++++++
>  src/tests/cmocka/test_ldap_auth.c     | 106 +++++++++++++++++++++++++++
>  src/tests/cmocka/test_sdap_access.c   |  73 +++++++++++++++++++
>  6 files changed, 393 insertions(+), 1 deletion(-)
>  create mode 100644 src/tests/cmocka/test_expire_common.c
>  create mode 100644 src/tests/cmocka/test_expire_common.h
>  create mode 100644 src/tests/cmocka/test_ldap_auth.c
>  create mode 100644 src/tests/cmocka/test_sdap_access.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 0f7725b06e0330bf17c38eb7b1829a8093fb1d70..6f787e625fbd67d55e953f374bd435f0bbcf3d76 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -212,6 +212,8 @@ if HAVE_CMOCKA
>          sbus-internal-tests \
>          sss_sifp-tests \
>          test_search_bases \
> +        test_ldap_auth \
> +        test_sdap_access \
>          sdap-tests \
>          test_sysdb_views \
>          test_sysdb_utils \
> @@ -1971,6 +1973,48 @@ test_search_bases_LDADD = \
>      libsss_krb5_common.la \
>      libsss_test_common.la
>  
> +test_ldap_auth_SOURCES = \
> +    $(sssd_be_SOURCES) \
> +    src/tests/cmocka/test_ldap_auth.c \
> +    src/tests/cmocka/test_expire_common.c \
> +    $(NULL)

I prefer not to have an extra line between _SOURCES and _CFLAGS for the
same target.

> +
> +test_ldap_auth_CFLAGS = \
> +    $(AM_CFLAGS) \
> +    -DUNIT_TESTING

Please add $(NULL) here.

> +test_ldap_auth_LDADD = \
> +    $(PAM_LIBS) \
> +    $(CMOCKA_LIBS) \
> +    $(POPT_LIBS) \
> +    $(SSSD_LIBS) \
> +    $(CARES_LIBS) \
> +    $(KRB5_LIBS) \
> +    $(SSSD_INTERNAL_LTLIBS) \
> +    libsss_ldap_common.la \
> +    libsss_test_common.la \
> +    $(NULL)
> +
> +test_sdap_access_SOURCES = \
> +    $(sssd_be_SOURCES) \
> +    src/tests/cmocka/test_sdap_access.c \
> +    src/tests/cmocka/test_expire_common.c \
> +    $(NULL)

Extra blank line

> +
> +test_sdap_access_CFLAGS = \
> +    $(AM_CFLAGS) \
> +    -DUNIT_TESTING

Missing $(NULL)

> +test_sdap_access_LDADD = \
> +    $(PAM_LIBS) \
> +    $(CMOCKA_LIBS) \
> +    $(POPT_LIBS) \
> +    $(SSSD_LIBS) \
> +    $(CARES_LIBS) \
> +    $(KRB5_LIBS) \
> +    $(SSSD_INTERNAL_LTLIBS) \
> +    libsss_ldap_common.la \
> +    libsss_test_common.la \
> +    $(NULL)
> +
>  ad_access_filter_tests_SOURCES = \
>      $(sssd_be_SOURCES) \
>      src/providers/ad/ad_common.c \
> diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
> index be2dacf5379a6842c841fe77fb61aa4ebea03e1f..9e9cfc9e3e6d6f0ef25fdd5eb962c5f42e24b60f 100644
> --- a/src/providers/ldap/sdap_access.c
> +++ b/src/providers/ldap/sdap_access.c
> @@ -528,7 +528,7 @@ static errno_t sdap_account_expired_rhds(struct pam_data *pd,
>  #define NDS_EXPIRED_MSG "The user account is expired"
>  #define NDS_TIME_MAP_MSG "The user account is not allowed at this time"
>  
> -static bool nds_check_expired(const char *exp_time_str)
> +bool nds_check_expired(const char *exp_time_str)
>  {
>      char *end;
>      struct tm tm;
> diff --git a/src/tests/cmocka/test_expire_common.c b/src/tests/cmocka/test_expire_common.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..73a3b23f50c3935b6cbb3320d50589f626445e3d
> --- /dev/null
> +++ b/src/tests/cmocka/test_expire_common.c
> @@ -0,0 +1,130 @@
> +/*
> +    Authors:
> +        Pavel Reichl <preichl at redhat.com>
> +
> +    Copyright (C) 2015 Red Hat
> +
> +    SSSD tests - common code for password expiration tests
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <setjmp.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <cmocka.h>
> +#include <time.h>
> +
> +#include "tests/common_check.h"
> +#include "tests/cmocka/test_expire_common.h"
> +
> +#define MAX 100
> +
> +static char *now_str(TALLOC_CTX *mem_ctx, const char* format, int s)
> +{
> +    time_t t = time(NULL) + s;
> +    struct tm *tm;
> +    size_t len;
> +    char *timestr;
> +
> +    timestr = talloc_array(mem_ctx, char, MAX);
> +
> +    tm = gmtime(&t);
> +    len = strftime(timestr, MAX, format, tm);
> +    if (len == 0) {
> +        return NULL;
> +    }
> +
> +    return timestr;
> +}
> +
> +int expire_test_setup(void **state)
> +{
> +    struct expire_test_ctx *exp_state;
> +    TALLOC_CTX *mem_ctx;
> +    char *past_time;
> +    char *future_time;
> +    char *invalid_format;
> +    char *invalid_longer_format;
> +
> +    mem_ctx = talloc_new(NULL);
> +    assert_non_null(mem_ctx);
> +
> +    exp_state = talloc(mem_ctx, struct expire_test_ctx);
> +    assert_non_null(exp_state);
> +
> +    *state = exp_state;
> +
> +    /* testing data */
> +    invalid_format = (void*)now_str(exp_state, "%Y%m%d%H%M%S", -20);
                        ^^^^^^
        I don't think you need the void* casts, can you just drop them?

> +    assert_non_null(invalid_format);
> +
> +    invalid_longer_format = (void*)now_str(exp_state, "%Y%m%d%H%M%SZA", -20);
> +    assert_non_null(invalid_longer_format);
> +
> +    past_time = (void*)now_str(exp_state, "%Y%m%d%H%M%SZ", -20);
> +    assert_non_null(past_time);
> +
> +    future_time = (void*)now_str(exp_state, "%Y%m%d%H%M%SZ", 20);
> +    assert_non_null(future_time);
> +
> +    exp_state->past_time = past_time;
> +    exp_state->future_time = future_time;
> +    exp_state->invalid_format = invalid_format;
> +    exp_state->invalid_longer_format = invalid_longer_format;
> +
> +    return 0;
> +}
> +
> +int expire_test_teardown(void **state)
> +{
> +    struct expire_test_ctx *test_ctx;
> +
> +    test_ctx = talloc_get_type(*state, struct expire_test_ctx);
> +    assert_non_null(test_ctx);
> +
> +    talloc_free(test_ctx);
> +
> +    return 0;
> +}
> +
> +void expire_test_tz(const char* tz,
> +                    void (*test_func)(void*, void*),
> +                    void *test_in,
> +                    void *_test_out)
> +{
> +    errno_t ret;
> +    const char *orig_tz = NULL;
> +
> +    orig_tz = getenv("TZ");
> +    if (orig_tz == NULL) {
> +        orig_tz = "";
> +    }
> +
> +    if (tz) {
> +        ret = setenv("TZ", tz, 1);
> +        assert_false(ret == -1);
> +    }
> +
> +    test_func(test_in, _test_out);
> +
> +    /* restore */
> +    if (orig_tz != NULL) {
> +        ret = setenv("TZ", orig_tz, 1);
> +        assert_false(ret == -1);
> +    }
> +}
> diff --git a/src/tests/cmocka/test_expire_common.h b/src/tests/cmocka/test_expire_common.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..0ccff14fc90aafc85915f8e6b2087d5ed09fa84a
> --- /dev/null
> +++ b/src/tests/cmocka/test_expire_common.h
> @@ -0,0 +1,39 @@
> +/*
> +    Authors:
> +        Pavel Reichl <preichl at redhat.com>
> +
> +    Copyright (C) 2015 Red Hat
> +
> +    SSSD tests: Tests for password expiration related functionality
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#ifndef __TEST_EXPIRE_COMMON_H
> +#define __TEST_EXPIRE_COMMON_H
> +
> +struct expire_test_ctx
> +{
> +    char *past_time;
> +    char *future_time;
> +    char *invalid_format;
> +    char *invalid_longer_format;
> +};
> +
> +int expire_test_setup(void **state);
> +int expire_test_teardown(void **state);
> +void expire_test_tz(const char* tz, void (*f)(void*, void*), void *in,
> +                    void *_out);
> +
> +#endif /* __TEST_EXPIRE_COMMON_H */
> diff --git a/src/tests/cmocka/test_ldap_auth.c b/src/tests/cmocka/test_ldap_auth.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..00bb3514e6826406b844e42f569a0ee657808252
> --- /dev/null
> +++ b/src/tests/cmocka/test_ldap_auth.c
> @@ -0,0 +1,106 @@
> +/*
> +    Authors:
> +        Pavel Reichl <preichl at redhat.com>
> +
> +    Copyright (C) 2015 Red Hat
> +
> +    SSSD tests - ldap auth
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <setjmp.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <cmocka.h>
> +
> +#include "tests/common_check.h"
> +#include "providers/ldap/ldap_auth.h"
> +#include "tests/cmocka/test_expire_common.h"
> +
> +struct check_pwexpire_policy_wrap_indata {
> +    enum pwexpire type;
> +    void *time_fmt;
> +};
> +
> +static void check_pwexpire_policy_wrap(void *in, void *_out)
> +{
> +    errno_t ret;
> +    struct check_pwexpire_policy_wrap_indata *data =
> +        (struct check_pwexpire_policy_wrap_indata*) in;
> +
> +    ret = check_pwexpire_policy(data->type, data->time_fmt, NULL, 0);
> +    *(errno_t*)_out = ret;
> +}
> +
> +static void test(void **state, enum pwexpire type)
> +{
> +    struct expire_test_ctx *tc;
> +    errno_t ret;
> +
> +    tc = talloc_get_type(*state, struct expire_test_ctx);
> +    assert_non_null(tc);
> +
> +    ret = check_pwexpire_policy(type,
> +                                (void*) tc->invalid_longer_format, NULL, 0);
> +    assert_true(ret == EINVAL);
> +
> +    ret = check_pwexpire_policy(type, (void*) tc->invalid_format,
> +                                NULL, 0);
> +    assert_true(ret == EINVAL);
> +
> +    ret = check_pwexpire_policy(type, (void*) tc->past_time,
> +                                NULL, 0);
> +    assert_true(ret == ERR_PASSWORD_EXPIRED);
> +
> +    ret = check_pwexpire_policy(type, (void*) tc->future_time,
> +                                NULL, 0);
> +    assert_true(ret == EOK);
> +
> +    /* changing time zone has no effect as time of expiration is in UTC */
> +    struct check_pwexpire_policy_wrap_indata data;
> +    data.type = type;
> +    data.time_fmt = (void*)tc->future_time;
> +    expire_test_tz("GST-2",
> +                   check_pwexpire_policy_wrap,
> +                   (void*)&data,
> +                   (void*)&ret);
> +    assert_true(ret == EOK);
> +
> +    data.time_fmt = (void*)tc->past_time;
> +    expire_test_tz("GST-2",
> +                   check_pwexpire_policy_wrap,
> +                   (void*)&data,
> +                   (void*)&ret);
> +    assert_true(ret == ERR_PASSWORD_EXPIRED);
> +}
> +
> +void test_pwexpire_krb(void **state)
> +{
> +    test(state, PWEXPIRE_KERBEROS);

Test is a static function that is only called once, does it need to be a
separate function?

> +}
> +
> +int main(void)
> +{
> +    const struct CMUnitTest tests[] = {
> +        cmocka_unit_test_setup_teardown(test_pwexpire_krb,
> +                                        expire_test_setup,
> +                                        expire_test_teardown)
> +    };
> +
> +    return cmocka_run_group_tests(tests, NULL, NULL);
> +}
> diff --git a/src/tests/cmocka/test_sdap_access.c b/src/tests/cmocka/test_sdap_access.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..6387e54ac092523dfd4407983525837c229d4772
> --- /dev/null
> +++ b/src/tests/cmocka/test_sdap_access.c
> @@ -0,0 +1,73 @@
> +/*
> +    Authors:
> +        Pavel Reichl <preichl at redhat.com>
> +
> +    Copyright (C) 2015 Red Hat
> +
> +    SSSD tests - sdap access
> +
> +    This program is free software; you can redistribute it and/or modify
> +    it under the terms of the GNU General Public License as published by
> +    the Free Software Foundation; either version 3 of the License, or
> +    (at your option) any later version.
> +
> +    This program is distributed in the hope that it will be useful,
> +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +    GNU General Public License for more details.
> +
> +    You should have received a copy of the GNU General Public License
> +    along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +*/
> +
> +#include <stdarg.h>
> +#include <stdlib.h>
> +#include <stddef.h>
> +#include <setjmp.h>
> +#include <unistd.h>
> +#include <sys/types.h>
> +#include <cmocka.h>
> +
> +#include "tests/common_check.h"
> +#include "tests/cmocka/test_expire_common.h"
> +
> +bool nds_check_expired(const char *exp_time_str);

Can you add "extern" here or at least a comment that we're linking with
a function from an sdap module?

> +
> +static void nds_check_expired_wrap(void *in, void *_out)
> +{
> +    *(bool*)_out = nds_check_expired((const char*)in);
> +}
> +
> +void test_nds_check_expire(void **state)
> +{
> +    struct expire_test_ctx *tc;
> +    bool res;
> +
> +    tc = talloc_get_type(*state, struct expire_test_ctx);
> +    assert_non_null(tc);
> +
> +    assert_false(nds_check_expired(NULL));
> +    assert_true(nds_check_expired(tc->invalid_longer_format));
> +    assert_true(nds_check_expired(tc->invalid_format));
> +    assert_true(nds_check_expired(tc->past_time));
> +    assert_false(nds_check_expired(tc->future_time));
> +
> +    /* changing time zone has no effect as time of expiration is in UTC */
> +    expire_test_tz("GST+2", nds_check_expired_wrap, (void*)tc->future_time,
> +                   (void*)&res);
> +    assert_false(res);
> +    expire_test_tz("GST-2", nds_check_expired_wrap, (void*)tc->future_time,
> +                   (void*)&res);
> +    assert_false(res);
> +}
> +
> +int main(void)
> +{
> +    const struct CMUnitTest tests[] = {
> +        cmocka_unit_test_setup_teardown(test_nds_check_expire,
> +                                        expire_test_setup,
> +                                        expire_test_teardown)
> +    };
> +
> +    return cmocka_run_group_tests(tests, NULL, NULL);
> +}
> -- 
> 2.1.0
> 

> From 546e0f40f8326f733133dac842b1335f0f1bd6c8 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Mon, 16 Mar 2015 06:38:43 -0400
> Subject: [PATCH 2/3] ldap: refactor check_pwexpire_kerberos to use util func

still ack

> From 7abde9e3a85c1ca6f9cfe1a26bbb9c42a37ab3d6 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl <preichl at redhat.com>
> Date: Mon, 16 Mar 2015 06:51:37 -0400
> Subject: [PATCH 3/3] ldap: refactor nds_check_expired to use util func

still ack



More information about the sssd-devel mailing list