On Fri, Sep 21, 2012 at 08:03:17AM -0400, Stephen Gallagher wrote:
> On 09/21/2012 05:41 AM, Jakub Hrozek wrote:
> > On Thu, Sep 20, 2012 at 09:34:37AM -0400, Stephen Gallagher wrote:
> >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> >>
> >> * [PATCH 1/2] AD: Detect domain controller compatibility version
> >> This patch allows us to read the domain controller version from
> >> the RootDSE, if it exists. This will be used by the next patch
> >> to determine if the SSSD can use tokenGroups for lookups.
> >>
> >> * [PATCH 2/2] AD: Optimize initgroups lookups with tokenGroups
> >> If we are doing ID-mapped lookups, we can take advantage of the
> >> AD-specific option "tokenGroups", which allows us to retrieve a
> >> pre-flattened list of all the group SIDs for which this user is a
> >> member. From this, we can convert them to GIDs and conclude the
> >> initgroups() lookup with only two LDAP queries (the search for
> >> the user and the base search for the user's tokengroups; the
> >> latter must be performed as a base search due to restrictions
> >> imposed on the tokenGroups attribute by AD).
> >>
> >> When we save these groups to the sysdb, if the group is being
> >> seen for the first time it will be temporarily given the name of
> >> the SID representing it. Later, when we look it up by name or
> >> GID, this group will be replaced with the real one.
> >>
> >
> > I only have one question:
> >
> > + /* Get the list of group SIDs */ + ret =
> > sysdb_attrs_get_el_ext(users[0], AD_TOKENGROUPS_ATTR, + false,
> > &el); + if (ret != EOK) { + if (ret == ENOENT) { +
> > DEBUG(SSSDBG_TRACE_LIBS, + ("No tokenGroups
> > entries for [%s]\n", + state->username));
> >
> > Should we just return success here? Does a missing togenGroup
> > imply a user that does not belong to any group?
> >
> > + } + DEBUG(SSSDBG_MINOR_FAILURE, + ("Could not read
> > tokenGroups attribute: [%s]\n", + strerror(ret))); + goto
> > done; + }
> >
>
> You're right, I meant to do more with that IF block and forgot about
> it (though in reality, that's a branch we should never take since all
> users are members of at least the built-in "Users" group that we later
> filter out).
>
> Just returning success here isn't sufficient, though. We need to make
> sure that the sysdb matches. The attached patch creates a new
> ldb_message_element with zero members that will short-circuit the
> group-processing loop below and advance on to where we correct the
> group memberships.
>
> Additionally, while fixing this I realized that I had forgotten to use
> the _recv() function in sdap_async_initgroups.c. It had been working
> by lucky coincidence because the _recv() function it was calling
> instead happened to have the exact same implementation (checking only
> for errors, not requiring a specific state variable). This is now also
> corrected.
>
Oops, I missed that, I did take a look that the new _recv() function is
the same as the other becaue initially it struck me that it doesn't
return anything. I didn't check it gets called correctly, though..
> Thanks for the review!
>
>
Ack to both patches.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
