New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reset password, state layer. #7740
Conversation
state/user.go
Outdated
@@ -543,6 +540,61 @@ func (u *User) ensureNotDeleted() error { | |||
return nil | |||
} | |||
|
|||
// ResetPassword cleans up password related field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResetPassword clears the user's password (if there is one), and generates a new secret key for the user.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
return nil, errors.Trace(err) | ||
} | ||
if u.IsDisabled() { | ||
return nil, fmt.Errorf("user deactivated") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cannot reset password for disabled user
?
or maybe add a DeferredAnnotate at the top of the function instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not convinced that this needs further clarification. The overall error that will come from here will be cannot reset password for user "bob": user deactivated
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I missed the fact that there's additional context below.
if err := u.st.db().Run(buildTxn); err != nil { | ||
return nil, errors.Annotatef(err, "cannot reset password for user %q", u.Name()) | ||
} | ||
u.doc.SecretKey = key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
u.doc.PasswordHash = ""
u.doc.PasswordSalt = ""
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point! done :D
state/user.go
Outdated
return key, nil | ||
} | ||
|
||
// generateSecretKey generates a random, 32-byte secret key. This can be used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you you please delete the bit about the CA cert? we're not using it for that. not sure why it was in there before, probably copy pasta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
||
key, err := u.ResetPassword() | ||
c.Assert(err, jc.ErrorIsNil) | ||
c.Assert(u.SecretKey(), gc.DeepEquals, key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also check that PasswordValid("anything") fails here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added check.
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Description of change
This is part of the functionality to provide controller admins with ability to reset user passwords/re-issue login tokens.
QA steps
This is only one the layers, there is no full-stack funcationaliy yet. All unit tests pass.
Documentation changes
n/a
This is an internal addition atm.
Bug reference
Initial part for https://bugs.launchpad.net/juju/+bug/1657187