diff --git a/go.mod b/go.mod index 5fb3a2f61..abcf4fb6c 100644 --- a/go.mod +++ b/go.mod @@ -59,7 +59,7 @@ require ( go.opentelemetry.io/otel/metric v1.45.0 // indirect go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.58.0 - golang.org/x/net v0.61.0 // indirect + golang.org/x/net v0.62.0 // indirect golang.org/x/oauth2 v0.37.0 golang.org/x/sync v0.24.0 golang.org/x/sys v0.49.0 // indirect diff --git a/go.sum b/go.sum index 06deac0b9..83df52e66 100644 --- a/go.sum +++ b/go.sum @@ -367,8 +367,8 @@ golang.org/x/crypto v0.58.0 h1:COkYLr4k7nDI4r6QtMML2AfzeIswWHAJthLS6K/M/54= golang.org/x/crypto v0.58.0/go.mod h1:Xh+kl5A+M0gMiWDytH2Hhr5Z7WbNCpv/eB/EFgfSk+w= golang.org/x/mod v0.42.0 h1:ICeL3t2L30ljSPHHxDnR4fqVcBt/zR2zJAb9juavmQg= golang.org/x/mod v0.42.0/go.mod h1:u5fzIrRInkL8MaO1cbRkUHT64v303YIT5KEqpNr7jRs= -golang.org/x/net v0.61.0 h1:RR+6j/BTBCrggOLugxa+66R8FvoN6MHlzDlZIbN9YFw= -golang.org/x/net v0.61.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= +golang.org/x/net v0.62.0 h1:18ELalfM5+XNRwzM2nYqXsZQ9ByS1uhj2O/U7+5jqRY= +golang.org/x/net v0.62.0/go.mod h1:WgDCOTH8iwtB66T4/GfI94AHo/hhdb7Hah9NDseQfiI= golang.org/x/oauth2 v0.37.0 h1:JUlcxA8oAtauLfiH8FX2/FkAWHAdi0QtGCGc+hofE98= golang.org/x/oauth2 v0.37.0/go.mod h1:IxwZNxUULJmpBFf9K/9NTMSIfZZuvuTy1gGxhigP/58= golang.org/x/sync v0.24.0 h1:iqiDrGoDgyuvAKrhLQthUEzU/R378N78gXbBjxed7oA= diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index a81b1a50c..c4a8cd3aa 100644 --- a/internal/api/coverage_gaps_test.go +++ b/internal/api/coverage_gaps_test.go @@ -853,7 +853,6 @@ func (s *stubEmailNotifier) SendPurchaseFailedNotification(_ context.Context, _ return nil } func (s *stubEmailNotifier) SendPasswordResetEmail(_ context.Context, _, _ string) error { return nil } -func (s *stubEmailNotifier) SendWelcomeEmail(_ context.Context, _, _, _ string) error { return nil } func (s *stubEmailNotifier) SendUserInviteEmail(_ context.Context, _, _ string) error { return nil } func (s *stubEmailNotifier) SendRIExchangePendingApproval(_ context.Context, _ email.RIExchangeNotificationData) error { return nil diff --git a/internal/auth/service.go b/internal/auth/service.go index afef57efe..9e2be3f13 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -23,7 +23,7 @@ const ( PasswordResetExpiry = 1 * time.Hour // PasswordSetupExpiry is how long an invited user has to set their - // initial password via the link in the welcome email. Longer than + // initial password via the link in the invite email. Longer than // PasswordResetExpiry because invites typically wait in an inbox // before the recipient acts on them. PasswordSetupExpiry = 7 * 24 * time.Hour diff --git a/internal/auth/service_reset_invalidation_db_test.go b/internal/auth/service_reset_invalidation_db_test.go index afd65d12b..ffdc15f53 100644 --- a/internal/auth/service_reset_invalidation_db_test.go +++ b/internal/auth/service_reset_invalidation_db_test.go @@ -90,9 +90,6 @@ func (m *inviteMailSink) SendPasswordResetEmail(_ context.Context, _, resetURL s m.resetURLs = append(m.resetURLs, resetURL) return nil } -func (m *inviteMailSink) SendWelcomeEmail(context.Context, string, string, string) error { - return nil -} func (m *inviteMailSink) SendUserInviteEmail(context.Context, string, string) error { return errors.New("smtp unavailable") } diff --git a/internal/auth/service_user.go b/internal/auth/service_user.go index 05a325e38..0f9b519c5 100644 --- a/internal/auth/service_user.go +++ b/internal/auth/service_user.go @@ -117,7 +117,7 @@ func (s *Service) CheckAdminExists(ctx context.Context) (bool, error) { // the user record. Returns an error if any field is invalid. // // When req.Password is empty the user is invited and will set their own -// password via the welcome email link, so the password validator is skipped. +// password via the invite email link, so the password validator is skipped. func (s *Service) validateCreateUserRequest(ctx context.Context, req CreateUserRequest) error { if _, err := mail.ParseAddress(req.Email); err != nil { return ErrInvalidEmail diff --git a/internal/auth/service_user_test.go b/internal/auth/service_user_test.go index a4d285a57..92ebdf8e7 100644 --- a/internal/auth/service_user_test.go +++ b/internal/auth/service_user_test.go @@ -256,10 +256,10 @@ func TestService_CreateUser(t *testing.T) { require.NotNil(t, captured) // Invited users land inactive and only flip to active after they - // set their password through the welcome-link flow. + // set their password through the invite-link flow. assert.False(t, captured.Active) - // A setup token must be stored so the welcome-link flow can find + // A setup token must be stored so the invite-link flow can find // the user; the placeholder hash must not be empty so the // password_hash NOT NULL constraint is satisfied and no client // input can match it. diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index fef94ecbd..9c0ac03e3 100644 --- a/internal/auth/store_postgres.go +++ b/internal/auth/store_postgres.go @@ -41,11 +41,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, 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, deactivated_at, password_version + SELECT ` + userSelectColumns + ` FROM users WHERE id = $1 ` @@ -56,11 +52,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, 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, deactivated_at, password_version + SELECT ` + userSelectColumns + ` FROM users WHERE email = $1 ` @@ -85,44 +77,11 @@ func (s *PostgresStore) CreateUser(ctx context.Context, user *User) error { user.UpdatedAt = now query := ` - INSERT INTO users ( - 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) + INSERT INTO users (` + userInsertColumns + `) + 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 - // stays at its default '{}' rather than NULL. The schema declares - // the column NOT NULL DEFAULT '{}' (migration 000052); passing a - // raw nil here triggers "violates not-null constraint" on insert. - recoveryCodes := user.MFARecoveryCodes - if recoveryCodes == nil { - recoveryCodes = []string{} - } - - _, err := s.db.Exec(ctx, query, - user.ID, - user.Email, - user.PasswordHash, - user.GroupIDs, - user.Active, - user.MFAEnabled, - user.MFASecret, - user.MFAPendingSecret, - user.MFAPendingSecretExpiresAt, - recoveryCodes, - user.PasswordResetToken, - user.PasswordResetExpiry, - user.FailedLoginAttempts, - user.LockedUntil, - user.PasswordHistory, - user.CreatedAt, - user.UpdatedAt, - user.LastLoginAt, - ) + _, err := s.db.Exec(ctx, query, userInsertArgs(user, user.GroupIDs)...) if err != nil { if isEmailDuplicateError(err) { @@ -160,11 +119,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, 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, deactivated_at, password_version + SELECT ` + userSelectColumns + ` FROM users ORDER BY created_at DESC LIMIT 10000 @@ -199,11 +154,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, 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, deactivated_at, password_version + SELECT ` + userSelectColumns + ` FROM users WHERE password_reset_token = $1 ` @@ -215,12 +166,61 @@ func (s *PostgresStore) GetUserByResetToken(ctx context.Context, token string) ( // HELPER FUNCTIONS // ========================================== +// userInsertColumns lists the users columns that CreateUser and +// CreateAdminIfNone write, in the order userInsertArgs returns their values. +// password_version and deactivated_at are left to their column defaults. +const userInsertColumns = `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` + +// userSelectColumns lists the users columns every user SELECT reads, in the +// order scanUser scans them. Every query that feeds scanUser must select +// exactly this list: several neighbors share a Go type (mfa_secret and +// mfa_pending_secret, or the timestamps), so a hand-copied list in a +// different order would scan one into the other without any error (#313). +const userSelectColumns = userInsertColumns + `, deactivated_at, password_version` + +// userInsertArgs returns the values for userInsertColumns, in the same order. +// groupIDs is passed separately because CreateAdminIfNone forces the +// Administrators group onto the row it writes. +func userInsertArgs(user *User, groupIDs []string) []any { + // Substitute an empty slice for nil so the Postgres TEXT[] column + // stays at its default '{}' rather than NULL. The schema declares + // the column NOT NULL DEFAULT '{}' (migration 000052); passing a + // raw nil here triggers "violates not-null constraint" on insert. + recoveryCodes := user.MFARecoveryCodes + if recoveryCodes == nil { + recoveryCodes = []string{} + } + return []any{ + user.ID, + user.Email, + user.PasswordHash, + groupIDs, + user.Active, + user.MFAEnabled, + user.MFASecret, + user.MFAPendingSecret, + user.MFAPendingSecretExpiresAt, + recoveryCodes, + user.PasswordResetToken, + user.PasswordResetExpiry, + user.FailedLoginAttempts, + user.LockedUntil, + user.PasswordHistory, + user.CreatedAt, + user.UpdatedAt, + user.LastLoginAt, + } +} + // Scanner interface for both Row and Rows. type Scanner interface { Scan(dest ...any) error } -// scanUser scans a user from a database row. func nullTimePtr(t sql.NullTime) *time.Time { if !t.Valid { return nil @@ -228,6 +228,7 @@ func nullTimePtr(t sql.NullTime) *time.Time { return &t.Time } +// scanUser scans a user from a row that selected userSelectColumns. func (s *PostgresStore) scanUser(scanner Scanner) (*User, error) { var user User var groupIDs []string diff --git a/internal/auth/store_postgres_admin.go b/internal/auth/store_postgres_admin.go index 2eaa152dc..98241c223 100644 --- a/internal/auth/store_postgres_admin.go +++ b/internal/auth/store_postgres_admin.go @@ -178,25 +178,14 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool // Administrators group would not satisfy the membership predicate, silently // breaking that contract. query := ` - INSERT INTO users ( - 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 - ) + INSERT INTO users (` + userInsertColumns + `) 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 - // Administrators group onto the bootstrap admin so it satisfies the - // membership predicate above (deduped to avoid a doubled entry if the - // caller already supplied it). - recoveryCodes := user.MFARecoveryCodes - if recoveryCodes == nil { - recoveryCodes = []string{} - } + // Force the Administrators group onto the bootstrap admin so it + // satisfies the membership predicate above (deduped to avoid a doubled + // entry if the caller already supplied it). groupIDs := user.GroupIDs if !containsGroup(groupIDs, DefaultAdminGroupID) { groupIDs = append(append([]string(nil), groupIDs...), DefaultAdminGroupID) @@ -221,15 +210,8 @@ func (s *PostgresStore) CreateAdminIfNone(ctx context.Context, user *User) (bool return false, fmt.Errorf("failed to acquire admin bootstrap lock: %w", err) } - tag, err := tx.Exec(ctx, query, - user.ID, user.Email, user.PasswordHash, - groupIDs, user.Active, user.MFAEnabled, user.MFASecret, - user.MFAPendingSecret, user.MFAPendingSecretExpiresAt, recoveryCodes, - user.PasswordResetToken, user.PasswordResetExpiry, - user.FailedLoginAttempts, user.LockedUntil, user.PasswordHistory, - user.CreatedAt, user.UpdatedAt, user.LastLoginAt, - DefaultAdminGroupID, - ) + args := append(userInsertArgs(user, groupIDs), DefaultAdminGroupID) + tag, err := tx.Exec(ctx, query, args...) if err != nil { // Email uniqueness can still fire if the bootstrap caller reuses // an email already held by a non-admin user. Surface as the same diff --git a/internal/auth/store_postgres_user_columns_db_test.go b/internal/auth/store_postgres_user_columns_db_test.go new file mode 100644 index 000000000..cd16c7f72 --- /dev/null +++ b/internal/auth/store_postgres_user_columns_db_test.go @@ -0,0 +1,146 @@ +//go:build integration + +package auth + +// Issue #313: round-trips every users column through the store on a real, +// migrated schema. Rows are written and read back with SQL that names its +// columns explicitly (in an order unrelated to userInsertColumns and +// userSelectColumns), so the check does not lean on the constants under +// test. A swap of two same-typed columns in either list, in scanUser or in +// userInsertArgs puts a value in the wrong place and fails here. + +import ( + "context" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/database" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// utcPtr normalizes a nullable timestamp read back from Postgres. +func utcPtr(t *time.Time) *time.Time { + if t == nil { + return nil + } + u := t.UTC() + return &u +} + +// normalizeUserTimes puts every timestamp in UTC at Postgres's microsecond +// precision so a struct comparison checks values, not locations. +func normalizeUserTimes(u *User) *User { + c := *u + c.CreatedAt = c.CreatedAt.UTC().Truncate(time.Microsecond) + c.UpdatedAt = c.UpdatedAt.UTC().Truncate(time.Microsecond) + c.MFAPendingSecretExpiresAt = utcPtr(c.MFAPendingSecretExpiresAt) + c.PasswordResetExpiry = utcPtr(c.PasswordResetExpiry) + c.LockedUntil = utcPtr(c.LockedUntil) + c.LastLoginAt = utcPtr(c.LastLoginAt) + c.DeactivatedAt = utcPtr(c.DeactivatedAt) + return &c +} + +// insertUserByName writes u with an INSERT that names every column itself. +func insertUserByName(t *testing.T, db *database.Connection, u *User) { + t.Helper() + _, err := db.Exec(context.Background(), ` + INSERT INTO users ( + password_version, deactivated_at, last_login_at, updated_at, created_at, + password_history, locked_until, failed_login_attempts, + password_reset_expiry, password_reset_token, mfa_recovery_codes, + mfa_pending_secret_expires_at, mfa_pending_secret, mfa_secret, + mfa_enabled, active, group_ids, password_hash, email, id + ) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14, $15, $16, $17, $18, $19, $20)`, + u.PasswordVersion, u.DeactivatedAt, u.LastLoginAt, u.UpdatedAt, u.CreatedAt, + u.PasswordHistory, u.LockedUntil, u.FailedLoginAttempts, + u.PasswordResetExpiry, u.PasswordResetToken, u.MFARecoveryCodes, + u.MFAPendingSecretExpiresAt, u.MFAPendingSecret, u.MFASecret, + u.MFAEnabled, u.Active, u.GroupIDs, u.PasswordHash, u.Email, u.ID, + ) + require.NoError(t, err) +} + +// readUserByName reads the row for id with a SELECT that names every column +// itself, independent of userSelectColumns and scanUser. +func readUserByName(t *testing.T, db *database.Connection, id string) *User { + t.Helper() + var u User + var mfaSecret, mfaPending, resetToken *string + err := db.QueryRow(context.Background(), ` + SELECT password_version, deactivated_at, last_login_at, updated_at, created_at, + password_history, locked_until, failed_login_attempts, + password_reset_expiry, password_reset_token, mfa_recovery_codes, + mfa_pending_secret_expires_at, mfa_pending_secret, mfa_secret, + mfa_enabled, active, group_ids::text[], password_hash, email, id::text + FROM users WHERE id = $1`, id).Scan( + &u.PasswordVersion, &u.DeactivatedAt, &u.LastLoginAt, &u.UpdatedAt, &u.CreatedAt, + &u.PasswordHistory, &u.LockedUntil, &u.FailedLoginAttempts, + &u.PasswordResetExpiry, &resetToken, &u.MFARecoveryCodes, + &u.MFAPendingSecretExpiresAt, &mfaPending, &mfaSecret, + &u.MFAEnabled, &u.Active, &u.GroupIDs, &u.PasswordHash, &u.Email, &u.ID, + ) + require.NoError(t, err) + for dst, src := range map[*string]*string{&u.MFASecret: mfaSecret, &u.MFAPendingSecret: mfaPending, &u.PasswordResetToken: resetToken} { + if src != nil { + *dst = *src + } + } + return &u +} + +func TestIntegration_UserColumns_ScanUserRoundTripsEveryColumn(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + ctx := context.Background() + want := distinctColumnsUser() + insertUserByName(t, db, want) + + byID, err := store.GetUserByID(ctx, want.ID) + require.NoError(t, err) + assert.Equal(t, want, normalizeUserTimes(byID), "GetUserByID") + + byEmail, err := store.GetUserByEmail(ctx, want.Email) + require.NoError(t, err) + assert.Equal(t, want, normalizeUserTimes(byEmail), "GetUserByEmail") + + byToken, err := store.GetUserByResetToken(ctx, want.PasswordResetToken) + require.NoError(t, err) + assert.Equal(t, want, normalizeUserTimes(byToken), "GetUserByResetToken") + + users, err := store.ListUsers(ctx) + require.NoError(t, err) + require.Len(t, users, 1) + assert.Equal(t, want, normalizeUserTimes(&users[0]), "ListUsers") +} + +// insertedByStore is what a store INSERT of u must leave in the row: +// every written field as given, deactivated_at and password_version at +// their column defaults. +func insertedByStore(u *User) *User { + c := *u + c.DeactivatedAt = nil + c.PasswordVersion = 0 + return normalizeUserTimes(&c) +} + +func TestIntegration_UserColumns_CreateUserWritesEveryColumn(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + user := distinctColumnsUser() + require.NoError(t, store.CreateUser(context.Background(), user)) + + assert.Equal(t, insertedByStore(user), normalizeUserTimes(readUserByName(t, db, user.ID))) +} + +func TestIntegration_UserColumns_CreateAdminIfNoneWritesEveryColumn(t *testing.T) { + db := setupAuthTestDB(t) + store := NewPostgresStore(db) + user := distinctColumnsUser() // already carries the Administrators group + created, err := store.CreateAdminIfNone(context.Background(), user) + require.NoError(t, err) + require.True(t, created) + + assert.Equal(t, insertedByStore(user), normalizeUserTimes(readUserByName(t, db, user.ID))) +} diff --git a/internal/auth/store_postgres_user_columns_test.go b/internal/auth/store_postgres_user_columns_test.go new file mode 100644 index 000000000..0385125be --- /dev/null +++ b/internal/auth/store_postgres_user_columns_test.go @@ -0,0 +1,231 @@ +package auth + +// Issue #313: every user SELECT reads userSelectColumns and scanUser scans +// it positionally; both INSERTs write userInsertColumns from +// userInsertArgs. Several neighboring columns share a Go type (mfa_secret +// and mfa_pending_secret, the three []string columns, the timestamps), so a +// list and its scan or argument order can drift apart without any error. +// These tests key every value by column name, so a reordering on either side +// puts a value into the wrong field and fails. The integration variant in +// store_postgres_user_columns_db_test.go repeats the round trip on a real, +// migrated schema. + +import ( + "context" + "regexp" + "strings" + "testing" + "time" + + "github.com/pashagolub/pgxmock/v5" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// splitColumnList turns a SQL column list into trimmed column names. +func splitColumnList(list string) []string { + parts := strings.Split(list, ",") + names := make([]string, 0, len(parts)) + for _, p := range parts { + names = append(names, strings.TrimSpace(p)) + } + return names +} + +// columnFixtureTimes returns seven distinct timestamps, one per timestamp +// column, so a swap between any two of them is visible. +func columnFixtureTimes() [7]time.Time { + base := time.Date(2026, 3, 1, 10, 0, 0, 0, time.UTC) + var ts [7]time.Time + for i := range ts { + ts[i] = base.Add(time.Duration(i+1) * time.Hour) + } + return ts +} + +// distinctColumnsUser returns a user whose every field holds a value no other +// same-typed field holds. Active and MFAEnabled differ so a swap of the two +// booleans shows too. +func distinctColumnsUser() *User { + ts := columnFixtureTimes() + return &User{ + ID: "0c5e3a1d-6f0b-4b7e-9a51-3d2f6c7b8e90", + Email: "columns-313@example.com", + PasswordHash: "hash-current", + GroupIDs: []string{DefaultAdminGroupID, DefaultPurchaserGroupID}, + Active: false, + MFAEnabled: true, + MFASecret: "mfa-secret-enrolled", + MFAPendingSecret: "mfa-secret-pending", + MFAPendingSecretExpiresAt: &ts[0], + MFARecoveryCodes: []string{"recovery-a", "recovery-b"}, + PasswordResetToken: "reset-token-hash", + PasswordResetExpiry: &ts[1], + FailedLoginAttempts: 3, + LockedUntil: &ts[2], + PasswordHistory: []string{"hash-old-1", "hash-old-2", "hash-old-3"}, + CreatedAt: ts[3], + UpdatedAt: ts[4], + LastLoginAt: &ts[5], + DeactivatedAt: &ts[6], + PasswordVersion: 9, + } +} + +// userRowByColumn maps each users column to the value a row for u would +// carry, keyed by name rather than position. +func userRowByColumn(u *User) map[string]any { + return map[string]any{ + "id": u.ID, + "email": u.Email, + "password_hash": u.PasswordHash, + "group_ids": u.GroupIDs, + "active": u.Active, + "mfa_enabled": u.MFAEnabled, + "mfa_secret": u.MFASecret, + "mfa_pending_secret": u.MFAPendingSecret, + "mfa_pending_secret_expires_at": *u.MFAPendingSecretExpiresAt, + "mfa_recovery_codes": u.MFARecoveryCodes, + "password_reset_token": u.PasswordResetToken, + "password_reset_expiry": *u.PasswordResetExpiry, + "failed_login_attempts": u.FailedLoginAttempts, + "locked_until": *u.LockedUntil, + "password_history": u.PasswordHistory, + "created_at": u.CreatedAt, + "updated_at": u.UpdatedAt, + "last_login_at": *u.LastLoginAt, + "deactivated_at": *u.DeactivatedAt, + "password_version": u.PasswordVersion, + } +} + +// userInsertArgsByColumn maps each insert column to the Exec argument the +// store must bind for u. created_at and updated_at are set by the store +// itself, so they are matched as "a time.Time inside the call window". +func userInsertArgsByColumn(u *User, after time.Time) map[string]any { + recent := pgxmock.ArgumentFunc(func(v any) bool { + ts, ok := v.(time.Time) + return ok && !ts.Before(after) && !ts.After(time.Now()) + }) + return map[string]any{ + "id": u.ID, + "email": u.Email, + "password_hash": u.PasswordHash, + "group_ids": u.GroupIDs, + "active": u.Active, + "mfa_enabled": u.MFAEnabled, + "mfa_secret": u.MFASecret, + "mfa_pending_secret": u.MFAPendingSecret, + "mfa_pending_secret_expires_at": u.MFAPendingSecretExpiresAt, + "mfa_recovery_codes": u.MFARecoveryCodes, + "password_reset_token": u.PasswordResetToken, + "password_reset_expiry": u.PasswordResetExpiry, + "failed_login_attempts": u.FailedLoginAttempts, + "locked_until": u.LockedUntil, + "password_history": u.PasswordHistory, + "created_at": recent, + "updated_at": recent, + "last_login_at": u.LastLoginAt, + } +} + +// valuesInListOrder looks every column of list up in byColumn and returns the +// values in the list's order. It fails the test on an unknown or duplicated +// column, or when byColumn has a column the list leaves out. +func valuesInListOrder(t *testing.T, list string, byColumn map[string]any) []any { + t.Helper() + names := splitColumnList(list) + seen := make(map[string]bool, len(names)) + values := make([]any, 0, len(names)) + for _, name := range names { + require.False(t, seen[name], "column %q listed twice", name) + seen[name] = true + v, ok := byColumn[name] + require.True(t, ok, "column %q has no fixture value", name) + values = append(values, v) + } + require.Len(t, names, len(byColumn), "column list and fixture disagree on the column set") + return values +} + +func TestUserSelectColumns_ScanUserReadsEveryColumnIntoItsField(t *testing.T) { + want := distinctColumnsUser() + names := splitColumnList(userSelectColumns) + row := valuesInListOrder(t, userSelectColumns, userRowByColumn(want)) + // Every query must embed the shared list verbatim. + selectRe := regexp.QuoteMeta("SELECT " + userSelectColumns + "\n\t\tFROM users") + + getters := []struct { + name string + args []any + get func(*PostgresStore) (*User, error) + }{ + {"GetUserByID", []any{want.ID}, func(s *PostgresStore) (*User, error) { + return s.GetUserByID(context.Background(), want.ID) + }}, + {"GetUserByEmail", []any{want.Email}, func(s *PostgresStore) (*User, error) { + return s.GetUserByEmail(context.Background(), want.Email) + }}, + {"GetUserByResetToken", []any{want.PasswordResetToken}, func(s *PostgresStore) (*User, error) { + return s.GetUserByResetToken(context.Background(), want.PasswordResetToken) + }}, + {"ListUsers", nil, func(s *PostgresStore) (*User, error) { + users, err := s.ListUsers(context.Background()) + if err != nil || len(users) != 1 { + return nil, err + } + return &users[0], nil + }}, + } + for _, tc := range getters { + t.Run(tc.name, func(t *testing.T) { + mock := newAuthPgxMock(t) + mock.ExpectQuery(selectRe).WithArgs(tc.args...). + WillReturnRows(pgxmock.NewRows(names).AddRow(row...)) + + got, err := tc.get(NewPostgresStore(mock)) + require.NoError(t, err) + assert.Equal(t, want, got) + }) + } +} + +func TestUserInsertColumns_CreateUserBindsEveryColumnFromItsField(t *testing.T) { + mock := newAuthPgxMock(t) + user := distinctColumnsUser() + before := time.Now() + args := valuesInListOrder(t, userInsertColumns, userInsertArgsByColumn(user, before)) + + mock.ExpectExec(regexp.QuoteMeta("INSERT INTO users (" + userInsertColumns + ")")). + WithArgs(args...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + + require.NoError(t, NewPostgresStore(mock).CreateUser(context.Background(), user)) +} + +func TestUserInsertColumns_CreateAdminIfNoneBindsEveryColumnFromItsField(t *testing.T) { + mock := newAuthPgxMock(t) + user := distinctColumnsUser() // already carries the Administrators group + before := time.Now() + args := valuesInListOrder(t, userInsertColumns, userInsertArgsByColumn(user, before)) + args = append(args, DefaultAdminGroupID) // $19, the NOT EXISTS binding + + expectBootstrapTxPrefix(mock) + mock.ExpectExec(regexp.QuoteMeta("INSERT INTO users (" + userInsertColumns + ")")). + WithArgs(args...). + WillReturnResult(pgxmock.NewResult("INSERT", 1)) + mock.ExpectCommit() + + created, err := NewPostgresStore(mock).CreateAdminIfNone(context.Background(), user) + require.NoError(t, err) + assert.True(t, created) +} + +// The select list is the insert list plus the two columns the inserts leave +// to their defaults; pin that so the two cannot drift into different orders. +func TestUserSelectColumns_ExtendInsertColumns(t *testing.T) { + insert := splitColumnList(userInsertColumns) + sel := splitColumnList(userSelectColumns) + require.Equal(t, insert, sel[:len(insert)]) + assert.Equal(t, []string{"deactivated_at", "password_version"}, sel[len(insert):]) +} diff --git a/internal/email/coverage_test.go b/internal/email/coverage_test.go index 750f436e8..e99e21673 100644 --- a/internal/email/coverage_test.go +++ b/internal/email/coverage_test.go @@ -65,21 +65,6 @@ func TestSMTPSender_SendPasswordResetEmail_RenderingSuccess(t *testing.T) { require.NoError(t, err) } -// TestSMTPSender_SendWelcomeEmail_RenderingSuccess tests rendering success path. -func TestSMTPSender_SendWelcomeEmail_RenderingSuccess(t *testing.T) { - sender := &SMTPSender{ - host: "smtp.example.com", - port: 587, - fromEmail: "", // Empty to trigger early return - fromName: "CUDly", - useTLS: true, - } - - ctx := context.Background() - err := sender.SendWelcomeEmail(ctx, "user@example.com", "https://dashboard.example.com", "admin") - require.NoError(t, err) -} - // TestSMTPSender_AllNotificationMethods_NoFromEmail tests all notification methods with empty fromEmail. func TestSMTPSender_AllNotificationMethods_NoFromEmail(t *testing.T) { sender := &SMTPSender{ @@ -203,12 +188,6 @@ func TestRenderFunctions_EdgeCases(t *testing.T) { assert.Contains(t, result, "Password Reset") }) - t.Run("RenderWelcomeEmail with empty fields", func(t *testing.T) { - result, err := RenderWelcomeEmail("", "", "") - require.NoError(t, err) - assert.Contains(t, result, "Welcome") - }) - t.Run("RenderNewRecommendationsEmail with nil recommendations", func(t *testing.T) { data := NotificationData{ DashboardURL: "https://example.com", @@ -395,32 +374,6 @@ func TestPasswordResetData_AllFields(t *testing.T) { assert.Equal(t, "https://example.com/reset?token=abc123", data.ResetURL) } -// TestWelcomeUserData_AllFields tests WelcomeUserData structure. -func TestWelcomeUserData_AllFields(t *testing.T) { - data := WelcomeUserData{ - Email: "newuser@example.com", - DashboardURL: "https://cudly.example.com", - Role: "operator", - } - - assert.Equal(t, "newuser@example.com", data.Email) - assert.Equal(t, "https://cudly.example.com", data.DashboardURL) - assert.Equal(t, "operator", data.Role) -} - -// TestWelcomeUserData_ViewerRole tests WelcomeUserData with viewer role. -func TestWelcomeUserData_ViewerRole(t *testing.T) { - data := WelcomeUserData{ - Email: "test@example.com", - DashboardURL: "https://dashboard.test.com", - Role: "viewer", - } - - assert.Equal(t, "test@example.com", data.Email) - assert.Equal(t, "https://dashboard.test.com", data.DashboardURL) - assert.Equal(t, "viewer", data.Role) -} - // TestProviderTypes tests provider type constants. func TestProviderTypes_Values(t *testing.T) { assert.Equal(t, ProviderType("aws"), ProviderAWS) @@ -544,12 +497,6 @@ func TestTemplateContent_HasRequiredSections(t *testing.T) { assert.Contains(t, passwordResetTemplate, "1 hour") assert.Contains(t, passwordResetTemplate, ".ResetURL") }) - - t.Run("welcomeUserTemplate has role", func(t *testing.T) { - assert.Contains(t, welcomeUserTemplate, ".Role") - assert.Contains(t, welcomeUserTemplate, ".DashboardURL") - assert.Contains(t, welcomeUserTemplate, ".Email") - }) } // TestRenderFunctions_MultipleRecommendations tests templates with multiple recommendations. @@ -603,7 +550,6 @@ func TestSMTPSender_MethodsWithNoNetwork(t *testing.T) { assert.NoError(t, sender.SendNotification(ctx, "Subject", "Body")) assert.NoError(t, sender.SendToEmail(ctx, "to@example.com", "Subject", "Body")) assert.NoError(t, sender.SendPasswordResetEmail(ctx, "user@example.com", "https://example.com/reset")) - assert.NoError(t, sender.SendWelcomeEmail(ctx, "user@example.com", "https://example.com", "user")) data := NotificationData{DashboardURL: "https://example.com"} assert.NoError(t, sender.SendNewRecommendationsNotification(ctx, data)) @@ -708,12 +654,6 @@ func TestSMTPSender_AllMethods_ConnectionFails(t *testing.T) { assert.Contains(t, err.Error(), "failed to send email via SMTP") }) - t.Run("SendWelcomeEmail fails", func(t *testing.T) { - err := sender.SendWelcomeEmail(ctx, "user@example.com", "https://example.com", "admin") - require.Error(t, err) - assert.Contains(t, err.Error(), "failed to send email via SMTP") - }) - t.Run("SendNewRecommendationsNotification fails", func(t *testing.T) { err := sender.SendNewRecommendationsNotification(ctx, data) require.Error(t, err) @@ -1064,7 +1004,6 @@ func TestSMTPSenderInterface_Compliance(t *testing.T) { _ = sender.SendPurchaseConfirmation _ = sender.SendPurchaseFailedNotification _ = sender.SendPasswordResetEmail - _ = sender.SendWelcomeEmail } // TestSMTPSender_SendToEmail_MultipleRecipientTypes tests various recipient formats. @@ -1121,25 +1060,6 @@ func TestRenderAllTemplates_FullCoverage(t *testing.T) { assert.NotEmpty(t, result) }) - // Test RenderWelcomeEmail with various inputs - t.Run("Welcome_admin", func(t *testing.T) { - result, err := RenderWelcomeEmail("admin@test.com", "https://dashboard.com", "admin") - require.NoError(t, err) - assert.NotEmpty(t, result) - }) - - t.Run("Welcome_user", func(t *testing.T) { - result, err := RenderWelcomeEmail("user@test.com", "https://dashboard.com", "user") - require.NoError(t, err) - assert.NotEmpty(t, result) - }) - - t.Run("Welcome_operator", func(t *testing.T) { - result, err := RenderWelcomeEmail("op@test.com", "https://dashboard.com", "operator") - require.NoError(t, err) - assert.NotEmpty(t, result) - }) - // Test notification data with different configurations t.Run("Recommendations_with_engine", func(t *testing.T) { data := NotificationData{ diff --git a/internal/email/interfaces.go b/internal/email/interfaces.go index 08438fdb8..a174def3e 100644 --- a/internal/email/interfaces.go +++ b/internal/email/interfaces.go @@ -17,7 +17,6 @@ type SenderInterface interface { SendPurchaseConfirmation(ctx context.Context, data NotificationData) error SendPurchaseFailedNotification(ctx context.Context, data NotificationData) error SendPasswordResetEmail(ctx context.Context, email, resetURL string) error - SendWelcomeEmail(ctx context.Context, email, dashboardURL, role string) error SendUserInviteEmail(ctx context.Context, email, setupURL string) error SendRIExchangePendingApproval(ctx context.Context, data RIExchangeNotificationData) error SendRIExchangeCompleted(ctx context.Context, data RIExchangeNotificationData) error diff --git a/internal/email/nop_sender.go b/internal/email/nop_sender.go index 02d730ad2..45ef4ee25 100644 --- a/internal/email/nop_sender.go +++ b/internal/email/nop_sender.go @@ -65,11 +65,6 @@ func (n *NopSender) SendPasswordResetEmail(_ context.Context, _, _ string) error return nil } -func (n *NopSender) SendWelcomeEmail(_ context.Context, _, _, role string) error { - logging.Debugf("email/nop: SendWelcomeEmail suppressed (role=%q)", role) - return nil -} - func (n *NopSender) SendUserInviteEmail(_ context.Context, _, _ string) error { logging.Debugf("email/nop: SendUserInviteEmail suppressed") return nil diff --git a/internal/email/nop_sender_test.go b/internal/email/nop_sender_test.go index dfd8d9816..de6cb1570 100644 --- a/internal/email/nop_sender_test.go +++ b/internal/email/nop_sender_test.go @@ -28,7 +28,6 @@ func TestNopSender_AllMethodsReturnNil(t *testing.T) { assert.NoError(t, n.SendPurchaseConfirmation(ctx, NotificationData{})) assert.NoError(t, n.SendPurchaseFailedNotification(ctx, NotificationData{})) assert.NoError(t, n.SendPasswordResetEmail(ctx, "user@example.com", "https://example/reset")) - assert.NoError(t, n.SendWelcomeEmail(ctx, "user@example.com", "https://example/dashboard", "admin")) assert.NoError(t, n.SendRIExchangePendingApproval(ctx, RIExchangeNotificationData{})) assert.NoError(t, n.SendRIExchangeCompleted(ctx, RIExchangeNotificationData{})) assert.NoError(t, n.SendPurchaseApprovalRequest(ctx, NotificationData{})) diff --git a/internal/email/sender.go b/internal/email/sender.go index 9ffbfe40e..f0b720973 100644 --- a/internal/email/sender.go +++ b/internal/email/sender.go @@ -303,8 +303,8 @@ func (s *Sender) SendToEmailWithCCMultipart(ctx context.Context, toEmail string, // twice. func (s *Sender) SendToEmailWithCC(ctx context.Context, toEmail string, ccEmails []string, subject, body string) error { // Empty or malformed FROM_EMAIL means nothing can go out. Return - // ErrNoFromEmail rather than nil so callers (password reset, invites, - // welcome mail) don't report a send that never happened (#270). + // ErrNoFromEmail rather than nil so callers (password reset, invites) + // don't report a send that never happened (#270). if !isValidFromEmail(s.fromEmail) { return ErrNoFromEmail } diff --git a/internal/email/sender_test.go b/internal/email/sender_test.go index 41151a101..575470b23 100644 --- a/internal/email/sender_test.go +++ b/internal/email/sender_test.go @@ -131,18 +131,6 @@ func TestPasswordResetData(t *testing.T) { assert.Equal(t, "https://dashboard.example.com/reset?token=abc123", data.ResetURL) } -func TestWelcomeUserData(t *testing.T) { - data := WelcomeUserData{ - Email: "newuser@example.com", - DashboardURL: "https://dashboard.example.com", - Role: "user", - } - - assert.Equal(t, "newuser@example.com", data.Email) - assert.Equal(t, "https://dashboard.example.com", data.DashboardURL) - assert.Equal(t, "user", data.Role) -} - func TestSender_SendNotification_NoTopic(t *testing.T) { sender := &Sender{ topicARN: "", @@ -325,19 +313,6 @@ func TestTemplates_PasswordReset(t *testing.T) { require.Error(t, err) } -func TestTemplates_WelcomeEmail(t *testing.T) { - sender := &Sender{ - sesClient: nil, - fromEmail: "noreply@example.com", - } - - ctx := context.Background() - err := sender.SendWelcomeEmail(ctx, "newuser@example.com", "https://dashboard.example.com", "user") - - // Will fail because sesClient is nil - require.Error(t, err) -} - func TestTemplateContents(t *testing.T) { // Verify template constants are not empty assert.NotEmpty(t, newRecommendationsTemplate) @@ -345,7 +320,6 @@ func TestTemplateContents(t *testing.T) { assert.NotEmpty(t, purchaseConfirmationTemplate) assert.NotEmpty(t, purchaseFailedTemplate) assert.NotEmpty(t, passwordResetTemplate) - assert.NotEmpty(t, welcomeUserTemplate) // Verify templates contain expected placeholders assert.Contains(t, newRecommendationsTemplate, "{{.DashboardURL}}") @@ -364,10 +338,6 @@ func TestTemplateContents(t *testing.T) { assert.Contains(t, passwordResetTemplate, "{{.ResetURL}}") assert.Contains(t, passwordResetTemplate, "Password Reset") - - assert.Contains(t, welcomeUserTemplate, "{{.Email}}") - assert.Contains(t, welcomeUserTemplate, "{{.Role}}") - assert.Contains(t, welcomeUserTemplate, "Welcome") } func TestSender_SendNotification_Success(t *testing.T) { diff --git a/internal/email/smtp_sender.go b/internal/email/smtp_sender.go index b53cb2583..b880076c7 100644 --- a/internal/email/smtp_sender.go +++ b/internal/email/smtp_sender.go @@ -450,16 +450,6 @@ func (s *SMTPSender) SendPasswordResetEmail(ctx context.Context, email, resetURL ) } -// SendWelcomeEmail sends a welcome email to new users as multipart/ -// alternative (text + styled HTML). Issue #355. -func (s *SMTPSender) SendWelcomeEmail(ctx context.Context, email, dashboardURL, role string) error { - return sendMultipartVia( - ctx, s, email, "Welcome to CUDly!", "welcome", - func() (string, error) { return RenderWelcomeEmail(email, dashboardURL, role) }, - func() (string, error) { return RenderWelcomeEmailHTML(email, dashboardURL, role) }, - ) -} - // SendUserInviteEmail sends an invite-with-setup-link email to a user // created without a password, as multipart/alternative (text + styled HTML). // Issue #355. diff --git a/internal/email/smtp_sender_test.go b/internal/email/smtp_sender_test.go index 275ed307b..c052043fa 100644 --- a/internal/email/smtp_sender_test.go +++ b/internal/email/smtp_sender_test.go @@ -164,19 +164,6 @@ func TestSMTPSender_SendPasswordResetEmail_NoFromEmail(t *testing.T) { require.NoError(t, err) } -func TestSMTPSender_SendWelcomeEmail_NoFromEmail(t *testing.T) { - sender := &SMTPSender{ - host: "smtp.example.com", - port: 587, - fromEmail: "", // No from email - should skip - } - - ctx := context.Background() - err := sender.SendWelcomeEmail(ctx, "user@example.com", "https://dashboard.example.com", "user") - - require.NoError(t, err) -} - func TestSMTPSender_SendNewRecommendationsNotification_NoFromEmail(t *testing.T) { sender := &SMTPSender{ host: "smtp.example.com", diff --git a/internal/email/smtp_server_test.go b/internal/email/smtp_server_test.go index 327fa5088..08efb2165 100644 --- a/internal/email/smtp_server_test.go +++ b/internal/email/smtp_server_test.go @@ -273,28 +273,6 @@ func TestSMTPSender_SendPasswordResetEmail_WithMockServer(t *testing.T) { require.NoError(t, err) } -// TestSMTPSender_SendWelcomeEmail_WithMockServer tests welcome email. -func TestSMTPSender_SendWelcomeEmail_WithMockServer(t *testing.T) { - server := newMockSMTPServer(t, false) - server.start(t) - defer server.stop() - - sender := &SMTPSender{ - host: "127.0.0.1", - port: server.port, - username: "", - password: "", - fromEmail: "noreply@cudly.io", - fromName: "CUDly", - useTLS: false, - } - - ctx := context.Background() - err := sender.SendWelcomeEmail(ctx, "newuser@example.com", "https://dashboard.example.com", "admin") - - require.NoError(t, err) -} - // TestSMTPSender_SendNewRecommendationsNotification_WithMockServer tests recommendations email. func TestSMTPSender_SendNewRecommendationsNotification_WithMockServer(t *testing.T) { server := newMockSMTPServer(t, false) diff --git a/internal/email/template_renderers.go b/internal/email/template_renderers.go index 5282e34d3..7710d8946 100644 --- a/internal/email/template_renderers.go +++ b/internal/email/template_renderers.go @@ -75,25 +75,6 @@ func RenderPasswordResetEmailHTML(email, resetURL string) (string, error) { }) } -// RenderWelcomeEmail renders the plain-text welcome email template. -func RenderWelcomeEmail(email, dashboardURL, role string) (string, error) { - return renderTextTemplate("welcome", welcomeUserTemplate, WelcomeUserData{ - Email: email, - DashboardURL: dashboardURL, - Role: role, - }) -} - -// RenderWelcomeEmailHTML renders the HTML half of the welcome email. -// Issue #355. -func RenderWelcomeEmailHTML(email, dashboardURL, role string) (string, error) { - return renderTemplate("welcome-html", welcomeUserHTMLTemplate, WelcomeUserData{ - Email: email, - DashboardURL: dashboardURL, - Role: role, - }) -} - // RenderUserInviteEmail renders the plain-text user-invite email template. func RenderUserInviteEmail(email, setupURL string) (string, error) { return renderTextTemplate("user-invite", userInviteTemplate, UserInviteData{ diff --git a/internal/email/template_renderers_test.go b/internal/email/template_renderers_test.go index 5cc0c7ec4..b4becb802 100644 --- a/internal/email/template_renderers_test.go +++ b/internal/email/template_renderers_test.go @@ -20,20 +20,6 @@ func TestRenderPasswordResetEmail(t *testing.T) { assert.Contains(t, result, "CUDly") } -func TestRenderWelcomeEmail(t *testing.T) { - email := "user@example.com" - dashboardURL := "https://dashboard.example.com" - role := "admin" - - result, err := RenderWelcomeEmail(email, dashboardURL, role) - - require.NoError(t, err) - assert.Contains(t, result, dashboardURL) - assert.Contains(t, result, role) - assert.Contains(t, result, "Welcome") - assert.Contains(t, result, "CUDly") -} - func TestRenderNewRecommendationsEmail(t *testing.T) { data := NotificationData{ DashboardURL: "https://dashboard.example.com", @@ -239,18 +225,6 @@ func TestRenderPurchaseConfirmationEmail_NoUpfrontCost(t *testing.T) { // Should not contain upfront cost line when it's 0 } -func TestWelcomeUserData_Structure(t *testing.T) { - data := WelcomeUserData{ - Email: "user@example.com", - DashboardURL: "https://dashboard.example.com", - Role: "admin", - } - - assert.Equal(t, "user@example.com", data.Email) - assert.Equal(t, "https://dashboard.example.com", data.DashboardURL) - assert.Equal(t, "admin", data.Role) -} - // Issue #287: extended approval-request plain-text template carries // per-rec Term/Payment/Upfront/AccountLabel + the requested-by header // + the cancellation-window note. @@ -485,18 +459,6 @@ func TestRenderUserInviteEmailHTML(t *testing.T) { assert.Contains(t, html, "Hello bob@example.com,") } -// TestRenderWelcomeEmailHTML — same assertions for the welcome flow. -func TestRenderWelcomeEmailHTML(t *testing.T) { - html, err := RenderWelcomeEmailHTML("carol@example.com", "https://dashboard.example", "admin") - require.NoError(t, err) - assert.NotEmpty(t, html) - - assert.Contains(t, html, `href="https://dashboard.example"`) - assert.Contains(t, html, ">Open dashboard<") - assert.Contains(t, html, "Hello carol@example.com,") - assert.Contains(t, html, "admin") -} - // TestPlainTextTemplates_NoHTMLEscaping asserts that special characters in data // fields (ampersand, apostrophe, angle brackets) survive verbatim in plain-text // email bodies. Previously all renderers used html/template which emitted HTML diff --git a/internal/email/templates.go b/internal/email/templates.go index dd0133118..36d7eb84b 100644 --- a/internal/email/templates.go +++ b/internal/email/templates.go @@ -203,55 +203,6 @@ const passwordResetHTMLTemplate = "" + ` -const welcomeUserTemplate = `Welcome to CUDly -================ - -Hello {{.Email}}, - -Your CUDly account has been created. - -You can log in at: -{{.DashboardURL}} - -Your role: {{.Role}} - -If you have any questions, please contact your administrator. - -This is an automated message from CUDly. -` - -// welcomeUserHTMLTemplate is the HTML half of welcomeUserTemplate. -// Inline-styled per email-client constraints. Issue #355. -const welcomeUserHTMLTemplate = ` -Welcome to CUDly - - -
- - - - - - - - - -
-

Welcome to CUDly

-

Hello {{.Email}},

-

Your CUDly account has been created with role {{.Role}}.

-
-Open dashboard -
-

If the button doesn't work, paste this URL into your browser:

-

{{.DashboardURL}}

-

If you have any questions, please contact your administrator.

-
-

This is an automated message from CUDly.

-
-
-` - const userInviteTemplate = `Welcome to CUDly ================ @@ -518,23 +469,6 @@ func (s *Sender) SendPasswordResetEmail(ctx context.Context, email, resetURL str ) } -// WelcomeUserData holds data for welcome emails. -type WelcomeUserData struct { - Email string - DashboardURL string - Role string -} - -// SendWelcomeEmail sends a welcome email to a new user as multipart/ -// alternative (plain text + styled HTML with a CTA button). Issue #355. -func (s *Sender) SendWelcomeEmail(ctx context.Context, email, dashboardURL, role string) error { - return sendMultipartVia( - ctx, s, email, "Welcome to CUDly", "welcome", - func() (string, error) { return RenderWelcomeEmail(email, dashboardURL, role) }, - func() (string, error) { return RenderWelcomeEmailHTML(email, dashboardURL, role) }, - ) -} - // UserInviteData holds data for invite emails sent to users that an admin // created without a password. The recipient sets their own password by // following SetupURL. @@ -803,7 +737,7 @@ const purchaseApprovalRequestHTMLTemplate = ` ` // sendMultipartVia is the generic dual-render send helper used by the -// invite / password-reset / welcome flows. The two render closures are +// invite and password-reset flows. The two render closures are // invoked back-to-back; an HTML render failure is non-fatal and degrades // to text-only delivery (matching sendPurchaseApprovalRequestVia's contract). // kind is a short label like "user-invite" used in the warn log on degrade. diff --git a/internal/email/templates_test.go b/internal/email/templates_test.go index 6841bb16a..49f1a4685 100644 --- a/internal/email/templates_test.go +++ b/internal/email/templates_test.go @@ -139,25 +139,6 @@ func TestSender_SendPasswordResetEmail_Success(t *testing.T) { mockSES.AssertExpectations(t) } -func TestSender_SendWelcomeEmail_Success(t *testing.T) { - mockSES := new(MockSESClient) - // GetAccount is called to check sandbox mode - return production mode (not sandbox) - mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). - Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) - mockSES.On("SendEmail", mock.Anything, mock.AnythingOfType("*sesv2.SendEmailInput")). - Return(&sesv2.SendEmailOutput{MessageId: aws.String("msg-456")}, nil) - - sender := NewSenderWithClients(nil, mockSES, SenderConfig{ - FromEmail: "noreply@example.com", - }) - - ctx := context.Background() - err := sender.SendWelcomeEmail(ctx, "newuser@example.com", "https://dashboard.example.com", "user") - - require.NoError(t, err) - mockSES.AssertExpectations(t) -} - // Test template success paths with no recommendations (edge case). func TestSender_SendNewRecommendationsNotification_EmptyRecommendations(t *testing.T) { mockSNS := new(MockSNSClient) @@ -259,18 +240,6 @@ func TestSender_SendPasswordResetEmail_NoFromEmail(t *testing.T) { require.ErrorIs(t, err, ErrNoFromEmail) } -func TestSender_SendWelcomeEmail_NoFromEmail(t *testing.T) { - sender := &Sender{ - fromEmail: "", - } - - ctx := context.Background() - err := sender.SendWelcomeEmail(ctx, "user@example.com", "https://example.com", "user") - - // Must surface the misconfiguration rather than report a send that never happened (#270). - require.ErrorIs(t, err, ErrNoFromEmail) -} - func TestSender_SendUserInviteEmail_NoFromEmail(t *testing.T) { sender := &Sender{ fromEmail: "", @@ -385,23 +354,6 @@ func TestSender_SendPasswordResetEmail_SESError(t *testing.T) { require.Error(t, err) } -func TestSender_SendWelcomeEmail_SESError(t *testing.T) { - mockSES := new(MockSESClient) - sender := &Sender{ - sesClient: mockSES, - fromEmail: "noreply@example.com", - } - - // GetAccount is called first - return production mode (not sandbox) - mockSES.On("GetAccount", mock.Anything, mock.AnythingOfType("*sesv2.GetAccountInput")). - Return(&sesv2.GetAccountOutput{ProductionAccessEnabled: true}, nil) - mockSES.On("SendEmail", mock.Anything, mock.Anything).Return(nil, assert.AnError) - - ctx := context.Background() - err := sender.SendWelcomeEmail(ctx, "user@example.com", "https://example.com", "admin") - require.Error(t, err) -} - // Test multiple recommendations in templates. func TestSender_SendNewRecommendationsNotification_MultipleRecommendations(t *testing.T) { mockSNS := new(MockSNSClient) diff --git a/internal/mocks/email.go b/internal/mocks/email.go index 0dc14bc9b..e7d0ee90a 100644 --- a/internal/mocks/email.go +++ b/internal/mocks/email.go @@ -54,12 +54,6 @@ func (m *MockEmailSender) SendPasswordResetEmail(ctx context.Context, emailAddr, return args.Error(0) } -// SendWelcomeEmail mocks the SendWelcomeEmail operation. -func (m *MockEmailSender) SendWelcomeEmail(ctx context.Context, emailAddr, dashboardURL, role string) error { - args := m.Called(ctx, emailAddr, dashboardURL, role) - return args.Error(0) -} - // SendPurchaseApprovalRequest mocks the SendPurchaseApprovalRequest operation. func (m *MockEmailSender) SendPurchaseApprovalRequest(ctx context.Context, data email.NotificationData) error { args := m.Called(ctx, data) @@ -91,7 +85,6 @@ type EmailSenderAPI interface { SendPurchaseConfirmation(ctx context.Context, data email.NotificationData) error SendPurchaseFailedNotification(ctx context.Context, data email.NotificationData) error SendPasswordResetEmail(ctx context.Context, email, resetURL string) error - SendWelcomeEmail(ctx context.Context, email, dashboardURL, role string) error SendPurchaseApprovalRequest(ctx context.Context, data email.NotificationData) error SendPurchaseExecutedNotification(ctx context.Context, data email.NotificationData) error } diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index 8d6b92631..1c8098d3d 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -210,11 +210,6 @@ func (m *MockEmailSender) SendPasswordResetEmail(ctx context.Context, emailAddr, return args.Error(0) } -func (m *MockEmailSender) SendWelcomeEmail(ctx context.Context, emailAddr, dashboardURL, role string) error { - args := m.Called(ctx, emailAddr, dashboardURL, role) - return args.Error(0) -} - func (m *MockEmailSender) SendUserInviteEmail(ctx context.Context, emailAddr, setupURL string) error { args := m.Called(ctx, emailAddr, setupURL) return args.Error(0) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 69f4d547d..2f911c52f 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -89,11 +89,6 @@ func (m *MockEmailSender) SendPasswordResetEmail(ctx context.Context, email, res return args.Error(0) } -func (m *MockEmailSender) SendWelcomeEmail(ctx context.Context, email, dashboardURL, role string) error { - args := m.Called(ctx, email, dashboardURL, role) - return args.Error(0) -} - func (m *MockEmailSender) SendUserInviteEmail(ctx context.Context, email, setupURL string) error { args := m.Called(ctx, email, setupURL) return args.Error(0) diff --git a/internal/server/app_test.go b/internal/server/app_test.go index 285eb867e..25ebdf1f9 100644 --- a/internal/server/app_test.go +++ b/internal/server/app_test.go @@ -281,9 +281,6 @@ func (n *noopEmailSender) SendPurchaseFailedNotification(ctx context.Context, da func (n *noopEmailSender) SendPasswordResetEmail(ctx context.Context, emailAddr, resetURL string) error { return nil } -func (n *noopEmailSender) SendWelcomeEmail(ctx context.Context, emailAddr, dashboardURL, role string) error { - return nil -} func (n *noopEmailSender) SendUserInviteEmail(ctx context.Context, emailAddr, setupURL string) error { return nil } diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index ddceda22e..d4546c847 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -1023,9 +1023,6 @@ func (m *mockEmailSender) SendPurchaseScheduledNotification(context.Context, ema func (m *mockEmailSender) SendPasswordResetEmail(context.Context, string, string) error { return nil } -func (m *mockEmailSender) SendWelcomeEmail(context.Context, string, string, string) error { - return nil -} func (m *mockEmailSender) SendUserInviteEmail(context.Context, string, string) error { return nil }