Reset user password, cli. #7754
Conversation
cmd/juju/user/change_password.go
Outdated
| A controller administrator can change the password for another user (on | ||
| that controller). | ||
| A controller administrator can change the password for another user | ||
| on the specified controller. If no controller is specified, current controller |
There was a problem hiding this comment.
"the" current controller is used.
cmd/juju/user/change_password.go
Outdated
| on the specified controller. If no controller is specified, current controller | ||
| will be used. | ||
|
|
||
| A controller administrator can also reset the password for another user. |
There was a problem hiding this comment.
for any user is probably how I would phrase it. It feels weird to have 2 paragraphs starting with the same sentence.
There was a problem hiding this comment.
The sentence is not the same :D
First one says "..can change...", Second one says ".. can also reset.."
cmd/juju/user/change_password.go
Outdated
| A controller administrator can also reset the password for another user. | ||
| This will invalidate previously set user password (if any) or | ||
| previously issued registration tokens. | ||
| Succefull password reset will result in a new registration token. |
There was a problem hiding this comment.
This will invalidate any password or registration string that was previously issued, and issue a new registration string to be used with juju register.
(or Successful if we want to keep that word)
cmd/juju/user/change_password.go
Outdated
| controllerName, err := c.ControllerName() | ||
| if err != nil { | ||
| return errors.Trace(err) | ||
| } | ||
| store := c.ClientStore() | ||
| accountDetails, err := store.AccountDetails(controllerName) | ||
| c.controllerName = controllerName |
There was a problem hiding this comment.
This feels odd to have a "c.ControllerName()" that doesn't set c.controllerName, and then a side effect of prepareRun that does.
Maybe we need
func (changePasswordCommand) EnsureControllerName() error {
}
after which we are sure that c.controllerName is valid?
There was a problem hiding this comment.
Sure as previously this was all part of Run, I guess it did not matter. I'll refactor into a separate method for maintainability.
cmd/juju/user/change_password.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (c *changePasswordCommand) changeOrResetString(past ...bool) string { |
There was a problem hiding this comment.
I don't think the ...bool aids clarity. (what could it mean to pass 2 booleans).
Just passing an always-required true/false is sufficient.
I'd also call the variable "pastTense"
cmd/juju/user/change_password.go
Outdated
| if err != nil { | ||
| return errors.Annotate(err, "generating controller user access token") | ||
| } | ||
| fmt.Fprintf(ctx.Stdout, "New controller access token for this user is %v\n", base64RegistrationData) |
There was a problem hiding this comment.
I believe for "add-user" we just generate the full CLI to give to them, so more:
Ask the user to run:
juju register BASE64STRING+OETUH
I'd like us to be consistent between the two, and giving the full CLI means they don't need to figure out what command that string is used for.
cmd/juju/user/change_password.go
Outdated
| ctx.Infof("Password for %q has been updated.", c.User) | ||
| func (c *changePasswordCommand) updateClientStore(ctx *cmd.Context, password string) error { | ||
| if c.accountDetails == nil { | ||
| ctx.Infof("Password for %q has been %v.", c.User, c.changeOrResetString(true)) |
There was a problem hiding this comment.
Its often a bit cleaner to do:
if c.accountDetails != nil {
//report
return nil
}
// Then the rest of the function doesn't have to be indented.
cmd/juju/user/change_password.go
Outdated
| if err := store.UpdateAccount(controllerName, *accountDetails); err != nil { | ||
| return errors.Annotate(err, "failed to update client credentials") | ||
| if c.accountDetails.Password != "" { | ||
| if password != "" { |
There was a problem hiding this comment.
Similarly here, you can invert these checks and just return 'nil' rather than getting deeper-and-deeper nesting.
| context, _, err := s.run(c, "--reset") | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| s.mockAPI.CheckCall(c, 0, "ResetPassword", "current-user") | ||
| c.Assert(cmdtesting.Stdout(context), gc.Matches, ` |
There was a problem hiding this comment.
It feels odd to have some of the content on stdout and some on stderr.
I understand the rationale (Your password has been reset being more of an informative messaage).
It would be nice to know that "You password has been reset" gets printed before the other.
There was a problem hiding this comment.
well, i think the ourder of stderr/stdout depends on how stderr and stdout is configured on the client side?
The best I can do code-wise is either put both as ctx.Infof (which currently goes to stderr) or both on stdout explicitly.
|
!!build!! failed to build? |
cmd/juju/user/change_password.go
Outdated
| if c.accountDetails.Password != "" { | ||
| macaroonErr = c.ClearControllerMacaroons(c.ClientStore(), c.controllerName) | ||
| if macaroonErr != nil { | ||
| macaroonErr = errors.Annotatef(err, "could not clear macaroon") |
There was a problem hiding this comment.
I do wonder if we want to expose the user to the concept of macaroon. Or if we should just say:
"could not clear local credential cache".
Not a big deal either way, just something to consider.
|
|
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
Description of change
This PR introduces --reset option on change-user-password command that enables controller admins to re-issue a controller access registration key for users.
This functionality can also be used when user forgot their password.
QA steps
Documentation changes
Final, cli layer, for https://bugs.launchpad.net/juju/+bug/1657187
Bug reference
Does this change fix a bug? Please add a link to it.