From 0e104f38e04aac62e7644253fc7037059c61ea42 Mon Sep 17 00:00:00 2001 From: Jake Howell Date: Tue, 28 Jul 2026 21:05:50 +1000 Subject: [PATCH] fix!: deprecate `login_type=none`, convert existing users to password login (#26851) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > 🤖 This PR was modified by Coder Agents on behalf of Jake Howell. Deprecates `login_type=none` (legacy passwordless machine users) in favour of premium **service accounts**, and migrates existing accounts off the deprecated path while preserving their identity. Resolves [DEVEX-226]. ## What this does - **Creation is gated** — `POST /users` and `coder users create` reject `login_type=none` (and the deprecated `--disable-login`) unless a service account is requested. - **Existing users are converted** — migration `000554_legacy_none_login_to_password` rewrites legacy non-system, non–service-account `login_type='none'` accounts to `login_type='password'`. Email addresses are **preserved** and existing API tokens remain valid. Admins can set a password if interactive login is desired. ## Why convert to `password` and not `is_service_account`? Migration `000433_add_is_service_account_to_users` adds two CHECK constraints: - `users_email_not_empty`: `(is_service_account = true) = (email = '')` - `users_service_account_login_type`: `is_service_account = false OR login_type = 'none'` Turning a real, email-bearing `login_type=none` user into a service account would require **blanking their email**. Converting to `password` instead preserves the account and its email. > ⚠️ **Breaking / one-way.** The `down` migration cannot restore which users originally had `login_type='none'`. Decision log - **Goal:** move existing `login_type=none` users off the deprecated path while preserving their identity/email. - **Constraint discovered:** the `is_service_account` CHECK constraints (migration `000433`) make a literal `none → service account` conversion require blanking emails, so this PR converts to `password` instead to keep emails intact. - **Implementation:** creation-gating in `cli/usercreate.go` and `coderd/users.go`, matching test updates, plus the `000554_legacy_none_login_to_password.{up,down}.sql` migration. - **CI fix:** the branch was behind `main` and its migration originally numbered `000534`, which collided with main's `000534_drop_chat_model_configs_provider`. Merged `main` and renumbered to `000554` (next free after main's `000553`). `make gen` produces no drift (the migration is data-only). > The service-account conversion alternative (#27182, which blanked emails) was closed in favour of this password-preserving approach. > > Docs follow-up: #27333. [DEVEX-226]: https://linear.app/issue/DEVEX-226 --------- Co-authored-by: Sushant P --- cli/usercreate.go | 15 ++-- cli/usercreate_test.go | 15 ++++ ...554_legacy_none_login_to_password.down.sql | 2 + ...00554_legacy_none_login_to_password.up.sql | 9 ++ coderd/database/migrations/migrate_test.go | 83 +++++++++++++++++++ coderd/userauth_test.go | 16 ++-- coderd/users.go | 7 ++ coderd/users_test.go | 67 +++++++++++++-- 8 files changed, 198 insertions(+), 16 deletions(-) create mode 100644 coderd/database/migrations/000554_legacy_none_login_to_password.down.sql create mode 100644 coderd/database/migrations/000554_legacy_none_login_to_password.up.sql diff --git a/cli/usercreate.go b/cli/usercreate.go index 1a90458259..dbbe92be66 100644 --- a/cli/usercreate.go +++ b/cli/usercreate.go @@ -44,10 +44,15 @@ func (r *RootCmd) userCreate() *serpent.Command { case disableLogin: return xerrors.New("You cannot use --disable-login with --service-account") } - } - - if disableLogin && loginType != "" { - return xerrors.New("You cannot specify both --disable-login and --login-type") + } else { + switch { + case disableLogin && loginType != "": + return xerrors.New("You cannot specify both --disable-login and --login-type") + case disableLogin: + return xerrors.New("--disable-login is deprecated. Use --service-account for machine-to-machine access.") + case loginType == string(codersdk.LoginTypeNone): + return xerrors.New("Login type 'none' is deprecated. Use --service-account for machine-to-machine access.") + } } client, err := r.InitClient(inv) @@ -200,7 +205,7 @@ Create a workspace `+pretty.Sprint(cliui.DefaultStyles.Code, "coder create")+`! { Flag: "disable-login", Hidden: true, - Description: "Deprecated: Use '--login-type=none'. \nDisabling login for a user prevents the user from authenticating via password or IdP login. Authentication requires an API key/token generated by an admin. " + + Description: "Deprecated: Use --service-account (requires Premium) for machine-to-machine access. \nDisabling login for a user prevents the user from authenticating via password or IdP login. Authentication requires an API key/token generated by an admin. " + "Be careful when using this flag as it can lock the user out of their account.", Value: serpent.BoolOf(&disableLogin), }, diff --git a/cli/usercreate_test.go b/cli/usercreate_test.go index 7453d37123..5fc9c86d81 100644 --- a/cli/usercreate_test.go +++ b/cli/usercreate_test.go @@ -160,6 +160,21 @@ func TestUserCreate(t *testing.T) { args: []string{"--service-account", "-u", "dean", "--password", "1n5ecureP4ssw0rd!"}, err: "You cannot use --password with --service-account", }, + { + name: "DisableLogin", + args: []string{"--disable-login", "-u", "dean"}, + err: "--disable-login is deprecated. Use --service-account for machine-to-machine access.", + }, + { + name: "LoginTypeNone", + args: []string{"--login-type", "none", "-u", "dean"}, + err: "Login type 'none' is deprecated. Use --service-account for machine-to-machine access.", + }, + { + name: "DisableLoginWithLoginType", + args: []string{"--disable-login", "--login-type", "password", "-u", "dean"}, + err: "You cannot specify both --disable-login and --login-type", + }, } for _, tt := range tests { diff --git a/coderd/database/migrations/000554_legacy_none_login_to_password.down.sql b/coderd/database/migrations/000554_legacy_none_login_to_password.down.sql new file mode 100644 index 0000000000..b6ae9ef796 --- /dev/null +++ b/coderd/database/migrations/000554_legacy_none_login_to_password.down.sql @@ -0,0 +1,2 @@ +-- We do not track which users had login_type 'none' before this migration. +-- This is a destructive migration that cannot be undone. diff --git a/coderd/database/migrations/000554_legacy_none_login_to_password.up.sql b/coderd/database/migrations/000554_legacy_none_login_to_password.up.sql new file mode 100644 index 0000000000..82c13214e7 --- /dev/null +++ b/coderd/database/migrations/000554_legacy_none_login_to_password.up.sql @@ -0,0 +1,9 @@ +-- Convert legacy users created with login_type 'none' to password auth. +-- OSS deployments cannot create service accounts without Premium. Existing +-- API tokens remain valid; admins can set a password if password login is +-- desired. +UPDATE users +SET login_type = 'password' +WHERE login_type = 'none' + AND is_service_account = false + AND is_system = false; diff --git a/coderd/database/migrations/migrate_test.go b/coderd/database/migrations/migrate_test.go index 11897bec83..f7f2ec7561 100644 --- a/coderd/database/migrations/migrate_test.go +++ b/coderd/database/migrations/migrate_test.go @@ -1717,6 +1717,89 @@ func TestMigration000546ChatHistoryAPIKeyConstraints(t *testing.T) { } } +func TestMigration000554LegacyNoneLoginToPassword(t *testing.T) { + t.Parallel() + + const priorMigrationVersion = 553 + + sqlDB := testSQLDB(t) + + next, err := migrations.Stepper(sqlDB) + require.NoError(t, err) + for { + version, more, err := next() + require.NoError(t, err) + if !more || version == priorMigrationVersion { + break + } + } + + ctx := testutil.Context(t, testutil.WaitSuperLong) + now := time.Now().UTC().Truncate(time.Microsecond) + + legacyNoneID := uuid.New() + serviceAccountID := uuid.New() + systemID := uuid.New() + passwordID := uuid.New() + + // A legacy machine user: login_type 'none', not a service account, not a + // system user. This is the only row the migration should convert. + _, err = sqlDB.ExecContext(ctx, + `INSERT INTO users (id, username, email, hashed_password, created_at, updated_at, status, rbac_roles, login_type, is_service_account, is_system) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)`, + legacyNoneID, "legacy-none", "legacy-none@test.com", []byte{}, now, now, "active", pq.StringArray{}, "none", false, false) + require.NoError(t, err) + + // A service account must keep login_type 'none' (a CHECK constraint requires + // service accounts to use 'none' and an empty email). + _, err = sqlDB.ExecContext(ctx, + `INSERT INTO users (id, username, email, hashed_password, created_at, updated_at, status, rbac_roles, login_type, is_service_account, is_system) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)`, + serviceAccountID, "service-account", "", []byte{}, now, now, "active", pq.StringArray{}, "none", true, false) + require.NoError(t, err) + + // A system user must be left untouched. + _, err = sqlDB.ExecContext(ctx, + `INSERT INTO users (id, username, email, hashed_password, created_at, updated_at, status, rbac_roles, login_type, is_service_account, is_system) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)`, + systemID, "system-user", "system@test.com", []byte{}, now, now, "active", pq.StringArray{}, "none", false, true) + require.NoError(t, err) + + // An existing password user must be left untouched. + _, err = sqlDB.ExecContext(ctx, + `INSERT INTO users (id, username, email, hashed_password, created_at, updated_at, status, rbac_roles, login_type, is_service_account, is_system) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)`, + passwordID, "password-user", "password@test.com", []byte("hashed"), now, now, "active", pq.StringArray{}, "password", false, false) + require.NoError(t, err) + + migrationSQL, err := os.ReadFile("000554_legacy_none_login_to_password.up.sql") + require.NoError(t, err) + _, err = sqlDB.ExecContext(ctx, string(migrationSQL)) + require.NoError(t, err) + + getUser := func(t *testing.T, id uuid.UUID) (loginType, email string) { + t.Helper() + err := sqlDB.QueryRowContext(ctx, + `SELECT login_type::text, email FROM users WHERE id = $1`, id).Scan(&loginType, &email) + require.NoError(t, err) + return loginType, email + } + + // The legacy machine user is converted to password auth with its email + // preserved. + gotLoginType, gotEmail := getUser(t, legacyNoneID) + require.Equal(t, "password", gotLoginType) + require.Equal(t, "legacy-none@test.com", gotEmail) + + // Service accounts, system users, and existing password users are unchanged. + gotLoginType, _ = getUser(t, serviceAccountID) + require.Equal(t, "none", gotLoginType) + gotLoginType, _ = getUser(t, systemID) + require.Equal(t, "none", gotLoginType) + gotLoginType, _ = getUser(t, passwordID) + require.Equal(t, "password", gotLoginType) +} + func TestMigration000498SoftDeleteStaleWorkspaceAgents(t *testing.T) { t.Parallel() diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index 463ce83651..709b6d3764 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -153,13 +153,19 @@ func TestUserLogin(t *testing.T) { t.Run("LoginTypeNone", func(t *testing.T) { t.Parallel() - anotherClient, anotherUser := coderdtest.CreateAnotherUserMutators(t, client, user.OrganizationID, nil, func(r *codersdk.CreateUserRequestWithOrgs) { - r.Password = "" - r.UserLoginType = codersdk.LoginTypeNone + client, db := coderdtest.NewWithDatabase(t, nil) + first := coderdtest.CreateFirstUser(t, client) + + noneUser := dbgen.User(t, db, database.User{ + LoginType: database.LoginTypeNone, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: first.OrganizationID, + UserID: noneUser.ID, }) - _, err := anotherClient.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ - Email: anotherUser.Email, + _, err := client.LoginWithPassword(context.Background(), codersdk.LoginWithPasswordRequest{ + Email: noneUser.Email, Password: "SomeSecurePassword!", }) require.Error(t, err) diff --git a/coderd/users.go b/coderd/users.go index 77c6954c6c..a9151bd6c6 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -488,6 +488,13 @@ func (api *API) postUser(rw http.ResponseWriter, r *http.Request) { req.UserLoginType = codersdk.LoginTypePassword } + if !req.ServiceAccount && req.UserLoginType == codersdk.LoginTypeNone { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: "Login type 'none' requires a service account.", + }) + return + } + if req.UserLoginType != codersdk.LoginTypePassword && req.Password != "" { httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: fmt.Sprintf("Password cannot be set for non-password (%q) authentication.", req.UserLoginType), diff --git a/coderd/users_test.go b/coderd/users_test.go index bf35cf1d7f..1b3916fe9b 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -287,6 +287,62 @@ func TestPostLogin(t *testing.T) { require.NotContains(t, apiErr.Message, string(codersdk.LoginTypeOIDC)) }) + // Regression: the legacy `login_type = 'none'` migration converts these + // accounts to password auth, but they have no password hash. Converting + // login type must never let someone authenticate with an empty or guessed + // password. + t.Run("ConvertedNoneUserHasNoUsablePassword", func(t *testing.T) { + t.Parallel() + client, db := coderdtest.NewWithDatabase(t, nil) + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) + defer cancel() + + // A legacy machine user was created with login_type 'none' and no + // password. dbgen.User substitutes a random hash for an empty one, so + // clear it explicitly to match the real account. + noneUser := dbgen.User(t, db, database.User{ + Email: "legacy-machine-user@coder.com", + LoginType: database.LoginTypeNone, + }) + //nolint:gocritic // Test setup requires a system context to clear the hash. + err := db.UpdateUserHashedPassword(dbauthz.AsSystemRestricted(ctx), database.UpdateUserHashedPasswordParams{ + ID: noneUser.ID, + HashedPassword: []byte{}, + }) + require.NoError(t, err) + + // Apply the migration's conversion: login_type 'none' -> 'password'. + //nolint:gocritic // Test setup requires a system context to convert the login type. + _, err = db.UpdateUserLoginType(dbauthz.AsSystemRestricted(ctx), database.UpdateUserLoginTypeParams{ + NewLoginType: database.LoginTypePassword, + UserID: noneUser.ID, + }) + require.NoError(t, err) + + // Neither an empty password nor a guessed one may authenticate. An empty + // password is rejected by request validation (400); a non-empty guess + // fails the hash comparison against the empty stored hash (401). Both must + // deny access. + cases := []struct { + name string + password string + wantStatus int + }{ + {"EmptyPassword", "", http.StatusBadRequest}, + {"GuessedPassword", "hunter2", http.StatusUnauthorized}, + } + for _, tc := range cases { + anonClient := codersdk.New(client.URL) + _, err := anonClient.LoginWithPassword(ctx, codersdk.LoginWithPasswordRequest{ + Email: noneUser.Email, + Password: tc.password, + }) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr, "%s must not authenticate", tc.name) + require.Equal(t, tc.wantStatus, apiErr.StatusCode(), "%s", tc.name) + } + }) + t.Run("Suspended", func(t *testing.T) { t.Parallel() auditor := audit.NewMock() @@ -952,18 +1008,17 @@ func TestPostUsers(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong) defer cancel() - user, err := client.CreateUserWithOrgs(ctx, codersdk.CreateUserRequestWithOrgs{ + _, err := client.CreateUserWithOrgs(ctx, codersdk.CreateUserRequestWithOrgs{ OrganizationIDs: []uuid.UUID{first.OrganizationID}, Email: "another@user.org", Username: "someone-else", Password: "", UserLoginType: codersdk.LoginTypeNone, }) - require.NoError(t, err) - - found, err := client.User(ctx, user.ID.String()) - require.NoError(t, err) - require.Equal(t, found.LoginType, codersdk.LoginTypeNone) + var apiErr *codersdk.Error + require.ErrorAs(t, err, &apiErr) + require.Equal(t, http.StatusBadRequest, apiErr.StatusCode()) + require.Contains(t, apiErr.Message, "service account") }) t.Run("CreateOIDCLoginType", func(t *testing.T) {