From 3e927407b6ff00288d055cd5be80da206fa09f7e Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 19:50:28 +0000 Subject: [PATCH 1/3] Extract read-then-authorize boilerplate into helper methods Co-authored-by: Shelley --- handlers/authorize.go | 67 +++++++++++++++++++++++++++++++++++++++++++ handlers/comments.go | 27 ++--------------- handlers/reactions.go | 14 +-------- handlers/reviews.go | 27 ++--------------- 4 files changed, 74 insertions(+), 61 deletions(-) create mode 100644 handlers/authorize.go diff --git a/handlers/authorize.go b/handlers/authorize.go new file mode 100644 index 00000000..e9a73a97 --- /dev/null +++ b/handlers/authorize.go @@ -0,0 +1,67 @@ +package handlers + +import ( + "fmt" + "log" + "net/http" + + "github.com/mtlynch/screenjournal/v2/screenjournal" + "github.com/mtlynch/screenjournal/v2/store" +) + +// readOwnedReview reads a review and verifies ownership. Returns false if it +// wrote an error response. +func (s Server) readOwnedReview(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, bool) { + review, err := s.getDB(r).ReadReview(id) + if err == store.ErrReviewNotFound { + http.Error(w, "Review not found", http.StatusNotFound) + return screenjournal.Review{}, false + } else if err != nil { + log.Printf("failed to read review: %v", err) + http.Error(w, fmt.Sprintf("Failed to read review: %v", err), http.StatusInternalServerError) + return screenjournal.Review{}, false + } + if !mustGetUsernameFromContext(r.Context()).Equal(review.Owner) { + http.Error(w, "You can't modify another user's review", http.StatusForbidden) + return screenjournal.Review{}, false + } + return review, true +} + +// readOwnedComment reads a comment and verifies ownership. Returns false if it +// wrote an error response. +func (s Server) readOwnedComment(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, bool) { + rc, err := s.getDB(r).ReadComment(id) + if err == store.ErrCommentNotFound { + http.Error(w, "Comment not found", http.StatusNotFound) + return screenjournal.ReviewComment{}, false + } else if err != nil { + log.Printf("failed to read comment: %v", err) + http.Error(w, fmt.Sprintf("Failed to read comment: %v", err), http.StatusInternalServerError) + return screenjournal.ReviewComment{}, false + } + if !mustGetUsernameFromContext(r.Context()).Equal(rc.Owner) { + http.Error(w, "Can't modify another user's comment", http.StatusForbidden) + return screenjournal.ReviewComment{}, false + } + return rc, true +} + +// readOwnedReaction reads a reaction and verifies ownership or admin status. +// Returns false if it wrote an error response. +func (s Server) readOwnedReaction(w http.ResponseWriter, r *http.Request, id screenjournal.ReactionID) (screenjournal.ReviewReaction, bool) { + rr, err := s.getDB(r).ReadReaction(id) + if err == store.ErrReactionNotFound { + http.Error(w, "Reaction not found", http.StatusNotFound) + return screenjournal.ReviewReaction{}, false + } else if err != nil { + log.Printf("failed to read reaction: %v", err) + http.Error(w, fmt.Sprintf("Failed to read reaction: %v", err), http.StatusInternalServerError) + return screenjournal.ReviewReaction{}, false + } + if !mustGetUsernameFromContext(r.Context()).Equal(rr.Owner) && !isAdmin(r.Context()) { + http.Error(w, "Can't delete another user's reaction", http.StatusForbidden) + return screenjournal.ReviewReaction{}, false + } + return rr, true +} diff --git a/handlers/comments.go b/handlers/comments.go index 26433307..4ce87fa8 100644 --- a/handlers/comments.go +++ b/handlers/comments.go @@ -197,18 +197,8 @@ func (s Server) commentsPut() http.HandlerFunc { return } - rc, err := s.getDB(r).ReadComment(req.CommentID) - if err == store.ErrCommentNotFound { - http.Error(w, "Comment not found", http.StatusNotFound) - return - } else if err != nil { - log.Printf("failed to read comment: %v", err) - http.Error(w, fmt.Sprintf("Failed to read comment: %v", err), http.StatusInternalServerError) - return - } - - if !mustGetUsernameFromContext(r.Context()).Equal(rc.Owner) { - http.Error(w, "Can't edit another user's comment", http.StatusForbidden) + rc, ok := s.readOwnedComment(w, r, req.CommentID) + if !ok { return } @@ -241,18 +231,7 @@ func (s Server) commentsDelete() http.HandlerFunc { return } - rc, err := s.getDB(r).ReadComment(cid) - if err == store.ErrCommentNotFound { - http.Error(w, "Comment not found", http.StatusNotFound) - return - } else if err != nil { - log.Printf("failed to read comment: %v", err) - http.Error(w, fmt.Sprintf("Failed to read comment: %v", err), http.StatusInternalServerError) - return - } - - if !mustGetUsernameFromContext(r.Context()).Equal(rc.Owner) { - http.Error(w, "Can't delete another user's comment", http.StatusForbidden) + if _, ok := s.readOwnedComment(w, r, cid); !ok { return } diff --git a/handlers/reactions.go b/handlers/reactions.go index 3d075b0f..0acc8fd5 100644 --- a/handlers/reactions.go +++ b/handlers/reactions.go @@ -119,19 +119,7 @@ func (s Server) reactionsDelete() http.HandlerFunc { return } - rr, err := s.getDB(r).ReadReaction(rid) - if err == store.ErrReactionNotFound { - http.Error(w, "Reaction not found", http.StatusNotFound) - return - } else if err != nil { - log.Printf("failed to read reaction: %v", err) - http.Error(w, fmt.Sprintf("Failed to read reaction: %v", err), http.StatusInternalServerError) - return - } - - loggedInUsername := mustGetUsernameFromContext(r.Context()) - if !loggedInUsername.Equal(rr.Owner) && !isAdmin(r.Context()) { - http.Error(w, "Can't delete another user's reaction", http.StatusForbidden) + if _, ok := s.readOwnedReaction(w, r, rid); !ok { return } diff --git a/handlers/reviews.go b/handlers/reviews.go index 274515fa..2ae7d08c 100644 --- a/handlers/reviews.go +++ b/handlers/reviews.go @@ -91,18 +91,8 @@ func (s Server) reviewsPut() http.HandlerFunc { return } - review, err := s.getDB(r).ReadReview(id) - if err == store.ErrReviewNotFound { - http.Error(w, "Review not found", http.StatusNotFound) - return - } else if err != nil { - http.Error(w, fmt.Sprintf("Failed to read review: %v", err), http.StatusInternalServerError) - return - } - - loggedInUsername := mustGetUsernameFromContext(r.Context()) - if !review.Owner.Equal(loggedInUsername) { - http.Error(w, "You can't edit another user's review", http.StatusForbidden) + review, ok := s.readOwnedReview(w, r, id) + if !ok { return } @@ -140,18 +130,7 @@ func (s Server) reviewsDelete() http.HandlerFunc { return } - review, err := s.getDB(r).ReadReview(id) - if err == store.ErrReviewNotFound { - http.Error(w, "Review not found", http.StatusNotFound) - return - } else if err != nil { - http.Error(w, fmt.Sprintf("Failed to read review: %v", err), http.StatusInternalServerError) - return - } - - loggedInUsername := mustGetUsernameFromContext(r.Context()) - if !review.Owner.Equal(loggedInUsername) { - http.Error(w, "You can't delete another user's review", http.StatusForbidden) + if _, ok := s.readOwnedReview(w, r, id); !ok { return } From be42eaae839b4bc3631326b5c07da9c73b69aa6a Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 21:05:27 +0000 Subject: [PATCH 2/3] Refactor write-path auth to operation helpers with owner-or-admin policy Replace the readOwned* helpers with a cleaner split between read and write concerns. - Keep read helpers focused on read errors only: readReviewOrWriteError, readCommentOrWriteError, readReactionOrWriteError. - Introduce operation-level write helpers that own authorization + mutation: updateReview, deleteReview, updateComment, deleteComment, deleteReaction. - Centralize policy with isOwnerOrAdmin so all mutating operations consistently allow either the resource owner or an admin. This preserves the refactor intent (reducing boilerplate) while avoiding API semantics where a "read" method implies mutation intent. Update handlers to call the new write helpers directly: - reviewsPut/reviewsDelete - commentsPut/commentsDelete - reactionsDelete Expand test coverage to lock in new behavior: - Reviews: admin can update another user's review. - Reviews: new delete test verifies admin can delete another user's review and non-admin cannot. - Comments: admin can update and delete another user's comment. - Existing reactions delete tests already covered admin/non-admin cases and continue to pass. --- handlers/authorize.go | 139 ++++++++++++++++++++++++----- handlers/comments.go | 22 +---- handlers/comments_test.go | 60 +++++++++++++ handlers/reactions.go | 8 +- handlers/reviews.go | 26 +----- handlers/reviews_test.go | 183 ++++++++++++++++++++++++++++++++++++++ 6 files changed, 369 insertions(+), 69 deletions(-) diff --git a/handlers/authorize.go b/handlers/authorize.go index e9a73a97..cd135c8f 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -9,9 +9,11 @@ import ( "github.com/mtlynch/screenjournal/v2/store" ) -// readOwnedReview reads a review and verifies ownership. Returns false if it -// wrote an error response. -func (s Server) readOwnedReview(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, bool) { +func (s Server) isOwnerOrAdmin(r *http.Request, owner screenjournal.Username) bool { + return mustGetUsernameFromContext(r.Context()).Equal(owner) || isAdmin(r.Context()) +} + +func (s Server) readReviewOrWriteError(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, bool) { review, err := s.getDB(r).ReadReview(id) if err == store.ErrReviewNotFound { http.Error(w, "Review not found", http.StatusNotFound) @@ -21,16 +23,10 @@ func (s Server) readOwnedReview(w http.ResponseWriter, r *http.Request, id scree http.Error(w, fmt.Sprintf("Failed to read review: %v", err), http.StatusInternalServerError) return screenjournal.Review{}, false } - if !mustGetUsernameFromContext(r.Context()).Equal(review.Owner) { - http.Error(w, "You can't modify another user's review", http.StatusForbidden) - return screenjournal.Review{}, false - } return review, true } -// readOwnedComment reads a comment and verifies ownership. Returns false if it -// wrote an error response. -func (s Server) readOwnedComment(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, bool) { +func (s Server) readCommentOrWriteError(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, bool) { rc, err := s.getDB(r).ReadComment(id) if err == store.ErrCommentNotFound { http.Error(w, "Comment not found", http.StatusNotFound) @@ -40,16 +36,10 @@ func (s Server) readOwnedComment(w http.ResponseWriter, r *http.Request, id scre http.Error(w, fmt.Sprintf("Failed to read comment: %v", err), http.StatusInternalServerError) return screenjournal.ReviewComment{}, false } - if !mustGetUsernameFromContext(r.Context()).Equal(rc.Owner) { - http.Error(w, "Can't modify another user's comment", http.StatusForbidden) - return screenjournal.ReviewComment{}, false - } return rc, true } -// readOwnedReaction reads a reaction and verifies ownership or admin status. -// Returns false if it wrote an error response. -func (s Server) readOwnedReaction(w http.ResponseWriter, r *http.Request, id screenjournal.ReactionID) (screenjournal.ReviewReaction, bool) { +func (s Server) readReactionOrWriteError(w http.ResponseWriter, r *http.Request, id screenjournal.ReactionID) (screenjournal.ReviewReaction, bool) { rr, err := s.getDB(r).ReadReaction(id) if err == store.ErrReactionNotFound { http.Error(w, "Reaction not found", http.StatusNotFound) @@ -59,9 +49,118 @@ func (s Server) readOwnedReaction(w http.ResponseWriter, r *http.Request, id scr http.Error(w, fmt.Sprintf("Failed to read reaction: %v", err), http.StatusInternalServerError) return screenjournal.ReviewReaction{}, false } - if !mustGetUsernameFromContext(r.Context()).Equal(rr.Owner) && !isAdmin(r.Context()) { + return rr, true +} + +func (s Server) updateReview(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, bool) { + review, ok := s.readReviewOrWriteError(w, r, id) + if !ok { + return screenjournal.Review{}, false + } + if !s.isOwnerOrAdmin(r, review.Owner) { + http.Error(w, "You can't edit another user's review", http.StatusForbidden) + return screenjournal.Review{}, false + } + + parsedRequest, err := parseReviewPutRequest(r) + if err != nil { + http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) + return screenjournal.Review{}, false + } + + review.Rating = parsedRequest.Rating + review.Blurb = parsedRequest.Blurb + review.Watched = parsedRequest.Watched + + if err := s.getDB(r).UpdateReview(review); err != nil { + log.Printf("failed to update review: %v", err) + http.Error(w, fmt.Sprintf("Failed to update review: %v", err), http.StatusInternalServerError) + return screenjournal.Review{}, false + } + + return review, true +} + +func (s Server) deleteReview(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) bool { + review, ok := s.readReviewOrWriteError(w, r, id) + if !ok { + return false + } + if !s.isOwnerOrAdmin(r, review.Owner) { + http.Error(w, "You can't delete another user's review", http.StatusForbidden) + return false + } + + if err := s.getDB(r).DeleteReview(id); err != nil { + log.Printf("failed to delete review: %v", err) + http.Error(w, fmt.Sprintf("Failed to delete review: %v", err), http.StatusInternalServerError) + return false + } + + return true +} + +func (s Server) updateComment(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, bool) { + rc, ok := s.readCommentOrWriteError(w, r, id) + if !ok { + return screenjournal.ReviewComment{}, false + } + if !s.isOwnerOrAdmin(r, rc.Owner) { + http.Error(w, "Can't edit another user's comment", http.StatusForbidden) + return screenjournal.ReviewComment{}, false + } + + parsedRequest, err := parseCommentPutRequest(r) + if err != nil { + http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) + log.Printf("invalid comment PUT request: %v", err) + return screenjournal.ReviewComment{}, false + } + + rc.CommentText = parsedRequest.CommentText + if err := s.getDB(r).UpdateComment(rc); err != nil { + log.Printf("failed to update comment: %v", err) + http.Error(w, fmt.Sprintf("Failed to update comment: %v", err), http.StatusInternalServerError) + return screenjournal.ReviewComment{}, false + } + + return rc, true +} + +func (s Server) deleteComment(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) bool { + rc, ok := s.readCommentOrWriteError(w, r, id) + if !ok { + return false + } + if !s.isOwnerOrAdmin(r, rc.Owner) { + http.Error(w, "Can't delete another user's comment", http.StatusForbidden) + return false + } + + if err := s.getDB(r).DeleteComment(id); err != nil { + log.Printf("failed to delete comment id=%v: %v", id, err) + http.Error(w, "Failed to delete comment: %v", http.StatusInternalServerError) + return false + } + + return true +} + +func (s Server) deleteReaction(w http.ResponseWriter, r *http.Request, id screenjournal.ReactionID) bool { + rr, ok := s.readReactionOrWriteError(w, r, id) + if !ok { + return false + } + if !s.isOwnerOrAdmin(r, rr.Owner) { http.Error(w, "Can't delete another user's reaction", http.StatusForbidden) - return screenjournal.ReviewReaction{}, false + return false } - return rr, true + + if err := s.getDB(r).DeleteReaction(id); err != nil { + log.Printf("failed to delete reaction id=%v: %v", id, err) + http.Error(w, "Failed to delete reaction", http.StatusInternalServerError) + return false + } + + return true } diff --git a/handlers/comments.go b/handlers/comments.go index 4ce87fa8..505dab4b 100644 --- a/handlers/comments.go +++ b/handlers/comments.go @@ -190,25 +190,17 @@ func (s Server) commentsPut() http.HandlerFunc { Funcs(moviePageFns). ParseFS(templatesFS, "templates/pages/reviews-for-single-media-entry.html")) return func(w http.ResponseWriter, r *http.Request) { - req, err := parseCommentPutRequest(r) + cid, err := commentIDFromRequestPath(r) if err != nil { - http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) - log.Printf("invalid comment PUT request: %v", err) + http.Error(w, "Invalid comment ID", http.StatusBadRequest) return } - rc, ok := s.readOwnedComment(w, r, req.CommentID) + rc, ok := s.updateComment(w, r, cid) if !ok { return } - rc.CommentText = req.CommentText - if err := s.getDB(r).UpdateComment(rc); err != nil { - log.Printf("failed to update comment: %v", err) - http.Error(w, fmt.Sprintf("Failed to update comment: %v", err), http.StatusInternalServerError) - return - } - if err := t.ExecuteTemplate(w, "comment", struct { Comment screenjournal.ReviewComment LoggedInUsername screenjournal.Username @@ -231,13 +223,7 @@ func (s Server) commentsDelete() http.HandlerFunc { return } - if _, ok := s.readOwnedComment(w, r, cid); !ok { - return - } - - if err := s.getDB(r).DeleteComment(cid); err != nil { - log.Printf("failed to delete comment id=%v: %v", cid, err) - http.Error(w, "Failed to delete comment: %v", http.StatusInternalServerError) + if !s.deleteComment(w, r, cid) { return } diff --git a/handlers/comments_test.go b/handlers/comments_test.go index 67640fdc..bb667e5a 100644 --- a/handlers/comments_test.go +++ b/handlers/comments_test.go @@ -403,6 +403,40 @@ func TestCommentsPut(t *testing.T) { }, status: http.StatusForbidden, }, + { + description: "allows an admin to update another user's comment", + route: "/api/comments/1", + payload: "comment=Admin%20updated%20this%20comment", + sessionToken: "adm123", + sessions: []mockSessionEntry{ + makeCommentsTestData().sessions.userA, + makeCommentsTestData().sessions.userB, + { + token: "adm123", + session: sessions.Session{ + Username: screenjournal.Username("admin"), + IsAdmin: true, + }, + }, + }, + comments: []screenjournal.ReviewComment{ + { + ID: screenjournal.CommentID(1), + Owner: makeCommentsTestData().sessions.userA.session.Username, + CommentText: screenjournal.CommentText("Good insights!"), + Review: makeCommentsTestData().reviews.userBTheWaterBoy, + }, + }, + status: http.StatusOK, + expectedComments: []screenjournal.ReviewComment{ + { + ID: screenjournal.CommentID(1), + Owner: makeCommentsTestData().sessions.userA.session.Username, + CommentText: screenjournal.CommentText("Admin updated this comment"), + Review: makeCommentsTestData().reviews.userBTheWaterBoy, + }, + }, + }, { description: "prevents an unauthenticated user from updating any comment", route: "/api/comments/1", @@ -573,6 +607,32 @@ func TestCommentsDelete(t *testing.T) { }, status: http.StatusForbidden, }, + { + description: "allows an admin to delete another user's comment", + route: "/api/comments/1", + sessionToken: "adm123", + sessions: []mockSessionEntry{ + makeCommentsTestData().sessions.userA, + makeCommentsTestData().sessions.userB, + { + token: "adm123", + session: sessions.Session{ + Username: screenjournal.Username("admin"), + IsAdmin: true, + }, + }, + }, + comments: []screenjournal.ReviewComment{ + { + ID: screenjournal.CommentID(1), + Owner: makeCommentsTestData().sessions.userA.session.Username, + CommentText: screenjournal.CommentText("Good insights!"), + Review: makeCommentsTestData().reviews.userBTheWaterBoy, + }, + }, + status: http.StatusNoContent, + expectedComments: []screenjournal.ReviewComment{}, + }, { description: "prevents an unauthenticated user from deleting any comment", route: "/api/comments/1", diff --git a/handlers/reactions.go b/handlers/reactions.go index 0acc8fd5..88124bf1 100644 --- a/handlers/reactions.go +++ b/handlers/reactions.go @@ -119,13 +119,7 @@ func (s Server) reactionsDelete() http.HandlerFunc { return } - if _, ok := s.readOwnedReaction(w, r, rid); !ok { - return - } - - if err := s.getDB(r).DeleteReaction(rid); err != nil { - log.Printf("failed to delete reaction id=%v: %v", rid, err) - http.Error(w, "Failed to delete reaction", http.StatusInternalServerError) + if !s.deleteReaction(w, r, rid) { return } diff --git a/handlers/reviews.go b/handlers/reviews.go index 2ae7d08c..34622061 100644 --- a/handlers/reviews.go +++ b/handlers/reviews.go @@ -91,27 +91,11 @@ func (s Server) reviewsPut() http.HandlerFunc { return } - review, ok := s.readOwnedReview(w, r, id) + review, ok := s.updateReview(w, r, id) if !ok { return } - parsedRequest, err := parseReviewPutRequest(r) - if err != nil { - http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) - return - } - - review.Rating = parsedRequest.Rating - review.Blurb = parsedRequest.Blurb - review.Watched = parsedRequest.Watched - - if err := s.getDB(r).UpdateReview(review); err != nil { - log.Printf("failed to update review: %v", err) - http.Error(w, fmt.Sprintf("Failed to update review: %v", err), http.StatusInternalServerError) - return - } - var newRoute string if review.MediaType() == screenjournal.MediaTypeMovie { newRoute = fmt.Sprintf("/movies/%d", review.Movie.ID.Int64()) @@ -130,13 +114,7 @@ func (s Server) reviewsDelete() http.HandlerFunc { return } - if _, ok := s.readOwnedReview(w, r, id); !ok { - return - } - - if err := s.getDB(r).DeleteReview(id); err != nil { - log.Printf("failed to delete review: %v", err) - http.Error(w, fmt.Sprintf("Failed to delete review: %v", err), http.StatusInternalServerError) + if !s.deleteReview(w, r, id) { return } diff --git a/handlers/reviews_test.go b/handlers/reviews_test.go index 22272062..5ada91f1 100644 --- a/handlers/reviews_test.go +++ b/handlers/reviews_test.go @@ -844,6 +844,66 @@ func TestReviewsPut(t *testing.T) { sessionToken: "def456", expectedStatus: http.StatusForbidden, }, + { + description: "allows an admin to overwrite another user's review", + localMovies: []screenjournal.Movie{ + { + TmdbID: screenjournal.TmdbID(38), + ImdbID: screenjournal.ImdbID("tt0338013"), + Title: screenjournal.MediaTitle("Eternal Sunshine of the Spotless Mind"), + ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), + }, + }, + priorReviews: []screenjournal.Review{ + { + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), + Rating: screenjournal.NewRating(5), + Watched: mustParseWatchDate("2022-10-28"), + Blurb: screenjournal.Blurb("It's my favorite movie!"), + Movie: screenjournal.Movie{ + ID: screenjournal.MovieID(1), + TmdbID: screenjournal.TmdbID(38), + ImdbID: screenjournal.ImdbID("tt0338013"), + Title: screenjournal.MediaTitle("Eternal Sunshine of the Spotless Mind"), + ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), + }, + }, + }, + sessions: []mockSessionEntry{ + { + token: "abc123", + session: sessions.Session{ + Username: screenjournal.Username("userA"), + }, + }, + { + token: "adm123", + session: sessions.Session{ + Username: screenjournal.Username("admin"), + IsAdmin: true, + }, + }, + }, + route: "/reviews/1", + payload: "rating=4&watch-date=2022-10-30&blurb=Admin%20updated%20this%20review", + sessionToken: "adm123", + expectedStatus: http.StatusSeeOther, + expected: screenjournal.Review{ + Owner: screenjournal.Username("userA"), + Rating: screenjournal.NewRating(4), + Watched: mustParseWatchDate("2022-10-30"), + Blurb: screenjournal.Blurb("Admin updated this review"), + Movie: screenjournal.Movie{ + ID: screenjournal.MovieID(1), + TmdbID: screenjournal.TmdbID(38), + ImdbID: screenjournal.ImdbID("tt0338013"), + Title: screenjournal.MediaTitle("Eternal Sunshine of the Spotless Mind"), + ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), + }, + Comments: []screenjournal.ReviewComment{}, + }, + }, } { t.Run(tt.description, func(t *testing.T) { dataStore := test_sqlite.New() @@ -913,6 +973,129 @@ func TestReviewsPut(t *testing.T) { } } +func TestReviewsDelete(t *testing.T) { + for _, tt := range []struct { + description string + sessionToken string + sessions []mockSessionEntry + expectedStatus int + expectedReviewsCount int + }{ + { + description: "allows an admin to delete another user's review", + sessionToken: "adm123", + sessions: []mockSessionEntry{ + { + token: "abc123", + session: sessions.Session{ + Username: screenjournal.Username("userA"), + }, + }, + { + token: "adm123", + session: sessions.Session{ + Username: screenjournal.Username("admin"), + IsAdmin: true, + }, + }, + }, + expectedStatus: http.StatusSeeOther, + expectedReviewsCount: 0, + }, + { + description: "prevents a non-admin user from deleting another user's review", + sessionToken: "def456", + sessions: []mockSessionEntry{ + { + token: "abc123", + session: sessions.Session{ + Username: screenjournal.Username("userA"), + }, + }, + { + token: "def456", + session: sessions.Session{ + Username: screenjournal.Username("userB"), + }, + }, + }, + expectedStatus: http.StatusForbidden, + expectedReviewsCount: 1, + }, + } { + t.Run(tt.description, func(t *testing.T) { + dataStore := test_sqlite.New() + + for _, s := range tt.sessions { + mockUser := screenjournal.User{ + Username: s.session.Username, + Email: screenjournal.Email(s.session.Username.String() + "@example.com"), + PasswordHash: screenjournal.PasswordHash("dummy-password-hash"), + } + if err := dataStore.InsertUser(mockUser); err != nil { + t.Fatalf("failed to insert mock user: %+v: %v", mockUser, err) + } + } + + movie := screenjournal.Movie{ + TmdbID: screenjournal.TmdbID(38), + ImdbID: screenjournal.ImdbID("tt0338013"), + Title: screenjournal.MediaTitle("Eternal Sunshine of the Spotless Mind"), + ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), + } + if _, err := dataStore.InsertMovie(movie); err != nil { + t.Fatalf("failed to insert mock movie %+v: %v", movie, err) + } + + review := screenjournal.Review{ + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), + Rating: screenjournal.NewRating(5), + Watched: mustParseWatchDate("2022-10-28"), + Blurb: screenjournal.Blurb("It's my favorite movie!"), + Movie: screenjournal.Movie{ + ID: screenjournal.MovieID(1), + TmdbID: screenjournal.TmdbID(38), + ImdbID: screenjournal.ImdbID("tt0338013"), + Title: screenjournal.MediaTitle("Eternal Sunshine of the Spotless Mind"), + ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), + }, + } + if _, err := dataStore.InsertReview(review); err != nil { + t.Fatalf("failed to insert mock review %+v: %v", review, err) + } + + sessionManager := newMockSessionManager(tt.sessions) + s := handlers.New(nilAuthenticator, nilAnnouncer, &sessionManager, dataStore, mockMetadataFinder{}) + + req, err := http.NewRequest("DELETE", "/reviews/1", strings.NewReader("")) + if err != nil { + t.Fatal(err) + } + req.AddCookie(&http.Cookie{ + Name: mockSessionTokenName, + Value: tt.sessionToken, + }) + + rec := httptest.NewRecorder() + s.Router().ServeHTTP(rec, req) + res := rec.Result() + + if got, want := res.StatusCode, tt.expectedStatus; got != want { + t.Fatalf("status=%d, want=%d", got, want) + } + + reviews, err := dataStore.ReadReviews() + if err != nil { + t.Fatalf("failed to read reviews: %v", err) + } + if got, want := len(reviews), tt.expectedReviewsCount; got != want { + t.Fatalf("reviewCount=%d, want=%d", got, want) + } + }) + } +} + func clearUnpredictableReviewProperties(r *screenjournal.Review) { r.ID = screenjournal.ReviewID(0) r.Created = time.Time{} From ca11e73bde34528aedd90e7a8cfd3bd5f58d8378 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 21:14:28 +0000 Subject: [PATCH 3/3] Split read helpers from HTTP writes using typed errors Refactor authorization/mutation helpers so read functions are pure data access and no longer write HTTP responses. - Replace readReviewOrWriteError/readCommentOrWriteError/readReactionOrWriteError with readReview/readComment/readReaction that return only (resource, error). - Keep authorization policy in write paths with errForbidden and owner-or-admin checks. - Make updateReview, deleteReview, updateComment, deleteComment, and deleteReaction return errors instead of writing responses. - Move HTTP status/message mapping into handlers for explicit, operation-specific behavior. --- handlers/authorize.go | 159 +++++++++++++++--------------------------- handlers/comments.go | 28 ++++++-- handlers/reactions.go | 11 ++- handlers/reviews.go | 29 +++++++- 4 files changed, 115 insertions(+), 112 deletions(-) diff --git a/handlers/authorize.go b/handlers/authorize.go index cd135c8f..0e915b2c 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -1,166 +1,119 @@ package handlers import ( - "fmt" - "log" + "errors" "net/http" "github.com/mtlynch/screenjournal/v2/screenjournal" - "github.com/mtlynch/screenjournal/v2/store" ) +var errForbidden = errors.New("forbidden") + func (s Server) isOwnerOrAdmin(r *http.Request, owner screenjournal.Username) bool { return mustGetUsernameFromContext(r.Context()).Equal(owner) || isAdmin(r.Context()) } -func (s Server) readReviewOrWriteError(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, bool) { - review, err := s.getDB(r).ReadReview(id) - if err == store.ErrReviewNotFound { - http.Error(w, "Review not found", http.StatusNotFound) - return screenjournal.Review{}, false - } else if err != nil { - log.Printf("failed to read review: %v", err) - http.Error(w, fmt.Sprintf("Failed to read review: %v", err), http.StatusInternalServerError) - return screenjournal.Review{}, false - } - return review, true +func (s Server) readReview(r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, error) { + return s.getDB(r).ReadReview(id) } -func (s Server) readCommentOrWriteError(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, bool) { - rc, err := s.getDB(r).ReadComment(id) - if err == store.ErrCommentNotFound { - http.Error(w, "Comment not found", http.StatusNotFound) - return screenjournal.ReviewComment{}, false - } else if err != nil { - log.Printf("failed to read comment: %v", err) - http.Error(w, fmt.Sprintf("Failed to read comment: %v", err), http.StatusInternalServerError) - return screenjournal.ReviewComment{}, false - } - return rc, true +func (s Server) readComment(r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, error) { + return s.getDB(r).ReadComment(id) } -func (s Server) readReactionOrWriteError(w http.ResponseWriter, r *http.Request, id screenjournal.ReactionID) (screenjournal.ReviewReaction, bool) { - rr, err := s.getDB(r).ReadReaction(id) - if err == store.ErrReactionNotFound { - http.Error(w, "Reaction not found", http.StatusNotFound) - return screenjournal.ReviewReaction{}, false - } else if err != nil { - log.Printf("failed to read reaction: %v", err) - http.Error(w, fmt.Sprintf("Failed to read reaction: %v", err), http.StatusInternalServerError) - return screenjournal.ReviewReaction{}, false - } - return rr, true +func (s Server) readReaction(r *http.Request, id screenjournal.ReactionID) (screenjournal.ReviewReaction, error) { + return s.getDB(r).ReadReaction(id) } -func (s Server) updateReview(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, bool) { - review, ok := s.readReviewOrWriteError(w, r, id) - if !ok { - return screenjournal.Review{}, false +func (s Server) updateReview( + r *http.Request, + id screenjournal.ReviewID, + updated reviewPutRequest, +) (screenjournal.Review, error) { + review, err := s.readReview(r, id) + if err != nil { + return screenjournal.Review{}, err } if !s.isOwnerOrAdmin(r, review.Owner) { - http.Error(w, "You can't edit another user's review", http.StatusForbidden) - return screenjournal.Review{}, false - } - - parsedRequest, err := parseReviewPutRequest(r) - if err != nil { - http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) - return screenjournal.Review{}, false + return screenjournal.Review{}, errForbidden } - review.Rating = parsedRequest.Rating - review.Blurb = parsedRequest.Blurb - review.Watched = parsedRequest.Watched + review.Rating = updated.Rating + review.Blurb = updated.Blurb + review.Watched = updated.Watched if err := s.getDB(r).UpdateReview(review); err != nil { - log.Printf("failed to update review: %v", err) - http.Error(w, fmt.Sprintf("Failed to update review: %v", err), http.StatusInternalServerError) - return screenjournal.Review{}, false + return screenjournal.Review{}, err } - return review, true + return review, nil } -func (s Server) deleteReview(w http.ResponseWriter, r *http.Request, id screenjournal.ReviewID) bool { - review, ok := s.readReviewOrWriteError(w, r, id) - if !ok { - return false +func (s Server) deleteReview(r *http.Request, id screenjournal.ReviewID) error { + review, err := s.readReview(r, id) + if err != nil { + return err } if !s.isOwnerOrAdmin(r, review.Owner) { - http.Error(w, "You can't delete another user's review", http.StatusForbidden) - return false + return errForbidden } if err := s.getDB(r).DeleteReview(id); err != nil { - log.Printf("failed to delete review: %v", err) - http.Error(w, fmt.Sprintf("Failed to delete review: %v", err), http.StatusInternalServerError) - return false + return err } - return true + return nil } -func (s Server) updateComment(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, bool) { - rc, ok := s.readCommentOrWriteError(w, r, id) - if !ok { - return screenjournal.ReviewComment{}, false +func (s Server) updateComment( + r *http.Request, + id screenjournal.CommentID, + commentText screenjournal.CommentText, +) (screenjournal.ReviewComment, error) { + rc, err := s.readComment(r, id) + if err != nil { + return screenjournal.ReviewComment{}, err } if !s.isOwnerOrAdmin(r, rc.Owner) { - http.Error(w, "Can't edit another user's comment", http.StatusForbidden) - return screenjournal.ReviewComment{}, false + return screenjournal.ReviewComment{}, errForbidden } - parsedRequest, err := parseCommentPutRequest(r) - if err != nil { - http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) - log.Printf("invalid comment PUT request: %v", err) - return screenjournal.ReviewComment{}, false - } - - rc.CommentText = parsedRequest.CommentText + rc.CommentText = commentText if err := s.getDB(r).UpdateComment(rc); err != nil { - log.Printf("failed to update comment: %v", err) - http.Error(w, fmt.Sprintf("Failed to update comment: %v", err), http.StatusInternalServerError) - return screenjournal.ReviewComment{}, false + return screenjournal.ReviewComment{}, err } - return rc, true + return rc, nil } -func (s Server) deleteComment(w http.ResponseWriter, r *http.Request, id screenjournal.CommentID) bool { - rc, ok := s.readCommentOrWriteError(w, r, id) - if !ok { - return false +func (s Server) deleteComment(r *http.Request, id screenjournal.CommentID) error { + rc, err := s.readComment(r, id) + if err != nil { + return err } if !s.isOwnerOrAdmin(r, rc.Owner) { - http.Error(w, "Can't delete another user's comment", http.StatusForbidden) - return false + return errForbidden } if err := s.getDB(r).DeleteComment(id); err != nil { - log.Printf("failed to delete comment id=%v: %v", id, err) - http.Error(w, "Failed to delete comment: %v", http.StatusInternalServerError) - return false + return err } - return true + return nil } -func (s Server) deleteReaction(w http.ResponseWriter, r *http.Request, id screenjournal.ReactionID) bool { - rr, ok := s.readReactionOrWriteError(w, r, id) - if !ok { - return false +func (s Server) deleteReaction(r *http.Request, id screenjournal.ReactionID) error { + rr, err := s.readReaction(r, id) + if err != nil { + return err } if !s.isOwnerOrAdmin(r, rr.Owner) { - http.Error(w, "Can't delete another user's reaction", http.StatusForbidden) - return false + return errForbidden } if err := s.getDB(r).DeleteReaction(id); err != nil { - log.Printf("failed to delete reaction id=%v: %v", id, err) - http.Error(w, "Failed to delete reaction", http.StatusInternalServerError) - return false + return err } - return true + return nil } diff --git a/handlers/comments.go b/handlers/comments.go index 505dab4b..69faaa27 100644 --- a/handlers/comments.go +++ b/handlers/comments.go @@ -1,6 +1,7 @@ package handlers import ( + "errors" "fmt" "html/template" "log" @@ -190,14 +191,23 @@ func (s Server) commentsPut() http.HandlerFunc { Funcs(moviePageFns). ParseFS(templatesFS, "templates/pages/reviews-for-single-media-entry.html")) return func(w http.ResponseWriter, r *http.Request) { - cid, err := commentIDFromRequestPath(r) + req, err := parseCommentPutRequest(r) if err != nil { - http.Error(w, "Invalid comment ID", http.StatusBadRequest) + http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) + log.Printf("invalid comment PUT request: %v", err) return } - rc, ok := s.updateComment(w, r, cid) - if !ok { + rc, err := s.updateComment(r, req.CommentID, req.CommentText) + if err == store.ErrCommentNotFound { + http.Error(w, "Comment not found", http.StatusNotFound) + return + } else if errors.Is(err, errForbidden) { + http.Error(w, "Can't edit another user's comment", http.StatusForbidden) + return + } else if err != nil { + log.Printf("failed to update comment: %v", err) + http.Error(w, fmt.Sprintf("Failed to update comment: %v", err), http.StatusInternalServerError) return } @@ -223,7 +233,15 @@ func (s Server) commentsDelete() http.HandlerFunc { return } - if !s.deleteComment(w, r, cid) { + if err := s.deleteComment(r, cid); err == store.ErrCommentNotFound { + http.Error(w, "Comment not found", http.StatusNotFound) + return + } else if errors.Is(err, errForbidden) { + http.Error(w, "Can't delete another user's comment", http.StatusForbidden) + return + } else if err != nil { + log.Printf("failed to delete comment id=%v: %v", cid, err) + http.Error(w, "Failed to delete comment: %v", http.StatusInternalServerError) return } diff --git a/handlers/reactions.go b/handlers/reactions.go index 88124bf1..1212f9ec 100644 --- a/handlers/reactions.go +++ b/handlers/reactions.go @@ -1,6 +1,7 @@ package handlers import ( + "errors" "fmt" "html/template" "log" @@ -119,7 +120,15 @@ func (s Server) reactionsDelete() http.HandlerFunc { return } - if !s.deleteReaction(w, r, rid) { + if err := s.deleteReaction(r, rid); err == store.ErrReactionNotFound { + http.Error(w, "Reaction not found", http.StatusNotFound) + return + } else if errors.Is(err, errForbidden) { + http.Error(w, "Can't delete another user's reaction", http.StatusForbidden) + return + } else if err != nil { + log.Printf("failed to delete reaction id=%v: %v", rid, err) + http.Error(w, "Failed to delete reaction", http.StatusInternalServerError) return } diff --git a/handlers/reviews.go b/handlers/reviews.go index 34622061..0a2fb7ef 100644 --- a/handlers/reviews.go +++ b/handlers/reviews.go @@ -1,6 +1,7 @@ package handlers import ( + "errors" "fmt" "log" "net/http" @@ -91,8 +92,22 @@ func (s Server) reviewsPut() http.HandlerFunc { return } - review, ok := s.updateReview(w, r, id) - if !ok { + parsedRequest, err := parseReviewPutRequest(r) + if err != nil { + http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) + return + } + + review, err := s.updateReview(r, id, parsedRequest) + if err == store.ErrReviewNotFound { + http.Error(w, "Review not found", http.StatusNotFound) + return + } else if errors.Is(err, errForbidden) { + http.Error(w, "You can't edit another user's review", http.StatusForbidden) + return + } else if err != nil { + log.Printf("failed to update review: %v", err) + http.Error(w, fmt.Sprintf("Failed to update review: %v", err), http.StatusInternalServerError) return } @@ -114,7 +129,15 @@ func (s Server) reviewsDelete() http.HandlerFunc { return } - if !s.deleteReview(w, r, id) { + if err := s.deleteReview(r, id); err == store.ErrReviewNotFound { + http.Error(w, "Review not found", http.StatusNotFound) + return + } else if errors.Is(err, errForbidden) { + http.Error(w, "You can't delete another user's review", http.StatusForbidden) + return + } else if err != nil { + log.Printf("failed to delete review: %v", err) + http.Error(w, fmt.Sprintf("Failed to delete review: %v", err), http.StatusInternalServerError) return }