Skip to content

Commit eb7b102

Browse files
johnstcnjohnstcn
committed
address comments
1 parent bcf108a commit eb7b102

File tree

7 files changed

+64
-11
lines changed

7 files changed

+64
-11
lines changed

‎cli/server.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -787,9 +787,9 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
787787
Msg: "Web Push notifications are disabled due to a system error. Please contact your Coder administrator.",
788788
}
789789
}
790-
options.WebpushDispatcher = webpusher
790+
options.WebPushDispatcher = webpusher
791791
} else {
792-
options.WebpushDispatcher = &webpush.NoopWebpusher{
792+
options.WebPushDispatcher = &webpush.NoopWebpusher{
793793
// Users will likely not see this message as the endpoints return 404
794794
// if not enabled. Just in case...
795795
Msg: "Web Push notifications are an experimental feature and are disabled by default. Enable the 'web-push' experiment to use this feature.",

‎coderd/coderd.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -262,8 +262,8 @@ type Options struct {
262262
OIDCConvertKeyCache cryptokeys.SigningKeycache
263263
Clock quartz.Clock
264264

265-
// WebpushDispatcher is a way to send notifications over Web Push.
266-
WebpushDispatcher webpush.Dispatcher
265+
// WebPushDispatcher is a way to send notifications over Web Push.
266+
WebPushDispatcher webpush.Dispatcher
267267
}
268268

269269
// @title Coder API
@@ -550,7 +550,7 @@ func New(options *Options) *API {
550550
UserQuietHoursScheduleStore: options.UserQuietHoursScheduleStore,
551551
AccessControlStore: options.AccessControlStore,
552552
Experiments: experiments,
553-
WebpushDispatcher: options.WebpushDispatcher,
553+
WebpushDispatcher: options.WebPushDispatcher,
554554
healthCheckGroup: &singleflight.Group[string, *healthsdk.HealthcheckReport]{},
555555
Acquirer: provisionerdserver.NewAcquirer(
556556
ctx,

‎coderd/coderdtest/coderdtest.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
286286
// nolint:gocritic // Gets/sets VAPID keys.
287287
pushNotifier, err := webpush.New(dbauthz.AsNotifier(context.Background()), options.Logger, options.Database)
288288
if err != nil {
289-
panic(xerrors.Errorf("failed to create push notifier: %w", err))
289+
panic(xerrors.Errorf("failed to create web push notifier: %w", err))
290290
}
291291
options.WebpushDispatcher = pushNotifier
292292
}
@@ -541,7 +541,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
541541
TrialGenerator: options.TrialGenerator,
542542
RefreshEntitlements: options.RefreshEntitlements,
543543
TailnetCoordinator: options.Coordinator,
544-
WebpushDispatcher: options.WebpushDispatcher,
544+
WebPushDispatcher: options.WebpushDispatcher,
545545
BaseDERPMap: derpMap,
546546
DERPMapUpdateFrequency: 150 * time.Millisecond,
547547
CoordinatorResumeTokenProvider: options.CoordinatorResumeTokenProvider,

‎coderd/database/dbmem/dbmem.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2453,6 +2453,8 @@ func (q *FakeQuerier) DeleteWebpushSubscriptionByUserIDAndEndpoint(_ context.Con
24532453
}
24542454

24552455
func (q *FakeQuerier) DeleteWebpushSubscriptions(_ context.Context, ids []uuid.UUID) error {
2456+
q.mutex.Lock()
2457+
defer q.mutex.Unlock()
24562458
for i, subscription := range q.webpushSubscriptions {
24572459
if slices.Contains(ids, subscription.ID) {
24582460
q.webpushSubscriptions[i] = q.webpushSubscriptions[len(q.webpushSubscriptions)-1]

‎coderd/notifications.go

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ package coderd
22

33
import (
44
"bytes"
5+
"database/sql"
56
"encoding/json"
7+
"errors"
68
"net/http"
9+
"slices"
710

811
"github.com/google/uuid"
912

@@ -396,11 +399,31 @@ func (api *API) deleteUserWebpushSubscription(rw http.ResponseWriter, r *http.Re
396399
return
397400
}
398401

399-
err := api.Database.DeleteWebpushSubscriptionByUserIDAndEndpoint(ctx, database.DeleteWebpushSubscriptionByUserIDAndEndpointParams{
402+
// Return NotFound if the subscription does not exist.
403+
if existing, err := api.Database.GetWebpushSubscriptionsByUserID(ctx, user.ID); err != nil && errors.Is(err, sql.ErrNoRows) {
404+
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
405+
Message: "Webpush subscription not found.",
406+
})
407+
return
408+
} else if idx := slices.IndexFunc(existing, func(s database.WebpushSubscription) bool {
409+
return s.Endpoint == req.Endpoint
410+
}); idx == -1 {
411+
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
412+
Message: "Webpush subscription not found.",
413+
})
414+
return
415+
}
416+
417+
if err := api.Database.DeleteWebpushSubscriptionByUserIDAndEndpoint(ctx, database.DeleteWebpushSubscriptionByUserIDAndEndpointParams{
400418
UserID: user.ID,
401419
Endpoint: req.Endpoint,
402-
})
403-
if err != nil {
420+
}); err != nil {
421+
if errors.Is(err, sql.ErrNoRows) {
422+
httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
423+
Message: "Webpush subscription not found.",
424+
})
425+
return
426+
}
404427
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
405428
Message: "Failed to delete push notification subscription.",
406429
Detail: err.Error(),

‎coderd/notifications_test.go

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -397,6 +397,7 @@ func TestWebpushSubscribeUnsubscribe(t *testing.T) {
397397
})
398398
owner := coderdtest.CreateFirstUser(t, client)
399399
memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
400+
_, anotherMember := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
400401

401402
handlerCalled := make(chan bool, 1)
402403
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
@@ -414,11 +415,34 @@ func TestWebpushSubscribeUnsubscribe(t *testing.T) {
414415
require.True(t, <-handlerCalled, "handler should have been called")
415416

416417
err = memberClient.PostTestWebpushMessage(ctx)
417-
require.NoError(t, err, "test web push notification")
418+
require.NoError(t, err, "test webpush message")
418419
require.True(t, <-handlerCalled, "handler should have been called again")
419420

420421
err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
421422
Endpoint: server.URL,
422423
})
423424
require.NoError(t, err, "delete webpush subscription")
425+
426+
// Deleting the subscription for a non-existent endpoint should return a 404
427+
err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
428+
Endpoint: server.URL,
429+
})
430+
var sdkError *codersdk.Error
431+
require.Error(t, err)
432+
require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
433+
require.Equal(t, http.StatusNotFound, sdkError.StatusCode())
434+
435+
// Creating a subscription for another user should not be allowed.
436+
err = memberClient.PostWebpushSubscription(ctx, anotherMember.ID.String(), codersdk.WebpushSubscription{
437+
Endpoint: server.URL,
438+
AuthKey: validEndpointAuthKey,
439+
P256DHKey: validEndpointP256dhKey,
440+
})
441+
require.Error(t, err, "create webpush subscription for another user")
442+
443+
// Deleting a subscription for another user should not be allowed.
444+
err = memberClient.DeleteWebpushSubscription(ctx, anotherMember.ID.String(), codersdk.DeleteWebpushSubscription{
445+
Endpoint: server.URL,
446+
})
447+
require.Error(t, err, "delete webpush subscription for another user")
424448
}

‎coderd/webpush/webpush.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, notification
9797
for _, subscription := range subscriptions {
9898
subscription := subscription
9999
eg.Go(func() error {
100+
// TODO: Implement some retry logic here. For now, this is just a
101+
// best-effort attempt.
100102
statusCode, body, err := n.webpushSend(ctx, notificationJSON, subscription.Endpoint, webpush.Keys{
101103
Auth: subscription.EndpointAuthKey,
102104
P256dh: subscription.EndpointP256dhKey,
@@ -186,6 +188,8 @@ func (n *Webpusher) Test(ctx context.Context, req codersdk.WebpushSubscription)
186188
return nil
187189
}
188190

191+
// PublicKey returns the VAPID public key for the webpush dispatcher.
192+
// Clients need this, so it's exposed via the BuildInfo endpoint.
189193
func (n *Webpusher) PublicKey() string {
190194
return n.VAPIDPublicKey
191195
}

0 commit comments

Comments
 (0)