fix(auth/users): close MED-11 lying field — DeactivatedAt loaded + enforced on login (A-2)

The MED-11 closure shipped users.deactivated_at + DELETE /api/v1/auth/users/{id}
+ cascade-revoke, but the federated-user soft-delete was reversible: the next
OIDC login under the same (provider, subject) tuple re-minted a session and
re-elevated the user.

Three legs of the chain were severed (each independently CRIT-shaped):

  Leg A — postgres/user.go::userColumns omitted `deactivated_at`, so scanUser
          never populated User.DeactivatedAt. Every Get / GetByOIDCSubject /
          ListAll returned DeactivatedAt = nil regardless of the column value.

  Leg B — postgres/user.go::Update SQL omitted `deactivated_at = $X`, so the
          handler's `u.DeactivatedAt = now()` mutation was a no-op write at
          the SQL level. Even with leg A closed, no row ever flipped.

  Leg C — oidc/service.go::upsertUser did not inspect DeactivatedAt on the
          existing-user path. Even with legs A + B closed, the OIDC login
          would still proceed normally.

The cascade-session-revoke half of the original closure remained correct, but
only for the duration of the user's current cookie. SOC 2 CC6.3 + ISO 27001
A.9.2.6 "user access removal" controls require both immediate revoke AND
persistent block — this fix restores the persistent-block leg.

Closure across layers:

  internal/repository/postgres/user.go
    - userColumns adds `deactivated_at`
    - scanUser reads via sql.NullTime intermediate (column is nullable)
    - Create writes deactivated_at explicitly (NULL for new active users;
      forward-compat for future seed-data flows that pre-populate the column)
    - Update writes deactivated_at on every call; nil DeactivatedAt → NULL
      (supports reactivation)

  internal/auth/oidc/service.go
    - New sentinel ErrUserDeactivated
    - upsertUser checks existing.DeactivatedAt != nil BEFORE mutating email /
      display_name / last_login_at — preserves last_login_at forensics on
      rejected login attempts (defense-in-depth pin against future
      "performance optimization" that reorders the gate)

  internal/api/handler/auth_session_oidc.go
    - classifyOIDCFailure adds typed errors.Is dispatch for ErrUserDeactivated
      → audit category "user_deactivated" (SOC/SIEM observability surface)

  internal/api/handler/auth_users.go
    - Self-deactivate guard on Deactivate: HTTP 409 + audit row
      auth.user_deactivate_self_rejected when caller targets own User row.
      Prevents an admin from one-way-door locking themselves out via the
      standard handler; break-glass remains the recovery path.
    - New Reactivate handler: inverse of Deactivate. Clears DeactivatedAt
      via Update; emits auth.user_reactivated audit row. Idempotent on
      already-active rows. Sessions revoked at deactivation stay revoked
      (cascade irreversible by design — user must complete fresh OIDC
      login).

  internal/api/router/router.go
    - POST /api/v1/auth/users/{id}/reactivate wired with auth.user.deactivate
      gate (reactivation is the inverse op, not a separate privilege)

  web/src/api/client.ts + web/src/pages/auth/UsersPage.tsx
    - authReactivateUser() client function
    - Reactivate button on deactivated rows in UsersPage

Regression coverage:

  Postgres (testcontainers, skipped under -short):
    TestUserRepository_DeactivatedAt_RoundTrip — Create → set DeactivatedAt
      → Update → Get / GetByOIDCSubject / ListAll round-trip the value
    TestUserRepository_DeactivatedAt_CreateWritesNullForActive — new active
      user reads back DeactivatedAt = nil
    TestUserRepository_DeactivatedAt_CreatePersistsPreDeactivated — Create
      with non-nil DeactivatedAt round-trips (forward-compat path)

  OIDC service:
    TestService_HandleCallback_RejectsDeactivatedUser — errors.Is
      ErrUserDeactivated; CallbackResult nil; persisted email / last_login_at
      / deactivated_at NOT mutated by the rejected attempt
    TestService_HandleCallback_AllowsReactivatedUser — DeactivatedAt = nil
      → happy path resumes
    TestService_HandleCallback_DeactivatedUserPreservesForensics —
      defense-in-depth pin against future regressions that reorder the
      gate-vs-mutation sequence

  Classifier:
    TestClassifyOIDCFailure extended — typed dispatch + wrapped variant
      round-trip through errors.Is

  Handler:
    TestAuthUsers_Deactivate_RejectsSelfDeactivate — HTTP 409 + audit
      row + cascade-revoke NOT fired + row stays active
    TestAuthUsers_Deactivate_OtherUser_HappyPath — HTTP 204 + cascade
      fires + row soft-deleted
    TestAuthUsers_Reactivate_HappyPath / _IdempotentOnActiveUser /
      _UnknownID / _MissingID / _UpdateError

Phase 6 verify gate green on the targeted packages: gofmt clean, go vet
clean, go test -short pass across internal/auth/oidc, internal/api/handler,
internal/api/router, internal/repository/postgres, internal/auth/...,
internal/service/..., internal/tlsprobe/..., internal/trustanchor/...,
internal/validation/...

Spec at cowork/auth-bundles-fixes-2026-05-11/02-crit-deactivated-at-enforcement.md
Closure annotation at cowork/auth-bundles-audit-2026-05-10.md MED-11 row.
Operator advisory in CHANGELOG.md v2.1.0 release notes.
This commit is contained in:
shankar0123
2026-05-11 02:21:05 +00:00
parent 661b6dbefb
commit a980e4c494
12 changed files with 877 additions and 14 deletions
+47 -8
View File
@@ -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)
}
+173
View File
@@ -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.