From ff7e894e4c57bb6a6b1acae46da1168467297f98 Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Sat, 17 Jun 2023 14:44:55 -0400 Subject: [PATCH] Add more tests, change endpoint --- server/server.go | 6 +- server/server_account.go | 2 +- server/server_webpush_test.go | 14 +-- server/webpush_store.go | 13 ++- server/webpush_store_test.go | 160 ++++++++++++++++++++++++++++++++-- web/src/app/Api.js | 6 +- web/src/app/utils.js | 2 +- 7 files changed, 176 insertions(+), 27 deletions(-) diff --git a/server/server.go b/server/server.go index d053b3d3..6da60112 100644 --- a/server/server.go +++ b/server/server.go @@ -85,6 +85,7 @@ var ( metricsPath = "/metrics" apiHealthPath = "/v1/health" apiStatsPath = "/v1/stats" + apiWebPushPath = "/v1/webpush" apiTiersPath = "/v1/tiers" apiUsersPath = "/v1/users" apiUsersAccessPath = "/v1/users/access" @@ -94,7 +95,6 @@ var ( apiAccountSettingsPath = "/v1/account/settings" apiAccountSubscriptionPath = "/v1/account/subscription" apiAccountReservationPath = "/v1/account/reservation" - apiAccountWebPushPath = "/v1/account/webpush" apiAccountPhonePath = "/v1/account/phone" apiAccountPhoneVerifyPath = "/v1/account/phone/verify" apiAccountBillingPortalPath = "/v1/account/billing/portal" @@ -490,9 +490,9 @@ func (s *Server) handleInternal(w http.ResponseWriter, r *http.Request, v *visit return s.ensureUser(s.ensureCallsEnabled(s.withAccountSync(s.handleAccountPhoneNumberAdd)))(w, r, v) } else if r.Method == http.MethodDelete && r.URL.Path == apiAccountPhonePath { return s.ensureUser(s.ensureCallsEnabled(s.withAccountSync(s.handleAccountPhoneNumberDelete)))(w, r, v) - } else if r.Method == http.MethodPost && apiAccountWebPushPath == r.URL.Path { + } else if r.Method == http.MethodPost && apiWebPushPath == r.URL.Path { return s.ensureWebPushEnabled(s.limitRequests(s.handleWebPushUpdate))(w, r, v) - } else if r.Method == http.MethodDelete && apiAccountWebPushPath == r.URL.Path { + } else if r.Method == http.MethodDelete && apiWebPushPath == r.URL.Path { return s.ensureWebPushEnabled(s.limitRequests(s.handleWebPushDelete))(w, r, v) } else if r.Method == http.MethodGet && r.URL.Path == apiStatsPath { return s.handleStats(w, r, v) diff --git a/server/server_account.go b/server/server_account.go index c9ebc702..f26cc2ff 100644 --- a/server/server_account.go +++ b/server/server_account.go @@ -170,7 +170,7 @@ func (s *Server) handleAccountDelete(w http.ResponseWriter, r *http.Request, v * if _, err := s.userManager.Authenticate(u.Name, req.Password); err != nil { return errHTTPBadRequestIncorrectPasswordConfirmation } - if s.webPush != nil { + if s.webPush != nil && u.ID != "" { if err := s.webPush.RemoveSubscriptionsByUserID(u.ID); err != nil { logvr(v, r).Err(err).Warn("Error removing web push subscriptions for %s", u.Name) } diff --git a/server/server_webpush_test.go b/server/server_webpush_test.go index 665156c8..d6b4535d 100644 --- a/server/server_webpush_test.go +++ b/server/server_webpush_test.go @@ -23,7 +23,7 @@ const ( func TestServer_WebPush_TopicAdd(t *testing.T) { s := newTestServer(t, newTestConfigWithWebPush(t)) - response := request(t, s, "POST", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), nil) + response := request(t, s, "POST", "/v1/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), nil) require.Equal(t, 200, response.Code) require.Equal(t, `{"success":true}`+"\n", response.Body.String()) @@ -40,7 +40,7 @@ func TestServer_WebPush_TopicAdd(t *testing.T) { func TestServer_WebPush_TopicAdd_InvalidEndpoint(t *testing.T) { s := newTestServer(t, newTestConfigWithWebPush(t)) - response := request(t, s, "POST", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, "https://ddos-target.example.com/webpush"), nil) + response := request(t, s, "POST", "/v1/webpush", payloadForTopics(t, []string{"test-topic"}, "https://ddos-target.example.com/webpush"), nil) require.Equal(t, 400, response.Code) require.Equal(t, `{"code":40039,"http":400,"error":"invalid request: web push endpoint unknown"}`+"\n", response.Body.String()) } @@ -53,7 +53,7 @@ func TestServer_WebPush_TopicAdd_TooManyTopics(t *testing.T) { topicList[i] = util.RandomString(5) } - response := request(t, s, "POST", "/v1/account/webpush", payloadForTopics(t, topicList, testWebPushEndpoint), nil) + response := request(t, s, "POST", "/v1/webpush", payloadForTopics(t, topicList, testWebPushEndpoint), nil) require.Equal(t, 400, response.Code) require.Equal(t, `{"code":40040,"http":400,"error":"invalid request: too many web push topic subscriptions"}`+"\n", response.Body.String()) } @@ -64,7 +64,7 @@ func TestServer_WebPush_TopicUnsubscribe(t *testing.T) { addSubscription(t, s, testWebPushEndpoint, "test-topic") requireSubscriptionCount(t, s, "test-topic", 1) - response := request(t, s, "POST", "/v1/account/webpush", payloadForTopics(t, []string{}, testWebPushEndpoint), nil) + response := request(t, s, "POST", "/v1/webpush", payloadForTopics(t, []string{}, testWebPushEndpoint), nil) require.Equal(t, 200, response.Code) require.Equal(t, `{"success":true}`+"\n", response.Body.String()) @@ -79,7 +79,7 @@ func TestServer_WebPush_TopicSubscribeProtected_Allowed(t *testing.T) { require.Nil(t, s.userManager.AddUser("ben", "ben", user.RoleUser)) require.Nil(t, s.userManager.AllowAccess("ben", "test-topic", user.PermissionReadWrite)) - response := request(t, s, "POST", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), map[string]string{ + response := request(t, s, "POST", "/v1/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), map[string]string{ "Authorization": util.BasicAuth("ben", "ben"), }) require.Equal(t, 200, response.Code) @@ -96,7 +96,7 @@ func TestServer_WebPush_TopicSubscribeProtected_Denied(t *testing.T) { config.AuthDefault = user.PermissionDenyAll s := newTestServer(t, config) - response := request(t, s, "POST", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), nil) + response := request(t, s, "POST", "/v1/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), nil) require.Equal(t, 403, response.Code) requireSubscriptionCount(t, s, "test-topic", 0) @@ -109,7 +109,7 @@ func TestServer_WebPush_DeleteAccountUnsubscribe(t *testing.T) { require.Nil(t, s.userManager.AddUser("ben", "ben", user.RoleUser)) require.Nil(t, s.userManager.AllowAccess("ben", "test-topic", user.PermissionReadWrite)) - response := request(t, s, "POST", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), map[string]string{ + response := request(t, s, "POST", "/v1/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), map[string]string{ "Authorization": util.BasicAuth("ben", "ben"), }) diff --git a/server/webpush_store.go b/server/webpush_store.go index d2b7ef27..42a2ee67 100644 --- a/server/webpush_store.go +++ b/server/webpush_store.go @@ -11,14 +11,15 @@ import ( ) const ( - subscriptionIDPrefix = "wps_" - subscriptionIDLength = 10 - subscriptionLimitPerSubscriberIP = 10 + subscriptionIDPrefix = "wps_" + subscriptionIDLength = 10 + subscriptionEndpointLimitPerSubscriberIP = 10 ) var ( errWebPushNoRows = errors.New("no rows found") errWebPushTooManySubscriptions = errors.New("too many subscriptions") + errWebPushUserIDCannotBeEmpty = errors.New("user ID cannot be empty") ) const ( @@ -60,6 +61,7 @@ const ( FROM subscription_topic st JOIN subscription s ON s.id = st.subscription_id WHERE st.topic = ? + ORDER BY endpoint ` selectWebPushSubscriptionsExpiringSoonQuery = `SELECT id, endpoint, key_auth, key_p256dh, user_id FROM subscription WHERE warned_at = 0 AND updated_at <= ?` insertWebPushSubscriptionQuery = ` @@ -164,7 +166,7 @@ func (c *webPushStore) UpsertSubscription(endpoint string, auth, p256dh, userID return err } } else { - if subscriptionCount >= subscriptionLimitPerSubscriberIP { + if subscriptionCount >= subscriptionEndpointLimitPerSubscriberIP { return errWebPushTooManySubscriptions } subscriptionID = util.RandomStringPrefix(subscriptionIDPrefix, subscriptionIDLength) @@ -250,6 +252,9 @@ func (c *webPushStore) RemoveSubscriptionsByEndpoint(endpoint string) error { // RemoveSubscriptionsByUserID removes all subscriptions for the given user ID func (c *webPushStore) RemoveSubscriptionsByUserID(userID string) error { + if userID == "" { + return errWebPushUserIDCannotBeEmpty + } _, err := c.db.Exec(deleteWebPushSubscriptionByUserIDQuery, userID) return err } diff --git a/server/webpush_store_test.go b/server/webpush_store_test.go index 03951a07..e96603fd 100644 --- a/server/webpush_store_test.go +++ b/server/webpush_store_test.go @@ -6,16 +6,11 @@ import ( "net/netip" "path/filepath" "testing" + "time" ) -func newTestWebPushStore(t *testing.T, filename string) *webPushStore { - webPush, err := newWebPushStore(filename) - require.Nil(t, err) - return webPush -} - func TestWebPushStore_UpsertSubscription_SubscriptionsForTopic(t *testing.T) { - webPush := newTestWebPushStore(t, filepath.Join(t.TempDir(), "webpush.db")) + webPush := newTestWebPushStore(t) defer webPush.Close() require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint, "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"test-topic", "mytopic"})) @@ -35,7 +30,7 @@ func TestWebPushStore_UpsertSubscription_SubscriptionsForTopic(t *testing.T) { } func TestWebPushStore_UpsertSubscription_SubscriberIPLimitReached(t *testing.T) { - webPush := newTestWebPushStore(t, filepath.Join(t.TempDir(), "webpush.db")) + webPush := newTestWebPushStore(t) defer webPush.Close() // Insert 10 subscriptions with the same IP address @@ -53,3 +48,152 @@ func TestWebPushStore_UpsertSubscription_SubscriberIPLimitReached(t *testing.T) // But with a different IP address it should be fine again require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint+"99", "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("9.9.9.9"), []string{"test-topic", "mytopic"})) } + +func TestWebPushStore_UpsertSubscription_UpdateTopics(t *testing.T) { + webPush := newTestWebPushStore(t) + defer webPush.Close() + + // Insert subscription with two topics, and another with one topic + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint+"0", "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"topic1", "topic2"})) + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint+"1", "auth-key", "p256dh-key", "", netip.MustParseAddr("9.9.9.9"), []string{"topic1"})) + + subs, err := webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 2) + require.Equal(t, testWebPushEndpoint+"0", subs[0].Endpoint) + require.Equal(t, testWebPushEndpoint+"1", subs[1].Endpoint) + + subs, err = webPush.SubscriptionsForTopic("topic2") + require.Nil(t, err) + require.Len(t, subs, 1) + require.Equal(t, testWebPushEndpoint+"0", subs[0].Endpoint) + + // Update the first subscription to have only one topic + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint+"0", "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"topic1"})) + + subs, err = webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 2) + require.Equal(t, testWebPushEndpoint+"0", subs[0].Endpoint) + + subs, err = webPush.SubscriptionsForTopic("topic2") + require.Nil(t, err) + require.Len(t, subs, 0) +} + +func TestWebPushStore_RemoveSubscriptionsByEndpoint(t *testing.T) { + webPush := newTestWebPushStore(t) + defer webPush.Close() + + // Insert subscription with two topics + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint, "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"topic1", "topic2"})) + subs, err := webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 1) + + // And remove it again + require.Nil(t, webPush.RemoveSubscriptionsByEndpoint(testWebPushEndpoint)) + subs, err = webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 0) +} + +func TestWebPushStore_RemoveSubscriptionsByUserID(t *testing.T) { + webPush := newTestWebPushStore(t) + defer webPush.Close() + + // Insert subscription with two topics + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint, "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"topic1", "topic2"})) + subs, err := webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 1) + + // And remove it again + require.Nil(t, webPush.RemoveSubscriptionsByUserID("u_1234")) + subs, err = webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 0) +} + +func TestWebPushStore_RemoveSubscriptionsByUserID_Empty(t *testing.T) { + webPush := newTestWebPushStore(t) + defer webPush.Close() + require.Equal(t, errWebPushUserIDCannotBeEmpty, webPush.RemoveSubscriptionsByUserID("")) +} + +func TestWebPushStore_MarkExpiryWarningSent(t *testing.T) { + webPush := newTestWebPushStore(t) + defer webPush.Close() + + // Insert subscription with two topics + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint, "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"topic1", "topic2"})) + subs, err := webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 1) + + // Mark them as warning sent + require.Nil(t, webPush.MarkExpiryWarningSent(subs)) + + rows, err := webPush.db.Query("SELECT endpoint FROM subscription WHERE warned_at > 0") + require.Nil(t, err) + defer rows.Close() + var endpoint string + require.True(t, rows.Next()) + require.Nil(t, rows.Scan(&endpoint)) + require.Nil(t, err) + require.Equal(t, testWebPushEndpoint, endpoint) + require.False(t, rows.Next()) +} + +func TestWebPushStore_SubscriptionsExpiring(t *testing.T) { + webPush := newTestWebPushStore(t) + defer webPush.Close() + + // Insert subscription with two topics + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint, "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"topic1", "topic2"})) + subs, err := webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 1) + + // Fake-mark them as soon-to-expire + _, err = webPush.db.Exec("UPDATE subscription SET updated_at = ? WHERE endpoint = ?", time.Now().Add(-8*24*time.Hour).Unix(), testWebPushEndpoint) + require.Nil(t, err) + + // Should not be cleaned up yet + require.Nil(t, webPush.RemoveExpiredSubscriptions(9*24*time.Hour)) + + // Run expiration + subs, err = webPush.SubscriptionsExpiring(7 * 24 * time.Hour) + require.Nil(t, err) + require.Len(t, subs, 1) + require.Equal(t, testWebPushEndpoint, subs[0].Endpoint) +} + +func TestWebPushStore_RemoveExpiredSubscriptions(t *testing.T) { + webPush := newTestWebPushStore(t) + defer webPush.Close() + + // Insert subscription with two topics + require.Nil(t, webPush.UpsertSubscription(testWebPushEndpoint, "auth-key", "p256dh-key", "u_1234", netip.MustParseAddr("1.2.3.4"), []string{"topic1", "topic2"})) + subs, err := webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 1) + + // Fake-mark them as expired + _, err = webPush.db.Exec("UPDATE subscription SET updated_at = ? WHERE endpoint = ?", time.Now().Add(-10*24*time.Hour).Unix(), testWebPushEndpoint) + require.Nil(t, err) + + // Run expiration + require.Nil(t, webPush.RemoveExpiredSubscriptions(9*24*time.Hour)) + + // List again, should be 0 + subs, err = webPush.SubscriptionsForTopic("topic1") + require.Nil(t, err) + require.Len(t, subs, 0) +} + +func newTestWebPushStore(t *testing.T) *webPushStore { + webPush, err := newWebPushStore(filepath.Join(t.TempDir(), "webpush.db")) + require.Nil(t, err) + return webPush +} diff --git a/web/src/app/Api.js b/web/src/app/Api.js index afe59c7c..b2bfd06f 100644 --- a/web/src/app/Api.js +++ b/web/src/app/Api.js @@ -6,7 +6,7 @@ import { topicUrlAuth, topicUrlJsonPoll, topicUrlJsonPollWithSince, - accountWebPushUrl, + webPushUrl, } from "./utils"; import userManager from "./UserManager"; import { fetchOrThrow } from "./errors"; @@ -117,7 +117,7 @@ class Api { async updateWebPush(pushSubscription, topics) { const user = await userManager.get(config.base_url); - const url = accountWebPushUrl(config.base_url); + const url = webPushUrl(config.base_url); console.log(`[Api] Updating Web Push subscription`, { url, topics, endpoint: pushSubscription.endpoint }); const serializedSubscription = JSON.parse(JSON.stringify(pushSubscription)); // Ugh ... https://stackoverflow.com/a/40525434/1440785 await fetchOrThrow(url, { @@ -134,7 +134,7 @@ class Api { async deleteWebPush(pushSubscription) { const user = await userManager.get(config.base_url); - const url = accountWebPushUrl(config.base_url); + const url = webPushUrl(config.base_url); console.log(`[Api] Deleting Web Push subscription`, { url, endpoint: pushSubscription.endpoint }); await fetchOrThrow(url, { method: "DELETE", diff --git a/web/src/app/utils.js b/web/src/app/utils.js index 8a1026d7..244d3321 100644 --- a/web/src/app/utils.js +++ b/web/src/app/utils.js @@ -21,6 +21,7 @@ export const topicUrlJsonPoll = (baseUrl, topic) => `${topicUrlJson(baseUrl, top export const topicUrlJsonPollWithSince = (baseUrl, topic, since) => `${topicUrlJson(baseUrl, topic)}?poll=1&since=${since}`; export const topicUrlAuth = (baseUrl, topic) => `${topicUrl(baseUrl, topic)}/auth`; export const topicShortUrl = (baseUrl, topic) => shortUrl(topicUrl(baseUrl, topic)); +export const webPushUrl = (baseUrl) => `${baseUrl}/v1/webpush`; export const accountUrl = (baseUrl) => `${baseUrl}/v1/account`; export const accountPasswordUrl = (baseUrl) => `${baseUrl}/v1/account/password`; export const accountTokenUrl = (baseUrl) => `${baseUrl}/v1/account/token`; @@ -32,7 +33,6 @@ export const accountBillingSubscriptionUrl = (baseUrl) => `${baseUrl}/v1/account export const accountBillingPortalUrl = (baseUrl) => `${baseUrl}/v1/account/billing/portal`; export const accountPhoneUrl = (baseUrl) => `${baseUrl}/v1/account/phone`; export const accountPhoneVerifyUrl = (baseUrl) => `${baseUrl}/v1/account/phone/verify`; -export const accountWebPushUrl = (baseUrl) => `${baseUrl}/v1/account/webpush`; export const validUrl = (url) => url.match(/^https?:\/\/.+/);