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(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?
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(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.
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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel