[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