mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix!: deprecate login_type=none, convert existing users to password login (#26851)
> 🤖 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 <zenithwolf1000@users.noreply.github.com>
This commit is contained in:
+61
-6
@@ -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) {
|
||||
|
||||
Reference in New Issue
Block a user