code review 6545054: os/user : use username as fullname if all else fails (o...

os/user : use username as fullname if all else fails (on windows)

Fixes issue 4113.

[shiv, 2012-09-21 15:07:33]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[brainman, 2012-09-23 04:12:34]

http://codereview.appspot.com/6545054/diff/5001/src/pkg/os/user/lookup_window...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/6545054/diff/5001/src/pkg/os/user/lookup_window...
src/pkg/os/user/lookup_windows.go:13: return username, nil
Are you sure, you want to return here?

[shiv, 2012-09-23 06:57:16]

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

Please take another look.

[shiv, 2012-09-23 07:00:33]

On 2012/09/23 04:12:34, brainman wrote:
>
http://codereview.appspot.com/6545054/diff/5001/src/pkg/os/user/lookup_window...
> src/pkg/os/user/lookup_windows.go:13: return username, nil
> Are you sure, you want to return here?

My bad. Updated patch is made available for review.

[brainman, 2012-09-24 04:40:04]

I appreciate your effort, but I do not see how your changes fixes issue 4113. I
have applied your patch, and running test on computer that is part of a domain,
but is disconnected from network. I still get similar error message:

C:\>user.test.exe -test.v -test.run=Current
=== RUN TestCurrent
--- FAIL: TestCurrent (0.00 seconds)
user_test.go:34:        Current: The specified domain either does not exist or
could not be contacted.
FAIL

Isn't your change suppose to handle these situations? I am confused.

Alex

[shiv, 2012-09-24 07:48:52]

On 2012/09/24 04:40:04, brainman wrote:
<snip>
> have applied your patch, and running test on computer that is part of a
domain,
> but is disconnected from network. I still get similar error message:
> 
<snip>

Alex, I suspect you found a discrepancy across Windows versions.
I tested the fix on Windows 7 where the bug was originally found.
What is your windows version?

As noted in the patch & bug-report, for the name lookup, I have used the API
call
syscall.GetUserNameEx(syscall.NameDisplay, &b[0], &n)

As per MS documentation, only syscall.NameSamCompatible is supported.

But in reality on my Win7 system, I can confirm that syscall.NameSamCompatible
does work (I have sent a clarification mail to MS doc team via the website, do
not know when the feedback will come)

syscall.NameDisplay => fetches the "full name"
syscall.NameSamCompatible => fetches only "domainname\username"

Using syscall.NameSamCompatible is not useful since that is not what is needed.

I found below URL (see comments section) that mentions about this discrepancy
http://sjohannes.wordpress.com/2010/06/19/win32-python-getting-users-display-...

Can you please confirm this is the case?

[brainman, 2012-09-24 10:57:19]

On 2012/09/24 07:48:52, gns wrote:
> ...
> What is your windows version?
> 

Windows 2000. But, I suspect, Windows XP will be the same. Would you like me to
try it?

Also testing on "Windows 7 Home Premium", as per your suggestion. Standalone
computer - not part of any domain. You proposed change fails:

C:\>\\rex\go\src\pkg\os\user\user.test.exe -test.v
=== RUN TestCurrent
--- FAIL: TestCurrent (0.02 seconds)
user_test.go:34:        Current: No mapping between account names and security
IDs was done.
=== RUN TestLookup
--- FAIL: TestLookup (0.00 seconds)
user_test.go:74:        Current: No mapping between account names and security
IDs was done.
=== RUN TestLookupId
--- FAIL: TestLookupId (0.00 seconds)
user_test.go:90:        Current: No mapping between account names and security
IDs was done.
FAIL

while our existing code PASSes. I do not think we should go ahead with your
propsal.

Alex

[brainman, 2012-09-25 01:41:17]

@gns

Thinking again about your problem. Perhaps, you could leave all existing code
alone and just add call to syscall.GetUserNameEx at the end of lookupFullName,
when everything else (syscall.TranslateAccountName and syscall.NetUserGetInfo)
fails.

It is an extra code, but, I guess, if it fixes issue 4113, then we should do it.

Another suggestion would be to use "username" instead of failing. That is,
probably, last resort solution.

Alex

[shiv, 2012-09-26 13:56:29]

Returning GetUserNameEx() when everything else fails may not be good enough
improvement on the current situation.

issue 4113 affects 2 kind of users:
[1] Users who are mobile - computer is connected-to/disconnected-from domain as
they move about
[2] Servers having AD authentication and running go service - if these have
intermittent network errors


For [1] returning GetUserNameEx() "finally" is ok.
[2] which is lot more critical, if likely, then Go will not be a production
quality base in such cases.


I was considering having a init function at file level which initializes itself
with details about the computer (windows version) so that when lookup is done,
appropriate execution path unique to that windows version can be done.
I haven't checked if this is feasible.

Do you see this workable?

[shiv, 2012-09-26 13:59:54]

On 2012/09/26 13:56:29, gns wrote:
> 
> [2] Servers having AD authentication and running go service - if these have
> intermittent network errors
> 

FYI: The timeout for such users is 20-30 seconds.

[brainman, 2012-09-27 04:08:00]

On 2012/09/26 13:56:29, gns wrote:
> ...
> issue 4113 affects 2 kind of users:
> [1] Users who are mobile - computer is connected-to/disconnected-from domain
as
> they move about
> [2] Servers having AD authentication and running go service - if these have
> intermittent network errors
> 
> 
> For [1] returning GetUserNameEx() "finally" is ok.

Good. That is what your original problem was about. So that covers it. Right?

> [2] which is lot more critical, ...

I am not concerned about that scenario at this stage. Not many people run Go
executables as windows service yet. And, I believe, most of the time, windows
services should be running in "Local System" account.

> I was considering having a init function at file level which initializes
itself
> with details about the computer (windows version) so that when lookup is done,
> appropriate execution path unique to that windows version can be done.
> I haven't checked if this is feasible.

How is knowledge of "windows version" going to help you here? What is wrong with
trying all our APIs in turn and stopping when it works?

> FYI: The timeout for such users is 20-30 seconds.

That could be a problem. But I do not see how we can avoid it. It is what it is.

Lastly, I do not think many people use this package. So, I do not want to put
too much effort into it. Also, I would like to keep complexity to the minimum
required.

Alex

[shiv, 2012-09-29 03:19:20]

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

Please take another look.

[brainman, 2012-09-29 12:30:44]

CL description is too long. Please, just say what it does. For example:

>>>
os/user: use username as fullname if all else fails (on windows)

Fixes issue 4113.
<<<

or something similar.

Alex

http://codereview.appspot.com/6545054/diff/18001/src/pkg/os/user/lookup_windo...
File src/pkg/os/user/lookup_windows.go (right):

http://codereview.appspot.com/6545054/diff/18001/src/pkg/os/user/lookup_windo...
src/pkg/os/user/lookup_windows.go:32: // username is returned as a least
destructive action
Lets not claim that we know what is happening here. Just say

// give up looking for the fullname.
// pretend that username is fullname.

or something to that effect.

[shiv, 2012-09-29 12:47:08]

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

Please take another look.

[brainman, 2012-09-29 12:52:14]

LGTM

I will leave for others to comment. For a day or so.

Thank you.

Alex

[brainman, 2012-10-03 00:33:32]

*** Submitted as http://code.google.com/p/go/source/detail?r=c763565f31ff ***

os/user : use username as fullname if all else fails (on windows)

Fixes issue 4113.

R=golang-dev, alex.brainman
CC=golang-dev
http://codereview.appspot.com/6545054

Committer: Alex Brainman <alex.brainman@gmail.com>