[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