From 2ee803a55442373a527cebc80f98e87dc51daf28 Mon Sep 17 00:00:00 2001 From: Jon Ayers Date: Thu, 20 Oct 2022 00:07:18 +0000 Subject: [PATCH] fix: delete all sessions on password change - Prevent users from reusing their old password as their new password. --- coderd/database/databasefake/databasefake.go | 13 +++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 12 ++++ coderd/database/queries/apikeys.sql | 6 ++ coderd/users.go | 27 ++++++++- coderd/users_test.go | 61 ++++++++++++++++++++ 6 files changed, 117 insertions(+), 3 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index ad839dd08ed2a..6c946ccfff24c 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -368,6 +368,19 @@ func (q *fakeQuerier) DeleteAPIKeyByID(_ context.Context, id string) error { return sql.ErrNoRows } +func (q *fakeQuerier) DeleteAPIKeysByUserID(_ context.Context, userID uuid.UUID) error { + q.mutex.Lock() + defer q.mutex.Unlock() + + for i := len(q.apiKeys) - 1; i >= 0; i-- { + if q.apiKeys[i].UserID == userID { + q.apiKeys = append(q.apiKeys[:i], q.apiKeys[i+1:]...) + } + } + + return nil +} + func (q *fakeQuerier) GetFileByHashAndCreator(_ context.Context, arg database.GetFileByHashAndCreatorParams) (database.File, error) { q.mutex.RLock() defer q.mutex.RUnlock() diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 16cb6f3151653..5a6b8675f35eb 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -20,6 +20,7 @@ type sqlcQuerier interface { // https://www.postgresql.org/docs/9.5/sql-select.html#SQL-FOR-UPDATE-SHARE AcquireProvisionerJob(ctx context.Context, arg AcquireProvisionerJobParams) (ProvisionerJob, error) DeleteAPIKeyByID(ctx context.Context, id string) error + DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteGroupByID(ctx context.Context, id uuid.UUID) error DeleteGroupMember(ctx context.Context, userID uuid.UUID) error diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 16a1109636a53..e7ce3e458ce1e 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -145,6 +145,18 @@ func (q *sqlQuerier) DeleteAPIKeyByID(ctx context.Context, id string) error { return err } +const deleteAPIKeysByUserID = `-- name: DeleteAPIKeysByUserID :exec +DELETE FROM + api_keys +WHERE + user_id = $1 +` + +func (q *sqlQuerier) DeleteAPIKeysByUserID(ctx context.Context, userID uuid.UUID) error { + _, err := q.db.ExecContext(ctx, deleteAPIKeysByUserID, userID) + return err +} + const getAPIKeyByID = `-- name: GetAPIKeyByID :one SELECT id, hashed_secret, user_id, last_used, expires_at, created_at, updated_at, login_type, lifetime_seconds, ip_address, scope diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 4dfb39b30b3ed..c0de70ba2e865 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -54,3 +54,9 @@ FROM api_keys WHERE id = $1; + +-- name: DeleteAPIKeysByUserID :exec +DELETE FROM + api_keys +WHERE + user_id = $1; diff --git a/coderd/users.go b/coderd/users.go index 5b56509786d6c..1e1682dbfd912 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -632,6 +632,14 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { } } + // Prevent users reusing their old password. + if match, _ := userpassword.Compare(string(user.HashedPassword), params.Password); match { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "New password cannot match old password.", + }) + return + } + hashedPassword, err := userpassword.Hash(params.Password) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ @@ -640,9 +648,22 @@ func (api *API) putUserPassword(rw http.ResponseWriter, r *http.Request) { }) return } - err = api.Database.UpdateUserHashedPassword(ctx, database.UpdateUserHashedPasswordParams{ - ID: user.ID, - HashedPassword: []byte(hashedPassword), + + err = api.Database.InTx(func(tx database.Store) error { + err = tx.UpdateUserHashedPassword(ctx, database.UpdateUserHashedPasswordParams{ + ID: user.ID, + HashedPassword: []byte(hashedPassword), + }) + if err != nil { + return xerrors.Errorf("update user hashed password: %w", err) + } + + err = tx.DeleteAPIKeysByUserID(ctx, user.ID) + if err != nil { + return xerrors.Errorf("delete api keys by user ID: %w", err) + } + + return nil }) if err != nil { httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ diff --git a/coderd/users_test.go b/coderd/users_test.go index e4e8b7d661868..bb55b909faf5b 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -644,6 +644,67 @@ func TestUpdateUserPassword(t *testing.T) { assert.Len(t, auditor.AuditLogs, 1) assert.Equal(t, database.AuditActionWrite, auditor.AuditLogs[0].Action) }) + + t.Run("ChangingPasswordDeletesKeys", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + user := coderdtest.CreateFirstUser(t, client) + ctx, _ := testutil.Context(t) + + apikey1, err := client.CreateToken(ctx, user.UserID.String(), codersdk.CreateTokenRequest{}) + require.NoError(t, err) + + apikey2, err := client.CreateToken(ctx, user.UserID.String(), codersdk.CreateTokenRequest{}) + require.NoError(t, err) + + err = client.UpdateUserPassword(ctx, "me", codersdk.UpdateUserPasswordRequest{ + Password: "newpassword", + }) + require.NoError(t, err) + + // Trying to get an API key should fail since our client's token + // has been deleted. + _, err = client.GetAPIKey(ctx, user.UserID.String(), apikey1.Key) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusUnauthorized, cerr.StatusCode()) + + resp, err := client.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: coderdtest.FirstUserParams.Email, + Password: "newpassword", + }) + require.NoError(t, err) + + client.SessionToken = resp.SessionToken + + // Trying to get an API key should fail since all keys are deleted + // on password change. + _, err = client.GetAPIKey(ctx, user.UserID.String(), apikey1.Key) + require.Error(t, err) + cerr = coderdtest.SDKError(t, err) + require.Equal(t, http.StatusNotFound, cerr.StatusCode()) + + _, err = client.GetAPIKey(ctx, user.UserID.String(), apikey2.Key) + require.Error(t, err) + cerr = coderdtest.SDKError(t, err) + require.Equal(t, http.StatusNotFound, cerr.StatusCode()) + }) + + t.Run("PasswordsMustDiffer", func(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + ctx, _ := testutil.Context(t) + + err := client.UpdateUserPassword(ctx, "me", codersdk.UpdateUserPasswordRequest{ + Password: coderdtest.FirstUserParams.Password, + }) + require.Error(t, err) + cerr := coderdtest.SDKError(t, err) + require.Equal(t, http.StatusBadRequest, cerr.StatusCode()) + }) } func TestGrantSiteRoles(t *testing.T) {