Enforce ownership on editing reviews (#94)

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");
+}

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();
+});

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)
 			}
 		})
 	}

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) {

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