feat(cli): add server user set-password-hash command #3974
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3974 +/- ##
==========================================
+ Coverage 75.86% 77.19% +1.32%
==========================================
Files 470 483 +13
Lines 37301 28848 -8453
==========================================
- Hits 28299 22269 -6030
+ Misses 7071 4674 -2397
+ Partials 1931 1905 -26 ☔ View full report in Codecov by Sentry. |
b64c170 to
bd27df2
Compare
bd27df2 to
628044e
Compare
redgoat650
left a comment
There was a problem hiding this comment.
A few nits, but looks good overall
internal/user/user_manager.go
Outdated
| var ErrUserNotFound = errors.New("user not found") | ||
|
|
||
| // ErrUserAlreadyExists indicates that a user already exist in the system when attempting to create a new one. | ||
| var ErrUserAlreadyExists = errors.New("user already exists found") |
There was a problem hiding this comment.
Was this meant to be just "user already exists"?
("already exists found" sounds kinda weird)
There was a problem hiding this comment.
yeap, probably edit error.
There was a problem hiding this comment.
or "found an already existing user" 😄 (JK, "user already exists" it is)
| // create a new user with and set the password using the password hash | ||
| e.RunAndExpectSuccess(t, "server", "users", "add", userFull, "--user-password-hash", passwordHash) |
There was a problem hiding this comment.
Do we want to test server users set also? Perhaps we can update the password hash and show fail/success when the same user connects with old/new passwords?
There was a problem hiding this comment.
Good suggestion.
Let me try, although it may require restarting the server or forcing/waiting for a refresh.
There was a problem hiding this comment.
@redgoat650 PTAL.
I added a success case after changing the user password (via a hash).
I'll owe the failure cases, those surfaced a minor issue with a resource not being released when the client command fails to execute. It may be similar to the issue in #3980 but on the client side. 3980 is on the server side, it was surfaced when fixing a test.
I'll address the "resource leakage" issue separately, and then I'll update this test, probably in a separate PR.
Includes tests for `user.HashPassword`
Test rountrip with `decodeHashedPassword` Test `user.passwordHash.validate()`
…-hash' CLI test for `server user hash-password` and `server user add/set --user-password-hash`
This helper encapsulates how a user profile is initialized. This way, internals of the user package do not need to be exposed.
Removes unused: - `user.DefaultPasswordHashingAlgorithm` const - `user.PasswordHashingAlgorithms()` helper
Extends the test to change the user password hash and connect to the server with the new password.
628044e to
6b36d55
Compare
redgoat650
left a comment
There was a problem hiding this comment.
Thanks @julio-lopez, +1 from me
|
@bathina2 FYI |
|
|
Objectives:
server user --user-passwordCLI command.userpackage.Adds a new
server user hash-passwordCLI command to generate the hash from a supplied password.Modifies the
server user set/add --user-password-hashCLI command to accept the password hash generated using thehash-passwordcommand.Adds
GetNewProfile(ctx, rep, username)helper to move implementation details to theuserpackage.Includes CLI and unit tests.
Cleans up and removes unused functions.