From 346e5e44629a2c4438cba0ed64fd7734e9569b93 Mon Sep 17 00:00:00 2001 From: Bruno Date: Wed, 4 May 2022 19:47:47 +0000 Subject: [PATCH 01/15] Add UpdateUserHashedPassword query --- coderd/database/queries/users.sql | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 2f0ecb3709b9b..d0c3d4c3e7b66 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -59,6 +59,14 @@ WHERE id = @id RETURNING *; +-- name: UpdateUserHashedPassword :exec +UPDATE + users +SET + hashed_password = $2 +WHERE + id = $1; + -- name: GetUsers :many SELECT * @@ -133,4 +141,4 @@ FROM LEFT JOIN organization_members ON id = user_id WHERE - id = @user_id; \ No newline at end of file + id = @user_id; From de39cf56a5463f84ec910b8ad2faa0380b0d7496 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 12:43:34 +0000 Subject: [PATCH 02/15] Add database functions --- coderd/database/databasefake/databasefake.go | 15 +++++++++++++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 19 +++++++++++++++++++ coderd/database/queries/users.sql | 2 +- 4 files changed, 36 insertions(+), 1 deletion(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 5726c9f7befe5..f33adf922f100 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -1314,6 +1314,21 @@ func (q *fakeQuerier) UpdateUserStatus(_ context.Context, arg database.UpdateUse return database.User{}, sql.ErrNoRows } +func (q *fakeQuerier) UpdateUserHashedPassword(_ context.Context, arg database.UpdateUserHashedPasswordParams) error { + q.mutex.Lock() + defer q.mutex.Unlock() + + for index, user := range q.users { + if user.ID != arg.ID { + continue + } + user.HashedPassword = arg.HashedPassword + q.users[index] = user + return nil + } + return sql.ErrNoRows +} + func (q *fakeQuerier) InsertWorkspace(_ context.Context, arg database.InsertWorkspaceParams) (database.Workspace, error) { q.mutex.Lock() defer q.mutex.Unlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index db2f3f0d49987..dbf6b5cfed8cd 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -100,6 +100,7 @@ type querier interface { UpdateTemplateActiveVersionByID(ctx context.Context, arg UpdateTemplateActiveVersionByIDParams) error UpdateTemplateDeletedByID(ctx context.Context, arg UpdateTemplateDeletedByIDParams) error UpdateTemplateVersionByID(ctx context.Context, arg UpdateTemplateVersionByIDParams) error + UpdateUserHashedPassword(ctx context.Context, arg UpdateUserHashedPasswordParams) error UpdateUserProfile(ctx context.Context, arg UpdateUserProfileParams) (User, error) UpdateUserRoles(ctx context.Context, arg UpdateUserRolesParams) (User, error) UpdateUserStatus(ctx context.Context, arg UpdateUserStatusParams) (User, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index f91070915c097..450d540a192ac 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -2264,6 +2264,25 @@ func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User return i, err } +const updateUserHashedPassword = `-- name: UpdateUserHashedPassword :exec +UPDATE + users +SET + hashed_password = $2 +WHERE + id = $1 +` + +type UpdateUserHashedPasswordParams struct { + ID uuid.UUID `db:"id" json:"id"` + HashedPassword []byte `db:"hashed_password" json:"hashed_password"` +} + +func (q *sqlQuerier) UpdateUserHashedPassword(ctx context.Context, arg UpdateUserHashedPasswordParams) error { + _, err := q.db.ExecContext(ctx, updateUserHashedPassword, arg.ID, arg.HashedPassword) + return err +} + const updateUserProfile = `-- name: UpdateUserProfile :one UPDATE users diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index d0c3d4c3e7b66..35ade0e9762a4 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -70,7 +70,7 @@ WHERE -- name: GetUsers :many SELECT * -FROM +FROM users WHERE CASE From 2fe171618e5698f9f38ba42ffd42965f2d14e911 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 13:10:42 +0000 Subject: [PATCH 03/15] Add update user password endpoint --- coderd/coderd.go | 1 + coderd/users.go | 58 +++++++++++++++++++++++++++++++++++++++++++++++ codersdk/users.go | 6 +++++ 3 files changed, 65 insertions(+) diff --git a/coderd/coderd.go b/coderd/coderd.go index 191d3339aa2b1..a1d028c7295f7 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -240,6 +240,7 @@ func New(options *Options) (http.Handler, func()) { r.Get("/", api.userByName) r.Put("/profile", api.putUserProfile) r.Put("/suspend", api.putUserSuspend) + r.Put("/password", api.putUserPassword) r.Get("/organizations", api.organizationsByUser) r.Post("/organizations", api.postOrganizationsByUser) // These roles apply to the site wide permissions. diff --git a/coderd/users.go b/coderd/users.go index 310bee8e79597..a56602d28ed89 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -360,6 +360,64 @@ func (api *api) putUserSuspend(rw http.ResponseWriter, r *http.Request) { httpapi.Write(rw, http.StatusOK, convertUser(suspendedUser, organizations)) } +func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { + user := httpmw.UserParam(r) + + var params codersdk.UpdateUserHashedPasswordRequest + if !httpapi.Read(rw, r, ¶ms) { + return + } + + // Check if the password is correct + equal, err := userpassword.Compare(string(user.HashedPassword), params.Password) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: fmt.Sprintf("compare: %s", err.Error()), + }) + } + if !equal { + // This message is the same as above to remove ease in detecting whether + // users are registered or not. Attackers still could with a timing attack. + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: "invalid email or password", + }) + return + } + + // Check if the new password and the confirmation match + if params.NewPassword != params.ConfirmNewPassword { + errors := []httpapi.Error{ + { + Field: "confirm_new_password", + Detail: "The value does not match the new password", + }, + } + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: fmt.Sprintf("The new password and the new password confirmation don't match"), + Errors: errors, + }) + return + } + + // Hash password and update it in the database + hashedPassword, hashError := userpassword.Hash(params.NewPassword) + if hashError != nil { + xerrors.Errorf("hash password: %w", hashError) + return + } + databaseError := api.Database.UpdateUserHashedPassword(r.Context(), database.UpdateUserHashedPasswordParams{ + HashedPassword: []byte(hashedPassword), + }) + if databaseError != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: fmt.Sprintf("put user password: %s", err.Error()), + }) + return + } + + httpapi.Write(rw, http.StatusNoContent, nil) +} + func (api *api) userRoles(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) diff --git a/codersdk/users.go b/codersdk/users.go index e2c8261febdec..d7b0566755600 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -72,6 +72,12 @@ type UpdateUserProfileRequest struct { Username string `json:"username" validate:"required,username"` } +type UpdateUserHashedPasswordRequest struct { + Password string `json:"password" validate:"required"` + NewPassword string `json:"new_password" validate:"required"` + ConfirmNewPassword string `json:"confirm_new_password" validate:"required"` +} + type UpdateRoles struct { Roles []string `json:"roles" validate:"required"` } From 212020aaf102abdebca84e7ae8ccd0eb91d34470 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 14:01:01 +0000 Subject: [PATCH 04/15] Add tests and fixes --- coderd/coderdtest/coderdtest.go | 19 ++++++------ coderd/users.go | 6 ++-- coderd/users_test.go | 51 +++++++++++++++++++++++++++++++++ codersdk/users.go | 15 +++++++++- 4 files changed, 78 insertions(+), 13 deletions(-) diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 074767f35920c..e205e272db3ce 100644 --- a/coderd/coderdtest/coderdtest.go +++ b/coderd/coderdtest/coderdtest.go @@ -174,21 +174,22 @@ func NewProvisionerDaemon(t *testing.T, client *codersdk.Client) io.Closer { return closer } +var FirstUserParams = codersdk.CreateFirstUserRequest{ + Email: "testuser@coder.com", + Username: "testuser", + Password: "testpass", + OrganizationName: "testorg", +} + // CreateFirstUser creates a user with preset credentials and authenticates // with the passed in codersdk client. func CreateFirstUser(t *testing.T, client *codersdk.Client) codersdk.CreateFirstUserResponse { - req := codersdk.CreateFirstUserRequest{ - Email: "testuser@coder.com", - Username: "testuser", - Password: "testpass", - OrganizationName: "testorg", - } - resp, err := client.CreateFirstUser(context.Background(), req) + resp, err := client.CreateFirstUser(context.Background(), FirstUserParams) require.NoError(t, err) login, err := client.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ - Email: req.Email, - Password: req.Password, + Email: FirstUserParams.Email, + Password: FirstUserParams.Password, }) require.NoError(t, err) client.SessionToken = login.SessionToken diff --git a/coderd/users.go b/coderd/users.go index a56602d28ed89..7412ef9f9f475 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -363,7 +363,7 @@ func (api *api) putUserSuspend(rw http.ResponseWriter, r *http.Request) { func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) - var params codersdk.UpdateUserHashedPasswordRequest + var params codersdk.UpdateUserPasswordRequest if !httpapi.Read(rw, r, ¶ms) { return } @@ -406,11 +406,12 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { return } databaseError := api.Database.UpdateUserHashedPassword(r.Context(), database.UpdateUserHashedPasswordParams{ + ID: user.ID, HashedPassword: []byte(hashedPassword), }) if databaseError != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("put user password: %s", err.Error()), + Message: fmt.Sprintf("put user password: %s", databaseError.Error()), }) return } @@ -635,7 +636,6 @@ func (api *api) postLogin(rw http.ResponseWriter, r *http.Request) { } // If the user doesn't exist, it will be a default struct. - equal, err := userpassword.Compare(string(user.HashedPassword), loginWithPassword.Password) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ diff --git a/coderd/users_test.go b/coderd/users_test.go index 531ac93be7aa6..e98d0417f942a 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -287,6 +287,57 @@ func TestUpdateUserProfile(t *testing.T) { }) } +func TestUpdateUserPassword(t *testing.T) { + t.Parallel() + + t.Run("WrongPassword", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + coderdtest.CreateFirstUser(t, client) + err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ + Password: "wrongpassword", + NewPassword: "newpassword", + ConfirmNewPassword: "newpassword", + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) + }) + + t.Run("DifferentPasswordConfirmation", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + coderdtest.CreateFirstUser(t, client) + err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ + Password: coderdtest.FirstUserParams.Password, + NewPassword: "newpassword", + ConfirmNewPassword: "wrongconfirmation", + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + }) + + t.Run("Success", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + coderdtest.CreateFirstUser(t, client) + err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ + Password: coderdtest.FirstUserParams.Password, + NewPassword: "newpassword", + ConfirmNewPassword: "newpassword", + }) + require.NoError(t, err, "update password request should be successful") + + // Check if the user can login using the new password + _, err = client.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ + Email: coderdtest.FirstUserParams.Email, + Password: "newpassword", + }) + require.NoError(t, err, "login should be successful") + }) +} + func TestGrantRoles(t *testing.T) { t.Parallel() t.Run("UpdateIncorrectRoles", func(t *testing.T) { diff --git a/codersdk/users.go b/codersdk/users.go index d7b0566755600..42e933add9286 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -72,7 +72,7 @@ type UpdateUserProfileRequest struct { Username string `json:"username" validate:"required,username"` } -type UpdateUserHashedPasswordRequest struct { +type UpdateUserPasswordRequest struct { Password string `json:"password" validate:"required"` NewPassword string `json:"new_password" validate:"required"` ConfirmNewPassword string `json:"confirm_new_password" validate:"required"` @@ -187,6 +187,19 @@ func (c *Client) SuspendUser(ctx context.Context, userID uuid.UUID) (User, error return user, json.NewDecoder(res.Body).Decode(&user) } +// Update user password +func (c *Client) UpdateUserPassword(ctx context.Context, userID uuid.UUID, req UpdateUserPasswordRequest) error { + res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/password", uuidOrMe(userID)), req) + if err != nil { + return err + } + defer res.Body.Close() + if res.StatusCode != http.StatusNoContent { + return readBodyAsError(res) + } + return nil +} + // UpdateUserRoles grants the userID the specified roles. // Include ALL roles the user has. func (c *Client) UpdateUserRoles(ctx context.Context, userID uuid.UUID, req UpdateRoles) (User, error) { From 2699445fac340dbd72b3ea4aa5d0dd8f69c6a196 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 14:25:34 +0000 Subject: [PATCH 05/15] Remove confirmation and fix lint issues --- coderd/users.go | 32 ++++++++------------------------ coderd/users_test.go | 14 -------------- 2 files changed, 8 insertions(+), 38 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 7412ef9f9f475..0fb107f8a4c7c 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -360,33 +360,17 @@ func (api *api) putUserSuspend(rw http.ResponseWriter, r *http.Request) { httpapi.Write(rw, http.StatusOK, convertUser(suspendedUser, organizations)) } -func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { +func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) error { user := httpmw.UserParam(r) var params codersdk.UpdateUserPasswordRequest if !httpapi.Read(rw, r, ¶ms) { - return - } - - // Check if the password is correct - equal, err := userpassword.Compare(string(user.HashedPassword), params.Password) - if err != nil { - httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("compare: %s", err.Error()), - }) - } - if !equal { - // This message is the same as above to remove ease in detecting whether - // users are registered or not. Attackers still could with a timing attack. - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "invalid email or password", - }) - return + return nil } // Check if the new password and the confirmation match if params.NewPassword != params.ConfirmNewPassword { - errors := []httpapi.Error{ + requestErrors := []httpapi.Error{ { Field: "confirm_new_password", Detail: "The value does not match the new password", @@ -394,16 +378,15 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { } httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ Message: fmt.Sprintf("The new password and the new password confirmation don't match"), - Errors: errors, + Errors: requestErrors, }) - return + return nil } // Hash password and update it in the database hashedPassword, hashError := userpassword.Hash(params.NewPassword) if hashError != nil { - xerrors.Errorf("hash password: %w", hashError) - return + return xerrors.Errorf("hash password: %w", hashError) } databaseError := api.Database.UpdateUserHashedPassword(r.Context(), database.UpdateUserHashedPasswordParams{ ID: user.ID, @@ -413,10 +396,11 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("put user password: %s", databaseError.Error()), }) - return + return nil } httpapi.Write(rw, http.StatusNoContent, nil) + return nil } func (api *api) userRoles(rw http.ResponseWriter, r *http.Request) { diff --git a/coderd/users_test.go b/coderd/users_test.go index e98d0417f942a..a326d4b593f35 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -290,20 +290,6 @@ func TestUpdateUserProfile(t *testing.T) { func TestUpdateUserPassword(t *testing.T) { t.Parallel() - t.Run("WrongPassword", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, nil) - coderdtest.CreateFirstUser(t, client) - err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ - Password: "wrongpassword", - NewPassword: "newpassword", - ConfirmNewPassword: "newpassword", - }) - var apiErr *codersdk.Error - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusUnauthorized, apiErr.StatusCode()) - }) - t.Run("DifferentPasswordConfirmation", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) From 355f1631928a87675275d58ed2b4656e70fdff57 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 14:30:46 +0000 Subject: [PATCH 06/15] Return hash error as server error --- coderd/users.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 0fb107f8a4c7c..7fe0226b686c9 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -360,12 +360,12 @@ func (api *api) putUserSuspend(rw http.ResponseWriter, r *http.Request) { httpapi.Write(rw, http.StatusOK, convertUser(suspendedUser, organizations)) } -func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) error { +func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) var params codersdk.UpdateUserPasswordRequest if !httpapi.Read(rw, r, ¶ms) { - return nil + return } // Check if the new password and the confirmation match @@ -380,13 +380,15 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) error { Message: fmt.Sprintf("The new password and the new password confirmation don't match"), Errors: requestErrors, }) - return nil + return } // Hash password and update it in the database hashedPassword, hashError := userpassword.Hash(params.NewPassword) if hashError != nil { - return xerrors.Errorf("hash password: %w", hashError) + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: fmt.Sprintf("hash password: %s", hashError.Error()), + }) } databaseError := api.Database.UpdateUserHashedPassword(r.Context(), database.UpdateUserHashedPasswordParams{ ID: user.ID, @@ -396,11 +398,10 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) error { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("put user password: %s", databaseError.Error()), }) - return nil + return } httpapi.Write(rw, http.StatusNoContent, nil) - return nil } func (api *api) userRoles(rw http.ResponseWriter, r *http.Request) { From 56b29fd821a0afff85b667a38b94c5e3b2fef51c Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Thu, 5 May 2022 12:07:36 -0500 Subject: [PATCH 07/15] Update coderd/database/databasefake/databasefake.go Co-authored-by: Mathias Fredriksson --- coderd/database/databasefake/databasefake.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index f33adf922f100..a9c7dea5fdbd5 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -1318,12 +1318,12 @@ func (q *fakeQuerier) UpdateUserHashedPassword(_ context.Context, arg database.U q.mutex.Lock() defer q.mutex.Unlock() - for index, user := range q.users { + for i, user := range q.users { if user.ID != arg.ID { continue } user.HashedPassword = arg.HashedPassword - q.users[index] = user + q.users[i] = user return nil } return sql.ErrNoRows From 30b8f15172b50e6b80b422c68c1acf1501eeb0bc Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 17:12:18 +0000 Subject: [PATCH 08/15] Improve readbility --- coderd/database/queries/users.sql | 2 +- coderd/users.go | 13 +++++++------ codersdk/users.go | 3 ++- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 35ade0e9762a4..d0c3d4c3e7b66 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -70,7 +70,7 @@ WHERE -- name: GetUsers :many SELECT * -FROM +FROM users WHERE CASE diff --git a/coderd/users.go b/coderd/users.go index 7fe0226b686c9..269a08f9b1214 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -384,19 +384,20 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { } // Hash password and update it in the database - hashedPassword, hashError := userpassword.Hash(params.NewPassword) - if hashError != nil { + hashedPassword, err := userpassword.Hash(params.NewPassword) + if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("hash password: %s", hashError.Error()), + Message: fmt.Sprintf("hash password: %s", err.Error()), }) + return } - databaseError := api.Database.UpdateUserHashedPassword(r.Context(), database.UpdateUserHashedPasswordParams{ + err = api.Database.UpdateUserHashedPassword(r.Context(), database.UpdateUserHashedPasswordParams{ ID: user.ID, HashedPassword: []byte(hashedPassword), }) - if databaseError != nil { + if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ - Message: fmt.Sprintf("put user password: %s", databaseError.Error()), + Message: fmt.Sprintf("put user password: %s", err.Error()), }) return } diff --git a/codersdk/users.go b/codersdk/users.go index 42e933add9286..baea5484225a5 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -187,7 +187,8 @@ func (c *Client) SuspendUser(ctx context.Context, userID uuid.UUID) (User, error return user, json.NewDecoder(res.Body).Decode(&user) } -// Update user password +// UpdateUserPassword updates a user password. +// It calls PUT /users/{user}/password func (c *Client) UpdateUserPassword(ctx context.Context, userID uuid.UUID, req UpdateUserPasswordRequest) error { res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/password", uuidOrMe(userID)), req) if err != nil { From f6be255e05f3d45af6ade198ca82a99757c67643 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 17:14:34 +0000 Subject: [PATCH 09/15] Add RBAC --- coderd/coderd.go | 5 ++++- coderd/rbac/object.go | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index a1d028c7295f7..8e6e727cac2db 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -240,7 +240,10 @@ func New(options *Options) (http.Handler, func()) { r.Get("/", api.userByName) r.Put("/profile", api.putUserProfile) r.Put("/suspend", api.putUserSuspend) - r.Put("/password", api.putUserPassword) + r.Route("/password", func(r chi.Router) { + r.Use(httpmw.WithRBACObject(rbac.ResourceUserPasswordRole)) + r.Put("/password", authorize(api.putUserPassword, rbac.ActionUpdate)) + }) r.Get("/organizations", api.organizationsByUser) r.Post("/organizations", api.postOrganizationsByUser) // These roles apply to the site wide permissions. diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index a4be9b1edab5b..dd12efc25fcb2 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -24,6 +24,10 @@ var ( Type: "user_role", } + ResourceUserPasswordRole = Object{ + Type: "user_password", + } + // ResourceWildcard represents all resource types ResourceWildcard = Object{ Type: WildcardSymbol, From 5df5763204f707c02e7efb522ae21fd4c6c15559 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 17:15:20 +0000 Subject: [PATCH 10/15] Fix route --- coderd/coderd.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 8e6e727cac2db..aa5cdbb21b86c 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -242,7 +242,7 @@ func New(options *Options) (http.Handler, func()) { r.Put("/suspend", api.putUserSuspend) r.Route("/password", func(r chi.Router) { r.Use(httpmw.WithRBACObject(rbac.ResourceUserPasswordRole)) - r.Put("/password", authorize(api.putUserPassword, rbac.ActionUpdate)) + r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate)) }) r.Get("/organizations", api.organizationsByUser) r.Post("/organizations", api.postOrganizationsByUser) From 69af903ca89098d763cfce38d805959bef38a69e Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 17:17:35 +0000 Subject: [PATCH 11/15] Add missing TS types --- site/src/api/typesGenerated.ts | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index ee4add928bb11..acef091100952 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -12,7 +12,7 @@ export interface AgentGitSSHKey { readonly private_key: string } -// From codersdk/users.go:105:6 +// From codersdk/users.go:111:6 export interface AuthMethods { readonly password: boolean readonly github: boolean @@ -44,7 +44,7 @@ export interface CreateFirstUserResponse { readonly organization_id: string } -// From codersdk/users.go:100:6 +// From codersdk/users.go:106:6 export interface CreateOrganizationRequest { readonly name: string } @@ -101,7 +101,7 @@ export interface CreateWorkspaceRequest { readonly parameter_values: CreateParameterRequest[] } -// From codersdk/users.go:96:6 +// From codersdk/users.go:102:6 export interface GenerateAPIKeyResponse { readonly key: string } @@ -119,13 +119,13 @@ export interface GoogleInstanceIdentityToken { readonly json_web_token: string } -// From codersdk/users.go:85:6 +// From codersdk/users.go:91:6 export interface LoginWithPasswordRequest { readonly email: string readonly password: string } -// From codersdk/users.go:91:6 +// From codersdk/users.go:97:6 export interface LoginWithPasswordResponse { readonly session_token: string } @@ -255,11 +255,18 @@ export interface UpdateActiveTemplateVersion { readonly id: string } -// From codersdk/users.go:75:6 +// From codersdk/users.go:81:6 export interface UpdateRoles { readonly roles: string[] } +// From codersdk/users.go:75:6 +export interface UpdateUserPasswordRequest { + readonly password: string + readonly new_password: string + readonly confirm_new_password: string +} + // From codersdk/users.go:70:6 export interface UpdateUserProfileRequest { readonly email: string @@ -291,7 +298,7 @@ export interface User { readonly organization_ids: string[] } -// From codersdk/users.go:79:6 +// From codersdk/users.go:85:6 export interface UserRoles { readonly roles: string[] readonly organization_roles: Record From d85092b86fb60db5bae2d52a97370fb64ed9fde5 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 18:17:21 +0000 Subject: [PATCH 12/15] Update update password request params --- coderd/users.go | 4 ++-- coderd/users_test.go | 10 ++++------ codersdk/users.go | 5 ++--- site/src/api/typesGenerated.ts | 15 +++++++-------- 4 files changed, 15 insertions(+), 19 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 269a08f9b1214..5be762a820855 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -369,7 +369,7 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { } // Check if the new password and the confirmation match - if params.NewPassword != params.ConfirmNewPassword { + if params.Password != params.ConfirmPassword { requestErrors := []httpapi.Error{ { Field: "confirm_new_password", @@ -384,7 +384,7 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { } // Hash password and update it in the database - hashedPassword, err := userpassword.Hash(params.NewPassword) + hashedPassword, err := userpassword.Hash(params.Password) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ Message: fmt.Sprintf("hash password: %s", err.Error()), diff --git a/coderd/users_test.go b/coderd/users_test.go index a326d4b593f35..d20be1c9c4bac 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -295,9 +295,8 @@ func TestUpdateUserPassword(t *testing.T) { client := coderdtest.New(t, nil) coderdtest.CreateFirstUser(t, client) err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ - Password: coderdtest.FirstUserParams.Password, - NewPassword: "newpassword", - ConfirmNewPassword: "wrongconfirmation", + Password: "newpassword", + ConfirmPassword: "wrongconfirmation", }) var apiErr *codersdk.Error require.ErrorAs(t, err, &apiErr) @@ -309,9 +308,8 @@ func TestUpdateUserPassword(t *testing.T) { client := coderdtest.New(t, nil) coderdtest.CreateFirstUser(t, client) err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ - Password: coderdtest.FirstUserParams.Password, - NewPassword: "newpassword", - ConfirmNewPassword: "newpassword", + Password: "newpassword", + ConfirmPassword: "newpassword", }) require.NoError(t, err, "update password request should be successful") diff --git a/codersdk/users.go b/codersdk/users.go index baea5484225a5..c1f17fa7ad128 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -73,9 +73,8 @@ type UpdateUserProfileRequest struct { } type UpdateUserPasswordRequest struct { - Password string `json:"password" validate:"required"` - NewPassword string `json:"new_password" validate:"required"` - ConfirmNewPassword string `json:"confirm_new_password" validate:"required"` + Password string `json:"password" validate:"required"` + ConfirmPassword string `json:"confirm_new_password" validate:"required"` } type UpdateRoles struct { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index acef091100952..de81c5f25542e 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -12,7 +12,7 @@ export interface AgentGitSSHKey { readonly private_key: string } -// From codersdk/users.go:111:6 +// From codersdk/users.go:110:6 export interface AuthMethods { readonly password: boolean readonly github: boolean @@ -44,7 +44,7 @@ export interface CreateFirstUserResponse { readonly organization_id: string } -// From codersdk/users.go:106:6 +// From codersdk/users.go:105:6 export interface CreateOrganizationRequest { readonly name: string } @@ -101,7 +101,7 @@ export interface CreateWorkspaceRequest { readonly parameter_values: CreateParameterRequest[] } -// From codersdk/users.go:102:6 +// From codersdk/users.go:101:6 export interface GenerateAPIKeyResponse { readonly key: string } @@ -119,13 +119,13 @@ export interface GoogleInstanceIdentityToken { readonly json_web_token: string } -// From codersdk/users.go:91:6 +// From codersdk/users.go:90:6 export interface LoginWithPasswordRequest { readonly email: string readonly password: string } -// From codersdk/users.go:97:6 +// From codersdk/users.go:96:6 export interface LoginWithPasswordResponse { readonly session_token: string } @@ -255,7 +255,7 @@ export interface UpdateActiveTemplateVersion { readonly id: string } -// From codersdk/users.go:81:6 +// From codersdk/users.go:80:6 export interface UpdateRoles { readonly roles: string[] } @@ -263,7 +263,6 @@ export interface UpdateRoles { // From codersdk/users.go:75:6 export interface UpdateUserPasswordRequest { readonly password: string - readonly new_password: string readonly confirm_new_password: string } @@ -298,7 +297,7 @@ export interface User { readonly organization_ids: string[] } -// From codersdk/users.go:85:6 +// From codersdk/users.go:84:6 export interface UserRoles { readonly roles: string[] readonly organization_roles: Record From 96ce7513f258105911ab22d5c602b121b3b99df3 Mon Sep 17 00:00:00 2001 From: Bruno Date: Thu, 5 May 2022 19:54:50 +0000 Subject: [PATCH 13/15] Remove confirm password from the API --- coderd/users.go | 16 ---------------- coderd/users_test.go | 16 +--------------- codersdk/users.go | 3 +-- site/src/api/typesGenerated.ts | 15 +++++++-------- 4 files changed, 9 insertions(+), 41 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 5be762a820855..a695ebf65475b 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -368,22 +368,6 @@ func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { return } - // Check if the new password and the confirmation match - if params.Password != params.ConfirmPassword { - requestErrors := []httpapi.Error{ - { - Field: "confirm_new_password", - Detail: "The value does not match the new password", - }, - } - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: fmt.Sprintf("The new password and the new password confirmation don't match"), - Errors: requestErrors, - }) - return - } - - // Hash password and update it in the database hashedPassword, err := userpassword.Hash(params.Password) if err != nil { httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ diff --git a/coderd/users_test.go b/coderd/users_test.go index d20be1c9c4bac..5d4d15a8f6021 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -290,26 +290,12 @@ func TestUpdateUserProfile(t *testing.T) { func TestUpdateUserPassword(t *testing.T) { t.Parallel() - t.Run("DifferentPasswordConfirmation", func(t *testing.T) { - t.Parallel() - client := coderdtest.New(t, nil) - coderdtest.CreateFirstUser(t, client) - err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ - Password: "newpassword", - ConfirmPassword: "wrongconfirmation", - }) - var apiErr *codersdk.Error - require.ErrorAs(t, err, &apiErr) - require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) - }) - t.Run("Success", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) coderdtest.CreateFirstUser(t, client) err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ - Password: "newpassword", - ConfirmPassword: "newpassword", + Password: "newpassword", }) require.NoError(t, err, "update password request should be successful") diff --git a/codersdk/users.go b/codersdk/users.go index c1f17fa7ad128..dd0ad207d91e3 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -73,8 +73,7 @@ type UpdateUserProfileRequest struct { } type UpdateUserPasswordRequest struct { - Password string `json:"password" validate:"required"` - ConfirmPassword string `json:"confirm_new_password" validate:"required"` + Password string `json:"password" validate:"required"` } type UpdateRoles struct { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index de81c5f25542e..e3fa4e53fc0d5 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -12,7 +12,7 @@ export interface AgentGitSSHKey { readonly private_key: string } -// From codersdk/users.go:110:6 +// From codersdk/users.go:109:6 export interface AuthMethods { readonly password: boolean readonly github: boolean @@ -44,7 +44,7 @@ export interface CreateFirstUserResponse { readonly organization_id: string } -// From codersdk/users.go:105:6 +// From codersdk/users.go:104:6 export interface CreateOrganizationRequest { readonly name: string } @@ -101,7 +101,7 @@ export interface CreateWorkspaceRequest { readonly parameter_values: CreateParameterRequest[] } -// From codersdk/users.go:101:6 +// From codersdk/users.go:100:6 export interface GenerateAPIKeyResponse { readonly key: string } @@ -119,13 +119,13 @@ export interface GoogleInstanceIdentityToken { readonly json_web_token: string } -// From codersdk/users.go:90:6 +// From codersdk/users.go:89:6 export interface LoginWithPasswordRequest { readonly email: string readonly password: string } -// From codersdk/users.go:96:6 +// From codersdk/users.go:95:6 export interface LoginWithPasswordResponse { readonly session_token: string } @@ -255,7 +255,7 @@ export interface UpdateActiveTemplateVersion { readonly id: string } -// From codersdk/users.go:80:6 +// From codersdk/users.go:79:6 export interface UpdateRoles { readonly roles: string[] } @@ -263,7 +263,6 @@ export interface UpdateRoles { // From codersdk/users.go:75:6 export interface UpdateUserPasswordRequest { readonly password: string - readonly confirm_new_password: string } // From codersdk/users.go:70:6 @@ -297,7 +296,7 @@ export interface User { readonly organization_ids: string[] } -// From codersdk/users.go:84:6 +// From codersdk/users.go:83:6 export interface UserRoles { readonly roles: string[] readonly organization_roles: Record From 4f9f506fdd32b7d8e2f6255f8fd9857126f6579f Mon Sep 17 00:00:00 2001 From: Bruno Quaresma Date: Thu, 5 May 2022 14:57:50 -0500 Subject: [PATCH 14/15] Update coderd/users.go Co-authored-by: Garrett Delfosse --- coderd/users.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index a695ebf65475b..108ad0813cb49 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -361,9 +361,10 @@ func (api *api) putUserSuspend(rw http.ResponseWriter, r *http.Request) { } func (api *api) putUserPassword(rw http.ResponseWriter, r *http.Request) { - user := httpmw.UserParam(r) - - var params codersdk.UpdateUserPasswordRequest + var ( + user = httpmw.UserParam(r) + params codersdk.UpdateUserPasswordRequest + ) if !httpapi.Read(rw, r, ¶ms) { return } From 671c56d17d6ce6c6eaff394c5c77393c533c8d2e Mon Sep 17 00:00:00 2001 From: Bruno Date: Fri, 6 May 2022 13:30:20 +0000 Subject: [PATCH 15/15] Remove user restriction and refactor tests --- coderd/httpmw/userparam.go | 8 -------- coderd/users_test.go | 31 ++++++++++++++++++++++++------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index ba91414de8f4c..16e768c4ba8c8 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -76,14 +76,6 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler { } } - apiKey := APIKey(r) - if apiKey.UserID != user.ID { - httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ - Message: "getting non-personal users isn't supported yet", - }) - return - } - ctx := context.WithValue(r.Context(), userParamContextKey{}, user) next.ServeHTTP(rw, r.WithContext(ctx)) }) diff --git a/coderd/users_test.go b/coderd/users_test.go index 5d4d15a8f6021..aaf5737ce6b61 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -290,21 +290,38 @@ func TestUpdateUserProfile(t *testing.T) { func TestUpdateUserPassword(t *testing.T) { t.Parallel() - t.Run("Success", func(t *testing.T) { + t.Run("MemberCantUpdateAdminPassword", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) - coderdtest.CreateFirstUser(t, client) - err := client.UpdateUserPassword(context.Background(), codersdk.Me, codersdk.UpdateUserPasswordRequest{ + admin := coderdtest.CreateFirstUser(t, client) + member := coderdtest.CreateAnotherUser(t, client, admin.OrganizationID) + err := member.UpdateUserPassword(context.Background(), admin.UserID, codersdk.UpdateUserPasswordRequest{ Password: "newpassword", }) - require.NoError(t, err, "update password request should be successful") + require.Error(t, err, "member should not be able to update admin password") + }) - // Check if the user can login using the new password + t.Run("AdminCanUpdateMemberPassword", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + admin := coderdtest.CreateFirstUser(t, client) + member, err := client.CreateUser(context.Background(), codersdk.CreateUserRequest{ + Email: "coder@coder.com", + Username: "coder", + Password: "password", + OrganizationID: admin.OrganizationID, + }) + require.NoError(t, err, "create member") + err = client.UpdateUserPassword(context.Background(), member.ID, codersdk.UpdateUserPasswordRequest{ + Password: "newpassword", + }) + require.NoError(t, err, "admin should be able to update member password") + // Check if the member can login using the new password _, err = client.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ - Email: coderdtest.FirstUserParams.Email, + Email: "coder@coder.com", Password: "newpassword", }) - require.NoError(t, err, "login should be successful") + require.NoError(t, err, "member should login successfully with the new password") }) }