Small refactor

server/errors.go

@@ -141,5 +141,6 @@ var (
 	errHTTPInternalError                             = &errHTTP{50001, http.StatusInternalServerError, "internal server error", "", nil}
 	errHTTPInternalErrorInvalidPath                  = &errHTTP{50002, http.StatusInternalServerError, "internal server error: invalid path", "", nil}
 	errHTTPInternalErrorMissingBaseURL               = &errHTTP{50003, http.StatusInternalServerError, "internal server error: base-url must be be configured for this feature", "https://ntfy.sh/docs/config/", nil}
+	errHTTPInternalErrorWebPushUnableToPublish       = &errHTTP{50004, http.StatusInternalServerError, "internal server error: unable to publish web push message", "", nil}
 	errHTTPInsufficientStorageUnifiedPush            = &errHTTP{50701, http.StatusInsufficientStorage, "cannot publish to UnifiedPush topic without previously active subscriber", "", nil}
 )

server/log.go

@@ -29,7 +29,7 @@ const (
 	tagResetter     = "resetter"
 	tagWebsocket    = "websocket"
 	tagMatrix       = "matrix"
-	tagWebPush      = "web_push"
+	tagWebPush      = "webpush"
 )
 
 var (

server/server_web_push.go

@@ -30,27 +30,19 @@ 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 == "" {
 		return errHTTPBadRequestWebPushSubscriptionInvalid
-	}
-
-	if !webPushEndpointAllowRegex.MatchString(payload.BrowserSubscription.Endpoint) {
+	} else if !webPushEndpointAllowRegex.MatchString(payload.BrowserSubscription.Endpoint) {
 		return errHTTPBadRequestWebPushEndpointUnknown
-	}
-
-	if len(payload.Topics) > webPushTopicSubscribeLimit {
+	} else if len(payload.Topics) > webPushTopicSubscribeLimit {
 		return errHTTPBadRequestWebPushTopicCountTooHigh
 	}
-
-	u := v.User()
-
 	topics, err := s.topicsFromIDs(payload.Topics...)
 	if err != nil {
 		return err
 	}
-
 	if s.userManager != nil {
+		u := v.User()
 		for _, t := range topics {
 			if err := s.userManager.Authorize(u, t.ID, user.PermissionRead); err != nil {
 				logvr(v, r).With(t).Err(err).Debug("Access to topic %s not authorized", t.ID)
@@ -58,11 +50,9 @@ func (s *Server) handleWebPushUpdate(w http.ResponseWriter, r *http.Request, v *
 			}
 		}
 	}
-
 	if err := s.webPush.UpdateSubscriptions(payload.Topics, v.MaybeUserID(), payload.BrowserSubscription); err != nil {
 		return err
 	}
-
 	return s.writeJSON(w, newSuccessResponse())
 }
 
@@ -70,69 +60,68 @@ func (s *Server) publishToWebPushEndpoints(v *visitor, m *message) {
 	subscriptions, err := s.webPush.SubscriptionsForTopic(m.Topic)
 	if err != nil {
 		logvm(v, m).Err(err).Warn("Unable to publish web push messages")
-
 		return
 	}
-
-	for i, xi := range subscriptions {
-		go func(i int, sub webPushSubscription) {
-			ctx := log.Context{"endpoint": sub.BrowserSubscription.Endpoint, "username": sub.UserID, "topic": m.Topic, "message_id": m.ID}
-
-			s.sendWebPushNotification(newWebPushPayload(fmt.Sprintf("%s/%s", s.config.BaseURL, m.Topic), *m), &sub, &ctx)
-		}(i, xi)
+	payload, err := json.Marshal(newWebPushPayload(fmt.Sprintf("%s/%s", s.config.BaseURL, m.Topic), m))
+	if err != nil {
+		log.Tag(tagWebPush).Err(err).Warn("Unable to marshal expiring payload")
+		return
+	}
+	for _, subscription := range subscriptions {
+		ctx := log.Context{"endpoint": subscription.BrowserSubscription.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 the updated_at field is not actually updated ever
+
 func (s *Server) expireOrNotifyOldSubscriptions() {
 	subscriptions, err := s.webPush.ExpireAndGetExpiringSubscriptions(s.config.WebPushExpiryWarningDuration, s.config.WebPushExpiryDuration)
 	if err != nil {
 		log.Tag(tagWebPush).Err(err).Warn("Unable to publish expiry imminent warning")
-
+		return
+	} else if len(subscriptions) == 0 {
 		return
 	}
-
-	for i, xi := range subscriptions {
-		go func(i int, sub webPushSubscription) {
-			ctx := log.Context{"endpoint": sub.BrowserSubscription.Endpoint}
-
-			s.sendWebPushNotification(newWebPushSubscriptionExpiringPayload(), &sub, &ctx)
-		}(i, xi)
+	payload, err := json.Marshal(newWebPushSubscriptionExpiringPayload())
+	if err != nil {
+		log.Tag(tagWebPush).Err(err).Warn("Unable to marshal expiring payload")
+		return
 	}
-
-	log.Tag(tagWebPush).Debug("Expired old subscriptions and published %d expiry imminent warnings", len(subscriptions))
+	go func() {
+		for _, subscription := range subscriptions {
+			ctx := log.Context{"endpoint": subscription.BrowserSubscription.Endpoint}
+			if err := s.sendWebPushNotification(payload, &subscription, &ctx); err != nil {
+				log.Tag(tagWebPush).Err(err).Fields(ctx).Warn("Unable to publish expiry imminent warning")
+			}
+		}
+	}()
+	log.Tag(tagWebPush).Debug("Expiring old subscriptions and published %d expiry imminent warnings", len(subscriptions))
 }
 
-func (s *Server) sendWebPushNotification(payload any, sub *webPushSubscription, ctx *log.Context) {
-	jsonPayload, err := json.Marshal(payload)
-
-	if err != nil {
-		log.Tag(tagWebPush).Err(err).Fields(*ctx).Debug("Unable to publish web push message")
-		return
-	}
-
-	resp, err := webpush.SendNotification(jsonPayload, &sub.BrowserSubscription, &webpush.Options{
+func (s *Server) sendWebPushNotification(message []byte, sub *webPushSubscription, ctx *log.Context) error {
+	resp, err := webpush.SendNotification(message, &sub.BrowserSubscription, &webpush.Options{
 		Subscriber:      s.config.WebPushEmailAddress,
 		VAPIDPublicKey:  s.config.WebPushPublicKey,
 		VAPIDPrivateKey: s.config.WebPushPrivateKey,
-		// Deliverability on iOS isn't great with lower urgency values,
-		// and thus we can't really map lower ntfy priorities to lower urgency values
-		Urgency: webpush.UrgencyHigh,
+		Urgency:         webpush.UrgencyHigh, // iOS requires this to ensure delivery
 	})
-
 	if err != nil {
-		log.Tag(tagWebPush).Err(err).Fields(*ctx).Debug("Unable to publish web push message")
+		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 {
-			log.Tag(tagWebPush).Err(err).Fields(*ctx).Warn("Unable to expire subscription")
+			return err
 		}
-		return
+		return err
 	}
-
-	// May want to handle at least 429 differently, but for now treat all errors the same
-	if !(200 <= resp.StatusCode && resp.StatusCode <= 299) {
-		log.Tag(tagWebPush).Fields(*ctx).Field("response", resp).Debug("Unable to publish web push message")
+	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 {
-			log.Tag(tagWebPush).Err(err).Fields(*ctx).Warn("Unable to expire subscription")
+			return err
 		}
-		return
+		return errHTTPInternalErrorWebPushUnableToPublish.Fields(*ctx)
 	}
+	return nil
 }

server/types.go

@@ -467,15 +467,21 @@ type apiStripeSubscriptionDeletedEvent struct {
 	Customer string `json:"customer"`
 }
 
+// List of possible Web Push events
+const (
+	webPushMessageEvent  = "message"
+	webPushExpiringEvent = "subscription_expiring"
+)
+
 type webPushPayload struct {
-	Event          string  `json:"event"`
-	SubscriptionID string  `json:"subscription_id"`
-	Message        message `json:"message"`
+	Event          string   `json:"event"`
+	SubscriptionID string   `json:"subscription_id"`
+	Message        *message `json:"message"`
 }
 
-func newWebPushPayload(subscriptionID string, message message) webPushPayload {
+func newWebPushPayload(subscriptionID string, message *message) webPushPayload {
 	return webPushPayload{
-		Event:          "message",
+		Event:          webPushMessageEvent,
 		SubscriptionID: subscriptionID,
 		Message:        message,
 	}
@@ -487,7 +493,7 @@ type webPushControlMessagePayload struct {
 
 func newWebPushSubscriptionExpiringPayload() webPushControlMessagePayload {
 	return webPushControlMessagePayload{
-		Event: "subscription_expiring",
+		Event: webPushExpiringEvent,
 	}
 }
 

server/web_push.go

@@ -63,11 +63,11 @@ func newWebPushStore(filename string) (*webPushStore, error) {
 
 func setupSubscriptionsDB(db *sql.DB) error {
 	// If 'subscriptions' table does not exist, this must be a new database
-	rowsMC, err := db.Query(selectWebPushSubscriptionsCountQuery)
+	rows, err := db.Query(selectWebPushSubscriptionsCountQuery)
 	if err != nil {
 		return setupNewSubscriptionsDB(db)
 	}
-	return rowsMC.Close()
+	return rows.Close()
 }
 
 func setupNewSubscriptionsDB(db *sql.DB) error {
@@ -83,7 +83,6 @@ func (c *webPushStore) UpdateSubscriptions(topics []string, userID string, subsc
 		return err
 	}
 	defer tx.Rollback()
-
 	if err = c.RemoveByEndpoint(subscription.Endpoint); err != nil {
 		return err
 	}
@@ -107,26 +106,35 @@ func (c *webPushStore) AddSubscription(topic string, userID string, subscription
 	return err
 }
 
-func (c *webPushStore) SubscriptionsForTopic(topic string) (subscriptions []webPushSubscription, err error) {
+func (c *webPushStore) SubscriptionsForTopic(topic string) (subscriptions []*webPushSubscription, err error) {
 	rows, err := c.db.Query(selectWebPushSubscriptionsForTopicQuery, topic)
 	if err != nil {
 		return nil, err
 	}
 	defer rows.Close()
 
-	var data []webPushSubscription
+	var data []*webPushSubscription
 	for rows.Next() {
-		i := webPushSubscription{}
-		err = rows.Scan(&i.BrowserSubscription.Endpoint, &i.BrowserSubscription.Keys.Auth, &i.BrowserSubscription.Keys.P256dh, &i.UserID)
-		if err != nil {
+		var userID, endpoint, auth, p256dh string
+		if err = rows.Scan(&endpoint, &auth, &p256dh, &userID); err != nil {
 			return nil, err
 		}
-		data = append(data, i)
+		data = append(data, &webPushSubscription{
+			UserID: userID,
+			BrowserSubscription: webpush.Subscription{
+				Endpoint: endpoint,
+				Keys: webpush.Keys{
+					Auth:   auth,
+					P256dh: p256dh,
+				},
+			},
+		})
 	}
 	return data, nil
 }
 
 func (c *webPushStore) ExpireAndGetExpiringSubscriptions(warningDuration time.Duration, expiryDuration time.Duration) (subscriptions []webPushSubscription, err error) {
+	// TODO this should be two functions
 	tx, err := c.db.Begin()
 	if err != nil {
 		return nil, err