From 81c4b621755235108d043f895d36b146be01a67c Mon Sep 17 00:00:00 2001 From: Florian Sylvain Date: Thu, 24 Sep 2026 16:32:40 +0200 Subject: [PATCH] fix: reading a single article follows the same rule as the list: anyone can read an online article, a verified user can also read an offline one, and an unknown or hidden id returns 404 --- api/handlers/article/articleHandler.go | 7 +- api/router.go | 2 +- features/not_found_errors.feature | 15 +- features/public_article_listing.feature | 8 +- features/public_article_read.feature | 132 ++++++++++ features/public_article_read.qa.md | 73 ++++++ features/verified_account_required.feature | 7 +- test/not_found_errors_test.go | 19 +- test/public_article_listing_test.go | 3 +- test/public_article_read_test.go | 268 ++++++++++++++++++++ test/verified_account_required_http_test.go | 4 +- 11 files changed, 510 insertions(+), 28 deletions(-) create mode 100644 features/public_article_read.feature create mode 100644 features/public_article_read.qa.md create mode 100644 test/public_article_read_test.go diff --git a/api/handlers/article/articleHandler.go b/api/handlers/article/articleHandler.go index 152a738..6d5973c 100644 --- a/api/handlers/article/articleHandler.go +++ b/api/handlers/article/articleHandler.go @@ -53,7 +53,12 @@ func (h *Handler) GetArticle(w http.ResponseWriter, r *http.Request) { return } - article, err := h.GetUseCase.GetOnlineArticle(id) + var article domain.Article + if h.callerIsVerified(r) { + article, err = h.GetUseCase.GetArticle(id) + } else { + article, err = h.GetUseCase.GetOnlineArticle(id) + } if err != nil { if errors.Is(err, domain.ErrArticleNotFound) { http.Error(w, apperr.ArticleNotFoundErrorMessage, http.StatusNotFound) diff --git a/api/router.go b/api/router.go index 77f97ba..5121c6b 100644 --- a/api/router.go +++ b/api/router.go @@ -36,11 +36,11 @@ func NewArticleRouter(container *UseCases) http.Handler { r.Group(func(r chi.Router) { r.Get("/", articleHandler.ListArticles) + r.Get("/{id}", articleHandler.GetArticle) }) r.Group(func(r chi.Router) { r.Use(jwtauth.Authenticator(container.Token)) r.Use(middleware.RequireVerifiedUser(container.GetUserUseCase)) - r.Get("/{id}", articleHandler.GetArticle) r.Post("/", articleHandler.PostArticle) r.Delete("/{id}", articleHandler.DeleteArticle) r.Patch("/{id}", articleHandler.UpdateArticleBody) diff --git a/features/not_found_errors.feature b/features/not_found_errors.feature index 7898233..7f5beb1 100644 --- a/features/not_found_errors.feature +++ b/features/not_found_errors.feature @@ -11,6 +11,10 @@ Feature: Domain not-found errors instead of raw GORM errors And ArticleRepository.Get and ArticleRepository.GetByName return article.ErrArticleNotFound, not gorm.ErrRecordNotFound, when no row matches And UserRepository.Get, UserRepository.GetByUsername and UserRepository.GetByEmail return user.ErrUserNotFound, not gorm.ErrRecordNotFound, when no row matches And GET /v1/article/{id} sits behind the jwtauth Verifier and Authenticator group, and requires a valid jwt cookie + And a later slice, features/public_article_read.feature, moves GET /v1/article/{id} into the + Verifier-only group and makes it return an offline article to a verified caller instead of + 404, mirroring the listing endpoint's visibility rule; the two scenarios below that depended + on the older, stricter rule are updated accordingly Scenario Outline: The article repository reports a missing article as a domain error Given a repository backed by an in-memory SQLite database containing an article titled "Backyard Renovation Update" with id 1 @@ -42,10 +46,10 @@ Feature: Domain not-found errors instead of raw GORM errors Then the response status is 404 And the response body is the text "Article not found" - Scenario: Fetching an offline article still answers 404 with the same message - Given a valid jwt cookie for a user with id 1 + Scenario: Fetching an offline article as an anonymous or unverified caller still answers 404 with the same message + Given no jwt cookie is sent And the article repository contains an article with id 2, is_online false - When the client sends GET /v1/article/2 with that cookie + When the client sends GET /v1/article/2 without a cookie Then the response status is 404 And the response body is the text "Article not found" @@ -56,11 +60,12 @@ Feature: Domain not-found errors instead of raw GORM errors Then the response status is 200 And the response body is the article's JSON representation - Scenario: The article route still requires authentication before existence is even checked + Scenario: An unknown article id answers 404 even without authentication Given no jwt cookie is sent And the article repository has no article with id 9999 When the client sends GET /v1/article/9999 without a cookie - Then the response status is 401 + Then the response status is 404 + And the response body is the text "Article not found" Scenario Outline: Login never reveals whether the username exists Given a user repository containing a user "alice" with password "hunter2example" diff --git a/features/public_article_listing.feature b/features/public_article_listing.feature index 86c1c4a..f8df2cf 100644 --- a/features/public_article_listing.feature +++ b/features/public_article_listing.feature @@ -14,8 +14,11 @@ Feature: Public and authenticated article listing in api/router.go along with GET /v1/article/{id}, POST /v1/article/ and DELETE /v1/article/{id}, so an anonymous request to any of them answers 401, not an online-only list And this slice moves GET /v1/article/ under jwtauth.Verifier only, so a missing or invalid jwt - cookie no longer blocks the request; GET /v1/article/{id}, POST /v1/article/ and - DELETE /v1/article/{id} keep requiring jwtauth.Verifier + Authenticator, unchanged + cookie no longer blocks the request; POST /v1/article/ and DELETE /v1/article/{id} keep + requiring jwtauth.Verifier + Authenticator, unchanged + And a later slice, features/public_article_read.feature, moves GET /v1/article/{id} into the + same Verifier-only group as GET /v1/article/, so the 401 Examples table below is updated + accordingly And Handler.ListArticles decides between ListAllArticles and ListOnlineArticles by checking the request for a valid jwt, read with jwtauth.FromContext from the token the router-level jwtauth.Verifier (header and cookie sources) placed in the request context, rather than @@ -53,7 +56,6 @@ Feature: Public and authenticated article listing Examples: | method | path | - | GET | /v1/article/1 | | POST | /v1/article/ | | DELETE | /v1/article/1 | diff --git a/features/public_article_read.feature b/features/public_article_read.feature new file mode 100644 index 0000000..718b63a --- /dev/null +++ b/features/public_article_read.feature @@ -0,0 +1,132 @@ +Feature: Single article read follows the same online/offline visibility rule as the list + + As a visitor of the public site and as an authenticated RenewCMS user + I want GET /v1/article/{id} to return an online article to anyone, and an offline article + only to a caller resolved as verified, with an unknown id and a hidden offline id + answering the exact same 404 + So that the public site can read a single published article through the API, and a verified + user can open their own unpublished draft to edit it, while a draft never leaks its existence + to anyone else + + Background: + Given internal/domain/article.Article has a json "is_online" field and the sentinel + article.ErrArticleNotFound + And GetArticleUseCase.GetArticle(id) returns the article regardless of is_online, and + GetArticleUseCase.GetOnlineArticle(id) returns article.ErrArticleNotFound when the article + exists but is offline, both already implemented and unchanged by this slice + And before this slice, GET /v1/article/{id} sits inside the jwtauth Verifier + Authenticator + + middleware.RequireVerifiedUser group in api/router.go, so a missing jwt cookie answers 401, an + authenticated-but-unverified caller answers 403, and even a verified caller always resolves + through GetOnlineArticle, so an offline article answers 404 no matter who asks + (features/not_found_errors.feature, features/verified_account_required.feature) + And this slice moves GET /v1/article/{id} out of that group and into the jwtauth.Verifier-only + group that already holds GET /v1/article/ (features/public_article_listing.feature), so a + missing or invalid jwt cookie no longer blocks the request; POST /v1/article/, + DELETE /v1/article/{id}, PATCH /v1/article/{id}, PATCH /v1/article/{id}/publish, + PATCH /v1/article/{id}/unpublish and POST /v1/article/{id}/image keep requiring + Verifier + Authenticator + RequireVerifiedUser, unchanged + And Handler.GetArticle reuses the existing Handler.callerIsVerified(r) helper already used by + Handler.ListArticles: when the request resolves to a verified account it calls + GetUseCase.GetArticle(id); otherwise (no jwt, malformed jwt, unresolvable user_id, a deleted + account, or an account that exists but is not yet verified) it calls + GetUseCase.GetOnlineArticle(id), exactly the same fallback rule + features/verified_account_required.feature already documents for the listing endpoint + And an id that does not parse as a uint32 still answers 400 with apperr.IdUint32ErrorMessage + before any lookup happens, for every caller, unchanged + And a domain article.ErrArticleNotFound, whether the id is truly unknown or is an offline + article hidden from this caller, still answers 404 with apperr.ArticleNotFoundErrorMessage + ("Article not found"), the identical body either way, so no response distinguishes the two + And a repository failure other than article.ErrArticleNotFound still answers 500 with the text + "Internal server error", regardless of caller state, unchanged + And this slice amends features/verified_account_required.feature: the Examples table of + "An authenticated but unverified user is refused on every protected article route" and of "A + request with no jwt cookie is still refused with 401, never 403" both drop their + "GET /v1/article/1" row, since that route no longer applies RequireVerifiedUser or requires a + cookie at all + And this slice amends features/not_found_errors.feature: "Fetching an offline article still + answers 404 with the same message" now holds only for an anonymous or unverified caller, not + a verified one, and "The article route still requires authentication before existence is even + checked" is replaced by this feature's public-visibility scenarios below + + Scenario: An anonymous visitor can read an online article + Given the article repository contains an online article titled "Backyard Renovation Update" with id 1 + When the client sends GET /v1/article/1 with no jwt cookie + Then the response status is 200 + And the response body is the article's JSON representation + + Scenario: An anonymous visitor is told an offline article does not exist + Given the article repository contains an offline article titled "Draft: Kitchen Remodel" with id 2 + When the client sends GET /v1/article/2 with no jwt cookie + Then the response status is 404 + And the response body is the text "Article not found" + + Scenario: An anonymous visitor is told an unknown id does not exist + Given the article repository has no article with id 9999 + When the client sends GET /v1/article/9999 with no jwt cookie + Then the response status is 404 + And the response body is the text "Article not found" + + Scenario: A verified user can open their own offline draft + Given a user repository containing user id 1, is_verified true + And a valid jwt cookie for user id 1 + And the article repository contains an offline article titled "Draft: Kitchen Remodel" with id 2 + When the client sends GET /v1/article/2 with that cookie + Then the response status is 200 + And the response body is the article's JSON representation + + Scenario: A verified user still reads an online article normally + Given a user repository containing user id 1, is_verified true + And a valid jwt cookie for user id 1 + And the article repository contains an online article titled "Backyard Renovation Update" with id 1 + When the client sends GET /v1/article/1 with that cookie + Then the response status is 200 + And the response body is the article's JSON representation + + Scenario: A verified user requesting an unknown id gets the same not-found response + Given a user repository containing user id 1, is_verified true + And a valid jwt cookie for user id 1 + And the article repository has no article with id 9999 + When the client sends GET /v1/article/9999 with that cookie + Then the response status is 404 + And the response body is the text "Article not found" + + Scenario: An authenticated but unverified user is told an offline article does not exist, not refused with 403 + Given a user repository containing user id 1, is_verified false + And a valid jwt cookie for user id 1 + And the article repository contains an offline article titled "Draft: Kitchen Remodel" with id 2 + When the client sends GET /v1/article/2 with that cookie + Then the response status is 404 + And the response body is the text "Article not found" + + Scenario: A malformed or expired jwt cookie falls back to the public online-only visibility + Given a jwt cookie whose value is not a token issued by JWT_SECRET + And the article repository contains an online article with id 1 and an offline article with id 2 + When the client sends GET /v1/article/1 with that cookie + Then the response status is 200 + When the client sends GET /v1/article/2 with that cookie + Then the response status is 404 + And the response body is the text "Article not found" + + Scenario: An id that isn't a valid uint32 still answers 400, regardless of caller + When the client sends GET /v1/article/not-a-number with no jwt cookie + Then the response status is 400 + And the response body is the text apperr.IdUint32ErrorMessage + + Scenario Outline: The rest of the article routes still require a verified account + Given no jwt cookie is sent + When the client sends + Then the response status is 401 + + Examples: + | method | path | + | POST | /v1/article/ | + | DELETE | /v1/article/1 | + | PATCH | /v1/article/1 | + | PATCH | /v1/article/1/publish | + | PATCH | /v1/article/1/unpublish | + + Scenario: A repository failure still answers 500 regardless of caller state + Given the article repository fails on its next call to Get + When the client sends GET /v1/article/1 with no jwt cookie + Then the response status is 500 + And the response body is the text "Internal server error" diff --git a/features/public_article_read.qa.md b/features/public_article_read.qa.md new file mode 100644 index 0000000..bacb0c1 --- /dev/null +++ b/features/public_article_read.qa.md @@ -0,0 +1,73 @@ +# QA procedure: public_article_read + +Package: `test` (black-box, extending `article_test.go`, `verified_account_required_http_test.go` +and `not_found_errors_test.go`). + +## Setup shared by every case +1. Call `validator.InitValidator()` once. +2. Reuse `mockArticleRepo` (`test/article_test.go`) and `mockUserRepo` (`test/verified_account_required_http_test.go`) + as-is; no fake changes expected. +3. Wire the real use cases over the fakes: `useCases.NewGetArticleUseCase`, `useCases.NewGetUserUseCase`, + plus `NewCreateArticleUseCase`/`NewListArticlesUseCase`/`NewDeleteArticleUseCase`/ + `NewUpdateArticleUseCase` for the regression routes. Build + `article.NewHandler(create, get, list, delete, update, getUser)`. +4. Build a test `jwtauth.New("HS256", []byte("test-secret"), nil)`. +5. Mount routes on a fresh `chi.NewRouter()` mirroring the amended `api/router.go` shape: + - `r.Route("/v1", func(r chi.Router) { r.Route("/article", func(r chi.Router) { ... }) })` + - `r.Use(jwtauth.Verifier(token))` on the whole `/article` group + - `r.Get("/", articleHandler.ListArticles)` and `r.Get("/{id}", articleHandler.GetArticle)` + directly under that Verifier-only group + - a nested `r.Group` adding `jwtauth.Authenticator(token)` then + `middleware.RequireVerifiedUser(getUserUseCase)`, containing `r.Post("/", ...)`, + `r.Delete("/{id}", ...)`, `r.Patch("/{id}", ...)`, `r.Patch("/{id}/publish", ...)`, + `r.Patch("/{id}/unpublish", ...)` + No live server. +6. Mint a valid jwt cookie for user id 1 via `token.Encode(map[string]any{"user_id": uint32(1)})`. +7. Seed the article repo directly, e.g. `repo.articles[1] = article.Article{ID: 1, Title: + "Backyard Renovation Update", IsOnline: true}` and `repo.articles[2] = article.Article{ID: 2, + Title: "Draft: Kitchen Remodel", IsOnline: false}`. +8. Seed the user repo with `repo.users[1] = user.User{ID: 1, IsVerified: }` as each case needs. + +## HTTP checks +9. `GET /v1/article/1` (online), no cookie -> 200, decode body, assert `id == 1`. +10. `GET /v1/article/2` (offline), no cookie -> 404, body `"Article not found\n"`. +11. `GET /v1/article/9999` (no article with that id), no cookie -> 404, body `"Article not found\n"`. +12. `GET /v1/article/2`, cookie for user 1 with `IsVerified: true` -> 200, decode body, assert `id == 2`. +13. `GET /v1/article/1`, cookie for user 1 with `IsVerified: true` -> 200, `id == 1`. +14. `GET /v1/article/9999`, cookie for user 1 with `IsVerified: true` -> 404, body `"Article not found\n"`. +15. `GET /v1/article/2`, cookie for user 1 with `IsVerified: false` -> 404, body `"Article not found\n"` + (never 403). +16. `GET /v1/article/1` and `GET /v1/article/2`, cookie named `jwt` whose value is an arbitrary + non-token string (e.g. `"not-a-real-token"`) -> 200 for id 1, 404 for id 2 (same as step 9/10, + proving the invalid cookie falls back to public visibility). +17. `GET /v1/article/not-a-number`, no cookie -> 400, body `apperr.IdUint32ErrorMessage + "\n"`. +18. Regression, no cookie: `POST /v1/article/` -> 401; `DELETE /v1/article/1` -> 401; + `PATCH /v1/article/1` -> 401; `PATCH /v1/article/1/publish` -> 401; + `PATCH /v1/article/1/unpublish` -> 401. +19. Configure the fake repo's `failSwitch` so the next `Get` call returns `errRepositoryFailure` + (`failNext("Get")`). `GET /v1/article/1` with no cookie -> 500, body `"Internal server error\n"`. + +## Required amendments to existing tests/specs (part of this slice, not optional cleanup) +20. `test/verified_account_required_http_test.go`: move `r.Get("/{id}", articleHandler.GetArticle)` + in `newVerifiedAccountRequiredRouter` out of the `Authenticator` + `RequireVerifiedUser` group + into the outer Verifier-only group (mirroring step 5 above); remove the + `{http.MethodGet, "/v1/article/1", ""}` row from `protectedRouteCases` and the + `{http.MethodGet, "/v1/article/1"}` row from `noCookieCases` in `TestVerifiedAccountRequiredHttp`, + since that route no longer produces 403 or 401. +21. `test/not_found_errors_test.go`: move `r.Get("/{id}", articleHandler.GetArticle)` in + `newNotFoundErrorsRouter` the same way; change the `"Fetching an offline article still answers + 404 with the same message"` subtest to use an unverified or anonymous caller (it currently uses + `newVerifiedUserRepo()`, which will now 200 instead of 404); replace `"The article route still + requires authentication before existence is even checked"` with an assertion that an anonymous + request for an unknown id now answers 404 (covered by step 11 above, so this subtest can instead + be deleted in favor of the new test file, or repurposed to check `GET /v1/article/1` with no + cookie still answers 200 as a cross-file regression note). + `features/verified_account_required.feature` and `features/not_found_errors.feature` get the + corresponding Examples-table/scenario edits described in this feature's Background. + +## Manual verification (not exercised by the Go test) +- SPA-visible behaviour: confirm the public front-office single-article view (unauthenticated) can + open a published article and 404s on a draft URL, and the authenticated back-office view can open + its own unpublished draft for editing, against the real embedded frontend and a real SQLite file. + Not exercised by `task test`; requires `task dev` or a built `task build` binary. +- No SMTP involvement. diff --git a/features/verified_account_required.feature b/features/verified_account_required.feature index 0a71e06..53bd91f 100644 --- a/features/verified_account_required.feature +++ b/features/verified_account_required.feature @@ -54,6 +54,11 @@ Feature: Protected article routes and the offline-inclusive listing require a ve for the purpose of choosing which list to show, so the listing still answers 200 with the online-only array rather than failing the request; only a failure inside ListOnlineArticles or ListAllArticles itself still answers 500, unchanged from features/public_article_listing.feature + And a later slice, features/public_article_read.feature, moves GET /v1/article/{id} out of the + Verifier + Authenticator + RequireVerifiedUser group entirely and into the Verifier-only group + next to GET /v1/article/, so that route no longer answers 401 for a missing cookie or 403 for + an unverified caller; the "GET /v1/article/1" row is therefore removed from both Examples + tables below Scenario Outline: An authenticated but unverified user is refused on every protected article route Given a user repository containing user id 1, is_verified false @@ -65,7 +70,6 @@ Feature: Protected article routes and the offline-inclusive listing require a ve Examples: | method | path | - | GET | /v1/article/1 | | POST | /v1/article/ | | DELETE | /v1/article/1 | | PATCH | /v1/article/1 | @@ -113,7 +117,6 @@ Feature: Protected article routes and the offline-inclusive listing require a ve Examples: | method | path | - | GET | /v1/article/1 | | POST | /v1/article/ | Scenario: A repository failure while resolving the caller's verification status answers 500 diff --git a/test/not_found_errors_test.go b/test/not_found_errors_test.go index 09c7ed3..5829045 100644 --- a/test/not_found_errors_test.go +++ b/test/not_found_errors_test.go @@ -3,7 +3,6 @@ package test import ( "RenewCMS/api/handlers/article" "RenewCMS/api/handlers/auth" - "RenewCMS/api/middleware" "RenewCMS/api/pkg/apperr" validator "RenewCMS/api/pkg/validator" domainArticle "RenewCMS/internal/domain/article" @@ -150,13 +149,9 @@ func newNotFoundErrorsRouter(articleRepo *mockArticleRepo, userRepo *mockUserRep r := chi.NewRouter() r.Route("/v1", func(r chi.Router) { - r.Group(func(r chi.Router) { + r.Route("/article", func(r chi.Router) { r.Use(jwtauth.Verifier(token)) - r.Use(jwtauth.Authenticator(token)) - r.Use(middleware.RequireVerifiedUser(getUserUseCase)) - r.Route("/article", func(r chi.Router) { - r.Get("/{id}", articleHandler.GetArticle) - }) + r.Get("/{id}", articleHandler.GetArticle) }) r.Route("/auth", func(r chi.Router) { r.Post("/login", authHandler.Login) @@ -203,7 +198,6 @@ func TestNotFoundErrorsHttp(t *testing.T) { t.Run("Fetching an offline article still answers 404 with the same message", func(t *testing.T) { router := newNotFoundErrorsRouter(newArticleRepo(), newVerifiedUserRepo(), token) req := httptest.NewRequest(http.MethodGet, "/v1/article/2", nil) - setJwtCookie(t, req, token, 1) rec := httptest.NewRecorder() router.ServeHTTP(rec, req) @@ -234,14 +228,17 @@ func TestNotFoundErrorsHttp(t *testing.T) { } }) - t.Run("The article route still requires authentication before existence is even checked", func(t *testing.T) { + t.Run("An unknown article id answers 404 even without authentication", func(t *testing.T) { router := newNotFoundErrorsRouter(newArticleRepo(), newMockUserRepo(), token) req := httptest.NewRequest(http.MethodGet, "/v1/article/9999", nil) rec := httptest.NewRecorder() router.ServeHTTP(rec, req) - if rec.Code != http.StatusUnauthorized { - t.Fatalf("expected status 401, got %d", rec.Code) + if rec.Code != http.StatusNotFound { + t.Fatalf("expected status 404, got %d", rec.Code) + } + if rec.Body.String() != apperr.ArticleNotFoundErrorMessage+"\n" { + t.Errorf("unexpected body: %q", rec.Body.String()) } }) diff --git a/test/public_article_listing_test.go b/test/public_article_listing_test.go index 67a6db1..e521302 100644 --- a/test/public_article_listing_test.go +++ b/test/public_article_listing_test.go @@ -56,11 +56,11 @@ func newPublicArticleListingRouter(articleRepo *mockArticleRepo, token *jwtauth. r.Use(jwtauth.Verifier(token)) r.Group(func(r chi.Router) { r.Get("/", articleHandler.ListArticles) + r.Get("/{id}", articleHandler.GetArticle) }) r.Group(func(r chi.Router) { r.Use(jwtauth.Authenticator(token)) r.Use(middleware.RequireVerifiedUser(getUserUseCase)) - r.Get("/{id}", articleHandler.GetArticle) r.Post("/", articleHandler.PostArticle) r.Delete("/{id}", articleHandler.DeleteArticle) }) @@ -155,7 +155,6 @@ func TestPublicArticleListingHttp(t *testing.T) { path string body []byte }{ - {"GET /v1/article/1 with no jwt cookie answers 401", http.MethodGet, "/v1/article/1", nil}, {"POST /v1/article/ with no jwt cookie answers 401", http.MethodPost, "/v1/article/", []byte(`{"title":"Valid Title","body":"Some content"}`)}, {"DELETE /v1/article/1 with no jwt cookie answers 401", http.MethodDelete, "/v1/article/1", nil}, } diff --git a/test/public_article_read_test.go b/test/public_article_read_test.go new file mode 100644 index 0000000..31261fa --- /dev/null +++ b/test/public_article_read_test.go @@ -0,0 +1,268 @@ +package test + +import ( + "RenewCMS/api/handlers/article" + "RenewCMS/api/middleware" + "RenewCMS/api/pkg/apperr" + validator "RenewCMS/api/pkg/validator" + domainArticle "RenewCMS/internal/domain/article" + domainUser "RenewCMS/internal/domain/user" + "RenewCMS/internal/infrastructure/useCases" + "bytes" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/go-chi/jwtauth/v5" +) + +func newPublicArticleReadRouter(articleRepo *mockArticleRepo, userRepo *mockUserRepo, token *jwtauth.JWTAuth) *chi.Mux { + getUserUseCase := useCases.NewGetUserUseCase(userRepo) + articleHandler := article.NewHandler( + useCases.NewCreateArticleUseCase(articleRepo), + useCases.NewGetArticleUseCase(articleRepo), + useCases.NewListArticlesUseCase(articleRepo), + useCases.NewDeleteArticleUseCase(articleRepo, newMockImageRepo()), + useCases.NewUpdateArticleUseCase(articleRepo), + getUserUseCase, + ) + + r := chi.NewRouter() + r.Route("/v1", func(r chi.Router) { + r.Route("/article", func(r chi.Router) { + r.Use(jwtauth.Verifier(token)) + r.Get("/", articleHandler.ListArticles) + r.Get("/{id}", articleHandler.GetArticle) + r.Group(func(r chi.Router) { + r.Use(jwtauth.Authenticator(token)) + r.Use(middleware.RequireVerifiedUser(getUserUseCase)) + r.Post("/", articleHandler.PostArticle) + r.Delete("/{id}", articleHandler.DeleteArticle) + r.Patch("/{id}", articleHandler.UpdateArticleBody) + r.Patch("/{id}/publish", articleHandler.PublishArticle) + r.Patch("/{id}/unpublish", articleHandler.UnpublishArticle) + }) + }) + }) + return r +} + +func seedPublicArticleReadRepo() *mockArticleRepo { + repo := newMockArticleRepo() + repo.articles[1] = domainArticle.Article{ID: 1, Title: "Backyard Renovation Update", IsOnline: true} + repo.articles[2] = domainArticle.Article{ID: 2, Title: "Draft: Kitchen Remodel", IsOnline: false} + return repo +} + +func newPublicArticleReadUserRepo(isVerified bool) *mockUserRepo { + repo := newMockUserRepo() + repo.users[1] = domainUser.User{ID: 1, IsVerified: isVerified} + return repo +} + +func mintPublicArticleReadCookie(t *testing.T, token *jwtauth.JWTAuth, userId uint32) *http.Cookie { + t.Helper() + _, tokenString, err := token.Encode(map[string]any{"user_id": userId}) + if err != nil { + t.Fatalf("failed to encode jwt: %v", err) + } + return &http.Cookie{Name: "jwt", Value: tokenString} +} + +func assertPublicArticleReadResponse(t *testing.T, rec *httptest.ResponseRecorder, expectedId uint32) { + t.Helper() + if rec.Code != http.StatusOK { + t.Fatalf("expected status 200, got %d, body %q", rec.Code, rec.Body.String()) + } + var got domainArticle.Article + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("failed to decode response: %v", err) + } + if got.ID != expectedId { + t.Errorf("expected article id %d, got %d", expectedId, got.ID) + } +} + +func assertPublicArticleReadNotFound(t *testing.T, rec *httptest.ResponseRecorder) { + t.Helper() + if rec.Code != http.StatusNotFound { + t.Fatalf("expected status 404, got %d, body %q", rec.Code, rec.Body.String()) + } + if rec.Body.String() != apperr.ArticleNotFoundErrorMessage+"\n" { + t.Errorf("unexpected body: %q", rec.Body.String()) + } +} + +func TestPublicArticleReadHttp(t *testing.T) { + validator.InitValidator() + token := jwtauth.New("HS256", []byte("test-secret"), nil) + + t.Run("An anonymous visitor can read an online article", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newMockUserRepo(), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/1", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assertPublicArticleReadResponse(t, rec, 1) + }) + + t.Run("An anonymous visitor is told an offline article does not exist", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newMockUserRepo(), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/2", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assertPublicArticleReadNotFound(t, rec) + }) + + t.Run("An anonymous visitor is told an unknown id does not exist", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newMockUserRepo(), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/9999", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assertPublicArticleReadNotFound(t, rec) + }) + + t.Run("A verified user can open their own offline draft", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newPublicArticleReadUserRepo(true), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/2", nil) + req.AddCookie(mintPublicArticleReadCookie(t, token, 1)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assertPublicArticleReadResponse(t, rec, 2) + }) + + t.Run("A verified user still reads an online article normally", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newPublicArticleReadUserRepo(true), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/1", nil) + req.AddCookie(mintPublicArticleReadCookie(t, token, 1)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assertPublicArticleReadResponse(t, rec, 1) + }) + + t.Run("A verified user requesting an unknown id gets the same not-found response", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newPublicArticleReadUserRepo(true), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/9999", nil) + req.AddCookie(mintPublicArticleReadCookie(t, token, 1)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assertPublicArticleReadNotFound(t, rec) + }) + + t.Run("An authenticated but unverified user is told an offline article does not exist, not refused with 403", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newPublicArticleReadUserRepo(false), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/2", nil) + req.AddCookie(mintPublicArticleReadCookie(t, token, 1)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + assertPublicArticleReadNotFound(t, rec) + }) + + t.Run("A malformed or expired jwt cookie falls back to the public online-only visibility", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newMockUserRepo(), token) + cookie := &http.Cookie{Name: "jwt", Value: "not-a-real-token"} + + onlineReq := httptest.NewRequest(http.MethodGet, "/v1/article/1", nil) + onlineReq.AddCookie(cookie) + onlineRec := httptest.NewRecorder() + router.ServeHTTP(onlineRec, onlineReq) + if onlineRec.Code != http.StatusOK { + t.Fatalf("expected status 200, got %d, body %q", onlineRec.Code, onlineRec.Body.String()) + } + + offlineReq := httptest.NewRequest(http.MethodGet, "/v1/article/2", nil) + offlineReq.AddCookie(cookie) + offlineRec := httptest.NewRecorder() + router.ServeHTTP(offlineRec, offlineReq) + assertPublicArticleReadNotFound(t, offlineRec) + }) + + t.Run("An id that isn't a valid uint32 still answers 400, regardless of caller", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newMockUserRepo(), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/not-a-number", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected status 400, got %d, body %q", rec.Code, rec.Body.String()) + } + if rec.Body.String() != apperr.IdUint32ErrorMessage+"\n" { + t.Errorf("unexpected body: %q", rec.Body.String()) + } + }) + + protectedRouteCases := []struct { + method string + path string + }{ + {http.MethodPost, "/v1/article/"}, + {http.MethodDelete, "/v1/article/1"}, + {http.MethodPatch, "/v1/article/1"}, + {http.MethodPatch, "/v1/article/1/publish"}, + {http.MethodPatch, "/v1/article/1/unpublish"}, + } + for _, tc := range protectedRouteCases { + t.Run("The rest of the article routes still require a verified account: "+tc.method+" "+tc.path, func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newMockUserRepo(), token) + req := httptest.NewRequest(tc.method, tc.path, nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("expected status 401, got %d, body %q", rec.Code, rec.Body.String()) + } + }) + } + + t.Run("A response write failure is reported instead of crashing the handler", func(t *testing.T) { + router := newPublicArticleReadRouter(seedPublicArticleReadRepo(), newMockUserRepo(), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/1", nil) + + originalStdout := os.Stdout + readEnd, writeEnd, err := os.Pipe() + if err != nil { + t.Fatalf("failed to create pipe: %v", err) + } + os.Stdout = writeEnd + + router.ServeHTTP(&erroringResponseWriter{}, req) + + writeEnd.Close() + os.Stdout = originalStdout + var captured bytes.Buffer + if _, err := io.Copy(&captured, readEnd); err != nil { + t.Fatalf("failed to read captured stdout: %v", err) + } + + if !strings.Contains(captured.String(), "Failed to write response") { + t.Errorf("expected the write failure to be reported, got %q", captured.String()) + } + }) + + t.Run("A repository failure still answers 500 regardless of caller state", func(t *testing.T) { + repo := seedPublicArticleReadRepo() + repo.fail.failNext("Get") + router := newPublicArticleReadRouter(repo, newMockUserRepo(), token) + req := httptest.NewRequest(http.MethodGet, "/v1/article/1", nil) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("expected status 500, got %d, body %q", rec.Code, rec.Body.String()) + } + if rec.Body.String() != "Internal server error\n" { + t.Errorf("unexpected body: %q", rec.Body.String()) + } + }) +} diff --git a/test/verified_account_required_http_test.go b/test/verified_account_required_http_test.go index af5afb7..9d79aa4 100644 --- a/test/verified_account_required_http_test.go +++ b/test/verified_account_required_http_test.go @@ -42,10 +42,10 @@ func newVerifiedAccountRequiredRouter(articleRepo *mockArticleRepo, userRepo *mo r.Route("/article", func(r chi.Router) { r.Use(jwtauth.Verifier(token)) r.Get("/", articleHandler.ListArticles) + r.Get("/{id}", articleHandler.GetArticle) r.Group(func(r chi.Router) { r.Use(jwtauth.Authenticator(token)) r.Use(middleware.RequireVerifiedUser(getUserUseCase)) - r.Get("/{id}", articleHandler.GetArticle) r.Post("/", articleHandler.PostArticle) r.Delete("/{id}", articleHandler.DeleteArticle) r.Patch("/{id}", articleHandler.UpdateArticleBody) @@ -97,7 +97,6 @@ func TestVerifiedAccountRequiredHttp(t *testing.T) { path string body string }{ - {http.MethodGet, "/v1/article/1", ""}, {http.MethodPost, "/v1/article/", `{"title": "Valid Title", "body": "Some content"}`}, {http.MethodDelete, "/v1/article/1", ""}, {http.MethodPatch, "/v1/article/1", `{"body": "Updated"}`}, @@ -222,7 +221,6 @@ func TestVerifiedAccountRequiredHttp(t *testing.T) { method string path string }{ - {http.MethodGet, "/v1/article/1"}, {http.MethodPost, "/v1/article/"}, } for _, tc := range noCookieCases {