Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions internal/auth/service_credentials_db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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++
})
Expand Down
14 changes: 7 additions & 7 deletions internal/auth/service_login_db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
2 changes: 0 additions & 2 deletions internal/auth/service_password.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 0 additions & 4 deletions internal/auth/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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
Expand Down
3 changes: 0 additions & 3 deletions internal/auth/service_user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
Expand Down
22 changes: 10 additions & 12 deletions internal/auth/store_postgres.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
10 changes: 5 additions & 5 deletions internal/auth/store_postgres_credentials.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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)
}
Expand Down
1 change: 0 additions & 1 deletion internal/auth/store_postgres_db_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,6 @@ func newBootstrapCandidate(email string) *User {
return &User{
Email: email,
PasswordHash: "test-hash",
Salt: "test-salt",
Active: true,
}
}
Expand Down
75 changes: 75 additions & 0 deletions internal/auth/store_postgres_no_salt_db_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
Loading
Loading