From a748143d885e7e3f2582a416d4ab1b26537173cf Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 20:34:27 +0000 Subject: [PATCH] chore(auth): drop the unused User.Salt field and stop touching users.salt User.Salt predates bcrypt (which embeds its own salt in the hash). Every writer set it to "" and nothing read it, but the store still SELECTed, INSERTed and UPDATEd the users.salt column, and the bootstrap admin INSERTs in migrate.go named it too. Remove the field and every reference to the column from application code: the scanUser SELECT lists, CreateUser, CreateAdminIfNone, UpdateUserCredentials, CompletePasswordReset and both ensureAdminUser INSERTs. The column stays (NOT NULL DEFAULT ''), so rows inserted without it remain valid and code still running from the previous release keeps working during a rolling deploy. Dropping the column is left to a later migration once no deployed release references it. New integration tests drop the column from a migrated database and drive every one of those statements, so they fail on main with `column "salt" does not exist`. Refs #313 Co-Authored-By: Claude Opus 5.5 --- internal/auth/service_credentials_db_test.go | 6 +- internal/auth/service_login_db_test.go | 14 ++-- internal/auth/service_password.go | 2 - internal/auth/service_test.go | 4 - internal/auth/service_user.go | 3 - internal/auth/store_postgres.go | 22 +++--- internal/auth/store_postgres_credentials.go | 10 +-- internal/auth/store_postgres_db_test.go | 1 - .../auth/store_postgres_no_salt_db_test.go | 75 +++++++++++++++++++ internal/auth/store_postgres_pgxmock_test.go | 58 +++++++------- internal/auth/store_postgres_test.go | 74 +++++++++--------- internal/auth/test_helpers.go | 1 - internal/auth/types.go | 1 - .../ensure_admin_user_no_salt_test.go | 44 +++++++++++ .../database/postgres/migrations/migrate.go | 8 +- 15 files changed, 213 insertions(+), 110 deletions(-) create mode 100644 internal/auth/store_postgres_no_salt_db_test.go create mode 100644 internal/database/postgres/migrations/ensure_admin_user_no_salt_test.go diff --git a/internal/auth/service_credentials_db_test.go b/internal/auth/service_credentials_db_test.go index 8c7bee1b0..badad7d5f 100644 --- a/internal/auth/service_credentials_db_test.go +++ b/internal/auth/service_credentials_db_test.go @@ -184,7 +184,7 @@ func TestIntegration_CredentialWritesPreserveConcurrentMFA(t *testing.T) { assert.True(t, f.svc.verifyPassword(credentialRaceNew, stored.PasswordHash)) assert.Equal(t, []string{f.user.PasswordHash}, stored.PasswordHistory) assertOnlyChanged(t, enrolled(), stored, func(w *User) { - w.Email, w.PasswordHash, w.Salt, w.PasswordHistory = "profile-both-new@example.com", stored.PasswordHash, "", stored.PasswordHistory + w.Email, w.PasswordHash, w.PasswordHistory = "profile-both-new@example.com", stored.PasswordHash, stored.PasswordHistory w.PasswordVersion++ }) _, err := f.svc.ValidateSession(ctx, f.session) @@ -202,7 +202,7 @@ func TestIntegration_CredentialWritesPreserveConcurrentMFA(t *testing.T) { assert.True(t, f.svc.verifyPassword(credentialRaceNew, stored.PasswordHash)) assert.Equal(t, []string{f.user.PasswordHash}, stored.PasswordHistory) assertOnlyChanged(t, enrolled(), stored, func(w *User) { - w.PasswordHash, w.Salt, w.PasswordHistory = stored.PasswordHash, "", stored.PasswordHistory + w.PasswordHash, w.PasswordHistory = stored.PasswordHash, stored.PasswordHistory w.PasswordVersion++ }) _, err := f.svc.ValidateSession(ctx, f.session) @@ -324,7 +324,7 @@ func TestIntegration_ResetConfirmPreservesConcurrentMFA(t *testing.T) { stored := f.stored() assert.True(t, f.svc.verifyPassword(credentialRaceNew, stored.PasswordHash)) assertOnlyChanged(t, enrolled(), stored, func(w *User) { - w.PasswordHash, w.Salt, w.PasswordHistory = stored.PasswordHash, "", []string{f.user.PasswordHash} + w.PasswordHash, w.PasswordHistory = stored.PasswordHash, []string{f.user.PasswordHash} w.PasswordResetToken, w.PasswordResetExpiry = "", nil w.PasswordVersion++ }) diff --git a/internal/auth/service_login_db_test.go b/internal/auth/service_login_db_test.go index 7b2244760..40a3931e0 100644 --- a/internal/auth/service_login_db_test.go +++ b/internal/auth/service_login_db_test.go @@ -37,13 +37,13 @@ func overwriteSecurityColumns(ctx context.Context, store *PostgresStore, u *User return store.withAdminInvariantLock(ctx, "overwrite security columns", func(tx pgx.Tx) error { tag, err := tx.Exec(ctx, ` UPDATE users SET - email = $2, password_hash = $3, salt = $4, group_ids = $5, - active = $6, deactivated_at = $7, mfa_enabled = $8, mfa_secret = $9, - mfa_pending_secret = $10, mfa_pending_secret_expires_at = $11, - mfa_recovery_codes = $12, password_reset_token = $13, - password_reset_expiry = $14, password_history = $15, updated_at = NOW() + email = $2, password_hash = $3, group_ids = $4, + active = $5, deactivated_at = $6, mfa_enabled = $7, mfa_secret = $8, + mfa_pending_secret = $9, mfa_pending_secret_expires_at = $10, + mfa_recovery_codes = $11, password_reset_token = $12, + password_reset_expiry = $13, password_history = $14, updated_at = NOW() WHERE id = $1`, - u.ID, u.Email, u.PasswordHash, u.Salt, u.GroupIDs, + u.ID, u.Email, u.PasswordHash, u.GroupIDs, u.Active, u.DeactivatedAt, u.MFAEnabled, u.MFASecret, u.MFAPendingSecret, u.MFAPendingSecretExpiresAt, u.MFARecoveryCodes, u.PasswordResetToken, @@ -78,7 +78,7 @@ func TestIntegration_LoginPreservesSecurityChanges(t *testing.T) { return readErr } stamp := time.Now().UTC().Truncate(time.Microsecond) - current.PasswordHash, current.Salt = "new-password-hash", "new-salt" + current.PasswordHash = "new-password-hash" current.Email = "changed-" + current.Email current.MFAEnabled, current.MFASecret = !current.MFAEnabled, "changed-secret" current.MFAPendingSecret, current.MFAPendingSecretExpiresAt = "pending-secret", &stamp diff --git a/internal/auth/service_password.go b/internal/auth/service_password.go index a8b437a3e..a40a79aaf 100644 --- a/internal/auth/service_password.go +++ b/internal/auth/service_password.go @@ -281,7 +281,6 @@ func (s *Service) ChangePassword(ctx context.Context, userID string, req ChangeP user.PasswordHistory = addToPasswordHistory(user.PasswordHash, user.PasswordHistory) // Set new password - user.Salt = "" // Not used anymore user.PasswordHash = passwordHash if err := s.store.UpdateUserCredentials(ctx, user, user.Email, readPasswordHash); err != nil { @@ -586,7 +585,6 @@ func (s *Service) processPasswordReset(user *User, newPassword string) error { user.PasswordHistory = addToPasswordHistory(user.PasswordHash, user.PasswordHistory) } - user.Salt = "" user.PasswordHash = passwordHash return nil } diff --git a/internal/auth/service_test.go b/internal/auth/service_test.go index b399dd4d9..ca83924fc 100644 --- a/internal/auth/service_test.go +++ b/internal/auth/service_test.go @@ -414,7 +414,6 @@ func TestLogin_WithMFA(t *testing.T) { ID: "user-123", Email: "mfa@example.com", PasswordHash: hash, - Salt: "", // Not used anymore Active: true, MFAEnabled: true, MFASecret: mfaSecret, @@ -472,7 +471,6 @@ func TestLogin_WithMFA_InvalidCode(t *testing.T) { ID: "user-123", Email: "mfa@example.com", PasswordHash: hash, - Salt: "", // Not used anymore Active: true, MFAEnabled: true, MFASecret: "JBSWY3DPEHPK3PXP", @@ -507,7 +505,6 @@ func TestLogin_WithMFA_MissingCode(t *testing.T) { ID: "user-123", Email: "mfa@example.com", PasswordHash: hash, - Salt: "", // Not used anymore Active: true, MFAEnabled: true, MFASecret: "JBSWY3DPEHPK3PXP", @@ -541,7 +538,6 @@ func TestLogin_WithMFA_NoSecret(t *testing.T) { ID: "user-123", Email: "mfa@example.com", PasswordHash: hash, - Salt: "", Active: true, MFAEnabled: true, MFASecret: "", // Data-integrity anomaly: MFA enabled but no secret stored diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index ac4455f7f..05a325e38 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -48,7 +48,6 @@ func (s *Service) SetupAdmin(ctx context.Context, req SetupAdminRequest) (*Login ID: uuid.New().String(), Email: req.Email, PasswordHash: passwordHash, - Salt: "", // Not used anymore, but kept for backward compatibility GroupIDs: []string{DefaultAdminGroupID}, CreatedAt: now, UpdatedAt: now, @@ -232,7 +231,6 @@ func (s *Service) CreateUser(ctx context.Context, actorUserID string, req Create ID: uuid.New().String(), Email: req.Email, PasswordHash: passwordHash, - Salt: "", // Not used anymore, but kept for backward compatibility GroupIDs: req.GroupIDs, CreatedAt: now, UpdatedAt: now, @@ -869,7 +867,6 @@ func (s *Service) updateUserPassword(user *User, newPassword string) (bool, erro user.PasswordHistory = addToPasswordHistory(user.PasswordHash, user.PasswordHistory) } - user.Salt = "" user.PasswordHash = hash return true, nil } diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index c71cd3185..5915249f7 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -44,7 +44,7 @@ var _ StoreInterface = (*PostgresStore)(nil) // GetUserByID retrieves a user by ID. func (s *PostgresStore) GetUserByID(ctx context.Context, userID string) (*User, error) { query := ` - SELECT id, email, password_hash, salt, group_ids, active, + SELECT id, email, password_hash, group_ids, active, mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, @@ -59,7 +59,7 @@ func (s *PostgresStore) GetUserByID(ctx context.Context, userID string) (*User, // GetUserByEmail retrieves a user by email. func (s *PostgresStore) GetUserByEmail(ctx context.Context, email string) (*User, error) { query := ` - SELECT id, email, password_hash, salt, group_ids, active, + SELECT id, email, password_hash, group_ids, active, mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, @@ -89,12 +89,12 @@ func (s *PostgresStore) CreateUser(ctx context.Context, user *User) error { query := ` INSERT INTO users ( - id, email, password_hash, salt, group_ids, active, + id, email, password_hash, group_ids, active, mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, created_at, updated_at, last_login_at - ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19) + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18) ` // Substitute an empty slice for nil so the Postgres TEXT[] column @@ -110,7 +110,6 @@ func (s *PostgresStore) CreateUser(ctx context.Context, user *User) error { user.ID, user.Email, user.PasswordHash, - user.Salt, user.GroupIDs, user.Active, user.MFAEnabled, @@ -209,7 +208,7 @@ func (s *PostgresStore) ListUsers(ctx context.Context) ([]User, error) { // LIMIT provides a safety cap against unbounded memory allocation on large installations. // Pagination support should be added if this limit proves insufficient. query := ` - SELECT id, email, password_hash, salt, group_ids, active, + SELECT id, email, password_hash, group_ids, active, mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, @@ -248,7 +247,7 @@ func (s *PostgresStore) ListUsers(ctx context.Context) ([]User, error) { // as "used" instead of "expired" (QA bug 11.2). func (s *PostgresStore) GetUserByResetToken(ctx context.Context, token string) (*User, error) { query := ` - SELECT id, email, password_hash, salt, group_ids, active, + SELECT id, email, password_hash, group_ids, active, mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, @@ -382,14 +381,14 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool // breaking that contract. query := ` INSERT INTO users ( - id, email, password_hash, salt, group_ids, active, + id, email, password_hash, group_ids, active, mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at, mfa_recovery_codes, password_reset_token, password_reset_expiry, failed_login_attempts, locked_until, password_history, created_at, updated_at, last_login_at ) - SELECT $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19 - WHERE NOT EXISTS (SELECT 1 FROM users WHERE group_ids @> ARRAY[$20::uuid] AND active = true) + SELECT $1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18 + WHERE NOT EXISTS (SELECT 1 FROM users WHERE group_ids @> ARRAY[$19::uuid] AND active = true) ` // See CreateUser for the nil-slice substitution rationale. Force the @@ -425,7 +424,7 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool } tag, err := tx.Exec(ctx, query, - user.ID, user.Email, user.PasswordHash, user.Salt, + user.ID, user.Email, user.PasswordHash, groupIDs, user.Active, user.MFAEnabled, user.MFASecret, user.MFAPendingSecret, user.MFAPendingSecretExpiresAt, recoveryCodes, user.PasswordResetToken, user.PasswordResetExpiry, @@ -737,7 +736,6 @@ func (s *PostgresStore) scanUser(scanner Scanner) (*User, error) { &user.ID, &user.Email, &user.PasswordHash, - &user.Salt, &groupIDs, &user.Active, &user.MFAEnabled, diff --git a/internal/auth/store_postgres_credentials.go b/internal/auth/store_postgres_credentials.go index cf2107a1e..d01301572 100644 --- a/internal/auth/store_postgres_credentials.go +++ b/internal/auth/store_postgres_credentials.go @@ -40,11 +40,11 @@ func (s *PostgresStore) UpdateUserAdminFields(ctx context.Context, user *User, r // the credentials written here (issue #526). func (s *PostgresStore) UpdateUserCredentials(ctx context.Context, user *User, readEmail, readPasswordHash string) error { result, err := s.db.Exec(ctx, ` - UPDATE users SET email = $4, password_hash = $5, salt = $6, - password_history = $7, password_reset_token = NULL, password_reset_expiry = NULL, + UPDATE users SET email = $4, password_hash = $5, + password_history = $6, password_reset_token = NULL, password_reset_expiry = NULL, updated_at = NOW() WHERE id = $1 AND email = $2 AND password_hash = $3 - `, user.ID, readEmail, readPasswordHash, user.Email, user.PasswordHash, user.Salt, user.PasswordHistory) + `, user.ID, readEmail, readPasswordHash, user.Email, user.PasswordHash, user.PasswordHistory) if err != nil { return fmt.Errorf("failed to update user credentials: %w", err) } @@ -60,11 +60,11 @@ func (s *PostgresStore) UpdateUserCredentials(ctx context.Context, user *User, r // deactivated (issues #493, #526). func (s *PostgresStore) CompletePasswordReset(ctx context.Context, user *User, readResetToken, readPasswordHash string) error { result, err := s.db.Exec(ctx, ` - UPDATE users SET password_hash = $4, salt = $5, password_history = $6, active = $7, + UPDATE users SET password_hash = $4, password_history = $5, active = $6, password_reset_token = NULL, password_reset_expiry = NULL, updated_at = NOW() WHERE id = $1 AND password_reset_token = $2 AND password_hash = $3 AND deactivated_at IS NULL AND password_reset_expiry > NOW() - `, user.ID, readResetToken, readPasswordHash, user.PasswordHash, user.Salt, user.PasswordHistory, user.Active) + `, user.ID, readResetToken, readPasswordHash, user.PasswordHash, user.PasswordHistory, user.Active) if err != nil { return fmt.Errorf("failed to complete password reset: %w", err) } diff --git a/internal/auth/store_postgres_db_test.go b/internal/auth/store_postgres_db_test.go index 6c580231d..ee66f63c9 100644 --- a/internal/auth/store_postgres_db_test.go +++ b/internal/auth/store_postgres_db_test.go @@ -65,7 +65,6 @@ func newBootstrapCandidate(email string) *User { return &User{ Email: email, PasswordHash: "test-hash", - Salt: "test-salt", Active: true, } } diff --git a/internal/auth/store_postgres_no_salt_db_test.go b/internal/auth/store_postgres_no_salt_db_test.go new file mode 100644 index 000000000..33b7fc475 --- /dev/null +++ b/internal/auth/store_postgres_no_salt_db_test.go @@ -0,0 +1,75 @@ +//go:build integration + +package auth + +// Issue #313: users.salt is a leftover from before bcrypt and nothing needs +// it. This test drops the column from a fully migrated database and drives +// every store statement that used to read or write it. If any of them still +// names the column, Postgres fails it with `column "salt" does not exist`, +// so a pass shows the code no longer depends on the column and a later +// migration can drop it without breaking this code. + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestIntegration_UserStoreWorksWithoutSaltColumn(t *testing.T) { + db := setupAuthTestDB(t) + ctx := t.Context() + _, err := db.Exec(ctx, "ALTER TABLE users DROP COLUMN salt") + require.NoError(t, err) + store := NewPostgresStore(db) + + // CreateAdminIfNone: the conditional bootstrap INSERT. + created, err := store.CreateAdminIfNone(ctx, newBootstrapCandidate("no-salt-admin@example.com")) + require.NoError(t, err) + require.True(t, created) + + // CreateUser, then every SELECT that goes through scanUser. + user := &User{Email: "no-salt@example.com", PasswordHash: "hash-1", Active: true, + GroupIDs: []string{DefaultPurchaserGroupID}} + require.NoError(t, store.CreateUser(ctx, user)) + + byID, err := store.GetUserByID(ctx, user.ID) + require.NoError(t, err) + assert.Equal(t, user.Email, byID.Email) + + byEmail, err := store.GetUserByEmail(ctx, user.Email) + require.NoError(t, err) + assert.Equal(t, user.ID, byEmail.ID) + + users, err := store.ListUsers(ctx) + require.NoError(t, err) + assert.Len(t, users, 2) + + // UpdateUserCredentials: the compare-and-set credential write. + changed := *byID + changed.PasswordHash = "hash-2" + changed.PasswordHistory = []string{"hash-1"} + require.NoError(t, store.UpdateUserCredentials(ctx, &changed, byID.Email, byID.PasswordHash)) + + // SetPasswordResetToken, GetUserByResetToken, CompletePasswordReset. + expiry := time.Now().Add(time.Hour) + withToken := changed + withToken.PasswordResetToken = "reset-token-hash-313" + withToken.PasswordResetExpiry = &expiry + require.NoError(t, store.SetPasswordResetToken(ctx, &withToken, nil)) + + byToken, err := store.GetUserByResetToken(ctx, withToken.PasswordResetToken) + require.NoError(t, err) + assert.Equal(t, user.ID, byToken.ID) + + reset := *byToken + reset.PasswordHash = "hash-3" + reset.PasswordHistory = []string{"hash-2", "hash-1"} + require.NoError(t, store.CompletePasswordReset(ctx, &reset, byToken.PasswordResetToken, byToken.PasswordHash)) + + final, err := store.GetUserByID(ctx, user.ID) + require.NoError(t, err) + assert.Equal(t, "hash-3", final.PasswordHash) + assert.Empty(t, final.PasswordResetToken) +} diff --git a/internal/auth/store_postgres_pgxmock_test.go b/internal/auth/store_postgres_pgxmock_test.go index 6e679d578..7c9b32a02 100644 --- a/internal/auth/store_postgres_pgxmock_test.go +++ b/internal/auth/store_postgres_pgxmock_test.go @@ -33,16 +33,16 @@ func newAuthPgxMock(t *testing.T) pgxmock.PgxPoolIface { } // createAdminIfNoneSQLRegex matches the full conditional bootstrap INSERT: -// 19 insert columns fed by SELECT $1..$19, guarded by the NOT EXISTS -// predicate bound to $20. +// 18 insert columns fed by SELECT $1..$18, guarded by the NOT EXISTS +// predicate bound to $19. The unused salt column is not written (issue #313). const createAdminIfNoneSQLRegex = `(?s)INSERT INTO users \(\s*` + - `id, email, password_hash, salt, group_ids, active,\s*` + + `id, email, password_hash, group_ids, active,\s*` + `mfa_enabled, mfa_secret, mfa_pending_secret, mfa_pending_secret_expires_at,\s*` + `mfa_recovery_codes, password_reset_token, password_reset_expiry,\s*` + `failed_login_attempts, locked_until, password_history,\s*` + `created_at, updated_at, last_login_at\s*\)\s*` + - `SELECT \$1, \$2, \$3, \$4, \$5, \$6, \$7, \$8, \$9, \$10, \$11, \$12, \$13, \$14, \$15, \$16, \$17, \$18, \$19\s*` + - `WHERE NOT EXISTS \(SELECT 1 FROM users WHERE group_ids @> ARRAY\[\$20::uuid\] AND active = true\)` + `SELECT \$1, \$2, \$3, \$4, \$5, \$6, \$7, \$8, \$9, \$10, \$11, \$12, \$13, \$14, \$15, \$16, \$17, \$18\s*` + + `WHERE NOT EXISTS \(SELECT 1 FROM users WHERE group_ids @> ARRAY\[\$19::uuid\] AND active = true\)` // bootstrapAdminUser returns a minimal valid first-admin candidate. func bootstrapAdminUser() *User { @@ -50,37 +50,35 @@ func bootstrapAdminUser() *User { ID: "11111111-2222-4333-8444-555555555555", Email: "bootstrap-admin@example.com", PasswordHash: "hash", - Salt: "salt", Active: true, } } -// createAdminArgs builds the 20 expected Exec arguments for CreateAdminIfNone: +// createAdminArgs builds the 19 expected Exec arguments for CreateAdminIfNone: // exact matches for the security-relevant ones (id, email, group_ids, active -// and the $20 predicate binding) and AnyArg for incidental fields whose +// and the $19 predicate binding) and AnyArg for incidental fields whose // values the function fills in (timestamps) or passes through untouched. func createAdminArgs(user *User, wantGroupIDs []string) []any { return []any{ user.ID, // $1 user.Email, // $2 user.PasswordHash, // $3 - user.Salt, // $4 - wantGroupIDs, // $5 - must contain the Administrators group - user.Active, // $6 - user.MFAEnabled, // $7 - pgxmock.AnyArg(), // $8 mfa_secret - pgxmock.AnyArg(), // $9 mfa_pending_secret - pgxmock.AnyArg(), // $10 mfa_pending_secret_expires_at - pgxmock.AnyArg(), // $11 mfa_recovery_codes - pgxmock.AnyArg(), // $12 password_reset_token - pgxmock.AnyArg(), // $13 password_reset_expiry - user.FailedLoginAttempts, // $14 - pgxmock.AnyArg(), // $15 locked_until - pgxmock.AnyArg(), // $16 password_history - pgxmock.AnyArg(), // $17 created_at (set inside) - pgxmock.AnyArg(), // $18 updated_at (set inside) - pgxmock.AnyArg(), // $19 last_login_at - DefaultAdminGroupID, // $20 NOT EXISTS predicate binding + wantGroupIDs, // $4 - must contain the Administrators group + user.Active, // $5 + user.MFAEnabled, // $6 + pgxmock.AnyArg(), // $7 mfa_secret + pgxmock.AnyArg(), // $8 mfa_pending_secret + pgxmock.AnyArg(), // $9 mfa_pending_secret_expires_at + pgxmock.AnyArg(), // $10 mfa_recovery_codes + pgxmock.AnyArg(), // $11 password_reset_token + pgxmock.AnyArg(), // $12 password_reset_expiry + user.FailedLoginAttempts, // $13 + pgxmock.AnyArg(), // $14 locked_until + pgxmock.AnyArg(), // $15 password_history + pgxmock.AnyArg(), // $16 created_at (set inside) + pgxmock.AnyArg(), // $17 updated_at (set inside) + pgxmock.AnyArg(), // $18 last_login_at + DefaultAdminGroupID, // $19 NOT EXISTS predicate binding } } @@ -314,7 +312,7 @@ func TestPGXMock_CountGroupMembers_QueryError(t *testing.T) { // userColumns matches the SELECT column order in ListUsers / scanUser. var userColumns = []string{ - "id", "email", "password_hash", "salt", "group_ids", "active", + "id", "email", "password_hash", "group_ids", "active", "mfa_enabled", "mfa_secret", "mfa_pending_secret", "mfa_pending_secret_expires_at", "mfa_recovery_codes", "password_reset_token", "password_reset_expiry", "failed_login_attempts", "locked_until", "password_history", @@ -331,7 +329,7 @@ func TestPGXMock_ListUsers_Success(t *testing.T) { deactivated := time.Date(2026, 3, 4, 5, 6, 7, 0, time.UTC) rows := pgxmock.NewRows(userColumns). AddRow( - "user-1", "admin@example.com", "hash1", "salt1", + "user-1", "admin@example.com", "hash1", []string{DefaultAdminGroupID}, true, true, "mfa-secret", nil, nil, []string{"code1"}, nil, nil, @@ -339,7 +337,7 @@ func TestPGXMock_ListUsers_Success(t *testing.T) { created, created, lastLogin, nil, int64(4), ). AddRow( - "user-2", "user@example.com", "hash2", "salt2", + "user-2", "user@example.com", "hash2", []string{}, false, false, nil, nil, nil, []string{}, nil, nil, @@ -347,7 +345,7 @@ func TestPGXMock_ListUsers_Success(t *testing.T) { created, created, nil, deactivated, int64(0), ) - mock.ExpectQuery(`(?s)SELECT id, email, password_hash, salt, group_ids, active,.*FROM users\s+ORDER BY created_at DESC\s+LIMIT 10000`). + mock.ExpectQuery(`(?s)SELECT id, email, password_hash, group_ids, active,.*FROM users\s+ORDER BY created_at DESC\s+LIMIT 10000`). WillReturnRows(rows) users, err := store.ListUsers(context.Background()) @@ -407,7 +405,7 @@ func TestPGXMock_ListUsers_RowError(t *testing.T) { created := time.Date(2026, 1, 2, 3, 4, 5, 0, time.UTC) rows := pgxmock.NewRows(userColumns). AddRow( - "user-1", "admin@example.com", "hash1", "salt1", + "user-1", "admin@example.com", "hash1", []string{}, true, false, nil, nil, nil, []string{}, nil, nil, diff --git a/internal/auth/store_postgres_test.go b/internal/auth/store_postgres_test.go index b270bc97a..de8a96e8d 100644 --- a/internal/auth/store_postgres_test.go +++ b/internal/auth/store_postgres_test.go @@ -66,8 +66,9 @@ func (m *MockRow) Scan(dest ...interface{}) error { // Helper function to create a mock row that returns a user // The scan order matches scanUser() in store_postgres.go (issue #89 added -// deactivated_at, so there are now 20 destinations): -// id, email, password_hash, salt, group_ids, active, +// deactivated_at; the unused salt column was dropped from the SELECT in +// issue #313, so the first 19 destinations are populated here): +// id, email, password_hash, group_ids, active, // mfa_enabled, mfa_secret (NullString), mfa_pending_secret (NullString), // mfa_pending_secret_expires_at (NullTime), mfa_recovery_codes ([]string), // reset_token (NullString), reset_expiry (NullTime), @@ -77,67 +78,66 @@ func createMockRowWithUser(user *User) *MockRow { return &MockRow{ scanFunc: func(dest ...interface{}) error { // Populate destination pointers with user data - if len(dest) >= 20 { + if len(dest) >= 19 { *dest[0].(*string) = user.ID *dest[1].(*string) = user.Email *dest[2].(*string) = user.PasswordHash - *dest[3].(*string) = user.Salt - *dest[4].(*[]string) = user.GroupIDs - *dest[5].(*bool) = user.Active - *dest[6].(*bool) = user.MFAEnabled - // dest[7] is sql.NullString for MFASecret + *dest[3].(*[]string) = user.GroupIDs + *dest[4].(*bool) = user.Active + *dest[5].(*bool) = user.MFAEnabled + // dest[6] is sql.NullString for MFASecret if user.MFASecret != "" { - *dest[7].(*sql.NullString) = sql.NullString{String: user.MFASecret, Valid: true} + *dest[6].(*sql.NullString) = sql.NullString{String: user.MFASecret, Valid: true} } else { - *dest[7].(*sql.NullString) = sql.NullString{Valid: false} + *dest[6].(*sql.NullString) = sql.NullString{Valid: false} } - // dest[8] is sql.NullString for MFAPendingSecret + // dest[7] is sql.NullString for MFAPendingSecret if user.MFAPendingSecret != "" { - *dest[8].(*sql.NullString) = sql.NullString{String: user.MFAPendingSecret, Valid: true} + *dest[7].(*sql.NullString) = sql.NullString{String: user.MFAPendingSecret, Valid: true} } else { - *dest[8].(*sql.NullString) = sql.NullString{Valid: false} + *dest[7].(*sql.NullString) = sql.NullString{Valid: false} } - // dest[9] is sql.NullTime for MFAPendingSecretExpiresAt + // dest[8] is sql.NullTime for MFAPendingSecretExpiresAt if user.MFAPendingSecretExpiresAt != nil { - *dest[9].(*sql.NullTime) = sql.NullTime{Time: *user.MFAPendingSecretExpiresAt, Valid: true} + *dest[8].(*sql.NullTime) = sql.NullTime{Time: *user.MFAPendingSecretExpiresAt, Valid: true} } else { - *dest[9].(*sql.NullTime) = sql.NullTime{Valid: false} + *dest[8].(*sql.NullTime) = sql.NullTime{Valid: false} } - // dest[10] is []string for MFARecoveryCodes - *dest[10].(*[]string) = user.MFARecoveryCodes - // dest[11] is sql.NullString for PasswordResetToken + // dest[9] is []string for MFARecoveryCodes + *dest[9].(*[]string) = user.MFARecoveryCodes + // dest[10] is sql.NullString for PasswordResetToken if user.PasswordResetToken != "" { - *dest[11].(*sql.NullString) = sql.NullString{String: user.PasswordResetToken, Valid: true} + *dest[10].(*sql.NullString) = sql.NullString{String: user.PasswordResetToken, Valid: true} } else { - *dest[11].(*sql.NullString) = sql.NullString{Valid: false} + *dest[10].(*sql.NullString) = sql.NullString{Valid: false} } - // dest[12] is sql.NullTime for PasswordResetExpiry + // dest[11] is sql.NullTime for PasswordResetExpiry if user.PasswordResetExpiry != nil { - *dest[12].(*sql.NullTime) = sql.NullTime{Time: *user.PasswordResetExpiry, Valid: true} + *dest[11].(*sql.NullTime) = sql.NullTime{Time: *user.PasswordResetExpiry, Valid: true} } else { - *dest[12].(*sql.NullTime) = sql.NullTime{Valid: false} + *dest[11].(*sql.NullTime) = sql.NullTime{Valid: false} } - *dest[13].(*int) = user.FailedLoginAttempts - // dest[14] is sql.NullTime for LockedUntil + *dest[12].(*int) = user.FailedLoginAttempts + // dest[13] is sql.NullTime for LockedUntil if user.LockedUntil != nil { - *dest[14].(*sql.NullTime) = sql.NullTime{Time: *user.LockedUntil, Valid: true} + *dest[13].(*sql.NullTime) = sql.NullTime{Time: *user.LockedUntil, Valid: true} } else { - *dest[14].(*sql.NullTime) = sql.NullTime{Valid: false} + *dest[13].(*sql.NullTime) = sql.NullTime{Valid: false} } - *dest[15].(*[]string) = user.PasswordHistory - *dest[16].(*time.Time) = user.CreatedAt - *dest[17].(*time.Time) = user.UpdatedAt - // dest[18] is sql.NullTime for LastLoginAt + *dest[14].(*[]string) = user.PasswordHistory + *dest[15].(*time.Time) = user.CreatedAt + *dest[16].(*time.Time) = user.UpdatedAt + // dest[17] is sql.NullTime for LastLoginAt if user.LastLoginAt != nil { - *dest[18].(*sql.NullTime) = sql.NullTime{Time: *user.LastLoginAt, Valid: true} + *dest[17].(*sql.NullTime) = sql.NullTime{Time: *user.LastLoginAt, Valid: true} } else { - *dest[18].(*sql.NullTime) = sql.NullTime{Valid: false} + *dest[17].(*sql.NullTime) = sql.NullTime{Valid: false} } - // dest[19] is sql.NullTime for DeactivatedAt + // dest[18] is sql.NullTime for DeactivatedAt if user.DeactivatedAt != nil { - *dest[19].(*sql.NullTime) = sql.NullTime{Time: *user.DeactivatedAt, Valid: true} + *dest[18].(*sql.NullTime) = sql.NullTime{Time: *user.DeactivatedAt, Valid: true} } else { - *dest[19].(*sql.NullTime) = sql.NullTime{Valid: false} + *dest[18].(*sql.NullTime) = sql.NullTime{Valid: false} } } return nil diff --git a/internal/auth/test_helpers.go b/internal/auth/test_helpers.go index abaecae17..ef1e030b6 100644 --- a/internal/auth/test_helpers.go +++ b/internal/auth/test_helpers.go @@ -370,7 +370,6 @@ func createTestUser(t *testing.T, password string) *User { ID: "user-123", Email: "test@example.com", PasswordHash: hash, - Salt: "", // Not used anymore, bcrypt handles salting GroupIDs: []string{DefaultAdminGroupID}, Active: true, CreatedAt: time.Now(), diff --git a/internal/auth/types.go b/internal/auth/types.go index 5fd07ac5a..14ef028cf 100644 --- a/internal/auth/types.go +++ b/internal/auth/types.go @@ -19,7 +19,6 @@ type User struct { DeactivatedAt *time.Time `json:"deactivated_at,omitempty" dynamodbav:"DeactivatedAt"` MFASecret string `json:"-" dynamodbav:"MFASecret,omitempty"` PasswordResetToken string `json:"-" dynamodbav:"PasswordResetToken,omitempty"` - Salt string `json:"-" dynamodbav:"Salt"` ID string `json:"id" dynamodbav:"PK"` MFAPendingSecret string `json:"-" dynamodbav:"MFAPendingSecret,omitempty"` PasswordHash string `json:"-" dynamodbav:"PasswordHash"` diff --git a/internal/database/postgres/migrations/ensure_admin_user_no_salt_test.go b/internal/database/postgres/migrations/ensure_admin_user_no_salt_test.go new file mode 100644 index 000000000..260814d02 --- /dev/null +++ b/internal/database/postgres/migrations/ensure_admin_user_no_salt_test.go @@ -0,0 +1,44 @@ +//go:build integration + +package migrations_test + +// Issue #313: the bootstrap admin INSERTs run after every migration on each +// boot, so they must not name users.salt, a leftover column that a later +// migration is meant to drop. This drops the column from a migrated database, +// then reruns RunMigrations (the migrations are already applied, so only the +// bootstrap runs) on both bootstrap paths. + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/migrations" + "github.com/LeanerCloud/cloud-commitments-platform/internal/database/postgres/testhelpers" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestEnsureAdminUser_WithoutSaltColumn(t *testing.T) { + t.Parallel() + ctx := context.Background() + migrationsPath := getMigrationsPath() + + for _, tc := range []struct{ name, email, password string }{ + {"no password", "no-salt-invite@test.example", ""}, + {"with password", "no-salt-admin@test.example", "TestPass!9aF"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + pool := testhelpers.RequireMigratedDatabase(ctx, t, migrationsPath).DB.Pool() + _, err := pool.Exec(ctx, "ALTER TABLE users DROP COLUMN salt") + require.NoError(t, err) + + require.NoError(t, migrations.RunMigrations(ctx, pool, migrationsPath, tc.email, tc.password)) + + assert.Equal(t, []string{defaultAdminGroupIDTest}, queryAdminGroupIDs(t, ctx, pool, tc.email)) + var active bool + require.NoError(t, pool.QueryRow(ctx, "SELECT active FROM users WHERE email = $1", tc.email).Scan(&active)) + assert.Equal(t, tc.password != "", active) + }) + } +} diff --git a/internal/database/postgres/migrations/migrate.go b/internal/database/postgres/migrations/migrate.go index 033545fe9..53a5edfea 100644 --- a/internal/database/postgres/migrations/migrate.go +++ b/internal/database/postgres/migrations/migrate.go @@ -167,9 +167,9 @@ func ensureAdminUser(ctx context.Context, pool *pgxpool.Pool, email, password st // The `role` column was removed by migration 000057 (issue #945). result, err := pool.Exec(ctx, ` INSERT INTO users ( - id, email, password_hash, salt, active, group_ids, created_at, updated_at + id, email, password_hash, active, group_ids, created_at, updated_at ) VALUES ( - gen_random_uuid(), $1, '', '', false, ARRAY[$2]::UUID[], NOW(), NOW() + gen_random_uuid(), $1, '', false, ARRAY[$2]::UUID[], NOW(), NOW() ) ON CONFLICT (email) DO NOTHING `, email, defaultAdminGroupID) @@ -220,9 +220,9 @@ func ensureAdminUserWithPassword(ctx context.Context, pool *pgxpool.Pool, email, // The `role` column was removed by migration 000057 (issue #945). result, err := pool.Exec(ctx, ` INSERT INTO users ( - id, email, password_hash, salt, active, group_ids, created_at, updated_at + id, email, password_hash, active, group_ids, created_at, updated_at ) VALUES ( - gen_random_uuid(), $1, $2, '', true, ARRAY[$3]::UUID[], NOW(), NOW() + gen_random_uuid(), $1, $2, true, ARRAY[$3]::UUID[], NOW(), NOW() ) ON CONFLICT (email) DO UPDATE SET password_hash = $2, active = true, updated_at = NOW()