[SSSD] [PATCHES] Add backend part of user views

Pavel Březina pbrezina at redhat.com
Mon Sep 29 12:52:10 UTC 2014


On 09/29/2014 01:01 PM, Sumit Bose wrote:
> On Mon, Sep 29, 2014 at 11:52:52AM +0200, Pavel Březina wrote:
>> On 09/26/2014 04:36 PM, Sumit Bose wrote:
>>> On Fri, Sep 26, 2014 at 01:41:39PM +0200, Pavel Březina wrote:
>>>> On 09/26/2014 01:38 PM, Pavel Březina wrote:
>>>>> On Wed, Sep 24, 2014 at 10:25:22AM +0200, Sumit Bose wrote:
>>>>>> Hi,
>>>>>>
>>>>>> this patches contain the IPA provider part of
>>>>>> https://fedorahosted.org/sssd/ticket/2375  as described in the FreeIPA
>>>>>> design page
>>>>>> http://www.freeipa.org/page/V4/Migrating_existing_environments_to_Trust
>>>>>>
>>>>>> Patches 0001, 0006 and 0007 contain new sysdb interfaces for views and
>>>>>> overrides. 0002 and 0003 refactor some already existing calls.
>>>>>>
>>>>>> 0004 adds the initial support for views and overrides and reads the view
>>>>>> name for the client from the FreeIPA server or sets it to 'default' when
>>>>>> running in ipa-server-mode. It adds some new options which currently
>>>>>> misses man page entries and are not added to the python config API. I
>>>>>> did this on purpose for the time being because I wanted to see first if
>>>>>> the list is complete or if some options ca be dropped.
>>>>>>
>>>>>> 0005 adds the request to get a specific override from the FreeIPA server
>>>>>> and finally in 0008 the overrides are read during a request and saves or
>>>>>> applied.
>>>>>>
>>>>>> This patches must be applied on to of the extdom patch I send yesterday.
>>>>>> To add and manage views and override on the server side please have a
>>>>>> look at Tomas's patch set on the freeipa-devel list
>>>>>> (https://www.redhat.com/archives/freeipa-devel/2014-September/msg00336.html).
>>>>>>
>>>>>>
>>>>>> bye,
>>>>>> Sumit
>>>
>>> Pavel, Jakub, thank you for the review.
>>>
>>>>>
>>>>> >From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
>>>>>> From: Sumit Bose <sbose at redhat.com>
>>>>>> Date: Tue, 16 Sep 2014 15:18:53 +0200
>>>>>> Subject: [PATCH 1/8] sysdb: add sysdb_update_view_name()
>>>>>
>>>>> ACK
>>>>>
>>>>> A unit test for the functions would be nice in order to avoid decreasing
>>>>> the code coverage of the sysdb module, but I realize we have a deadline.
>>>>> We should file a ticket, though.
>>>
>>> yes, I feel quite bad about the missing test, but my plan is to add them
>>> after the dealine
>>>
>>>>>
>>>>> It would also be nice to add a comment that it's not possible to have a
>>>>> view w/o a name but we are trying to be on the safe side b/c name is not
>>>>> MUST as we discussed on IRC with Sumit.
>>>
>>> done
>>
>> Ack.
>>
>>>>>
>>>>> >From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
>>>>>> From: Sumit Bose <sbose at redhat.com>
>>>>>> Date: Tue, 16 Sep 2014 15:22:08 +0200
>>>>>> Subject: [PATCH 2/8] Add sdap_deref_search_with_filter_send()
>>>>>
>>>>> I would prefer to also add a matching _done and _recv functions, even if
>>>>> they were wrappers around existing functions. But tevent coding style is
>>>>> important to follow, we already had bugs about reusing a single tevent
>>>>> callbacks for multiple requests.
>>
>>> +    if (sdap_is_control_supported(sh, LDAP_CONTROL_X_DEREF)) {
>>> +        DEBUG(SSSDBG_TRACE_INTERNAL, "Server supports OpenLDAP deref\n");
>>> +        state->deref_type = SDAP_DEREF_OPENLDAP;
>>> +
>>> +        subreq = sdap_x_deref_search_send(state, ev, opts, sh, search_base,
>>> +                                          filter, deref_attr, attrs, maps,
>>> +                                          num_maps, timeout);
>>> +        if (!subreq) {
>>> +            DEBUG(SSSDBG_OP_FAILURE, "Cannot start OpenLDAP deref search\n");
>>> +            goto fail;
>>> +        }
>>> +    } else {
>>> +        DEBUG(SSSDBG_OP_FAILURE,
>>> +              "Server does not support any known deref method!\n");
>>> +        goto fail;
>>> +    }
>>
>>
>> Would it be possible to use sdap_has_deref_support() instead of manually
>> testing the support for OpenLDAP deref or you can't use ASQ for some reason?
>
> no, afaik ASQ can only do base searches and follow the references in the
> given DN, so adding a filter won't work.
>
>>
>>>
>>> done
>>>
>>>>>
>>>>> We can do it post-rebase, though. Just please file a ticket.
>>>>>
>>>>>
>>>>> >From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
>>>>>> From: Sumit Bose <sbose at redhat.com>
>>>>>> Date: Fri, 19 Sep 2014 10:26:01 +0200
>>>>>> Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
>>>>>> client code
>>>>>
>>>>> ACK
>>
>> Still Ack.
>>
>>>>>
>>>>> ...aaand I stopped here.
>>>>>
>>>>> That was Jakub's part of the review.
>>>>>
>>>>> Patch 4: IPA: add view support and get view name
>>>>>
>>>>>>          if (ret != EOK) {
>>>>>> -            DEBUG(SSSDBG_CRIT_FAILURE, "Unable to set SRV lookup
>>>>>> plugin "
>>>>>> -                                        "[%d]: %s\n", ret,
>>>>>> strerror(ret));
>>>>>> -            goto done;
>>>>>> +            if (ret == ENOENT) {
>>>>>> +                DEBUG(SSSDBG_CRIT_FAILURE,
>>>>>> +                      "Cannot find view name in the cache. " \
>>>>>> +                      "Will do online llokup later\n");
>>>>>                                           ^ typo
>>>
>>> fixed
>>>
>>>>>> +            } else {
>>>>>> +                DEBUG(SSSDBG_OP_FAILURE, "sysdb_get_view_name
>>>>>> failed.\n");
>>>>>> +                goto done;
>>>>>> +            }
>>>>>> +        }
>>>>>
>>>>>>      ctx->ranges_search_bases = id_ctx->ipa_options->ranges_search_bases;
>>>>>> +    ctx->host_search_bases = id_ctx->ipa_options->host_search_bases;
>>>>>>      ctx->configured_explicit = configured_explicit;
>>>>>
>>>>> This change seems to be unrelated to this patch. Shouldn't it be a
>>>>> separate commit?
>>>
>>> no, the view name is an attribute of the host object so we must be able
>>> to search it.
>>
>> Ok. Typo is still there, I'm sending a trivial patch. Feel free to squash
>> it.
>
> squashed in.
>
>>
>> Ack.
>>
>>>
>>>>>
>>>>> Patch 5: views: add ipa_get_ad_override_send()
>>>>>
>>>>> Looks good, but can you add some more tracing debug messages there? For
>>>>> example what view are we searching for and that an override was found.
>>>
>>> done
>>
>> Ack.
>>
>>>
>>>>>
>>>>> Patch 6:
>>>>>
>>>>>> +    const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
>>>>>> +                              SYSDB_OVERRIDE_DN,
>>>>>> +                                   NULL};
>>>>>
>>>>> Indentation.
>>>
>>> fixed
>>>
>>>>>
>>>>>> +        if (ret != EOK) {
>>>>>> +            DEBUG(SSSDBG_CRIT_FAILURE,
>>>>>> +                  "Missing anchor in overide attributes.\n");
>>>>>                                             ^ typo
>>>
>>> fixed
>>>
>>>>>> +            ret = EINVAL;
>>>>>> +            goto done;
>>>>>> +        }
>>>>>
>>>>>> +    if (ret != EOK) {
>>>>>> +        if (ret == ENOENT) {
>>>>>> +            DEBUG(SSSDBG_CRIT_FAILURE, "Object to override does not
>>>>>> exists.\n");
>>>>>> +        } else {
>>>>>> +            DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_entry failed.\n");
>>>>>> +        }
>>>>>> +        goto done;
>>>>>> +    }
>>>>>
>>>>> Can you use if (ret == ENOENT) else if (ret != EOK) here? I think it is
>>>>> simpler.
>>>
>>> I prefer it the other way, becasue I think it makes it more clear that
>>> it is an error in all cases and only adds a special messages for the
>>> ENOENT case.
>>
>> Ok.
>>
>>>
>>>>>
>>>>>> +    if (obj_override_dn != NULL) {
>>>>>> +        if (strcmp(obj_override_dn, override_dn_str) != 0) {
>>>>>> +            DEBUG(SSSDBG_CRIT_FAILURE,
>>>>>> +                  "Existing [%s] and new [%s] overid DN do not
>>>>>> match.\n",
>>>>>                                                   ^ typo
>>>
>>> fixed
>>>
>>>>>> +                   obj_override_dn, override_dn_str);
>>>>>> +            ret = EINVAL;
>>>>>> +            goto done;
>>>>>> +        }
>>>>>> +
>>>>>> +        add_ref = false;
>>>>>
>>> +    }
>>
>>> +        case SYSDB_MEMBER_GROUP:
>>> +            ret = ldb_msg_add_string(msg, SYSDB_OBJECTCLASS,
>>> +                                     SYSDB_OVERRIDE_USER_CLASS);
>>> +            break;
>>
>> Shouldn't it say SYSDB_OVERRIDE_GROUP_CLASS?
>
> D'oh, fixed.
>
>>
>>>>>
>>>>> Patch 7: sysdb: sysdb_apply_default_override
>>>>>
>>>>> Ack.
>>
>> Ack stays, but can you clarify what is the intended use case of this
>> function in the commit message please?
>
> Comment added.
>
>>
>>>>>
>>>>> Patch 8: views: get overrides during user and group lookups
>>>>>
>>>>> Ack.
>>
>>>     if (override_attrs != NULL) {
>>>         ret = sysdb_apply_default_override(state->user_dom->sysdb,
>>>                                            override_attrs,
>>>                                            state->obj_msg->dn);
>>>         if (ret != EOK) {
>>>             DEBUG(SSSDBG_OP_FAILURE, "sysdb_apply_default_override failed.\n");
>>>             goto fail;
>>>         }
>>>     }
>>
>> I thought the sysdb_apply_default_override is used to override original
>> object in sysdb with information from the default view. This looks like it
>> is overwritten with data from whatever view we are using.
>
> This code path is in ipa-server-mode where the default view is
> hardcoded/fixed by definition. I added a comment to make this clear.
>
>>
>>>>
>>>> BTW I checked out your repo. The patches couldn't be applied to master even
>>>> with the extdom patch here on the list.
>>>
>>> I send a new version of the extdom patch as well all rebase on master, I
>>> hope it will work now.
>>>
>>> New version attached.
>>>
>>> bye,
>>> Sumit
>>
>> I get this warning:
>> /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c: In function
>> 'ipa_get_ad_override_done':
>> /home/pbrezina/workspace/sssd/src/providers/ipa/ipa_views.c:207:9: error:
>> format '%d' expects argument of type 'int', but argument 6 has type 'size_t'
>> [-Werror=format]
>
> Your fix is squashed in.
>
> Thank you for the review. New version attached.
>
> bye,
> Sumit

Hi,
I'm sending two more trivial debug patches to squash in. Otherwise its 
an code wise ack. I'm running some tests now.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-fix-debug-message-squash-into-IPA-add-view-support-a.patch
Type: text/x-patch
Size: 1039 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140929/7efc0206/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-decrease-debug-level-squash-into-IPA-add-view-suppor.patch
Type: text/x-patch
Size: 1412 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140929/7efc0206/attachment-0001.bin>


More information about the sssd-devel mailing list