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]