From 3e927407b6ff00288d055cd5be80da206fa09f7e Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 19:50:28 +0000 Subject: [PATCH 1/9] 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/9] 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/9] 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 } From befbbe3926405136f31b064e7982c4ed1f99f675 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 21:26:07 +0000 Subject: [PATCH 4/9] Introduce request-scoped access service between handlers and store Refactor authorization and mutation flow to add a dedicated layer between HTTP handlers and persistence. Rationale: - Handlers should focus on HTTP concerns (parse request, map errors to status/messages, render response). - Store should remain persistence-only and unaware of actor/session policy. - Access control and mutation workflows belong in a middle layer with explicit actor context. What changed: - Added accessService in handlers/authorize.go, created per request via Server.access(r). - accessService captures db + actor + admin flag once, then applies owner-or-admin policy consistently. - Moved review/comment/reaction mutation methods onto accessService: - updateReview - deleteReview - updateComment - deleteComment - deleteReaction - Kept typed error return style (including errForbidden) so handlers continue to own HTTP mapping. - Updated reviews/comments/reactions handlers to call access := s.access(r) and invoke service methods instead of thin Server wrappers. Outcome: - Cleaner separation of concerns: handlers <-> access service <-> store. - Removes awkward thin Server.read* wrappers while preserving existing behavior and response semantics. --- handlers/authorize.go | 72 +++++++++++++++++++++++++------------------ handlers/comments.go | 6 ++-- handlers/reactions.go | 3 +- handlers/reviews.go | 6 ++-- 4 files changed, 52 insertions(+), 35 deletions(-) diff --git a/handlers/authorize.go b/handlers/authorize.go index 0e915b2c..8b62d5b1 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -9,32 +9,45 @@ import ( 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()) +type accessService struct { + db Store + actor screenjournal.Username + actorIsAdmin bool } -func (s Server) readReview(r *http.Request, id screenjournal.ReviewID) (screenjournal.Review, error) { - return s.getDB(r).ReadReview(id) +func (s Server) access(r *http.Request) accessService { + return accessService{ + db: s.getDB(r), + actor: mustGetUsernameFromContext(r.Context()), + actorIsAdmin: isAdmin(r.Context()), + } +} + +func (a accessService) isOwnerOrAdmin(owner screenjournal.Username) bool { + return a.actor.Equal(owner) || a.actorIsAdmin +} + +func (a accessService) readReview(id screenjournal.ReviewID) (screenjournal.Review, error) { + return a.db.ReadReview(id) } -func (s Server) readComment(r *http.Request, id screenjournal.CommentID) (screenjournal.ReviewComment, error) { - return s.getDB(r).ReadComment(id) +func (a accessService) readComment(id screenjournal.CommentID) (screenjournal.ReviewComment, error) { + return a.db.ReadComment(id) } -func (s Server) readReaction(r *http.Request, id screenjournal.ReactionID) (screenjournal.ReviewReaction, error) { - return s.getDB(r).ReadReaction(id) +func (a accessService) readReaction(id screenjournal.ReactionID) (screenjournal.ReviewReaction, error) { + return a.db.ReadReaction(id) } -func (s Server) updateReview( - r *http.Request, +func (a accessService) updateReview( id screenjournal.ReviewID, updated reviewPutRequest, ) (screenjournal.Review, error) { - review, err := s.readReview(r, id) + review, err := a.readReview(id) if err != nil { return screenjournal.Review{}, err } - if !s.isOwnerOrAdmin(r, review.Owner) { + if !a.isOwnerOrAdmin(review.Owner) { return screenjournal.Review{}, errForbidden } @@ -42,76 +55,75 @@ func (s Server) updateReview( review.Blurb = updated.Blurb review.Watched = updated.Watched - if err := s.getDB(r).UpdateReview(review); err != nil { + if err := a.db.UpdateReview(review); err != nil { return screenjournal.Review{}, err } return review, nil } -func (s Server) deleteReview(r *http.Request, id screenjournal.ReviewID) error { - review, err := s.readReview(r, id) +func (a accessService) deleteReview(id screenjournal.ReviewID) error { + review, err := a.readReview(id) if err != nil { return err } - if !s.isOwnerOrAdmin(r, review.Owner) { + if !a.isOwnerOrAdmin(review.Owner) { return errForbidden } - if err := s.getDB(r).DeleteReview(id); err != nil { + if err := a.db.DeleteReview(id); err != nil { return err } return nil } -func (s Server) updateComment( - r *http.Request, +func (a accessService) updateComment( id screenjournal.CommentID, commentText screenjournal.CommentText, ) (screenjournal.ReviewComment, error) { - rc, err := s.readComment(r, id) + rc, err := a.readComment(id) if err != nil { return screenjournal.ReviewComment{}, err } - if !s.isOwnerOrAdmin(r, rc.Owner) { + if !a.isOwnerOrAdmin(rc.Owner) { return screenjournal.ReviewComment{}, errForbidden } rc.CommentText = commentText - if err := s.getDB(r).UpdateComment(rc); err != nil { + if err := a.db.UpdateComment(rc); err != nil { return screenjournal.ReviewComment{}, err } return rc, nil } -func (s Server) deleteComment(r *http.Request, id screenjournal.CommentID) error { - rc, err := s.readComment(r, id) +func (a accessService) deleteComment(id screenjournal.CommentID) error { + rc, err := a.readComment(id) if err != nil { return err } - if !s.isOwnerOrAdmin(r, rc.Owner) { + if !a.isOwnerOrAdmin(rc.Owner) { return errForbidden } - if err := s.getDB(r).DeleteComment(id); err != nil { + if err := a.db.DeleteComment(id); err != nil { return err } return nil } -func (s Server) deleteReaction(r *http.Request, id screenjournal.ReactionID) error { - rr, err := s.readReaction(r, id) +func (a accessService) deleteReaction(id screenjournal.ReactionID) error { + rr, err := a.readReaction(id) if err != nil { return err } - if !s.isOwnerOrAdmin(r, rr.Owner) { + if !a.isOwnerOrAdmin(rr.Owner) { return errForbidden } - if err := s.getDB(r).DeleteReaction(id); err != nil { + if err := a.db.DeleteReaction(id); err != nil { return err } diff --git a/handlers/comments.go b/handlers/comments.go index 69faaa27..cfa5e2d3 100644 --- a/handlers/comments.go +++ b/handlers/comments.go @@ -198,7 +198,8 @@ func (s Server) commentsPut() http.HandlerFunc { return } - rc, err := s.updateComment(r, req.CommentID, req.CommentText) + access := s.access(r) + rc, err := access.updateComment(req.CommentID, req.CommentText) if err == store.ErrCommentNotFound { http.Error(w, "Comment not found", http.StatusNotFound) return @@ -233,7 +234,8 @@ func (s Server) commentsDelete() http.HandlerFunc { return } - if err := s.deleteComment(r, cid); err == store.ErrCommentNotFound { + access := s.access(r) + if err := access.deleteComment(cid); err == store.ErrCommentNotFound { http.Error(w, "Comment not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { diff --git a/handlers/reactions.go b/handlers/reactions.go index 1212f9ec..92ce5799 100644 --- a/handlers/reactions.go +++ b/handlers/reactions.go @@ -120,7 +120,8 @@ func (s Server) reactionsDelete() http.HandlerFunc { return } - if err := s.deleteReaction(r, rid); err == store.ErrReactionNotFound { + access := s.access(r) + if err := access.deleteReaction(rid); err == store.ErrReactionNotFound { http.Error(w, "Reaction not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { diff --git a/handlers/reviews.go b/handlers/reviews.go index 0a2fb7ef..9c1be58d 100644 --- a/handlers/reviews.go +++ b/handlers/reviews.go @@ -98,7 +98,8 @@ func (s Server) reviewsPut() http.HandlerFunc { return } - review, err := s.updateReview(r, id, parsedRequest) + access := s.access(r) + review, err := access.updateReview(id, parsedRequest) if err == store.ErrReviewNotFound { http.Error(w, "Review not found", http.StatusNotFound) return @@ -129,7 +130,8 @@ func (s Server) reviewsDelete() http.HandlerFunc { return } - if err := s.deleteReview(r, id); err == store.ErrReviewNotFound { + access := s.access(r) + if err := access.deleteReview(id); err == store.ErrReviewNotFound { http.Error(w, "Review not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { From 3f7c7ac01774d005da5e7029d0653c8855d536bb Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 21:42:28 +0000 Subject: [PATCH 5/9] Enforce access-control path via restricted Store and AccessStore Narrow Server DB usage so handlers cannot directly call guarded mutators. Design change: - Split data access interfaces into Store (restricted) and AccessStore (privileged). - Store now excludes guarded mutation methods for reviews/comments/reactions. - AccessStore embeds Store and adds those guarded mutators. Wiring changes: - Server now stores AccessStore as its single DB dependency. - Added getAccessDB(...) for the access layer; kept getDB(...) returning restricted Store. - Updated dev/prod DB helpers to support both methods. Access layer update: - accessService now depends on AccessStore via getAccessDB(...). Result: - Compile-time guardrail: handler code using getDB(...) cannot bypass access controls by calling protected mutations directly. - Protected write operations remain centralized in the access service path. --- handlers/authorize.go | 4 ++-- handlers/db_dev.go | 19 +++++++++++++++---- handlers/db_prod.go | 4 ++++ handlers/server.go | 4 ++-- handlers/store.go | 16 +++++++++++----- 5 files changed, 34 insertions(+), 13 deletions(-) diff --git a/handlers/authorize.go b/handlers/authorize.go index 8b62d5b1..9dacb082 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -10,14 +10,14 @@ import ( var errForbidden = errors.New("forbidden") type accessService struct { - db Store + db AccessStore actor screenjournal.Username actorIsAdmin bool } func (s Server) access(r *http.Request) accessService { return accessService{ - db: s.getDB(r), + db: s.getAccessDB(r), actor: mustGetUsernameFromContext(r.Context()), actorIsAdmin: isAdmin(r.Context()), } diff --git a/handlers/db_dev.go b/handlers/db_dev.go index 490fa27c..7c2f2b0b 100644 --- a/handlers/db_dev.go +++ b/handlers/db_dev.go @@ -186,13 +186,13 @@ type ( dbSettings struct { isolateBySession bool - tokenToDB map[dbToken]Store + tokenToDB map[dbToken]AccessStore lock sync.RWMutex } ) var sharedDBSettings = dbSettings{ - tokenToDB: map[dbToken]Store{}, + tokenToDB: map[dbToken]AccessStore{}, } func (dbs *dbSettings) IsSessionIsolationEnabled() bool { @@ -208,13 +208,13 @@ func (dbs *dbSettings) EnableSessionIsolation() { log.Print("per-session database = on") } -func (dbs *dbSettings) GetDB(token dbToken) Store { +func (dbs *dbSettings) GetDB(token dbToken) AccessStore { dbs.lock.RLock() defer dbs.lock.RUnlock() return dbs.tokenToDB[token] } -func (dbs *dbSettings) SaveDB(token dbToken, db Store) { +func (dbs *dbSettings) SaveDB(token dbToken, db AccessStore) { dbs.lock.Lock() defer dbs.lock.Unlock() dbs.tokenToDB[token] = db @@ -231,6 +231,17 @@ func (s Server) getDB(r *http.Request) Store { return sharedDBSettings.GetDB(dbToken(c.Value)) } +func (s Server) getAccessDB(r *http.Request) AccessStore { + if !sharedDBSettings.IsSessionIsolationEnabled() { + return s.store + } + c, err := r.Cookie(dbTokenCookieName) + if err != nil { + panic(err) + } + return sharedDBSettings.GetDB(dbToken(c.Value)) +} + func (s Server) getAuthenticator(r *http.Request) Authenticator { if !sharedDBSettings.IsSessionIsolationEnabled() { return s.authenticator diff --git a/handlers/db_prod.go b/handlers/db_prod.go index 1e04a4c0..4b46b1fc 100644 --- a/handlers/db_prod.go +++ b/handlers/db_prod.go @@ -14,6 +14,10 @@ func (s Server) getDB(*http.Request) Store { return s.store } +func (s Server) getAccessDB(*http.Request) AccessStore { + return s.store +} + func (s Server) getAuthenticator(_ *http.Request) Authenticator { return s.authenticator } diff --git a/handlers/server.go b/handlers/server.go index 926911ce..a8704ccd 100644 --- a/handlers/server.go +++ b/handlers/server.go @@ -43,7 +43,7 @@ type ( authenticator Authenticator announcer Announcer sessionManager SessionManager - store Store + store AccessStore metadataFinder MetadataFinder } ) @@ -55,7 +55,7 @@ func (s Server) Router() *mux.Router { // New creates a new server with all the state it needs to satisfy HTTP // requests. -func New(authenticator Authenticator, announcer Announcer, sessionManager SessionManager, store Store, metadataFinder MetadataFinder) Server { +func New(authenticator Authenticator, announcer Announcer, sessionManager SessionManager, store AccessStore, metadataFinder MetadataFinder) Server { s := Server{ router: mux.NewRouter(), authenticator: authenticator, diff --git a/handlers/store.go b/handlers/store.go index 76f7a7bc..dd175499 100644 --- a/handlers/store.go +++ b/handlers/store.go @@ -9,17 +9,12 @@ type Store interface { ReadReview(screenjournal.ReviewID) (screenjournal.Review, error) ReadReviews(...store.ReadReviewsOption) ([]screenjournal.Review, error) InsertReview(screenjournal.Review) (screenjournal.ReviewID, error) - UpdateReview(screenjournal.Review) error - DeleteReview(screenjournal.ReviewID) error ReadComments(screenjournal.ReviewID) ([]screenjournal.ReviewComment, error) ReadComment(screenjournal.CommentID) (screenjournal.ReviewComment, error) InsertComment(screenjournal.ReviewComment) (screenjournal.CommentID, error) - UpdateComment(screenjournal.ReviewComment) error - DeleteComment(screenjournal.CommentID) error ReadReactions(screenjournal.ReviewID) ([]screenjournal.ReviewReaction, error) ReadReaction(screenjournal.ReactionID) (screenjournal.ReviewReaction, error) InsertReaction(screenjournal.ReviewReaction) (screenjournal.ReactionID, error) - DeleteReaction(screenjournal.ReactionID) error ReadMovie(screenjournal.MovieID) (screenjournal.Movie, error) ReadMovieByTmdbID(screenjournal.TmdbID) (screenjournal.Movie, error) InsertMovie(screenjournal.Movie) (screenjournal.MovieID, error) @@ -50,3 +45,14 @@ type Store interface { DeletePasswordResetEntry(screenjournal.PasswordResetToken) error DeleteExpiredPasswordResetEntries() error } + +// AccessStore exposes mutating methods that are guarded by access checks. +// Handlers should use these methods via the access service, not directly. +type AccessStore interface { + Store + UpdateReview(screenjournal.Review) error + DeleteReview(screenjournal.ReviewID) error + UpdateComment(screenjournal.ReviewComment) error + DeleteComment(screenjournal.CommentID) error + DeleteReaction(screenjournal.ReactionID) error +} From 939279bab1717e92da6b7dd71c52748d5c6958a8 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 21:49:31 +0000 Subject: [PATCH 6/9] Use one Store interface with access-controlled getDB wrapper Collapse the dual-interface model (Store + AccessStore) into a single Store interface while keeping access checks centralized. Design: - Keep one Store interface that includes both read and mutation methods. - Make Server.getDB(r) return a dbService wrapper (not the raw store). - dbService embeds Store for passthrough behavior and enforces owner-or-admin checks on guarded mutation flows. Implementation details: - Removed AccessStore and rewired Server to depend only on Store. - Updated dev/prod getDB wiring so handlers always receive dbService. - Preserved handler call style: db := s.getDB(r). - Kept access enforcement in dbService methods for review/comment/reaction mutation operations. Outcome: - Simpler API surface (one DB interface). - Consistent naming (getDB/db) without introducing s.access. - Access checks remain centralized while non-guarded methods pass through transparently. --- handlers/authorize.go | 72 +++++++++++++++++++------------------------ handlers/comments.go | 8 ++--- handlers/db_dev.go | 24 ++++++--------- handlers/db_prod.go | 11 +++---- handlers/reactions.go | 4 +-- handlers/reviews.go | 8 ++--- handlers/server.go | 4 +-- handlers/store.go | 16 +++------- 8 files changed, 64 insertions(+), 83 deletions(-) diff --git a/handlers/authorize.go b/handlers/authorize.go index 9dacb082..26d5a61a 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -9,45 +9,37 @@ import ( var errForbidden = errors.New("forbidden") -type accessService struct { - db AccessStore - actor screenjournal.Username - actorIsAdmin bool +type dbService struct { + Store + request *http.Request } -func (s Server) access(r *http.Request) accessService { - return accessService{ - db: s.getAccessDB(r), - actor: mustGetUsernameFromContext(r.Context()), - actorIsAdmin: isAdmin(r.Context()), - } -} - -func (a accessService) isOwnerOrAdmin(owner screenjournal.Username) bool { - return a.actor.Equal(owner) || a.actorIsAdmin +func (d dbService) isOwnerOrAdmin(owner screenjournal.Username) bool { + return mustGetUsernameFromContext(d.request.Context()).Equal(owner) || + isAdmin(d.request.Context()) } -func (a accessService) readReview(id screenjournal.ReviewID) (screenjournal.Review, error) { - return a.db.ReadReview(id) +func (d dbService) readReview(id screenjournal.ReviewID) (screenjournal.Review, error) { + return d.ReadReview(id) } -func (a accessService) readComment(id screenjournal.CommentID) (screenjournal.ReviewComment, error) { - return a.db.ReadComment(id) +func (d dbService) readComment(id screenjournal.CommentID) (screenjournal.ReviewComment, error) { + return d.ReadComment(id) } -func (a accessService) readReaction(id screenjournal.ReactionID) (screenjournal.ReviewReaction, error) { - return a.db.ReadReaction(id) +func (d dbService) readReaction(id screenjournal.ReactionID) (screenjournal.ReviewReaction, error) { + return d.ReadReaction(id) } -func (a accessService) updateReview( +func (d dbService) updateReview( id screenjournal.ReviewID, updated reviewPutRequest, ) (screenjournal.Review, error) { - review, err := a.readReview(id) + review, err := d.readReview(id) if err != nil { return screenjournal.Review{}, err } - if !a.isOwnerOrAdmin(review.Owner) { + if !d.isOwnerOrAdmin(review.Owner) { return screenjournal.Review{}, errForbidden } @@ -55,75 +47,75 @@ func (a accessService) updateReview( review.Blurb = updated.Blurb review.Watched = updated.Watched - if err := a.db.UpdateReview(review); err != nil { + if err := d.UpdateReview(review); err != nil { return screenjournal.Review{}, err } return review, nil } -func (a accessService) deleteReview(id screenjournal.ReviewID) error { - review, err := a.readReview(id) +func (d dbService) deleteReview(id screenjournal.ReviewID) error { + review, err := d.readReview(id) if err != nil { return err } - if !a.isOwnerOrAdmin(review.Owner) { + if !d.isOwnerOrAdmin(review.Owner) { return errForbidden } - if err := a.db.DeleteReview(id); err != nil { + if err := d.DeleteReview(id); err != nil { return err } return nil } -func (a accessService) updateComment( +func (d dbService) updateComment( id screenjournal.CommentID, commentText screenjournal.CommentText, ) (screenjournal.ReviewComment, error) { - rc, err := a.readComment(id) + rc, err := d.readComment(id) if err != nil { return screenjournal.ReviewComment{}, err } - if !a.isOwnerOrAdmin(rc.Owner) { + if !d.isOwnerOrAdmin(rc.Owner) { return screenjournal.ReviewComment{}, errForbidden } rc.CommentText = commentText - if err := a.db.UpdateComment(rc); err != nil { + if err := d.UpdateComment(rc); err != nil { return screenjournal.ReviewComment{}, err } return rc, nil } -func (a accessService) deleteComment(id screenjournal.CommentID) error { - rc, err := a.readComment(id) +func (d dbService) deleteComment(id screenjournal.CommentID) error { + rc, err := d.readComment(id) if err != nil { return err } - if !a.isOwnerOrAdmin(rc.Owner) { + if !d.isOwnerOrAdmin(rc.Owner) { return errForbidden } - if err := a.db.DeleteComment(id); err != nil { + if err := d.DeleteComment(id); err != nil { return err } return nil } -func (a accessService) deleteReaction(id screenjournal.ReactionID) error { - rr, err := a.readReaction(id) +func (d dbService) deleteReaction(id screenjournal.ReactionID) error { + rr, err := d.readReaction(id) if err != nil { return err } - if !a.isOwnerOrAdmin(rr.Owner) { + if !d.isOwnerOrAdmin(rr.Owner) { return errForbidden } - if err := a.db.DeleteReaction(id); err != nil { + if err := d.DeleteReaction(id); err != nil { return err } diff --git a/handlers/comments.go b/handlers/comments.go index cfa5e2d3..7676ac65 100644 --- a/handlers/comments.go +++ b/handlers/comments.go @@ -198,8 +198,8 @@ func (s Server) commentsPut() http.HandlerFunc { return } - access := s.access(r) - rc, err := access.updateComment(req.CommentID, req.CommentText) + db := s.getDB(r) + rc, err := db.updateComment(req.CommentID, req.CommentText) if err == store.ErrCommentNotFound { http.Error(w, "Comment not found", http.StatusNotFound) return @@ -234,8 +234,8 @@ func (s Server) commentsDelete() http.HandlerFunc { return } - access := s.access(r) - if err := access.deleteComment(cid); err == store.ErrCommentNotFound { + db := s.getDB(r) + if err := db.deleteComment(cid); err == store.ErrCommentNotFound { http.Error(w, "Comment not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { diff --git a/handlers/db_dev.go b/handlers/db_dev.go index 7c2f2b0b..80b47d20 100644 --- a/handlers/db_dev.go +++ b/handlers/db_dev.go @@ -186,13 +186,13 @@ type ( dbSettings struct { isolateBySession bool - tokenToDB map[dbToken]AccessStore + tokenToDB map[dbToken]Store lock sync.RWMutex } ) var sharedDBSettings = dbSettings{ - tokenToDB: map[dbToken]AccessStore{}, + tokenToDB: map[dbToken]Store{}, } func (dbs *dbSettings) IsSessionIsolationEnabled() bool { @@ -208,19 +208,19 @@ func (dbs *dbSettings) EnableSessionIsolation() { log.Print("per-session database = on") } -func (dbs *dbSettings) GetDB(token dbToken) AccessStore { +func (dbs *dbSettings) GetDB(token dbToken) Store { dbs.lock.RLock() defer dbs.lock.RUnlock() return dbs.tokenToDB[token] } -func (dbs *dbSettings) SaveDB(token dbToken, db AccessStore) { +func (dbs *dbSettings) SaveDB(token dbToken, db Store) { dbs.lock.Lock() defer dbs.lock.Unlock() dbs.tokenToDB[token] = db } -func (s Server) getDB(r *http.Request) Store { +func (s Server) getRawDB(r *http.Request) Store { if !sharedDBSettings.IsSessionIsolationEnabled() { return s.store } @@ -231,22 +231,18 @@ func (s Server) getDB(r *http.Request) Store { return sharedDBSettings.GetDB(dbToken(c.Value)) } -func (s Server) getAccessDB(r *http.Request) AccessStore { - if !sharedDBSettings.IsSessionIsolationEnabled() { - return s.store +func (s Server) getDB(r *http.Request) dbService { + return dbService{ + Store: s.getRawDB(r), + request: r, } - c, err := r.Cookie(dbTokenCookieName) - if err != nil { - panic(err) - } - return sharedDBSettings.GetDB(dbToken(c.Value)) } func (s Server) getAuthenticator(r *http.Request) Authenticator { if !sharedDBSettings.IsSessionIsolationEnabled() { return s.authenticator } - return auth.New(s.getDB(r)) + return auth.New(s.getRawDB(r)) } func dbPerSessionPost() http.HandlerFunc { diff --git a/handlers/db_prod.go b/handlers/db_prod.go index 4b46b1fc..b42cfd93 100644 --- a/handlers/db_prod.go +++ b/handlers/db_prod.go @@ -10,12 +10,11 @@ func (s *Server) addDevRoutes() { // no-op } -func (s Server) getDB(*http.Request) Store { - return s.store -} - -func (s Server) getAccessDB(*http.Request) AccessStore { - return s.store +func (s Server) getDB(r *http.Request) dbService { + return dbService{ + Store: s.store, + request: r, + } } func (s Server) getAuthenticator(_ *http.Request) Authenticator { diff --git a/handlers/reactions.go b/handlers/reactions.go index 92ce5799..f62ef79a 100644 --- a/handlers/reactions.go +++ b/handlers/reactions.go @@ -120,8 +120,8 @@ func (s Server) reactionsDelete() http.HandlerFunc { return } - access := s.access(r) - if err := access.deleteReaction(rid); err == store.ErrReactionNotFound { + db := s.getDB(r) + if err := db.deleteReaction(rid); err == store.ErrReactionNotFound { http.Error(w, "Reaction not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { diff --git a/handlers/reviews.go b/handlers/reviews.go index 9c1be58d..3ef3f879 100644 --- a/handlers/reviews.go +++ b/handlers/reviews.go @@ -98,8 +98,8 @@ func (s Server) reviewsPut() http.HandlerFunc { return } - access := s.access(r) - review, err := access.updateReview(id, parsedRequest) + db := s.getDB(r) + review, err := db.updateReview(id, parsedRequest) if err == store.ErrReviewNotFound { http.Error(w, "Review not found", http.StatusNotFound) return @@ -130,8 +130,8 @@ func (s Server) reviewsDelete() http.HandlerFunc { return } - access := s.access(r) - if err := access.deleteReview(id); err == store.ErrReviewNotFound { + db := s.getDB(r) + if err := db.deleteReview(id); err == store.ErrReviewNotFound { http.Error(w, "Review not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { diff --git a/handlers/server.go b/handlers/server.go index a8704ccd..926911ce 100644 --- a/handlers/server.go +++ b/handlers/server.go @@ -43,7 +43,7 @@ type ( authenticator Authenticator announcer Announcer sessionManager SessionManager - store AccessStore + store Store metadataFinder MetadataFinder } ) @@ -55,7 +55,7 @@ func (s Server) Router() *mux.Router { // New creates a new server with all the state it needs to satisfy HTTP // requests. -func New(authenticator Authenticator, announcer Announcer, sessionManager SessionManager, store AccessStore, metadataFinder MetadataFinder) Server { +func New(authenticator Authenticator, announcer Announcer, sessionManager SessionManager, store Store, metadataFinder MetadataFinder) Server { s := Server{ router: mux.NewRouter(), authenticator: authenticator, diff --git a/handlers/store.go b/handlers/store.go index dd175499..76f7a7bc 100644 --- a/handlers/store.go +++ b/handlers/store.go @@ -9,12 +9,17 @@ type Store interface { ReadReview(screenjournal.ReviewID) (screenjournal.Review, error) ReadReviews(...store.ReadReviewsOption) ([]screenjournal.Review, error) InsertReview(screenjournal.Review) (screenjournal.ReviewID, error) + UpdateReview(screenjournal.Review) error + DeleteReview(screenjournal.ReviewID) error ReadComments(screenjournal.ReviewID) ([]screenjournal.ReviewComment, error) ReadComment(screenjournal.CommentID) (screenjournal.ReviewComment, error) InsertComment(screenjournal.ReviewComment) (screenjournal.CommentID, error) + UpdateComment(screenjournal.ReviewComment) error + DeleteComment(screenjournal.CommentID) error ReadReactions(screenjournal.ReviewID) ([]screenjournal.ReviewReaction, error) ReadReaction(screenjournal.ReactionID) (screenjournal.ReviewReaction, error) InsertReaction(screenjournal.ReviewReaction) (screenjournal.ReactionID, error) + DeleteReaction(screenjournal.ReactionID) error ReadMovie(screenjournal.MovieID) (screenjournal.Movie, error) ReadMovieByTmdbID(screenjournal.TmdbID) (screenjournal.Movie, error) InsertMovie(screenjournal.Movie) (screenjournal.MovieID, error) @@ -45,14 +50,3 @@ type Store interface { DeletePasswordResetEntry(screenjournal.PasswordResetToken) error DeleteExpiredPasswordResetEntries() error } - -// AccessStore exposes mutating methods that are guarded by access checks. -// Handlers should use these methods via the access service, not directly. -type AccessStore interface { - Store - UpdateReview(screenjournal.Review) error - DeleteReview(screenjournal.ReviewID) error - UpdateComment(screenjournal.ReviewComment) error - DeleteComment(screenjournal.CommentID) error - DeleteReaction(screenjournal.ReactionID) error -} From dc42a24dd4d3a36c8dcf750abdc04592f205ec78 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 22:03:08 +0000 Subject: [PATCH 7/9] Store only wrapped DB provider in Server and trim handler Store Refactor Server to depend on a DB provider abstraction instead of directly retaining the raw Store implementation. What changed: - Added dbProvider interface with request-scoped hooks for DB and authenticator resolution. - Server now stores dbProvider (field: db) and constructs it in New(...) via newDBProvider(store). - Introduced handlers/db.go as the single entrypoint for getDB/getAuthenticator delegation. - Implemented build-specific providers: - staticDBProvider for prod - sessionDBProvider for dev (supports per-session DB isolation and dynamic authenticator source) This removes direct raw-store retention from Server and ensures all handler DB access flows through wrapped request-aware DB resolution. Also reduced handlers.Store to methods actually used by handlers/access wrapper: - Removed ReadReviewSubscribers - Removed ReadCommentSubscribers (Those methods remain available on announce/email store interfaces where they are used.) Outcome: - Stronger architectural boundary (Server -> provider -> wrapped DB). - Cleaner handler-facing Store interface with less accidental coupling. --- handlers/db.go | 11 +++++++++++ handlers/db_dev.go | 24 +++++++++++++++++------- handlers/db_prod.go | 18 ++++++++++++++---- handlers/server.go | 9 +++++++-- handlers/store.go | 5 ----- 5 files changed, 49 insertions(+), 18 deletions(-) create mode 100644 handlers/db.go diff --git a/handlers/db.go b/handlers/db.go new file mode 100644 index 00000000..c12d2ac6 --- /dev/null +++ b/handlers/db.go @@ -0,0 +1,11 @@ +package handlers + +import "net/http" + +func (s Server) getDB(r *http.Request) dbService { + return s.db.dbForRequest(r) +} + +func (s Server) getAuthenticator(r *http.Request) Authenticator { + return s.db.authenticatorForRequest(r, s.authenticator) +} diff --git a/handlers/db_dev.go b/handlers/db_dev.go index 80b47d20..ed4e10b0 100644 --- a/handlers/db_dev.go +++ b/handlers/db_dev.go @@ -195,6 +195,16 @@ var sharedDBSettings = dbSettings{ tokenToDB: map[dbToken]Store{}, } +type sessionDBProvider struct { + store Store +} + +func newDBProvider(store Store) dbProvider { + return sessionDBProvider{ + store: store, + } +} + func (dbs *dbSettings) IsSessionIsolationEnabled() bool { dbs.lock.RLock() dbs.lock.RUnlock() @@ -220,9 +230,9 @@ func (dbs *dbSettings) SaveDB(token dbToken, db Store) { dbs.tokenToDB[token] = db } -func (s Server) getRawDB(r *http.Request) Store { +func (p sessionDBProvider) rawDB(r *http.Request) Store { if !sharedDBSettings.IsSessionIsolationEnabled() { - return s.store + return p.store } c, err := r.Cookie(dbTokenCookieName) if err != nil { @@ -231,18 +241,18 @@ func (s Server) getRawDB(r *http.Request) Store { return sharedDBSettings.GetDB(dbToken(c.Value)) } -func (s Server) getDB(r *http.Request) dbService { +func (p sessionDBProvider) dbForRequest(r *http.Request) dbService { return dbService{ - Store: s.getRawDB(r), + Store: p.rawDB(r), request: r, } } -func (s Server) getAuthenticator(r *http.Request) Authenticator { +func (p sessionDBProvider) authenticatorForRequest(r *http.Request, fallback Authenticator) Authenticator { if !sharedDBSettings.IsSessionIsolationEnabled() { - return s.authenticator + return fallback } - return auth.New(s.getRawDB(r)) + return auth.New(p.rawDB(r)) } func dbPerSessionPost() http.HandlerFunc { diff --git a/handlers/db_prod.go b/handlers/db_prod.go index b42cfd93..5ad931ca 100644 --- a/handlers/db_prod.go +++ b/handlers/db_prod.go @@ -10,13 +10,23 @@ func (s *Server) addDevRoutes() { // no-op } -func (s Server) getDB(r *http.Request) dbService { +type staticDBProvider struct { + store Store +} + +func newDBProvider(store Store) dbProvider { + return staticDBProvider{ + store: store, + } +} + +func (p staticDBProvider) dbForRequest(r *http.Request) dbService { return dbService{ - Store: s.store, + Store: p.store, request: r, } } -func (s Server) getAuthenticator(_ *http.Request) Authenticator { - return s.authenticator +func (p staticDBProvider) authenticatorForRequest(_ *http.Request, fallback Authenticator) Authenticator { + return fallback } diff --git a/handlers/server.go b/handlers/server.go index 926911ce..2a549ac3 100644 --- a/handlers/server.go +++ b/handlers/server.go @@ -38,12 +38,17 @@ type ( GetTvShow(id screenjournal.TmdbID) (screenjournal.TvShow, error) } + dbProvider interface { + dbForRequest(*http.Request) dbService + authenticatorForRequest(*http.Request, Authenticator) Authenticator + } + Server struct { router *mux.Router authenticator Authenticator announcer Announcer sessionManager SessionManager - store Store + db dbProvider metadataFinder MetadataFinder } ) @@ -61,7 +66,7 @@ func New(authenticator Authenticator, announcer Announcer, sessionManager Sessio authenticator: authenticator, announcer: announcer, sessionManager: sessionManager, - store: store, + db: newDBProvider(store), metadataFinder: metadataFinder, } diff --git a/handlers/store.go b/handlers/store.go index 76f7a7bc..bf0cc4ba 100644 --- a/handlers/store.go +++ b/handlers/store.go @@ -37,11 +37,6 @@ type Store interface { ReadSignupInvitation(screenjournal.InviteCode) (screenjournal.SignupInvitation, error) ReadSignupInvitations() ([]screenjournal.SignupInvitation, error) DeleteSignupInvitation(screenjournal.InviteCode) error - ReadReviewSubscribers() ([]screenjournal.EmailSubscriber, error) - ReadCommentSubscribers( - screenjournal.ReviewID, - screenjournal.Username, - ) ([]screenjournal.EmailSubscriber, error) ReadNotificationPreferences(screenjournal.Username) (screenjournal.NotificationPreferences, error) UpdateNotificationPreferences(screenjournal.Username, screenjournal.NotificationPreferences) error InsertPasswordResetEntry(screenjournal.PasswordResetEntry) error From 46e8687cc8022ed0610e6501174a0cab90e4cf5e Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Fri, 6 Feb 2026 18:24:39 -0500 Subject: [PATCH 8/9] work in progress --- handlers/authorize.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/handlers/authorize.go b/handlers/authorize.go index 26d5a61a..c4edbd93 100644 --- a/handlers/authorize.go +++ b/handlers/authorize.go @@ -31,10 +31,7 @@ func (d dbService) readReaction(id screenjournal.ReactionID) (screenjournal.Revi return d.ReadReaction(id) } -func (d dbService) updateReview( - id screenjournal.ReviewID, - updated reviewPutRequest, -) (screenjournal.Review, error) { +func (d dbService) updateReview(id screenjournal.ReviewID, updated reviewPutRequest) (screenjournal.Review, error) { review, err := d.readReview(id) if err != nil { return screenjournal.Review{}, err @@ -70,10 +67,7 @@ func (d dbService) deleteReview(id screenjournal.ReviewID) error { return nil } -func (d dbService) updateComment( - id screenjournal.CommentID, - commentText screenjournal.CommentText, -) (screenjournal.ReviewComment, error) { +func (d dbService) updateComment(id screenjournal.CommentID, commentText screenjournal.CommentText) (screenjournal.ReviewComment, error) { rc, err := d.readComment(id) if err != nil { return screenjournal.ReviewComment{}, err From b99285889b473f27958bda731a809f221dd45624 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Mon, 16 Feb 2026 18:12:15 -0500 Subject: [PATCH 9/9] Don't keep db var --- handlers/comments.go | 6 ++---- handlers/reactions.go | 3 +-- handlers/reviews.go | 6 ++---- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/handlers/comments.go b/handlers/comments.go index 7676ac65..d8312471 100644 --- a/handlers/comments.go +++ b/handlers/comments.go @@ -198,8 +198,7 @@ func (s Server) commentsPut() http.HandlerFunc { return } - db := s.getDB(r) - rc, err := db.updateComment(req.CommentID, req.CommentText) + rc, err := s.getDB(r).updateComment(req.CommentID, req.CommentText) if err == store.ErrCommentNotFound { http.Error(w, "Comment not found", http.StatusNotFound) return @@ -234,8 +233,7 @@ func (s Server) commentsDelete() http.HandlerFunc { return } - db := s.getDB(r) - if err := db.deleteComment(cid); err == store.ErrCommentNotFound { + if err := s.getDB(r).deleteComment(cid); err == store.ErrCommentNotFound { http.Error(w, "Comment not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { diff --git a/handlers/reactions.go b/handlers/reactions.go index f62ef79a..52d88e8a 100644 --- a/handlers/reactions.go +++ b/handlers/reactions.go @@ -120,8 +120,7 @@ func (s Server) reactionsDelete() http.HandlerFunc { return } - db := s.getDB(r) - if err := db.deleteReaction(rid); err == store.ErrReactionNotFound { + if err := s.getDB(r).deleteReaction(rid); err == store.ErrReactionNotFound { http.Error(w, "Reaction not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) { diff --git a/handlers/reviews.go b/handlers/reviews.go index 3ef3f879..fb938609 100644 --- a/handlers/reviews.go +++ b/handlers/reviews.go @@ -98,8 +98,7 @@ func (s Server) reviewsPut() http.HandlerFunc { return } - db := s.getDB(r) - review, err := db.updateReview(id, parsedRequest) + review, err := s.getDB(r).updateReview(id, parsedRequest) if err == store.ErrReviewNotFound { http.Error(w, "Review not found", http.StatusNotFound) return @@ -130,8 +129,7 @@ func (s Server) reviewsDelete() http.HandlerFunc { return } - db := s.getDB(r) - if err := db.deleteReview(id); err == store.ErrReviewNotFound { + if err := s.getDB(r).deleteReview(id); err == store.ErrReviewNotFound { http.Error(w, "Review not found", http.StatusNotFound) return } else if errors.Is(err, errForbidden) {