[SSSD] [PATCHES] support local overrides

Pavel Březina pbrezina at redhat.com
Mon Jul 27 17:51:06 UTC 2015


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!

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-VIEWS-TEST-add-null-check.patch
Type: text/x-patch
Size: 1898 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150727/4fe1f3c3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-SYSDB-prepare-for-LOCAL-view.patch
Type: text/x-patch
Size: 7126 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150727/4fe1f3c3/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-TOOLS-add-common-command-framework.patch
Type: text/x-patch
Size: 17243 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150727/4fe1f3c3/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-TOOLS-add-sss_override-for-local-overrides.patch
Type: text/x-patch
Size: 29395 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150727/4fe1f3c3/attachment-0003.bin>


More information about the sssd-devel mailing list