From 9e5b2c8ae0d75779c062b660579abc12cfe6fadd Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:24:15 +0000 Subject: [PATCH 1/3] refactor(auth): share one users column list between the SELECTs, INSERTs and scanUser The four user SELECTs (GetUserByID, GetUserByEmail, ListUsers, GetUserByResetToken) each carried a hand-aligned copy of the 20-column list that scanUser reads positionally, and CreateUser and CreateAdminIfNone each carried their own 18-column INSERT list and argument list. Swapping two same-typed neighbors (mfa_secret and mfa_pending_secret, say) in one copy kept every existing test green. All four SELECTs now read userSelectColumns, both INSERTs write userInsertColumns with values from userInsertArgs, and the select list is the insert list plus deactivated_at and password_version. New tests key every value by column name (pgxmock) and round-trip every column through a real migrated Postgres with SQL that names its own columns. Refs #313 Co-Authored-By: Claude Opus 5.5 --- internal/auth/store_postgres.go | 115 ++++----- internal/auth/store_postgres_admin.go | 30 +-- .../store_postgres_user_columns_db_test.go | 146 +++++++++++ .../auth/store_postgres_user_columns_test.go | 231 ++++++++++++++++++ 4 files changed, 441 insertions(+), 81 deletions(-) create mode 100644 internal/auth/store_postgres_user_columns_db_test.go create mode 100644 internal/auth/store_postgres_user_columns_test.go diff --git a/internal/auth/store_postgres.go b/internal/auth/store_postgres.go index fef94ecb..9c0ac03e 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 2eaa152d..98241c22 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 00000000..cd16c7f7 --- /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 00000000..0385125b --- /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):]) +} From 6d759dd2a0003dd7594101c6cc7a98f9f69fd006 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 27 Oct 2026 23:24:15 +0000 Subject: [PATCH 2/3] chore(email): drop the never-called SendWelcomeEmail and its role-carrying templates Nothing calls SendWelcomeEmail: invited users get SendUserInviteEmail. It still carried a role parameter from the removed role model. Remove it from email.SenderInterface, Sender, SMTPSender, NopSender, the mocks.MockEmailSender and EmailSenderAPI, and the per-package test stubs, together with the welcome templates, WelcomeUserData and the two renderers that only it used. Comments that called the invite email the welcome email now say invite. Refs #313 Co-Authored-By: Claude Opus 5.5 --- internal/api/coverage_gaps_test.go | 1 - internal/auth/service.go | 2 +- .../service_reset_invalidation_db_test.go | 3 - internal/auth/service_user.go | 2 +- internal/auth/service_user_test.go | 4 +- internal/email/coverage_test.go | 80 ------------------- internal/email/interfaces.go | 1 - internal/email/nop_sender.go | 5 -- internal/email/nop_sender_test.go | 1 - internal/email/sender.go | 4 +- internal/email/sender_test.go | 30 ------- internal/email/smtp_sender.go | 10 --- internal/email/smtp_sender_test.go | 13 --- internal/email/smtp_server_test.go | 22 ----- internal/email/template_renderers.go | 19 ----- internal/email/template_renderers_test.go | 38 --------- internal/email/templates.go | 68 +--------------- internal/email/templates_test.go | 48 ----------- internal/mocks/email.go | 7 -- internal/purchase/mocks_test.go | 5 -- internal/scheduler/scheduler_test.go | 5 -- internal/server/app_test.go | 3 - internal/server/handler_ri_exchange_test.go | 3 - 23 files changed, 7 insertions(+), 367 deletions(-) diff --git a/internal/api/coverage_gaps_test.go b/internal/api/coverage_gaps_test.go index a81b1a50..c4a8cd3a 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 afef57ef..9e2be3f1 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 afd65d12..ffdc15f5 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 05a325e3..0f9b519c 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 a4d285a5..92ebdf8e 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/email/coverage_test.go b/internal/email/coverage_test.go index 750f436e..e99e2167 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 08438fdb..a174def3 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 02d730ad..45ef4ee2 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 dfd8d981..de6cb157 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 9ffbfe40..f0b72097 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 41151a10..575470b2 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 b53cb258..b880076c 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 275ed307..c052043f 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 327fa508..08efb216 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 5282e34d..7710d894 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 5cc0c7ec..b4becb80 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 dd013311..36d7eb84 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 6841bb16..49f1a468 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 0dc14bc9..e7d0ee90 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 8d6b9263..1c8098d3 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 69f4d547..2f911c52 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 285eb867..25ebdf1f 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 ddceda22..d4546c84 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 } From 7b84ab9f1f7bda081bd4b16728a2194ca02898ef Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Wed, 28 Oct 2026 00:19:39 +0000 Subject: [PATCH 3/3] fix(deps): bump golang.org/x/net to v0.62.0 (GO-2026-6860, GO-2026-6866) govulncheck in the Docker image build now fails on these two advisories, both fixed in x/net v0.62.0. Co-Authored-By: Claude Opus 5.5 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5fb3a2f6..abcf4fb6 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 06deac0b..83df52e6 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=