Post the browser's push subscription from every page, and register it once

Only room pages posted the subscription again, so a browser that
changed its password and closed the profile page went without
notifications until it next opened a room. A small controller in the
layout does it now, still once per session, and the bell is back to how
it was.

Two tabs posting at once after a password change could both miss the
row and insert two, so the endpoint is looked up again under the same
write lock as the session check.

From #371.

(cherry picked from commit 3786ee5244)

app/controllers/users/push_subscriptions_controller.rb

@@ -10,7 +10,7 @@ class Users::PushSubscriptionsController < ApplicationController
 
     # Existing endpoints must pass current validations too
     if subscription.valid?
-      head save_while_signed_in(subscription) ? :ok : :unauthorized
+      head register_while_signed_in(subscription) ? :ok : :unauthorized
     else
       head :unprocessable_entity
     end
@@ -27,12 +27,18 @@ class Users::PushSubscriptionsController < ApplicationController
     end
 
     # A password change ends every session and subscription. A request that authenticated just before it mustn't
-    # put one back, so the session is checked under the same write lock as the save. Validation, which looks up
-    # the endpoint's address, runs before taking the lock.
-    def save_while_signed_in(subscription)
+    # put one back, so the session is checked under the same write lock as the save, and the endpoint is looked up
+    # again there so two tabs posting at once register it once. Validation, which resolves the endpoint's address,
+    # runs before taking the lock.
+    def register_while_signed_in(subscription)
       Push::Subscription.transaction do
-        Session.exists?(Current.session.id) &&
-          (subscription.new_record? ? subscription.save!(validate: false) : subscription.touch)
+        if Session.exists?(Current.session.id)
+          if existing = @push_subscriptions.find_by(push_subscription_params)
+            existing.touch
+          else
+            subscription.save!(validate: false)
+          end
+        end
       end
     end
 

app/javascript/controllers/notifications_controller.js

@@ -4,10 +4,8 @@ import { pageIsTurboPreview } from "helpers/turbo_helpers"
 import { onNextEventLoopTick } from "helpers/timing_helpers"
 import { getCookie, setCookie } from "lib/cookie"
 
-const PUSH_SUBSCRIPTION_SYNCED_KEY = "push-subscription-synced-session"
-
 export default class extends Controller {
-  static values = { subscriptionsUrl: String, session: String }
+  static values = { subscriptionsUrl: String }
   static targets = [ "notAllowedNotice", "bell", "details" ]
   static classes = [ "attention" ]
 
@@ -27,8 +25,6 @@ export default class extends Controller {
           this.#showBellAlert()
         }
       }
-
-      this.#resyncPushSubscriptionOncePerSession()
     }
   }
 
@@ -113,21 +109,6 @@ export default class extends Controller {
     if (!response.ok) subscription.unsubscribe()
   }
 
-  // A password change removes the account's subscriptions, the one this browser holds included. Post it again
-  // once per session, so a browser that's still signed in keeps getting notifications. A failure here leaves the
-  // browser's own subscription alone: the next session tries again.
-  async #resyncPushSubscriptionOncePerSession() {
-    if (this.#allowed && this.sessionValue && localStorage.getItem(PUSH_SUBSCRIPTION_SYNCED_KEY) !== this.sessionValue) {
-      const registration = await this.#serviceWorkerRegistration
-      const subscription = await registration?.pushManager?.getSubscription()
-
-      if (Notification.permission === "granted" && subscription) {
-        await post(this.subscriptionsUrlValue, { body: this.#extractJsonPayloadAsString(subscription), responseKind: "turbo-stream" })
-        localStorage.setItem(PUSH_SUBSCRIPTION_SYNCED_KEY, this.sessionValue)
-      }
-    }
-  }
-
   async #requestPermissionAndSubscribe(registration) {
     const permission = await Notification.requestPermission()
     if (permission === "granted") this.#subscribe(registration)

app/javascript/controllers/push_subscription_sync_controller.js

@@ -0,0 +1,32 @@
+import { Controller } from "@hotwired/stimulus"
+import { post } from "@rails/request.js"
+
+const SYNCED_SESSION_KEY = "push-subscription-synced-session"
+
+// A password change removes the account's push subscriptions, the one this browser holds included. Post it again
+// once per session, so a browser that's still signed in keeps getting notifications. A failure here leaves the
+// browser's own subscription alone: the next session tries again.
+export default class extends Controller {
+  static values = { url: String, session: String }
+
+  async connect() {
+    if (this.#allowed && this.sessionValue && localStorage.getItem(SYNCED_SESSION_KEY) !== this.sessionValue) {
+      const registration = await navigator.serviceWorker.getRegistration(window.location.origin)
+      const subscription = await registration?.pushManager?.getSubscription()
+
+      if (Notification.permission === "granted" && subscription) {
+        await post(this.urlValue, { body: this.#payload(subscription), responseKind: "turbo-stream" })
+        localStorage.setItem(SYNCED_SESSION_KEY, this.sessionValue)
+      }
+    }
+  }
+
+  get #allowed() {
+    return navigator.serviceWorker && window.Notification
+  }
+
+  #payload(subscription) {
+    const { endpoint, keys: { p256dh, auth } } = subscription.toJSON()
+    return JSON.stringify({ push_subscription: { endpoint, p256dh_key: p256dh, auth_key: auth } })
+  }
+}

app/views/layouts/application.html.erb

@@ -63,6 +63,11 @@
 
     <%= render "layouts/lightbox" %>
 
+    <% if Current.session %>
+      <%= tag.div hidden: true, data: { controller: "push-subscription-sync",
+            push_subscription_sync_url_value: user_push_subscriptions_path, push_subscription_sync_session_value: push_subscription_session_marker } %>
+    <% end %>
+
     <a href="https://once.com" id="app-logo" target="_blank" aria-label="Once software from 37signals home page">
       <%= image_tag "campfire-icon.png", alt: "Campfire logo", width: 256, height: 216 %>
     </a>

app/views/rooms/involvements/_bell.html.erb

@@ -1,7 +1,6 @@
 <span>
   <span class="button_to_change_notifying"
-      data-controller="notifications" data-notifications-subscriptions-url-value="<%= user_push_subscriptions_path %>"
-      data-notifications-session-value="<%= push_subscription_session_marker %>" data-notifications-attention-class="btn--pulsing">
+      data-controller="notifications" data-notifications-subscriptions-url-value="<%= user_push_subscriptions_path %>" data-notifications-attention-class="btn--pulsing">
     <%= turbo_frame_tag dom_id(room, :involvement), data: {
           controller: "turbo-frame", action: "notifications:ready@window->turbo-frame#load", turbo_frame_url_param: room_involvement_path(room) } do %>
       <button class="btn" data-action="click->notifications#attemptToSubscribe" data-notifications-target="bell">

test/controllers/users/push_subscriptions_controller_test.rb

@@ -106,16 +106,32 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest
     assert_response :unauthorized
   end
 
-  test "a room names its session for the browser to post its subscription again in a new one" do
+  test "pages name their session for the browser to post its subscription again in a new one" do
+    get user_profile_url
+    assert_equal user_push_subscriptions_path, css_select("[data-push-subscription-sync-url-value]").first["data-push-subscription-sync-url-value"]
+
     get room_url(rooms(:watercooler))
-    first_marker = css_select("[data-notifications-session-value]").first["data-notifications-session-value"]
+    first_marker = css_select("[data-push-subscription-sync-session-value]").first["data-push-subscription-sync-session-value"]
 
     assert_match(/\A\h{64}\z/, first_marker)
     assert_not_includes response.body, users(:david).sessions.order(:id).last.token
 
     sign_in :david
     get room_url(rooms(:watercooler))
-    assert_not_equal first_marker, css_select("[data-notifications-session-value]").first["data-notifications-session-value"]
+    assert_not_equal first_marker, css_select("[data-push-subscription-sync-session-value]").first["data-push-subscription-sync-session-value"]
+  end
+
+  test "two posts of the same new subscription at once register it once" do
+    Push::Subscription.any_instance.stubs(:valid?).with do
+      users(:david).push_subscriptions.build(new_subscription_params).save!(validate: false) unless Push::Subscription.exists?(endpoint: new_subscription_params["endpoint"])
+      true
+    end.returns(true)
+
+    assert_difference -> { Push::Subscription.where(endpoint: new_subscription_params["endpoint"]).count }, +1 do
+      post user_push_subscriptions_url, params: { push_subscription: new_subscription_params }
+    end
+
+    assert_response :ok
   end
 
   private

test/system/push_subscription_resync_test.rb

@@ -21,11 +21,10 @@ class PushSubscriptionResyncTest < ApplicationSystemTestCase
     join_room rooms(:hq)
     assert_equal first_sync, subscription.reload.updated_at, "posted again in the same session"
 
+    first_id = subscription.id
     change_password
-    assert_nil Push::Subscription.find_by(endpoint: ENDPOINT)
-
-    join_room rooms(:hq)
     assert_subscription_registered
+    assert_not_equal first_id, subscription.id, "not removed and posted again by the profile page"
     assert_not page.evaluate_script("localStorage.getItem('unsubscribed-for-test')")
   end
 
@@ -65,7 +64,7 @@ class PushSubscriptionResyncTest < ApplicationSystemTestCase
     end
 
     def assert_subscription_registered
-      assert_selector "[data-controller~=notifications]", visible: :all
+      assert_selector "[data-controller~=push-subscription-sync]", visible: :all
       Timeout.timeout(5) { sleep 0.1 until Push::Subscription.exists?(endpoint: ENDPOINT, user: users(:david)) }
     end