[SSSD] [PATCHES] support local overrides

Jakub Hrozek jhrozek at redhat.com
Mon Jul 27 20:55:44 UTC 2015


On Mon, Jul 27, 2015 at 10:00:03PM +0200, Jakub Hrozek wrote:
> 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

Pushed to master:
    * b69cb1787209e85cc246eb9a944242689bfe0c46
    * 284937e6b5b0c9d7a1d3382d0d2820d1168842fb
    * a8d31510d12af6ee39fb3e1e13f3a4f6bdef33c1
    * 166a622bcba319aaa7190fbc348af887cbba30bf



More information about the sssd-devel mailing list