From cc309e87e9bf0d73d29ca6b0d828028aebafc90a Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Sun, 12 Feb 2023 14:09:44 -0500 Subject: [PATCH] Remove awkward subscription id --- server/errors.go | 1 + server/server.go | 7 +-- server/server_account.go | 79 +++++++------------------ server/server_account_test.go | 9 ++- user/manager_test.go | 2 - user/types.go | 9 ++- web/src/app/AccountApi.js | 27 ++++++--- web/src/app/SubscriptionManager.js | 16 ++--- web/src/app/utils.js | 1 - web/src/components/SubscribeDialog.js | 6 +- web/src/components/SubscriptionPopup.js | 8 +-- web/src/components/hooks.js | 6 +- 12 files changed, 67 insertions(+), 104 deletions(-) diff --git a/server/errors.go b/server/errors.go index ddc3462..a00105c 100644 --- a/server/errors.go +++ b/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", ""} diff --git a/server/server.go b/server/server.go index 775319e..288b138 100644 --- a/server/server.go +++ b/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) diff --git a/server/server_account.go b/server/server_account.go index e2a9ee8..d7dbd98 100644 --- a/server/server_account.go +++ b/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 diff --git a/server/server_account_test.go b/server/server_account_test.go index c68dd8e..cc4f4cd 100644 --- a/server/server_account_test.go +++ b/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) diff --git a/user/manager_test.go b/user/manager_test.go index 18144aa..f809b5a 100644 --- a/user/manager_test.go +++ b/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) diff --git a/user/types.go b/user/types.go index 6df1c26..0363c97 100644 --- a/user/types.go +++ b/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"` diff --git a/web/src/app/AccountApi.js b/web/src/app/AccountApi.js index 8a78e27..6382d1f 100644 --- a/web/src/app/AccountApi.js +++ b/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()), }); } diff --git a/web/src/app/SubscriptionManager.js b/web/src/app/SubscriptionManager.js index ef9ff42..cdfe50e 100644 --- a/web/src/app/SubscriptionManager.js +++ b/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 diff --git a/web/src/app/utils.js b/web/src/app/utils.js index c53a0f3..88f67ce 100644 --- a/web/src/app/utils.js +++ b/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`; diff --git a/web/src/components/SubscribeDialog.js b/web/src/components/SubscribeDialog.js index b24501b..7ea0052 100644 --- a/web/src/components/SubscribeDialog.js +++ b/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) { diff --git a/web/src/components/SubscriptionPopup.js b/web/src/components/SubscriptionPopup.js index e0f3cd5..b33313c 100644 --- a/web/src/components/SubscriptionPopup.js +++ b/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) { diff --git a/web/src/components/hooks.js b/web/src/components/hooks.js index c6f85df..b1ce8ff 100644 --- a/web/src/components/hooks.js +++ b/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) {