fix: exit reset password request before passwords are compared (#13856)

This commit is contained in:
Colin Adler
2024-07-09 14:28:39 -05:00
committed by GitHub
parent 3894ae17a7
commit d50ffa78f6
5 changed files with 47 additions and 4 deletions

View File

@ -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.

View File

@ -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")
})
})

View File

@ -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) {

View File

@ -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, &params) {
return
}

View File

@ -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()