Started working on add/remove user support

Started working on add/remove user support

The internal api server and client code for user management. 

> Currently just supports add|remove user. 

> Removal just means setting an inactive flag on that user
which will mean you cannot login as that user.

> There are no roles currently. All users have the same full access.

> Future MPs will cover user roles and the command line interface.

https://code.launchpad.net/~mattyw/juju-core/add_users_first_pass/+merge/205593

(do not edit description out of merge proposal)

[mattyw, 2014-02-19 13:39:44]

Please take a look.

[dimitern, 2014-02-25 10:50:58]

Good direction, but it needs a bit more polishing I think.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:23: func (c *Client) AddUser(tag, password
string) ([]params.ErrorResult, error) {
Why do you need to return []params.ErrorResult when you can only have one error?
Just and error return should suffice (here and below).

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:25: result := new(params.ErrorResults)
I'd prefer (here and below):

var result params.ErrorResults

and then pass it as &result in st.Call below.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client_test.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/pack...
File state/api/usermanager/package_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/pack...
state/api/usermanager/package_test.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/package_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/package_test.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:9: "github.com/loggo/loggo"
Blank line after loggo import please.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:19: AddUser(arg params.ModifyUsers)
(params.ErrorResults, error)
So, unless you're thinking of making both of these calls bulk, like taking
[]params.ModifyUsers (perhaps then rename it to ModifyUser?), having
[]ErrorResult, and error seems unnecessary (just 1 error return to match the
single user arg is enough).

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:27: resources   *common.Resources
Unless you need to add watchers, you don't really need resources.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:47: getCanRead := func()
(common.AuthFunc, error) {
Since you don't have anything to read, I'd remove this for now, until we need it
(e.g. when ListUsers is added).

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:54: getCanWrite := func()
(common.AuthFunc, error) {
s/getCanWrite/getCanModify/ ?

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:63: state:       st,
please run this through gofmt - it seems the indentation here is inconsistent?

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:72: _, err :=
api.state.AddUser(args.Tag, args.Password)
You should use the getCanWrite function here, before calling state.AddUser, see
other facades for examples.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:84: user, err :=
api.state.User(args.Tag)
ditto

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:86: return params.ErrorResults{
See? This whole block just to return a single error is horrible :)

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:1: // Copyright 2013 Canonical
Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:7: gc "launchpad.net/gocheck"
Blank line after gocheck please.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:28: s.AddCleanup(func(_ *gc.C) {
s.resources.StopAll() })
Even if you needed resources, there's no need to call AddCleanup - the apiserver
takes care of that.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:41: func (s *userManagerSuite)
TestNewUserManagerAcceptsClient(c *gc.C) {
I don't think you need this test, since if it didn't work most of the others
will fail.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:57: Tag:      "foobar",
This is not a tag, it's a username. So if your intention was to pass names
instead of tags, please rename ModifyUsers.Tag to User. Also, since it's a
single user request, you can also s/ModifyUsers/ModifyUser/.

OTOH, the API should talk tags over the wire, so leaving the field "Tag", but
actually passing and expecting tags is better IMO.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:68: c.Assert(user, gc.NotNil)
Check the user's password here?

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:86: c.Assert(user.IsInactive(),
gc.Equals, true) //Removal makes the user inactive
Move the comment above the Assert call? s/in active/inactive/, leading space
after //

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:89: /* Since removing a user
just sets them inactive you cannot add a user
// instead of /* */ ?

https://codereview.appspot.com/61620043/diff/50001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/user.go#newcode170
state/user.go:170: Id:     u.Name(),
You should Assert: txn.DocExists at least here, and handle the txn.ErrAborted in
the if block below, check other state methods for examples.

[dimitern, 2014-02-25 10:50:58]

Good direction, but it needs a bit more polishing I think.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:23: func (c *Client) AddUser(tag, password
string) ([]params.ErrorResult, error) {
Why do you need to return []params.ErrorResult when you can only have one error?
Just and error return should suffice (here and below).

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:25: result := new(params.ErrorResults)
I'd prefer (here and below):

var result params.ErrorResults

and then pass it as &result in st.Call below.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client_test.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/pack...
File state/api/usermanager/package_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/pack...
state/api/usermanager/package_test.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/package_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/package_test.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:1: // Copyright 2013 Canonical Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:9: "github.com/loggo/loggo"
Blank line after loggo import please.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:19: AddUser(arg params.ModifyUsers)
(params.ErrorResults, error)
So, unless you're thinking of making both of these calls bulk, like taking
[]params.ModifyUsers (perhaps then rename it to ModifyUser?), having
[]ErrorResult, and error seems unnecessary (just 1 error return to match the
single user arg is enough).

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:27: resources   *common.Resources
Unless you need to add watchers, you don't really need resources.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:47: getCanRead := func()
(common.AuthFunc, error) {
Since you don't have anything to read, I'd remove this for now, until we need it
(e.g. when ListUsers is added).

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:54: getCanWrite := func()
(common.AuthFunc, error) {
s/getCanWrite/getCanModify/ ?

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:63: state:       st,
please run this through gofmt - it seems the indentation here is inconsistent?

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:72: _, err :=
api.state.AddUser(args.Tag, args.Password)
You should use the getCanWrite function here, before calling state.AddUser, see
other facades for examples.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:84: user, err :=
api.state.User(args.Tag)
ditto

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:86: return params.ErrorResults{
See? This whole block just to return a single error is horrible :)

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:1: // Copyright 2013 Canonical
Ltd.
s/2013/2014/

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:7: gc "launchpad.net/gocheck"
Blank line after gocheck please.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:28: s.AddCleanup(func(_ *gc.C) {
s.resources.StopAll() })
Even if you needed resources, there's no need to call AddCleanup - the apiserver
takes care of that.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:41: func (s *userManagerSuite)
TestNewUserManagerAcceptsClient(c *gc.C) {
I don't think you need this test, since if it didn't work most of the others
will fail.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:57: Tag:      "foobar",
This is not a tag, it's a username. So if your intention was to pass names
instead of tags, please rename ModifyUsers.Tag to User. Also, since it's a
single user request, you can also s/ModifyUsers/ModifyUser/.

OTOH, the API should talk tags over the wire, so leaving the field "Tag", but
actually passing and expecting tags is better IMO.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:68: c.Assert(user, gc.NotNil)
Check the user's password here?

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:86: c.Assert(user.IsInactive(),
gc.Equals, true) //Removal makes the user inactive
Move the comment above the Assert call? s/in active/inactive/, leading space
after //

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:89: /* Since removing a user
just sets them inactive you cannot add a user
// instead of /* */ ?

https://codereview.appspot.com/61620043/diff/50001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/user.go#newcode170
state/user.go:170: Id:     u.Name(),
You should Assert: txn.DocExists at least here, and handle the txn.ErrAborted in
the if block below, check other state methods for examples.

[fwereade, 2014-02-27 13:16:14]

dimitern's comments look sound, WIPping this for now

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/root.go#ne...
state/apiserver/root.go:110: // UserManager returns an object that provides
access to the UserManager API
"Manager" always makes me a bit sad, but IIRC there's precedent...

[mattyw, 2014-02-27 15:15:22]

Just pushing updates to this branch

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:1: // Copyright 2013 Canonical Ltd.
On 2014/02/25 10:50:58, dimitern wrote:
> s/2013/2014/

Done.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:23: func (c *Client) AddUser(tag, password
string) ([]params.ErrorResult, error) {
On 2014/02/25 10:50:58, dimitern wrote:
> Why do you need to return []params.ErrorResult when you can only have one
error?
> Just and error return should suffice (here and below).

Done.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client.go:25: result := new(params.ErrorResults)
On 2014/02/25 10:50:58, dimitern wrote:
> I'd prefer (here and below):
> 
> var result params.ErrorResults
> 
> and then pass it as &result in st.Call below.

Done.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/clie...
state/api/usermanager/client_test.go:1: // Copyright 2013 Canonical Ltd.
On 2014/02/25 10:50:58, dimitern wrote:
> s/2013/2014/

Done.

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/pack...
File state/api/usermanager/package_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/api/usermanager/pack...
state/api/usermanager/package_test.go:1: // Copyright 2013 Canonical Ltd.
On 2014/02/25 10:50:58, dimitern wrote:
> s/2013/2014/

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/root.go
File state/apiserver/root.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/root.go#ne...
state/apiserver/root.go:110: // UserManager returns an object that provides
access to the UserManager API
On 2014/02/27 13:16:14, fwereade wrote:
> "Manager" always makes me a bit sad, but IIRC there's precedent...

Totally happy to remove this comment - I was following precedent

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/package_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/package_test.go:1: // Copyright 2013 Canonical Ltd.
On 2014/02/25 10:50:58, dimitern wrote:
> s/2013/2014/

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:1: // Copyright 2013 Canonical Ltd.
On 2014/02/25 10:50:58, dimitern wrote:
> s/2013/2014/

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:9: "github.com/loggo/loggo"
On 2014/02/25 10:50:58, dimitern wrote:
> Blank line after loggo import please.

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:19: AddUser(arg params.ModifyUsers)
(params.ErrorResults, error)
On 2014/02/25 10:50:58, dimitern wrote:
> So, unless you're thinking of making both of these calls bulk, like taking
> []params.ModifyUsers (perhaps then rename it to ModifyUser?), having
> []ErrorResult, and error seems unnecessary (just 1 error return to match the
> single user arg is enough). 

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:27: resources   *common.Resources
On 2014/02/25 10:50:58, dimitern wrote:
> Unless you need to add watchers, you don't really need resources.

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:47: getCanRead := func()
(common.AuthFunc, error) {
On 2014/02/25 10:50:58, dimitern wrote:
> Since you don't have anything to read, I'd remove this for now, until we need
it
> (e.g. when ListUsers is added).

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:54: getCanWrite := func()
(common.AuthFunc, error) {
On 2014/02/25 10:50:58, dimitern wrote:
> s/getCanWrite/getCanModify/ ?

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:63: state:       st,
On 2014/02/25 10:50:58, dimitern wrote:
> please run this through gofmt - it seems the indentation here is inconsistent?

This is where gofmt puts it. Note the nil is not part of the &UserManagerAPI
structure

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:72: _, err :=
api.state.AddUser(args.Tag, args.Password)
On 2014/02/25 10:50:58, dimitern wrote:
> You should use the getCanWrite function here, before calling state.AddUser,
see
> other facades for examples.

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:84: user, err :=
api.state.User(args.Tag)
On 2014/02/25 10:50:58, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:86: return params.ErrorResults{
On 2014/02/25 10:50:58, dimitern wrote:
> See? This whole block just to return a single error is horrible :)

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager_test.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:1: // Copyright 2013 Canonical
Ltd.
On 2014/02/25 10:50:58, dimitern wrote:
> s/2013/2014/

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:7: gc "launchpad.net/gocheck"
On 2014/02/25 10:50:58, dimitern wrote:
> Blank line after gocheck please.

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:28: s.AddCleanup(func(_ *gc.C) {
s.resources.StopAll() })
On 2014/02/25 10:50:58, dimitern wrote:
> Even if you needed resources, there's no need to call AddCleanup - the
apiserver
> takes care of that.

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:41: func (s *userManagerSuite)
TestNewUserManagerAcceptsClient(c *gc.C) {
On 2014/02/25 10:50:58, dimitern wrote:
> I don't think you need this test, since if it didn't work most of the others
> will fail.

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:68: c.Assert(user, gc.NotNil)
On 2014/02/25 10:50:58, dimitern wrote:
> Check the user's password here?

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:86: c.Assert(user.IsInactive(),
gc.Equals, true) //Removal makes the user inactive
On 2014/02/25 10:50:58, dimitern wrote:
> Move the comment above the Assert call? s/in active/inactive/, leading space
> after //

Done.

https://codereview.appspot.com/61620043/diff/50001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:89: /* Since removing a user
just sets them inactive you cannot add a user
On 2014/02/25 10:50:58, dimitern wrote:
> // instead of /* */ ?

Done.

https://codereview.appspot.com/61620043/diff/50001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/50001/state/user.go#newcode170
state/user.go:170: Id:     u.Name(),
On 2014/02/25 10:50:58, dimitern wrote:
> You should Assert: txn.DocExists at least here, and handle the txn.ErrAborted
in
> the if block below, check other state methods for examples.

Done.

[mattyw, 2014-02-27 15:18:12]

Please take a look.

[fwereade, 2014-03-06 12:11:34]

several comments

https://codereview.appspot.com/61620043/diff/70001/state/api/usermanager/clie...
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/api/usermanager/clie...
state/api/usermanager/client_test.go:34: 
Can we get a SetPassword api as well please? I really want it to be
easy/possible to add-user to create a jenv; send it to someone; and have them
reset their password so the original creator can't impersonate them.

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:20: AddUser(arg params.ModifyUser)
(params.ErrorResult, error)
Bulk calls, please. We don't do this properly in most of the client API, but all
new APIs should do. See any of the internal APIs for details.

The sheer bulk of code is a bit sucky, but I would really like to move towards
consistent bulk calls.

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager_test.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:83: // that has been previously
been removed
Hmm. Should we maybe reactivate that user?

(honest question, not intended to be leading)

https://codereview.appspot.com/61620043/diff/70001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/user.go#newcode170
state/user.go:170: Id:     u.Name(),
I'm worried that this would let us delete the admin user, and render an
environment unusable. Please at least prevent "admin" from being made inactive.

https://codereview.appspot.com/61620043/diff/70001/state/user.go#newcode184
state/user.go:184: func (u *User) IsInactive() bool {
The API server should handle this, and not let inactive users log in. Did I just
miss the code that handles that?

[mattyw, 2014-03-07 02:01:21]

Please take a look.

https://codereview.appspot.com/61620043/diff/70001/state/api/usermanager/clie...
File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/api/usermanager/clie...
state/api/usermanager/client_test.go:34: 
On 2014/03/06 12:11:35, fwereade wrote:
> Can we get a SetPassword api as well please? I really want it to be
> easy/possible to add-user to create a jenv; send it to someone; and have them
> reset their password so the original creator can't impersonate them.
https://bugs.launchpad.net/juju-core/+bug/1288750

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:20: AddUser(arg params.ModifyUser)
(params.ErrorResult, error)
On 2014/03/06 12:11:35, fwereade wrote:
> Bulk calls, please. We don't do this properly in most of the client API, but
all
> new APIs should do. See any of the internal APIs for details.
> 
> The sheer bulk of code is a bit sucky, but I would really like to move towards
> consistent bulk calls.

Done.

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager_test.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:83: // that has been previously
been removed
On 2014/03/06 12:11:35, fwereade wrote:
> Hmm. Should we maybe reactivate that user?
> 
> (honest question, not intended to be leading)

https://bugs.launchpad.net/juju-core/+bug/1288745

https://codereview.appspot.com/61620043/diff/70001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:83: // that has been previously
been removed
On 2014/03/06 12:11:35, fwereade wrote:
> Hmm. Should we maybe reactivate that user?
> 
> (honest question, not intended to be leading)

https://bugs.launchpad.net/juju-core/+bug/1288745

https://codereview.appspot.com/61620043/diff/70001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/user.go#newcode170
state/user.go:170: Id:     u.Name(),
On 2014/03/06 12:11:35, fwereade wrote:
> I'm worried that this would let us delete the admin user, and render an
> environment unusable. Please at least prevent "admin" from being made
inactive.

Done.

https://codereview.appspot.com/61620043/diff/70001/state/user.go#newcode184
state/user.go:184: func (u *User) IsInactive() bool {
On 2014/03/06 12:11:35, fwereade wrote:
> The API server should handle this, and not let inactive users log in. Did I
just
> miss the code that handles that?

This should be dealt with in the next merge proposal - the one that introduces
the login command

[fwereade, 2014-03-07 10:00:40]

Nearly there, mostly just trivials. Ping me if anything's unclear, I expect the
last review here will be pretty much a formality (modulo my request that you
land the deactivation stuff all at once).

https://codereview.appspot.com/61620043/diff/70001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/user.go#newcode184
state/user.go:184: func (u *User) IsInactive() bool {
On 2014/03/07 02:01:22, mattyw wrote:
> On 2014/03/06 12:11:35, fwereade wrote:
> > The API server should handle this, and not let inactive users log in. Did I
> just
> > miss the code that handles that?
> 
> This should be dealt with in the next merge proposal - the one that introduces
> the login command
> 

That's not really ideal, because when this CL lands we'll have an API that
"deactivates" users but doesn't really work. But if you delay landing this CL to
arrive with the followup I'll be ok with it.

https://codereview.appspot.com/61620043/diff/90001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/api/params/params.go...
state/api/params/params.go:328: // ModifyUser stores the parameters used for a
UserManager.Add|Remove call
and, sorry, I don't think we should use this for Remove: the only the Tag field
is actually used. There's an Entities struct in params that fits perfectly,
though.

FWIW: I don't quite know what the deal is with verby param names: I think it's
just a matter of following existing "conventions" that we haven't really been
able to expunge. But params should generally be nouns, if possible, and
*probably* shouldn't reference the types on which they operate in the name;
doing so just makes it harder to sanely reuse them elsewhere.

https://codereview.appspot.com/61620043/diff/90001/state/api/params/params.go...
state/api/params/params.go:335: Params []ModifyUser
FWIW, this is identical to `PasswordChanges` in internal.go -- can we possibly
promote that to this file and use it instead? I think it's a bit more of a
generally applicable name.

https://codereview.appspot.com/61620043/diff/90001/state/api/usermanager/clie...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/api/usermanager/clie...
state/api/usermanager/client.go:12: // lp:1288750
by convention:

// TODO(mattyw) 2014-03-07 bug #1288750
// Need a SetPassword method.

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:59: return result, err
Ahh, got it. Failure to create a perm func is the sort of high-level error that
you should return as the simple plain `err`; you don't need to jam it into the
ErrorResults as well (the error is not specific to the first param, it's a
general problem on the api server -- and that's a potential panic because you
don't know for sure that len(args) > 0).

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:66: _, err =
api.state.AddUser(arg.Tag, arg.Password)
feels to me like := is ideal there but I guess it's debatable

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:81: result.Results[0].Error =
common.ServerError(err)
d

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:91: result.Results[i].Error =
common.ServerError(fmt.Errorf("Failed to find user %s: %s", arg.Tag, err))
In general, not-found errors should be expressed to API clients as ErrPerm. I'm
not quite sure whether this specific case is a potential source of information
leakage, but I'd rather be safe than sorry.

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager_test.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:87: // TODO: This test should be
removed 1288745
TODO(<name>) <date> bug #<number>

https://codereview.appspot.com/61620043/diff/90001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/user.go#newcode164
state/user.go:164: func (u *User) SetInactive() error {
Would you use "deactivated" across the board please? I think it's a better fit
for the model we're aiming at.

[mattyw, 2014-03-07 14:12:12]

Please take a look.

https://codereview.appspot.com/61620043/diff/70001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/70001/state/user.go#newcode184
state/user.go:184: func (u *User) IsInactive() bool {
On 2014/03/07 10:00:40, fwereade wrote:
> On 2014/03/07 02:01:22, mattyw wrote:
> > On 2014/03/06 12:11:35, fwereade wrote:
> > > The API server should handle this, and not let inactive users log in. Did
I
> > just
> > > miss the code that handles that?
> > 
> > This should be dealt with in the next merge proposal - the one that
introduces
> > the login command
> > 
> 
> That's not really ideal, because when this CL lands we'll have an API that
> "deactivates" users but doesn't really work. But if you delay landing this CL
to
> arrive with the followup I'll be ok with it.

Done.

https://codereview.appspot.com/61620043/diff/90001/state/api/usermanager/clie...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/api/usermanager/clie...
state/api/usermanager/client.go:12: // lp:1288750
On 2014/03/07 10:00:40, fwereade wrote:
> by convention:
> 
> // TODO(mattyw) 2014-03-07 bug #1288750
> // Need a SetPassword method.

Done.

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:59: return result, err
On 2014/03/07 10:00:40, fwereade wrote:
> Ahh, got it. Failure to create a perm func is the sort of high-level error
that
> you should return as the simple plain `err`; you don't need to jam it into the
> ErrorResults as well (the error is not specific to the first param, it's a
> general problem on the api server -- and that's a potential panic because you
> don't know for sure that len(args) > 0).

Done.

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:66: _, err =
api.state.AddUser(arg.Tag, arg.Password)
On 2014/03/07 10:00:40, fwereade wrote:
> feels to me like := is ideal there but I guess it's debatable

Done.

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:81: result.Results[0].Error =
common.ServerError(err)
On 2014/03/07 10:00:40, fwereade wrote:
> d

Done.

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager.go:91: result.Results[i].Error =
common.ServerError(fmt.Errorf("Failed to find user %s: %s", arg.Tag, err))
On 2014/03/07 10:00:40, fwereade wrote:
> In general, not-found errors should be expressed to API clients as ErrPerm.
I'm
> not quite sure whether this specific case is a potential source of information
> leakage, but I'd rather be safe than sorry.

Done.

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
File state/apiserver/usermanager/usermanager_test.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/apiserver/usermanage...
state/apiserver/usermanager/usermanager_test.go:87: // TODO: This test should be
removed 1288745
On 2014/03/07 10:00:40, fwereade wrote:
> TODO(<name>) <date> bug #<number>

Done.

https://codereview.appspot.com/61620043/diff/90001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/90001/state/user.go#newcode164
state/user.go:164: func (u *User) SetInactive() error {
On 2014/03/07 10:00:40, fwereade wrote:
> Would you use "deactivated" across the board please? I think it's a better fit
> for the model we're aiming at.

Done.

[fwereade, 2014-03-07 14:52:31]

Trivials and test tweaks only: LGTM with the following assuming you're happy
with them.

https://codereview.appspot.com/61620043/diff/110001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/api/deployer/unit.g...
state/api/deployer/unit.go:65: Entities: []params.EntityPassword{
sorry, I must have been unclear -- this *is* a compatibility break, so you need
a struct whose field is "Changes" in this particular case.

https://codereview.appspot.com/61620043/diff/110001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/api/params/params.g...
state/api/params/params.go:31: Entities []EntityPassword
ie *if* you just call this field "Changes" then it'll work across the board and
(I think) not be *completely* shite (because if you squint it's still a relevant
field name in all the cases where it's used).

https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
state/api/usermanager/client.go:38: return *results, err
wait, don't you need to unpack the error from ErrorResults here? and just above,
too?

do a couple of API client tests for adding existing user, and removing admin
user, to verify that you've got the right unpacking going on

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/login_tes...
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/login_tes...
state/apiserver/login_test.go:117: c.Assert(err, gc.ErrorMatches, `unknown
object type "Machiner"`)
maybe better to use Client() and demonstrate that that's unavailable -- Machiner
should *never* be available for a client, but even Client() should be
unavailable until successfully logged in.

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/login_tes...
state/apiserver/login_test.go:121: c.Assert(err, gc.ErrorMatches, "invalid
entity name or password")
...so maybe check you can't get to Client() after this as well?

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/usermanag...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/usermanag...
state/apiserver/usermanager/usermanager.go:58: result.Results[0].Error =
common.ServerError(err)
d as well, sorry, I must have been unclear last review: this is a potential
panic because you don't know if any actual params were passed in

https://codereview.appspot.com/61620043/diff/110001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/user.go#newcode172
state/user.go:172: if u.doc.Name == "admin" {
can we pull this out into a constant please? Can't remember exactly where the
admin user is created but it shouldn't be done with a magic string (but it
probably is at the moment:/ ).

[mattyw, 2014-03-10 13:55:06]

https://codereview.appspot.com/61620043/diff/110001/state/api/deployer/unit.go
File state/api/deployer/unit.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/api/deployer/unit.g...
state/api/deployer/unit.go:65: Entities: []params.EntityPassword{
On 2014/03/07 14:52:32, fwereade wrote:
> sorry, I must have been unclear -- this *is* a compatibility break, so you
need
> a struct whose field is "Changes" in this particular case.

Done.

https://codereview.appspot.com/61620043/diff/110001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/api/params/params.g...
state/api/params/params.go:31: Entities []EntityPassword
On 2014/03/07 14:52:32, fwereade wrote:
> ie *if* you just call this field "Changes" then it'll work across the board
and
> (I think) not be *completely* shite (because if you squint it's still a
relevant
> field name in all the cases where it's used).

Done.

https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
state/api/usermanager/client.go:38: return *results, err
On 2014/03/07 14:52:32, fwereade wrote:
> wait, don't you need to unpack the error from ErrorResults here? and just
above,
> too?
> 
> do a couple of API client tests for adding existing user, and removing admin
> user, to verify that you've got the right unpacking going on

Not sure what you mean by unpacking the errors, I've added the tests

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/login_tes...
File state/apiserver/login_test.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/login_tes...
state/apiserver/login_test.go:117: c.Assert(err, gc.ErrorMatches, `unknown
object type "Machiner"`)
On 2014/03/07 14:52:32, fwereade wrote:
> maybe better to use Client() and demonstrate that that's unavailable --
Machiner
> should *never* be available for a client, but even Client() should be
> unavailable until successfully logged in.

Done.

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/login_tes...
state/apiserver/login_test.go:121: c.Assert(err, gc.ErrorMatches, "invalid
entity name or password")
On 2014/03/07 14:52:32, fwereade wrote:
> ...so maybe check you can't get to Client() after this as well?

Done.

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/usermanag...
File state/apiserver/usermanager/usermanager.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/apiserver/usermanag...
state/apiserver/usermanager/usermanager.go:58: result.Results[0].Error =
common.ServerError(err)
On 2014/03/07 14:52:32, fwereade wrote:
> d as well, sorry, I must have been unclear last review: this is a potential
> panic because you don't know if any actual params were passed in

Done.

https://codereview.appspot.com/61620043/diff/110001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/user.go#newcode172
state/user.go:172: if u.doc.Name == "admin" {
On 2014/03/07 14:52:32, fwereade wrote:
> can we pull this out into a constant please? Can't remember exactly where the
> admin user is created but it shouldn't be done with a magic string (but it
> probably is at the moment:/ ).

Done.

[mattyw, 2014-03-10 14:10:07]

Please take a look.

[rog, 2014-03-12 14:03:49]

https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
state/api/usermanager/client.go:38: return *results, err
On 2014/03/10 13:55:06, mattyw wrote:
> On 2014/03/07 14:52:32, fwereade wrote:
> > wait, don't you need to unpack the error from ErrorResults here? and just
> above,
> > too?
> > 
> > do a couple of API client tests for adding existing user, and removing admin
> > user, to verify that you've got the right unpacking going on
> 
> Not sure what you mean by unpacking the errors, I've added the tests

These methods should be:
func (c *Client) AddUser(tag, password string) error
func (c *Client) RemoveUser(tag string) error

you'll need to check two errors.

https://codereview.appspot.com/61620043/diff/110001/state/user.go
File state/user.go (right):

https://codereview.appspot.com/61620043/diff/110001/state/user.go#newcode172
state/user.go:172: if u.doc.Name == "admin" {
On 2014/03/10 13:55:06, mattyw wrote:
> On 2014/03/07 14:52:32, fwereade wrote:
> > can we pull this out into a constant please? Can't remember exactly where
the
> > admin user is created but it shouldn't be done with a magic string (but it
> > probably is at the moment:/ ).
> 
> Done.

Aside: I actually think it may be fine to deactivate the admin user (perhaps
checking that there's still at least one existing user). That shouldn't block
this though.

[fwereade, 2014-03-13 11:21:08]

On 2014/03/12 14:03:49, rog wrote:
>
https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
> File state/api/usermanager/client.go (right):
> 
>
https://codereview.appspot.com/61620043/diff/110001/state/api/usermanager/cli...
> state/api/usermanager/client.go:38: return *results, err
> On 2014/03/10 13:55:06, mattyw wrote:
> > On 2014/03/07 14:52:32, fwereade wrote:
> > > wait, don't you need to unpack the error from ErrorResults here? and just
> > above,
> > > too?
> > > 
> > > do a couple of API client tests for adding existing user, and removing
admin
> > > user, to verify that you've got the right unpacking going on
> > 
> > Not sure what you mean by unpacking the errors, I've added the tests
> 
> These methods should be:
> func (c *Client) AddUser(tag, password string) error
> func (c *Client) RemoveUser(tag string) error
> 
> you'll need to check two errors.
> 
> https://codereview.appspot.com/61620043/diff/110001/state/user.go
> File state/user.go (right):
> 
> https://codereview.appspot.com/61620043/diff/110001/state/user.go#newcode172
> state/user.go:172: if u.doc.Name == "admin" {
> On 2014/03/10 13:55:06, mattyw wrote:
> > On 2014/03/07 14:52:32, fwereade wrote:
> > > can we pull this out into a constant please? Can't remember exactly where
> the
> > > admin user is created but it shouldn't be done with a magic string (but it
> > > probably is at the moment:/ ).
> > 
> > Done.
> 
> Aside: I actually think it may be fine to deactivate the admin user (perhaps
> checking that there's still at least one existing user). That shouldn't block
> this though.

It's fine until we have users with more restrictive roles, and I worry that
we'll make it possible to lock oneself out of the system permanently. By forcing
the existence of the admin user we get around that.

[mattyw, 2014-03-13 13:40:03]

Please take a look.

[mattyw, 2014-03-13 14:21:58]

Please take a look.

[mattyw, 2014-03-13 14:29:53]

Please take a look.