From 22ed1292e838ce196c3b101cf226a483a496cf0a Mon Sep 17 00:00:00 2001 From: defelmnq Date: Thu, 24 Oct 2024 03:35:05 +0200 Subject: [PATCH 1/5] fix(password): improve logic --- coderd/users.go | 22 ++++++++++++++++++++-- coderd/users_test.go | 2 ++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 73ac5c4212a42..0c0f54ca13e4c 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1018,6 +1018,7 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { ctx = r.Context() user = httpmw.UserParam(r) params codersdk.UpdateUserPasswordRequest + apiKey = httpmw.APIKey(r) auditor = *api.Auditor.Load() aReq, commitAudit = audit.InitRequest[database.User](rw, &audit.RequestParams{ Audit: auditor, @@ -1045,7 +1046,25 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { return } - err := userpassword.Validate(params.Password) + admin, err := api.Database.GetUserByID(ctx, apiKey.UserID) + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching user.", + Detail: err.Error(), + }) + return + } + + // only admins or owners can change passwords without sending old_password + if params.OldPassword == "" && (!slice.Contains(admin.RBACRoles, codersdk.RoleUserAdmin) && + !slice.Contains(admin.RBACRoles, codersdk.RoleOwner)) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Old password is required for non-admin users.", + }) + return + } + + err = userpassword.Validate(params.Password) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid password.", @@ -1059,7 +1078,6 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { return } - // admins can change passwords without sending old_password if params.OldPassword != "" { // if they send something let's validate it ok, err := userpassword.Compare(string(user.HashedPassword), params.OldPassword) diff --git a/coderd/users_test.go b/coderd/users_test.go index bd66bdb1d9a09..57637e19d23ef 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1044,6 +1044,7 @@ func TestUpdateUserPassword(t *testing.T) { OrganizationIDs: []uuid.UUID{owner.OrganizationID}, }) require.NoError(t, err, "create member") + err = client.UpdateUserPassword(ctx, member.ID.String(), codersdk.UpdateUserPasswordRequest{ Password: "SomeNewStrongPassword!", }) @@ -1097,6 +1098,7 @@ func TestUpdateUserPassword(t *testing.T) { Password: "newpassword", }) require.Error(t, err, "member should not be able to update own password without providing old password") + require.ErrorContains(t, err, "Old password is required for non-admin users.") }) t.Run("AuditorCantTellIfPasswordIncorrect", func(t *testing.T) { From dc46e3e4da626e69601d28fc86335f74b565c810 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Thu, 24 Oct 2024 12:51:55 +0000 Subject: [PATCH 2/5] feat(password): add test for validate auditor use case and change logic --- coderd/users.go | 19 ++++--------------- coderd/users_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/coderd/users.go b/coderd/users.go index 0c0f54ca13e4c..815f62a424b22 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1046,25 +1046,14 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { return } - admin, err := api.Database.GetUserByID(ctx, apiKey.UserID) - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching user.", - Detail: err.Error(), - }) - return - } - - // only admins or owners can change passwords without sending old_password - if params.OldPassword == "" && (!slice.Contains(admin.RBACRoles, codersdk.RoleUserAdmin) && - !slice.Contains(admin.RBACRoles, codersdk.RoleOwner)) { + // A user need to put its own password to update it + if apiKey.UserID == user.ID && params.OldPassword == "" { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Old password is required for non-admin users.", + Message: "Old password is required.", }) - return } - err = userpassword.Validate(params.Password) + err := userpassword.Validate(params.Password) if err != nil { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Invalid password.", diff --git a/coderd/users_test.go b/coderd/users_test.go index 57637e19d23ef..ebaa1323f40d4 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1057,6 +1057,31 @@ func TestUpdateUserPassword(t *testing.T) { require.NoError(t, err, "member should login successfully with the new password") }) + t.Run("AuditorCantUpdateOtherUserPassword", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + owner := coderdtest.CreateFirstUser(t, client) + + auditor, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID, rbac.RoleAuditor()) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + member, err := client.CreateUserWithOrgs(ctx, codersdk.CreateUserRequestWithOrgs{ + Email: "coder@coder.com", + Username: "coder", + Password: "SomeStrongPassword!", + OrganizationIDs: []uuid.UUID{owner.OrganizationID}, + }) + require.NoError(t, err, "create member") + + err = auditor.UpdateUserPassword(ctx, member.ID.String(), codersdk.UpdateUserPasswordRequest{ + Password: "SomeNewStrongPassword!", + }) + require.Error(t, err, "auditor should not be able to update member password") + require.ErrorContains(t, err, "unexpected status code 404: Resource not found or you do not have access to this resource") + }) + t.Run("MemberCanUpdateOwnPassword", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() From 70455d43511c07d8a2eecdcd3123b5c53cc1ce3b Mon Sep 17 00:00:00 2001 From: defelmnq Date: Thu, 24 Oct 2024 12:56:37 +0000 Subject: [PATCH 3/5] feat(password): add test for validate auditor use case and change logic --- coderd/users.go | 1 + 1 file changed, 1 insertion(+) diff --git a/coderd/users.go b/coderd/users.go index 815f62a424b22..5e521da3a6004 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1051,6 +1051,7 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "Old password is required.", }) + return } err := userpassword.Validate(params.Password) From 90ee4393fc84d1161ba0744fc8baa63c6d276c4d Mon Sep 17 00:00:00 2001 From: defelmnq Date: Thu, 24 Oct 2024 12:56:57 +0000 Subject: [PATCH 4/5] feat(password): add test for validate auditor use case and change logic --- coderd/users_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index ebaa1323f40d4..7a482c5a4d08f 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1044,7 +1044,6 @@ func TestUpdateUserPassword(t *testing.T) { OrganizationIDs: []uuid.UUID{owner.OrganizationID}, }) require.NoError(t, err, "create member") - err = client.UpdateUserPassword(ctx, member.ID.String(), codersdk.UpdateUserPasswordRequest{ Password: "SomeNewStrongPassword!", }) From 94c4311a05e6b2a9b449debac6375987559e8159 Mon Sep 17 00:00:00 2001 From: defelmnq Date: Thu, 24 Oct 2024 13:28:00 +0000 Subject: [PATCH 5/5] feat(password): add test for validate admin use case and change logic --- coderd/users_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/coderd/users_test.go b/coderd/users_test.go index 7a482c5a4d08f..c33ca933a9d96 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -1122,7 +1122,7 @@ func TestUpdateUserPassword(t *testing.T) { Password: "newpassword", }) require.Error(t, err, "member should not be able to update own password without providing old password") - require.ErrorContains(t, err, "Old password is required for non-admin users.") + require.ErrorContains(t, err, "Old password is required.") }) t.Run("AuditorCantTellIfPasswordIncorrect", func(t *testing.T) { @@ -1159,7 +1159,7 @@ func TestUpdateUserPassword(t *testing.T) { require.Equal(t, int32(http.StatusNotFound), auditor.AuditLogs()[numLogs-1].StatusCode) }) - t.Run("AdminCanUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { + t.Run("AdminCantUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() client := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) @@ -1176,7 +1176,8 @@ func TestUpdateUserPassword(t *testing.T) { }) numLogs++ // add an audit log for user update - require.NoError(t, err, "admin should be able to update own password without providing old password") + require.Error(t, err, "admin should not be able to update own password without providing old password") + require.ErrorContains(t, err, "Old password is required.") require.Len(t, auditor.AuditLogs(), numLogs) require.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[numLogs-1].Action) @@ -1196,7 +1197,8 @@ func TestUpdateUserPassword(t *testing.T) { require.NoError(t, err) err = client.UpdateUserPassword(ctx, "me", codersdk.UpdateUserPasswordRequest{ - Password: "MyNewSecurePassword!", + OldPassword: "SomeSecurePassword!", + Password: "MyNewSecurePassword!", }) require.NoError(t, err)