fix(api): dead accounts cannot log in, hold sessions, or keep identity assets

This commit is contained in:
Lemon-miaow committed 2026-09-23 05:28:32 +08:00
1 parent ba9d98f7ce
commit 58535890c4
7 files changed
+306 -11

No files matched your search

+60 -1
View File
@@ -103,6 +103,10 @@ type fakeRepo struct {
// user admin fakes
seededUsers []seededUser
fakeQuotas map[string]*QuotaView
// deletedIDs remembers soft-deleted user ids: DeleteUser drops the row from
// seededUsers (so listings hide it, mirroring the WHERE deleted_at IS NULL
// query), and this set keeps the account dead for the liveness guards.
deletedIDs map[string]bool
// pingErr, when non-nil, is returned by Ping to simulate DB liveness check
// failures in /readyz tests.
pingErr error
@@ -233,6 +237,7 @@ func newFakeRepo() *fakeRepo {
discoverableChallenges: map[string]*fakeDiscoverableChallenge{},
fakeQuotas: map[string]*QuotaView{},
migrations: map[string]*fakeMigration{},
deletedIDs: map[string]bool{},
}
}
@@ -315,6 +320,9 @@ func (f *fakeRepo) RedeemPlayerBindCode(_ context.Context, newUserID, code strin
return "", "", "", ErrPlayerBindForbidden // staff must use op.console; do not consume
}
}
if f.seededDead(existing) {
return "", "", "", ErrPlayerAccountRetired // dead account; do not consume
}
delete(f.linkCodes, code)
return existing, rec.mcUUID, rec.authSource, nil
}
@@ -803,6 +811,9 @@ func (f *fakeRepo) SessionUser(_ context.Context, tokenHash string, now time.Tim
}
for _, u := range f.staff {
if u.ID == s.userID {
if f.seededDead(u.ID) {
return nil, ErrNotFound
}
return &SessionedUser{
ID: u.ID, Email: u.Email, Role: u.Role,
}, nil
@@ -895,11 +906,29 @@ func (f *fakeRepo) ListUsers(_ context.Context, opts ListUsersOpts) ([]UserView,
}
func (f *fakeRepo) UserDetail(_ context.Context, userID string) (*UserDetail, error) {
deletedAt := time.Unix(1_700_000_000, 0)
for _, su := range f.seededUsers {
if su.view.ID == userID {
return &su.detail, nil
}
}
// Legacy fixtures seeded only into f.staff are live accounts (nothing marked
// them disabled or deleted), so detail reads must resolve them too — the
// liveness guards (discoverable login, owner protection) treat "unknown" as a
// fault, and these fixtures are known.
for _, u := range f.staff {
if u.ID == userID {
if f.deletedIDs[userID] {
// A soft-deleted account still HAS a detail row; it is flagged, not gone.
return &UserDetail{UserView: UserView{
ID: u.ID, Username: u.Username, Role: u.Role, Disabled: true,
}, DeletedAt: &deletedAt}, nil
}
return &UserDetail{UserView: UserView{
ID: u.ID, Username: u.Username, Email: u.Email, Role: u.Role,
}}, nil
}
}
return nil, ErrNotFound
}
@@ -959,6 +988,20 @@ func (f *fakeRepo) DeleteUser(_ context.Context, userID, _ string) error {
for i, su := range f.seededUsers {
if su.view.ID == userID {
f.seededUsers = append(f.seededUsers[:i], f.seededUsers[i+1:]...)
f.deletedIDs[userID] = true
// Mirror PGRepo: deletion severs the account's identity assets so the
// closed account keeps neither a login credential nor a MC-UUID claim.
for uuid, uid := range f.links {
if uid == userID {
delete(f.links, uuid)
delete(f.linkAuthSource, uuid)
}
}
for cid, cred := range f.passkeyCreds {
if cred.UserID == userID {
delete(f.passkeyCreds, cid)
}
}
return nil
}
}
@@ -1118,6 +1161,22 @@ func (f *fakeRepo) liveUserExists(id string) bool {
return false
}
// seededDead mirrors PGRepo's liveness filters (audit #33): a seeded user that was
// disabled or soft-deleted is dead for the login doors and session validation. A
// fixture that was never seeded (legacy tests put it straight into f.staff) is
// treated as live, matching the fakes' pre-existing behavior.
func (f *fakeRepo) seededDead(id string) bool {
if f.deletedIDs[id] {
return true
}
for _, su := range f.seededUsers {
if su.view.ID == id {
return su.view.Disabled || su.detail.DeletedAt != nil
}
}
return false
}
func (f *fakeRepo) GetQuotas(_ context.Context, userID string) (*QuotaView, error) {
if !f.liveUserExists(userID) {
return nil, ErrNotFound
@@ -1212,7 +1271,7 @@ func (f *fakeRepo) LinkAccount(_ context.Context, userID, mcUUID, authSource str
// is indistinguishable from no account: both yield ErrNotFound.
func (f *fakeRepo) UserByEmail(_ context.Context, email string) (*StaffUser, error) {
for _, u := range f.staff {
if u.EmailVerified && strings.EqualFold(u.Email, email) {
if u.EmailVerified && strings.EqualFold(u.Email, email) && !f.seededDead(u.ID) {
su := *u
return &su, nil
}
+7
View File
@@ -57,6 +57,13 @@ var (
// provably never mints a session for a staff identity. It is distinct from
// ErrConflict so the handler answers 403 (wrong door) rather than 409.
ErrPlayerBindForbidden = errors.New("bind code belongs to a staff account")
// ErrPlayerAccountRetired means a Bind-Code redemption resolved to an account the
// platform has closed: an owner soft-deleted it, or it is disabled (locked out).
// Reusing the row would mint a fresh session for a dead account — the same
// resurrection the login doors refuse by resolving only live accounts — so the
// redeemer gets an explicit 403 instead. The code is NOT consumed, so re-enabling
// the account and retrying still works within the code's TTL.
ErrPlayerAccountRetired = errors.New("player account is retired or disabled")
// ErrEmailTaken means a verified email would collide with another account's
// already-verified address (spec §B email-first login foundation; the
// users_verified_email_unique index ships in migration 0020).
+93
View File
@@ -1,7 +1,9 @@
package api
import (
"context"
"encoding/json"
"errors"
"net/http"
"net/http/httptest"
"testing"
@@ -618,3 +620,94 @@ func TestLoginEmailStartFailedDeliveryReleasesCooldown(t *testing.T) {
t.Errorf("mailer calls = %d, want 2 (one failed, one delivered)", mailer.calls)
}
}
// A disabled or soft-deleted account is DEAD at every door: the pre-session
// resolvers refuse it (uniformly, so the door stays no-oracle), a session that
// was live a moment ago stops authenticating, DeleteUser severs the account's
// passkeys and Minecraft links, and the bind door refuses to reuse the retired
// identity instead of minting a session for it. Audit #33 found the opposite
// live: a deleted user re-logged-in through the email door and GET /me answered
// 200 — deletion and the disable lockout were both bypassable by logging in again.
func TestDeadAccountsCannotLogInOrKeepSessions(t *testing.T) {
ctx := context.Background()
now := time.Unix(1_700_000_000, 0)
repo := newFakeRepo()
repo.settings[LocalAuthEnabledKey] = []byte("true")
repo.seedUser(UserView{ID: "u-dead", Username: "dead", Email: "[email protected]", Role: "user"})
repo.staff["dead"].EmailVerified = true
mailer := &captureMailer{}
api := newTestAPI(repo, newFakeCluster())
api.Mailer = mailer
eh := api.ExternalHandler()
// Control: alive — the door resolves the account and mails a real code, and a
// session minted for it authenticates.
if w := do(eh, "POST", "/api/v1/auth/email/start", `{"email":"[email protected]"}`, jsonHeader); w.Code != http.StatusAccepted {
t.Fatalf("alive start: code = %d, want 202 (%s)", w.Code, w.Body.String())
}
if mailer.calls != 1 {
t.Fatalf("alive start mailed %d codes, want 1", mailer.calls)
}
repo.sessions["h-live"] = &fakeSession{userID: "u-dead", expiresAt: now.Add(time.Hour)}
if _, err := repo.SessionUser(ctx, "h-live", now); err != nil {
t.Fatalf("live SessionUser: %v", err)
}
// Disabled: the start is neutral (no mail), verify refuses, session dies.
if err := repo.SetUserDisabled(ctx, "u-dead", true); err != nil {
t.Fatalf("disable: %v", err)
}
// The alive start's reservation must not mask the neutral branch: clear the
// throttle's window (test-only; the limiter itself is rebuilt lazily once).
lim := api.otpLimiter()
lim.mu.Lock()
lim.last = map[string]time.Time{}
lim.mu.Unlock()
if w := do(eh, "POST", "/api/v1/auth/email/start", `{"email":"[email protected]"}`, jsonHeader); w.Code != http.StatusAccepted {
t.Fatalf("disabled start: code = %d, want 202 neutral (%s)", w.Code, w.Body.String())
}
if mailer.calls != 1 {
t.Fatalf("disabled start mailed a code (%d calls) — a dead account must resolve to nothing", mailer.calls)
}
if w := do(eh, "POST", "/api/v1/auth/email/verify", `{"email":"[email protected]","code":"000000"}`, jsonHeader); w.Code != http.StatusBadRequest || decodeErr(t, w) != "invalid_code" {
t.Fatalf("disabled verify: code = %d body %s, want 400 invalid_code", w.Code, w.Body.String())
}
if _, err := repo.SessionUser(ctx, "h-live", now); !errors.Is(err, ErrNotFound) {
t.Fatalf("disabled SessionUser = %v, want ErrNotFound", err)
}
// Deleted: same refusals; assets severed (links released, passkeys dropped).
if err := repo.SetUserDisabled(ctx, "u-dead", false); err != nil {
t.Fatalf("re-enable: %v", err)
}
uuid := "11111111-2222-3333-4444-555555555555"
repo.links[uuid] = "u-dead"
repo.passkeyCreds["pk-1"] = PasskeyCredential{ID: "pk-1", UserID: "u-dead", CredentialID: "cred-1"}
if err := repo.DeleteUser(ctx, "u-dead", "test"); err != nil {
t.Fatalf("delete: %v", err)
}
if w := do(eh, "POST", "/api/v1/auth/email/verify", `{"email":"[email protected]","code":"000000"}`, jsonHeader); w.Code != http.StatusBadRequest || decodeErr(t, w) != "invalid_code" {
t.Fatalf("deleted verify: code = %d body %s, want 400 invalid_code", w.Code, w.Body.String())
}
if _, err := repo.SessionUser(ctx, "h-live", now); !errors.Is(err, ErrNotFound) {
t.Fatalf("deleted SessionUser = %v, want ErrNotFound", err)
}
if _, ok := repo.links[uuid]; ok {
t.Error("DeleteUser left the Minecraft link: the UUID stays claimed forever")
}
if _, ok := repo.passkeyCreds["pk-1"]; ok {
t.Error("DeleteUser left the passkey: a login credential outlives the account")
}
// The bind door refuses to reuse the retired identity (a stale link that
// predates the fix, or a username squatted by the deleted row).
repo.links[uuid] = "u-dead"
repo.linkCodes["CODE1234"] = fakeLinkCode{mcUUID: uuid, authSource: "mojang", expiresAt: now.Add(time.Hour)}
if _, _, _, err := repo.RedeemPlayerBindCode(ctx, "u-new", "CODE1234", now); !errors.Is(err, ErrPlayerAccountRetired) {
t.Fatalf("bind redeem onto a deleted account = %v, want ErrPlayerAccountRetired", err)
}
if _, ok := repo.linkCodes["CODE1234"]; !ok {
t.Error("refused redeem consumed the code; re-enabling the account must stay retryable within TTL")
}
}
+8
View File
@@ -109,6 +109,14 @@ func (a *API) handleBindRedeem(w http.ResponseWriter, r *http.Request) {
writeError(w, r, newError(http.StatusForbidden, "staff_account",
"that Minecraft account belongs to staff; sign in at the operator console"))
return
case errors.Is(err, ErrPlayerAccountRetired):
// The linked Felis account is disabled or soft-deleted: the door refuses to
// reuse it, because minting a session here would resurrect the account the
// owner just retired (audit #33). The code survives, so re-enabling the
// account and retrying within its TTL still works.
writeError(w, r, newError(http.StatusForbidden, "account_retired",
"this Minecraft account's Felis account is disabled or deleted; contact the operator"))
return
case err != nil:
writeError(w, r, err)
return
@@ -148,6 +148,13 @@ func (a *API) handlePasskeyLoginDiscoverableFinish(w http.ResponseWriter, r *htt
if err != nil {
return PasskeyUser{}, err
}
// A disabled or soft-deleted account must not complete a login even when it
// still holds a credential (UserByID is an unfiltered lookup shared with admin
// reads, so the liveness check lives here, at the door). Fail closed with the
// same opaque outcome as an unknown handle (audit #33).
if d, err := a.Repo.UserDetail(r.Context(), u.ID); err != nil || d.Disabled || d.DeletedAt != nil {
return PasskeyUser{}, ErrNotFound
}
creds, err := a.Repo.PasskeyCredentialsForUser(r.Context(), u.ID)
if err != nil {
return PasskeyUser{}, err
+48 -10
View File
@@ -161,13 +161,17 @@ func (p *PGRepo) RedeemPlayerBindCode(ctx context.Context, newUserID, code strin
// Create-or-fetch keyed on the verified UUID. An already-linked role='user' player
// is fetched (idempotent "log in via the game"); any STAFF account (admin or
// owner, i.e. role != 'user') is refused (op.console only) BEFORE any consume, so
// the code survives; an unlinked UUID births a fresh role='user' player with a
// uuid-derived unique username.
// the code survives; a DISABLED or soft-deleted account is refused the same way
// (audit #33 — a dead account must not resurrect through the bind door); an
// unlinked UUID births a fresh role='user' player with a uuid-derived unique
// username.
userID := newUserID
var existingRole string
var disabled, deleted bool
switch err := tx.QueryRowContext(ctx,
`SELECT u.id, u.role::text FROM account_links al JOIN users u ON u.id = al.user_id WHERE al.mc_uuid = $1`,
mcUUID).Scan(&userID, &existingRole); {
`SELECT u.id, u.role::text, u.disabled, u.deleted_at IS NOT NULL
FROM account_links al JOIN users u ON u.id = al.user_id WHERE al.mc_uuid = $1`,
mcUUID).Scan(&userID, &existingRole, &disabled, &deleted); {
case errors.Is(err, sql.ErrNoRows):
if _, err := tx.ExecContext(ctx,
`INSERT INTO users (id, username, role) VALUES ($1, $2, 'user')
@@ -176,15 +180,21 @@ func (p *PGRepo) RedeemPlayerBindCode(ctx context.Context, newUserID, code strin
return "", "", "", fmt.Errorf("create player: %w", err)
}
// Re-read by username so a cross-code race converges on the winner's row
// (our id was discarded by DO NOTHING) instead of a bare 500.
// (our id was discarded by DO NOTHING) instead of a bare 500 — and so a
// deleted row squatting on the username is refused rather than reused.
var role string
var dis, del bool
if err := tx.QueryRowContext(ctx,
`SELECT id, role::text FROM users WHERE username = $1`, mcUUID).Scan(&userID, &role); err != nil {
`SELECT id, role::text, disabled, deleted_at IS NOT NULL FROM users WHERE username = $1`,
mcUUID).Scan(&userID, &role, &dis, &del); err != nil {
return "", "", "", fmt.Errorf("create player: %w", err)
}
if role != "user" {
return "", "", "", ErrPlayerBindForbidden
}
if dis || del {
return "", "", "", ErrPlayerAccountRetired
}
if _, err := tx.ExecContext(ctx,
`INSERT INTO account_links (user_id, mc_uuid, auth_source) VALUES ($1, $2, $3)
ON CONFLICT (mc_uuid) DO NOTHING`,
@@ -197,6 +207,9 @@ func (p *PGRepo) RedeemPlayerBindCode(ctx context.Context, newUserID, code strin
if existingRole != "user" {
return "", "", "", ErrPlayerBindForbidden // staff must use op.console
}
if disabled || deleted {
return "", "", "", ErrPlayerAccountRetired
}
}
if _, err := tx.ExecContext(ctx,
@@ -1025,9 +1038,14 @@ func (p *PGRepo) CreateSession(ctx context.Context, tokenHash, userID string, ex
// SessionUser resolves a live (unrevoked, unexpired at now) session hash to its
// user, or ErrNotFound.
func (p *PGRepo) SessionUser(ctx context.Context, tokenHash string, now time.Time) (*SessionedUser, error) {
// The disabled/deleted filter is the belt to the doors' braces: even a session
// minted for an account that was alive a moment ago stops authenticating the
// instant the account is disabled or soft-deleted, so every authenticated route
// is fail-closed regardless of which door minted the cookie (audit #33).
const q = `SELECT u.id, COALESCE(u.email, ''), u.role::text, COALESCE(u.email_verified, false)
FROM sessions s JOIN users u ON u.id = s.user_id
WHERE s.token_hash = $1 AND s.revoked_at IS NULL AND s.expires_at > $2`
WHERE s.token_hash = $1 AND s.revoked_at IS NULL AND s.expires_at > $2
AND u.disabled = false AND u.deleted_at IS NULL`
var u SessionedUser
switch err := p.db.QueryRowContext(ctx, q, tokenHash, now).Scan(
&u.ID, &u.Email, &u.Role, &u.EmailVerified); {
@@ -1557,8 +1575,11 @@ func (p *PGRepo) userView(ctx context.Context, userID string) (*UserView, error)
}
// DeleteUser soft-deletes a user in one transaction: sets deleted_at, revokes
// every live session, and releases every owned server. The row is preserved so
// audit_logs.actor references survive.
// every live session, releases every owned server, and severs the account's
// identity assets (passkey credentials, Minecraft links) so a closed account
// cannot keep a login credential or pin an in-game identity via
// UNIQUE(mc_uuid)(audit #33). The user row is preserved so audit_logs.actor
// references survive.
func (p *PGRepo) DeleteUser(ctx context.Context, userID, _ string) error {
tx, err := p.db.BeginTx(ctx, nil)
if err != nil {
@@ -1591,6 +1612,18 @@ func (p *PGRepo) DeleteUser(ctx context.Context, userID, _ string) error {
return err
}
// Sever the login credentials and in-game bindings: a passkey is a standing
// login foothold and occupies credential_id UNIQUE, and an account_links row
// would keep the Minecraft UUID claimed forever, blocking any future binding.
if _, err := tx.ExecContext(ctx,
`DELETE FROM webauthn_credentials WHERE user_id = $1`, userID); err != nil {
return err
}
if _, err := tx.ExecContext(ctx,
`DELETE FROM account_links WHERE user_id = $1`, userID); err != nil {
return err
}
// Soft-delete the user row.
if _, err := tx.ExecContext(ctx,
`UPDATE users SET disabled = true, deleted_at = now() WHERE id = $1`,
@@ -2002,8 +2035,13 @@ func (p *PGRepo) RedeemMigration(ctx context.Context, targetUserID, codeHash str
func (p *PGRepo) UserByEmail(ctx context.Context, email string) (*StaffUser, error) {
// lower() on both sides honors the interface's case-insensitivity contract
// and matches the users_verified_email_unique index (lower(email)).
// Disabled and soft-deleted accounts are invisible here on purpose: every caller
// is a pre-session LOGIN door (console email/passkey, op-login, /auth/options),
// and a dead account must not be able to mint a session again — deletion and the
// disable lockout would otherwise be bypassable by simply logging in (audit #33).
const q = `SELECT id, username, COALESCE(email, ''), role::text, email_verified
FROM users WHERE lower(email) = lower($1) AND email_verified = true`
FROM users WHERE lower(email) = lower($1) AND email_verified = true
AND disabled = false AND deleted_at IS NULL`
var u StaffUser
switch err := p.db.QueryRowContext(ctx, q, email).Scan(
&u.ID, &u.Username, &u.Email, &u.Role, &u.EmailVerified); {
+83
View File
@@ -481,6 +481,89 @@ func TestAdminSubresourcesRequireLiveUser(t *testing.T) {
}
}
// A disabled or soft-deleted account must be dead in the real database: login
// resolution (UserByEmail) refuses it, session validation (SessionUser) refuses
// even a freshly inserted session, and DeleteUser severs the account's passkeys
// and Minecraft links so the closed account keeps neither a standing credential
// nor the UNIQUE(mc_uuid) claim. Live audit #33: a deleted user re-logged-in via
// the email door and held a working session.
func TestDeadAccountsAreLockedOutInPG(t *testing.T) {
ctx := context.Background()
u := newUser(t, "user", "dead")
addr := "dead-" + suffix(t) + "@example.net"
now := mustNow()
if err := repo.CreateEmailOTP(ctx, "de-"+suffix(t), u.ID, addr, "h", "onboard_email", now.Add(5*time.Minute)); err != nil {
t.Fatalf("CreateEmailOTP: %v", err)
}
if _, err := repo.VerifyEmailOTP(ctx, u.ID, "onboard_email", "h", now); err != nil {
t.Fatalf("VerifyEmailOTP: %v", err)
}
// Alive: the login door resolves the account and a session validates.
if _, err := repo.UserByEmail(ctx, addr); err != nil {
t.Fatalf("alive UserByEmail: %v", err)
}
hash := "h-" + suffix(t)
if err := repo.CreateSession(ctx, hash, u.ID, now.Add(time.Hour)); err != nil {
t.Fatalf("CreateSession: %v", err)
}
if _, err := repo.SessionUser(ctx, hash, now); err != nil {
t.Fatalf("alive SessionUser: %v", err)
}
// Disabled: invisible to the door, session stops authenticating.
if err := repo.SetUserDisabled(ctx, u.ID, true); err != nil {
t.Fatalf("SetUserDisabled: %v", err)
}
if _, err := repo.UserByEmail(ctx, addr); !errors.Is(err, api.ErrNotFound) {
t.Fatalf("disabled UserByEmail = %v, want ErrNotFound", err)
}
if _, err := repo.SessionUser(ctx, hash, now); !errors.Is(err, api.ErrNotFound) {
t.Fatalf("disabled SessionUser = %v, want ErrNotFound", err)
}
if err := repo.SetUserDisabled(ctx, u.ID, false); err != nil {
t.Fatalf("re-enable: %v", err)
}
// Deleted: same refusals, a fresh session cannot authenticate, and the
// identity assets are gone from the tables.
mc := testUUID(t)
if err := repo.LinkAccount(ctx, u.ID, mc, "mojang"); err != nil {
t.Fatalf("LinkAccount: %v", err)
}
if _, err := db.ExecContext(ctx,
`INSERT INTO webauthn_credentials (id, user_id, credential_id, public_key) VALUES ($1,$2,$3,'pk')`,
"cred-"+suffix(t), u.ID, "cid-"+suffix(t)); err != nil {
t.Fatalf("seed passkey: %v", err)
}
if err := repo.DeleteUser(ctx, u.ID, "pgint"); err != nil {
t.Fatalf("DeleteUser: %v", err)
}
if _, err := repo.UserByEmail(ctx, addr); !errors.Is(err, api.ErrNotFound) {
t.Fatalf("deleted UserByEmail = %v, want ErrNotFound", err)
}
hash2 := "h2-" + suffix(t)
if err := repo.CreateSession(ctx, hash2, u.ID, now.Add(time.Hour)); err != nil {
t.Fatalf("CreateSession (deleted): %v", err)
}
if _, err := repo.SessionUser(ctx, hash2, now); !errors.Is(err, api.ErrNotFound) {
t.Fatalf("deleted SessionUser = %v, want ErrNotFound", err)
}
for _, q := range []string{
`SELECT count(*) FROM account_links WHERE user_id = $1`,
`SELECT count(*) FROM webauthn_credentials WHERE user_id = $1`,
} {
var n int
if err := db.QueryRowContext(ctx, q, u.ID).Scan(&n); err != nil {
t.Fatalf("count: %v", err)
}
if n != 0 {
t.Errorf("DeleteUser left %d rows matching %q; the closed account must keep no asset", n, q)
}
}
}
// The owner tier the panel gates on must actually be WRITTEN: until this
// contract had a test, every provisioning path wrote 'admin', so the whole
// owner surface (user administration) was unreachable in a fresh install.