Add email-based password reset flow with PasswordResetStore interface#4
Conversation
…erface Agent-Logs-Url: https://github.com/amalgamated-tools/goauth/sessions/dcc0e9ea-8e81-40f6-8af6-ba5f8292d088 Co-authored-by: veverkap <22348+veverkap@users.noreply.github.com>
|
/grumpy |
|
😤 Fine. Grumpy Code Reviewer 🔥 finished the review. It wasn't completely terrible. I guess. 🙄 |
There was a problem hiding this comment.
Pull request overview
Adds an email-based password reset flow to the handler package and introduces a new auth.PasswordResetStore interface to let consuming applications persist and validate reset tokens.
Changes:
- Added
PasswordResetTokenandPasswordResetStoretoauth/types.gofor reset token persistence/lookup/cleanup. - Implemented
PasswordResetHandlerwith/password-reset/requestand/password-reset/confirmendpoints. - Added comprehensive handler tests plus a
mockPasswordResetStorefor test support.
Show a summary per file
| File | Description |
|---|---|
auth/types.go |
Introduces reset token model + storage interface for implementations to provide persistence. |
handler/password_reset.go |
Implements request + confirm endpoints including token generation, hashing, persistence, and password update. |
handler/password_reset_test.go |
Adds tests for success/error paths and TTL defaulting behavior. |
handler/helpers_test.go |
Adds mockPasswordResetStore used by the new handler tests. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
handler/password_reset.go:139
- ResetPassword updates the password solely based on resetToken.UserID. If an OIDC-only user ever obtains a reset token (e.g., via a future store implementation or an earlier RequestReset bug), this will set a password for an account that previously had no password hash. To preserve the current contract that OIDC-only accounts cannot use password auth, fetch the user (e.g., Users.FindByID) and reject resets when PasswordHash is empty (or otherwise encode that constraint in the reset token/store).
if err := h.Users.UpdatePassword(r.Context(), resetToken.UserID, string(hash)); err != nil {
slog.ErrorContext(r.Context(), "password reset: update password", slog.Any("error", err))
writeError(r.Context(), w, http.StatusInternalServerError, "failed to update password")
return
- Files reviewed: 4/4 changed files
- Comments generated: 3
There was a problem hiding this comment.
Four real issues, one minor oversight. The sql.ErrNoRows in the handler is a layering violation that'll bite you later. The orphaned token on email failure is a correctness bug. No rate limiting on an unauthenticated write endpoint is a security gap. The cleanup method has no callers and no docs — it's a maintenance trap waiting to spring. Fix those before merging.
😤 Reluctantly reviewed by Grumpy Code Reviewer 🔥
To install this agentic workflow, run
gh aw add amalgamated-tools/biblioteka/.github/workflows/grumpy-reviewer.md@0dff8ccb0ced8d634877b4201f25795e659dced0
- Add auth.ErrNotFound sentinel; handlers no longer import database/sql - Skip token creation/reset for OIDC-only accounts (PasswordHash == "") - Delete orphaned token when SendResetEmail fails to avoid ghost tokens - Handle auth.ErrExpiredToken as a client error (400) alongside ErrInvalidToken - Add slog.ErrorContext for bcrypt hash failures in ResetPassword - Add optional RateLimiter field to PasswordResetHandler for abuse protection - Add RateLimiter.Allow helper method to auth package - Document DeleteExpiredPasswordResetTokens cleanup scheduling requirement - Update all tests to reflect new behaviour; add coverage for new paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds a complete email-based password reset flow: token generation, hashed storage, delivery via caller-supplied callback, and consumption on use.
Interface (
auth/types.go)New
PasswordResetTokenstruct andPasswordResetStoreinterface:Handler (
handler/password_reset.go)PasswordResetHandlerwith two endpoints:RequestReset— looks up user by email, generates 32 random bytes, stores SHA-256 hash, callsSendResetEmail(ctx, email, rawToken). Always returns200 OKto prevent email enumeration.ResetPassword— hashes the submitted token, validates via store, checks expiry, bcrypt-hashes the new password, updates it, then deletes the token. Token deletion failure is logged but does not fail the response since the password update already succeeded.Token security follows the same pattern as API keys: raw token is returned to the caller only at creation; only its SHA-256 hash is persisted.