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.