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.