From d50ffa78f67ffcfe8d4a01f506856a56f27debee Mon Sep 17 00:00:00 2001 From: Colin Adler Date: Tue, 9 Jul 2024 14:28:39 -0500 Subject: [PATCH] fix: exit reset password request before passwords are compared (#13856) --- coderd/coderd.go | 1 + coderd/httpmw/authz_test.go | 4 +-- coderd/notifications/manager_test.go | 3 +-- coderd/users.go | 5 ++++ coderd/users_test.go | 38 ++++++++++++++++++++++++++++ 5 files changed, 47 insertions(+), 4 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 97b8a93376..3e77490651 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1018,6 +1018,7 @@ func New(options *Options) *API { }) r.Put("/appearance", api.putUserAppearanceSettings) r.Route("/password", func(r chi.Router) { + r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute)) r.Put("/", api.putUserPassword) }) // These roles apply to the site wide permissions. diff --git a/coderd/httpmw/authz_test.go b/coderd/httpmw/authz_test.go index 706590e210..317d812f3c 100644 --- a/coderd/httpmw/authz_test.go +++ b/coderd/httpmw/authz_test.go @@ -18,7 +18,7 @@ func TestAsAuthzSystem(t *testing.T) { t.Parallel() userActor := coderdtest.RandomRBACSubject() - base := http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { + base := http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { actor, ok := dbauthz.ActorFromContext(r.Context()) assert.True(t, ok, "actor should exist") assert.True(t, userActor.Equal(actor), "actor should be the user actor") @@ -80,7 +80,7 @@ func TestAsAuthzSystem(t *testing.T) { mwAssertUser, ) r.Handle("/", base) - r.NotFound(func(writer http.ResponseWriter, request *http.Request) { + r.NotFound(func(http.ResponseWriter, *http.Request) { assert.Fail(t, "should not hit not found, the route should be correct") }) }) diff --git a/coderd/notifications/manager_test.go b/coderd/notifications/manager_test.go index d0d6355f0c..30a736488e 100644 --- a/coderd/notifications/manager_test.go +++ b/coderd/notifications/manager_test.go @@ -7,7 +7,6 @@ import ( "testing" "time" - "github.com/coder/serpent" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -15,7 +14,6 @@ import ( "cdr.dev/slog" "cdr.dev/slog/sloggers/slogtest" - "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbmem" @@ -24,6 +22,7 @@ import ( "github.com/coder/coder/v2/coderd/notifications/dispatch" "github.com/coder/coder/v2/coderd/notifications/types" "github.com/coder/coder/v2/testutil" + "github.com/coder/serpent" ) func TestBufferedUpdates(t *testing.T) { diff --git a/coderd/users.go b/coderd/users.go index 5ef0b2f831..4372a4f7de 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -913,6 +913,11 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { defer commitAudit() aReq.Old = user + if !api.Authorize(r, policy.ActionUpdatePersonal, user) { + httpapi.ResourceNotFound(rw) + return + } + if !httpapi.Read(ctx, rw, r, ¶ms) { return } diff --git a/coderd/users_test.go b/coderd/users_test.go index 758a3ba738..af4a2c2975 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -19,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/exp/slices" "golang.org/x/sync/errgroup" + "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/audit" "github.com/coder/coder/v2/coderd/coderdtest" @@ -826,6 +827,7 @@ func TestUpdateUserPassword(t *testing.T) { }) require.NoError(t, err, "member should login successfully with the new password") }) + t.Run("MemberCanUpdateOwnPassword", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() @@ -853,6 +855,7 @@ func TestUpdateUserPassword(t *testing.T) { require.Len(t, auditor.AuditLogs(), numLogs) require.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[numLogs-1].Action) }) + t.Run("MemberCantUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) @@ -867,6 +870,41 @@ func TestUpdateUserPassword(t *testing.T) { }) require.Error(t, err, "member should not be able to update own password without providing old password") }) + + t.Run("AuditorCantTellIfPasswordIncorrect", func(t *testing.T) { + t.Parallel() + auditor := audit.NewMock() + adminClient := coderdtest.New(t, &coderdtest.Options{Auditor: auditor}) + + adminUser := coderdtest.CreateFirstUser(t, adminClient) + + auditorClient, _ := coderdtest.CreateAnotherUser(t, adminClient, + adminUser.OrganizationID, + rbac.RoleAuditor(), + ) + + _, memberUser := coderdtest.CreateAnotherUser(t, adminClient, adminUser.OrganizationID) + numLogs := len(auditor.AuditLogs()) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + err := auditorClient.UpdateUserPassword(ctx, memberUser.ID.String(), codersdk.UpdateUserPasswordRequest{ + Password: "MySecurePassword!", + }) + numLogs++ // add an audit log for user update + + require.Error(t, err, "auditors shouldn't be able to update passwords") + var httpErr *codersdk.Error + require.True(t, xerrors.As(err, &httpErr)) + // ensure that the error we get is "not found" and not "bad request" + require.Equal(t, http.StatusNotFound, httpErr.StatusCode()) + + require.Len(t, auditor.AuditLogs(), numLogs) + require.Equal(t, database.AuditActionWrite, auditor.AuditLogs()[numLogs-1].Action) + require.Equal(t, int32(http.StatusNotFound), auditor.AuditLogs()[numLogs-1].StatusCode) + }) + t.Run("AdminCanUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { t.Parallel() auditor := audit.NewMock()