Add some extra tests around the sudo checks.

Add some extra tests around the sudo checks.

Encapsulate the sudo checks, and make a function to
get the uid and gid for the user.

https://code.launchpad.net/~thumper/juju-core/local-sudo-caller/+merge/174904

(do not edit description out of merge proposal)

[thumper, 2013-07-16 03:19:53]

Please take a look.

[wallyworld, 2013-07-16 03:56:35]

LGTM

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:16: var rootCheckFunction = func() bool {
Perhaps checkRoot() is better?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:86: // change ownership of the directories.
I don't think the above comment belongs here

[jameinel, 2013-07-16 06:55:50]

LGTM

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:16: var rootCheckFunction = func() bool {
On 2013/07/16 03:56:35, wallyworld wrote:
> Perhaps checkRoot() is better?


checkIfRoot ?

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go
File environs/local/config_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go#...
environs/local/config_test.go:87: defer local.SetRootCheckFunction(rootCheck)
I would probably label this origRootCheck or something to that effect, so it is
immediately clear that this was the value before setting it without having to
dig up the return value of SetRootCheckFunction.

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go
File environs/local/export_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go#...
environs/local/export_test.go:17: }
This is fine as is, though for testing stuff I do still prefer the style of

return func() {
 rootCheckFunction = old
}

But if you ever want to inspect what the real root check does, I suppose this is
fine, too.

[rog, 2013-07-16 13:30:01]

LGTM with a few minor suggestions and general support of john's remarks, thought
i don't mind much.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:16: var rootCheckFunction = func() bool {
On 2013/07/16 06:55:50, jameinel wrote:
> On 2013/07/16 03:56:35, wallyworld wrote:
> > Perhaps checkRoot() is better?
> 
> 
> checkIfRoot ?

isRoot? (or even "amRoot")

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:81: // if the values have been set.  If the values
haven't been set, then
this reads slightly ambiguously to me.

perhaps:

// getSudoCallerIds returns the user id and group id of the SUDO caller.
// If either is unset, it returns zero for both values.
// An error is returned if the relevant environment variables
// are not valid integers.
?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:84: func getSudoCallerIds() (uid, gid int, err error) {
sudoCallerIds ?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:92: logger.Errorf("expected %q for SUDO_UID to be an
int: %v", uidStr, err)
do we need to log this error here? wouldn't
it be better to return a better error which
will be surely be logged later?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:98: uid = 0 // clear out any value we may have
i think that rather than messing around assigning to the return values,
i'd do:

if uidStr == "" || gidStr == "" {
    return 0, 0, nil
}
uid, err := strconv.Atoi(...)
if err != nil {
   return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", gidStr)
}
gid, err := stdconv.Atoi(...)
if err != nil {
   return 0, 0, fmt.Errorf("similar...")
}
return uid, gid, nil

this keeps things a bit more obvious, with better error messages
to boot (the error from Atoi is rarely useful)

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:126: if info.IsDir() && err == nil {
if err == nil && info.IsDir() {

(if there's an error, the info may be nil)

although in fact, i think:

if info != nil && info.IsDir() {

is probably better still.
not that any of this is likely to happen running as root.

[mue, 2013-07-16 14:14:35]

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:17: return os.Getuid() == 0
Any need why this is a variable?

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:98: uid = 0 // clear out any value we may have
On 2013/07/16 13:30:01, rog wrote:
> i think that rather than messing around assigning to the return values,
> i'd do:
> 
> if uidStr == "" || gidStr == "" {
>     return 0, 0, nil
> }
> uid, err := strconv.Atoi(...)
> if err != nil {
>    return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", gidStr)
> }
> gid, err := stdconv.Atoi(...)
> if err != nil {
>    return 0, 0, fmt.Errorf("similar...")
> }
> return uid, gid, nil
> 
> this keeps things a bit more obvious, with better error messages
> to boot (the error from Atoi is rarely useful)

+1

[thumper, 2013-07-16 21:55:04]

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:16: var rootCheckFunction = func() bool {
On 2013/07/16 13:30:01, rog wrote:
> On 2013/07/16 06:55:50, jameinel wrote:
> > On 2013/07/16 03:56:35, wallyworld wrote:
> > > Perhaps checkRoot() is better?
> > 
> > 
> > checkIfRoot ?
> 
> isRoot? (or even "amRoot")
> 

Went with checkIfRoot

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:17: return os.Getuid() == 0
On 2013/07/16 14:14:35, mue wrote:
> Any need why this is a variable?

Yes, we need to override it for tests.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:81: // if the values have been set.  If the values
haven't been set, then
On 2013/07/16 13:30:01, rog wrote:
> this reads slightly ambiguously to me.
> 
> perhaps:
> 
> // getSudoCallerIds returns the user id and group id of the SUDO caller.
> // If either is unset, it returns zero for both values.
> // An error is returned if the relevant environment variables
> // are not valid integers.
> ?

Shamelessly copied.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:84: func getSudoCallerIds() (uid, gid int, err error) {
On 2013/07/16 13:30:01, rog wrote:
> sudoCallerIds ?

You really don't like "get" do you?

Renamed.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:86: // change ownership of the directories.
On 2013/07/16 03:56:35, wallyworld wrote:
> I don't think the above comment belongs here

No it isn't.  Deleted.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:92: logger.Errorf("expected %q for SUDO_UID to be an
int: %v", uidStr, err)
On 2013/07/16 13:30:01, rog wrote:
> do we need to log this error here? wouldn't
> it be better to return a better error which
> will be surely be logged later?

Cleaned up like below.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:98: uid = 0 // clear out any value we may have
On 2013/07/16 14:14:35, mue wrote:
> On 2013/07/16 13:30:01, rog wrote:
> > i think that rather than messing around assigning to the return values,
> > i'd do:
> > 
> > if uidStr == "" || gidStr == "" {
> >     return 0, 0, nil
> > }
> > uid, err := strconv.Atoi(...)
> > if err != nil {
> >    return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", gidStr)
> > }
> > gid, err := stdconv.Atoi(...)
> > if err != nil {
> >    return 0, 0, fmt.Errorf("similar...")
> > }
> > return uid, gid, nil
> > 
> > this keeps things a bit more obvious, with better error messages
> > to boot (the error from Atoi is rarely useful)
> 
> +1

Much nicer.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newco...
environs/local/config.go:126: if info.IsDir() && err == nil {
On 2013/07/16 13:30:01, rog wrote:
> if err == nil && info.IsDir() {
> 
> (if there's an error, the info may be nil)
> 
> although in fact, i think:
> 
> if info != nil && info.IsDir() {
> 
> is probably better still.
> not that any of this is likely to happen running as root.

Done.

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go
File environs/local/config_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go#...
environs/local/config_test.go:87: defer local.SetRootCheckFunction(rootCheck)
On 2013/07/16 06:55:50, jameinel wrote:
> I would probably label this origRootCheck or something to that effect, so it
is
> immediately clear that this was the value before setting it without having to
> dig up the return value of SetRootCheckFunction.

Done.

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go
File environs/local/export_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go#...
environs/local/export_test.go:17: }
On 2013/07/16 06:55:50, jameinel wrote:
> This is fine as is, though for testing stuff I do still prefer the style of
> 
> return func() {
>  rootCheckFunction = old
> }
> 
> But if you ever want to inspect what the real root check does, I suppose this
is
> fine, too.

Still getting used to passing around lambdas.  I think that returning the
restore function is a good idea.