Remove awkward subscription id

server/errors.go

@@ -76,6 +76,7 @@ var (
 	errHTTPForbidden                                 = &errHTTP{40301, http.StatusForbidden, "forbidden", "https://ntfy.sh/docs/publish/#authentication"}
 	errHTTPConflictUserExists                        = &errHTTP{40901, http.StatusConflict, "conflict: user already exists", ""}
 	errHTTPConflictTopicReserved                     = &errHTTP{40902, http.StatusConflict, "conflict: access control entry for topic or topic pattern already exists", ""}
+	errHTTPConflictSubscriptionExists                = &errHTTP{40903, http.StatusConflict, "conflict: topic subscription already exists", ""}
 	errHTTPEntityTooLargeAttachment                  = &errHTTP{41301, http.StatusRequestEntityTooLarge, "attachment too large, or bandwidth limit reached", "https://ntfy.sh/docs/publish/#limitations"}
 	errHTTPEntityTooLargeMatrixRequest               = &errHTTP{41302, http.StatusRequestEntityTooLarge, "Matrix request is larger than the max allowed length", ""}
 	errHTTPEntityTooLargeJSONBody                    = &errHTTP{41303, http.StatusRequestEntityTooLarge, "JSON body too large", ""}

server/server.go

@@ -39,8 +39,6 @@ import (
   - tiers
   - api
   - tokens
-- LOW: UI: Flickering upgrade banner when logging in
-- LOW: get rid of reservation id, replace with DELETE X-Topic: ...
 
 */
 
@@ -98,7 +96,6 @@ var (
 	apiAccountBillingSubscriptionCheckoutSuccessTemplate = "/v1/account/billing/subscription/success/{CHECKOUT_SESSION_ID}"
 	apiAccountBillingSubscriptionCheckoutSuccessRegex    = regexp.MustCompile(`/v1/account/billing/subscription/success/(.+)$`)
 	apiAccountReservationSingleRegex                     = regexp.MustCompile(`/v1/account/reservation/([-_A-Za-z0-9]{1,64})$`)
-	apiAccountSubscriptionSingleRegex                    = regexp.MustCompile(`^/v1/account/subscription/([-_A-Za-z0-9]{16})$`)
 	staticRegex                                          = regexp.MustCompile(`^/static/.+`)
 	docsRegex                                            = regexp.MustCompile(`^/docs(|/.*)$`)
 	fileRegex                                            = regexp.MustCompile(`^/file/([-_A-Za-z0-9]{1,64})(?:\.[A-Za-z0-9]{1,16})?$`)
@@ -404,9 +401,9 @@ func (s *Server) handleInternal(w http.ResponseWriter, r *http.Request, v *visit
 		return s.ensureUser(s.withAccountSync(s.handleAccountSettingsChange))(w, r, v)
 	} else if r.Method == http.MethodPost && r.URL.Path == apiAccountSubscriptionPath {
 		return s.ensureUser(s.withAccountSync(s.handleAccountSubscriptionAdd))(w, r, v)
-	} else if r.Method == http.MethodPatch && apiAccountSubscriptionSingleRegex.MatchString(r.URL.Path) {
+	} else if r.Method == http.MethodPatch && r.URL.Path == apiAccountSubscriptionPath {
 		return s.ensureUser(s.withAccountSync(s.handleAccountSubscriptionChange))(w, r, v)
-	} else if r.Method == http.MethodDelete && apiAccountSubscriptionSingleRegex.MatchString(r.URL.Path) {
+	} else if r.Method == http.MethodDelete && r.URL.Path == apiAccountSubscriptionPath {
 		return s.ensureUser(s.withAccountSync(s.handleAccountSubscriptionDelete))(w, r, v)
 	} else if r.Method == http.MethodPost && r.URL.Path == apiAccountReservationPath {
 		return s.ensureUser(s.withAccountSync(s.handleAccountReservationAdd))(w, r, v)

server/server_account.go

@@ -12,8 +12,6 @@ import (
 )
 
 const (
-	subscriptionIDLength      = 16
-	subscriptionIDPrefix      = "su_"
 	syncTopicAccountSyncEvent = "sync"
 	tokenExpiryDuration       = 72 * time.Hour // Extend tokens by this much
 )
@@ -323,52 +321,36 @@ func (s *Server) handleAccountSubscriptionAdd(w http.ResponseWriter, r *http.Req
 		return err
 	}
 	u := v.User()
-	if u.Prefs == nil {
-		u.Prefs = &user.Prefs{}
+	prefs := u.Prefs
+	if prefs == nil {
+		prefs = &user.Prefs{}
 	}
-	newSubscription.ID = "" // Client cannot set ID
-	for _, subscription := range u.Prefs.Subscriptions {
+	for _, subscription := range prefs.Subscriptions {
 		if newSubscription.BaseURL == subscription.BaseURL && newSubscription.Topic == subscription.Topic {
-			newSubscription = subscription
-			break
+			return errHTTPConflictSubscriptionExists
 		}
 	}
-	if newSubscription.ID == "" {
-		newSubscription.ID = util.RandomStringPrefix(subscriptionIDPrefix, subscriptionIDLength)
-		prefs := u.Prefs
-		prefs.Subscriptions = append(prefs.Subscriptions, newSubscription)
-		logvr(v, r).
-			Tag(tagAccount).
-			Fields(log.Context{
-				"base_url": newSubscription.BaseURL,
-				"topic":    newSubscription.Topic,
-			}).
-			Debug("Adding subscription for user %s", u.Name)
-		if err := s.userManager.ChangeSettings(u.ID, prefs); err != nil {
-			return err
-		}
+	prefs.Subscriptions = append(prefs.Subscriptions, newSubscription)
+	logvr(v, r).Tag(tagAccount).With(newSubscription).Debug("Adding subscription for user %s", u.Name)
+	if err := s.userManager.ChangeSettings(u.ID, prefs); err != nil {
+		return err
 	}
 	return s.writeJSON(w, newSubscription)
 }
 
 func (s *Server) handleAccountSubscriptionChange(w http.ResponseWriter, r *http.Request, v *visitor) error {
-	matches := apiAccountSubscriptionSingleRegex.FindStringSubmatch(r.URL.Path)
-	if len(matches) != 2 {
-		return errHTTPInternalErrorInvalidPath
-	}
-	subscriptionID := matches[1]
 	updatedSubscription, err := readJSONWithLimit[user.Subscription](r.Body, jsonBodyBytesLimit, false)
 	if err != nil {
 		return err
 	}
 	u := v.User()
-	if u.Prefs == nil || u.Prefs.Subscriptions == nil {
+	prefs := u.Prefs
+	if prefs == nil || prefs.Subscriptions == nil {
 		return errHTTPNotFound
 	}
-	prefs := u.Prefs
 	var subscription *user.Subscription
 	for _, sub := range prefs.Subscriptions {
-		if sub.ID == subscriptionID {
+		if sub.BaseURL == updatedSubscription.BaseURL && sub.Topic == updatedSubscription.Topic {
 			sub.DisplayName = updatedSubscription.DisplayName
 			subscription = sub
 			break
@@ -377,14 +359,7 @@ func (s *Server) handleAccountSubscriptionChange(w http.ResponseWriter, r *http.
 	if subscription == nil {
 		return errHTTPNotFound
 	}
-	logvr(v, r).
-		Tag(tagAccount).
-		Fields(log.Context{
-			"base_url":     subscription.BaseURL,
-			"topic":        subscription.Topic,
-			"display_name": subscription.DisplayName,
-		}).
-		Debug("Changing subscription for user %s", u.Name)
+	logvr(v, r).Tag(tagAccount).With(subscription).Debug("Changing subscription for user %s", u.Name)
 	if err := s.userManager.ChangeSettings(u.ID, prefs); err != nil {
 		return err
 	}
@@ -392,31 +367,23 @@ func (s *Server) handleAccountSubscriptionChange(w http.ResponseWriter, r *http.
 }
 
 func (s *Server) handleAccountSubscriptionDelete(w http.ResponseWriter, r *http.Request, v *visitor) error {
-	matches := apiAccountSubscriptionSingleRegex.FindStringSubmatch(r.URL.Path)
-	if len(matches) != 2 {
-		return errHTTPInternalErrorInvalidPath
-	}
-	subscriptionID := matches[1]
+	// DELETEs cannot have a body, and we don't want it in the path
+	deleteBaseURL := readParam(r, "X-BaseURL", "BaseURL")
+	deleteTopic := readParam(r, "X-Topic", "Topic")
 	u := v.User()
-	if u.Prefs == nil || u.Prefs.Subscriptions == nil {
+	prefs := u.Prefs
+	if prefs == nil || prefs.Subscriptions == nil {
 		return nil
 	}
 	newSubscriptions := make([]*user.Subscription, 0)
-	for _, subscription := range u.Prefs.Subscriptions {
-		if subscription.ID == subscriptionID {
-			logvr(v, r).
-				Tag(tagAccount).
-				Fields(log.Context{
-					"base_url": subscription.BaseURL,
-					"topic":    subscription.Topic,
-				}).
-				Debug("Removing subscription for user %s", u.Name)
+	for _, sub := range u.Prefs.Subscriptions {
+		if sub.BaseURL == deleteBaseURL && sub.Topic == deleteTopic {
+			logvr(v, r).Tag(tagAccount).With(sub).Debug("Removing subscription for user %s", u.Name)
 		} else {
-			newSubscriptions = append(newSubscriptions, subscription)
+			newSubscriptions = append(newSubscriptions, sub)
 		}
 	}
-	if len(newSubscriptions) < len(u.Prefs.Subscriptions) {
-		prefs := u.Prefs
+	if len(newSubscriptions) < len(prefs.Subscriptions) {
 		prefs.Subscriptions = newSubscriptions
 		if err := s.userManager.ChangeSettings(u.ID, prefs); err != nil {
 			return err

server/server_account_test.go

@@ -214,13 +214,11 @@ func TestAccount_Subscription_AddUpdateDelete(t *testing.T) {
 	require.Equal(t, 200, rr.Code)
 	account, _ := util.UnmarshalJSON[apiAccountResponse](io.NopCloser(rr.Body))
 	require.Equal(t, 1, len(account.Subscriptions))
-	require.NotEmpty(t, account.Subscriptions[0].ID)
 	require.Equal(t, "http://abc.com", account.Subscriptions[0].BaseURL)
 	require.Equal(t, "def", account.Subscriptions[0].Topic)
 	require.Nil(t, account.Subscriptions[0].DisplayName)
 
-	subscriptionID := account.Subscriptions[0].ID
-	rr = request(t, s, "PATCH", "/v1/account/subscription/"+subscriptionID, `{"display_name": "ding dong"}`, map[string]string{
+	rr = request(t, s, "PATCH", "/v1/account/subscription", `{"base_url": "http://abc.com", "topic": "def", "display_name": "ding dong"}`, map[string]string{
 		"Authorization": util.BasicAuth("phil", "phil"),
 	})
 	require.Equal(t, 200, rr.Code)
@@ -231,13 +229,14 @@ func TestAccount_Subscription_AddUpdateDelete(t *testing.T) {
 	require.Equal(t, 200, rr.Code)
 	account, _ = util.UnmarshalJSON[apiAccountResponse](io.NopCloser(rr.Body))
 	require.Equal(t, 1, len(account.Subscriptions))
-	require.Equal(t, subscriptionID, account.Subscriptions[0].ID)
 	require.Equal(t, "http://abc.com", account.Subscriptions[0].BaseURL)
 	require.Equal(t, "def", account.Subscriptions[0].Topic)
 	require.Equal(t, util.String("ding dong"), account.Subscriptions[0].DisplayName)
 
-	rr = request(t, s, "DELETE", "/v1/account/subscription/"+subscriptionID, "", map[string]string{
+	rr = request(t, s, "DELETE", "/v1/account/subscription", "", map[string]string{
 		"Authorization": util.BasicAuth("phil", "phil"),
+		"X-BaseURL":     "http://abc.com",
+		"X-Topic":       "def",
 	})
 	require.Equal(t, 200, rr.Code)
 

user/manager_test.go

@@ -726,7 +726,6 @@ func TestManager_ChangeSettings(t *testing.T) {
 		},
 		Subscriptions: []*Subscription{
 			{
-				ID:          "someID",
 				BaseURL:     "https://ntfy.sh",
 				Topic:       "mytopic",
 				DisplayName: util.String("My Topic"),
@@ -742,7 +741,6 @@ func TestManager_ChangeSettings(t *testing.T) {
 	require.Equal(t, util.String("ding"), u.Prefs.Notification.Sound)
 	require.Equal(t, util.Int(2), u.Prefs.Notification.MinPriority)
 	require.Nil(t, u.Prefs.Notification.DeleteAfter)
-	require.Equal(t, "someID", u.Prefs.Subscriptions[0].ID)
 	require.Equal(t, "https://ntfy.sh", u.Prefs.Subscriptions[0].BaseURL)
 	require.Equal(t, "mytopic", u.Prefs.Subscriptions[0].Topic)
 	require.Equal(t, util.String("My Topic"), u.Prefs.Subscriptions[0].DisplayName)

user/types.go

@@ -105,12 +105,19 @@ func (t *Tier) Context() log.Context {
 
 // Subscription represents a user's topic subscription
 type Subscription struct {
-	ID          string  `json:"id"`
 	BaseURL     string  `json:"base_url"`
 	Topic       string  `json:"topic"`
 	DisplayName *string `json:"display_name"`
 }
 
+// Context returns fields for the log
+func (s *Subscription) Context() log.Context {
+	return log.Context{
+		"base_url": s.BaseURL,
+		"topic":    s.Topic,
+	}
+}
+
 // NotificationPrefs represents the user's notification settings
 type NotificationPrefs struct {
 	Sound       *string `json:"sound,omitempty"`

web/src/app/AccountApi.js

@@ -173,9 +173,12 @@ class AccountApi {
         });
     }
 
-    async addSubscription(payload) {
+    async addSubscription(baseUrl, topic) {
         const url = accountSubscriptionUrl(config.base_url);
-        const body = JSON.stringify(payload);
+        const body = JSON.stringify({
+            base_url: baseUrl,
+            topic: topic
+        });
         console.log(`[AccountApi] Adding user subscription ${url}: ${body}`);
         const response = await fetchOrThrow(url, {
             method: "POST",
@@ -187,9 +190,13 @@ class AccountApi {
         return subscription;
     }
 
-    async updateSubscription(remoteId, payload) {
-        const url = accountSubscriptionSingleUrl(config.base_url, remoteId);
-        const body = JSON.stringify(payload);
+    async updateSubscription(baseUrl, topic, payload) {
+        const url = accountSubscriptionUrl(config.base_url);
+        const body = JSON.stringify({
+            base_url: baseUrl,
+            topic: topic,
+            ...payload
+        });
         console.log(`[AccountApi] Updating user subscription ${url}: ${body}`);
         const response = await fetchOrThrow(url, {
             method: "PATCH",
@@ -201,12 +208,16 @@ class AccountApi {
         return subscription;
     }
 
-    async deleteSubscription(remoteId) {
-        const url = accountSubscriptionSingleUrl(config.base_url, remoteId);
+    async deleteSubscription(baseUrl, topic) {
+        const url = accountSubscriptionUrl(config.base_url);
         console.log(`[AccountApi] Removing user subscription ${url}`);
+        const headers = {
+            "X-BaseURL": baseUrl,
+            "X-Topic":  topic,
+        }
         await fetchOrThrow(url, {
             method: "DELETE",
-            headers: withBearerAuth({}, session.token())
+            headers: withBearerAuth(headers, session.token()),
         });
     }
 

web/src/app/SubscriptionManager.js

@@ -29,7 +29,6 @@ class SubscriptionManager {
             topic: topic,
             mutedUntil: 0,
             last: null,
-            remoteId: null,
             internal: internal || false
         };
         await db.subscriptions.put(subscription);
@@ -40,24 +39,23 @@ class SubscriptionManager {
         console.log(`[SubscriptionManager] Syncing subscriptions from remote`, remoteSubscriptions);
 
         // Add remote subscriptions
-        let remoteIds = [];
+        let remoteIds = []; // = topicUrl(baseUrl, topic)
         for (let i = 0; i < remoteSubscriptions.length; i++) {
             const remote = remoteSubscriptions[i];
-            const local = await this.add(remote.base_url, remote.topic);
+            const local = await this.add(remote.base_url, remote.topic, false);
             const reservation = remoteReservations?.find(r => remote.base_url === config.base_url && remote.topic === r.topic) || null;
             await this.update(local.id, {
-                remoteId: remote.id,
                 displayName: remote.display_name, // May be undefined
                 reservation: reservation // May be null!
             });
-            remoteIds.push(remote.id);
+            remoteIds.push(local.id);
         }
 
         // Remove local subscriptions that do not exist remotely
         const localSubscriptions = await db.subscriptions.toArray();
         for (let i = 0; i < localSubscriptions.length; i++) {
             const local = localSubscriptions[i];
-            const remoteExists = local.remoteId && remoteIds.includes(local.remoteId);
+            const remoteExists = remoteIds.includes(local.id);
             if (!local.internal && !remoteExists) {
                 await this.remove(local.id);
             }
@@ -174,12 +172,6 @@ class SubscriptionManager {
         });
     }
 
-    async setRemoteId(subscriptionId, remoteId) {
-        await db.subscriptions.update(subscriptionId, {
-            remoteId: remoteId
-        });
-    }
-
     async setReservation(subscriptionId, reservation) {
         await db.subscriptions.update(subscriptionId, {
             reservation: reservation

web/src/app/utils.js

@@ -23,7 +23,6 @@ export const accountPasswordUrl = (baseUrl) => `${baseUrl}/v1/account/password`;
 export const accountTokenUrl = (baseUrl) => `${baseUrl}/v1/account/token`;
 export const accountSettingsUrl = (baseUrl) => `${baseUrl}/v1/account/settings`;
 export const accountSubscriptionUrl = (baseUrl) => `${baseUrl}/v1/account/subscription`;
-export const accountSubscriptionSingleUrl = (baseUrl, id) => `${baseUrl}/v1/account/subscription/${id}`;
 export const accountReservationUrl = (baseUrl) => `${baseUrl}/v1/account/reservation`;
 export const accountReservationSingleUrl = (baseUrl, topic) => `${baseUrl}/v1/account/reservation/${topic}`;
 export const accountBillingSubscriptionUrl = (baseUrl) => `${baseUrl}/v1/account/billing/subscription`;

web/src/components/SubscribeDialog.js

@@ -299,11 +299,7 @@ export const subscribeTopic = async (baseUrl, topic) => {
     const subscription = await subscriptionManager.add(baseUrl, topic);
     if (session.exists()) {
         try {
-            const remoteSubscription = await accountApi.addSubscription({
-                base_url: baseUrl,
-                topic: topic
-            });
-            await subscriptionManager.setRemoteId(subscription.id, remoteSubscription.id);
+            await accountApi.addSubscription(baseUrl, topic);
         } catch (e) {
             console.log(`[SubscribeDialog] Subscribing to topic ${topic} failed`, e);
             if (e instanceof UnauthorizedError) {

web/src/components/SubscriptionPopup.js

@@ -109,9 +109,9 @@ export const SubscriptionPopup = (props) => {
     const handleUnsubscribe = async () => {
         console.log(`[SubscriptionPopup] Unsubscribing from ${props.subscription.id}`, props.subscription);
         await subscriptionManager.remove(props.subscription.id);
-        if (session.exists() && props.subscription.remoteId) {
+        if (session.exists() && !subscription.internal) {
             try {
-                await accountApi.deleteSubscription(props.subscription.remoteId);
+                await accountApi.deleteSubscription(props.subscription.baseUrl, props.subscription.topic);
             } catch (e) {
                 console.log(`[SubscriptionPopup] Error unsubscribing`, e);
                 if (e instanceof UnauthorizedError) {
@@ -198,10 +198,10 @@ const DisplayNameDialog = (props) => {
 
     const handleSave = async () => {
         await subscriptionManager.setDisplayName(subscription.id, displayName);
-        if (session.exists() && subscription.remoteId) {
+        if (session.exists() && !subscription.internal) {
             try {
                 console.log(`[SubscriptionSettingsDialog] Updating subscription display name to ${displayName}`);
-                await accountApi.updateSubscription(subscription.remoteId, { display_name: displayName });
+                await accountApi.updateSubscription(subscription.baseUrl, subscription.topic, { display_name: displayName });
             } catch (e) {
                 console.log(`[SubscriptionSettingsDialog] Error updating subscription`, e);
                 if (e instanceof UnauthorizedError) {

web/src/components/hooks.js

@@ -100,11 +100,7 @@ export const useAutoSubscribe = (subscriptions, selected) => {
                 const subscription = await subscriptionManager.add(baseUrl, params.topic);
                 if (session.exists()) {
                     try {
-                        const remoteSubscription = await accountApi.addSubscription({
-                            base_url: baseUrl,
-                            topic: params.topic
-                        });
-                        await subscriptionManager.setRemoteId(subscription.id, remoteSubscription.id);
+                        await accountApi.addSubscription(baseUrl, params.topic);
                     } catch (e) {
                         console.log(`[Hooks] Auto-subscribing failed`, e);
                         if (e instanceof UnauthorizedError) {