From 2edead59134748d5724bf3a9dab0d819696c5dc9 Mon Sep 17 00:00:00 2001 From: Alessio Date: Mon, 2 Dec 2024 18:50:54 -0800 Subject: [PATCH] BUGFIX: fix a bunch of issues with HTMX error-response toasts - Add tests for a bunch of error cases and non-happy paths --- internal/webserver/handler_bookmarks.go | 2 +- internal/webserver/handler_bookmarks_test.go | 41 +++++++++++- internal/webserver/handler_lists_test.go | 67 +++++++++++++++++--- internal/webserver/handler_messages.go | 17 ++++- internal/webserver/handler_messages_test.go | 65 +++++++++++++++++++ internal/webserver/handler_notifications.go | 2 +- internal/webserver/handler_search.go | 2 +- internal/webserver/handler_user_feed.go | 6 +- internal/webserver/response_helpers.go | 27 +++++--- internal/webserver/server_test.go | 4 +- 10 files changed, 203 insertions(+), 30 deletions(-) diff --git a/internal/webserver/handler_bookmarks.go b/internal/webserver/handler_bookmarks.go index 8809411..d23069c 100644 --- a/internal/webserver/handler_bookmarks.go +++ b/internal/webserver/handler_bookmarks.go @@ -15,7 +15,7 @@ func (app *Application) Bookmarks(w http.ResponseWriter, r *http.Request) { if r.URL.Query().Has("scrape") { if app.IsScrapingDisabled { app.InfoLog.Printf("Would have scraped: %s", r.URL.Path) - http.Error(w, "Scraping is disabled (are you logged in?)", 401) // TODO: toast + app.error_401(w, r) return } diff --git a/internal/webserver/handler_bookmarks_test.go b/internal/webserver/handler_bookmarks_test.go index 1e84ae9..99a7d6d 100644 --- a/internal/webserver/handler_bookmarks_test.go +++ b/internal/webserver/handler_bookmarks_test.go @@ -23,12 +23,47 @@ func TestBookmarksTab(t *testing.T) { tweets := cascadia.QueryAll(root, selector(".timeline > .tweet")) assert.Len(tweets, 2) - // Double check pagination works properly - resp = do_request_with_active_user(httptest.NewRequest("GET", "/bookmarks?cursor=1800452344077464795", nil)) + // With pagination + req := httptest.NewRequest("GET", "/bookmarks?cursor=1800452344077464795", nil) + req.Header.Set("HX-Request", "true") + resp = do_request_with_active_user(req) require.Equal(resp.StatusCode, 200) root, err = html.Parse(resp.Body) require.NoError(err) - tweets = cascadia.QueryAll(root, selector(".timeline > .tweet")) + tweets = cascadia.QueryAll(root, selector(".tweet")) assert.Len(tweets, 1) } + +// When scraping is disabled, should 401 +func TestBookmarksScrape(t *testing.T) { + require := require.New(t) + + // Attempt to scrape with scraping disabled + resp := do_request_with_active_user(httptest.NewRequest("GET", "/bookmarks?scrape", nil)) + require.Equal(resp.StatusCode, 401) +} + +// If cursor is invalid, it should 400 +func TestBookmarksInvalidCursor(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + + // HTMX version + req := httptest.NewRequest("GET", "/bookmarks?cursor=asdf", nil) + req.Header.Set("HX-Request", "true") + resp := do_request_with_active_user(req) + require.Equal(resp.StatusCode, 400) + // Piggyback in testing of HTMX 400 error toasts + assert.Equal("beforeend", resp.Header.Get("HX-Reswap")) + assert.Equal("#toasts", resp.Header.Get("HX-Retarget")) + assert.Equal("false", resp.Header.Get("HX-Push-Url")) + + // Non-HTMX version + req1 := httptest.NewRequest("GET", "/bookmarks?cursor=asdf", nil) + resp1 := do_request_with_active_user(req1) + require.Equal(resp1.StatusCode, 400) + assert.Equal("", resp1.Header.Get("HX-Reswap")) + assert.Equal("", resp1.Header.Get("HX-Retarget")) + assert.Equal("", resp1.Header.Get("HX-Push-Url")) +} diff --git a/internal/webserver/handler_lists_test.go b/internal/webserver/handler_lists_test.go index fa830f1..bc4eb40 100644 --- a/internal/webserver/handler_lists_test.go +++ b/internal/webserver/handler_lists_test.go @@ -24,23 +24,46 @@ func TestListsIndex(t *testing.T) { assert.True(t, len(cascadia.QueryAll(root, selector(".list-preview"))) >= 2) } -func TestListDetail(t *testing.T) { +// Show the users who are on a List +func TestListDetailUsers(t *testing.T) { require := require.New(t) assert := assert.New(t) - // Users resp := do_request(httptest.NewRequest("GET", "/lists/1/users", nil)) require.Equal(resp.StatusCode, 200) root, err := html.Parse(resp.Body) require.NoError(err) assert.Len(cascadia.QueryAll(root, selector(".users-list .author-info")), 5) +} - // Feed - resp1 := do_request(httptest.NewRequest("GET", "/lists/2", nil)) - require.Equal(resp1.StatusCode, 200) - root1, err := html.Parse(resp1.Body) +// Show the timeline geenrated for a List +func TestListFeed(t *testing.T) { + require := require.New(t) + assert := assert.New(t) + + resp := do_request(httptest.NewRequest("GET", "/lists/2", nil)) + require.Equal(resp.StatusCode, 200) + root, err := html.Parse(resp.Body) require.NoError(err) - assert.Len(cascadia.QueryAll(root1, selector(".timeline > .tweet")), 3) + assert.Len(cascadia.QueryAll(root, selector(".timeline > .tweet")), 3) + + // With pagination + req1 := httptest.NewRequest("GET", "/lists/2?cursor=1629523652000", nil) + req1.Header.Set("HX-Request", "true") + resp1 := do_request(req1) + require.Equal(resp1.StatusCode, 200) + root2, err := html.Parse(resp1.Body) + require.NoError(err) + assert.Len(cascadia.QueryAll(root2, selector(":not(.tweet__quoted-tweet) > .tweet")), 2) +} + +func TestListFeedInvalidCursor(t *testing.T) { + require := require.New(t) + + req := httptest.NewRequest("GET", "/lists/2?cursor=asdf", nil) + req.Header.Set("HX-Request", "true") + resp := do_request(req) + require.Equal(resp.StatusCode, 400) } func TestListDetailDoesntExist(t *testing.T) { @@ -89,7 +112,23 @@ func TestListAddAndDeleteUser(t *testing.T) { assert.Len(cascadia.QueryAll(root, selector(".users-list .author-info")), 2) } -func TestCreateNewList(t *testing.T) { +// Adding invalid users should 400 +func TestListAddInvalidUser(t *testing.T) { + require := require.New(t) + + resp := do_request(httptest.NewRequest("GET", "/lists/2/add_user?user_handle=jkwfjekj", nil)) + require.Equal(resp.StatusCode, 400) +} + +// Deleting invalid users should 400 +func TestListRemoveInvalidUser(t *testing.T) { + require := require.New(t) + + resp := do_request(httptest.NewRequest("GET", "/lists/2/remove_user?user_handle=fwefjkl", nil)) + require.Equal(resp.StatusCode, 400) +} + +func TestCreateNewListThenDelete(t *testing.T) { require := require.New(t) assert := assert.New(t) @@ -111,4 +150,16 @@ func TestCreateNewList(t *testing.T) { root, err = html.Parse(resp.Body) require.NoError(err) assert.Len(cascadia.QueryAll(root, selector(".list-preview")), num_lists+1) + + // Delete it; should redirect back to Lists index page + resp_delete := do_request(httptest.NewRequest("DELETE", fmt.Sprintf("/lists/%d", num_lists+1), nil)) + require.Equal(resp_delete.StatusCode, 302) + require.Equal("/lists", resp_delete.Header.Get("Location")) + + // Should be N lists again + resp = do_request(httptest.NewRequest("GET", "/lists", nil)) + require.Equal(resp.StatusCode, 200) + root, err = html.Parse(resp.Body) + require.NoError(err) + assert.Len(cascadia.QueryAll(root, selector(".list-preview")), num_lists) } diff --git a/internal/webserver/handler_messages.go b/internal/webserver/handler_messages.go index efc0b65..d92572b 100644 --- a/internal/webserver/handler_messages.go +++ b/internal/webserver/handler_messages.go @@ -33,6 +33,11 @@ func (app *Application) message_mark_as_read(w http.ResponseWriter, r *http.Requ c.PageSize = 1 chat_contents := app.Profile.GetChatRoomMessagesByCursor(c) last_message_id := chat_contents.MessageIDs[len(chat_contents.MessageIDs)-1] + if app.IsScrapingDisabled { + app.InfoLog.Printf("Would have scraped: %s", r.URL.Path) + app.error_401(w, r) + return + } panic_if(app.API.MarkDMChatRead(room_id, last_message_id)) room := chat_contents.Rooms[room_id] participant, is_ok := room.Participants[app.ActiveUser.ID] @@ -42,7 +47,7 @@ func (app *Application) message_mark_as_read(w http.ResponseWriter, r *http.Requ participant.LastReadEventID = last_message_id room.Participants[app.ActiveUser.ID] = participant panic_if(app.Profile.SaveChatRoom(room)) - app.toast(w, r, Toast{ + app.toast(w, r, 200, Toast{ Title: "Success", Message: `Conversation marked as "read"`, Type: "success", @@ -65,7 +70,11 @@ func (app *Application) message_send(w http.ResponseWriter, r *http.Request) { if err != nil { in_reply_to_id = 0 } - + if app.IsScrapingDisabled { + app.InfoLog.Printf("Would have scraped: %s", r.URL.Path) + app.error_401(w, r) + return + } trove, err := app.API.SendDMMessage(room_id, message_data.Text, scraper.DMMessageID(in_reply_to_id)) if err != nil { panic(err) @@ -81,6 +90,10 @@ func (app *Application) message_detail(w http.ResponseWriter, r *http.Request) { is_sending := len(parts) == 1 && parts[0] == "send" chat_view_data, global_data := app.get_message_global_data() + if _, is_ok := chat_view_data.Rooms[room_id]; !is_ok { + app.error_404(w, r) + return + } if len(parts) == 1 && parts[0] == "mark-as-read" { app.message_mark_as_read(w, r) diff --git a/internal/webserver/handler_messages_test.go b/internal/webserver/handler_messages_test.go index ad2f273..fd72b98 100644 --- a/internal/webserver/handler_messages_test.go +++ b/internal/webserver/handler_messages_test.go @@ -2,6 +2,7 @@ package webserver_test import ( "testing" + "strings" "net/http/httptest" @@ -9,8 +10,32 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/html" + + "gitlab.com/offline-twitter/twitter_offline_engine/pkg/scraper" + "gitlab.com/offline-twitter/twitter_offline_engine/internal/webserver" ) +func TestMessagesIndexPageRequiresActiveUser(t *testing.T) { + assert := assert.New(t) + require := require.New(t) + + // HTMX version + req := httptest.NewRequest("GET", "/messages", nil) + req.Header.Set("HX-Request", "true") + resp := do_request(req) // No active user + require.Equal(401, resp.StatusCode) + // Piggyback in testing of HTMX 401 toasts + assert.Equal("beforeend", resp.Header.Get("HX-Reswap")) + assert.Equal("#toasts", resp.Header.Get("HX-Retarget")) + assert.Equal("false", resp.Header.Get("HX-Push-Url")) + + // Non-HTMX version + req1 := httptest.NewRequest("GET", "/messages", nil) + resp1 := do_request(req1) // No active user + require.Equal(401, resp1.StatusCode) + assert.Equal("", resp1.Header.Get("HX-Reswap")) // HX-* stuff should be unset +} + // Loading the index page should work if you're logged in func TestMessagesIndexPage(t *testing.T) { assert := assert.New(t) @@ -18,12 +43,32 @@ func TestMessagesIndexPage(t *testing.T) { // Chat list resp := do_request_with_active_user(httptest.NewRequest("GET", "/messages", nil)) + require.Equal(200, resp.StatusCode) root, err := html.Parse(resp.Body) require.NoError(err) assert.Len(cascadia.QueryAll(root, selector(".chat-list .chat-list-entry")), 2) assert.Len(cascadia.QueryAll(root, selector(".chat-view .dm-message")), 0) // No messages until you click on one } +// Users should only be able to open chats they're a member of +func TestMessagesRoomRequiresCorrectUser(t *testing.T) { + require := require.New(t) + + // No active user + resp := do_request(httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328", nil)) + require.Equal(401, resp.StatusCode) + + // Wrong user (not in the chat) + // Copied from `do_request_with_active_user` + recorder := httptest.NewRecorder() + app := webserver.NewApp(profile) + app.IsScrapingDisabled = true + app.ActiveUser = scraper.User{ID: 782982734, Handle: "Not a real user"} // Simulate a login + app.WithMiddlewares().ServeHTTP(recorder, httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328", nil)) + resp2 := recorder.Result() + require.Equal(404, resp2.StatusCode) +} + // Open a chat room func TestMessagesRoom(t *testing.T) { assert := assert.New(t) @@ -31,6 +76,7 @@ func TestMessagesRoom(t *testing.T) { // Chat detail resp := do_request_with_active_user(httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328", nil)) + require.Equal(200, resp.StatusCode) root, err := html.Parse(resp.Body) require.NoError(err) assert.Len(cascadia.QueryAll(root, selector(".chat-list .chat-list-entry")), 2) // Chat list still renders @@ -59,6 +105,7 @@ func TestMessagesRoomPollForUpdates(t *testing.T) { req := httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328?poll&latest_timestamp=1686025129141", nil) req.Header.Set("HX-Request", "true") resp := do_request_with_active_user(req) + require.Equal(200, resp.StatusCode) root, err := html.Parse(resp.Body) require.NoError(err) assert.Len(cascadia.QueryAll(root, selector(".dm-message")), 3) @@ -86,6 +133,7 @@ func TestMessagesRoomPollForUpdatesEmptyResult(t *testing.T) { req := httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328?poll&latest_timestamp=1686025129144", nil) req.Header.Set("HX-Request", "true") resp := do_request_with_active_user(req) + require.Equal(200, resp.StatusCode) root, err := html.Parse(resp.Body) require.NoError(err) assert.Len(cascadia.QueryAll(root, selector(".dm-message")), 0) @@ -113,7 +161,24 @@ func TestMessagesPaginate(t *testing.T) { req := httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328?cursor=1686025129142", nil) req.Header.Set("HX-Request", "true") resp := do_request_with_active_user(req) + require.Equal(200, resp.StatusCode) root, err := html.Parse(resp.Body) require.NoError(err) assert.Len(cascadia.QueryAll(root, selector(".dm-message")), 2) } + +// When scraping is disabled, marking as read should 401 +func TestMessagesMarkAsRead(t *testing.T) { + require := require.New(t) + + resp := do_request_with_active_user(httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328/mark-as-read", nil)) + require.Equal(resp.StatusCode, 401) +} + +// When scraping is disabled, sending a message should 401 +func TestMessagesSend(t *testing.T) { + require := require.New(t) + + resp := do_request_with_active_user(httptest.NewRequest("GET", "/messages/1488963321701171204-1178839081222115328/send", strings.NewReader(`{"text": "bleh"}`))) + require.Equal(401, resp.StatusCode) +} diff --git a/internal/webserver/handler_notifications.go b/internal/webserver/handler_notifications.go index 1a2c3e0..62602fe 100644 --- a/internal/webserver/handler_notifications.go +++ b/internal/webserver/handler_notifications.go @@ -40,7 +40,7 @@ func (app *Application) NotificationsMarkAsRead(w http.ResponseWriter, r *http.R if err != nil { panic(err) } - app.toast(w, r, Toast{ + app.toast(w, r, 200, Toast{ Title: "Success", Message: `Notifications marked as "read"`, Type: "success", diff --git a/internal/webserver/handler_search.go b/internal/webserver/handler_search.go index 32c9b54..2f5c5b8 100644 --- a/internal/webserver/handler_search.go +++ b/internal/webserver/handler_search.go @@ -98,7 +98,7 @@ func (app *Application) Search(w http.ResponseWriter, r *http.Request) { if r.URL.Query().Has("scrape") { if app.IsScrapingDisabled { app.InfoLog.Printf("Would have scraped: %s", r.URL.Path) - http.Error(w, "Scraping is disabled (are you logged in?)", 401) + app.error_401(w, r) return } diff --git a/internal/webserver/handler_user_feed.go b/internal/webserver/handler_user_feed.go index 9eb411e..9ac2f48 100644 --- a/internal/webserver/handler_user_feed.go +++ b/internal/webserver/handler_user_feed.go @@ -42,7 +42,7 @@ func (app *Application) UserFeed(w http.ResponseWriter, r *http.Request) { if r.URL.Query().Has("scrape") { if app.IsScrapingDisabled { app.InfoLog.Printf("Would have scraped: %s", r.URL.Path) - http.Error(w, "Scraping is disabled (are you logged in?)", 401) + app.error_401(w, r) return } @@ -155,7 +155,7 @@ func (app *Application) UserFollowees(w http.ResponseWriter, r *http.Request, us if r.URL.Query().Has("scrape") { if app.IsScrapingDisabled { app.InfoLog.Printf("Would have scraped: %s", r.URL.Path) - http.Error(w, "Scraping is disabled (are you logged in?)", 401) + app.error_401(w, r) return } @@ -181,7 +181,7 @@ func (app *Application) UserFollowers(w http.ResponseWriter, r *http.Request, us if r.URL.Query().Has("scrape") { if app.IsScrapingDisabled { app.InfoLog.Printf("Would have scraped: %s", r.URL.Path) - http.Error(w, "Scraping is disabled (are you logged in?)", 401) + app.error_401(w, r) return } diff --git a/internal/webserver/response_helpers.go b/internal/webserver/response_helpers.go index ac9f21c..8ccc341 100644 --- a/internal/webserver/response_helpers.go +++ b/internal/webserver/response_helpers.go @@ -14,26 +14,27 @@ func panic_if(err error) { func (app *Application) error_400_with_message(w http.ResponseWriter, r *http.Request, msg string) { if is_htmx(r) { - w.WriteHeader(400) - app.toast(w, r, Toast{Title: "Bad Request", Message: msg, Type: "error"}) + app.toast(w, r, 400, Toast{Title: "Bad Request", Message: msg, Type: "error"}) } else { http.Error(w, fmt.Sprintf("Bad Request\n\n%s", msg), 400) } } func (app *Application) error_401(w http.ResponseWriter, r *http.Request) { + msg := "Please log in or set an active session." + if app.ActiveUser.ID != 0 { + msg += " (There is currently an active user, but scraping is disabled.)" + } if is_htmx(r) { - w.WriteHeader(401) - app.toast(w, r, Toast{Title: "Login required", Message: "Please log in or set an active session", Type: "error"}) + app.toast(w, r, 401, Toast{Title: "Login required", Message: msg, Type: "error"}) } else { - http.Error(w, "Please log in or set an active session", 401) + http.Error(w, msg, 401) } } func (app *Application) error_404(w http.ResponseWriter, r *http.Request) { if is_htmx(r) { - w.WriteHeader(404) - app.toast(w, r, Toast{Title: "Not found", Type: "error"}) + app.toast(w, r, 404, Toast{Title: "Not found", Type: "error"}) } else { http.Error(w, "Not Found", 404) } @@ -45,15 +46,23 @@ func (app *Application) error_500(w http.ResponseWriter, r *http.Request, err er if err2 != nil { panic(err2) } - app.toast(w, r, Toast{Title: "Server error", Message: err.Error(), Type: "error"}) + + if is_htmx(r) { + app.toast(w, r, 500, Toast{Title: "Server error", Message: err.Error(), Type: "error"}) + } else { + http.Error(w, err.Error(), 500) + } } -func (app *Application) toast(w http.ResponseWriter, r *http.Request, t Toast) { +// The toast is the primary payload (i.e., not OOB) +func (app *Application) toast(w http.ResponseWriter, r *http.Request, status_code int, t Toast) { // Reset the HTMX response to return an error toast and append it to the Toasts container w.Header().Set("HX-Reswap", "beforeend") w.Header().Set("HX-Retarget", "#toasts") w.Header().Set("HX-Push-Url", "false") + w.WriteHeader(status_code) // Must be called after all `Header.Set(...)` + app.buffered_render_htmx(w, "toast", PageGlobalData{}, t) } diff --git a/internal/webserver/server_test.go b/internal/webserver/server_test.go index fca4d73..bcce349 100644 --- a/internal/webserver/server_test.go +++ b/internal/webserver/server_test.go @@ -46,7 +46,7 @@ func do_request(req *http.Request) *http.Response { recorder := httptest.NewRecorder() app := webserver.NewApp(profile) app.IsScrapingDisabled = true - app.ServeHTTP(recorder, req) + app.WithMiddlewares().ServeHTTP(recorder, req) return recorder.Result() } @@ -56,7 +56,7 @@ func do_request_with_active_user(req *http.Request) *http.Response { app := webserver.NewApp(profile) app.IsScrapingDisabled = true app.ActiveUser = scraper.User{ID: 1488963321701171204, Handle: "Offline_Twatter"} // Simulate a login - app.ServeHTTP(recorder, req) + app.WithMiddlewares().ServeHTTP(recorder, req) return recorder.Result() }