fix(api): ConsumeLoginEmailOTP honesty — wrong/expired/consumed codes are ErrOTPInvalid 400, not a 500
The PG implementation was a single UPDATE ... WHERE code_hash that returned
ErrNotFound on zero rows: every wrong, expired, replayed or superseded code on
the pre-session email-login door (and the op-login finish / migration confirm
doors) fell through to writeError's unmapped-error 500, and attempts were never
charged so otpMaxAttempts/ErrOTPLocked could not trigger. The fake repo and the
Repo interface ("SAME code lifecycle as VerifyEmailOTP") already documented the
intended contract; only the PG side had drifted.
Mirror VerifyEmailOTP's transaction without its users write: SELECT ... FOR
UPDATE the newest live row, expiry + attempt cap before the hash compare,
mismatch charges one attempt and returns ErrOTPInvalid without consuming,
match consumes and commits. Verified live on the VM: 5 wrong guesses return
400 and stop at attempts=5 (correct code then also refused, unconsumed);
fresh code redeems; replay returns 400.
This commit is contained in:
1 file changed
+48
-16
+48
-16
@@ -1878,29 +1878,61 @@ func (p *PGRepo) UserByEmail(ctx context.Context, email string) (*StaffUser, err
|
|||||||
return &u, nil
|
return &u, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// ConsumeLoginEmailOTP redeems a live code for the PRE-SESSION email login door.
|
// ConsumeLoginEmailOTP redeems the live code for the PRE-SESSION email login door
|
||||||
// Unlike VerifyEmailOTP it has no identity side-effects: it neither writes
|
// with the SAME lifecycle as VerifyEmailOTP (FOR UPDATE, expiry + attempt cap
|
||||||
// users.email nor runs the verified-email uniqueness guard — login already
|
// before the hash compare, a mismatch charges one attempt without consuming) but
|
||||||
// resolved the userID via UserByEmail, which requires email_verified, so the
|
// with NO identity side-effects: it neither writes users.email nor runs the
|
||||||
// address is settled. Zero rows affected (no live code, expired, consumed, or
|
// verified-email uniqueness guard — login already resolved the userID via
|
||||||
// hash mismatch) → ErrNotFound.
|
// UserByEmail, which requires email_verified, so the address is settled. Errors
|
||||||
|
// are exactly ErrOTPInvalid / ErrOTPLocked (ErrEmailTaken is structurally
|
||||||
|
// impossible here).
|
||||||
func (p *PGRepo) ConsumeLoginEmailOTP(ctx context.Context, userID, purpose, codeHash string, now time.Time) error {
|
func (p *PGRepo) ConsumeLoginEmailOTP(ctx context.Context, userID, purpose, codeHash string, now time.Time) error {
|
||||||
res, err := p.db.ExecContext(ctx,
|
tx, err := p.db.BeginTx(ctx, nil)
|
||||||
`UPDATE email_otps SET consumed_at = $4
|
|
||||||
WHERE user_id = $1 AND purpose = $2 AND code_hash = $3
|
|
||||||
AND consumed_at IS NULL AND expires_at > $4`,
|
|
||||||
userID, purpose, codeHash, now)
|
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
n, err := res.RowsAffected()
|
defer tx.Rollback() //nolint:errcheck // no-op after commit
|
||||||
if err != nil {
|
|
||||||
|
var (
|
||||||
|
id string
|
||||||
|
storedHash string
|
||||||
|
attempts int
|
||||||
|
expiresAt time.Time
|
||||||
|
)
|
||||||
|
switch err := tx.QueryRowContext(ctx,
|
||||||
|
`SELECT id, code_hash, attempts, expires_at FROM email_otps
|
||||||
|
WHERE user_id = $1 AND purpose = $2 AND consumed_at IS NULL
|
||||||
|
ORDER BY created_at DESC LIMIT 1 FOR UPDATE`,
|
||||||
|
userID, purpose).Scan(&id, &storedHash, &attempts, &expiresAt); {
|
||||||
|
case errors.Is(err, sql.ErrNoRows):
|
||||||
|
// Nothing live: never minted, already consumed, or superseded.
|
||||||
|
return ErrOTPInvalid
|
||||||
|
case err != nil:
|
||||||
return err
|
return err
|
||||||
}
|
}
|
||||||
if n == 0 {
|
|
||||||
return ErrNotFound
|
if !expiresAt.After(now) {
|
||||||
|
return ErrOTPInvalid
|
||||||
}
|
}
|
||||||
return nil
|
if attempts >= otpMaxAttempts {
|
||||||
|
return ErrOTPLocked
|
||||||
|
}
|
||||||
|
if storedHash != codeHash {
|
||||||
|
if _, err := tx.ExecContext(ctx,
|
||||||
|
`UPDATE email_otps SET attempts = attempts + 1 WHERE id = $1`, id); err != nil {
|
||||||
|
return fmt.Errorf("record otp attempt: %w", err)
|
||||||
|
}
|
||||||
|
if err := tx.Commit(); err != nil {
|
||||||
|
return err
|
||||||
|
}
|
||||||
|
return ErrOTPInvalid
|
||||||
|
}
|
||||||
|
|
||||||
|
if _, err := tx.ExecContext(ctx,
|
||||||
|
`UPDATE email_otps SET consumed_at = $2 WHERE id = $1`, id, now); err != nil {
|
||||||
|
return fmt.Errorf("consume otp: %w", err)
|
||||||
|
}
|
||||||
|
return tx.Commit()
|
||||||
}
|
}
|
||||||
|
|
||||||
// ---- op.console staff login: in-game approval state machine (spec §B op-login) ----
|
// ---- op.console staff login: in-game approval state machine (spec §B op-login) ----
|
||||||
|
|||||||
Reference in new issue
Block a user