Merge Fix 02 (CRIT A-2): close MED-11 lying field — DeactivatedAt loaded + enforced on login

2026-06-07 13:51:36 +00:00 · 2026-05-11 11:16:07 +00:00
parent 027a5a1468 78485f7429
commit 11a1f0babd
12 changed files with 877 additions and 14 deletions
@@ -23,19 +23,33 @@ func NewUserRepository(db *sql.DB) *UserRepository {
 	return &UserRepository{db: db}
 }

+// Audit 2026-05-11 A-2 — deactivated_at column added in migration
+// 000045 (MED-11 foundation) but pre-fix never read here. The
+// federated-user soft-delete flow at
+// internal/api/handler/auth_users.go::Deactivate set the column on
+// Update, but Get / GetByOIDCSubject / ListAll all returned User
+// with zero-value DeactivatedAt regardless. The OIDC login path
+// trusts the returned struct, so a deactivated user's next login
+// re-elevated them. Adding the column to userColumns + scanUser
+// closes the read leg; service.go's upsertUser closes the enforce leg.
 const userColumns = `id, tenant_id, email, display_name, oidc_subject,
 		oidc_provider_id, last_login_at, webauthn_credentials,
-		created_at, updated_at`
+		created_at, updated_at, deactivated_at`

 func scanUser(row interface{ Scan(...interface{}) error }) (*userdomain.User, error) {
 	var u userdomain.User
+	var deactivatedAt sql.NullTime
 	if err := row.Scan(
 		&u.ID, &u.TenantID, &u.Email, &u.DisplayName, &u.OIDCSubject,
 		&u.OIDCProviderID, &u.LastLoginAt, &u.WebAuthnCredentials,
-		&u.CreatedAt, &u.UpdatedAt,
+		&u.CreatedAt, &u.UpdatedAt, &deactivatedAt,
 	); err != nil {
 		return nil, err
 	}
+	if deactivatedAt.Valid {
+		t := deactivatedAt.Time
+		u.DeactivatedAt = &t
+	}
 	return &u, nil
 }

@@ -74,14 +88,26 @@ func (r *UserRepository) GetByOIDCSubject(ctx context.Context, providerID, subje
 // Create persists a new user. Translates SQLSTATE 23505 into
 // ErrUserDuplicateOIDCSubject (the unique constraint on
 // (oidc_provider_id, oidc_subject)).
+//
+// Audit 2026-05-11 A-2 — deactivated_at written explicitly. New rows
+// pre-fix had deactivated_at NULL by schema default; the explicit
+// write makes forward-compat with future seed-data paths that
+// pre-populate the column (e.g. migration of an external user roster
+// where some entries land deactivated). nil → NULL via sql.NullTime.
 func (r *UserRepository) Create(ctx context.Context, u *userdomain.User) error {
+	var deactivatedAt sql.NullTime
+	if u.DeactivatedAt != nil {
+		deactivatedAt = sql.NullTime{Time: *u.DeactivatedAt, Valid: true}
+	}
 	_, err := r.db.ExecContext(ctx, `
 		INSERT INTO users (
 			id, tenant_id, email, display_name, oidc_subject,
-			oidc_provider_id, last_login_at, webauthn_credentials
-		) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`,
+			oidc_provider_id, last_login_at, webauthn_credentials,
+			deactivated_at
+		) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9)`,
 		u.ID, u.TenantID, u.Email, u.DisplayName, u.OIDCSubject,
-		u.OIDCProviderID, u.LastLoginAt, u.WebAuthnCredentials)
+		u.OIDCProviderID, u.LastLoginAt, u.WebAuthnCredentials,
+		deactivatedAt)
 	if err != nil {
 		var pqErr *pq.Error
 		if errors.As(err, &pqErr) && pqErr.Code == "23505" {
@@ -93,18 +119,31 @@ func (r *UserRepository) Create(ctx context.Context, u *userdomain.User) error {
 }

 // Update writes the mutable fields (email, display_name, last_login_at,
-// webauthn_credentials) back to the row. Immutable: id, tenant_id,
-// oidc_subject, oidc_provider_id, created_at. updated_at = NOW().
+// webauthn_credentials, deactivated_at) back to the row. Immutable:
+// id, tenant_id, oidc_subject, oidc_provider_id, created_at.
+// updated_at = NOW().
+//
+// Audit 2026-05-11 A-2 — deactivated_at is now in the mutable set so
+// the federated-user soft-delete flow at
+// internal/api/handler/auth_users.go::Deactivate persists. Pre-fix the
+// Update SQL omitted it; the handler set u.DeactivatedAt = now on the
+// in-memory struct, called Update, the SQL ignored the field, and the
+// row was unchanged. nil DeactivatedAt → NULL (supports reactivation).
 func (r *UserRepository) Update(ctx context.Context, u *userdomain.User) error {
+	var deactivatedAt sql.NullTime
+	if u.DeactivatedAt != nil {
+		deactivatedAt = sql.NullTime{Time: *u.DeactivatedAt, Valid: true}
+	}
 	res, err := r.db.ExecContext(ctx, `
 		UPDATE users SET
 			email = $2,
 			display_name = $3,
 			last_login_at = $4,
 			webauthn_credentials = $5,
+			deactivated_at = $6,
 			updated_at = NOW()
 		WHERE id = $1`,
-		u.ID, u.Email, u.DisplayName, u.LastLoginAt, u.WebAuthnCredentials)
+		u.ID, u.Email, u.DisplayName, u.LastLoginAt, u.WebAuthnCredentials, deactivatedAt)
 	if err != nil {
 		return fmt.Errorf("users update: %w", err)
 	}
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"testing"
+	"time"

 	userdomain "github.com/certctl-io/certctl/internal/auth/user/domain"
 	"github.com/certctl-io/certctl/internal/repository"
@@ -193,6 +194,178 @@ func TestUserRepository_ListAll(t *testing.T) {
 	}
 }

+// TestUserRepository_DeactivatedAt_RoundTrip pins the A-2 closure at
+// the SQL layer. Pre-fix scanUser did not include deactivated_at in
+// userColumns, Update did not write it, and Create did not write it.
+// Result: a non-nil DeactivatedAt set in the in-memory User by the
+// handler was lost on persist + always nil on read. This test
+// exercises both legs.
+//
+// Audit 2026-05-11 A-2 — round-trip a non-nil DeactivatedAt through
+// Update and verify Get + GetByOIDCSubject + ListAll all return it
+// non-nil. Then clear it (reactivate path) and verify the nil
+// round-trips back.
+func TestUserRepository_DeactivatedAt_RoundTrip(t *testing.T) {
+	if testing.Short() {
+		t.Skip("integration test in short mode")
+	}
+	db := getTestDB(t).freshSchema(t)
+	providerRepo := postgres.NewOIDCProviderRepository(db)
+	userRepo := postgres.NewUserRepository(db)
+	ctx := context.Background()
+
+	p := newValidProvider("deact-rt")
+	if err := providerRepo.Create(ctx, p); err != nil {
+		t.Fatalf("Create provider: %v", err)
+	}
+	u := newValidUser("deactivated-user", p.ID)
+	if err := userRepo.Create(ctx, u); err != nil {
+		t.Fatalf("Create user: %v", err)
+	}
+
+	// Sanity: a freshly-created row reads back nil.
+	got, err := userRepo.Get(ctx, u.ID)
+	if err != nil {
+		t.Fatalf("Get (fresh): %v", err)
+	}
+	if got.DeactivatedAt != nil {
+		t.Errorf("freshly-created user has non-nil DeactivatedAt: %v", got.DeactivatedAt)
+	}
+
+	// Soft-delete: set DeactivatedAt and Update.
+	now := time.Now().UTC().Truncate(time.Microsecond) // pg precision
+	got.DeactivatedAt = &now
+	if err := userRepo.Update(ctx, got); err != nil {
+		t.Fatalf("Update (deactivate): %v", err)
+	}
+
+	// Read via Get.
+	rb, err := userRepo.Get(ctx, u.ID)
+	if err != nil {
+		t.Fatalf("Get (post-deactivate): %v", err)
+	}
+	if rb.DeactivatedAt == nil {
+		t.Fatal("Get returned nil DeactivatedAt after Update set it (A-2 regression)")
+	}
+	if !rb.DeactivatedAt.Equal(now) {
+		t.Errorf("Get round-trip DeactivatedAt mismatch: got %v want %v", *rb.DeactivatedAt, now)
+	}
+
+	// Read via GetByOIDCSubject.
+	rs, err := userRepo.GetByOIDCSubject(ctx, p.ID, u.OIDCSubject)
+	if err != nil {
+		t.Fatalf("GetByOIDCSubject (post-deactivate): %v", err)
+	}
+	if rs.DeactivatedAt == nil {
+		t.Error("GetByOIDCSubject returned nil DeactivatedAt after Update set it (A-2 regression — OIDC login path leak)")
+	}
+
+	// Read via ListAll.
+	rows, err := userRepo.ListAll(ctx, "t-default")
+	if err != nil {
+		t.Fatalf("ListAll: %v", err)
+	}
+	if len(rows) != 1 || rows[0].DeactivatedAt == nil {
+		t.Errorf("ListAll: expected 1 row with non-nil DeactivatedAt; got %d rows, first DeactivatedAt=%v",
+			len(rows), func() interface{} {
+				if len(rows) == 0 {
+					return "no rows"
+				}
+				return rows[0].DeactivatedAt
+			}())
+	}
+
+	// Reactivate: clear DeactivatedAt and verify the nil round-trips.
+	rb.DeactivatedAt = nil
+	if err := userRepo.Update(ctx, rb); err != nil {
+		t.Fatalf("Update (reactivate): %v", err)
+	}
+	rfin, err := userRepo.Get(ctx, u.ID)
+	if err != nil {
+		t.Fatalf("Get (post-reactivate): %v", err)
+	}
+	if rfin.DeactivatedAt != nil {
+		t.Errorf("Get returned non-nil DeactivatedAt after reactivate Update cleared it: %v", *rfin.DeactivatedAt)
+	}
+}
+
+// TestUserRepository_DeactivatedAt_CreateWritesNullForActive pins
+// the Create path's behavior for the common case (active user).
+// Pre-fix Create omitted deactivated_at entirely so the column took
+// the schema default (NULL). Now Create writes it explicitly; the
+// observable behavior is unchanged for nil, but a regression would
+// flip new users to deactivated.
+//
+// Audit 2026-05-11 A-2.
+func TestUserRepository_DeactivatedAt_CreateWritesNullForActive(t *testing.T) {
+	if testing.Short() {
+		t.Skip("integration test in short mode")
+	}
+	db := getTestDB(t).freshSchema(t)
+	providerRepo := postgres.NewOIDCProviderRepository(db)
+	userRepo := postgres.NewUserRepository(db)
+	ctx := context.Background()
+
+	p := newValidProvider("create-nil")
+	if err := providerRepo.Create(ctx, p); err != nil {
+		t.Fatalf("Create provider: %v", err)
+	}
+	u := newValidUser("active-user", p.ID)
+	u.DeactivatedAt = nil // explicit: new user is active
+	if err := userRepo.Create(ctx, u); err != nil {
+		t.Fatalf("Create: %v", err)
+	}
+
+	got, err := userRepo.Get(ctx, u.ID)
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	if got.DeactivatedAt != nil {
+		t.Errorf("active user has non-nil DeactivatedAt after Create: %v", *got.DeactivatedAt)
+	}
+}
+
+// TestUserRepository_DeactivatedAt_CreatePersistsPreDeactivated
+// covers the forward-compat path where a future seed-data flow
+// (e.g. migration of an external user roster where some entries
+// land deactivated) pre-populates the column on insert. Pre-fix
+// Create omitted the column entirely, so this case wasn't
+// representable; the A-2 closure makes the explicit write part of
+// the Create contract.
+//
+// Audit 2026-05-11 A-2.
+func TestUserRepository_DeactivatedAt_CreatePersistsPreDeactivated(t *testing.T) {
+	if testing.Short() {
+		t.Skip("integration test in short mode")
+	}
+	db := getTestDB(t).freshSchema(t)
+	providerRepo := postgres.NewOIDCProviderRepository(db)
+	userRepo := postgres.NewUserRepository(db)
+	ctx := context.Background()
+
+	p := newValidProvider("create-deact")
+	if err := providerRepo.Create(ctx, p); err != nil {
+		t.Fatalf("Create provider: %v", err)
+	}
+	u := newValidUser("seed-deactivated", p.ID)
+	pre := time.Now().UTC().Add(-1 * time.Hour).Truncate(time.Microsecond)
+	u.DeactivatedAt = &pre
+	if err := userRepo.Create(ctx, u); err != nil {
+		t.Fatalf("Create: %v", err)
+	}
+
+	got, err := userRepo.Get(ctx, u.ID)
+	if err != nil {
+		t.Fatalf("Get: %v", err)
+	}
+	if got.DeactivatedAt == nil {
+		t.Fatal("DeactivatedAt nil after Create persisted a pre-deactivated user")
+	}
+	if !got.DeactivatedAt.Equal(pre) {
+		t.Errorf("Create round-trip DeactivatedAt: got %v want %v", *got.DeactivatedAt, pre)
+	}
+}
+
 // TestUserRepository_DeletingProviderRefusedWhenUsersReference complements
 // the OIDCProviderRepository test of the same shape; pinning both ends
 // of the FK ON DELETE RESTRICT contract.