Preliminary support for basic juju id commands:

Preliminary support for basic juju id commands:

juju add-user <username> <password>

if password is not supplied uses gopass to take the password from the command line

juju remove-user <username>

doesn't actually remove the user. But mark them as inactive

add-user and remove-user aren't registered yet as they are useless without
login. But the details of this are still being worked out.

This branch also contains fixes for lp:1285256. The EnvCommandBase includes a
function for ensuring the envname is not "".

https://code.launchpad.net/~mattyw/juju-core/user-add-remove-cli/+merge/205715

(do not edit description out of merge proposal)

[mattyw, 2014-02-20 15:00:00]

Please take a look.

[dimitern, 2014-02-25 11:56:07]

I'm not quite sure changing NewAPIFrom name and replacing configstore.Store with
EnvironInfo is a good idea. Some comments below, but I'd advise you to seek a
review from Roger as well.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newcode1
cmd/juju/adduser.go:1: // Copyright 2012, 2013 Canonical Ltd.
s/2012, 2013/2014/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:13: "code.google.com/p/gopass"
I don't think we need to add this dependency. Can't you just read it? Or it's
the echo is disabled (like passwd does)?

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:16: type AdduserCommand struct {
s/AdduserCommand/AddUserCommand/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:18: Tag      string
s/Tag/User/ - all CLI commands use names rather than tags. And you're actually
taking names as args.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:23: return &cmd.Info{
There should be a help text and perhaps a doc comment on the AddUserCommand
explaining in more details, with examples.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:25: Args:    "<username> <password>",
According to this, the user might assume both username and password are required
arguments, which I start to think it's actually a good idea, and a chance to
simplify the implementation and not require extra dependencies.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:32: return errors.New("no username supplied")
fmt.Errorf seems appropriate here, no need to import both fmt and errors just
for this.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser_test.go
File cmd/juju/adduser_test.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser_test.go#...
cmd/juju/adduser_test.go:1: // Copyright 2012, 2013 Canonical Ltd.
2014..

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser_test.go#...
cmd/juju/adduser_test.go:13: type AdduserSuite struct {
s/AdduserSuite/AddUserSuite/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go
File cmd/juju/login.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode1
cmd/juju/login.go:1: // Copyright 2012, 2013 Canonical Ltd.
2014

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode15
cmd/juju/login.go:15: "code.google.com/p/gopass"
Making the password required will allow you to drop this.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode20
cmd/juju/login.go:20: Tag      string
Again, s/Tag/User/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode28
cmd/juju/login.go:28: Purpose: "login as user",
As with AddUser: help text and doc string on LoginCommand

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode34
cmd/juju/login.go:34: return errors.New("no username supplied")
s/errors.New/fmt.Errorf/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode71
cmd/juju/login.go:71: return fmt.Errorf("Failed to login %v", err)
s/Failed to login/login failed: %v/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode77
cmd/juju/login.go:77: return fmt.Errorf("Failed to write login data %v", err)
s/Failed to write login data %v/cannot write API credentials: %v/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode79
cmd/juju/login.go:79: 
d

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login_test.go
File cmd/juju/login_test.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login_test.go#ne...
cmd/juju/login_test.go:1: // Copyright 2012, 2013 Canonical Ltd.
2014

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login_test.go#ne...
cmd/juju/login_test.go:25: _, err = testing.RunCommand(c, &AdduserCommand{},
[]string{"foobar", "password"})
Now this appears a few times and it's worth having:

func runLogin(user, pass string) error {
    _, err := testing.RunCommand(c, &LoginCommand{}, []string{user, pass})
    return err
}

in here and a similar one - runAddUser in adduser_test.go, so you can use them
both here and there.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser.go
File cmd/juju/removeuser.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser.go#ne...
cmd/juju/removeuser.go:1: // Copyright 2012, 2013 Canonical Ltd.
2014

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser.go#ne...
cmd/juju/removeuser.go:13: type RemoveuserCommand struct {
s/RemoveuserCommand/RemoveUserCommand/. Also doc comment please.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser.go#ne...
cmd/juju/removeuser.go:15: Tag string
s/Tag/User/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser.go#ne...
cmd/juju/removeuser.go:22: Purpose: "removes a user",
Add help text + examples please.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser_test.go
File cmd/juju/removeuser_test.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser_test....
cmd/juju/removeuser_test.go:1: // Copyright 2012, 2013 Canonical Ltd.
2014

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser_test....
cmd/juju/removeuser_test.go:13: type RemoveuserSuite struct {
s/RemoveuserSuite/RemoveUserSuite/

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/removeuser_test....
cmd/juju/removeuser_test.go:20: _, err := testing.RunCommand(c,
&AdduserCommand{}, []string{"foobar", "password"})
As with the others, add runRemoveUser helper and use it.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go
File cmd/juju/whoami.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode1
cmd/juju/whoami.go:1: // Copyright 2012, 2013 Canonical Ltd.
2014

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode11
cmd/juju/whoami.go:11: //"launchpad.net/juju-core/names"
d

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode14
cmd/juju/whoami.go:14: type WhoamiCommand struct {
doc comment

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode23
cmd/juju/whoami.go:23: Purpose: "prints the name of the currently logged in
user",
Help text, examples.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode29
cmd/juju/whoami.go:29: c.out.AddFlags(f, "yaml", map[string]cmd.Formatter{
s/"yaml"/"format"/ ? see how other similar commands define the format arg.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode49
cmd/juju/whoami.go:49: //_, id, err := names.ParseTag(user, names.UserTagKind)
Yes, you do need to parse the tag and output it as user name, as all CLI
commands work with names, not tags.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami_test.go
File cmd/juju/whoami_test.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami_test.go#n...
cmd/juju/whoami_test.go:1: // Copyright 2012, 2013 Canonical Ltd.
2014

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami_test.go#n...
cmd/juju/whoami_test.go:37: stdout := ctx.Stdout.(*bytes.Buffer).Bytes()
s/.Bytes()/.String()/, then you won't need to cast it to string later.

https://codereview.appspot.com/51450047/diff/180001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode60
juju/api.go:60: //TODO (mattyw) This function should be deleted
Why?

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode62
juju/api.go:62: store := configstore.NewMem()
This seems wrong - you're effectively removing the cached login and calling
environAPIInfo just for the endpoints. It can be simplified perhaps by calling
environAPIInfo(nil, environ) and checking/setting creds in environAPIInfo only
if environInfo != nil.

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode111
juju/api.go:111: func NewUserManagerClient(envName string) (*usermanager.Client,
error) {
doc comment since this is now exported.

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode120
juju/api.go:120: /*
Remove the commented code please.

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode184
juju/api.go:184: //store, err := configstore.Default()
delete this.

Also it seems you don't have to do this at all if info!=nil, right?

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode322
juju/api.go:322: if environInfo.APICredentials().User != "" {
You should check both User != "" and Password != "" here.

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode326
juju/api.go:326: 
d

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode341
juju/api.go:341: info.SetAPIEndpoint(configstore.APIEndpoint{
Add a check just before this:

if len(apiInfo.Addrs) > 0 && len(apiInfo.CACert) > 0 {
... set api endpoints
}

Then you can use this instead of the new cacheAPICreds you added.

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode359
juju/api.go:359: func cacheAPICreds(info configstore.EnvironInfo, apiInfo
*api.Info) error {
You don't need this, please remove it and see above.

[rog, 2014-02-26 17:14:29]

I haven't finished the review yet, I'm afraid, but publishing interim comments
anyway. Looks good in general so far.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go
File cmd/juju/adduser.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:13: "code.google.com/p/gopass"
On 2014/02/25 11:56:07, dimitern wrote:
> I don't think we need to add this dependency. Can't you just read it? Or it's
> the echo is disabled (like passwd does)?

I agree - at the least, we should use a decent implementation that doesn't shell
out to stty (since we already have go.crypto as a dependency, we could use
ReadPassword from code.google.com/p/go.crypto/ssh/terminal)

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser.go#newco...
cmd/juju/adduser.go:25: Args:    "<username> <password>",
On 2014/02/25 11:56:07, dimitern wrote:
> According to this, the user might assume both username and password are
required
> arguments, which I start to think it's actually a good idea, and a chance to
> simplify the implementation and not require extra dependencies.

That's definitely a possibility, although for the login command, we probably
want to read the password in the same way.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go
File cmd/juju/login.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode15
cmd/juju/login.go:15: "code.google.com/p/gopass"
On 2014/02/25 11:56:07, dimitern wrote:
> Making the password required will allow you to drop this.

I don't think we should require the password to be typed in full view when
logging in.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login_test.go
File cmd/juju/login_test.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login_test.go#ne...
cmd/juju/login_test.go:25: _, err = testing.RunCommand(c, &AdduserCommand{},
[]string{"foobar", "password"})
On 2014/02/25 11:56:07, dimitern wrote:
> Now this appears a few times and it's worth having:
> 
> func runLogin(user, pass string) error {
>     _, err := testing.RunCommand(c, &LoginCommand{}, []string{user, pass})
>     return err
> }
> 

I actually disagree here - they're both a single line only,
and and the existing version is clearer (you don't need
to go looking for runLogin to find out that it's running
the LoginCommand code).

If we had a helper function to log in without running the LoginCommand
code, it would be worth it, but it's actually good that
we're directly checking that juju login works after juju add-user.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/main.go#newcode121
cmd/juju/main.go:121: // Manage users commands.
s/ commands// ?

https://codereview.appspot.com/51450047/diff/180001/juju/api.go
File juju/api.go (right):

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode60
juju/api.go:60: //TODO (mattyw) This function should be deleted
On 2014/02/25 11:56:07, dimitern wrote:
> Why?

APIConn is a holdover from previous times - nothing uses it now.

I've actually suggested that this be done before this branch
merges.

https://codereview.appspot.com/51450047/diff/180001/juju/api.go#newcode111
juju/api.go:111: func NewUserManagerClient(envName string) (*usermanager.Client,
error) {
On 2014/02/25 11:56:07, dimitern wrote:
> doc comment since this is now exported.

Rather than create a new function in this package for
every client type, I'd prefer to follow the precedent of
the agent types, and have a method on api.State for
each.

e.g. (in state/api):

func (st *State) UserManager() *usermanager.Client

then have a single function here that returns a *api.State.
We can then delete NewKeyManagerClient too.

[fwereade, 2014-03-06 11:57:47]

A few comments, let me know your thoughts.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser_test.go
File cmd/juju/adduser_test.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser_test.go#...
cmd/juju/adduser_test.go:13: type AdduserSuite struct {
On 2014/02/25 11:56:07, dimitern wrote:
> s/AdduserSuite/AddUserSuite/

...and similar changes across the board please.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/adduser_test.go#...
cmd/juju/adduser_test.go:25: c.Assert(err, gc.ErrorMatches, "user already
exists")
sorry, I didn't pay proper attention to the initial CL -- would we accept an
empty password? and is there any facility for having juju autogenerate a strong
password?

And I'd also really like it if add-user could directly generate a .jenv that you
can just email directly to the new user. Sane?

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go
File cmd/juju/login.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode73
cmd/juju/login.go:73: defer conn.Close()
may as well just close the conn here, right? and check the error :)

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login.go#newcode80
cmd/juju/login.go:80: return nil
I'm a little bit uncertain about this... ISTM that it enables a (future) path by
which you can add a user with restricted perms; log in as that user; and whoops,
you've overwritten the only copy of your admin creds.

If we have add-user emitting .jenvs, though, then login could plausibly *accept*
a .jenv to be copied into the environments dir (and warn before overwriting an
existing one). Sensible?

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login_test.go
File cmd/juju/login_test.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/login_test.go#ne...
cmd/juju/login_test.go:25: _, err = testing.RunCommand(c, &AdduserCommand{},
[]string{"foobar", "password"})
On 2014/02/26 17:14:29, rog wrote:
> On 2014/02/25 11:56:07, dimitern wrote:
> > Now this appears a few times and it's worth having:
> > 
> > func runLogin(user, pass string) error {
> >     _, err := testing.RunCommand(c, &LoginCommand{}, []string{user, pass})
> >     return err
> > }
> > 
> 
> I actually disagree here - they're both a single line only,
> and and the existing version is clearer (you don't need
> to go looking for runLogin to find out that it's running
> the LoginCommand code).

LoginSuite.run(c *gc.C, args ...string) might work though. I have yet to be
convinced that it's a good idea to duplicate code just to avoid reading a
trivial function implementation -- once you've read it once, you *know* what it
does and you don't need to keep checking back, but if you dupe the code
everywhere you have to read it everywhere -- and it enables a whole class of
screwups where you *think* it does the same thing it does everywhere else, but
is actually subtly different.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go
File cmd/juju/whoami.go (right):

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode23
cmd/juju/whoami.go:23: Purpose: "prints the name of the currently logged in
user",
And, hmm, given what we're doing with the .jenv, I'm wondering if there isn't a
big overlap with `juju switch` here.

https://codereview.appspot.com/51450047/diff/180001/cmd/juju/whoami.go#newcode29
cmd/juju/whoami.go:29: c.out.AddFlags(f, "yaml", map[string]cmd.Formatter{
On 2014/02/25 11:56:07, dimitern wrote:
> s/"yaml"/"format"/ ? see how other similar commands define the format arg.

Further to this, I think it might be appropriate to have the "smart" formatter
by default; even if it ain't really all that smart, it's consistent with other
uses.