From 4e5656b8f970cc472197e599676bc52d61abdf78 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Sun, 1 Jan 2023 11:01:03 -0500 Subject: [PATCH] Enforce ownership on editing reviews (#94) --- e2e/helpers/login.js | 18 +- e2e/reviews.spec.ts | 48 +++++- handlers/auth_test.go | 20 ++- handlers/db_dev.go | 6 + handlers/reviews.go | 11 ++ handlers/reviews_test.go | 356 ++++++++++++++++++++++++++++++++++++--- handlers/users_test.go | 2 +- handlers/views.go | 11 ++ 8 files changed, 432 insertions(+), 40 deletions(-) diff --git a/e2e/helpers/login.js b/e2e/helpers/login.js index edd56f8a..de3a53d1 100644 --- a/e2e/helpers/login.js +++ b/e2e/helpers/login.js @@ -1,14 +1,26 @@ import { expect } from "@playwright/test"; -export async function loginAsAdmin(page) { +async function loginAsUser(page, username, password) { await page.goto("/"); await page.locator("data-test-id=sign-in-btn").click(); await expect(page).toHaveURL("/login"); - await page.locator("id=username").fill("dummyadmin"); - await page.locator("id=password").fill("dummypass"); + await page.locator("id=username").fill(username); + await page.locator("id=password").fill(password); await page.locator("form input[type='submit']").click(); await expect(page).toHaveURL("/reviews"); } + +export async function loginAsAdmin(page) { + await loginAsUser(page, "dummyadmin", "dummypass"); +} + +export async function loginAsUserA(page) { + await loginAsUser(page, "userA", "password123"); +} + +export async function loginAsUserB(page) { + await loginAsUser(page, "userB", "password456"); +} diff --git a/e2e/reviews.spec.ts b/e2e/reviews.spec.ts index 2db6e76c..048e636d 100644 --- a/e2e/reviews.spec.ts +++ b/e2e/reviews.spec.ts @@ -1,11 +1,11 @@ import { test, expect } from "@playwright/test"; import { populateDummyData, wipeDB } from "./helpers/db.js"; -import { loginAsAdmin } from "./helpers/login.js"; +import { loginAsUserA, loginAsUserB } from "./helpers/login.js"; test.beforeEach(async ({ page }) => { await wipeDB(page); await populateDummyData(page); - await loginAsAdmin(page); + await loginAsUserA(page); }); test("adds a new rating and fills in only required fields", async ({ @@ -34,7 +34,7 @@ test("adds a new rating and fills in only required fields", async ({ const reviewCard = await page.locator(":nth-match(.card, 1)"); await expect(reviewCard.locator(".card-title")).toHaveText("Slow Learners"); await expect(reviewCard.locator(".card-subtitle")).toHaveText( - /dummyadmin watched this .+ ago/, + /userA watched this .+ ago/, { useInnerText: true } ); await expect( @@ -87,7 +87,7 @@ You'll like it if you enjoy things like Children's Hospital, Comedy Bang Bang, o "Weird: The Al Yankovic Story" ); await expect(reviewCard.locator(".card-subtitle")).toHaveText( - /dummyadmin watched this .+ ago/, + /userA watched this .+ ago/, { useInnerText: true } ); await expect( @@ -111,7 +111,7 @@ If you think of Weird Al as just a parody music guy, give it a chance. I was nev "Weird: The Al Yankovic Story (2022)" ); await expect(page.locator(".card-subtitle")).toHaveText( - /dummyadmin watched this .+ ago/, + /userA watched this .+ ago/, { useInnerText: true } ); @@ -163,7 +163,7 @@ test("adds a new rating and fills all fields", async ({ page }) => { "Eternal Sunshine of the Spotless Mind" ); await expect(reviewCard.locator(".card-subtitle")).toHaveText( - /dummyadmin watched this .+ ago/, + /userA watched this .+ ago/, { useInnerText: true } ); await expect( @@ -225,7 +225,7 @@ test("adds a new rating and edits the details", async ({ page }) => { await expect(page).toHaveURL("/reviews"); await expect(reviewCard.locator(".card-subtitle")).toHaveText( - /dummyadmin watched this .+ ago/, + /userA watched this .+ ago/, { useInnerText: true } ); await expect( @@ -277,7 +277,7 @@ test("adds a new rating and cancels the edit", async ({ page }) => { await expect(page).toHaveURL("/reviews"); await expect(reviewCard.locator(".card-subtitle")).toHaveText( - /dummyadmin watched this .+ ago/, + /userA watched this .+ ago/, { useInnerText: true } ); await expect( @@ -293,3 +293,35 @@ test("adds a new rating and cancels the edit", async ({ page }) => { "Am I in the Matrix right now?" ); }); + +test("editing another user's review fails", async ({ page, browser }) => { + await page.locator("data-test-id=add-rating").click(); + + await page.locator("title-search #media-title").fill("the matri"); + const matchingTitle = await page.locator( + "#search-results-list li:first-child span" + ); + await expect(matchingTitle).toHaveText("The Matrix (1999)"); + await matchingTitle.click(); + await expect(page.locator("title-search #media-title")).toHaveValue( + "The Matrix" + ); + + await page.locator("#rating-select").selectOption({ label: "8" }); + await page.locator("#watched-date").fill("2022-10-29"); + await page.locator("#blurb").fill("Am I in the Matrix right now?"); + + await page.locator("form input[type='submit']").click(); + + await expect(page).toHaveURL("/reviews"); + + // Switch to other user. + const guestContext = await browser.newContext(); + const guestPage = await guestContext.newPage(); + await loginAsUserB(guestPage); + + const response = await guestPage.goto("/reviews/1/edit"); + await expect(response?.status()).toBe(403); + + await guestContext.close(); +}); diff --git a/handlers/auth_test.go b/handlers/auth_test.go index 7596babd..3087073f 100644 --- a/handlers/auth_test.go +++ b/handlers/auth_test.go @@ -10,6 +10,7 @@ import ( "github.com/mtlynch/screenjournal/v2" "github.com/mtlynch/screenjournal/v2/auth/simple" "github.com/mtlynch/screenjournal/v2/handlers" + "github.com/mtlynch/screenjournal/v2/handlers/sessions" "github.com/mtlynch/screenjournal/v2/metadata" "github.com/mtlynch/screenjournal/v2/store/test_sqlite" ) @@ -99,7 +100,7 @@ func TestAuthPost(t *testing.T) { authenticator := simple.New(dataStore) var nilMetadataFinder metadata.Finder - sessionManager := mockSessionManager{} + sessionManager := newMockSessionManager([]mockSession{}) s := handlers.New(authenticator, &sessionManager, dataStore, nilMetadataFinder) @@ -115,8 +116,21 @@ func TestAuthPost(t *testing.T) { if got, want := w.Code, tt.status; got != want { t.Fatalf("httpStatus=%v, want=%v", got, want) } - if got, want := sessionManager.lastSession, tt.matchingUser; !reflect.DeepEqual(got, want) { - t.Errorf("user=%v, want=%v", got, want) + + if tt.status != http.StatusOK { + return + } + + sessionsCreated := []sessions.Session{} + for _, session := range sessionManager.sessions { + sessionsCreated = append(sessionsCreated, session) + } + + if got, want := len(sessionsCreated), 1; got != want { + t.Fatalf("count(sessions)=%d, want=%d", got, want) + } + if got, want := sessionsCreated[0].User, tt.matchingUser; !reflect.DeepEqual(got, want) { + t.Errorf("user=%+v, want=%+v", got, want) } }) } diff --git a/handlers/db_dev.go b/handlers/db_dev.go index 12ef6aed..1b09ecfe 100644 --- a/handlers/db_dev.go +++ b/handlers/db_dev.go @@ -32,6 +32,12 @@ func (s Server) populateDummyData() http.HandlerFunc { IsAdmin: false, Email: screenjournal.Email("userA@example.com"), }, + { + Username: screenjournal.Username("userB"), + PasswordHash: screenjournal.NewPasswordHash(screenjournal.Password("password456")), + IsAdmin: false, + Email: screenjournal.Email("userB@example.com"), + }, } return func(w http.ResponseWriter, r *http.Request) { diff --git a/handlers/reviews.go b/handlers/reviews.go index 9783c934..3bea314a 100644 --- a/handlers/reviews.go +++ b/handlers/reviews.go @@ -54,6 +54,12 @@ func (s Server) reviewsPost() http.HandlerFunc { func (s Server) reviewsPut() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { + loggedInUser, ok := userFromContext(r.Context()) + if !ok { + http.Error(w, "You must be logged in to edit reviews", http.StatusUnauthorized) + return + } + id, err := reviewIDFromRequestPath(r) if err != nil { http.Error(w, "Invalid review ID", http.StatusBadRequest) @@ -69,6 +75,11 @@ func (s Server) reviewsPut() http.HandlerFunc { return } + if !review.Owner.Equal(loggedInUser.Username) { + http.Error(w, "You can't edit another user's review", http.StatusForbidden) + return + } + if err := updateReviewFromRequest(r, &review); err != nil { http.Error(w, fmt.Sprintf("Invalid request: %v", err), http.StatusBadRequest) return diff --git a/handlers/reviews_test.go b/handlers/reviews_test.go index a325e61b..4d3af968 100644 --- a/handlers/reviews_test.go +++ b/handlers/reviews_test.go @@ -1,6 +1,7 @@ package handlers_test import ( + "errors" "fmt" "log" "net/http" @@ -17,6 +18,7 @@ import ( "github.com/mtlynch/screenjournal/v2/handlers/parse" "github.com/mtlynch/screenjournal/v2/handlers/sessions" "github.com/mtlynch/screenjournal/v2/metadata" + "github.com/mtlynch/screenjournal/v2/random" "github.com/mtlynch/screenjournal/v2/store" "github.com/mtlynch/screenjournal/v2/store/test_sqlite" ) @@ -27,21 +29,47 @@ func (a mockAuthenticator) Authenticate(screenjournal.Username, screenjournal.Pa return screenjournal.User{}, nil } +type mockSession struct { + token string + session sessions.Session +} + type mockSessionManager struct { - lastSession screenjournal.User + sessions map[string]sessions.Session +} + +func newMockSessionManager(mockSessions []mockSession) mockSessionManager { + sessions := make(map[string]sessions.Session, len(mockSessions)) + for _, ms := range mockSessions { + sessions[ms.token] = ms.session + } + return mockSessionManager{ + sessions: sessions, + } } func (sm *mockSessionManager) CreateSession(w http.ResponseWriter, r *http.Request, user screenjournal.User) error { - sm.lastSession = user + token := random.String(10, []rune("abcdefghijklmnopqrstuvwxyz0123456789")) + sm.sessions[token] = sessions.Session{ + User: user, + } + http.SetCookie(w, &http.Cookie{ + Name: "token", + Value: token, + }) return nil } -func (sm mockSessionManager) SessionFromRequest(*http.Request) (sessions.Session, error) { - return sessions.Session{ - User: screenjournal.User{ - Username: screenjournal.Username("dummyadmin"), - }, - }, nil +func (sm mockSessionManager) SessionFromRequest(r *http.Request) (sessions.Session, error) { + token, err := r.Cookie("token") + if err != nil { + return sessions.Session{}, errors.New("mock session manager: no token cookie found") + } + session, ok := sm.sessions[token.Value] + if !ok { + return sessions.Session{}, errors.New("mock session manager: no session associated with token") + } + return session, nil } func (sm mockSessionManager) EndSession(*http.Request, http.ResponseWriter) {} @@ -79,8 +107,10 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { for _, tt := range []struct { description string payload string + sessionToken string localMovies []screenjournal.Movie remoteMovieInfo []metadata.MovieInfo + sessions []mockSession expected screenjournal.Review }{ { @@ -91,6 +121,7 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { "watched":"2022-10-28T00:00:00-04:00", "blurb": "It's my favorite movie!" }`, + sessionToken: "abc123", localMovies: []screenjournal.Movie{ { TmdbID: screenjournal.TmdbID(38), @@ -99,6 +130,16 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("dummyadmin"), + }, + }, + }, + }, expected: screenjournal.Review{ Owner: screenjournal.Username("dummyadmin"), Rating: screenjournal.Rating(10), @@ -121,6 +162,7 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { "watched":"2022-10-21T00:00:00-04:00", "blurb": "" }`, + sessionToken: "abc123", localMovies: []screenjournal.Movie{ { ID: screenjournal.MovieID(1), @@ -130,6 +172,16 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { ReleaseDate: screenjournal.ReleaseDate(mustParseDate("1998-06-12")), }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("dummyadmin"), + }, + }, + }, + }, expected: screenjournal.Review{ Owner: screenjournal.Username("dummyadmin"), Rating: screenjournal.Rating(9), @@ -152,6 +204,7 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { "watched":"2022-10-28T00:00:00-04:00", "blurb": "It's my favorite movie!" }`, + sessionToken: "abc123", remoteMovieInfo: []metadata.MovieInfo{ { TmdbID: screenjournal.TmdbID(38), @@ -160,6 +213,16 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("dummyadmin"), + }, + }, + }, + }, expected: screenjournal.Review{ Owner: screenjournal.Username("dummyadmin"), Rating: screenjournal.Rating(10), @@ -184,13 +247,19 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { } } - s := handlers.New(mockAuthenticator{}, &mockSessionManager{}, dataStore, NewMockMetadataFinder(tt.remoteMovieInfo)) + sessionManager := newMockSessionManager(tt.sessions) + + s := handlers.New(mockAuthenticator{}, &sessionManager, dataStore, NewMockMetadataFinder(tt.remoteMovieInfo)) req, err := http.NewRequest("POST", "/api/reviews", strings.NewReader(tt.payload)) if err != nil { t.Fatal(err) } req.Header.Add("Content-Type", "text/json") + req.AddCookie(&http.Cookie{ + Name: "token", + Value: tt.sessionToken, + }) w := httptest.NewRecorder() s.Router().ServeHTTP(w, req) @@ -220,16 +289,40 @@ func TestReviewsPostAcceptsValidRequest(t *testing.T) { func TestReviewsPostRejectsInvalidRequest(t *testing.T) { for _, tt := range []struct { - description string - payload string + description string + payload string + sessionToken string + sessions []mockSession }{ { - description: "empty string", - payload: "", + description: "empty string", + payload: "", + sessionToken: "abc123", + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, }, { - description: "empty payload", - payload: "{}", + description: "empty payload", + payload: "{}", + sessionToken: "abc123", + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, }, { description: "invalid title field (non-string)", @@ -239,6 +332,17 @@ func TestReviewsPostRejectsInvalidRequest(t *testing.T) { "watched":"2022-10-28T00:00:00-04:00", "blurb": "It's my favorite movie!" }`, + sessionToken: "abc123", + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, }, { description: "invalid title field (too long)", @@ -248,18 +352,34 @@ func TestReviewsPostRejectsInvalidRequest(t *testing.T) { "watched":"2022-10-28T00:00:00-04:00", "blurb": "It's my favorite movie!" }`, strings.Repeat("A", parse.MediaTitleMaxLength+1)), + sessionToken: "abc123", + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, }, } { t.Run(tt.description, func(t *testing.T) { dataStore := test_sqlite.New() - s := handlers.New(mockAuthenticator{}, &mockSessionManager{}, dataStore, mockMetadataFinder{}) + sessionManager := newMockSessionManager(tt.sessions) + s := handlers.New(mockAuthenticator{}, &sessionManager, dataStore, mockMetadataFinder{}) req, err := http.NewRequest("POST", "/api/reviews", strings.NewReader(tt.payload)) if err != nil { t.Fatal(err) } req.Header.Add("Content-Type", "text/json") + req.AddCookie(&http.Cookie{ + Name: "token", + Value: tt.sessionToken, + }) w := httptest.NewRecorder() s.Router().ServeHTTP(w, req) @@ -276,8 +396,10 @@ func TestReviewsPutAcceptsValidRequest(t *testing.T) { description string localMovies []screenjournal.Movie priorReviews []screenjournal.Review + sessions []mockSession route string payload string + sessionToken string expected screenjournal.Review }{ { @@ -299,6 +421,7 @@ func TestReviewsPutAcceptsValidRequest(t *testing.T) { priorReviews: []screenjournal.Review{ { ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -311,13 +434,26 @@ func TestReviewsPutAcceptsValidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/1", payload: `{ "rating": 8, "watched":"2022-10-30T00:00:00-04:00", "blurb": "It's a pretty good movie!" }`, + sessionToken: "abc123", expected: screenjournal.Review{ + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(8), Watched: mustParseWatchDate("2022-10-30T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's a pretty good movie!"), @@ -349,6 +485,7 @@ func TestReviewsPutAcceptsValidRequest(t *testing.T) { priorReviews: []screenjournal.Review{ { ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(9), Watched: mustParseWatchDate("2022-10-21T00:00:00-04:00"), Blurb: screenjournal.Blurb("Love Norm McDonald!"), @@ -361,13 +498,26 @@ func TestReviewsPutAcceptsValidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/1", payload: `{ "rating": 5, "watched":"2022-10-28T00:00:00-04:00", "blurb": "" }`, + sessionToken: "abc123", expected: screenjournal.Review{ + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(5), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb(""), @@ -396,13 +546,18 @@ func TestReviewsPutAcceptsValidRequest(t *testing.T) { } } - s := handlers.New(mockAuthenticator{}, &mockSessionManager{}, dataStore, mockMetadataFinder{}) + sessionManager := newMockSessionManager(tt.sessions) + s := handlers.New(mockAuthenticator{}, &sessionManager, dataStore, mockMetadataFinder{}) req, err := http.NewRequest("PUT", tt.route, strings.NewReader(tt.payload)) if err != nil { t.Fatal(err) } req.Header.Add("Content-Type", "text/json") + req.AddCookie(&http.Cookie{ + Name: "token", + Value: tt.sessionToken, + }) w := httptest.NewRecorder() s.Router().ServeHTTP(w, req) @@ -433,8 +588,10 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { description string localMovies []screenjournal.Movie priorReviews []screenjournal.Review + sessions []mockSession route string payload string + sessionToken string status int }{ { @@ -449,6 +606,8 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, priorReviews: []screenjournal.Review{ { + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -461,13 +620,24 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/0", payload: `{ "rating": 8, "watched":"2022-10-30T00:00:00-04:00", "blurb": "It's a pretty good movie!" }`, - status: http.StatusBadRequest, + sessionToken: "abc123", + status: http.StatusBadRequest, }, { description: "rejects request with non-existent review ID", @@ -481,6 +651,7 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, priorReviews: []screenjournal.Review{ { + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -493,13 +664,24 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/9876", payload: `{ "rating": 8, "watched":"2022-10-30T00:00:00-04:00", "blurb": "It's a pretty good movie!" }`, - status: http.StatusNotFound, + sessionToken: "abc123", + status: http.StatusNotFound, }, { description: "rejects request with malformed JSON", @@ -513,6 +695,8 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, priorReviews: []screenjournal.Review{ { + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -525,12 +709,23 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/1", payload: `{ "rating": 8, "watched":"2022-10-30T00:00:00-04:00", "blurb": "no JSON ending brace!"`, - status: http.StatusBadRequest, + sessionToken: "abc123", + status: http.StatusBadRequest, }, { description: "rejects request with missing rating field", @@ -544,6 +739,8 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, priorReviews: []screenjournal.Review{ { + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -556,12 +753,23 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/1", payload: `{ "watched":"2022-10-30T00:00:00-04:00", "blurb": "It's a pretty good movie!" }`, - status: http.StatusBadRequest, + sessionToken: "abc123", + status: http.StatusBadRequest, }, { description: "rejects request with missing watched field", @@ -575,6 +783,8 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, priorReviews: []screenjournal.Review{ { + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -587,12 +797,23 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/1", payload: `{ "rating": 8, "blurb": "It's a pretty good movie!" }`, - status: http.StatusBadRequest, + sessionToken: "abc123", + status: http.StatusBadRequest, }, { description: "rejects request with numeric blurb field", @@ -606,6 +827,8 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, priorReviews: []screenjournal.Review{ { + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -618,13 +841,24 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/1", payload: `{ "rating": 8, "watched":"2022-10-30T00:00:00-04:00", "blurb": 6 }`, - status: http.StatusBadRequest, + sessionToken: "abc123", + status: http.StatusBadRequest, }, { description: "rejects request with script tag in blurb field", @@ -638,6 +872,8 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, priorReviews: []screenjournal.Review{ { + ID: screenjournal.ReviewID(1), + Owner: screenjournal.Username("userA"), Rating: screenjournal.Rating(10), Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), Blurb: screenjournal.Blurb("It's my favorite movie!"), @@ -650,13 +886,77 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { }, }, }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + }, route: "/api/reviews/1", payload: `{ "rating": 8, "watched":"2022-10-30T00:00:00-04:00", "blurb": "Nothing evil going on here..." }`, - status: http.StatusBadRequest, + sessionToken: "abc123", + status: http.StatusBadRequest, + }, + { + description: "prevents a user from overwriting 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.Rating(10), + Watched: mustParseWatchDate("2022-10-28T00:00:00-04:00"), + Blurb: screenjournal.Blurb("It's my favorite movie!"), + Movie: screenjournal.Movie{ + ID: screenjournal.MovieID(1), + TmdbID: screenjournal.TmdbID(38), + ImdbID: screenjournal.ImdbID("tt0338013"), + Title: "Eternal Sunshine of the Spotless Mind", + ReleaseDate: screenjournal.ReleaseDate(mustParseDate("2004-03-19")), + }, + }, + }, + sessions: []mockSession{ + { + token: "abc123", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userA"), + }, + }, + }, + { + token: "def456", + session: sessions.Session{ + User: screenjournal.User{ + Username: screenjournal.Username("userB"), + }, + }, + }, + }, + route: "/api/reviews/1", + payload: `{ + "rating": 8, + "watched":"2022-10-30T00:00:00-04:00", + "blurb": "I'm overwriting userA's review!" + }`, + sessionToken: "def456", + status: http.StatusForbidden, }, } { t.Run(tt.description, func(t *testing.T) { @@ -674,13 +974,19 @@ func TestReviewsPutRejectsInvalidRequest(t *testing.T) { } } - s := handlers.New(mockAuthenticator{}, &mockSessionManager{}, dataStore, mockMetadataFinder{}) + sessionManager := newMockSessionManager(tt.sessions) + + s := handlers.New(mockAuthenticator{}, &sessionManager, dataStore, mockMetadataFinder{}) req, err := http.NewRequest("PUT", tt.route, strings.NewReader(tt.payload)) if err != nil { t.Fatal(err) } req.Header.Add("Content-Type", "text/json") + req.AddCookie(&http.Cookie{ + Name: "token", + Value: tt.sessionToken, + }) w := httptest.NewRecorder() s.Router().ServeHTTP(w, req) diff --git a/handlers/users_test.go b/handlers/users_test.go index 8e51032b..8380869c 100644 --- a/handlers/users_test.go +++ b/handlers/users_test.go @@ -162,7 +162,7 @@ func TestUsersPut(t *testing.T) { authenticator := simple.New(dataStore) var nilMetadataFinder metadata.Finder - sessionManager := mockSessionManager{} + sessionManager := newMockSessionManager([]mockSession{}) s := handlers.New(authenticator, &sessionManager, dataStore, nilMetadataFinder) diff --git a/handlers/views.go b/handlers/views.go index dd9ef12e..86319ed3 100644 --- a/handlers/views.go +++ b/handlers/views.go @@ -245,6 +245,12 @@ func (s Server) reviewsReadGet() http.HandlerFunc { func (s Server) reviewsEditGet() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { + loggedInUser, ok := userFromContext(r.Context()) + if !ok { + http.Error(w, "You must be logged in to edit reviews", http.StatusUnauthorized) + return + } + id, err := reviewIDFromRequestPath(r) if err != nil { http.Error(w, "Invalid review ID", http.StatusBadRequest) @@ -261,6 +267,11 @@ func (s Server) reviewsEditGet() http.HandlerFunc { return } + if !review.Owner.Equal(loggedInUser.Username) { + http.Error(w, "You can't edit another user's review", http.StatusForbidden) + return + } + if err := renderTemplate(w, "reviews-edit.html", struct { commonProps RatingOptions []int