Skip to content

Commit 960c5db

Browse files
johnstcnjohnstcn
committed
webpush: refactor notification test and send logic
1 parent 46c2cd8 commit 960c5db

File tree

3 files changed

+89
-94
lines changed

3 files changed

+89
-94
lines changed

‎coderd/notifications.go

Lines changed: 4 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,8 @@ package coderd
33
import (
44
"bytes"
55
"encoding/json"
6-
"io"
76
"net/http"
8-
"strconv"
97

10-
"github.com/SherClockHolmes/webpush-go"
118
"github.com/google/uuid"
129

1310
"cdr.dev/slog"
@@ -345,55 +342,21 @@ func (api *API) postUserWebpushSubscription(rw http.ResponseWriter, r *http.Requ
345342
return
346343
}
347344

348-
notificationJSON, err := json.Marshal(codersdk.WebpushMessage{
349-
Title: "It's working!",
350-
Body: "You've subscribed to push notifications.",
351-
})
352-
if err != nil {
345+
if err := api.WebpushDispatcher.Test(ctx, req); err != nil {
353346
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
354-
Message: "Failed to marshal notification",
347+
Message: "Failed to test webpush subscription",
355348
Detail: err.Error(),
356349
})
357350
return
358351
}
359352

360-
// Before inserting the subscription into the database, we send a test notification
361-
// to ensure the subscription is valid.
362-
resp, err := webpush.SendNotificationWithContext(r.Context(), notificationJSON, &webpush.Subscription{
363-
Endpoint: req.Endpoint,
364-
Keys: webpush.Keys{
365-
Auth: req.AuthKey,
366-
P256dh: req.P256DHKey,
367-
},
368-
}, &webpush.Options{
369-
VAPIDPublicKey: api.WebpushDispatcher.PublicKey(),
370-
VAPIDPrivateKey: api.WebpushDispatcher.PrivateKey(),
371-
})
372-
if err != nil {
373-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
374-
Message: "Failed to send notification",
375-
Detail: err.Error(),
376-
})
377-
return
378-
}
379-
defer resp.Body.Close()
380-
if resp.StatusCode != http.StatusCreated {
381-
body, _ := io.ReadAll(resp.Body)
382-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
383-
Message: "Failed to send notification. Status code: " + strconv.Itoa(resp.StatusCode),
384-
Detail: string(body),
385-
})
386-
return
387-
}
388-
389-
_, err = api.Database.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{
353+
if _, err := api.Database.InsertWebpushSubscription(ctx, database.InsertWebpushSubscriptionParams{
390354
CreatedAt: dbtime.Now(),
391355
UserID: user.ID,
392356
Endpoint: req.Endpoint,
393357
EndpointAuthKey: req.AuthKey,
394358
EndpointP256dhKey: req.P256DHKey,
395-
})
396-
if err != nil {
359+
}); err != nil {
397360
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
398361
Message: "Failed to insert push notification subscription.",
399362
Detail: err.Error(),

‎coderd/notifications_test.go

Lines changed: 25 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -385,40 +385,36 @@ const (
385385
validEndpointP256dhKey = "BNNL5ZaTfK81qhXOx23+wewhigUeFb632jN6LvRWCFH1ubQr77FE/9qV1FuojuRmHP42zmf34rXgW80OvUVDgTk="
386386
)
387387

388-
func TestPushNotificationSubscription(t *testing.T) {
388+
func TestWebpushSubscribeUnsubscribe(t *testing.T) {
389389
t.Parallel()
390390

391-
t.Run("CRUD", func(t *testing.T) {
392-
t.Parallel()
391+
ctx := testutil.Context(t, testutil.WaitShort)
393392

394-
ctx := testutil.Context(t, testutil.WaitShort)
393+
client := coderdtest.New(t, nil)
394+
owner := coderdtest.CreateFirstUser(t, client)
395+
memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
395396

396-
client := coderdtest.New(t, nil)
397-
owner := coderdtest.CreateFirstUser(t, client)
398-
memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
399-
400-
handlerCalled := make(chan bool, 1)
401-
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
402-
w.WriteHeader(http.StatusCreated)
403-
handlerCalled <- true
404-
}))
405-
defer server.Close()
406-
407-
err := memberClient.PostWebpushSubscription(ctx, "me", codersdk.WebpushSubscription{
408-
Endpoint: server.URL,
409-
AuthKey: validEndpointAuthKey,
410-
P256DHKey: validEndpointP256dhKey,
411-
})
412-
require.NoError(t, err, "create webpush subscription")
413-
require.True(t, <-handlerCalled, "handler should have been called")
397+
handlerCalled := make(chan bool, 1)
398+
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
399+
w.WriteHeader(http.StatusCreated)
400+
handlerCalled <- true
401+
}))
402+
defer server.Close()
414403

415-
err = memberClient.PostTestWebpushMessage(ctx)
416-
require.NoError(t, err, "test web push notification")
417-
require.True(t, <-handlerCalled, "handler should have been called again")
404+
err := memberClient.PostWebpushSubscription(ctx, "me", codersdk.WebpushSubscription{
405+
Endpoint: server.URL,
406+
AuthKey: validEndpointAuthKey,
407+
P256DHKey: validEndpointP256dhKey,
408+
})
409+
require.NoError(t, err, "create webpush subscription")
410+
require.True(t, <-handlerCalled, "handler should have been called")
418411

419-
err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
420-
Endpoint: server.URL,
421-
})
422-
require.NoError(t, err, "delete webpush subscription")
412+
err = memberClient.PostTestWebpushMessage(ctx)
413+
require.NoError(t, err, "test web push notification")
414+
require.True(t, <-handlerCalled, "handler should have been called again")
415+
416+
err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
417+
Endpoint: server.URL,
423418
})
419+
require.NoError(t, err, "delete webpush subscription")
424420
}

‎coderd/webpush/webpush.go

Lines changed: 60 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,12 @@ import (
2424
// Dispatcher is an interface that can be used to dispatch
2525
// push notifications over Web Push.
2626
type Dispatcher interface {
27+
// Dispatch sends a notification to all subscriptions for a user. Any
28+
// notifications that fail to send are silently dropped.
2729
Dispatch(ctx context.Context, userID uuid.UUID, notification codersdk.WebpushMessage) error
30+
// Test sends a test notification to a subscription to ensure it is valid.
31+
Test(ctx context.Context, req codersdk.WebpushSubscription) error
2832
PublicKey() string
29-
PrivateKey() string
3033
}
3134

3235
// New creates a new Dispatcher to dispatch notifications via Web Push.
@@ -93,24 +96,15 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, notification
9396
for _, subscription := range subscriptions {
9497
subscription := subscription
9598
eg.Go(func() error {
96-
n.log.Debug(ctx, "dispatching via push", slog.F("subscription", subscription.Endpoint))
97-
cpy := slices.Clone(notificationJSON) // Need to copy as webpush.SendNotificationWithContext modifies the slice.
98-
resp, err := webpush.SendNotificationWithContext(ctx, cpy, &webpush.Subscription{
99-
Endpoint: subscription.Endpoint,
100-
Keys: webpush.Keys{
101-
Auth: subscription.EndpointAuthKey,
102-
P256dh: subscription.EndpointP256dhKey,
103-
},
104-
}, &webpush.Options{
105-
VAPIDPublicKey: n.VAPIDPublicKey,
106-
VAPIDPrivateKey: n.VAPIDPrivateKey,
99+
statusCode, body, err := n.webpushSend(ctx, notificationJSON, subscription.Endpoint, webpush.Keys{
100+
Auth: subscription.EndpointAuthKey,
101+
P256dh: subscription.EndpointP256dhKey,
107102
})
108103
if err != nil {
109104
return xerrors.Errorf("send notification: %w", err)
110105
}
111-
defer resp.Body.Close()
112106

113-
if resp.StatusCode == http.StatusGone {
107+
if statusCode == http.StatusGone {
114108
// The subscription is no longer valid, remove it.
115109
mu.Lock()
116110
cleanupSubscriptions = append(cleanupSubscriptions, subscription.ID)
@@ -119,10 +113,9 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, notification
119113
}
120114

121115
// 200, 201, and 202 are common for successful delivery.
122-
if resp.StatusCode > http.StatusAccepted {
116+
if statusCode > http.StatusAccepted {
123117
// It's likely the subscription failed to deliver for some reason.
124-
body, _ := io.ReadAll(resp.Body)
125-
return xerrors.Errorf("web push dispatch failed with status code %d: %s", resp.StatusCode, string(body))
118+
return xerrors.Errorf("web push dispatch failed with status code %d: %s", statusCode, string(body))
126119
}
127120

128121
return nil
@@ -145,12 +138,55 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, notification
145138
return nil
146139
}
147140

148-
func (n *Webpusher) PublicKey() string {
149-
return n.VAPIDPublicKey
141+
func (n *Webpusher) webpushSend(ctx context.Context, msg []byte, endpoint string, keys webpush.Keys) (int, []byte, error) {
142+
// Copy the message to avoid modifying the original.
143+
cpy := slices.Clone(msg)
144+
resp, err := webpush.SendNotificationWithContext(ctx, cpy, &webpush.Subscription{
145+
Endpoint: endpoint,
146+
Keys: keys,
147+
}, &webpush.Options{
148+
VAPIDPublicKey: n.VAPIDPublicKey,
149+
VAPIDPrivateKey: n.VAPIDPrivateKey,
150+
})
151+
if err != nil {
152+
return -1, nil, xerrors.Errorf("send notification: %w", err)
153+
}
154+
defer resp.Body.Close()
155+
body, err := io.ReadAll(resp.Body)
156+
if err != nil {
157+
return -1, nil, xerrors.Errorf("read response body: %w", err)
158+
}
159+
160+
return resp.StatusCode, body, nil
161+
}
162+
163+
func (n *Webpusher) Test(ctx context.Context, req codersdk.WebpushSubscription) error {
164+
notificationJSON, err := json.Marshal(codersdk.WebpushMessage{
165+
Title: "Test",
166+
Body: "This is a test notification",
167+
})
168+
if err != nil {
169+
return xerrors.Errorf("marshal notification: %w", err)
170+
}
171+
statusCode, body, err := n.webpushSend(ctx, notificationJSON, req.Endpoint, webpush.Keys{
172+
Auth: req.AuthKey,
173+
P256dh: req.P256DHKey,
174+
})
175+
if err != nil {
176+
return xerrors.Errorf("send test notification: %w", err)
177+
}
178+
179+
// 200, 201, and 202 are common for successful delivery.
180+
if statusCode > http.StatusAccepted {
181+
// It's likely the subscription failed to deliver for some reason.
182+
return xerrors.Errorf("web push dispatch failed with status code %d: %s", statusCode, string(body))
183+
}
184+
185+
return nil
150186
}
151187

152-
func (n *Webpusher) PrivateKey() string {
153-
return n.VAPIDPrivateKey
188+
func (n *Webpusher) PublicKey() string {
189+
return n.VAPIDPublicKey
154190
}
155191

156192
// NoopWebpusher is a Dispatcher that does nothing except return an error.
@@ -164,11 +200,11 @@ func (n *NoopWebpusher) Dispatch(context.Context, uuid.UUID, codersdk.WebpushMes
164200
return xerrors.New(n.Msg)
165201
}
166202

167-
func (*NoopWebpusher) PublicKey() string {
168-
return ""
203+
func (n *NoopWebpusher) Test(context.Context, codersdk.WebpushSubscription) error {
204+
return xerrors.New(n.Msg)
169205
}
170206

171-
func (*NoopWebpusher) PrivateKey() string {
207+
func (*NoopWebpusher) PublicKey() string {
172208
return ""
173209
}
174210

0 commit comments

Comments
 (0)