diff --git a/cmd/server/main.go b/cmd/server/main.go index 308c423..7ab73fc 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -5,10 +5,12 @@ import ( "log" "net/http" "strings" + "time" "github.com/jackc/pgx/v5/pgxpool" "github.com/vasyakrg/dns-autoresolver/internal/api" + "github.com/vasyakrg/dns-autoresolver/internal/auth" "github.com/vasyakrg/dns-autoresolver/internal/config" "github.com/vasyakrg/dns-autoresolver/internal/crypto" "github.com/vasyakrg/dns-autoresolver/internal/provider/registry" @@ -18,6 +20,10 @@ import ( "github.com/vasyakrg/dns-autoresolver/internal/web" ) +// sessionTTL is how long a login session cookie remains valid before the +// user must re-authenticate. +const sessionTTL = 720 * time.Hour + // isAPIPath reports whether path must be routed to the API router rather // than the SPA. "/api" (no trailing slash) counts as an API path too — // only strings.HasPrefix(path, "/api/") would otherwise miss it and fall @@ -46,12 +52,13 @@ func main() { log.Fatalf("cipher: %v", err) } st := store.New(pool) + sessions := auth.NewSessions(st, sessionTTL) reg := registry.New() reg.Register(selectel.New()) svc := service.New(st, st, reg, cipher) - a := &api.API{Svc: svc, Store: st, Cipher: cipher, Reg: reg} + a := &api.API{Svc: svc, Store: st, Cipher: cipher, Reg: reg, Auth: st, Sessions: sessions} apiRouter := api.NewRouter(a) webHandler, err := web.Handler() diff --git a/internal/api/api.go b/internal/api/api.go index 9de9798..16710cd 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -61,6 +61,7 @@ type ProviderRegistry interface { type AuthStore interface { RegisterUser(ctx context.Context, email, passwordHash string) (store.User, store.Project, error) GetUserByEmail(ctx context.Context, email string) (store.User, error) + GetUserByID(ctx context.Context, userID uuid.UUID) (store.User, error) GetUserProject(ctx context.Context, userID uuid.UUID) (store.Project, error) } diff --git a/internal/api/auth_handlers.go b/internal/api/auth_handlers.go index 5ec3c04..76d3c38 100644 --- a/internal/api/auth_handlers.go +++ b/internal/api/auth_handlers.go @@ -2,17 +2,43 @@ package api import ( "context" + "errors" "log" "net/http" + "strings" "time" "github.com/google/uuid" "github.com/vasyakrg/dns-autoresolver/internal/auth" + "github.com/vasyakrg/dns-autoresolver/internal/store" ) const sessionCookieName = "session" +// dummyPasswordHash is a valid-format argon2 hash with no real matching +// password. handleLogin runs VerifyPassword against it whenever the email +// lookup fails, so a login attempt for an unregistered email takes the same +// wall-clock time as one for a registered email with a wrong password — +// otherwise the timing difference would let an attacker enumerate which +// emails are registered. +var dummyPasswordHash string + +func init() { + h, err := auth.HashPassword("dns-autoresolver-timing-guard-dummy") + if err != nil { + panic("api: failed to initialize dummy password hash: " + err.Error()) + } + dummyPasswordHash = h +} + +// normalizeEmail trims surrounding whitespace and lowercases the email so +// storage and lookup are always consistent regardless of how the client +// cased or padded the input. +func normalizeEmail(email string) string { + return strings.ToLower(strings.TrimSpace(email)) +} + // ctxKeyUserID is a private context key carrying the authenticated user's ID. // Task 4's RequireAuth middleware sets it after validating the session // cookie; handleMe reads it back. @@ -45,7 +71,8 @@ func (a *API) handleRegister(w http.ResponseWriter, r *http.Request) { if !decodeBody(w, r, &req) { return } - if req.Email == "" || req.Password == "" { + email := normalizeEmail(req.Email) + if email == "" || req.Password == "" { writeErr(w, http.StatusBadRequest, "email and password are required") return } @@ -57,8 +84,12 @@ func (a *API) handleRegister(w http.ResponseWriter, r *http.Request) { return } - u, p, err := a.Auth.RegisterUser(r.Context(), req.Email, hash) + u, p, err := a.Auth.RegisterUser(r.Context(), email, hash) if err != nil { + if errors.Is(err, store.ErrEmailTaken) { + writeErr(w, http.StatusConflict, "email already registered") + return + } log.Printf("api: register user failed: %v", err) writeErr(w, http.StatusInternalServerError, "internal error") return @@ -87,9 +118,14 @@ func (a *API) handleLogin(w http.ResponseWriter, r *http.Request) { if !decodeBody(w, r, &req) { return } + email := normalizeEmail(req.Email) - u, err := a.Auth.GetUserByEmail(r.Context(), req.Email) + u, err := a.Auth.GetUserByEmail(r.Context(), email) if err != nil { + // No such user: still spend the argon2 verification cost against a + // fixed dummy hash (see dummyPasswordHash) so this path isn't + // distinguishable by timing from a wrong-password rejection below. + _, _ = auth.VerifyPassword(dummyPasswordHash, req.Password) invalidCredentials(w) return } @@ -131,8 +167,7 @@ func (a *API) handleLogout(w http.ResponseWriter, r *http.Request) { // handleMe returns the authenticated caller's identity + default project. // The user ID comes from the request context, set by Task 4's RequireAuth // middleware after validating the session cookie (tests set it directly via -// context.WithValue in the interim). AuthStore has no GetUserByID — the -// email field is intentionally left empty here; see task-3-report.md. +// context.WithValue in the interim). func (a *API) handleMe(w http.ResponseWriter, r *http.Request) { userID, ok := userIDFromContext(r.Context()) if !ok { @@ -140,6 +175,13 @@ func (a *API) handleMe(w http.ResponseWriter, r *http.Request) { return } + u, err := a.Auth.GetUserByID(r.Context(), userID) + if err != nil { + log.Printf("api: get user by id failed: %v", err) + writeErr(w, http.StatusInternalServerError, "internal error") + return + } + p, err := a.Auth.GetUserProject(r.Context(), userID) if err != nil { log.Printf("api: get user project failed: %v", err) @@ -147,8 +189,5 @@ func (a *API) handleMe(w http.ResponseWriter, r *http.Request) { return } - writeJSON(w, http.StatusOK, authResponse{ - User: userResponse{ID: userID.String()}, - Project: projectResponse{ID: p.ID.String(), Name: p.Name}, - }) + writeJSON(w, http.StatusOK, toAuthResponse(u, p)) } diff --git a/internal/api/auth_test.go b/internal/api/auth_test.go index 13ecbb9..58f9402 100644 --- a/internal/api/auth_test.go +++ b/internal/api/auth_test.go @@ -20,6 +20,7 @@ import ( type mockAuthStore struct { registerUserFn func(ctx context.Context, email, passwordHash string) (store.User, store.Project, error) getUserByEmailFn func(ctx context.Context, email string) (store.User, error) + getUserByIDFn func(ctx context.Context, userID uuid.UUID) (store.User, error) getUserProjectFn func(ctx context.Context, userID uuid.UUID) (store.Project, error) } @@ -31,6 +32,10 @@ func (m *mockAuthStore) GetUserByEmail(ctx context.Context, email string) (store return m.getUserByEmailFn(ctx, email) } +func (m *mockAuthStore) GetUserByID(ctx context.Context, userID uuid.UUID) (store.User, error) { + return m.getUserByIDFn(ctx, userID) +} + func (m *mockAuthStore) GetUserProject(ctx context.Context, userID uuid.UUID) (store.Project, error) { return m.getUserProjectFn(ctx, userID) } @@ -125,6 +130,59 @@ func TestAuthRegister_Success(t *testing.T) { } } +// TestAuthRegister_NormalizesEmail verifies the fix for the email-consistency +// gap: a padded/mixed-case email is trimmed+lowercased before it reaches the +// store, so storage and later lookups are always consistent. +func TestAuthRegister_NormalizesEmail(t *testing.T) { + a, authStore, _ := newTestAuthAPI() + userID := uuid.New() + var gotEmail string + authStore.registerUserFn = func(_ context.Context, email, passwordHash string) (store.User, store.Project, error) { + gotEmail = email + return store.User{ID: userID, Email: email}, store.Project{ID: uuid.New(), UserID: userID, Name: "default"}, nil + } + + router := NewRouter(a) + body := `{"email":" Alice@X.com ","password":"correct-horse"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/register", strings.NewReader(body)) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status %d, body %s", w.Code, w.Body.String()) + } + if gotEmail != "alice@x.com" { + t.Fatalf("expected normalized email passed to RegisterUser, got %q", gotEmail) + } +} + +// TestAuthRegister_DuplicateEmailReturns409 verifies the fix for the +// duplicate-registration gap: RegisterUser reporting store.ErrEmailTaken +// must surface as 409, not a generic 500. +func TestAuthRegister_DuplicateEmailReturns409(t *testing.T) { + a, authStore, _ := newTestAuthAPI() + authStore.registerUserFn = func(context.Context, string, string) (store.User, store.Project, error) { + return store.User{}, store.Project{}, store.ErrEmailTaken + } + + router := NewRouter(a) + body := `{"email":"dup@example.com","password":"correct-horse"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/register", strings.NewReader(body)) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusConflict { + t.Fatalf("status %d, body %s", w.Code, w.Body.String()) + } + var got map[string]string + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if got["error"] != "email already registered" { + t.Fatalf(`expected error "email already registered", got %q`, got["error"]) + } +} + // --- login --- func TestAuthLogin_CorrectPassword(t *testing.T) { @@ -159,6 +217,40 @@ func TestAuthLogin_CorrectPassword(t *testing.T) { } } +// TestAuthLogin_NormalizesEmail verifies that a login for a padded/mixed-case +// email reaches GetUserByEmail already trimmed+lowercased — the same +// normalization applied on register, so "Alice@X.com" at registration and +// "alice@x.com" at login resolve to the same account. +func TestAuthLogin_NormalizesEmail(t *testing.T) { + a, authStore, _ := newTestAuthAPI() + hash, err := auth.HashPassword("correct-horse") + if err != nil { + t.Fatal(err) + } + userID := uuid.New() + var gotEmail string + authStore.getUserByEmailFn = func(_ context.Context, email string) (store.User, error) { + gotEmail = email + return store.User{ID: userID, Email: email, PasswordHash: hash}, nil + } + authStore.getUserProjectFn = func(_ context.Context, uid uuid.UUID) (store.Project, error) { + return store.Project{ID: uuid.New(), UserID: uid, Name: "default"}, nil + } + + router := NewRouter(a) + body := `{"email":" Alice@X.com ","password":"correct-horse"}` + req := httptest.NewRequest(http.MethodPost, "/api/v1/auth/login", strings.NewReader(body)) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status %d, body %s", w.Code, w.Body.String()) + } + if gotEmail != "alice@x.com" { + t.Fatalf("expected normalized email passed to GetUserByEmail, got %q", gotEmail) + } +} + func TestAuthLogin_WrongPassword(t *testing.T) { a, authStore, _ := newTestAuthAPI() hash, err := auth.HashPassword("correct-horse") @@ -240,6 +332,46 @@ func TestAuthLogout_ClearsSessionAndDestroys(t *testing.T) { } } +// --- me --- + +// TestAuthMe_ReturnsRealEmail verifies the fix for the /me gap: the handler +// now resolves the authenticated user via GetUserByID and returns their real +// email, instead of leaving it blank. +func TestAuthMe_ReturnsRealEmail(t *testing.T) { + a, authStore, _ := newTestAuthAPI() + userID := uuid.New() + projectID := uuid.New() + authStore.getUserByIDFn = func(_ context.Context, id uuid.UUID) (store.User, error) { + if id != userID { + t.Fatalf("unexpected user id: %s", id) + } + return store.User{ID: userID, Email: "me@example.com"}, nil + } + authStore.getUserProjectFn = func(_ context.Context, uid uuid.UUID) (store.Project, error) { + return store.Project{ID: projectID, UserID: uid, Name: "default"}, nil + } + + router := NewRouter(a) + req := httptest.NewRequest(http.MethodGet, "/api/v1/auth/me", nil) + req = req.WithContext(context.WithValue(req.Context(), ctxKeyUserID{}, userID)) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("status %d, body %s", w.Code, w.Body.String()) + } + var got authResponse + if err := json.Unmarshal(w.Body.Bytes(), &got); err != nil { + t.Fatal(err) + } + if got.User.ID != userID.String() || got.User.Email != "me@example.com" { + t.Fatalf("unexpected user in /me response: %+v", got.User) + } + if got.Project.ID != projectID.String() { + t.Fatalf("unexpected project in /me response: %+v", got.Project) + } +} + // errNoRowsForTest stands in for a "not found" error a real store would // return (e.g. pgx.ErrNoRows) — handlers must not distinguish it from any // other GetUserByEmail failure in the response they send. diff --git a/internal/store/auth_test.go b/internal/store/auth_test.go index 2bedd35..21092df 100644 --- a/internal/store/auth_test.go +++ b/internal/store/auth_test.go @@ -1,6 +1,7 @@ package store import ( + "errors" "testing" "time" @@ -57,6 +58,41 @@ func TestGetUserByEmail_FindsRegisteredUser(t *testing.T) { } } +// TestRegisterUser_DuplicateEmailReturnsErrEmailTaken verifies the fix for +// the duplicate-registration gap: a second RegisterUser call for an +// already-taken email must fail with the ErrEmailTaken sentinel (mapped from +// the UNIQUE constraint violation on users.email), not a generic pgx error. +func TestRegisterUser_DuplicateEmailReturnsErrEmailTaken(t *testing.T) { + s, ctx := newStore(t) + + if _, _, err := s.RegisterUser(ctx, "dup@example.com", "argon2-hash"); err != nil { + t.Fatal(err) + } + + if _, _, err := s.RegisterUser(ctx, "dup@example.com", "argon2-hash"); !errors.Is(err, ErrEmailTaken) { + t.Fatalf("expected ErrEmailTaken, got %v", err) + } +} + +// TestGetUserByID_ReturnsUser verifies the fix for the /me gap: GetUserByID +// returns the same user created by RegisterUser, including their real email. +func TestGetUserByID_ReturnsUser(t *testing.T) { + s, ctx := newStore(t) + + u, _, err := s.RegisterUser(ctx, "gina@example.com", "argon2-hash") + if err != nil { + t.Fatal(err) + } + + got, err := s.GetUserByID(ctx, u.ID) + if err != nil { + t.Fatal(err) + } + if got.ID != u.ID || got.Email != "gina@example.com" { + t.Fatalf("unexpected user: %+v", got) + } +} + // TestSessionLifecycle_CreateGetDelete verifies CreateSession + GetSessionUser // round-trips to the owning user ID, an expired session is excluded from // GetSessionUser, and DeleteSession removes the session. diff --git a/internal/store/db/users.sql.go b/internal/store/db/users.sql.go index c76c410..8fef920 100644 --- a/internal/store/db/users.sql.go +++ b/internal/store/db/users.sql.go @@ -48,3 +48,19 @@ func (q *Queries) GetUserByEmail(ctx context.Context, email string) (User, error ) return i, err } + +const getUserByID = `-- name: GetUserByID :one +SELECT id, email, created_at, password_hash FROM users WHERE id = $1 +` + +func (q *Queries) GetUserByID(ctx context.Context, id uuid.UUID) (User, error) { + row := q.db.QueryRow(ctx, getUserByID, id) + var i User + err := row.Scan( + &i.ID, + &i.Email, + &i.CreatedAt, + &i.PasswordHash, + ) + return i, err +} diff --git a/internal/store/queries/users.sql b/internal/store/queries/users.sql index d68dc45..31d7fde 100644 --- a/internal/store/queries/users.sql +++ b/internal/store/queries/users.sql @@ -3,3 +3,6 @@ INSERT INTO users (id, email, password_hash) VALUES ($1, $2, $3) RETURNING *; -- name: GetUserByEmail :one SELECT * FROM users WHERE email = $1; + +-- name: GetUserByID :one +SELECT * FROM users WHERE id = $1; diff --git a/internal/store/tenant.go b/internal/store/tenant.go index 37961a0..fccc469 100644 --- a/internal/store/tenant.go +++ b/internal/store/tenant.go @@ -7,6 +7,7 @@ import ( "github.com/google/uuid" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgtype" "github.com/vasyakrg/dns-autoresolver/internal/provider" @@ -14,6 +15,11 @@ import ( "github.com/vasyakrg/dns-autoresolver/internal/store/dto" ) +// ErrEmailTaken is returned by RegisterUser when the email is already +// registered — a UNIQUE constraint violation (pgerrcode 23505) on +// users.email. +var ErrEmailTaken = errors.New("store: email already registered") + // Account/Template/Domain are provider-neutral domain structs returned by the // thin wrappers below, so callers (internal/api) never need to import // internal/store/db directly. @@ -279,6 +285,17 @@ func (s *Store) GetUserByEmail(ctx context.Context, email string) (User, error) return toUser(u), nil } +// GetUserByID looks up a user by primary key — used by handleMe (Task 3 +// hardening) to return the authenticated caller's real email instead of +// leaving it blank. +func (s *Store) GetUserByID(ctx context.Context, id uuid.UUID) (User, error) { + u, err := s.q.GetUserByID(ctx, id) + if err != nil { + return User{}, err + } + return toUser(u), nil +} + func (s *Store) CreateProjectForUser(ctx context.Context, userID uuid.UUID, name string) (Project, error) { p, err := s.q.CreateProject(ctx, db.CreateProjectParams{ID: uuid.New(), UserID: userID, Name: name}) if err != nil { @@ -337,6 +354,10 @@ func (s *Store) RegisterUser(ctx context.Context, email, passwordHash string) (U uid := uuid.New() dbu, err := q.CreateUser(ctx, db.CreateUserParams{ID: uid, Email: email, PasswordHash: ptr(passwordHash)}) if err != nil { + var pgErr *pgconn.PgError + if errors.As(err, &pgErr) && pgErr.Code == "23505" { + return User{}, Project{}, ErrEmailTaken + } return User{}, Project{}, err } dbp, err := q.CreateProject(ctx, db.CreateProjectParams{ID: uuid.New(), UserID: uid, Name: "default"})