[SSSD] [Patch] Cmocka unit test patch for authtok module

Jakub Hrozek jhrozek at redhat.com
Wed Nov 13 14:46:38 UTC 2013


On Tue, Nov 12, 2013 at 04:53:23PM +0530, Pallavi Jha wrote:
> Hi,
> 
> I have made the required changes in the patch attached with the mail.
> Kindly review it.
> 
> With regards
> Pallavi Kumari Jha
> Computer Science Engineering
> 

Hi,

this version is much better. I only have a few comments now, see below:

> new file mode 100644
> index 0000000000000000000000000000000000000000..87e3c25ae7992ee48916725b87e616db42827462
> --- /dev/null
> +++ b/src/tests/cmocka/test_authtok.c
> @@ -0,0 +1,309 @@
> +/*
> +    SSSD
> +
> +    authtok - Utilities tests
> +
> +    Authors:
> +        Pallavi Jha <pallavikumarijha at gmail.com>
> +
> +    Copyright (C) 2013 Red Hat
> +
> +    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 <stddef.h>
> +#include <setjmp.h>
> +#include <cmocka.h>
> +#include <string.h>
> +
> +#include "util/authtok.h"
> +
> +
> +struct test_state
> +{

Can you make the above "struct test_state {" ? That would match the
rest of the SSSD code better..

> +    struct sss_auth_token *authtoken;
> +};
> +
> +static void setup(void **state)
> +{
> +    struct test_state *ts = NULL;
> +
> +    ts = talloc(NULL, struct test_state);
> +    assert_non_null(ts);
> +
> +    ts->authtoken = sss_authtok_new(ts);
> +    assert_non_null(ts->authtoken);
> +
> +    *state = (void *)ts;
> +}
> +
> +static void teardown(void **state)
> +{
> +    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
> +    talloc_free(ts);
> +}
> +
> +static void test_sss_authtok_new(void **state)
> +{
> +    struct test_state *ts = talloc_get_type_abort(*state, struct test_state);
> +    struct sss_auth_token *authtoken;
> +
> +    authtoken = sss_authtok_new(ts);
> +    assert_non_null(authtoken);
> +
> +    talloc_free(authtoken);
> +}
> +
> +/* @test_authtok_type_x : tests following functions for different value of type
> + * sss_authtok_set
> + * sss_authtok_get_type
> + * sss_authtok_get_size
> + * sss_authtok_get_data
> + * sss_authtok_get_password
> + * sss_authtok_get_ccfile
> + *
> + * @test_authtok_type_1 : type => SSS_AUTHTOK_TYPE_PASSWORD

I think you can rename the functions to test_authtok_passwd,
test_authtok_ccfile and test_authtok_empty. The names would be more
descriptive

> + * @test_authtok_type_2 : type => SSS_AUTHTOK_TYPE_CCFILE
> + * @test_authtok_type_3 : type => SSS_AUTHTOK_TYPE_EMPTY
> + */
> +
> +/* Test when type has value SSS_AUTHTOK_TYPE_PASSWORD */
> +static void test_sss_authtok_type_1(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    uint8_t *data;
> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +
> +    data = strdup("password");

"data" being uint8_t* actually causes a compilation warning here:
/home/remote/jhrozek/devel/sssd/src/tests/cmocka/test_authtok.c:93:10: warning: assigning to 'uint8_t *'
      (aka 'unsigned char *') from 'char *' converts between pointers to integer types with different
      sign [-Wpointer-sign]
    data = strdup("password");

I would suggest to compile with stricter CFLAGS to see the warnings.

I think given we know password is a string here, we can just use "const char *"
as type of "data" and then typcast to uint8_t* just when passing the password
to sss_authtok_set

> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);

This line would then say:
  ret = sss_authtok_set(ts->authtoken, type, (uint8_t *) data, len);

> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> + 
> +    assert_int_equal(ret, EACCES);

This is technically correct, but I think we should just test for the return
code being non-EOK, with:
       assert_int_not_equal(ret, EOK);

> +
> +    ret = sss_authtok_set(ts->authtoken, type, data, 0);

I think it would make sense to also test sss_authtok_set_password in
this function in addition to the more generic sss_authtok_set. Similarly
for other tests, call both sss_authtok_set with the right type and also
sss_authtok_set_$type.

> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);

I didn't understand why you called sss_authtok_get_ccfile() twice on the
same authtoken? Was one of them supposed to be _get_empty?

> + 
> +    assert_int_equal(ret, EACCES);
> +}
> +
> +/* Test when type has value SSS_AUTHTOK_TYPE_CCFILE */
> +static void test_sss_authtok_type_2(void **state)

Same comment about renaming the function.

> +{
> +    size_t len;
> +    errno_t ret;
> +    uint8_t *data;

Same comment about data type here.

> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> + 
> +    data = strdup("password");

The content of authtok of type CCFILE is usually a path to a Kerberos
credentials file. Can you change the example from password to something that
looks like a path? (It doesn't have to be real, feel free to use something like
"/path/to/cc_file")

> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_CCFILE;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> + 
> +    assert_int_equal(ret, EACCES);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +
> +    ret = sss_authtok_set(ts->authtoken, type, data, 0);
> + 
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(ts->authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(ts->authtoken));
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> + 
> +    assert_int_equal(ret, EACCES);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(data, pwd);
> +    assert_int_equal(len - 1, ret_len);
> +}
> +
> +/* Test when type has value SSS_AUTHTOK_TYPE_EMPTY */
> +static void test_sss_authtok_type_3(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    uint8_t *data;
> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> + 
> +    data = strdup("password");
> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_EMPTY;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);

I think you can safely omit "data" and "len" from this call.

> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> +    assert_null(sss_authtok_get_data(ts->authtoken));
> + 
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> + 
> +    ret = sss_authtok_set(ts->authtoken, type, '\0', 0);
> +    assert_int_equal(ret, EOK);
> +
> +    assert_int_equal(type, sss_authtok_get_type(ts->authtoken));
> +    assert_int_equal(0, sss_authtok_get_size(ts->authtoken));
> +    assert_null(sss_authtok_get_data(ts->authtoken));
> + 
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +
> +    ret = sss_authtok_get_ccfile(ts->authtoken, pwd, ret_len);
> +
> +    assert_int_equal(ret, ENOENT);
> +}
> +
> +static void test_sss_authtok_wipe_password(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    uint8_t *data;
> +    size_t ret_len;
> +    const char *pwd;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> + 
> +    data = strdup("password");
> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> +    ts = talloc_get_type_abort(*state, struct test_state);
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +
> +    sss_authtok_wipe_password(ts->authtoken);
> +
> +    ret = sss_authtok_get_password(ts->authtoken, &pwd, &ret_len);
> + 
> +    assert_int_equal(ret, EOK);
> +    assert_string_equal(pwd, "");
> +    assert_int_equal(len - 1, ret_len);

Hm, I think you actually might have found a bug here. I find it suprising
that after wiping the password the length is retained..I think the function
should set length to zero. I'll file a ticket about this and discuss with the
other developers.

> +}
> +
> +static void test_sss_authtok_copy(void **state)
> +{
> +    size_t len;
> +    errno_t ret;
> +    uint8_t *data;
> +    TALLOC_CTX *mem_ctx;
> +    struct test_state *ts;
> +    enum sss_authtok_type type;
> +    struct sss_auth_token *dest_authtoken;
> + 
> +    mem_ctx = talloc_new(NULL);

I think you can use "ts" as talloc context instead of NULL here.

> +    assert_non_null(mem_ctx);
> + 
> +    ts= talloc_get_type_abort(*state, struct test_state);
> + 
> +    dest_authtoken = sss_authtok_new(mem_ctx);
> +    assert_non_null(dest_authtoken);
> +
> +    data = strdup("password");
> +    len = strlen(data) + 1;
> +    type = SSS_AUTHTOK_TYPE_EMPTY;
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(EOK, sss_authtok_copy(ts->authtoken, dest_authtoken));
> +
> +    type = SSS_AUTHTOK_TYPE_PASSWORD;
> +    ret = sss_authtok_set(ts->authtoken, type, data, len);
> +
> +    assert_int_equal(ret, EOK);
> +
> +    ret = sss_authtok_copy(ts->authtoken, dest_authtoken);
> +
> +    assert_int_equal(ret, EOK);
> +    assert_int_equal(type, sss_authtok_get_type(dest_authtoken));
> +    assert_string_equal(data, sss_authtok_get_data(dest_authtoken));
> +    assert_int_equal(len, sss_authtok_get_size(dest_authtoken));
> +
> +    talloc_free(mem_ctx);
> +}
> +
> +int main(void)
> +{
> +    const UnitTest tests[] = {
> +        unit_test_setup_teardown(test_sss_authtok_new, setup, teardown),
> +        unit_test_setup_teardown(test_sss_authtok_type_1, setup, teardown),
> +        unit_test_setup_teardown(test_sss_authtok_type_2, setup, teardown),
> +        unit_test_setup_teardown(test_sss_authtok_type_3, setup, teardown),
> +        unit_test_setup_teardown(test_sss_authtok_wipe_password, setup,
> +                                 teardown),
> +        unit_test_setup_teardown(test_sss_authtok_copy, setup, teardown)
> +    };
> +
> +    return run_tests(tests);
> +}

Thanks for the patch! You're making progress and hopefully we'll be able to accept the
next version.



More information about the sssd-devel mailing list