PR feedback

coder · Emyrk · Jun 30, 2023 · Jun 13, 2023 · Jun 14, 2023 · Jun 14, 2023
commit 949e6872e3e9104389331a1f341377f11bdd51a3
diff --git a/coderd/database/dbfake/dbfake.go b/coderd/database/dbfake/dbfake.go
@@ -4887,7 +4887,10 @@ func (q *fakeQuerier) UpdateUserLoginType(_ context.Context, arg database.Update
 
 	for i, u := range q.users {
 		if u.ID == arg.UserID {
-			u.LoginType = arg.LoginType
+			u.LoginType = arg.NewLoginType
+			if arg.NewLoginType != database.LoginTypePassword {
+				u.HashedPassword = []byte{}
+			}
 			q.users[i] = u
 			return u, nil
 		}

diff --git a/coderd/database/dbgen/dbgen.go b/coderd/database/dbgen/dbgen.go
@@ -199,7 +199,7 @@ func User(t testing.TB, db database.Store, orig database.User) database.User {
 		ID:             takeFirst(orig.ID, uuid.New()),
 		Email:          takeFirst(orig.Email, namesgenerator.GetRandomName(1)),
 		Username:       takeFirst(orig.Username, namesgenerator.GetRandomName(1)),
-		HashedPassword: takeFirstSlice(orig.HashedPassword, []byte{}),
+		HashedPassword: takeFirstSlice(orig.HashedPassword, []byte(must(cryptorand.String(32)))),
 		CreatedAt:      takeFirst(orig.CreatedAt, database.Now()),
 		UpdatedAt:      takeFirst(orig.UpdatedAt, database.Now()),
 		RBACRoles:      takeFirstSlice(orig.RBACRoles, []string{}),

diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go
@@ -441,6 +441,51 @@ func TestUserLastSeenFilter(t *testing.T) {
 	})
 }
 
+func TestUserChangeLoginType(t *testing.T) {
+	t.Parallel()
+	if testing.Short() {
+		t.SkipNow()
+	}
+
+	sqlDB := testSQLDB(t)
+	err := migrations.Up(sqlDB)
+	require.NoError(t, err)
+	db := database.New(sqlDB)
+	ctx := context.Background()
+
+	alice := dbgen.User(t, db, database.User{
+		LoginType: database.LoginTypePassword,
+	})
+	bob := dbgen.User(t, db, database.User{
+		LoginType: database.LoginTypePassword,
+	})
+	bobExpPass := bob.HashedPassword
+	require.NotEmpty(t, alice.HashedPassword, "hashed password should not start empty")
+	require.NotEmpty(t, bob.HashedPassword, "hashed password should not start empty")
+
+	alice, err = db.UpdateUserLoginType(ctx, database.UpdateUserLoginTypeParams{
+		NewLoginType: database.LoginTypeOIDC,
+		UserID:       alice.ID,
+	})
+
+	require.NoError(t, err)
+	require.Empty(t, alice.HashedPassword, "hashed password should be empty")
+
+	// First check other users are not affected
+	bob, err = db.GetUserByID(ctx, bob.ID)
+	require.NoError(t, err)
+	require.Equal(t, bobExpPass, bob.HashedPassword, "hashed password should not change")
+
+	// Then check password -> password is a noop
+	bob, err = db.UpdateUserLoginType(ctx, database.UpdateUserLoginTypeParams{
+		NewLoginType: database.LoginTypePassword,
+		UserID:       bob.ID,
+	})
+	bob, err = db.GetUserByID(ctx, bob.ID)
+	require.NoError(t, err)
+	require.Equal(t, bobExpPass, bob.HashedPassword, "hashed password should not change")
+}
+
 func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) {
 	t.Helper()
 	require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg)

diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/siteconfig.sql b/coderd/database/queries/siteconfig.sql
@@ -17,7 +17,6 @@ SELECT
 	COALESCE((SELECT value FROM site_configs WHERE key = 'default_proxy_icon_url'), '/emojis/1f3e1.png') :: text AS icon_url
 ;
 
-
 -- name: InsertDeploymentID :exec
 INSERT INTO site_configs (key, value) VALUES ('deployment_id', $1);
 
@@ -58,7 +57,6 @@ SELECT value FROM site_configs WHERE key = 'app_signing_key';
 INSERT INTO site_configs (key, value) VALUES ('app_signing_key', $1)
 ON CONFLICT (key) DO UPDATE set value = $1 WHERE site_configs.key = 'app_signing_key';
 
-
 -- name: GetOauthSigningKey :one
 SELECT value FROM site_configs WHERE key = 'oauth_signing_key';
 

diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql
@@ -2,7 +2,14 @@
 UPDATE
 	users
 SET
-	login_type = @login_type
+	login_type = @new_login_type,
+	hashed_password = CASE WHEN @new_login_type = 'password' :: login_type THEN
+		users.hashed_password
+	ELSE
+		-- If the login type is not password, then the password should be
+        -- cleared.
+		'':: bytea
+	END
 WHERE
 	id = @user_id RETURNING *;
 

diff --git a/coderd/database/types.go b/coderd/database/types.go
@@ -1,13 +1,13 @@
 package database
 
 import (
-	"database/sql/driver"
 	"encoding/json"
 	"time"
 
-	"golang.org/x/xerrors"
+	"database/sql/driver"
 
 	"github.com/google/uuid"
+	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/coderd/rbac"
 )

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -637,8 +637,8 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
 	aReq.New = key
 	aReq.UserID = key.UserID
 
-	for i := range cookies {
-		http.SetCookie(rw, cookies[i])
+	for _, cookie := range cookies {
+		http.SetCookie(rw, cookie)
 	}
 
 	redirect := state.Redirect
@@ -1392,8 +1392,8 @@ func (api *API) convertUserToOauth(ctx context.Context, r *http.Request, db data
 	// will be converted.
 	// nolint:gocritic // system query to update user login type
 	user, err = db.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{
-		LoginType: params.LoginType,
-		UserID:    user.ID,
+		NewLoginType: params.LoginType,
+		UserID:       user.ID,
 	})
 	if err != nil {
 		return database.User{}, httpError{