Validate usernames

Validate usernames

Move username regex validation into
names.IsUser. Update codebase to
ensure usernames are validated
where need.

There are a few comments which are
questions for the reviewer. I will
remove them on LGTM.

https://code.launchpad.net/~waigani/juju-core/use_vailduser_regex/+merge/220535

(do not edit description out of merge proposal)

[waigani, 2014-05-21 22:08:40]

Please take a look.

[menn0, 2014-05-21 23:06:16]

Just a few things.

https://codereview.appspot.com/92560043/diff/1/agent/mongo/upgrade.go
File agent/mongo/upgrade.go (right):

https://codereview.appspot.com/92560043/diff/1/agent/mongo/upgrade.go#newcode61
agent/mongo/upgrade.go:61: }
Is this really necessary? We're already preventing users with improperly formed
usernames from being created and the login will fail if the user doesn't exist.

https://codereview.appspot.com/92560043/diff/1/charm/url_test.go
File charm/url_test.go (right):

https://codereview.appspot.com/92560043/diff/1/charm/url_test.go#newcode179
charm/url_test.go:179: // these tests and would like to migrate them over.
It's definitely not worth have lots of similar tests in 2 places. I would say
the majority of the tests should go with the underlying function. 

Here (charm/url_test.go) I would just do a couple of basic tests, one with a
valid username and one with a invalid username just to make sure a check is
being done and then do the exhaustive testing for names.IsUser().

If you like this style of test (I agree it's better too) then move and adapt
these tests for names.IsUser

https://codereview.appspot.com/92560043/diff/1/cmd/juju/set_test.go
File cmd/juju/set_test.go (right):

https://codereview.appspot.com/92560043/diff/1/cmd/juju/set_test.go#newcode37
cmd/juju/set_test.go:37: // Do we want to validate username before setting it in
the config?
No. The "username" here is just a configuration option for a dummy service for
this test. Juju can't know what kind of policy is required for a charm's option
that might happen to be a username for the software the charm is responsible
for.

https://codereview.appspot.com/92560043/diff/1/cmd/plugins/juju-restore/resto...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/92560043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:480: // Do we want different users to be
able to ssh in?
I wouldn't worry about this. Outside of the scope of this work.

https://codereview.appspot.com/92560043/diff/1/environs/configstore/interface.go
File environs/configstore/interface.go (right):

https://codereview.appspot.com/92560043/diff/1/environs/configstore/interface...
environs/configstore/interface.go:75: // Do we want to validate creds.User with
names.IsUser?
Probably not required. As per earlier, if the username is invalid,
authentication will just fail.

https://codereview.appspot.com/92560043/diff/1/names/user_test.go
File names/user_test.go (right):

https://codereview.appspot.com/92560043/diff/1/names/user_test.go#newcode21
names/user_test.go:21: // This is third place we are testing username
validation. Amalgamate?
As per earlier comment, move most of the testing here?

https://codereview.appspot.com/92560043/diff/1/provider/joyent/config.go
File provider/joyent/config.go (right):

https://codereview.appspot.com/92560043/diff/1/provider/joyent/config.go#newc...
provider/joyent/config.go:61: // validate them?
These loook like joyent specific account details. We should apply the Juju user
name policy to these as there's no guarantee that their policy matches ours.

https://codereview.appspot.com/92560043/diff/1/provider/openstack/config.go
File provider/openstack/config.go (right):

https://codereview.appspot.com/92560043/diff/1/provider/openstack/config.go#n...
provider/openstack/config.go:148: // Do we want to validate username here?
Same as the joyent case. We shouldn't apply Juju's user name policy to openstack
usernames.

https://codereview.appspot.com/92560043/diff/1/state/api/usermanager/client.go
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/92560043/diff/1/state/api/usermanager/client.g...
state/api/usermanager/client.go:32: // happy to fall back to just server if this
is overkill.
I don't think this is overkill. Belts and braces and all that.

[waigani, 2014-05-22 00:59:14]

Please take a look.

https://codereview.appspot.com/92560043/diff/1/agent/mongo/upgrade.go
File agent/mongo/upgrade.go (right):

https://codereview.appspot.com/92560043/diff/1/agent/mongo/upgrade.go#newcode61
agent/mongo/upgrade.go:61: }
On 2014/05/21 23:06:15, menn0 wrote:
> Is this really necessary? We're already preventing users with improperly
formed
> usernames from being created and the login will fail if the user doesn't
exist. 

Yep. I thought you'd say that. I think I was concerned with sanitation of db
calls, but that is a separate issue.

https://codereview.appspot.com/92560043/diff/1/charm/url_test.go
File charm/url_test.go (right):

https://codereview.appspot.com/92560043/diff/1/charm/url_test.go#newcode179
charm/url_test.go:179: // these tests and would like to migrate them over.
On 2014/05/21 23:06:15, menn0 wrote:
> It's definitely not worth have lots of similar tests in 2 places. I would say
> the majority of the tests should go with the underlying function. 
> 
> Here (charm/url_test.go) I would just do a couple of basic tests, one with a
> valid username and one with a invalid username just to make sure a check is
> being done and then do the exhaustive testing for names.IsUser().
> 
> If you like this style of test (I agree it's better too) then move and adapt
> these tests for names.IsUser

Sounds good!

https://codereview.appspot.com/92560043/diff/1/cmd/juju/set_test.go
File cmd/juju/set_test.go (right):

https://codereview.appspot.com/92560043/diff/1/cmd/juju/set_test.go#newcode37
cmd/juju/set_test.go:37: // Do we want to validate username before setting it in
the config?
On 2014/05/21 23:06:15, menn0 wrote:
> No. The "username" here is just a configuration option for a dummy service for
> this test. Juju can't know what kind of policy is required for a charm's
option
> that might happen to be a username for the software the charm is responsible
> for.

Done.

https://codereview.appspot.com/92560043/diff/1/cmd/plugins/juju-restore/resto...
File cmd/plugins/juju-restore/restore.go (right):

https://codereview.appspot.com/92560043/diff/1/cmd/plugins/juju-restore/resto...
cmd/plugins/juju-restore/restore.go:480: // Do we want different users to be
able to ssh in?
On 2014/05/21 23:06:15, menn0 wrote:
> I wouldn't worry about this. Outside of the scope of this work.

Done.

https://codereview.appspot.com/92560043/diff/1/environs/configstore/interface.go
File environs/configstore/interface.go (right):

https://codereview.appspot.com/92560043/diff/1/environs/configstore/interface...
environs/configstore/interface.go:75: // Do we want to validate creds.User with
names.IsUser?
On 2014/05/21 23:06:15, menn0 wrote:
> Probably not required. As per earlier, if the username is invalid,
> authentication will just fail.

Done.

https://codereview.appspot.com/92560043/diff/1/provider/joyent/config.go
File provider/joyent/config.go (right):

https://codereview.appspot.com/92560043/diff/1/provider/joyent/config.go#newc...
provider/joyent/config.go:61: // validate them?
On 2014/05/21 23:06:15, menn0 wrote:
> These loook like joyent specific account details. We should apply the Juju
user
> name policy to these as there's no guarantee that their policy matches ours.

We should NOT apply, right? How do provider specific user accounts relate to
juju users, if at all?

https://codereview.appspot.com/92560043/diff/1/provider/openstack/config.go
File provider/openstack/config.go (right):

https://codereview.appspot.com/92560043/diff/1/provider/openstack/config.go#n...
provider/openstack/config.go:148: // Do we want to validate username here?
On 2014/05/21 23:06:15, menn0 wrote:
> Same as the joyent case. We shouldn't apply Juju's user name policy to
openstack
> usernames.

Same question as joyent case. How do provider users relate to juju users? A
provider will expect users that conform to their policies when authenticating
and authorising use of resources within it's environment.

Turning the question on its head, does Juju's user name policy need to conform
to the provider's policy? Will juju users be proxies for provider users?

https://codereview.appspot.com/92560043/diff/1/state/api/usermanager/client.go
File state/api/usermanager/client.go (right):

https://codereview.appspot.com/92560043/diff/1/state/api/usermanager/client.g...
state/api/usermanager/client.go:32: // happy to fall back to just server if this
is overkill.
On 2014/05/21 23:06:15, menn0 wrote:
> I don't think this is overkill. Belts and braces and all that.

Done.

[menn0, 2014-05-22 01:13:44]

Nice. LGTM

https://codereview.appspot.com/92560043/diff/1/provider/joyent/config.go
File provider/joyent/config.go (right):

https://codereview.appspot.com/92560043/diff/1/provider/joyent/config.go#newc...
provider/joyent/config.go:61: // validate them?
On 2014/05/22 00:59:13, waigani wrote:
> On 2014/05/21 23:06:15, menn0 wrote:
> > These loook like joyent specific account details. We should apply the Juju
> user
> > name policy to these as there's no guarantee that their policy matches ours.
> 
> We should NOT apply, right? How do provider specific user accounts relate to
> juju users, if at all?

Sorry, missed a word. Definitely NOT :)

https://codereview.appspot.com/92560043/diff/1/provider/openstack/config.go
File provider/openstack/config.go (right):

https://codereview.appspot.com/92560043/diff/1/provider/openstack/config.go#n...
provider/openstack/config.go:148: // Do we want to validate username here?
On 2014/05/22 00:59:13, waigani wrote:
> On 2014/05/21 23:06:15, menn0 wrote:
> > Same as the joyent case. We shouldn't apply Juju's user name policy to
> openstack
> > usernames.
> 
> Same question as joyent case. How do provider users relate to juju users? A
> provider will expect users that conform to their policies when authenticating
> and authorising use of resources within it's environment.
> 
> Turning the question on its head, does Juju's user name policy need to conform
> to the provider's policy? Will juju users be proxies for provider users?

The provider account details and juju usernames live in different worlds.
There's no real relationship between them.