[SSSD] [PATCHES] support local overrides

Jakub Hrozek jhrozek at redhat.com
Mon Jul 27 15:07:58 UTC 2015


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.



More information about the sssd-devel mailing list