[SSSD] [PATCHES] support local overrides

Jakub Hrozek jhrozek at redhat.com
Mon Jul 27 20:00:03 UTC 2015


On Mon, Jul 27, 2015 at 07:51:06PM +0200, Pavel Březina wrote:
> On 07/27/2015 05:27 PM, 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:
> >>>>>https://fedorahosted.org/sssd/ticket/2584
> >>>>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 at 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 at 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 at redhat.com>
> >>>Date: Fri, 24 Jul 2015 09:58:11 +0200
> >>>Subject: [PATCH 3/3] TOOLS: add sss_override for local overrides
> >>>
> >>>Resolves:
> >>>https://fedorahosted.org/sssd/ticket/2584
> >>
> >>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 at 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
> 
> Few more bug fixed during irc review. Thanks Jakub!
> 

These patches work well for me, all attributes can be overriden,
existing overrides in IPA case are detected and the crash is gone.

Both CI and Coverity are happy as well.

ACK



More information about the sssd-devel mailing list