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

[Pavel Březina]

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
> > From dedbb5ae2faa13d396e84ce9911ab1e9c0246836 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > 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.
>
> 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.
> > From c09a8553d7371e46b84e980f38d017c7f84c2f87 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > 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.
>
> We can do it post-rebase, though. Just please file a ticket.
> > From 3097c76ba9498b90df69935d24dc9ce55f602937 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Fri, 19 Sep 2014 10:26:01 +0200
> > Subject: [PATCH 3/8] IPA: make IPA ID context available to extdom
> > client code
>
> 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
> >        } 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?
>
> 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.
>
> Patch 6:
> > const char *obj_attrs[] = { SYSDB_OBJECTCLASS,
> >                          SYSDB_OVERRIDE_DN,
> >                               NULL};
>
> Indentation.
> >    if (ret != EOK) {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "Missing anchor in overide attributes.\n");
>                                         ^ typo
> >        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.
> > 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
> >               obj_override_dn, override_dn_str);
> >        ret = EINVAL;
> >        goto done;
> >    }
> >    add_ref = false;
> > }
>
> Patch 7: sysdb: sysdb_apply_default_override
>
> Ack.
>
> Patch 8: views: get overrides during user and group lookups
>
> Ack.

BTW I checked out your repo. The patches couldn't be applied to master 
even with the extdom patch here on the list.