From cf054959bd39ef9d0b67cc00ea0044dcc8bb5aad Mon Sep 17 00:00:00 2001 From: Trey T Date: Sat, 6 Jun 2026 10:49:37 -0500 Subject: [PATCH] Auth: require email-verified by default for all app-data routes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously only 2 share-code routes required a verified email; every other authenticated route (residences, tasks, contractors, documents, notifications, subscription, users, uploads, media — ~70 routes) accepted an authenticated but UNVERIFIED user. This inverts the default to verified-by-default. - router.go: add a `verified` sub-group that applies RequireVerified() ONCE at the group level, and move all app-data route setups under it. Verification is now the default; new routes are gated automatically. The authenticated-only allow-list is just the sign-up surface (/auth/me, /auth/profile, /auth/account). Public stays: register, health, webhooks, lookups. - kratos_auth.go: fix a latent bug the gating exposed — the Redis session cache stored the verified flag for 24h, so a user who verified their email mid-session was still seen as unverified until the TTL expired (sign up -> verify -> create residence would 403). Now only a cached verified=true is trusted (verification is sticky); a cached verified=false re-resolves the live status from Kratos. - auth_safety_test.go: add RequireVerified unit tests (verified passes, unverified -> 403, no-user -> 401). Validated: API gating test (unverified->403, verified->200) + full iOS XCUITest suite green (211 passed) including the onboarding verify->use-immediately flow. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/middleware/auth_safety_test.go | 74 +++++++++++++++++++++++++ internal/middleware/kratos_auth.go | 20 ++++--- internal/router/router.go | 59 ++++++++++++-------- 3 files changed, 124 insertions(+), 29 deletions(-) diff --git a/internal/middleware/auth_safety_test.go b/internal/middleware/auth_safety_test.go index a6dde67..c9bb23f 100644 --- a/internal/middleware/auth_safety_test.go +++ b/internal/middleware/auth_safety_test.go @@ -9,6 +9,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/treytartt/honeydue-api/internal/apperrors" "github.com/treytartt/honeydue-api/internal/config" "github.com/treytartt/honeydue-api/internal/models" ) @@ -117,3 +118,76 @@ func TestAdminAuth_QueryParamToken_Rejected(t *testing.T) { assert.Equal(t, http.StatusUnauthorized, rec.Code, "query param token must be rejected") assert.Contains(t, rec.Body.String(), "Authorization required") } + +// requireVerifiedContext builds an Echo context primed as the Authenticate +// middleware would leave it: an auth_user and the verified flag. +func requireVerifiedContext(user *models.User, verified bool) (echo.Context, *httptest.ResponseRecorder) { + e := echo.New() + req := httptest.NewRequest(http.MethodGet, "/api/residences/", nil) + rec := httptest.NewRecorder() + c := e.NewContext(req, rec) + if user != nil { + c.Set(AuthUserKey, user) + } + c.Set(AuthVerifiedKey, verified) + return c, rec +} + +// TestRequireVerified_VerifiedUser_Passes confirms a verified user reaches the +// wrapped handler. This is the default tier for all app-data routes now that +// RequireVerified is applied at the `verified` group level in the router. +func TestRequireVerified_VerifiedUser_Passes(t *testing.T) { + m := &KratosAuth{} + c, _ := requireVerifiedContext(&models.User{Username: "v"}, true) + + reached := false + handler := m.RequireVerified()(func(c echo.Context) error { + reached = true + return c.NoContent(http.StatusOK) + }) + + err := handler(c) + assert.NoError(t, err) + assert.True(t, reached, "verified user should reach the handler") +} + +// TestRequireVerified_UnverifiedUser_403 is the core gating assertion for the +// new policy: an authenticated-but-unverified user is rejected with 403 on a +// data route, NOT allowed through. +func TestRequireVerified_UnverifiedUser_403(t *testing.T) { + m := &KratosAuth{} + c, _ := requireVerifiedContext(&models.User{Username: "u"}, false) + + reached := false + handler := m.RequireVerified()(func(c echo.Context) error { + reached = true + return c.NoContent(http.StatusOK) + }) + + err := handler(c) + require.Error(t, err) + assert.False(t, reached, "unverified user must NOT reach the handler") + + var appErr *apperrors.AppError + require.ErrorAs(t, err, &appErr) + assert.Equal(t, http.StatusForbidden, appErr.Code) +} + +// TestRequireVerified_NoUser_401 confirms RequireVerified rejects an +// unauthenticated request with 401 (defense-in-depth even though Authenticate +// runs first in the router). +func TestRequireVerified_NoUser_401(t *testing.T) { + m := &KratosAuth{} + c, _ := requireVerifiedContext(nil, false) + + handler := m.RequireVerified()(func(c echo.Context) error { + return c.NoContent(http.StatusOK) + }) + + err := handler(c) + require.Error(t, err) + + var appErr *apperrors.AppError + require.ErrorAs(t, err, &appErr) + assert.Equal(t, http.StatusUnauthorized, appErr.Code) +} diff --git a/internal/middleware/kratos_auth.go b/internal/middleware/kratos_auth.go index 27f3509..f021590 100644 --- a/internal/middleware/kratos_auth.go +++ b/internal/middleware/kratos_auth.go @@ -131,14 +131,20 @@ func (m *KratosAuth) resolve(c echo.Context) (*models.User, bool, string, error) cacheKey := kratosSessionPrefix + hashCredential(cred) if m.cache != nil { if v, err := m.cache.GetString(ctx, cacheKey); err == nil && v != "" { - if user, verified, ok := m.userFromCacheValue(ctx, v); ok { - // Sliding-window refresh: extend the TTL on every successful - // hit so active users don't get bounced when their original - // cache entry would have otherwise expired. Best-effort — - // failure to refresh just means the entry expires on the - // original schedule. + // Only a cached `verified=true` is authoritative — email verification + // is sticky (it never reverts), so we can safely short-circuit. + // A cached `verified=false` is deliberately NOT trusted: the user may + // have verified their email since this entry was written, and a stale + // false would lock a just-verified user out of every verified-gated + // route until the 24h TTL expired (e.g. sign up -> verify -> create a + // residence immediately). On a cached false we fall through and + // re-resolve the live status from Kratos /whoami below. + if user, verified, ok := m.userFromCacheValue(ctx, v); ok && verified { + // Sliding-window refresh: extend the TTL on every successful hit + // so active (verified) users aren't bounced when their entry + // would otherwise expire. Best-effort. _ = m.cache.SetString(ctx, cacheKey, v, kratosSessionCacheTTL) - return user, verified, cred, nil + return user, true, cred, nil } } } diff --git a/internal/router/router.go b/internal/router/router.go index 235b97e..beb86e5 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -342,29 +342,44 @@ func SetupRouter(deps *Dependencies) *echo.Echo { // Subscription webhook routes (no auth - called by Apple/Google servers) setupWebhookRoutes(api, subscriptionWebhookHandler) - // Protected routes (auth required) + // Authenticated routes (valid session required). This level is the + // sign-up / shell allow-list: an authenticated-but-UNVERIFIED user may + // call these (read their own user + verification status, complete their + // profile during sign-up). EVERYTHING else requires a verified email + // (see the `verified` sub-group below). protected := api.Group("") protected.Use(authMiddleware.Authenticate()) protected.Use(custommiddleware.TimezoneMiddleware()) { + // Allow-list — authenticated, may be unverified. setupProtectedAuthRoutes(protected, authHandler) - setupResidenceRoutes(protected, residenceHandler, authMiddleware.RequireVerified()) - setupTaskRoutes(protected, taskHandler) - setupSuggestionRoutes(protected, suggestionHandler) - setupContractorRoutes(protected, contractorHandler) - setupDocumentRoutes(protected, documentHandler) - setupNotificationRoutes(protected, notificationHandler) - setupSubscriptionRoutes(protected, subscriptionHandler) - setupUserRoutes(protected, userHandler) - // Upload routes (only if storage service is configured) - if uploadHandler != nil { - setupUploadRoutes(protected, uploadHandler) - } + // Verified routes (authenticated AND email-verified) — the DEFAULT + // for all app data and actions. RequireVerified is applied ONCE at + // the group level so verification is the default and every route + // added under here is gated automatically. (The previous per-route + // approach left ~70 routes unverified — see the LIVE auth audit.) + verified := protected.Group("") + verified.Use(authMiddleware.RequireVerified()) + { + setupResidenceRoutes(verified, residenceHandler) + setupTaskRoutes(verified, taskHandler) + setupSuggestionRoutes(verified, suggestionHandler) + setupContractorRoutes(verified, contractorHandler) + setupDocumentRoutes(verified, documentHandler) + setupNotificationRoutes(verified, notificationHandler) + setupSubscriptionRoutes(verified, subscriptionHandler) + setupUserRoutes(verified, userHandler) - // Media routes (authenticated media serving) - if mediaHandler != nil { - setupMediaRoutes(protected, mediaHandler) + // Upload routes (only if storage service is configured) + if uploadHandler != nil { + setupUploadRoutes(verified, uploadHandler) + } + + // Media routes (verified media serving) + if mediaHandler != nil { + setupMediaRoutes(verified, mediaHandler) + } } } } @@ -583,7 +598,7 @@ func setupPublicDataRoutes(api *echo.Group, residenceHandler *handlers.Residence } // setupResidenceRoutes configures residence routes -func setupResidenceRoutes(api *echo.Group, residenceHandler *handlers.ResidenceHandler, requireVerified echo.MiddlewareFunc) { +func setupResidenceRoutes(api *echo.Group, residenceHandler *handlers.ResidenceHandler) { residences := api.Group("/residences") { residences.GET("/", residenceHandler.ListResidences) @@ -598,11 +613,11 @@ func setupResidenceRoutes(api *echo.Group, residenceHandler *handlers.ResidenceH residences.DELETE("/:id/", residenceHandler.DeleteResidence) residences.GET("/:id/share-code/", residenceHandler.GetShareCode) - // Audit LIVE-L19: generating a residence share code requires a - // verified email — it blocks bad-faith unverified signups from - // minting share codes. - residences.POST("/:id/generate-share-code/", residenceHandler.GenerateShareCode, requireVerified) - residences.POST("/:id/generate-share-package/", residenceHandler.GenerateSharePackage, requireVerified) + // Verification is now enforced at the group level for ALL residence + // routes (see the `verified` group in SetupRouter) — the previous + // per-route RequireVerified on just these two is no longer needed. + residences.POST("/:id/generate-share-code/", residenceHandler.GenerateShareCode) + residences.POST("/:id/generate-share-package/", residenceHandler.GenerateSharePackage) residences.POST("/:id/generate-tasks-report/", residenceHandler.GenerateTasksReport) residences.GET("/:id/users/", residenceHandler.GetResidenceUsers) residences.DELETE("/:id/users/:user_id/", residenceHandler.RemoveResidenceUser)