Merge lp:~thumper/juju-core/user-display-name into lp:~go-bot/juju-core/trunk

Proposed by Tim Penhey
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
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+221823@code.launchpad.net

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.

https://codereview.appspot.com/102970043/

To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

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):
   A [revision details]
   M agent/bootstrap.go
   M cmd/juju/authorizedkeys_test.go
   M cmd/juju/user_add.go
   M cmd/juju/user_add_test.go
   M juju/conn_test.go
   M juju/testing/conn.go
   M provider/dummy/environs.go
   M state/api/params/params.go
   M state/api/usermanager/client.go
   M state/api/usermanager/client_test.go
   M state/apiserver/charms_test.go
   M state/apiserver/client/api_test.go
   M state/apiserver/client/client_test.go
   M state/apiserver/login_test.go
   M state/apiserver/usermanager/usermanager.go
   M state/apiserver/usermanager/usermanager_test.go
   M state/compat_test.go
   M state/conn_test.go
   M state/megawatcher_internal_test.go
   M state/state_test.go
   M state/user.go
   M state/user_test.go

Revision history for this message
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://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go
File cmd/juju/user_add.go (left):

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#oldcode44
cmd/juju/user_add.go:44: Args: "<username> <password>",
Sorry for missing this when I implemented --password

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

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#newcode109
cmd/juju/user_add.go:109: return err
errors.Trace() here too?

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

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go#oldcode37
state/api/usermanager/client.go:37: p := params.EntityPasswords{Changes:
[]params.EntityPassword{u}}
These names were terrible! Glad you've fixed it.

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

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go#newcode37
state/api/usermanager/client.go:37: Changes:
[]params.ModifyUser{{Username: username, DisplayName: displayName,
Password: password}},
This is much more readable.

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

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_test.go#newcode42
state/api/usermanager/client_test.go:42: err :=
s.APIState.Call("UserManager", "", "AddUser", userArgs, results)
Maybe add a comment here about why s.APIState.Call("UserManager",...) is
used here instead of s.usermanager.AddUser()? I understand why but had
to think about it for a bit.

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (left):

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go#oldcode39
state/apiserver/charms_test.go:39: password, err :=
utils.RandomPassword()
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://codereview.appspot.com/102970043/diff/1/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode45
state/user_test.go:45: func (s *UserSuite) makeUser(c *gc.C) *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.

https://codereview.appspot.com/102970043/

Revision history for this message
Jesse Meek (waigani) wrote :
Download full text (3.4 KiB)

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://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go
> File cmd/juju/user_add.go (left):

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#oldcode44
> cmd/juju/user_add.go:44: Args: "<username> <password>",
> Sorry for missing this when I implemented --password

> https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go
> File cmd/juju/user_add.go (right):

https://codereview.appspot.com/102970043/diff/1/cmd/juju/user_add.go#newcode109
> cmd/juju/user_add.go:109: return err
> errors.Trace() here too?

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go
> File state/api/usermanager/client.go (left):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go#oldcode37
> state/api/usermanager/client.go:37: p :=
params.EntityPasswords{Changes:
> []params.EntityPassword{u}}
> These names were terrible! Glad you've fixed it.

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

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client.go#newcode37
> state/api/usermanager/client.go:37: Changes:
[]params.ModifyUser{{Username:
> username, DisplayName: displayName, Password: password}},
> This is much more readable.

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_test.go
> File state/api/usermanager/client_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_test.go#newcode42
> state/api/usermanager/client_test.go:42: err :=
s.APIState.Call("UserManager",
> "", "AddUser", userArgs, results)
> Maybe add a comment here about why s.APIState.Call("UserManager",...)
is used
> here instead of s.usermanager.AddUser()? I understand why but had to
think about
> it for a bit.

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go
> File state/apiserver/charms_test.go (left):

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go#oldcode39
> state/apiserver/charms_test.go:39: password, err :=
utils.RandomPassword()
> 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://codereview.appspot.com/102970043/diff/1/state/user_test.go
> File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode45
> state/user_test.go:45: func (s *UserSuite) makeUser(c *gc.C)
*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 ...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :

https://codereview.appspot.com/102970043/diff/1/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode75
state/user_test.go:75: user := s.makeUser(c)
It seems you are making a user for every test. Why not makeUser in
SetUpTest?

https://codereview.appspot.com/102970043/

Revision history for this message
Tim Penhey (thumper) wrote :

New branch is moving to github.

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

https://codereview.appspot.com/102970043/diff/1/state/api/usermanager/client_test.go#newcode42
state/api/usermanager/client_test.go:42: err :=
s.APIState.Call("UserManager", "", "AddUser", userArgs, results)
On 2014/06/03 05:17:26, menn0 wrote:
> Maybe add a comment here about why s.APIState.Call("UserManager",...)
is used
> here instead of s.usermanager.AddUser()? I understand why but had to
think about
> it for a bit.

Done.

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go
File state/apiserver/charms_test.go (left):

https://codereview.appspot.com/102970043/diff/1/state/apiserver/charms_test.go#oldcode39
state/apiserver/charms_test.go:39: password, err :=
utils.RandomPassword()
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://codereview.appspot.com/102970043/diff/1/state/user_test.go
File state/user_test.go (right):

https://codereview.appspot.com/102970043/diff/1/state/user_test.go#newcode75
state/user_test.go:75: user := s.makeUser(c)
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 :-)

https://codereview.appspot.com/102970043/

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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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)

Subscribers

People subscribed via source and target branches

to status/vote changes: