More refactor

This commit is contained in:
binwiederhier 2023-06-08 23:09:38 -04:00
parent 9d38aeb863
commit 966ffe1669
8 changed files with 110 additions and 130 deletions

View file

@ -171,7 +171,7 @@ func (s *Server) handleAccountDelete(w http.ResponseWriter, r *http.Request, v *
return errHTTPBadRequestIncorrectPasswordConfirmation
}
if s.webPush != nil {
if err := s.webPush.RemoveByUserID(u.ID); err != nil {
if err := s.webPush.RemoveSubscriptionsByUserID(u.ID); err != nil {
logvr(v, r).Err(err).Warn("Error removing web push subscriptions for %s", u.Name)
}
}

View file

@ -29,15 +29,15 @@ const (
var webPushEndpointAllowRegex = regexp.MustCompile(webPushEndpointAllowRegexStr)
func (s *Server) handleWebPushUpdate(w http.ResponseWriter, r *http.Request, v *visitor) error {
payload, err := readJSONWithLimit[webPushSubscriptionPayload](r.Body, jsonBodyBytesLimit, false)
if err != nil || payload.BrowserSubscription.Endpoint == "" || payload.BrowserSubscription.Keys.P256dh == "" || payload.BrowserSubscription.Keys.Auth == "" {
req, err := readJSONWithLimit[apiWebPushUpdateSubscriptionRequest](r.Body, jsonBodyBytesLimit, false)
if err != nil || req.Endpoint == "" || req.P256dh == "" || req.Auth == "" {
return errHTTPBadRequestWebPushSubscriptionInvalid
} else if !webPushEndpointAllowRegex.MatchString(payload.BrowserSubscription.Endpoint) {
} else if !webPushEndpointAllowRegex.MatchString(req.Endpoint) {
return errHTTPBadRequestWebPushEndpointUnknown
} else if len(payload.Topics) > webPushTopicSubscribeLimit {
} else if len(req.Topics) > webPushTopicSubscribeLimit {
return errHTTPBadRequestWebPushTopicCountTooHigh
}
topics, err := s.topicsFromIDs(payload.Topics...)
topics, err := s.topicsFromIDs(req.Topics...)
if err != nil {
return err
}
@ -50,7 +50,7 @@ func (s *Server) handleWebPushUpdate(w http.ResponseWriter, r *http.Request, v *
}
}
}
if err := s.webPush.UpdateSubscriptions(payload.Topics, v.MaybeUserID(), payload.BrowserSubscription); err != nil {
if err := s.webPush.UpsertSubscription(req.Endpoint, req.Topics, v.MaybeUserID(), req.Auth, req.P256dh); err != nil {
return err
}
return s.writeJSON(w, newSuccessResponse())
@ -68,16 +68,13 @@ func (s *Server) publishToWebPushEndpoints(v *visitor, m *message) {
return
}
for _, subscription := range subscriptions {
ctx := log.Context{"endpoint": subscription.BrowserSubscription.Endpoint, "username": subscription.UserID, "topic": m.Topic, "message_id": m.ID}
ctx := log.Context{"endpoint": subscription.Endpoint, "username": subscription.UserID, "topic": m.Topic, "message_id": m.ID}
if err := s.sendWebPushNotification(payload, subscription, &ctx); err != nil {
log.Tag(tagWebPush).Err(err).Fields(ctx).Warn("Unable to publish web push message")
}
}
}
// TODO this should return error
// TODO rate limiting
func (s *Server) pruneOrNotifyWebPushSubscriptions() {
if s.config.WebPushPublicKey == "" {
return
@ -103,8 +100,8 @@ func (s *Server) pruneOrNotifyWebPushSubscriptionsInternal() error {
return err
}
for _, subscription := range subscriptions {
ctx := log.Context{"endpoint": subscription.BrowserSubscription.Endpoint}
if err := s.sendWebPushNotification(payload, &subscription, &ctx); err != nil {
ctx := log.Context{"endpoint": subscription.Endpoint}
if err := s.sendWebPushNotification(payload, subscription, &ctx); err != nil {
log.Tag(tagWebPush).Err(err).Fields(ctx).Warn("Unable to publish expiry imminent warning")
return err
}
@ -114,7 +111,7 @@ func (s *Server) pruneOrNotifyWebPushSubscriptionsInternal() error {
}
func (s *Server) sendWebPushNotification(message []byte, sub *webPushSubscription, ctx *log.Context) error {
resp, err := webpush.SendNotification(message, &sub.BrowserSubscription, &webpush.Options{
resp, err := webpush.SendNotification(message, sub.ToSubscription(), &webpush.Options{
Subscriber: s.config.WebPushEmailAddress,
VAPIDPublicKey: s.config.WebPushPublicKey,
VAPIDPrivateKey: s.config.WebPushPrivateKey,
@ -122,14 +119,14 @@ func (s *Server) sendWebPushNotification(message []byte, sub *webPushSubscriptio
})
if err != nil {
log.Tag(tagWebPush).Err(err).Fields(*ctx).Debug("Unable to publish web push message, removing endpoint")
if err := s.webPush.RemoveByEndpoint(sub.BrowserSubscription.Endpoint); err != nil {
if err := s.webPush.RemoveSubscriptionsByEndpoint(sub.Endpoint); err != nil {
return err
}
return err
}
if (resp.StatusCode < 200 || resp.StatusCode > 299) && resp.StatusCode != 429 {
log.Tag(tagWebPush).Fields(*ctx).Field("response_code", resp.StatusCode).Debug("Unable to publish web push message, unexpected response")
if err := s.webPush.RemoveByEndpoint(sub.BrowserSubscription.Endpoint); err != nil {
if err := s.webPush.RemoveSubscriptionsByEndpoint(sub.Endpoint); err != nil {
return err
}
return errHTTPInternalErrorWebPushUnableToPublish.Fields(*ctx)

View file

@ -3,27 +3,25 @@ package server
import (
"encoding/json"
"fmt"
"github.com/stretchr/testify/require"
"heckel.io/ntfy/user"
"heckel.io/ntfy/util"
"io"
"net/http"
"net/http/httptest"
"strings"
"sync/atomic"
"testing"
"github.com/SherClockHolmes/webpush-go"
"github.com/stretchr/testify/require"
"heckel.io/ntfy/user"
"heckel.io/ntfy/util"
)
const (
defaultEndpoint = "https://updates.push.services.mozilla.com/wpush/v1/AAABBCCCDDEEEFFF"
testWebPushEndpoint = "https://updates.push.services.mozilla.com/wpush/v1/AAABBCCCDDEEEFFF"
)
func TestServer_WebPush_TopicAdd(t *testing.T) {
s := newTestServer(t, newTestConfigWithWebPush(t))
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, defaultEndpoint), nil)
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), nil)
require.Equal(t, 200, response.Code)
require.Equal(t, `{"success":true}`+"\n", response.Body.String())
@ -31,9 +29,9 @@ func TestServer_WebPush_TopicAdd(t *testing.T) {
require.Nil(t, err)
require.Len(t, subs, 1)
require.Equal(t, subs[0].BrowserSubscription.Endpoint, defaultEndpoint)
require.Equal(t, subs[0].BrowserSubscription.Keys.P256dh, "p256dh-key")
require.Equal(t, subs[0].BrowserSubscription.Keys.Auth, "auth-key")
require.Equal(t, subs[0].Endpoint, testWebPushEndpoint)
require.Equal(t, subs[0].P256dh, "p256dh-key")
require.Equal(t, subs[0].Auth, "auth-key")
require.Equal(t, subs[0].UserID, "")
}
@ -53,7 +51,7 @@ func TestServer_WebPush_TopicAdd_TooManyTopics(t *testing.T) {
topicList[i] = util.RandomString(5)
}
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, topicList, defaultEndpoint), nil)
response := request(t, s, "PUT", "/v1/account/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())
}
@ -61,10 +59,10 @@ func TestServer_WebPush_TopicAdd_TooManyTopics(t *testing.T) {
func TestServer_WebPush_TopicUnsubscribe(t *testing.T) {
s := newTestServer(t, newTestConfigWithWebPush(t))
addSubscription(t, s, "test-topic", defaultEndpoint)
addSubscription(t, s, testWebPushEndpoint, "test-topic")
requireSubscriptionCount(t, s, "test-topic", 1)
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{}, defaultEndpoint), nil)
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{}, testWebPushEndpoint), nil)
require.Equal(t, 200, response.Code)
require.Equal(t, `{"success":true}`+"\n", response.Body.String())
@ -79,7 +77,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, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, defaultEndpoint), map[string]string{
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), map[string]string{
"Authorization": util.BasicAuth("ben", "ben"),
})
require.Equal(t, 200, response.Code)
@ -96,7 +94,7 @@ func TestServer_WebPush_TopicSubscribeProtected_Denied(t *testing.T) {
config.AuthDefault = user.PermissionDenyAll
s := newTestServer(t, config)
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, defaultEndpoint), nil)
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), nil)
require.Equal(t, 403, response.Code)
requireSubscriptionCount(t, s, "test-topic", 0)
@ -109,7 +107,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, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, defaultEndpoint), map[string]string{
response := request(t, s, "PUT", "/v1/account/webpush", payloadForTopics(t, []string{"test-topic"}, testWebPushEndpoint), map[string]string{
"Authorization": util.BasicAuth("ben", "ben"),
})
@ -129,7 +127,6 @@ func TestServer_WebPush_Publish(t *testing.T) {
s := newTestServer(t, newTestConfigWithWebPush(t))
var received atomic.Bool
pushService := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, err := io.ReadAll(r.Body)
require.Nil(t, err)
@ -140,8 +137,7 @@ func TestServer_WebPush_Publish(t *testing.T) {
}))
defer pushService.Close()
addSubscription(t, s, "test-topic", pushService.URL+"/push-receive")
addSubscription(t, s, pushService.URL+"/push-receive", "test-topic")
request(t, s, "PUT", "/test-topic", "web push test", nil)
waitFor(t, func() bool {
@ -153,19 +149,15 @@ func TestServer_WebPush_Publish_RemoveOnError(t *testing.T) {
s := newTestServer(t, newTestConfigWithWebPush(t))
var received atomic.Bool
pushService := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
_, err := io.ReadAll(r.Body)
require.Nil(t, err)
// Gone
w.WriteHeader(410)
w.WriteHeader(http.StatusGone)
received.Store(true)
}))
defer pushService.Close()
addSubscription(t, s, "test-topic", pushService.URL+"/push-receive")
addSubscription(t, s, "test-topic-abc", pushService.URL+"/push-receive")
addSubscription(t, s, pushService.URL+"/push-receive", "test-topic", "test-topic-abc")
requireSubscriptionCount(t, s, "test-topic", 1)
requireSubscriptionCount(t, s, "test-topic-abc", 1)
@ -195,7 +187,7 @@ func TestServer_WebPush_Expiry(t *testing.T) {
}))
defer pushService.Close()
addSubscription(t, s, "test-topic", pushService.URL+"/push-receive")
addSubscription(t, s, pushService.URL+"/push-receive", "test-topic")
requireSubscriptionCount(t, s, "test-topic", 1)
_, err := s.webPush.db.Exec("UPDATE subscriptions SET updated_at = datetime('now', '-7 days')")
@ -225,30 +217,18 @@ func payloadForTopics(t *testing.T, topics []string, endpoint string) string {
return fmt.Sprintf(`{
"topics": %s,
"browser_subscription":{
"endpoint": "%s",
"keys": {
"p256dh": "p256dh-key",
"auth": "auth-key"
}
}
"endpoint": "%s",
"p256dh": "p256dh-key",
"auth": "auth-key"
}`, topicsJSON, endpoint)
}
func addSubscription(t *testing.T, s *Server, topic string, url string) {
err := s.webPush.AddSubscription(topic, "", webpush.Subscription{
Endpoint: url,
Keys: webpush.Keys{
// connected to a local test VAPID key, not a leak!
Auth: "kSC3T8aN1JCQxxPdrFLrZg",
P256dh: "BMKKbxdUU_xLS7G1Wh5AN8PvWOjCzkCuKZYb8apcqYrDxjOF_2piggBnoJLQYx9IeSD70fNuwawI3e9Y8m3S3PE",
},
})
require.Nil(t, err)
func addSubscription(t *testing.T, s *Server, endpoint string, topics ...string) {
require.Nil(t, s.webPush.UpsertSubscription(endpoint, topics, "", "kSC3T8aN1JCQxxPdrFLrZg", "BMKKbxdUU_xLS7G1Wh5AN8PvWOjCzkCuKZYb8apcqYrDxjOF_2piggBnoJLQYx9IeSD70fNuwawI3e9Y8m3S3PE")) // Test auth and p256dh
}
func requireSubscriptionCount(t *testing.T, s *Server, topic string, expectedLength int) {
subs, err := s.webPush.SubscriptionsForTopic("test-topic")
subs, err := s.webPush.SubscriptionsForTopic(topic)
require.Nil(t, err)
require.Len(t, subs, expectedLength)
}

View file

@ -1,6 +1,7 @@
package server
import (
"github.com/SherClockHolmes/webpush-go"
"net/http"
"net/netip"
"time"
@ -8,7 +9,6 @@ import (
"heckel.io/ntfy/log"
"heckel.io/ntfy/user"
"github.com/SherClockHolmes/webpush-go"
"heckel.io/ntfy/util"
)
@ -467,6 +467,13 @@ type apiStripeSubscriptionDeletedEvent struct {
Customer string `json:"customer"`
}
type apiWebPushUpdateSubscriptionRequest struct {
Endpoint string `json:"endpoint"`
Auth string `json:"auth"`
P256dh string `json:"p256dh"`
Topics []string `json:"topics"`
}
// List of possible Web Push events
const (
webPushMessageEvent = "message"
@ -498,11 +505,18 @@ func newWebPushSubscriptionExpiringPayload() webPushControlMessagePayload {
}
type webPushSubscription struct {
BrowserSubscription webpush.Subscription
UserID string
Endpoint string
Auth string
P256dh string
UserID string
}
type webPushSubscriptionPayload struct {
BrowserSubscription webpush.Subscription `json:"browser_subscription"`
Topics []string `json:"topics"`
func (w *webPushSubscription) ToSubscription() *webpush.Subscription {
return &webpush.Subscription{
Endpoint: w.Endpoint,
Keys: webpush.Keys{
Auth: w.Auth,
P256dh: w.P256dh,
},
}
}

View file

@ -5,7 +5,6 @@ import (
"fmt"
"time"
"github.com/SherClockHolmes/webpush-go"
_ "github.com/mattn/go-sqlite3" // SQLite driver
)
@ -41,7 +40,7 @@ const (
deleteWebPushSubscriptionsByAgeQuery = `DELETE FROM subscriptions WHERE warning_sent = 1 AND updated_at <= datetime('now', ?)`
selectWebPushSubscriptionsForTopicQuery = `SELECT endpoint, key_auth, key_p256dh, user_id FROM subscriptions WHERE topic = ?`
selectWebPushSubscriptionsExpiringSoonQuery = `SELECT DISTINCT endpoint, key_auth, key_p256dh FROM subscriptions WHERE warning_sent = 0 AND updated_at <= datetime('now', ?)`
selectWebPushSubscriptionsExpiringSoonQuery = `SELECT DISTINCT endpoint, key_auth, key_p256dh, user_id FROM subscriptions WHERE warning_sent = 0 AND updated_at <= datetime('now', ?)`
updateWarningSentQuery = `UPDATE subscriptions SET warning_sent = true WHERE warning_sent = 0 AND updated_at <= datetime('now', ?)`
)
@ -89,65 +88,48 @@ func setupNewWebPushDB(db *sql.DB) error {
return nil
}
// UpdateSubscriptions updates the subscriptions for the given topics and user ID. It always first deletes all
// UpsertSubscription adds or updates Web Push subscriptions for the given topics and user ID. It always first deletes all
// existing entries for a given endpoint.
func (c *webPushStore) UpdateSubscriptions(topics []string, userID string, subscription webpush.Subscription) error {
func (c *webPushStore) UpsertSubscription(endpoint string, topics []string, userID, auth, p256dh string) error {
tx, err := c.db.Begin()
if err != nil {
return err
}
defer tx.Rollback()
if _, err := tx.Exec(deleteWebPushSubscriptionByEndpointQuery, subscription.Endpoint); err != nil {
if _, err := tx.Exec(deleteWebPushSubscriptionByEndpointQuery, endpoint); err != nil {
return err
}
for _, topic := range topics {
if _, err = tx.Exec(insertWebPushSubscriptionQuery, topic, userID, subscription.Endpoint, subscription.Keys.Auth, subscription.Keys.P256dh); err != nil {
if _, err = tx.Exec(insertWebPushSubscriptionQuery, topic, userID, endpoint, auth, p256dh); err != nil {
return err
}
}
return tx.Commit()
}
func (c *webPushStore) AddSubscription(topic string, userID string, subscription webpush.Subscription) error {
_, err := c.db.Exec(
insertWebPushSubscriptionQuery,
topic,
userID,
subscription.Endpoint,
subscription.Keys.Auth,
subscription.Keys.P256dh,
)
return err
}
func (c *webPushStore) SubscriptionsForTopic(topic string) (subscriptions []*webPushSubscription, err error) {
func (c *webPushStore) SubscriptionsForTopic(topic string) ([]*webPushSubscription, error) {
rows, err := c.db.Query(selectWebPushSubscriptionsForTopicQuery, topic)
if err != nil {
return nil, err
}
defer rows.Close()
var data []*webPushSubscription
subscriptions := make([]*webPushSubscription, 0)
for rows.Next() {
var userID, endpoint, auth, p256dh string
var endpoint, auth, p256dh, userID string
if err = rows.Scan(&endpoint, &auth, &p256dh, &userID); err != nil {
return nil, err
}
data = append(data, &webPushSubscription{
UserID: userID,
BrowserSubscription: webpush.Subscription{
Endpoint: endpoint,
Keys: webpush.Keys{
Auth: auth,
P256dh: p256dh,
},
},
subscriptions = append(subscriptions, &webPushSubscription{
Endpoint: endpoint,
Auth: auth,
P256dh: p256dh,
UserID: userID,
})
}
return data, nil
return subscriptions, nil
}
func (c *webPushStore) ExpireAndGetExpiringSubscriptions(warningDuration time.Duration, expiryDuration time.Duration) (subscriptions []webPushSubscription, err error) {
func (c *webPushStore) ExpireAndGetExpiringSubscriptions(warningDuration time.Duration, expiryDuration time.Duration) ([]*webPushSubscription, error) {
// TODO this should be two functions
tx, err := c.db.Begin()
if err != nil {
@ -166,15 +148,18 @@ func (c *webPushStore) ExpireAndGetExpiringSubscriptions(warningDuration time.Du
}
defer rows.Close()
var data []webPushSubscription
subscriptions := make([]*webPushSubscription, 0)
for rows.Next() {
i := webPushSubscription{}
err = rows.Scan(&i.BrowserSubscription.Endpoint, &i.BrowserSubscription.Keys.Auth, &i.BrowserSubscription.Keys.P256dh)
fmt.Printf("%v+", i)
if err != nil {
var endpoint, auth, p256dh, userID string
if err = rows.Scan(&endpoint, &auth, &p256dh, &userID); err != nil {
return nil, err
}
data = append(data, i)
subscriptions = append(subscriptions, &webPushSubscription{
Endpoint: endpoint,
Auth: auth,
P256dh: p256dh,
UserID: userID,
})
}
// also set warning as sent
@ -187,22 +172,16 @@ func (c *webPushStore) ExpireAndGetExpiringSubscriptions(warningDuration time.Du
return nil, err
}
return data, nil
return subscriptions, nil
}
func (c *webPushStore) RemoveByEndpoint(endpoint string) error {
_, err := c.db.Exec(
deleteWebPushSubscriptionByEndpointQuery,
endpoint,
)
func (c *webPushStore) RemoveSubscriptionsByEndpoint(endpoint string) error {
_, err := c.db.Exec(deleteWebPushSubscriptionByEndpointQuery, endpoint)
return err
}
func (c *webPushStore) RemoveByUserID(userID string) error {
_, err := c.db.Exec(
deleteWebPushSubscriptionByUserIDQuery,
userID,
)
func (c *webPushStore) RemoveSubscriptionsByUserID(userID string) error {
_, err := c.db.Exec(deleteWebPushSubscriptionByUserIDQuery, userID)
return err
}

View file

@ -0,0 +1,12 @@
package server
import (
"github.com/stretchr/testify/require"
"testing"
)
func newTestWebPushStore(t *testing.T, filename string) *webPushStore {
webPush, err := newWebPushStore(filename)
require.Nil(t, err)
return webPush
}

View file

@ -115,22 +115,22 @@ class Api {
throw new Error(`Unexpected server response ${response.status}`);
}
async updateWebPushSubscriptions(topics, browserSubscription) {
async updateWebPushSubscriptions(topics, pushSubscription) {
const user = await userManager.get(config.base_url);
const url = accountWebPushUrl(config.base_url);
console.log(`[Api] Sending Web Push Subscriptions`, { url, topics, endpoint: browserSubscription.endpoint });
const response = await fetch(url, {
console.log(`[Api] Sending Web Push Subscriptions`, { url, topics, endpoint: pushSubscription.endpoint });
console.log(`[Api] Sending Web Push Subscriptions`, { pushSubscription });
const serializedSubscription = JSON.parse(JSON.stringify(pushSubscription)); // Ugh ... https://stackoverflow.com/a/40525434/1440785
await fetchOrThrow(url, {
method: "PUT",
headers: maybeWithAuth({}, user),
body: JSON.stringify({ topics, browser_subscription: browserSubscription }),
body: JSON.stringify({
endpoint: serializedSubscription.endpoint,
auth: serializedSubscription.keys.auth,
p256dh: serializedSubscription.keys.p256dh,
topics
}),
});
if (response.ok) {
return true;
}
throw new Error(`Unexpected server response ${response.status}`);
}
}

View file

@ -58,7 +58,7 @@ class Notifier {
return existingSubscription;
}
// Create a new subscription only if web push is enabled.
// Create a new subscription only if web push is enabled.
// It is possible that web push was previously enabled and then disabled again
// in which case there would be an existingSubscription.
// but if it was _not_ enabled previously, we reach here, and only create a new
@ -76,11 +76,9 @@ class Notifier {
async pushManager() {
const registration = await navigator.serviceWorker.getRegistration();
if (!registration) {
throw new Error("No service worker registration found");
}
return registration.pushManager;
}