On Sun, Jan 13, 2013 at 11:30:16PM +0100, Jakub Hrozek wrote:
> On Fri, Jan 11, 2013 at 10:46:57AM -0500, Simo Sorce wrote:
> > New rebase pushed
>
> Thank you, this patchset compiles and all tests pass. I don't think it is
> realistic to review every codepath in every patch, it's too big. I reviewed
> the domain initialization carefully, but then many patches "just" add the
> domain parameter to a sysdb function and adjust the callers. So I used gcov
> to see code coverage of the changed functions and read the code, but not
> that carefully as when reviewing a regular patch, if the code was covered.
>
> I attached the gcov output after my review of the first part of this
> patchset and some more unit tests. Please squash the unit tests into the
> approproate patches or simply include them on top, whatever suits you
> better.
>
> tl;dr version - it's mostly good. I only have some suggestions, nothing
> blocking the push. Given the sheer size of the patchset, I'd say that we
> should push the patchset and fix the remaining issues in separate
> tickets/patches to spare us from rebasing the whole thing when the whole
> patchset is reviewed.
>
> Patch 0001: The Big sysdb/domain split-up!
> Ack
>
> Patch 0002: Refactor sysdb initialization
> Ack with a suggestion:
>
> The sysdb initialization looks cleaner to me now. I tested
> sysdb clean initialization and upgrade and both look fine to me. While
> you are touching the sysdb init, what about removing the "alt_db_path"
> parameter of sysdb_init? It is not used anywhere and I can't think of a
> meaningful way to use it.
>
> Patch 0003: Refactor single domain initialization
> Ack. This matches the intent of the refactoring.
>
> Patch 0004: Remove the sysdb_ctx_get_domain() function.
> Ack.
>
> Patches 0005-0011: Make sysdb_*_dn() require a domain explictly
> Ack. sysdb_netgroup_base_dn() was not covered with unit test, so I wrote
> one,
>
> Patch 0012: Move range objects into their own top-level tree.
> Ack from my point of view, but Sumit would be a better authority on the
> subdomains design.
>
> Patch 0013: Pass domain to sysdb_get<pw/gr>nam() functions
> This one is bigger and more involved than the rest of refactoring
> patches. I think it would be nice to have a separate macro like
> "domain_has_subdomain" that would be more self-explanatory than
> "if (domain->parent && domain->fqnames)".
>
> I think the ENOMEM part is wrong (a leftover probably):
> + if (domain->parent && domain->fqnames) {
> + ret = ENOMEM;
> + src_name = talloc_asprintf(tmp_ctx, domain->names->fq_fmt,
> + name, domain->name);
> + } else {
>
> One thing that surprised me is dom->fqnames being set to true now. I think
> I would expect it to be true even before the patches? I know it's
> mentioned in the commit message, I'm just wondering if the code worked
> by accident until now?
>
> Other than that, retrieving users and groups by name worked for me for
> both native and trusted users and groups.
>
> Patches 0014-0015: Pass domain to sysdb_get<pwu/grg><id() functions
> Ack. Code looks fine, is covered by tests and getting both trusted and
> native users and groups by ID works fine.
> Enumeration also works fine.
>
> Patch 0016: Add domain option to sysdb_get/netgr/attrs() fns
> Ack. Code is covered, looks sane, retrieving and updating netgroups works
> fine.
>
> Patch 0017: Add domain argument to sysdb_initgroups()
> Ack. Code is covered, looks sane, user login works fine. I was suprised that
> sysdb_initgroups, in spite of being one of the most important sysdb
> functions doesn't have a unit test, so I wrote one.
>
> Patch 0018: Add domain argument to sysdb_get_user_attr()
> Ack. Code is covered, looks sane and authentication still works and sets the
> lastOnlineAuth correctly.
>
> Patch 0019-0022: Add domain to sysdb_search_user/group_by_name/uid/gid
> Ack. Code is covered. I tested the basic ID operations (get{pw,gr}{uid,nam}),
> and also some basic HBAC and SELinux sanity tests.
>
> Patch 0023: Add domain arg to sysdb_search_netgroup_by_name
> Ack. Netgroups still seem to work, code is covered and looks sane
>
> Patch 0024-0027: Add domain argument to sysdb_set_user/group/netgroup_attr()
> The last patch is misnamed, it should say "netgroup", not "group".
> Otherwise Ack, code is covered, looks sane.
>
> Patch 0028: Add domain argument to sysdb_get_new_id()
> Ack. Local domain tools still work OK. I added a sanity test to the test
> suite to cover the code.
>
> Patch 0029-0031: Add domain argument to sysdb_add{,_basic}_user()
> Ack. Retrieving users and groups still works, the code is mostly covered.
>
> Patch 0032: Add domain arguments to sysdb_add_inetgroup fns
> Ack, apart from typo in the commit message. Code is covered and
> netgroups still seem to work (both LDAP and IPA).
>
> Patch 0033-0034: Add domain argument to sysdb_store_user/group
> Ack. Saving users and groups still work, code looks OK and is covered.
>
> Patch 0035: Add domain arg to sysdb group member functions.
> Ack. There's some room for improving the code coverage, but adding both
> users and groups as members works.
>
> Patch 0036-0037: Add domain argument to sysdb_cache_passwd/auth
> Ack. Code is covered and cached auth still works.
>
> Patch 0038-0040: Add domain argument to sysdb_store/search/delete_custom
> Ack. Code is covered and functionality that uses the custom tree, such
> as HBAC, still works.
>
> That leaves less than 20 patches to go. I'll review them tomorrow.
Short version - please squash in or apply on top the attached unit
tests.
- Why was sysdb_idmap_dn made non static?
The rest looks good. See below for more information.
Patch 0041: Add domain arg to sysdb_search_users()
Ack. I added a unit test as there was none in sysdb tests.
Patch 0042: Add domain argument to sysdb_delete_user()
Ack, code looks sane, is covered. I tested by looking up a user I
removed from the server previously.
Patch 0043: Add domain argument to sysdb_search_groups()
Ack. I added a unit test as there was none in sysdb tests.
Patch 0044: Add domain argument to sysdb_delete_group()
Ack, code looks sane, is covered. I tested by looking up a group I
removed from the server previously.
Patch 0045: Add domain arg to sysdb_search/delete_netgroup()
Ack, code is covered by tests, retrieving netgroups still works.
Patch 0046: Add domain argument to sysdb_has/set_enumerated
Ack. Code is tested and enumeration still works.
Patch 0047: Add domain argument to sysdb_remove_attrs()
Ack. I added a unit test and tested with service records as well.
Patch 0048: Add domain argument to sysdb_idmap_ funcitons
Why did you make sysdb_idmap_dn non static? The code needs tests, but
that's outside the scope of the review. AD provider smoke test went OK.
Patch 0049: Add domain arguemnt to sysdb_get_real_name()
Ack by code inspection only this time. I don't have any LDAP server with
aliases.
Patch 0050: Add domain argument to sysdb autofs functions
Ack. Code is tested and basic autofs sanity test went OK.
Patch 0051: Add domain argument to sysdb selinux functions
Ack. There are no unit tests, but my selinux test scripts passed and the
code looks good.
Patch 0052: Add domain arguments to sysdb services functions
Ack. I only tested services with the proxy back end, but I think that's
good enough. They also have a unit test.
Patch 0053: Add domain arguments to sysdb ssh functions
Ack. Covered by unit tests and the CLI still works fine.
Patch 0054: Add domain arguments to sysdb sudo functions
Ack. Code needs coverage but basic sanity sudo test went fine.
Patch 0055: Add domain to some subdomain functions
Ack. I tested some basic ID operations in a trusted domain and they
worked OK.
Patch 0056: Pass the domain to upgrade functions
Visual ack. I only tested upgrade from 0.14 to 0.15 but also this
patch looks good to me.
Patch 0057: Move mpg flag to the domain where it belongs
Ack. The local domain still worked fine for me.
Patch 0058: Kill sysdb->domain
Ack
Patch 0059: Stop creating fake sysdb contexts
Ack from my point of view, but Sumit is a better authority on
subdomains. But ID operations in a trusted domain still worked.
>From 4945ab7477b29b2b7ea85cfda433758de39eb858 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Mon, 14 Jan 2013 13:27:14 +0100
Subject: [PATCH 1/3] tests: unit test for test_sysdb_search_users
---
src/tests/sysdb-tests.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index
c42f287cf0ecb4101bd65047908f4dedbaee1535..8d3b98aeecd0b2b9173ef5f22017d1f8a9c0927b
100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -1132,6 +1132,34 @@ START_TEST (test_sysdb_set_user_attr)
}
END_TEST
+START_TEST (test_sysdb_search_users)
+{
+ struct sysdb_test_ctx *test_ctx;
+ int ret;
+ const char *attrs[] = { SYSDB_NAME, NULL };
+ char *filter;
+ size_t count;
+ struct ldb_message **msgs;
+
+ /* Setup */
+ ret = setup_sysdb_tests(&test_ctx);
+ fail_if(ret != EOK, "Could not set up the test");
+
+ filter = talloc_asprintf(test_ctx,
+ "(&("SYSDB_UIDNUM"=%d)("SYSDB_SHELL"=/bin/ksh))",
+ _i);
+ fail_if(filter == NULL, "OOM");
+
+ ret = sysdb_search_users(test_ctx, test_ctx->sysdb, test_ctx->domain,
+ filter, attrs, &count, &msgs);
+ talloc_free(filter);
+ fail_if(ret != EOK, "Search failed: %d", ret);
+ fail_if(count != 1, "Did not find the expected user\n");
+
+ talloc_free(test_ctx);
+}
+END_TEST
+
START_TEST (test_sysdb_get_user_attr)
{
struct sysdb_test_ctx *test_ctx;
@@ -4854,6 +4882,9 @@ Suite *create_sysdb_suite(void)
/* Change their attribute */
tcase_add_loop_test(tc_sysdb, test_sysdb_set_user_attr, 27010, 27020);
+ /* Find the users by their new attribute */
+ tcase_add_loop_test(tc_sysdb, test_sysdb_search_users, 27010, 27020);
+
/* Verify the change */
tcase_add_loop_test(tc_sysdb, test_sysdb_get_user_attr, 27010, 27020);
--
1.8.0.2
>From 44c19d3ea3bfde05d054c7c5d024419b2ee8d511 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Mon, 14 Jan 2013 13:37:05 +0100
Subject: [PATCH 2/3] tests: adda a unit test for test_sysdb_search_groups
---
src/tests/sysdb-tests.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index
8d3b98aeecd0b2b9173ef5f22017d1f8a9c0927b..779a6c930f8b01609ad1f30c779226c2112c7ba6
100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -992,6 +992,32 @@ done:
}
END_TEST
+START_TEST (test_sysdb_search_groups)
+{
+ struct sysdb_test_ctx *test_ctx;
+ int ret;
+ const char *attrs[] = { SYSDB_NAME, NULL };
+ char *filter;
+ size_t count;
+ struct ldb_message **msgs;
+
+ /* Setup */
+ ret = setup_sysdb_tests(&test_ctx);
+ fail_if(ret != EOK, "Could not set up the test");
+
+ filter = talloc_asprintf(test_ctx, "("SYSDB_GIDNUM"=%d)", _i);
+ fail_if(filter == NULL, "OOM");
+
+ ret = sysdb_search_groups(test_ctx, test_ctx->sysdb, test_ctx->domain,
+ filter, attrs, &count, &msgs);
+ talloc_free(filter);
+ fail_if(ret != EOK, "Search failed: %d", ret);
+ fail_if(count != 1, "Did not find the expected group\n");
+
+ talloc_free(test_ctx);
+}
+END_TEST
+
START_TEST (test_sysdb_getpwuid)
{
struct sysdb_test_ctx *test_ctx;
@@ -4896,6 +4922,9 @@ Suite *create_sysdb_suite(void)
/* Verify the groups can be queried by GID */
tcase_add_loop_test(tc_sysdb, test_sysdb_getgrgid, 28010, 28020);
+ /* Find the users by GID using a filter */
+ tcase_add_loop_test(tc_sysdb, test_sysdb_search_groups, 28010, 28020);
+
/* Enumerate the groups */
tcase_add_test(tc_sysdb, test_sysdb_enumgrent);
--
1.8.0.2
>From 62f1935f995f7ca5100a2d370c4b529fd31a0977 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhro...@redhat.com>
Date: Mon, 14 Jan 2013 14:01:04 +0100
Subject: [PATCH 3/3] tests: unit test for sysdb_remove_attrs
---
src/tests/sysdb-tests.c | 45 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index
779a6c930f8b01609ad1f30c779226c2112c7ba6..0b042cbd807fc43b6a9f874174c182553ab668b0
100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -1186,6 +1186,48 @@ START_TEST (test_sysdb_search_users)
}
END_TEST
+START_TEST (test_sysdb_remove_attrs)
+{
+ struct sysdb_test_ctx *test_ctx;
+ int ret;
+ char *rmattrs[2];
+ char *username;
+ struct ldb_result *res;
+ const char *shell;
+
+ ret = setup_sysdb_tests(&test_ctx);
+ fail_if(ret != EOK, "Could not set up the test");
+
+ username = talloc_asprintf(test_ctx, "testuser%d", _i);
+ fail_if(username == NULL, "OOM");
+
+ ret = sysdb_getpwnam(test_ctx,
+ test_ctx->sysdb,
+ test_ctx->domain,
+ username, &res);
+ fail_if(ret != EOK, "sysdb_getpwnam failed for username %s (%d: %s)",
+ username, ret, strerror(ret));
+ shell = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_SHELL, NULL);
+ fail_unless(shell != NULL, "Did not find user shell before removal");
+
+ rmattrs[0] = discard_const(SYSDB_SHELL);
+ rmattrs[1] = NULL;
+
+ ret = sysdb_remove_attrs(test_ctx->sysdb, test_ctx->domain,
+ username, SYSDB_MEMBER_USER, rmattrs);
+ fail_if(ret != EOK, "Removing attributes failed: %d", ret);
+
+ ret = sysdb_getpwnam(test_ctx,
+ test_ctx->sysdb,
+ test_ctx->domain,
+ username, &res);
+ fail_if(ret != EOK, "sysdb_getpwnam failed for username %s (%d: %s)",
+ username, ret, strerror(ret));
+ shell = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_SHELL, NULL);
+ fail_unless(shell == NULL, "Found user shell after removal");
+}
+END_TEST
+
START_TEST (test_sysdb_get_user_attr)
{
struct sysdb_test_ctx *test_ctx;
@@ -4914,6 +4956,9 @@ Suite *create_sysdb_suite(void)
/* Verify the change */
tcase_add_loop_test(tc_sysdb, test_sysdb_get_user_attr, 27010, 27020);
+ /* Remove the attribute */
+ tcase_add_loop_test(tc_sysdb, test_sysdb_remove_attrs, 27010, 27020);
+
/* Create a new group */
tcase_add_loop_test(tc_sysdb, test_sysdb_store_group, 28010, 28020);
--
1.8.0.2
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
