Merge lp:~thumper/juju-core/user-display-name into lp:~go-bot/juju-core/trunk
- user-display-name
- Merge into trunk
Status: | Needs review |
---|---|
Proposed branch: | lp:~thumper/juju-core/user-display-name |
Merge into: | lp:~go-bot/juju-core/trunk |
Diff against target: |
1164 lines (+351/-240) 22 files modified
agent/bootstrap.go (+1/-1) cmd/juju/authorizedkeys_test.go (+4/-8) cmd/juju/user_add.go (+43/-28) cmd/juju/user_add_test.go (+148/-76) juju/conn_test.go (+1/-1) juju/testing/conn.go (+6/-0) provider/dummy/environs.go (+1/-1) state/api/params/params.go (+12/-2) state/api/usermanager/client.go (+8/-7) state/api/usermanager/client_test.go (+18/-5) state/apiserver/charms_test.go (+2/-5) state/apiserver/client/api_test.go (+1/-2) state/apiserver/client/client_test.go (+2/-5) state/apiserver/login_test.go (+1/-2) state/apiserver/usermanager/usermanager.go (+9/-4) state/apiserver/usermanager/usermanager_test.go (+24/-19) state/compat_test.go (+1/-1) state/conn_test.go (+1/-1) state/megawatcher_internal_test.go (+1/-1) state/state_test.go (+2/-2) state/user.go (+16/-9) state/user_test.go (+49/-60) |
To merge this branch: | bzr merge lp:~thumper/juju-core/user-display-name |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+221823@code.launchpad.net |
Commit message
Description of the change
Add a display name option for new users
This ended up being more work than I hoped as I refactored the apiserver
parts, many tests, and made sure there was backwards compatability with
older client apis.
Also changed how the CLI command was tested so it didn't need a
JujuConnSuite.
Menno Finlay-Smits (menno.smits) wrote : | # |
LGTM - only minor comments and praise added in-line. The user add tests
look much better.
One thing: can this actually land without schema migration support?
https:/
File cmd/juju/
https:/
cmd/juju/
Sorry for missing this when I implemented --password
https:/
File cmd/juju/
https:/
cmd/juju/
errors.Trace() here too?
https:/
File state/api/
https:/
state/api/
[]params.
These names were terrible! Glad you've fixed it.
https:/
File state/api/
https:/
state/api/
[]params.
Password: password}},
This is much more readable.
https:/
File state/api/
https:/
state/api/
s.APIState.
Maybe add a comment here about why s.APIState.
used here instead of s.usermanager.
to think about it for a bit.
https:/
File state/apiserver
https:/
state/apiserver
utils.RandomPas
Play devil's advocate: this change means the user has a fixed password
instead of a random one. Are you sure that doesn't negatively impact any
existing tests? I can't think of why it would but it is a test
functionality change.
https:/
File state/user_test.go (right):
https:/
state/user_
{
addSomeUser might be a better name, indicating that a test/throwaway
user is being created. It also hints at the username being used.
Jesse Meek (waigani) wrote : | # |
On 2014/06/03 05:17:26, menn0 wrote:
> LGTM - only minor comments and praise added in-line. The user add
tests look
> much better.
> One thing: can this actually land without schema migration support?
> https:/
> File cmd/juju/
https:/
> cmd/juju/
> Sorry for missing this when I implemented --password
> https:/
> File cmd/juju/
https:/
> cmd/juju/
> errors.Trace() here too?
https:/
> File state/api/
https:/
> state/api/
params.
> []params.
> These names were terrible! Glad you've fixed it.
https:/
> File state/api/
https:/
> state/api/
[]params.
> username, DisplayName: displayName, Password: password}},
> This is much more readable.
https:/
> File state/api/
https:/
> state/api/
s.APIState.
> "", "AddUser", userArgs, results)
> Maybe add a comment here about why s.APIState.
is used
> here instead of s.usermanager.
think about
> it for a bit.
https:/
> File state/apiserver
https:/
> state/apiserver
utils.RandomPas
> Play devil's advocate: this change means the user has a fixed password
instead
> of a random one. Are you sure that doesn't negatively impact any
existing tests?
> I can't think of why it would but it is a test functionality change.
> https:/
> File state/user_test.go (right):
https:/
> state/user_
*state.User {
> addSomeUser might be a better name, indicating that a test/throwaway
user is
> being created. It also hints at the username being used.
LGTM. Just one suggestion inline. The bulk of the changes look to be
updating AddUser to take an extra arg. As to schema migration support, I
think adding fields is fine - its when we change or remove them ...
Jesse Meek (waigani) wrote : | # |
https:/
File state/user_test.go (right):
https:/
state/user_
It seems you are making a user for every test. Why not makeUser in
SetUpTest?
Tim Penhey (thumper) wrote : | # |
New branch is moving to github.
https:/
File state/api/
https:/
state/api/
s.APIState.
On 2014/06/03 05:17:26, menn0 wrote:
> Maybe add a comment here about why s.APIState.
is used
> here instead of s.usermanager.
think about
> it for a bit.
Done.
https:/
File state/apiserver
https:/
state/apiserver
utils.RandomPas
On 2014/06/03 05:17:26, menn0 wrote:
> Play devil's advocate: this change means the user has a fixed password
instead
> of a random one. Are you sure that doesn't negatively impact any
existing tests?
> I can't think of why it would but it is a test functionality change.
Sure, but the tests all still pass :-)
I'm considering a way to change the way we create test fixture objects.
A cunning plan.
https:/
File state/user_test.go (right):
https:/
state/user_
On 2014/06/03 21:29:26, waigani wrote:
> It seems you are making a user for every test. Why not makeUser in
SetUpTest?
Not quite every test :-)
Unmerged revisions
- 2819. By Tim Penhey
-
Fix the test.
- 2818. By Tim Penhey
-
Lots of test rework.
- 2817. By Tim Penhey
-
Merge trunk.
- 2816. By Tim Penhey
-
Minor cleanups
- 2815. By Tim Penhey
-
Update other test call sites when they don't care about user details.
- 2814. By Tim Penhey
-
Update client calls
- 2813. By Tim Penhey
-
Fix the state tests.
- 2812. By Tim Penhey
-
The state changes for more params.
- 2811. By Tim Penhey
-
Extra params for AddUser.
Preview Diff
1 | === modified file 'agent/bootstrap.go' |
2 | --- agent/bootstrap.go 2014-05-08 07:04:54 +0000 |
3 | +++ agent/bootstrap.go 2014-06-03 04:22:37 +0000 |
4 | @@ -121,7 +121,7 @@ |
5 | func initBootstrapUser(st *state.State, passwordHash string) error { |
6 | logger.Debugf("adding admin user") |
7 | // Set up initial authentication. |
8 | - u, err := st.AddUser("admin", "") |
9 | + u, err := st.AddUser("admin", "", "") |
10 | if err != nil { |
11 | return err |
12 | } |
13 | |
14 | === modified file 'cmd/juju/authorizedkeys_test.go' |
15 | --- cmd/juju/authorizedkeys_test.go 2014-05-22 06:03:06 +0000 |
16 | +++ cmd/juju/authorizedkeys_test.go 2014-06-03 04:22:37 +0000 |
17 | @@ -139,8 +139,7 @@ |
18 | key1 := sshtesting.ValidKeyOne.Key + " user@host" |
19 | key2 := sshtesting.ValidKeyTwo.Key + " another@host" |
20 | s.setAuthorizedKeys(c, key1, key2) |
21 | - _, err := s.State.AddUser("fred", "password") |
22 | - c.Assert(err, gc.IsNil) |
23 | + s.AddUser(c, "fred") |
24 | |
25 | context, err := coretesting.RunCommand(c, envcmd.Wrap(&ListKeysCommand{}), "--user", "fred") |
26 | c.Assert(err, gc.IsNil) |
27 | @@ -174,8 +173,7 @@ |
28 | func (s *AddKeySuite) TestAddKeyNonDefaultUser(c *gc.C) { |
29 | key1 := sshtesting.ValidKeyOne.Key + " user@host" |
30 | s.setAuthorizedKeys(c, key1) |
31 | - _, err := s.State.AddUser("fred", "password") |
32 | - c.Assert(err, gc.IsNil) |
33 | + s.AddUser(c, "fred") |
34 | |
35 | key2 := sshtesting.ValidKeyTwo.Key + " another@host" |
36 | context, err := coretesting.RunCommand(c, envcmd.Wrap(&AddKeysCommand{}), "--user", "fred", key2) |
37 | @@ -206,8 +204,7 @@ |
38 | key1 := sshtesting.ValidKeyOne.Key + " user@host" |
39 | key2 := sshtesting.ValidKeyTwo.Key + " another@host" |
40 | s.setAuthorizedKeys(c, key1, key2) |
41 | - _, err := s.State.AddUser("fred", "password") |
42 | - c.Assert(err, gc.IsNil) |
43 | + s.AddUser(c, "fred") |
44 | |
45 | context, err := coretesting.RunCommand(c, envcmd.Wrap(&DeleteKeysCommand{}), |
46 | "--user", "fred", sshtesting.ValidKeyTwo.Fingerprint) |
47 | @@ -240,8 +237,7 @@ |
48 | func (s *ImportKeySuite) TestImportKeyNonDefaultUser(c *gc.C) { |
49 | key1 := sshtesting.ValidKeyOne.Key + " user@host" |
50 | s.setAuthorizedKeys(c, key1) |
51 | - _, err := s.State.AddUser("fred", "password") |
52 | - c.Assert(err, gc.IsNil) |
53 | + s.AddUser(c, "fred") |
54 | |
55 | context, err := coretesting.RunCommand(c, envcmd.Wrap(&ImportKeysCommand{}), "--user", "fred", "lp:validuser") |
56 | c.Assert(err, gc.IsNil) |
57 | |
58 | === modified file 'cmd/juju/user_add.go' |
59 | --- cmd/juju/user_add.go 2014-05-26 00:06:16 +0000 |
60 | +++ cmd/juju/user_add.go 2014-06-03 04:22:37 +0000 |
61 | @@ -8,6 +8,7 @@ |
62 | "os" |
63 | "strings" |
64 | |
65 | + "github.com/juju/errors" |
66 | "launchpad.net/gnuflag" |
67 | |
68 | "launchpad.net/juju-core/cmd" |
69 | @@ -33,40 +34,49 @@ |
70 | |
71 | type UserAddCommand struct { |
72 | envcmd.EnvCommandBase |
73 | - User string |
74 | - Password string |
75 | - outPath string |
76 | + User string |
77 | + DisplayName string |
78 | + Password string |
79 | + outPath string |
80 | } |
81 | |
82 | func (c *UserAddCommand) Info() *cmd.Info { |
83 | return &cmd.Info{ |
84 | Name: "add", |
85 | - Args: "<username> <password>", |
86 | + Args: "<username> [<display name>]", |
87 | Purpose: "adds a user", |
88 | Doc: userAddCommandDoc, |
89 | } |
90 | } |
91 | |
92 | func (c *UserAddCommand) SetFlags(f *gnuflag.FlagSet) { |
93 | - f.StringVar(&(c.Password), "password", "", "Password for new user") |
94 | - f.StringVar(&(c.outPath), "o", "", "Output an environment file for new user") |
95 | - f.StringVar(&(c.outPath), "output", "", "") |
96 | + f.StringVar(&c.Password, "password", "", "Password for new user") |
97 | + f.StringVar(&c.outPath, "o", "", "Output an environment file for new user") |
98 | + f.StringVar(&c.outPath, "output", "", "") |
99 | } |
100 | |
101 | func (c *UserAddCommand) Init(args []string) error { |
102 | - switch len(args) { |
103 | - case 0: |
104 | + if len(args) == 0 { |
105 | return fmt.Errorf("no username supplied") |
106 | - case 1: |
107 | - c.User = args[0] |
108 | - default: |
109 | - return cmd.CheckEmpty(args[1:]) |
110 | - } |
111 | - return nil |
112 | + } |
113 | + c.User, args = args[0], args[1:] |
114 | + if len(args) > 0 { |
115 | + c.DisplayName, args = args[0], args[1:] |
116 | + } |
117 | + return cmd.CheckEmpty(args) |
118 | +} |
119 | + |
120 | +type addUserAPI interface { |
121 | + AddUser(username, displayname, password string) error |
122 | + Close() error |
123 | +} |
124 | + |
125 | +var getAddUserAPI = func(c *UserAddCommand) (addUserAPI, error) { |
126 | + return juju.NewUserManagerClient(c.EnvName) |
127 | } |
128 | |
129 | func (c *UserAddCommand) Run(ctx *cmd.Context) error { |
130 | - client, err := juju.NewUserManagerClient(c.EnvName) |
131 | + client, err := getAddUserAPI(c) |
132 | if err != nil { |
133 | return err |
134 | } |
135 | @@ -74,19 +84,24 @@ |
136 | if c.Password == "" { |
137 | c.Password, err = utils.RandomPassword() |
138 | if err != nil { |
139 | - return fmt.Errorf("Failed to generate password: %v", err) |
140 | + return errors.Annotate(err, "failed to generate password") |
141 | } |
142 | } |
143 | |
144 | - err = client.AddUser(c.User, c.Password) |
145 | + err = client.AddUser(c.User, c.DisplayName, c.Password) |
146 | if err != nil { |
147 | return err |
148 | } |
149 | - fmt.Fprintf(ctx.Stdout, "user \"%s\" added with password \"%s\"\n", c.User, c.Password) |
150 | + user := c.User |
151 | + if c.DisplayName != "" { |
152 | + user = fmt.Sprintf("%s (%s)", c.DisplayName, user) |
153 | + } |
154 | + |
155 | + fmt.Fprintf(ctx.Stdout, "user %q added with password %q\n", user, c.Password) |
156 | |
157 | if c.outPath != "" { |
158 | outPath := NormaliseJenvPath(ctx, c.outPath) |
159 | - err := GenerateUserJenv(c.EnvName, c.User, c.Password, outPath) |
160 | + err = GenerateUserJenv(c.EnvName, c.User, c.Password, outPath) |
161 | if err == nil { |
162 | fmt.Fprintf(ctx.Stdout, "environment file written to %s\n", outPath) |
163 | } |
164 | @@ -101,14 +116,14 @@ |
165 | return ctx.AbsPath(outPath) |
166 | } |
167 | |
168 | -func GenerateUserJenv(envName, user, password, outPath string) (err error) { |
169 | +func GenerateUserJenv(envName, user, password, outPath string) error { |
170 | store, err := configstore.Default() |
171 | if err != nil { |
172 | - return |
173 | + return errors.Trace(err) |
174 | } |
175 | storeInfo, err := store.ReadInfo(envName) |
176 | if err != nil { |
177 | - return |
178 | + return errors.Trace(err) |
179 | } |
180 | outputInfo := configstore.EnvironInfoData{} |
181 | outputInfo.User = user |
182 | @@ -117,17 +132,17 @@ |
183 | outputInfo.CACert = storeInfo.APIEndpoint().CACert |
184 | yaml, err := cmd.FormatYaml(outputInfo) |
185 | if err != nil { |
186 | - return |
187 | + return errors.Trace(err) |
188 | } |
189 | |
190 | outFile, err := os.Create(outPath) |
191 | if err != nil { |
192 | - return |
193 | + return errors.Trace(err) |
194 | } |
195 | defer outFile.Close() |
196 | - _, err = outFile.Write(yaml) |
197 | + outFile.Write(yaml) |
198 | if err != nil { |
199 | - return |
200 | + return errors.Trace(err) |
201 | } |
202 | - return |
203 | + return nil |
204 | } |
205 | |
206 | === modified file 'cmd/juju/user_add_test.go' |
207 | --- cmd/juju/user_add_test.go 2014-05-26 00:06:16 +0000 |
208 | +++ cmd/juju/user_add_test.go 2014-06-03 04:22:37 +0000 |
209 | @@ -4,11 +4,10 @@ |
210 | package main |
211 | |
212 | import ( |
213 | - "bytes" |
214 | + "errors" |
215 | + "fmt" |
216 | "io/ioutil" |
217 | "path/filepath" |
218 | - "regexp" |
219 | - "strings" |
220 | |
221 | jc "github.com/juju/testing/checkers" |
222 | gc "launchpad.net/gocheck" |
223 | @@ -16,107 +15,180 @@ |
224 | |
225 | "launchpad.net/juju-core/cmd" |
226 | "launchpad.net/juju-core/cmd/envcmd" |
227 | - jujutesting "launchpad.net/juju-core/juju/testing" |
228 | + "launchpad.net/juju-core/environs/configstore" |
229 | "launchpad.net/juju-core/testing" |
230 | ) |
231 | |
232 | // All of the functionality of the AddUser api call is contained elsewhere. |
233 | // This suite provides basic tests for the "user add" command |
234 | type UserAddCommandSuite struct { |
235 | - jujutesting.JujuConnSuite |
236 | + testing.FakeJujuHomeSuite |
237 | + mockAPI *mockAddUserAPI |
238 | } |
239 | |
240 | var _ = gc.Suite(&UserAddCommandSuite{}) |
241 | |
242 | +func (s *UserAddCommandSuite) SetUpTest(c *gc.C) { |
243 | + s.FakeJujuHomeSuite.SetUpTest(c) |
244 | + s.mockAPI = &mockAddUserAPI{} |
245 | + s.PatchValue(&getAddUserAPI, func(c *UserAddCommand) (addUserAPI, error) { |
246 | + return s.mockAPI, nil |
247 | + }) |
248 | +} |
249 | + |
250 | func newUserAddCommand() cmd.Command { |
251 | return envcmd.Wrap(&UserAddCommand{}) |
252 | } |
253 | |
254 | -func (s *UserAddCommandSuite) TestUserAdd(c *gc.C) { |
255 | - _, err := testing.RunCommand(c, newUserAddCommand(), "foobar") |
256 | - c.Assert(err, gc.IsNil) |
257 | - |
258 | - _, err = testing.RunCommand(c, newUserAddCommand(), "foobar") |
259 | - c.Assert(err, gc.ErrorMatches, "Failed to create user: user already exists") |
260 | -} |
261 | - |
262 | -func (s *UserAddCommandSuite) TestTooManyArgs(c *gc.C) { |
263 | - _, err := testing.RunCommand(c, newUserAddCommand(), "foobar", "whoops") |
264 | - c.Assert(err, gc.ErrorMatches, `unrecognized args: \["whoops"\]`) |
265 | -} |
266 | - |
267 | -func (s *UserAddCommandSuite) TestNotEnoughArgs(c *gc.C) { |
268 | - _, err := testing.RunCommand(c, newUserAddCommand()) |
269 | - c.Assert(err, gc.ErrorMatches, `no username supplied`) |
270 | -} |
271 | - |
272 | -func (s *UserAddCommandSuite) TestGeneratePassword(c *gc.C) { |
273 | - ctx, err := testing.RunCommand(c, newUserAddCommand(), "foobar") |
274 | - |
275 | - c.Assert(err, gc.IsNil) |
276 | - user, password, filename := parseUserAddStdout(c, ctx) |
277 | - c.Assert(user, gc.Equals, "foobar") |
278 | - // Let's not try to assume too much about the password generation |
279 | - // algorithm other than there will be at least 10 characters. |
280 | - c.Assert(password, gc.Matches, "..........+") |
281 | - c.Assert(filename, gc.Equals, "") |
282 | -} |
283 | - |
284 | -func (s *UserAddCommandSuite) TestUserSpecifiedPassword(c *gc.C) { |
285 | - ctx, err := testing.RunCommand(c, newUserAddCommand(), "foobar", "--password", "frogdog") |
286 | - c.Assert(err, gc.IsNil) |
287 | - |
288 | - user, password, filename := parseUserAddStdout(c, ctx) |
289 | - c.Assert(user, gc.Equals, "foobar") |
290 | - c.Assert(password, gc.Equals, "frogdog") |
291 | - c.Assert(filename, gc.Equals, "") |
292 | +func (s *UserAddCommandSuite) TestAddUserJustUsername(c *gc.C) { |
293 | + context, err := testing.RunCommand(c, newUserAddCommand(), "foobar") |
294 | + c.Assert(err, gc.IsNil) |
295 | + c.Assert(s.mockAPI.username, gc.Equals, "foobar") |
296 | + c.Assert(s.mockAPI.displayname, gc.Equals, "") |
297 | + // Password is generated |
298 | + c.Assert(s.mockAPI.password, gc.Not(gc.Equals), "") |
299 | + expected := fmt.Sprintf(`user "foobar" added with password %q`, s.mockAPI.password) |
300 | + c.Assert(testing.Stdout(context), gc.Equals, expected+"\n") |
301 | +} |
302 | + |
303 | +func (s *UserAddCommandSuite) TestAddUserUsernameAndDisplayname(c *gc.C) { |
304 | + context, err := testing.RunCommand(c, newUserAddCommand(), "foobar", "Foo Bar") |
305 | + c.Assert(err, gc.IsNil) |
306 | + c.Assert(s.mockAPI.username, gc.Equals, "foobar") |
307 | + c.Assert(s.mockAPI.displayname, gc.Equals, "Foo Bar") |
308 | + // Password is generated |
309 | + c.Assert(s.mockAPI.password, gc.Not(gc.Equals), "") |
310 | + expected := fmt.Sprintf(`user "Foo Bar (foobar)" added with password %q`, s.mockAPI.password) |
311 | + c.Assert(testing.Stdout(context), gc.Equals, expected+"\n") |
312 | +} |
313 | + |
314 | +func (s *UserAddCommandSuite) TestAddUserUsernameAndDisplaynameWithPassword(c *gc.C) { |
315 | + context, err := testing.RunCommand(c, newUserAddCommand(), "foobar", "Foo Bar", "--password", "password") |
316 | + c.Assert(err, gc.IsNil) |
317 | + c.Assert(s.mockAPI.username, gc.Equals, "foobar") |
318 | + c.Assert(s.mockAPI.displayname, gc.Equals, "Foo Bar") |
319 | + c.Assert(s.mockAPI.password, gc.Equals, "password") |
320 | + expected := `user "Foo Bar (foobar)" added with password "password"` |
321 | + c.Assert(testing.Stdout(context), gc.Equals, expected+"\n") |
322 | +} |
323 | + |
324 | +func (s *UserAddCommandSuite) TestAddUserErrorResponse(c *gc.C) { |
325 | + s.mockAPI.failMessage = "failed to create user, chaos ensues" |
326 | + context, err := testing.RunCommand(c, newUserAddCommand(), "foobar") |
327 | + c.Assert(err, gc.ErrorMatches, "failed to create user, chaos ensues") |
328 | + c.Assert(s.mockAPI.username, gc.Equals, "foobar") |
329 | + c.Assert(s.mockAPI.displayname, gc.Equals, "") |
330 | + c.Assert(testing.Stdout(context), gc.Equals, "") |
331 | +} |
332 | + |
333 | +func (s *UserAddCommandSuite) TestInit(c *gc.C) { |
334 | + for i, test := range []struct { |
335 | + args []string |
336 | + user string |
337 | + displayname string |
338 | + password string |
339 | + outPath string |
340 | + errorString string |
341 | + }{ |
342 | + { |
343 | + errorString: "no username supplied", |
344 | + }, { |
345 | + args: []string{"foobar"}, |
346 | + user: "foobar", |
347 | + }, { |
348 | + args: []string{"foobar", "Foo Bar"}, |
349 | + user: "foobar", |
350 | + displayname: "Foo Bar", |
351 | + }, { |
352 | + args: []string{"foobar", "Foo Bar", "extra"}, |
353 | + errorString: `unrecognized args: \["extra"\]`, |
354 | + }, { |
355 | + args: []string{"foobar", "--password", "password"}, |
356 | + user: "foobar", |
357 | + password: "password", |
358 | + }, { |
359 | + args: []string{"foobar", "--output", "somefile"}, |
360 | + user: "foobar", |
361 | + outPath: "somefile", |
362 | + }, { |
363 | + args: []string{"foobar", "-o", "somefile"}, |
364 | + user: "foobar", |
365 | + outPath: "somefile", |
366 | + }, |
367 | + } { |
368 | + c.Logf("test %d", i) |
369 | + addUserCmd := &UserAddCommand{} |
370 | + err := testing.InitCommand(addUserCmd, test.args) |
371 | + if test.errorString == "" { |
372 | + c.Check(addUserCmd.User, gc.Equals, test.user) |
373 | + c.Check(addUserCmd.DisplayName, gc.Equals, test.displayname) |
374 | + c.Check(addUserCmd.Password, gc.Equals, test.password) |
375 | + c.Check(addUserCmd.outPath, gc.Equals, test.outPath) |
376 | + } else { |
377 | + c.Check(err, gc.ErrorMatches, test.errorString) |
378 | + } |
379 | + } |
380 | +} |
381 | + |
382 | +func fakeBootstrapEnvironment(c *gc.C, envName string) { |
383 | + store, err := configstore.Default() |
384 | + c.Assert(err, gc.IsNil) |
385 | + envInfo, err := store.CreateInfo(envName) |
386 | + c.Assert(err, gc.IsNil) |
387 | + envInfo.SetBootstrapConfig(map[string]interface{}{"random": "extra data"}) |
388 | + envInfo.SetAPIEndpoint(configstore.APIEndpoint{ |
389 | + Addresses: []string{"localhost:12345"}, |
390 | + CACert: testing.CACert, |
391 | + }) |
392 | + envInfo.SetAPICredentials(configstore.APICredentials{ |
393 | + User: "admin", |
394 | + Password: "password", |
395 | + }) |
396 | + err = envInfo.Write() |
397 | + c.Assert(err, gc.IsNil) |
398 | } |
399 | |
400 | func (s *UserAddCommandSuite) TestJenvOutput(c *gc.C) { |
401 | + fakeBootstrapEnvironment(c, "erewhemos") |
402 | outputName := filepath.Join(c.MkDir(), "output") |
403 | ctx, err := testing.RunCommand(c, newUserAddCommand(), |
404 | "foobar", "--password", "password", "--output", outputName) |
405 | c.Assert(err, gc.IsNil) |
406 | |
407 | - user, password, filename := parseUserAddStdout(c, ctx) |
408 | - c.Assert(user, gc.Equals, "foobar") |
409 | - c.Assert(password, gc.Equals, "password") |
410 | - c.Assert(filename, gc.Equals, outputName+".jenv") |
411 | + expected := fmt.Sprintf(`user "foobar" added with password %q`, s.mockAPI.password) |
412 | + expected = fmt.Sprintf("%s\nenvironment file written to %s.jenv\n", expected, outputName) |
413 | + c.Assert(testing.Stdout(ctx), gc.Equals, expected) |
414 | |
415 | - raw, err := ioutil.ReadFile(filename) |
416 | + raw, err := ioutil.ReadFile(outputName + ".jenv") |
417 | c.Assert(err, gc.IsNil) |
418 | d := map[string]interface{}{} |
419 | err = goyaml.Unmarshal(raw, &d) |
420 | c.Assert(err, gc.IsNil) |
421 | c.Assert(d["user"], gc.Equals, "foobar") |
422 | c.Assert(d["password"], gc.Equals, "password") |
423 | - _, found := d["state-servers"] |
424 | - c.Assert(found, gc.Equals, true) |
425 | - _, found = d["ca-cert"] |
426 | - c.Assert(found, gc.Equals, true) |
427 | -} |
428 | - |
429 | -// parseUserAddStdout parses the output from the "juju user add" |
430 | -// command and checks that it has the correct form, returning the |
431 | -// interesting parts. The .jenv filename will be an empty string when |
432 | -// it wasn't included in the output. |
433 | -func parseUserAddStdout(c *gc.C, ctx *cmd.Context) (user string, password string, filename string) { |
434 | - stdout := strings.TrimSpace(ctx.Stdout.(*bytes.Buffer).String()) |
435 | - lines := strings.Split(stdout, "\n") |
436 | - c.Assert(len(lines), jc.LessThan, 3) |
437 | - |
438 | - reLine0 := regexp.MustCompile(`^user "(.+)" added with password "(.+)"$`) |
439 | - line0Matches := reLine0.FindStringSubmatch(lines[0]) |
440 | - c.Assert(len(line0Matches), gc.Equals, 3) |
441 | - user, password = line0Matches[1], line0Matches[2] |
442 | - |
443 | - if len(lines) == 2 { |
444 | - reLine1 := regexp.MustCompile(`^environment file written to (.+)$`) |
445 | - line1Matches := reLine1.FindStringSubmatch(lines[1]) |
446 | - c.Assert(len(line1Matches), gc.Equals, 2) |
447 | - filename = line1Matches[1] |
448 | - } else { |
449 | - filename = "" |
450 | + c.Assert(d["state-servers"], gc.DeepEquals, []interface{}{"localhost:12345"}) |
451 | + c.Assert(d["ca-cert"], gc.DeepEquals, testing.CACert) |
452 | + _, found := d["bootstrap-config"] |
453 | + c.Assert(found, jc.IsFalse) |
454 | +} |
455 | + |
456 | +type mockAddUserAPI struct { |
457 | + failMessage string |
458 | + username string |
459 | + displayname string |
460 | + password string |
461 | +} |
462 | + |
463 | +func (m *mockAddUserAPI) AddUser(username, displayname, password string) error { |
464 | + m.username = username |
465 | + m.displayname = displayname |
466 | + m.password = password |
467 | + if m.failMessage == "" { |
468 | + return nil |
469 | } |
470 | - return |
471 | + return errors.New(m.failMessage) |
472 | +} |
473 | + |
474 | +func (*mockAddUserAPI) Close() error { |
475 | + return nil |
476 | } |
477 | |
478 | === modified file 'juju/conn_test.go' |
479 | --- juju/conn_test.go 2014-05-20 04:27:02 +0000 |
480 | +++ juju/conn_test.go 2014-06-03 04:22:37 +0000 |
481 | @@ -501,7 +501,7 @@ |
482 | |
483 | func (s *DeployLocalSuite) TestDeployOwnerTag(c *gc.C) { |
484 | usermanager := usermanager.NewClient(s.APIState) |
485 | - err := usermanager.AddUser("foobar", "") |
486 | + err := usermanager.AddUser("foobar", "", "") |
487 | c.Assert(err, gc.IsNil) |
488 | service, err := juju.DeployService(s.State, |
489 | juju.DeployServiceParams{ |
490 | |
491 | === modified file 'juju/testing/conn.go' |
492 | --- juju/testing/conn.go 2014-05-26 08:25:47 +0000 |
493 | +++ juju/testing/conn.go 2014-06-03 04:22:37 +0000 |
494 | @@ -98,6 +98,12 @@ |
495 | s.setUpConn(c) |
496 | } |
497 | |
498 | +func (s *JujuConnSuite) AddUser(c *gc.C, username string) *state.User { |
499 | + user, err := s.State.AddUser(username, "", "password") |
500 | + c.Assert(err, gc.IsNil) |
501 | + return user |
502 | +} |
503 | + |
504 | func (s *JujuConnSuite) StateInfo(c *gc.C) *state.Info { |
505 | info, _, err := s.Conn.Environ.StateInfo() |
506 | c.Assert(err, gc.IsNil) |
507 | |
508 | === modified file 'provider/dummy/environs.go' |
509 | --- provider/dummy/environs.go 2014-05-16 13:03:36 +0000 |
510 | +++ provider/dummy/environs.go 2014-06-03 04:22:37 +0000 |
511 | @@ -629,7 +629,7 @@ |
512 | if err := st.SetAdminMongoPassword(utils.UserPasswordHash(password, utils.CompatSalt)); err != nil { |
513 | panic(err) |
514 | } |
515 | - _, err = st.AddUser("admin", password) |
516 | + _, err = st.AddUser("admin", "", password) |
517 | if err != nil { |
518 | panic(err) |
519 | } |
520 | |
521 | === modified file 'state/api/params/params.go' |
522 | --- state/api/params/params.go 2014-05-27 08:30:44 +0000 |
523 | +++ state/api/params/params.go 2014-06-03 04:22:37 +0000 |
524 | @@ -382,10 +382,20 @@ |
525 | Keys []string |
526 | } |
527 | |
528 | +// ModifyUsers holds the parameters for making a UserManager Add or Modify calls. |
529 | +type ModifyUsers struct { |
530 | + Changes []ModifyUser |
531 | +} |
532 | + |
533 | // ModifyUser stores the parameters used for a UserManager.Add|Remove call |
534 | type ModifyUser struct { |
535 | - Tag string |
536 | - Password string |
537 | + // Tag is here purely for backwards compatability. Older clients will |
538 | + // attempt to use the EntityPassword structure, so we need a Tag here |
539 | + // (which will be treated as Username) |
540 | + Tag string |
541 | + Username string |
542 | + DisplayName string |
543 | + Password string |
544 | } |
545 | |
546 | // MarshalJSON implements json.Marshaler. |
547 | |
548 | === modified file 'state/api/usermanager/client.go' |
549 | --- state/api/usermanager/client.go 2014-05-23 11:24:54 +0000 |
550 | +++ state/api/usermanager/client.go 2014-06-03 04:22:37 +0000 |
551 | @@ -29,14 +29,15 @@ |
552 | return c.st.Close() |
553 | } |
554 | |
555 | -func (c *Client) AddUser(tag, password string) error { |
556 | - if !names.IsUser(tag) { |
557 | - return fmt.Errorf("invalid user name %q", tag) |
558 | - } |
559 | - u := params.EntityPassword{Tag: tag, Password: password} |
560 | - p := params.EntityPasswords{Changes: []params.EntityPassword{u}} |
561 | +func (c *Client) AddUser(username, displayName, password string) error { |
562 | + if !names.IsUser(username) { |
563 | + return fmt.Errorf("invalid user name %q", username) |
564 | + } |
565 | + userArgs := params.ModifyUsers{ |
566 | + Changes: []params.ModifyUser{{Username: username, DisplayName: displayName, Password: password}}, |
567 | + } |
568 | results := new(params.ErrorResults) |
569 | - err := c.call("AddUser", p, results) |
570 | + err := c.call("AddUser", userArgs, results) |
571 | if err != nil { |
572 | return err |
573 | } |
574 | |
575 | === modified file 'state/api/usermanager/client_test.go' |
576 | --- state/api/usermanager/client_test.go 2014-05-23 11:24:54 +0000 |
577 | +++ state/api/usermanager/client_test.go 2014-06-03 04:22:37 +0000 |
578 | @@ -8,6 +8,7 @@ |
579 | |
580 | jujutesting "launchpad.net/juju-core/juju/testing" |
581 | "launchpad.net/juju-core/state" |
582 | + "launchpad.net/juju-core/state/api/params" |
583 | "launchpad.net/juju-core/state/api/usermanager" |
584 | ) |
585 | |
586 | @@ -26,14 +27,26 @@ |
587 | } |
588 | |
589 | func (s *usermanagerSuite) TestAddUser(c *gc.C) { |
590 | - err := s.usermanager.AddUser("foobar", "password") |
591 | + err := s.usermanager.AddUser("foobar", "Foo Bar", "password") |
592 | + c.Assert(err, gc.IsNil) |
593 | + _, err = s.State.User("foobar") |
594 | + c.Assert(err, gc.IsNil) |
595 | +} |
596 | + |
597 | +func (s *usermanagerSuite) TestAddUserOldClient(c *gc.C) { |
598 | + userArgs := params.EntityPasswords{ |
599 | + Changes: []params.EntityPassword{{Tag: "foobar", Password: "password"}}, |
600 | + } |
601 | + results := new(params.ErrorResults) |
602 | + |
603 | + err := s.APIState.Call("UserManager", "", "AddUser", userArgs, results) |
604 | c.Assert(err, gc.IsNil) |
605 | _, err = s.State.User("foobar") |
606 | c.Assert(err, gc.IsNil) |
607 | } |
608 | |
609 | func (s *usermanagerSuite) TestRemoveUser(c *gc.C) { |
610 | - err := s.usermanager.AddUser("foobar", "password") |
611 | + err := s.usermanager.AddUser("foobar", "Foo Bar", "password") |
612 | c.Assert(err, gc.IsNil) |
613 | _, err = s.State.User("foobar") |
614 | c.Assert(err, gc.IsNil) |
615 | @@ -45,12 +58,12 @@ |
616 | } |
617 | |
618 | func (s *usermanagerSuite) TestAddExistingUser(c *gc.C) { |
619 | - err := s.usermanager.AddUser("foobar", "password") |
620 | + err := s.usermanager.AddUser("foobar", "Foo Bar", "password") |
621 | c.Assert(err, gc.IsNil) |
622 | |
623 | // Try adding again |
624 | - err = s.usermanager.AddUser("foobar", "password") |
625 | - c.Assert(err, gc.ErrorMatches, "Failed to create user: user already exists") |
626 | + err = s.usermanager.AddUser("foobar", "Foo Bar", "password") |
627 | + c.Assert(err, gc.ErrorMatches, "failed to create user: user already exists") |
628 | } |
629 | |
630 | func (s *usermanagerSuite) TestCantRemoveAdminUser(c *gc.C) { |
631 | |
632 | === modified file 'state/apiserver/charms_test.go' |
633 | --- state/apiserver/charms_test.go 2014-03-13 23:30:56 +0000 |
634 | +++ state/apiserver/charms_test.go 2014-06-03 04:22:37 +0000 |
635 | @@ -36,12 +36,9 @@ |
636 | |
637 | func (s *authHttpSuite) SetUpTest(c *gc.C) { |
638 | s.JujuConnSuite.SetUpTest(c) |
639 | - password, err := utils.RandomPassword() |
640 | - c.Assert(err, gc.IsNil) |
641 | - user, err := s.State.AddUser("joe", password) |
642 | - c.Assert(err, gc.IsNil) |
643 | + user := s.AddUser(c, "joe") |
644 | s.userTag = user.Tag() |
645 | - s.password = password |
646 | + s.password = "password" |
647 | } |
648 | |
649 | func (s *authHttpSuite) sendRequest(c *gc.C, tag, password, method, uri, contentType string, body io.Reader) (*http.Response, error) { |
650 | |
651 | === modified file 'state/apiserver/client/api_test.go' |
652 | --- state/apiserver/client/api_test.go 2014-05-13 04:30:48 +0000 |
653 | +++ state/apiserver/client/api_test.go 2014-06-03 04:22:37 +0000 |
654 | @@ -277,8 +277,7 @@ |
655 | setDefaultPassword(c, u) |
656 | add(u) |
657 | |
658 | - u, err = s.State.AddUser("other", "password") |
659 | - c.Assert(err, gc.IsNil) |
660 | + u = s.AddUser(c, "other") |
661 | setDefaultPassword(c, u) |
662 | add(u) |
663 | |
664 | |
665 | === modified file 'state/apiserver/client/client_test.go' |
666 | --- state/apiserver/client/client_test.go 2014-05-16 02:43:52 +0000 |
667 | +++ state/apiserver/client/client_test.go 2014-06-03 04:22:37 +0000 |
668 | @@ -27,7 +27,6 @@ |
669 | "launchpad.net/juju-core/state" |
670 | "launchpad.net/juju-core/state/api" |
671 | "launchpad.net/juju-core/state/api/params" |
672 | - "launchpad.net/juju-core/state/api/usermanager" |
673 | "launchpad.net/juju-core/state/apiserver/client" |
674 | "launchpad.net/juju-core/state/presence" |
675 | coretesting "launchpad.net/juju-core/testing" |
676 | @@ -834,12 +833,10 @@ |
677 | defer restore() |
678 | curl, _ := addCharm(c, store, "dummy") |
679 | |
680 | - usermanager := usermanager.NewClient(s.APIState) |
681 | - err := usermanager.AddUser("foobar", "password") |
682 | - c.Assert(err, gc.IsNil) |
683 | + s.AddUser(c, "foobar") |
684 | s.APIState = s.OpenAPIAs(c, "user-foobar", "password") |
685 | |
686 | - err = s.APIState.Client().ServiceDeploy( |
687 | + err := s.APIState.Client().ServiceDeploy( |
688 | curl.String(), "service", 3, "", constraints.Value{}, "", |
689 | ) |
690 | c.Assert(err, gc.IsNil) |
691 | |
692 | === modified file 'state/apiserver/login_test.go' |
693 | --- state/apiserver/login_test.go 2014-04-17 13:14:49 +0000 |
694 | +++ state/apiserver/login_test.go 2014-06-03 04:22:37 +0000 |
695 | @@ -130,8 +130,7 @@ |
696 | st, err := api.Open(info, fastDialOpts) |
697 | c.Assert(err, gc.IsNil) |
698 | defer st.Close() |
699 | - u, err := s.State.AddUser("inactive", "password") |
700 | - c.Assert(err, gc.IsNil) |
701 | + u := s.AddUser(c, "inactive") |
702 | err = u.Deactivate() |
703 | c.Assert(err, gc.IsNil) |
704 | |
705 | |
706 | === modified file 'state/apiserver/usermanager/usermanager.go' |
707 | --- state/apiserver/usermanager/usermanager.go 2014-04-25 13:57:06 +0000 |
708 | +++ state/apiserver/usermanager/usermanager.go 2014-06-03 04:22:37 +0000 |
709 | @@ -6,6 +6,7 @@ |
710 | import ( |
711 | "fmt" |
712 | |
713 | + "github.com/juju/errors" |
714 | "github.com/juju/loggo" |
715 | |
716 | "launchpad.net/juju-core/state" |
717 | @@ -17,7 +18,7 @@ |
718 | |
719 | // UserManager defines the methods on the usermanager API end point. |
720 | type UserManager interface { |
721 | - AddUser(arg params.EntityPasswords) (params.ErrorResults, error) |
722 | + AddUser(arg params.ModifyUsers) (params.ErrorResults, error) |
723 | RemoveUser(arg params.Entities) (params.ErrorResults, error) |
724 | } |
725 | |
726 | @@ -48,7 +49,7 @@ |
727 | nil |
728 | } |
729 | |
730 | -func (api *UserManagerAPI) AddUser(args params.EntityPasswords) (params.ErrorResults, error) { |
731 | +func (api *UserManagerAPI) AddUser(args params.ModifyUsers) (params.ErrorResults, error) { |
732 | result := params.ErrorResults{ |
733 | Results: make([]params.ErrorResult, len(args.Changes)), |
734 | } |
735 | @@ -65,9 +66,13 @@ |
736 | result.Results[0].Error = common.ServerError(common.ErrPerm) |
737 | continue |
738 | } |
739 | - _, err := api.state.AddUser(arg.Tag, arg.Password) |
740 | + username := arg.Username |
741 | + if username == "" { |
742 | + username = arg.Tag |
743 | + } |
744 | + _, err := api.state.AddUser(username, arg.DisplayName, arg.Password) |
745 | if err != nil { |
746 | - err = fmt.Errorf("Failed to create user: %v", err) |
747 | + err = errors.Annotate(err, "failed to create user") |
748 | result.Results[i].Error = common.ServerError(err) |
749 | continue |
750 | } |
751 | |
752 | === modified file 'state/apiserver/usermanager/usermanager_test.go' |
753 | --- state/apiserver/usermanager/usermanager_test.go 2014-04-17 12:47:50 +0000 |
754 | +++ state/apiserver/usermanager/usermanager_test.go 2014-06-03 04:22:37 +0000 |
755 | @@ -44,32 +44,36 @@ |
756 | } |
757 | |
758 | func (s *userManagerSuite) TestAddUser(c *gc.C) { |
759 | - arg := params.EntityPassword{ |
760 | - Tag: "foobar", |
761 | - Password: "password", |
762 | - } |
763 | - |
764 | - args := params.EntityPasswords{Changes: []params.EntityPassword{arg}} |
765 | + args := params.ModifyUsers{ |
766 | + Changes: []params.ModifyUser{{ |
767 | + Username: "foobar", |
768 | + DisplayName: "Foo Bar", |
769 | + Password: "password", |
770 | + }}} |
771 | |
772 | result, err := s.usermanager.AddUser(args) |
773 | // Check that the call is succesful |
774 | c.Assert(err, gc.IsNil) |
775 | - c.Assert(result, gc.DeepEquals, params.ErrorResults{Results: []params.ErrorResult{params.ErrorResult{Error: nil}}}) |
776 | + c.Assert(result.Results, gc.HasLen, 1) |
777 | + c.Assert(result.Results[0], gc.DeepEquals, params.ErrorResult{Error: nil}) |
778 | // Check that the call results in a new user being created |
779 | user, err := s.State.User("foobar") |
780 | c.Assert(err, gc.IsNil) |
781 | c.Assert(user, gc.NotNil) |
782 | + c.Assert(user.Name(), gc.Equals, "foobar") |
783 | + c.Assert(user.DisplayName(), gc.Equals, "Foo Bar") |
784 | } |
785 | |
786 | func (s *userManagerSuite) TestRemoveUser(c *gc.C) { |
787 | - arg := params.EntityPassword{ |
788 | - Tag: "foobar", |
789 | - Password: "password", |
790 | - } |
791 | + args := params.ModifyUsers{ |
792 | + Changes: []params.ModifyUser{{ |
793 | + Username: "foobar", |
794 | + DisplayName: "Foo Bar", |
795 | + Password: "password", |
796 | + }}} |
797 | removeArg := params.Entity{ |
798 | Tag: "foobar", |
799 | } |
800 | - args := params.EntityPasswords{Changes: []params.EntityPassword{arg}} |
801 | removeArgs := params.Entities{Entities: []params.Entity{removeArg}} |
802 | _, err := s.usermanager.AddUser(args) |
803 | c.Assert(err, gc.IsNil) |
804 | @@ -83,21 +87,22 @@ |
805 | c.Assert(err, gc.IsNil) |
806 | // Removal makes the user in active |
807 | c.Assert(user.IsDeactivated(), gc.Equals, true) |
808 | - c.Assert(user.PasswordValid(arg.Password), gc.Equals, false) |
809 | + c.Assert(user.PasswordValid(args.Changes[0].Password), gc.Equals, false) |
810 | } |
811 | |
812 | // Since removing a user just deacitvates them you cannot add a user |
813 | // that has been previously been removed |
814 | // TODO(mattyw) 2014-03-07 bug #1288745 |
815 | func (s *userManagerSuite) TestCannotAddRemoveAdd(c *gc.C) { |
816 | - arg := params.EntityPassword{ |
817 | - Tag: "addremove", |
818 | - Password: "password", |
819 | - } |
820 | removeArg := params.Entity{ |
821 | Tag: "foobar", |
822 | } |
823 | - args := params.EntityPasswords{Changes: []params.EntityPassword{arg}} |
824 | + args := params.ModifyUsers{ |
825 | + Changes: []params.ModifyUser{{ |
826 | + Username: "foobar", |
827 | + DisplayName: "Foo Bar", |
828 | + Password: "password", |
829 | + }}} |
830 | removeArgs := params.Entities{Entities: []params.Entity{removeArg}} |
831 | _, err := s.usermanager.AddUser(args) |
832 | c.Assert(err, gc.IsNil) |
833 | @@ -106,7 +111,7 @@ |
834 | c.Assert(err, gc.IsNil) |
835 | _, err = s.State.User("addremove") |
836 | result, err := s.usermanager.AddUser(args) |
837 | - expectedError := apiservertesting.ServerError("Failed to create user: user already exists") |
838 | + expectedError := apiservertesting.ServerError("failed to create user: user already exists") |
839 | c.Assert(result, gc.DeepEquals, params.ErrorResults{ |
840 | Results: []params.ErrorResult{ |
841 | params.ErrorResult{expectedError}}}) |
842 | |
843 | === modified file 'state/compat_test.go' |
844 | --- state/compat_test.go 2014-05-20 04:27:02 +0000 |
845 | +++ state/compat_test.go 2014-06-03 04:22:37 +0000 |
846 | @@ -68,7 +68,7 @@ |
847 | } |
848 | |
849 | func (s *compatSuite) TestGetServiceWithoutNetworksIsOK(c *gc.C) { |
850 | - _, err := s.state.AddUser(AdminUser, "pass") |
851 | + _, err := s.state.AddUser(AdminUser, "", "pass") |
852 | c.Assert(err, gc.IsNil) |
853 | charm := addCharm(c, s.state, "quantal", testing.Charms.Dir("mysql")) |
854 | service, err := s.state.AddService("mysql", "user-admin", charm, nil, nil) |
855 | |
856 | === modified file 'state/conn_test.go' |
857 | --- state/conn_test.go 2014-05-27 02:47:01 +0000 |
858 | +++ state/conn_test.go 2014-06-03 04:22:37 +0000 |
859 | @@ -59,7 +59,7 @@ |
860 | cs.services = cs.MgoSuite.Session.DB("juju").C("services") |
861 | cs.units = cs.MgoSuite.Session.DB("juju").C("units") |
862 | cs.stateServers = cs.MgoSuite.Session.DB("juju").C("stateServers") |
863 | - cs.State.AddUser(state.AdminUser, "pass") |
864 | + cs.State.AddUser(state.AdminUser, "", "pass") |
865 | } |
866 | |
867 | func (cs *ConnSuite) TearDownTest(c *gc.C) { |
868 | |
869 | === modified file 'state/megawatcher_internal_test.go' |
870 | --- state/megawatcher_internal_test.go 2014-05-20 04:27:02 +0000 |
871 | +++ state/megawatcher_internal_test.go 2014-06-03 04:22:37 +0000 |
872 | @@ -47,7 +47,7 @@ |
873 | s.BaseSuite.SetUpTest(c) |
874 | s.MgoSuite.SetUpTest(c) |
875 | s.State = TestingInitialize(c, nil, Policy(nil)) |
876 | - s.State.AddUser(AdminUser, "pass") |
877 | + s.State.AddUser(AdminUser, "", "pass") |
878 | } |
879 | |
880 | func (s *storeManagerStateSuite) TearDownTest(c *gc.C) { |
881 | |
882 | === modified file 'state/state_test.go' |
883 | --- state/state_test.go 2014-05-27 06:12:16 +0000 |
884 | +++ state/state_test.go 2014-06-03 04:22:37 +0000 |
885 | @@ -2372,7 +2372,7 @@ |
886 | svc := s.AddTestingService(c, "ser-vice2", s.AddTestingCharm(c, "mysql")) |
887 | _, err = svc.AddUnit() |
888 | c.Assert(err, gc.IsNil) |
889 | - _, err = s.State.AddUser("arble", "pass") |
890 | + _, err = s.State.AddUser("arble", "", "pass") |
891 | c.Assert(err, gc.IsNil) |
892 | s.AddTestingService(c, "wordpress", s.AddTestingCharm(c, "wordpress")) |
893 | eps, err := s.State.InferEndpoints([]string{"wordpress", "ser-vice2"}) |
894 | @@ -2464,7 +2464,7 @@ |
895 | c.Assert(err, gc.IsNil) |
896 | |
897 | // Parse a user entity name. |
898 | - user, err := s.State.AddUser("arble", "pass") |
899 | + user, err := s.State.AddUser("arble", "", "pass") |
900 | c.Assert(err, gc.IsNil) |
901 | coll, id, err = state.ParseTag(s.State, user.Tag()) |
902 | c.Assert(coll, gc.Equals, "users") |
903 | |
904 | === modified file 'state/user.go' |
905 | --- state/user.go 2014-05-21 03:07:36 +0000 |
906 | +++ state/user.go 2014-06-03 04:22:37 +0000 |
907 | @@ -22,9 +22,9 @@ |
908 | } |
909 | |
910 | // AddUser adds a user to the state. |
911 | -func (st *State) AddUser(name, password string) (*User, error) { |
912 | - if !names.IsUser(name) { |
913 | - return nil, fmt.Errorf("invalid user name %q", name) |
914 | +func (st *State) AddUser(username, displayName, password string) (*User, error) { |
915 | + if !names.IsUser(username) { |
916 | + return nil, errors.Errorf("invalid user name %q", username) |
917 | } |
918 | salt, err := utils.RandomSalt() |
919 | if err != nil { |
920 | @@ -33,23 +33,24 @@ |
921 | u := &User{ |
922 | st: st, |
923 | doc: userDoc{ |
924 | - Name: name, |
925 | + Name: username, |
926 | + DisplayName: displayName, |
927 | PasswordHash: utils.UserPasswordHash(password, salt), |
928 | PasswordSalt: salt, |
929 | }, |
930 | } |
931 | ops := []txn.Op{{ |
932 | C: st.users.Name, |
933 | - Id: name, |
934 | + Id: username, |
935 | Assert: txn.DocMissing, |
936 | Insert: &u.doc, |
937 | }} |
938 | err = st.runTransaction(ops) |
939 | if err == txn.ErrAborted { |
940 | - err = fmt.Errorf("user already exists") |
941 | + err = errors.New("user already exists") |
942 | } |
943 | if err != nil { |
944 | - return nil, err |
945 | + return nil, errors.Trace(err) |
946 | } |
947 | return u, nil |
948 | } |
949 | @@ -68,7 +69,7 @@ |
950 | func (st *State) User(name string) (*User, error) { |
951 | u := &User{st: st} |
952 | if err := st.getUser(name, &u.doc); err != nil { |
953 | - return nil, err |
954 | + return nil, errors.Trace(err) |
955 | } |
956 | return u, nil |
957 | } |
958 | @@ -81,7 +82,8 @@ |
959 | |
960 | type userDoc struct { |
961 | Name string `bson:"_id_"` |
962 | - Deactivated bool // Removing users means they still exist, but are marked deactivated |
963 | + DisplayName string |
964 | + Deactivated bool // Removing users means they still exist, but are marked deactivated |
965 | PasswordHash string |
966 | PasswordSalt string |
967 | } |
968 | @@ -91,6 +93,11 @@ |
969 | return u.doc.Name |
970 | } |
971 | |
972 | +// DisplayName returns the display name of the user. |
973 | +func (u *User) DisplayName() string { |
974 | + return u.doc.DisplayName |
975 | +} |
976 | + |
977 | // Tag returns the Tag for |
978 | // the user ("user-$username") |
979 | func (u *User) Tag() string { |
980 | |
981 | === modified file 'state/user_test.go' |
982 | --- state/user_test.go 2014-05-22 00:48:08 +0000 |
983 | +++ state/user_test.go 2014-06-03 04:22:37 +0000 |
984 | @@ -17,67 +17,69 @@ |
985 | ConnSuite |
986 | } |
987 | |
988 | -var _ = gc.Suite(&UserSuite{}) |
989 | +var ( |
990 | + _ = gc.Suite(&UserSuite{}) |
991 | +) |
992 | |
993 | func (s *UserSuite) TestAddUserInvalidNames(c *gc.C) { |
994 | for _, name := range []string{ |
995 | "", |
996 | "b^b", |
997 | } { |
998 | - u, err := s.State.AddUser(name, "password") |
999 | + u, err := s.State.AddUser(name, "ignored", "ignored") |
1000 | c.Assert(err, gc.ErrorMatches, `invalid user name "`+regexp.QuoteMeta(name)+`"`) |
1001 | c.Assert(u, gc.IsNil) |
1002 | } |
1003 | } |
1004 | |
1005 | -func (s *UserSuite) TestAddUserValidName(c *gc.C) { |
1006 | +func (s *UserSuite) addUser(c *gc.C, name, displayName, password string) *state.User { |
1007 | + user, err := s.State.AddUser(name, displayName, password) |
1008 | + c.Assert(err, gc.IsNil) |
1009 | + c.Assert(user, gc.NotNil) |
1010 | + c.Assert(user.Name(), gc.Equals, name) |
1011 | + c.Assert(user.DisplayName(), gc.Equals, displayName) |
1012 | + c.Assert(user.PasswordValid(password), jc.IsTrue) |
1013 | + return user |
1014 | +} |
1015 | + |
1016 | +func (s *UserSuite) makeUser(c *gc.C) *state.User { |
1017 | + return s.addUser(c, "someuser", "displayName", "a-password") |
1018 | +} |
1019 | + |
1020 | +func (s *UserSuite) TestAddUser(c *gc.C) { |
1021 | name := "f00-Bar.ram77" |
1022 | - u, err := s.State.AddUser(name, "password") |
1023 | - c.Check(u, gc.NotNil) |
1024 | - c.Assert(err, gc.IsNil) |
1025 | - c.Assert(u.Name(), gc.Equals, name) |
1026 | -} |
1027 | - |
1028 | -func (s *UserSuite) TestAddUser(c *gc.C) { |
1029 | - u, err := s.State.AddUser("aa", "b") |
1030 | - c.Check(u, gc.NotNil) |
1031 | - c.Assert(err, gc.IsNil) |
1032 | - |
1033 | - c.Assert(u.Name(), gc.Equals, "aa") |
1034 | - c.Assert(u.PasswordValid("b"), jc.IsTrue) |
1035 | - |
1036 | - u1, err := s.State.User("aa") |
1037 | - c.Check(u1, gc.NotNil) |
1038 | - c.Assert(err, gc.IsNil) |
1039 | - |
1040 | - c.Assert(u1.Name(), gc.Equals, "aa") |
1041 | - c.Assert(u1.PasswordValid("b"), jc.IsTrue) |
1042 | + displayName := "Display" |
1043 | + password := "password" |
1044 | + |
1045 | + s.addUser(c, name, displayName, password) |
1046 | + |
1047 | + user, err := s.State.User(name) |
1048 | + c.Assert(err, gc.IsNil) |
1049 | + c.Check(user, gc.NotNil) |
1050 | + c.Assert(user.Name(), gc.Equals, name) |
1051 | + c.Assert(user.DisplayName(), gc.Equals, displayName) |
1052 | + c.Assert(user.PasswordValid(password), jc.IsTrue) |
1053 | } |
1054 | |
1055 | func (s *UserSuite) TestCheckUserExists(c *gc.C) { |
1056 | - u, err := s.State.AddUser("aa", "b") |
1057 | - c.Check(u, gc.NotNil) |
1058 | - c.Assert(err, gc.IsNil) |
1059 | - e, err := state.CheckUserExists(s.State, "aa") |
1060 | - c.Assert(err, gc.IsNil) |
1061 | - c.Assert(e, gc.Equals, true) |
1062 | - e, err = state.CheckUserExists(s.State, "notAUser") |
1063 | - c.Assert(err, gc.IsNil) |
1064 | - c.Assert(e, gc.Equals, false) |
1065 | + user := s.makeUser(c) |
1066 | + exists, err := state.CheckUserExists(s.State, user.Name()) |
1067 | + c.Assert(err, gc.IsNil) |
1068 | + c.Assert(exists, jc.IsTrue) |
1069 | + exists, err = state.CheckUserExists(s.State, "notAUser") |
1070 | + c.Assert(err, gc.IsNil) |
1071 | + c.Assert(exists, jc.IsFalse) |
1072 | } |
1073 | |
1074 | func (s *UserSuite) TestSetPassword(c *gc.C) { |
1075 | - u, err := s.State.AddUser("someuser", "password") |
1076 | - c.Assert(err, gc.IsNil) |
1077 | - |
1078 | + user := s.makeUser(c) |
1079 | testSetPassword(c, func() (state.Authenticator, error) { |
1080 | - return s.State.User(u.Name()) |
1081 | + return s.State.User(user.Name()) |
1082 | }) |
1083 | } |
1084 | |
1085 | func (s *UserSuite) TestAddUserSetsSalt(c *gc.C) { |
1086 | - u, err := s.State.AddUser("someuser", "a-password") |
1087 | - c.Assert(err, gc.IsNil) |
1088 | + u := s.makeUser(c) |
1089 | salt, hash := state.GetUserPasswordSaltAndHash(u) |
1090 | c.Check(hash, gc.Not(gc.Equals), "") |
1091 | c.Check(salt, gc.Not(gc.Equals), "") |
1092 | @@ -86,8 +88,7 @@ |
1093 | } |
1094 | |
1095 | func (s *UserSuite) TestSetPasswordChangesSalt(c *gc.C) { |
1096 | - u, err := s.State.AddUser("someuser", "a-password") |
1097 | - c.Assert(err, gc.IsNil) |
1098 | + u := s.makeUser(c) |
1099 | origSalt, origHash := state.GetUserPasswordSaltAndHash(u) |
1100 | c.Check(origSalt, gc.Not(gc.Equals), "") |
1101 | // Even though the password is the same, we take this opportunity to |
1102 | @@ -101,10 +102,9 @@ |
1103 | } |
1104 | |
1105 | func (s *UserSuite) TestSetPasswordHash(c *gc.C) { |
1106 | - u, err := s.State.AddUser("someuser", "password") |
1107 | - c.Assert(err, gc.IsNil) |
1108 | + u := s.makeUser(c) |
1109 | |
1110 | - err = u.SetPasswordHash(utils.UserPasswordHash("foo", utils.CompatSalt), utils.CompatSalt) |
1111 | + err := u.SetPasswordHash(utils.UserPasswordHash("foo", utils.CompatSalt), utils.CompatSalt) |
1112 | c.Assert(err, gc.IsNil) |
1113 | |
1114 | c.Assert(u.PasswordValid("foo"), jc.IsTrue) |
1115 | @@ -120,10 +120,9 @@ |
1116 | } |
1117 | |
1118 | func (s *UserSuite) TestSetPasswordHashWithSalt(c *gc.C) { |
1119 | - u, err := s.State.AddUser("someuser", "password") |
1120 | - c.Assert(err, gc.IsNil) |
1121 | + u := s.makeUser(c) |
1122 | |
1123 | - err = u.SetPasswordHash(utils.UserPasswordHash("foo", "salted"), "salted") |
1124 | + err := u.SetPasswordHash(utils.UserPasswordHash("foo", "salted"), "salted") |
1125 | c.Assert(err, gc.IsNil) |
1126 | |
1127 | c.Assert(u.PasswordValid("foo"), jc.IsTrue) |
1128 | @@ -133,11 +132,10 @@ |
1129 | } |
1130 | |
1131 | func (s *UserSuite) TestPasswordValidUpdatesSalt(c *gc.C) { |
1132 | - u, err := s.State.AddUser("someuser", "password") |
1133 | - c.Assert(err, gc.IsNil) |
1134 | + u := s.makeUser(c) |
1135 | |
1136 | compatHash := utils.UserPasswordHash("foo", utils.CompatSalt) |
1137 | - err = u.SetPasswordHash(compatHash, "") |
1138 | + err := u.SetPasswordHash(compatHash, "") |
1139 | c.Assert(err, gc.IsNil) |
1140 | beforeSalt, beforeHash := state.GetUserPasswordSaltAndHash(u) |
1141 | c.Assert(beforeSalt, gc.Equals, "") |
1142 | @@ -160,20 +158,11 @@ |
1143 | c.Assert(lastHash, gc.Equals, afterHash) |
1144 | } |
1145 | |
1146 | -func (s *UserSuite) TestName(c *gc.C) { |
1147 | - u, err := s.State.AddUser("someuser", "password") |
1148 | - c.Assert(err, gc.IsNil) |
1149 | - |
1150 | - c.Assert(u.Name(), gc.Equals, "someuser") |
1151 | - c.Assert(u.Tag(), gc.Equals, "user-someuser") |
1152 | -} |
1153 | - |
1154 | func (s *UserSuite) TestDeactivate(c *gc.C) { |
1155 | - u, err := s.State.AddUser("someuser", "password") |
1156 | - c.Assert(err, gc.IsNil) |
1157 | + u := s.makeUser(c) |
1158 | c.Assert(u.IsDeactivated(), gc.Equals, false) |
1159 | |
1160 | - err = u.Deactivate() |
1161 | + err := u.Deactivate() |
1162 | c.Assert(err, gc.IsNil) |
1163 | c.Assert(u.IsDeactivated(), gc.Equals, true) |
1164 | c.Assert(u.PasswordValid(""), gc.Equals, false) |
Reviewers: mp+221823_ code.launchpad. net,
Message:
Please take a look.
Description:
Add a display name option for new users
This ended up being more work than I hoped as I refactored the apiserver
parts, many tests, and made sure there was backwards compatability with
older client apis.
Also changed how the CLI command was tested so it didn't need a
JujuConnSuite.
https:/ /code.launchpad .net/~thumper/ juju-core/ user-display- name/+merge/ 221823
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/102970043/
Affected files (+353, -240 lines): authorizedkeys_ test.go user_add. go user_add_ test.go conn.go dummy/environs. go params/ params. go usermanager/ client. go usermanager/ client_ test.go /charms_ test.go /client/ api_test. go /client/ client_ test.go /login_ test.go /usermanager/ usermanager. go /usermanager/ usermanager_ test.go test.go er_internal_ test.go
A [revision details]
M agent/bootstrap.go
M cmd/juju/
M cmd/juju/
M cmd/juju/
M juju/conn_test.go
M juju/testing/
M provider/
M state/api/
M state/api/
M state/api/
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/apiserver
M state/compat_
M state/conn_test.go
M state/megawatch
M state/state_test.go
M state/user.go
M state/user_test.go