mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 13:51:36 +00:00
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:
@@ -317,6 +317,21 @@ var (
|
||||
// Audit 2026-05-10 MED-9 closure.
|
||||
ErrProviderDisabled = errors.New("oidc: provider is disabled")
|
||||
|
||||
// ErrUserDeactivated signals the federated user row's
|
||||
// `deactivated_at` is non-NULL. Audit 2026-05-11 A-2 closure —
|
||||
// the deactivate flow at internal/api/handler/auth_users.go::Deactivate
|
||||
// sets the column + cascade-revokes sessions, but pre-fix the OIDC
|
||||
// login path never consulted it, so the very next login re-elevated
|
||||
// the deactivated user. The check fires inside upsertUser before
|
||||
// Update bumps last_login_at; an attempt to log in via OIDC after
|
||||
// deactivation surfaces as audit `failure_category=user_deactivated`
|
||||
// and the LoginPage's reason-aware error rendering shows the
|
||||
// operator-friendly message.
|
||||
//
|
||||
// Reactivation surface: admin POSTs /api/v1/auth/users/{id}/reactivate
|
||||
// (auth.user.deactivate perm) to clear the column.
|
||||
ErrUserDeactivated = errors.New("oidc: user account is deactivated")
|
||||
|
||||
// ErrGroupsUnmapped: the user's groups don't match any of the
|
||||
// operator's group_role_mappings for this provider. No session
|
||||
// minted; audit row records auth.oidc_login_unmapped_groups.
|
||||
@@ -809,6 +824,19 @@ func (s *Service) upsertUser(
|
||||
|
||||
existing, err := s.users.GetByOIDCSubject(ctx, provider.ID, subject)
|
||||
if err == nil {
|
||||
// Audit 2026-05-11 A-2 — refuse login for deactivated users.
|
||||
// The admin `DELETE /api/v1/auth/users/{id}` flow sets
|
||||
// `users.deactivated_at` + cascade-revokes existing sessions.
|
||||
// Without this gate the very next OIDC login mints a fresh
|
||||
// session and re-elevates. Compliance-table-flipping bug.
|
||||
//
|
||||
// Defense order: the check runs BEFORE the email / display-name
|
||||
// mutation + last_login_at bump so a deactivated user's
|
||||
// last_login_at field isn't silently updated by a rejected
|
||||
// login attempt (forensics value).
|
||||
if existing.DeactivatedAt != nil {
|
||||
return nil, ErrUserDeactivated
|
||||
}
|
||||
// Update last_login_at, email, display_name (per the Phase 1
|
||||
// mutable-field contract).
|
||||
existing.Email = email
|
||||
|
||||
@@ -934,6 +934,172 @@ func TestService_UpsertUser_UpdateExistingPath(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestService_HandleCallback_RejectsDeactivatedUser pins the A-2
|
||||
// CRIT closure. A federated user whose `users.deactivated_at` is
|
||||
// non-nil must NOT be able to log in via OIDC; HandleCallback must
|
||||
// return ErrUserDeactivated BEFORE the email/display-name mutation
|
||||
// and last_login_at bump, and BEFORE the session mint.
|
||||
//
|
||||
// Audit 2026-05-11 A-2 — pre-fix, the deactivate handler set
|
||||
// `users.deactivated_at` on the in-memory struct, but: (a) the SQL
|
||||
// Update omitted the column so the write was a no-op; (b) the
|
||||
// postgres SELECT didn't include the column so even if (a) were
|
||||
// fixed scanUser returned DeactivatedAt = nil; (c) upsertUser never
|
||||
// looked at DeactivatedAt. The lying-field chain meant the very
|
||||
// next OIDC login re-elevated the user. This test pins the
|
||||
// service-layer leg of the closure (the SQL legs are pinned by
|
||||
// postgres/user_test.go).
|
||||
func TestService_HandleCallback_RejectsDeactivatedUser(t *testing.T) {
|
||||
idp := newMockIdP(t)
|
||||
users := newStubUsers()
|
||||
|
||||
// Pre-seed the user as deactivated. The default mockIdP subject
|
||||
// is "test-subject"; the provider ID is "op-deact".
|
||||
deactivatedAt := time.Now().UTC().Add(-1 * time.Hour)
|
||||
prov := makeProvider(idp.URL(), "op-deact")
|
||||
seeded := &userdomain.User{
|
||||
ID: "u-deactivated",
|
||||
TenantID: prov.TenantID,
|
||||
Email: "deactivated@example.com",
|
||||
DisplayName: "Deactivated User",
|
||||
OIDCSubject: "test-subject",
|
||||
OIDCProviderID: "op-deact",
|
||||
LastLoginAt: time.Now().UTC().Add(-2 * time.Hour),
|
||||
WebAuthnCredentials: []byte("[]"),
|
||||
CreatedAt: time.Now().UTC().Add(-24 * time.Hour),
|
||||
UpdatedAt: time.Now().UTC().Add(-1 * time.Hour),
|
||||
DeactivatedAt: &deactivatedAt,
|
||||
}
|
||||
users.byID[seeded.ID] = seeded
|
||||
users.bySubject["op-deact:test-subject"] = seeded
|
||||
originalLastLogin := seeded.LastLoginAt
|
||||
|
||||
pl := newStubPreLogin()
|
||||
mappings := &stubMappings{roleIDs: []string{"r-operator"}}
|
||||
sessions := &stubSessions{}
|
||||
svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "")
|
||||
|
||||
cookie, _, err := pl.CreatePreLogin(context.Background(), "op-deact", "deact-state", "test-nonce-fixed",
|
||||
"v-deactiveeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", "", "")
|
||||
if err != nil {
|
||||
t.Fatalf("CreatePreLogin: %v", err)
|
||||
}
|
||||
|
||||
res, err := svc.HandleCallback(context.Background(), cookie, "code", "deact-state", "", "10.0.0.1", "Mozilla/5.0")
|
||||
if !errors.Is(err, ErrUserDeactivated) {
|
||||
t.Fatalf("err = %v; want ErrUserDeactivated", err)
|
||||
}
|
||||
if res != nil {
|
||||
t.Errorf("CallbackResult should be nil on rejection, got %+v", res)
|
||||
}
|
||||
|
||||
// Defense order pin — the rejected attempt must NOT have touched
|
||||
// the persisted row's mutable fields. (Pre-fix the upsertUser
|
||||
// path would update email + last_login_at first and only catch
|
||||
// later; A-2 closure moves the check to the head of the function.)
|
||||
row := users.byID["u-deactivated"]
|
||||
if row.LastLoginAt != originalLastLogin {
|
||||
t.Errorf("last_login_at advanced on rejected login: %v -> %v", originalLastLogin, row.LastLoginAt)
|
||||
}
|
||||
if row.Email != "deactivated@example.com" {
|
||||
t.Errorf("email mutated on rejected login: %q", row.Email)
|
||||
}
|
||||
if row.DeactivatedAt == nil {
|
||||
t.Error("deactivated_at was cleared on rejected login")
|
||||
}
|
||||
}
|
||||
|
||||
// TestService_HandleCallback_AllowsReactivatedUser covers the
|
||||
// Reactivate handler's wire end: after `users.deactivated_at` is
|
||||
// cleared, the next OIDC login goes through the update path
|
||||
// normally. Pins the inverse of TestService_HandleCallback_RejectsDeactivatedUser.
|
||||
func TestService_HandleCallback_AllowsReactivatedUser(t *testing.T) {
|
||||
idp := newMockIdP(t)
|
||||
users := newStubUsers()
|
||||
|
||||
prov := makeProvider(idp.URL(), "op-react")
|
||||
seeded := &userdomain.User{
|
||||
ID: "u-reactivated",
|
||||
TenantID: prov.TenantID,
|
||||
Email: "reactivated@example.com",
|
||||
DisplayName: "Reactivated User",
|
||||
OIDCSubject: "test-subject",
|
||||
OIDCProviderID: "op-react",
|
||||
LastLoginAt: time.Now().UTC().Add(-2 * time.Hour),
|
||||
WebAuthnCredentials: []byte("[]"),
|
||||
CreatedAt: time.Now().UTC().Add(-24 * time.Hour),
|
||||
UpdatedAt: time.Now().UTC().Add(-1 * time.Hour),
|
||||
DeactivatedAt: nil, // active again
|
||||
}
|
||||
users.byID[seeded.ID] = seeded
|
||||
users.bySubject["op-react:test-subject"] = seeded
|
||||
|
||||
pl := newStubPreLogin()
|
||||
mappings := &stubMappings{roleIDs: []string{"r-operator"}}
|
||||
sessions := &stubSessions{}
|
||||
svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "")
|
||||
|
||||
cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-react", "react-state", "test-nonce-fixed",
|
||||
"v-reactiveeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee", "", "")
|
||||
res, err := svc.HandleCallback(context.Background(), cookie, "code", "react-state", "", "10.0.0.1", "Mozilla/5.0")
|
||||
if err != nil {
|
||||
t.Fatalf("HandleCallback after reactivation: %v", err)
|
||||
}
|
||||
if res == nil || res.User == nil {
|
||||
t.Fatal("expected non-nil callback result after reactivation")
|
||||
}
|
||||
if res.User.ID != "u-reactivated" {
|
||||
t.Errorf("CallbackResult.User.ID = %q; want u-reactivated", res.User.ID)
|
||||
}
|
||||
}
|
||||
|
||||
// TestService_HandleCallback_DeactivatedUserPreservesForensics
|
||||
// makes the defense-in-depth claim explicit: a rejected login does
|
||||
// not bump last_login_at. This guards against a regression where
|
||||
// someone "fixes" upsertUser by re-ordering the assignments to set
|
||||
// LastLoginAt before checking DeactivatedAt.
|
||||
func TestService_HandleCallback_DeactivatedUserPreservesForensics(t *testing.T) {
|
||||
idp := newMockIdP(t)
|
||||
users := newStubUsers()
|
||||
|
||||
deactivatedAt := time.Now().UTC().Add(-30 * time.Minute)
|
||||
prov := makeProvider(idp.URL(), "op-forensic")
|
||||
seeded := &userdomain.User{
|
||||
ID: "u-forensic",
|
||||
TenantID: prov.TenantID,
|
||||
Email: "forensic@example.com",
|
||||
DisplayName: "Forensic User",
|
||||
OIDCSubject: "test-subject",
|
||||
OIDCProviderID: "op-forensic",
|
||||
LastLoginAt: time.Now().UTC().Add(-48 * time.Hour),
|
||||
WebAuthnCredentials: []byte("[]"),
|
||||
CreatedAt: time.Now().UTC().Add(-72 * time.Hour),
|
||||
UpdatedAt: time.Now().UTC().Add(-30 * time.Minute),
|
||||
DeactivatedAt: &deactivatedAt,
|
||||
}
|
||||
users.byID[seeded.ID] = seeded
|
||||
users.bySubject["op-forensic:test-subject"] = seeded
|
||||
frozenLastLogin := seeded.LastLoginAt
|
||||
|
||||
pl := newStubPreLogin()
|
||||
mappings := &stubMappings{roleIDs: []string{"r-operator"}}
|
||||
sessions := &stubSessions{}
|
||||
svc := NewService(&stubProviderLookup{provider: prov}, mappings, users, sessions, pl, "")
|
||||
|
||||
cookie, _, _ := pl.CreatePreLogin(context.Background(), "op-forensic", "for-state", "test-nonce-fixed",
|
||||
"v-forensiccccccccccccccccccccccccccccccccc", "", "")
|
||||
_, err := svc.HandleCallback(context.Background(), cookie, "code", "for-state", "", "10.0.0.1", "Mozilla/5.0")
|
||||
if !errors.Is(err, ErrUserDeactivated) {
|
||||
t.Fatalf("err = %v; want ErrUserDeactivated", err)
|
||||
}
|
||||
|
||||
row := users.byID["u-forensic"]
|
||||
if row.LastLoginAt != frozenLastLogin {
|
||||
t.Errorf("last_login_at advanced on rejected login (forensics tainted): %v -> %v",
|
||||
frozenLastLogin, row.LastLoginAt)
|
||||
}
|
||||
}
|
||||
|
||||
// TestService_ATHash_CoversAllAllowedAlgs pins the at_hash alg dispatch
|
||||
// for every algorithm in DefaultAllowedAlgs.
|
||||
func TestService_ATHash_CoversAllAllowedAlgs(t *testing.T) {
|
||||
|
||||
Reference in New Issue
Block a user