address comments

coder · johnstcn · Mar 27, 2025 · Mar 17, 2025 · Mar 25, 2025 · Mar 25, 2025
commit eb7b1028b865c89eb6fdfca430487bbf5ff13ab0
diff --git a/cli/server.go b/cli/server.go
@@ -787,9 +787,9 @@ func (r *RootCmd) Server(newAPI func(context.Context, *coderd.Options) (*coderd.
 						Msg: "Web Push notifications are disabled due to a system error. Please contact your Coder administrator.",
 					}
 				}
-				options.WebpushDispatcher = webpusher
+				options.WebPushDispatcher = webpusher
 			} else {
-				options.WebpushDispatcher = &webpush.NoopWebpusher{
+				options.WebPushDispatcher = &webpush.NoopWebpusher{
 					// Users will likely not see this message as the endpoints return 404
 					// if not enabled. Just in case...
 					Msg: "Web Push notifications are an experimental feature and are disabled by default. Enable the 'web-push' experiment to use this feature.",

diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -262,8 +262,8 @@ type Options struct {
 	OIDCConvertKeyCache   cryptokeys.SigningKeycache
 	Clock                 quartz.Clock
 
-	// WebpushDispatcher is a way to send notifications over Web Push.
-	WebpushDispatcher webpush.Dispatcher
+	// WebPushDispatcher is a way to send notifications over Web Push.
+	WebPushDispatcher webpush.Dispatcher
 }
 
 // @title Coder API
@@ -550,7 +550,7 @@ func New(options *Options) *API {
 		UserQuietHoursScheduleStore: options.UserQuietHoursScheduleStore,
 		AccessControlStore:          options.AccessControlStore,
 		Experiments:                 experiments,
-		WebpushDispatcher:           options.WebpushDispatcher,
+		WebpushDispatcher:           options.WebPushDispatcher,
 		healthCheckGroup:            &singleflight.Group[string, *healthsdk.HealthcheckReport]{},
 		Acquirer: provisionerdserver.NewAcquirer(
 			ctx,

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -286,7 +286,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
 		// nolint:gocritic // Gets/sets VAPID keys.
 		pushNotifier, err := webpush.New(dbauthz.AsNotifier(context.Background()), options.Logger, options.Database)
 		if err != nil {
-			panic(xerrors.Errorf("failed to create push notifier: %w", err))
+			panic(xerrors.Errorf("failed to create web push notifier: %w", err))
 		}
 		options.WebpushDispatcher = pushNotifier
 	}
@@ -541,7 +541,7 @@ func NewOptions(t testing.TB, options *Options) (func(http.Handler), context.Can
 			TrialGenerator:                     options.TrialGenerator,
 			RefreshEntitlements:                options.RefreshEntitlements,
 			TailnetCoordinator:                 options.Coordinator,
-			WebpushDispatcher:                  options.WebpushDispatcher,
+			WebPushDispatcher:                  options.WebpushDispatcher,
 			BaseDERPMap:                        derpMap,
 			DERPMapUpdateFrequency:             150 * time.Millisecond,
 			CoordinatorResumeTokenProvider:     options.CoordinatorResumeTokenProvider,

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -2453,6 +2453,8 @@ func (q *FakeQuerier) DeleteWebpushSubscriptionByUserIDAndEndpoint(_ context.Con
 }
 
 func (q *FakeQuerier) DeleteWebpushSubscriptions(_ context.Context, ids []uuid.UUID) error {
+	q.mutex.Lock()
+	defer q.mutex.Unlock()
 	for i, subscription := range q.webpushSubscriptions {
 		if slices.Contains(ids, subscription.ID) {
 			q.webpushSubscriptions[i] = q.webpushSubscriptions[len(q.webpushSubscriptions)-1]

diff --git a/coderd/notifications.go b/coderd/notifications.go
@@ -2,8 +2,11 @@ package coderd
 
 import (
 	"bytes"
+	"database/sql"
 	"encoding/json"
+	"errors"
 	"net/http"
+	"slices"
 
 	"github.com/google/uuid"
 
@@ -396,11 +399,31 @@ func (api *API) deleteUserWebpushSubscription(rw http.ResponseWriter, r *http.Re
 		return
 	}
 
-	err := api.Database.DeleteWebpushSubscriptionByUserIDAndEndpoint(ctx, database.DeleteWebpushSubscriptionByUserIDAndEndpointParams{
+	// Return NotFound if the subscription does not exist.
+	if existing, err := api.Database.GetWebpushSubscriptionsByUserID(ctx, user.ID); err != nil && errors.Is(err, sql.ErrNoRows) {
+		httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
+			Message: "Webpush subscription not found.",
+		})
+		return
+	} else if idx := slices.IndexFunc(existing, func(s database.WebpushSubscription) bool {
+		return s.Endpoint == req.Endpoint
+	}); idx == -1 {
+		httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
+			Message: "Webpush subscription not found.",
+		})
+		return
+	}
+
+	if err := api.Database.DeleteWebpushSubscriptionByUserIDAndEndpoint(ctx, database.DeleteWebpushSubscriptionByUserIDAndEndpointParams{
 		UserID:   user.ID,
 		Endpoint: req.Endpoint,
-	})
-	if err != nil {
+	}); err != nil {
+		if errors.Is(err, sql.ErrNoRows) {
+			httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{
+				Message: "Webpush subscription not found.",
+			})
+			return
+		}
 		httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
 			Message: "Failed to delete push notification subscription.",
 			Detail:  err.Error(),

diff --git a/coderd/notifications_test.go b/coderd/notifications_test.go
@@ -397,6 +397,7 @@ func TestWebpushSubscribeUnsubscribe(t *testing.T) {
 	})
 	owner := coderdtest.CreateFirstUser(t, client)
 	memberClient, _ := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
+	_, anotherMember := coderdtest.CreateAnotherUser(t, client, owner.OrganizationID)
 
 	handlerCalled := make(chan bool, 1)
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
@@ -414,11 +415,34 @@ func TestWebpushSubscribeUnsubscribe(t *testing.T) {
 	require.True(t, <-handlerCalled, "handler should have been called")
 
 	err = memberClient.PostTestWebpushMessage(ctx)
-	require.NoError(t, err, "test web push notification")
+	require.NoError(t, err, "test webpush message")
 	require.True(t, <-handlerCalled, "handler should have been called again")
 
 	err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
 		Endpoint: server.URL,
 	})
 	require.NoError(t, err, "delete webpush subscription")
+
+	// Deleting the subscription for a non-existent endpoint should return a 404
+	err = memberClient.DeleteWebpushSubscription(ctx, "me", codersdk.DeleteWebpushSubscription{
+		Endpoint: server.URL,
+	})
+	var sdkError *codersdk.Error
+	require.Error(t, err)
+	require.ErrorAsf(t, err, &sdkError, "error should be of type *codersdk.Error")
+	require.Equal(t, http.StatusNotFound, sdkError.StatusCode())
+
+	// Creating a subscription for another user should not be allowed.
+	err = memberClient.PostWebpushSubscription(ctx, anotherMember.ID.String(), codersdk.WebpushSubscription{
+		Endpoint:  server.URL,
+		AuthKey:   validEndpointAuthKey,
+		P256DHKey: validEndpointP256dhKey,
+	})
+	require.Error(t, err, "create webpush subscription for another user")
+
+	// Deleting a subscription for another user should not be allowed.
+	err = memberClient.DeleteWebpushSubscription(ctx, anotherMember.ID.String(), codersdk.DeleteWebpushSubscription{
+		Endpoint: server.URL,
+	})
+	require.Error(t, err, "delete webpush subscription for another user")
 }
diff --git a/coderd/webpush/webpush.go b/coderd/webpush/webpush.go
@@ -97,6 +97,8 @@ func (n *Webpusher) Dispatch(ctx context.Context, userID uuid.UUID, notification
 	for _, subscription := range subscriptions {
 		subscription := subscription
 		eg.Go(func() error {
+			// TODO: Implement some retry logic here. For now, this is just a
+			// best-effort attempt.
 			statusCode, body, err := n.webpushSend(ctx, notificationJSON, subscription.Endpoint, webpush.Keys{
 				Auth:   subscription.EndpointAuthKey,
 				P256dh: subscription.EndpointP256dhKey,
@@ -186,6 +188,8 @@ func (n *Webpusher) Test(ctx context.Context, req codersdk.WebpushSubscription)
 	return nil
 }
 
+// PublicKey returns the VAPID public key for the webpush dispatcher.
+// Clients need this, so it's exposed via the BuildInfo endpoint.
 func (n *Webpusher) PublicKey() string {
 	return n.VAPIDPublicKey
 }