code review 4589049: os/user: group lookup functions

os/user: group lookup functions

Adding group lookup functions analogous to the user lookup ones to the
os/user package.

[andre, 2011-06-12 06:13:52]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-06-13 16:46:37]

http://codereview.appspot.com/4589049/diff/4001/src/pkg/os/user/user.go
File src/pkg/os/user/user.go (right):

http://codereview.appspot.com/4589049/diff/4001/src/pkg/os/user/user.go#newco...
src/pkg/os/user/user.go:24: Members   []string
I think Members should probably a method on *Group, rather than populated by
lookup.  The members list could be huge, and it should be possible to purely do
a name<->gid lookup, without the extra member lookup cost.

[andre, 2011-06-13 20:29:46]

On 2011/06/13 16:46:37, bradfitz wrote:
> I think Members should probably a method on *Group, rather than populated by
> lookup.  The members list could be huge, and it should be possible to purely
do
> a name<->gid lookup, without the extra member lookup cost.

Ok, the latest patch does that. I also did some refactoring to avoid code
duplication.

In the patch, the Members method returns a []string. Do you think it should
return []*User instead, ie, calling user.Lookup for each group member?

Thanks,
Andre

[rsc, 2011-06-14 15:38:54]

Shouldn't there be a 'is this user in this group' function?
99% of the time you don't care about the member list,
you just care about that question.

[andre, 2011-06-14 17:09:18]

On 2011/06/14 15:38:54, rsc wrote:
> Shouldn't there be a 'is this user in this group' function?
> 99% of the time you don't care about the member list,
> you just care about that question.

I can add something like this to the patch (untested):

func (u *User) IsInGroup(groupname string) (bool, os.Error) {
    g, err := user.LookupGroup(groupname)
    if err != nil {
        return false, err
    }
    if u.Gid == g.Gid {
        return true, nil
    }
    members, err = g.Members()
    if err != nil {
        return false, err
    }
    for _, member := range members {
        if u.Username == member {
            return true, nil
        }
    }
    return false, nil
}

What do you think?

Thanks,
Andre

[rsc, 2011-06-14 17:14:46]

On Tue, Jun 14, 2011 at 13:09,  <andrenth@gmail.com> wrote:
> On 2011/06/14 15:38:54, rsc wrote:
>>
>> Shouldn't there be a 'is this user in this group' function?
>> 99% of the time you don't care about the member list,
>> you just care about that question.
>
> I can add something like this to the patch (untested):
>
> func (u *User) IsInGroup(groupname string) (bool, os.Error) {
>    g, err := user.LookupGroup(groupname)
>    if err != nil {
>        return false, err
>    }
>    if u.Gid == g.Gid {
>        return true, nil
>    }
>    members, err = g.Members()
>    if err != nil {
>        return false, err
>    }
>    for _, member := range members {
>        if u.Username == member {
>            return true, nil
>        }
>    }
>    return false, nil
> }
>
> What do you think?

func (u *User) In(g *Group) (bool, os.Error)

seems better.  But more importantly, the implementation
should not just range over the member list.
It should avoid retrieving the member list if at all possible.

[andre, 2011-06-14 17:32:21]

On 2011/06/14 17:14:46, rsc wrote:
> func (u *User) In(g *Group) (bool, os.Error)
> 
> seems better.  But more importantly, the implementation
> should not just range over the member list.
> It should avoid retrieving the member list if at all possible.

Well, I'm doing it this way because the list of members comes from struct
group's gr_mem member. I'm not aware of a C library function that allows a
direct query without retrieving the list.

[rsc, 2011-06-14 17:34:07]

On Tue, Jun 14, 2011 at 13:32,  <andrenth@gmail.com> wrote:
> On 2011/06/14 17:14:46, rsc wrote:
>>
>> func (u *User) In(g *Group) (bool, os.Error)
>
>> seems better.  But more importantly, the implementation
>> should not just range over the member list.
>> It should avoid retrieving the member list if at all possible.
>
> Well, I'm doing it this way because the list of members comes from
> struct group's gr_mem member. I'm not aware of a C library function that
> allows a direct query without retrieving the list.

That's fine as long as it is in the per-os file, so that other systems
with better interfaces can do better.

[bradfitz, 2011-06-14 18:02:08]

On Tue, Jun 14, 2011 at 10:32 AM, <andrenth@gmail.com> wrote:

> On 2011/06/14 17:14:46, rsc wrote:
>
>> func (u *User) In(g *Group) (bool, os.Error)
>>
>
>  seems better.  But more importantly, the implementation
>> should not just range over the member list.
>> It should avoid retrieving the member list if at all possible.
>>
>
> Well, I'm doing it this way because the list of members comes from
> struct group's gr_mem member. I'm not aware of a C library function that
> allows a direct query without retrieving the list.
>

getgroups?
getgrouplist?

[andre, 2011-06-14 18:10:30]

On 2011/06/14 18:02:08, bradfitz wrote:
> getgroups?

This returns a list of groups for the calling process, not for a user.

> getgrouplist?

This returns a list of groups, so I don't think there's an advantage over just
using the gr_mem field.

[andre, 2011-06-14 18:11:18]

On 2011/06/14 17:34:07, rsc wrote:
> That's fine as long as it is in the per-os file, so that other systems
> with better interfaces can do better.

Ok, new patch set uploaded.

Thanks,
Andre

[rsc, 2011-06-14 18:12:34]

>> getgrouplist?
>
> This returns a list of groups, so I don't think there's an advantage
> over just using the gr_mem field.

There is if the number of groups the user is in
is significantly less than the number of users
in the group.

Typically group membership lists get much
bigger than user membership lists.  Using getgrouplist
makes sense to me.

[andre, 2011-06-14 20:35:55]

On 2011/06/14 18:12:34, rsc wrote:
> There is if the number of groups the user is in
> is significantly less than the number of users
> in the group.
> 
> Typically group membership lists get much
> bigger than user membership lists.  Using getgrouplist
> makes sense to me.

I have the patch using getgrouplist() ready, but unless I misunderstood how
getgrouplist() works, I'm still not sure it's the best solution.

The gr_mem solution has to scan the user database looking for the user, then
scan the member list to build gr_mem, and then in Go we scan it again to check
for membership. So the time it takes to run depends on the size of the group
database and the number of members of the given group.

The getgrouplist() function has to scan the member list of every group to search
for the ones which have the user as a member and build the group list, and then
in Go we scan it to check if the requested group is in the list. Therefore the
time it takes to run depends on the size of the group database and the number of
members of *every* group, and those numbers, as you mention, can be big.

If the getgrouplist() is still preferred, though, I'll upload the patch right
away.

Thanks,
Andre

[rsc, 2011-06-14 20:42:12]

> The getgrouplist() function has to scan the member list of every group
> to search for the ones which have the user as a member and build the
> group list, and then in Go we scan it to check if the requested group is
> in the list. Therefore the time it takes to run depends on the size of
> the group database and the number of members of *every* group, and those
> numbers, as you mention, can be big.

You have described one possible implementation.
Another possible implementation is to keep both
kinds of lists (user->[group], group->[user]) in the
database, keep them in sync, and use only the
one that is needed to satisfy a particular request.

I believe the grp.h routines use a processed, cached
copy of /etc/group when they use that file, so no
problem there.  Also, if they are querying remote
user databases, getgrouplist has the potential to
be much faster.

Put another way, getgrouplist is what login uses:
it's going to have a good implementation.

Russ

[andre, 2011-06-14 20:54:16]

New patch set uploaded.

[andre, 2011-06-16 12:53:08]

By the way, the User type defines Uid and Gid as ints (I did the same for Group
in the patch sets). Wouldn't it be more correct to redefined them as uints?

Best regards,
Andre

[bradfitz, 2011-06-22 18:33:08]

Sorry for the slow review.

http://codereview.appspot.com/4589049/diff/11006/src/pkg/os/user/lookup_unix.go
File src/pkg/os/user/lookup_unix.go (right):

http://codereview.appspot.com/4589049/diff/11006/src/pkg/os/user/lookup_unix....
src/pkg/os/user/lookup_unix.go:70: nameC := C.CString(u.Username)
think you're missing a free here.  see below:

defer C.free(unsafe.Pointer(nameC))

http://codereview.appspot.com/4589049/diff/11006/src/pkg/os/user/user.go
File src/pkg/os/user/user.go (right):

http://codereview.appspot.com/4589049/diff/11006/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:21: type Group struct {
all exported types and variables need a comment

http://codereview.appspot.com/4589049/diff/11006/src/pkg/os/user/user.go#newc...
src/pkg/os/user/user.go:23: Groupname string
Not sure about this.  It's consistent with User, but "username" is logically a
single word (in common unix/C speak), but I don't think "groupname" is a thing,
so "Groupname" looks funny.

Just Name perhaps?  Any future "full name" could just be called "Description" or
something if that got added in the future.

[andre, 2011-06-22 20:46:09]

Hi

On 2011/06/22 18:33:08, bradfitz wrote:
> think you're missing a free here

> all exported types and variables need a comment

> "Groupname" looks funny.

The lastest patch set fixes those. I also prefer "Name" instead of Groupname but
initially I thought it would be better to be consistent with the User type. Just
"Name" reads much better though.

Regards,
Andre

[russross, 2011-07-08 17:31:29]

The Linux implementation here requires two calls to getgrgid_r to retrieve a
list of group members (once to get the group, another to satisfy the Members
call).  This seems a little silly since getgrgid_r returns the full list of
members both times.  I understand Russ Cox's point (Hi Russ) about large groups,
especially in environments with large LDAP setups.  Still, it would be nice to
avoid repeating the call when it isn't necessary.

What about an unexported field in Group to hold the member list when it happens
to be available?  Then Members would just return it.  This would permit a
flexible interface but also keep things simple on simple systems.

I don't know what getgrnam_r does when the list is too big to fit in a 1024-byte
buffer (return ERANGE?), but it could fall back to an explicit call to satisfy
Members in that instance.  Semantically, a cached list would be a little
different than the current code: if the  group list changes between the two
calls, the current solution could retrieve the updated list while the proposed
change would get a stale copy.  This doesn't seem like a big concern to me,
especially if possible races are documented from the beginning.

[andre, 2011-07-09 03:23:47]

On Fri, Jul 8, 2011 at 2:31 PM,  <russross@gmail.com> wrote:
> The Linux implementation here requires two calls to getgrgid_r to
> retrieve a list of group members (once to get the group, another to
> satisfy the Members call).  This seems a little silly since getgrgid_r
> returns the full list of members both times.  I understand Russ Cox's
> point (Hi Russ) about large groups, especially in environments with
> large LDAP setups.  Still, it would be nice to avoid repeating the call
> when it isn't necessary.
>
> What about an unexported field in Group to hold the member list when it
> happens to be available?  Then Members would just return it.  This would
> permit a flexible interface but also keep things simple on simple
> systems.
>
> I don't know what getgrnam_r does when the list is too big to fit in a
> 1024-byte buffer (return ERANGE?), but it could fall back to an explicit
> call to satisfy Members in that instance.  Semantically, a cached list
> would be a little different than the current code: if the  group list
> changes between the two calls, the current solution could retrieve the
> updated list while the proposed change would get a stale copy.  This
> doesn't seem like a big concern to me, especially if possible races are
> documented from the beginning.

Yes, I guess ERANGE would be returned
(http://pubs.opengroup.org/onlinepubs/9699919799/functions/getgrgid.html),
at which point we could retry with a larger buffer.

I guess the options are either to avoid getting the member list twice
or to avoid keeping it in memory when we might not even use it. If
it's decided to always keep it in memory, I believe it would be
clearer if Members was a field in the Group struct and not a method.
Accessing "g.Members" would make it clear that we're getting the list
of members from when g was initialized, while "g.Members()" feels like
saying "give me the list of members of g at this moment", and the
possibility of getting an outdated list seems confusing.

[akumar, 2013-02-28 22:01:51]

Hi, what's the status of this? Was this dropped for some reason? Are there other
attempts?

Thanks

[bradfitz, 2013-03-01 00:34:33]

I think it was simply forgotten.

Alex, any comments on its interface?  Is it too simplistic to represent
Windows groups?

On Thu, Feb 28, 2013 at 2:01 PM, <seed@mail.nanosouffle.net> wrote:

> Hi, what's the status of this? Was this dropped for some reason? Are
> there other attempts?
>
> Thanks
>
>
>
https://codereview.appspot.**com/4589049/<https://codereview.appspot.com/4589...
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[brainman, 2013-03-01 00:54:33]

On 2013/03/01 00:34:33, bradfitz wrote:
> 
> Alex, any comments on its interface?  Is it too simplistic to represent
> Windows groups?
> 

I don't know how useful it is. But what you have so far, is, probably, doable on
windows. Maybe no easy, but I cannot tell until I tried.

Alex

https://codereview.appspot.com/4589049/diff/24001/src/pkg/os/user/lookup_unix.go
File src/pkg/os/user/lookup_unix.go (right):

https://codereview.appspot.com/4589049/diff/24001/src/pkg/os/user/lookup_unix...
src/pkg/os/user/lookup_unix.go:108: func LookupGroupId(gid int) (*Group,
os.Error) {
s/int/string/

https://codereview.appspot.com/4589049/diff/24001/src/pkg/os/user/user.go
File src/pkg/os/user/user.go (right):

https://codereview.appspot.com/4589049/diff/24001/src/pkg/os/user/user.go#new...
src/pkg/os/user/user.go:23: Gid  int    // group id
Make it string, like User.Gid.

https://codereview.appspot.com/4589049/diff/24001/src/pkg/os/user/user_test.go
File src/pkg/os/user/user_test.go (right):

https://codereview.appspot.com/4589049/diff/24001/src/pkg/os/user/user_test.g...
src/pkg/os/user/user_test.go:69: gid := syscall.Getgid()
This is not "cross-platform".

[r, 2013-08-29 01:09:26]

Is this going anywhere?

[andre, 2013-08-29 11:25:41]

On 2013/08/29 01:09:26, r wrote:
> Is this going anywhere?

If you guys need it, I can redo the patches against the current Go sources.

[r, 2013-08-29 13:06:59]

If you'd like to do that, please do.

-rob

[andre, 2013-08-31 23:40:24]

On 2013/08/29 13:06:59, r wrote:
> If you'd like to do that, please do.

Rob, couldn't find a way clpatch this as it would always abort. Maybe I did
something wrong, as I don't really remember the workflow to update an issue very
well (been a long time!).

Anyway, I ended up creating a new issue for this:
https://codereview.appspot.com/13454043

Hope it's fine that way.

Cheers,
Andre

[gobot, 2013-12-20 16:26:15]

Replacing golang-dev with golang-codereviews.

[rsc, 2014-03-05 20:06:37]

R=close