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()