On (23/06/14 19:36), Jakub Hrozek wrote:
Hi,
When I was reviewing PavelB's duplicate members patch, I realized our
nested group unit tests could use some improvements. Please see the
attached patches and their commit messages.
From bc8fd57de30abad8c2b5e74def22927bb2870830 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 23 Jun 2014 17:50:45 +0200
Subject: [PATCH 1/6] TESTS: Add confdb domain base DN to sss_test_ctx
Creation of the path to the domain's confdb entry was duplicated in the
tests. Rather than adding yet another duplication, I added the path as
another field of the sss_test_ctx structure.
---
From patch, it is clean that it was not used on many places.
It will be usefull in
future.
ACK
From 532cee578213b3d0d9d13229c72adefeb126f8cf Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 23 Jun 2014 17:52:03 +0200
Subject: [PATCH 2/6] TESTS: Use the right confdb path
The nested group test only worked by accident. Its confdb settings were
not applied because a wrong confdb path was used.
---
ACK
From 09ca16557b3eb556f38a959d1749160c42a33578 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 23 Jun 2014 18:07:51 +0200
Subject: [PATCH 3/6] TESTS: Fix group search base
After fixing the confdb initialization I realized the group DN couldn't
be parsed. This patch fixes that.
---
src/tests/cmocka/test_nested_groups.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/tests/cmocka/test_nested_groups.c
b/src/tests/cmocka/test_nested_groups.c
index 9f38e3dd388dcafcdaa61cb39136c30fa58b399a..bf678c2b2025e54e6ed50b8b33fe41a78ffec007
100644
--- a/src/tests/cmocka/test_nested_groups.c
+++ b/src/tests/cmocka/test_nested_groups.c
@@ -43,7 +43,7 @@
/* put users and groups under the same container so we can easily run the
* same tests cases for several search base scenarios */
#define OBJECT_BASE_DN "cn=objects,dc=test,dc=com"
-#define GROUP_BASE_DN "cn=groups" OBJECT_BASE_DN
+#define GROUP_BASE_DN "cn=groups," OBJECT_BASE_DN
Could you used constant
from SYSDB_GROUPS_CONTAINER from sysdb.h?
From 0cd01ebe4f7f61e830cb06c907b04ff07200dbd1 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 23 Jun 2014 19:06:52 +0200
Subject: [PATCH 4/6] TESTS: Do not require replies from mocked
sdap_get_generic_recv to be talloc contexts
While it's beneficial for the real implementation of
sdap_get_generic_recv() to move memory around, the mocked implementation
can just pass around pointer.
---
make sense.
ACK
From 67a524e5ce1b7c48b99104a8726b18a090550ae4 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 23 Jun 2014 19:07:41 +0200
Subject: [PATCH 5/6] TESTS: Change how mock_sysdb_user() is implemented
For the purpose of unit tests, it's better to create a user object with
a UID and a name.
---
ACK
From 3907236536cd4157ab068311b76f0b0babde2681 Mon Sep 17 00:00:00
2001
From: Jakub Hrozek <jhrozek(a)redhat.com>
Date: Mon, 23 Jun 2014 19:07:59 +0200
Subject: [PATCH 6/6] TESTS: Add more tests for nested groups processing
Adds unit test for basic group retrieval functionality as well as for
testing duplicate members in the LDAP group entry.
Could you add reference (git
hash) to patch with PB's fix?
---
src/tests/cmocka/test_nested_groups.c | 130 +++++++++++++++++++++++++++++++++-
1 file changed, 129 insertions(+), 1 deletion(-)
diff --git a/src/tests/cmocka/test_nested_groups.c
b/src/tests/cmocka/test_nested_groups.c
index bf678c2b2025e54e6ed50b8b33fe41a78ffec007..693d0fb33837643566d7a9ab0dc02bd5cff9c849
100644
--- a/src/tests/cmocka/test_nested_groups.c
+++ b/src/tests/cmocka/test_nested_groups.c
@@ -44,6 +44,7 @@
* same tests cases for several search base scenarios */
#define OBJECT_BASE_DN "cn=objects,dc=test,dc=com"
#define GROUP_BASE_DN "cn=groups," OBJECT_BASE_DN
+#define USER_BASE_DN "cn=users," OBJECT_BASE_DN
Could you used constant
from SYSDB_USERS_CONTAINER from sysdb.h?
struct nested_groups_test_ctx {
struct sss_test_ctx *tctx;
@@ -114,11 +115,136 @@ static void nested_groups_test_one_group_no_members(void **state)
assert_true(rootgroup == test_ctx->groups[0]);
}
+static void nested_groups_test_one_group_unique_members(void **state)
+{
+ struct nested_groups_test_ctx *test_ctx = NULL;
+ struct sysdb_attrs *rootgroup = NULL;
+ struct tevent_req *req = NULL;
+ TALLOC_CTX *req_mem_ctx = NULL;
+ errno_t ret;
+ const char *name;
+ const char *users[] = { "cn=user1,"USER_BASE_DN,
+ "cn=user2,"USER_BASE_DN,
+ NULL };
+ const struct sysdb_attrs *user1_reply[2] = { NULL };
+ const struct sysdb_attrs *user2_reply[2] = { NULL };
+
+ test_ctx = talloc_get_type_abort(*state, struct nested_groups_test_ctx);
+
+ /* mock return values */
+ rootgroup = mock_sysdb_group_rfc2307bis(test_ctx, GROUP_BASE_DN, 1000,
+ "rootgroup", users);
+
+ user1_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2001, "user1");
+ assert_non_null(user1_reply[0]);
+ will_return(sdap_get_generic_recv, 1);
+ will_return(sdap_get_generic_recv, user1_reply);
+
+ user2_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2002, "user2");
+ assert_non_null(user2_reply[0]);
+ will_return(sdap_get_generic_recv, 1);
+ will_return(sdap_get_generic_recv, user2_reply);
+
+ sss_will_return_always(sdap_has_deref_support, false);
+
+ /* run test, check for memory leaks */
+ req_mem_ctx = talloc_new(global_talloc_context);
+ assert_non_null(req_mem_ctx);
+ check_leaks_push(req_mem_ctx);
+
+ req = sdap_nested_group_send(req_mem_ctx, test_ctx->tctx->ev,
+ test_ctx->sdap_domain, test_ctx->sdap_opts,
+ test_ctx->sdap_handle, rootgroup);
+ assert_non_null(req);
+ tevent_req_set_callback(req, nested_groups_test_done, test_ctx);
+
+ ret = test_ev_loop(test_ctx->tctx);
+ assert_true(check_leaks_pop(req_mem_ctx) == true);
+ talloc_zfree(req_mem_ctx);
+
+ /* check return code */
+ assert_int_equal(ret, ERR_OK);
+
+ /* Check the users */
+ assert_int_equal(test_ctx->num_users, 2);
+ assert_int_equal(test_ctx->num_groups, 1);
+
+ ret = sysdb_attrs_get_string(test_ctx->users[0], SYSDB_NAME, &name);
+ assert_int_equal(ret, EOK);
+ assert_string_equal(name, "user1");
+
+ ret = sysdb_attrs_get_string(test_ctx->users[1], SYSDB_NAME, &name);
+ assert_int_equal(ret, EOK);
+ assert_string_equal(name, "user2");
+}
+
+static void nested_groups_test_one_group_dup_users(void **state)
+{
+ struct nested_groups_test_ctx *test_ctx = NULL;
+ struct sysdb_attrs *rootgroup = NULL;
+ struct tevent_req *req = NULL;
+ TALLOC_CTX *req_mem_ctx = NULL;
+ errno_t ret;
+ const char *name;
+ const char *users[] = { "cn=user1,"USER_BASE_DN,
+ "cn=user1,"USER_BASE_DN,
+ NULL };
+ const struct sysdb_attrs *user1_reply[2] = { NULL };
+ const struct sysdb_attrs *user2_reply[2] = { NULL };
+
+ test_ctx = talloc_get_type_abort(*state, struct nested_groups_test_ctx);
+
+ /* mock return values */
+ rootgroup = mock_sysdb_group_rfc2307bis(test_ctx, GROUP_BASE_DN, 1000,
+ "rootgroup", users);
+
+ user1_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2001, "user1");
+ assert_non_null(user1_reply[0]);
+ will_return(sdap_get_generic_recv, 1);
+ will_return(sdap_get_generic_recv, user1_reply);
+
+ user2_reply[0] = mock_sysdb_user(test_ctx, USER_BASE_DN, 2001, "user1");
+ assert_non_null(user2_reply[0]);
+ will_return(sdap_get_generic_recv, 1);
+ will_return(sdap_get_generic_recv, user2_reply);
+
+ sss_will_return_always(sdap_has_deref_support, false);
+
+ /* run test, check for memory leaks */
+ req_mem_ctx = talloc_new(global_talloc_context);
+ assert_non_null(req_mem_ctx);
+ check_leaks_push(req_mem_ctx);
+
+ req = sdap_nested_group_send(req_mem_ctx, test_ctx->tctx->ev,
+ test_ctx->sdap_domain, test_ctx->sdap_opts,
+ test_ctx->sdap_handle, rootgroup);
+ assert_non_null(req);
+ tevent_req_set_callback(req, nested_groups_test_done, test_ctx);
+
+ ret = test_ev_loop(test_ctx->tctx);
+ assert_true(check_leaks_pop(req_mem_ctx) == true);
+ talloc_zfree(req_mem_ctx);
+
+ /* check return code */
+ assert_int_equal(ret, ERR_OK);
^^^^^^
+
+ /* Check the users */
+ assert_int_equal(test_ctx->num_users, 1);
+ assert_int_equal(test_ctx->num_groups, 1);
+
+ ret = sysdb_attrs_get_string(test_ctx->users[0], SYSDB_NAME, &name);
+ assert_int_equal(ret, EOK);
^^^
You mixed EOK and ERR_OK. According to git history, ERR_OK is newer.
We should decide and use just one in next patches.
+ assert_string_equal(name, "user1");
+}
+
void nested_groups_test_setup(void **state)
{
struct nested_groups_test_ctx *test_ctx = NULL;
static struct sss_test_conf_param params[] = {
{ "ldap_schema", "rfc2307bis" }, /* enable nested groups */
+ { "ldap_search_base", OBJECT_BASE_DN },
+ { "ldap_user_search_base", USER_BASE_DN },
+ { "ldap_group_search_base", GROUP_BASE_DN },
{ NULL, NULL }
};
@@ -163,7 +289,9 @@ int main(int argc, const char *argv[])
};
const UnitTest tests[] = {
- new_test(one_group_no_members)
+ new_test(one_group_no_members),
+ new_test(one_group_unique_members),
+ new_test(one_group_dup_users)
};
/* Set debug level to invalid value so we can deside if -d 0 was used. */
I tested with reverted aptch "nested groups: do not fail if we get one entry
twice" and unit test fail. It's good.
PB modified function sdap_nested_group_single_step_process on two places
1) in case SDAP_NESTED_GROUP_DN_USER
2) in case SDAP_NESTED_GROUP_DN_GROUP
Unit test covers only the first part. Do we want ot also test the 2nd one?
LS