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(a)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(a)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?
>
> 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(a)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.
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?
>>>
>>> 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?
>>>
>>> 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.
>>
>> 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]