On Mon, Jul 27, 2015 at 05:27:29PM +0200, Pavel Březina wrote:
On 07/27/2015 05:07 PM, Jakub Hrozek wrote:
On Mon, Jul 27, 2015 at 12:00:25PM +0200, Pavel Březina wrote:
On 07/26/2015 08:14 PM, Jakub Hrozek wrote:
On Fri, Jul 24, 2015 at 01:08:17PM +0200, Pavel Březina wrote:
btw (unrelated to these patches) it seems strange to require the user of the sysdb database to connect to the db and then separately initialize subdomains...I would vote for merging that code..
+1
I'll file a ticket for michal.
+#include <talloc.h> +#include <popt.h>
+#include "confdb/confdb.h"
+struct sss_tool_ctx {
- struct confdb_ctx *confdb;
- char *confdb_path;
- char *default_domain;
- struct sss_domain_info *domains;
I wonder if the tool_ctx must be open for consumers? Currently only domains is used, maybe we could get away with a getter and an opaque structure? We can always make the structure public in future, but if it's public from the start, then it's easier to add cruft..
I think it is not worth the effort with such simple structure. I am all for opaque structures but here is just no gain in code safety. It would only make code lines bigger.
Fine.
+enum sss_tool_domain {
- SSS_TOOL_DOMAIN_REQUIRED,
- SSS_TOOL_DOMAIN_OPTIONAL
+};
+int sss_tool_parse_name(TALLOC_CTX *mem_ctx,
struct sss_tool_ctx *tool_ctx,const char *input,enum sss_tool_domain require_domain,Do we need require_domain now that you added the getpwnam?
No, removed.
Thanks
[...]
Hmm looks like you should have a check for res validity here, for cases no domain match..
Added a dom validity.
Thanks.
btw it might be nice to split this large function into smaller ones. Maybe the do-while check could be a separate function returning res.
There is no way to split it that would be actually beneficial. There are three blocks: a) beginning - getpwnam b) middle - iteration c) end - linearized dn
If you take away the iteration you can directly return dn from the new function since you don't need res at all so that would also took away the end. And the beginning belongs to the function were iteration is IMHO.
OK
From 45e80988edc6dab703209068942fad5378d38e72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 24 Jul 2015 09:55:28 +0200 Subject: [PATCH 1/3] SYSDB: prepare for LOCAL view
Objects doesn't have to have overrideDN specified when using LOCAL view. Since the view is not stored on the server we do not want to contact LDAP therefore we special case LOCAL view saying that it is OK that this attribute is missing.
Preparation for: https://fedorahosted.org/sssd/ticket/2584
src/db/sysdb.h | 3 +- src/db/sysdb_views.c | 7 +++++ src/tests/cmocka/test_sysdb_views.c | 62 +++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 0f745ccb1a646d77ba4ad3d714d5f4dce0a51211..3bb2e50320594d1b6f7e3daa3a64134e46b60d22 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -157,9 +157,10 @@ #define SYSDB_AD_ACCOUNT_EXPIRES "adAccountExpires" #define SYSDB_AD_USER_ACCOUNT_CONTROL "adUserAccountControl"
+#define SYSDB_DEFAULT_VIEW_NAME "default" +#define SYSDB_LOCAL_VIEW_NAME "LOCAL" /* reserved for client-side overrides */ #define SYSDB_VIEW_CLASS "view" #define SYSDB_VIEW_NAME "viewName" -#define SYSDB_DEFAULT_VIEW_NAME "default" #define SYSDB_OVERRIDE_CLASS "overrride" #define SYSDB_OVERRIDE_ANCHOR_UUID "overrideAnchorUUID" #define SYSDB_OVERRIDE_USER_CLASS "userOverride" diff --git a/src/db/sysdb_views.c b/src/db/sysdb_views.c index aadd6018f4d1e2ca33e2e00dd8b13b55a8c03f3e..f4560344e992d8245e37a5a4e2f74c7b70ce41ec 100644 --- a/src/db/sysdb_views.c +++ b/src/db/sysdb_views.c @@ -1186,9 +1186,16 @@ errno_t sysdb_add_overrides_to_object(struct sss_domain_info *domain, override_dn_str = ldb_msg_find_attr_as_string(obj, SYSDB_OVERRIDE_DN, NULL); if (override_dn_str == NULL) {
if (strcmp(domain->view_name, SYSDB_LOCAL_VIEW_NAME) == 0) {/* LOCAL view doesn't have to have overrideDN specified. */ret = EOK;goto done;}DEBUG(SSSDBG_CRIT_FAILURE, "Missing override DN for objext [%s].\n", ldb_dn_get_linearized(obj->dn));ret = ENOENT; goto done; }diff --git a/src/tests/cmocka/test_sysdb_views.c b/src/tests/cmocka/test_sysdb_views.c index 1fb598219e9ee581e465ddbb32ba9f2544600c26..5d2d50ef94093664465305b53831ed878cf2c871 100644 --- a/src/tests/cmocka/test_sysdb_views.c +++ b/src/tests/cmocka/test_sysdb_views.c @@ -275,6 +275,64 @@ void test_sysdb_add_overrides_to_object(void **state) assert_int_equal(ldb_val_string_cmp(&el->values[1], "OVERRIDEKEY2"), 0); }
+void test_sysdb_add_overrides_to_object_local(void **state) +{
- int ret;
- struct ldb_message *orig;
- struct ldb_message_element *el;
- char *tmp_str;
- struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
struct sysdb_test_ctx);- orig = ldb_msg_new(test_ctx);
- assert_non_null(orig);
- tmp_str = talloc_strdup(orig, "ORIGNAME");
Please put assert_non_null here and to other assignments to tmp_str in these two tests, otherwise ACK
- ret = ldb_msg_add_string(orig, SYSDB_NAME, tmp_str);
- assert_int_equal(ret, EOK);
- tmp_str = talloc_strdup(orig, "ORIGGECOS");
- ret = ldb_msg_add_string(orig, SYSDB_GECOS, tmp_str);
- assert_int_equal(ret, EOK);
- test_ctx->domain->has_views = true;
- test_ctx->domain->view_name = "LOCAL";
- ret = sysdb_add_overrides_to_object(test_ctx->domain, orig, NULL, NULL);
- assert_int_equal(ret, EOK);
+}
From 5a7b46aee3acd9a1d40cc85cb8085e9d43f48a7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 22 Jul 2015 10:02:02 +0200 Subject: [PATCH 2/3] TOOLS: add common command framework
ACK
From 3babec59cb170457c441000b66ff76895526aba7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 24 Jul 2015 09:58:11 +0200 Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
There was one more Coverity warning: Error: UNINIT (CWE-457): [#def1] sssd-1.13.1/src/tools/sss_override.c:202: var_decl: Declaring variable "ret" without initializer. sssd-1.13.1/src/tools/sss_override.c:252: uninit_use: Using uninitialized value "ret". # 250| # 251| done: # 252|-> if (ret != EOK) { # 253| talloc_free(attrs); # 254| return NULL;
Otherwise looks good. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks, new patches are attached. In addition to your comments:
- one new patch that adds null check in original unit test
- man page and tool change that says that zero ids are not supported
I am still waiting for Coverity and CI results.
From db9ba08b8c359eec318fa3dfc28e038618ea35f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 27 Jul 2015 17:24:45 +0200 Subject: [PATCH 1/4] VIEWS TEST: add null-check
ACK
From 9b67c5777f51cedd1b6669d0f103c232e80e9ad6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 24 Jul 2015 09:55:28 +0200 Subject: [PATCH 2/4] SYSDB: prepare for LOCAL view
ACK
From 39b1bf17cb52e000c8ef74862fb88a4762176730 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Wed, 22 Jul 2015 10:02:02 +0200 Subject: [PATCH 3/4] TOOLS: add common command framework
ACK
From 99c97b41a6feeb0edd5be11d27b8e832d4694843 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Fri, 24 Jul 2015 09:58:11 +0200 Subject: [PATCH 4/4] TOOLS: add sss_override for local overrides
there is still an issue with user getpwnam finds but sysdb_getpwnam not:
(gdb) set args user-add root -n toor (gdb) r Starting program: /usr/sbin/sss_override user-add root -n toor Missing separate debuginfos, use: dnf debuginfo-install glibc-2.21-5.fc22.x86_64 [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib64/libthread_db.so.1". ldb: unable to dlopen /usr/lib64/ldb/modules/ldb/memberof.la : /usr/lib64/ldb/modules/ldb/memberof.la: invalid ELF header
Program received signal SIGSEGV, Segmentation fault. override_user_add (cmdline=0x7fffffffe390, tool_ctx=0x6110c0, pvt=0x0) at /sssd/src/tools/sss_override.c:567 567 fprintf(stderr, _("Unable to find user %s@%s.\n"), Missing separate debuginfos, use: dnf debuginfo-install bzip2-libs-1.0.6-14.fc22.x86_64 cyrus-sasl-lib-2.1.26-22.fc22.x86_64 dbus-libs-1.8.18-1.fc22.x86_64 elfutils-libelf-0.161-6.fc22.x86_64 elfutils-libs-0.161-6.fc22.x86_64 glib2-2.44.1-1.fc22.x86_64 libattr-2.4.47-9.fc22.x86_64 libbasicobjects-0.1.1-24.fc22.x86_64 libcap-2.24-7.fc22.x86_64 libcollection-0.6.2-24.fc22.x86_64 libdhash-0.4.3-24.fc22.x86_64 libgcc-5.1.1-1.fc22.x86_64 libgcrypt-1.6.3-4.fc22.x86_64 libgpg-error-1.17-2.fc22.x86_64 libini_config-1.1.0-24.fc22.x86_64 libldb-1.1.20-1.fc22.x86_64 libpath_utils-0.2.1-24.fc22.x86_64 libref_array-0.1.4-24.fc22.x86_64 libselinux-2.3-10.fc22.x86_64 libtalloc-2.1.2-1.fc22.x86_64 libtdb-1.3.4-1.fc22.x86_64 libtevent-0.9.24-1.fc22.x86_64 nspr-4.10.8-1.fc22.x86_64 nss-3.19.2-1.0.fc22.x86_64 nss-softokn-freebl-3.19.2-1.0.fc22.x86_64 nss-util-3.19.2-1.0.fc22.x86_64 openldap-2.4.40-12.fc22.x86_64 openssl-libs-1.0.1k-11.fc22.x86_64 pcre-8.37-2.fc22.x86_64 popt-1.16-5.fc22.x86_64 systemd-libs-219-19.fc22.x86_64 xz-libs-5.2.0-2.fc22.x86_64 zlib-1.2.8-7.fc22.x86_64 (gdb) list 567 562 return EXIT_FAILURE; 563 } 564 565 dn = get_user_dn_and_domain(tool_ctx, tool_ctx->domains, &user); 566 if (dn == NULL) { 567 fprintf(stderr, _("Unable to find user %s@%s.\n"), 568 user.orig_name, user.domain->name); 569 return EXIT_FAILURE; 570 } 571 (gdb) p user $1 = {input_name = 0x7fffffffe7b5 "root", orig_name = 0x611a30 "root", domain = 0x0, name = 0x619c00 "toor", uid = 0, gid = 0, home = 0x0, shell = 0x0, gecos = 0x0}
Same issue is there with groups btw.