Hi, I'm sending the first version of the patch solving issues with nested groups without GID. The patch is definitely not final, let's say it's the first draft of final patch. It depends on Jakub's recent series of rfc2307bis patches and it won't be final until they are pushed.
At this moment I'd like to hear your opinion on the concept of the patch.
Thanks in advance Jan
On 05/14/2011 01:29 AM, Jan Zeleny wrote:
Hi, I'm sending the first version of the patch solving issues with nested groups without GID. The patch is definitely not final, let's say it's the first draft of final patch. It depends on Jakub's recent series of rfc2307bis patches and it won't be final until they are pushed.
At this moment I'd like to hear your opinion on the concept of the patch.
Thanks in advance Jan
Instead of modifying the global search filter to not include GID and using the generic sdap_nested_group_lookup_group(), why not create a separate function with a special non-posix search filter as a fallback if neither user nor posix-group filters match?
I haven't done a full review but two other comments -
I think that a sysdb_attrs_get_bool function would be handy instead of handling the TRUE/FALSE strings ourselves.
The SYSDB_POSIX attribute should be named differently to indicate it is a bool, "isPosix" perhaps. "posixGroup" is commonly used as objectlass.
On 05/14/2011 01:29 AM, Jan Zeleny wrote:
Hi, I'm sending the first version of the patch solving issues with nested groups without GID. The patch is definitely not final, let's say it's the first draft of final patch. It depends on Jakub's recent series of rfc2307bis patches and it won't be final until they are pushed.
At this moment I'd like to hear your opinion on the concept of the patch.
Thanks in advance Jan
Instead of modifying the global search filter to not include GID and using the generic sdap_nested_group_lookup_group(), why not create a separate function with a special non-posix search filter as a fallback if neither user nor posix-group filters match?
Frankly I don't like the idea of creating new functions with almost the same code. The file has 5.5k lines as it is and I don't want to add more just to separate couple lines of code.
I haven't done a full review but two other comments -
I think that a sysdb_attrs_get_bool function would be handy instead of handling the TRUE/FALSE strings ourselves.
I was considering this too, thanks for this comment :-)
The SYSDB_POSIX attribute should be named differently to indicate it is a bool, "isPosix" perhaps. "posixGroup" is commonly used as objectlass.
Good thought. I'll change it.
Jan
On Thu, 2011-05-19 at 13:53 +0200, Jan Zelený wrote:
On 05/14/2011 01:29 AM, Jan Zeleny wrote:
Hi, I'm sending the first version of the patch solving issues with nested groups without GID. The patch is definitely not final, let's say it's the first draft of final patch. It depends on Jakub's recent series of rfc2307bis patches and it won't be final until they are pushed.
At this moment I'd like to hear your opinion on the concept of the patch.
Thanks in advance Jan
Instead of modifying the global search filter to not include GID and using the generic sdap_nested_group_lookup_group(), why not create a separate function with a special non-posix search filter as a fallback if neither user nor posix-group filters match?
Frankly I don't like the idea of creating new functions with almost the same code. The file has 5.5k lines as it is and I don't want to add more just to separate couple lines of code.
I agree to this sentiment, that said I don't get why you change the filter at all based on whteher gid is 0 or not. The filter is an OR filter it will always match regardless you can post filter stuf just as easily checking whether the returned entry has the gidNumber attribute or not ?
Or am I missing something ?
Simo.
On Thu, 2011-05-19 at 16:21 +0200, Jakub Hrozek wrote:
On 05/19/2011 04:07 PM, Simo Sorce wrote:
The filter is an OR filter it will always match
Actually, it's an AND filter, something along the lines of:
(&(cn=name)(objectclass=posixGroup)(cn=*)(gidNumber=*))
I guess I was looking at the wrong part of the patch, this is what I was referring to:
+ if (gid) { + /* Search for users that are members of this group, or + * that have this group as their primary GID + */ + subfilter = talloc_asprintf(tmpctx, "(|(%s=%s)(%s=%lu))", + SYSDB_MEMBEROF, dn, + SYSDB_GIDNUM, (long unsigned) gid); + } else { + subfilter = talloc_asprintf(tmpctx, "(%s=%s)", + SYSDB_MEMBEROF, dn); }
I don't really see the need for the if/else here.
The other change to the filter that removes the gid part looks ok to me. Both GIDs and Names are unique for groups so filtering for both or for one of them should yield the same results and perhaps be even slightly faster as one less index is pulled up during the search.
Simo.
Simo Sorce simo@redhat.com wrote:
On Thu, 2011-05-19 at 16:21 +0200, Jakub Hrozek wrote:
On 05/19/2011 04:07 PM, Simo Sorce wrote:
The filter is an OR filter it will always match
Actually, it's an AND filter, something along the lines of:
(&(cn=name)(objectclass=posixGroup)(cn=*)(gidNumber=*))
I guess I was looking at the wrong part of the patch, this is what I was referring to:
if (gid) {/* Search for users that are members of this group, or* that have this group as their primary GID*/subfilter = talloc_asprintf(tmpctx, "(|(%s=%s)(%s=%lu))",SYSDB_MEMBEROF, dn,SYSDB_GIDNUM, (long unsigned)gid);
} else {subfilter = talloc_asprintf(tmpctx, "(%s=%s)",SYSDB_MEMBEROF, dn); }I don't really see the need for the if/else here.
You are right, this way it makes no sense. I remember exactly the idea behind this, but this is poor implementation on my side. Thanks for catching it.
The other change to the filter that removes the gid part looks ok to me. Both GIDs and Names are unique for groups so filtering for both or for one of them should yield the same results and perhaps be even slightly faster as one less index is pulled up during the search.
Simo.
Jan
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Simo Sorce