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.
Thanks for the review!
> Fixes
https://fedorahosted.org/sssd/ticket/1355
I'm going to move the ticket to 1.9.0. This enhancement is not
really intrusive and provides performance gain so big that I don't
think we should wait.
_______________________________________________ sssd-devel mailing
list sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel