|
|
|
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)
Patch Set 1 #
Total comments: 19
Patch Set 2 : Validate usernames #
Total messages: 4
|
waigani
Please take a look.
|
11 years, 7 months ago (2014年05月21日 22:08:40 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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.
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.
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.