fix: delete all sessions on password change (coder#4659)

sreya · web-flow · commit 7a5ae1e552aa · 2022-10-19T21:12:03.000-05:00
- Prevent users from reusing their old password
  as their new password.
diff --git 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
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git 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
@@ -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
@@ -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) {
