diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index a3517a8..6b33452 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/app/controllers/users/push_subscriptions_controller.rb @@ -28,15 +28,17 @@ class Users::PushSubscriptionsController < ApplicationController # 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, 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. + # again there, past the query cache, 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 - if Session.exists?(Current.session.id) - if existing = @push_subscriptions.find_by(push_subscription_params) - existing.touch - else - subscription.save!(validate: false) + Push::Subscription.uncached do + Push::Subscription.transaction do + 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 diff --git a/app/javascript/controllers/notifications_controller.js b/app/javascript/controllers/notifications_controller.js index 3ce53a5..f7dfe7e 100644 --- a/app/javascript/controllers/notifications_controller.js +++ b/app/javascript/controllers/notifications_controller.js @@ -106,7 +106,8 @@ export default class extends Controller { async #syncPushSubscription(subscription) { const response = await post(this.subscriptionsUrlValue, { body: this.#extractJsonPayloadAsString(subscription), responseKind: "turbo-stream" }) - if (!response.ok) subscription.unsubscribe() + // Only a refused endpoint is for good. A session signed out mid-request posts it again after signing in. + if (response.unprocessableEntity) subscription.unsubscribe() } async #requestPermissionAndSubscribe(registration) { diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb index 7815626..cbfacb4 100644 --- a/test/controllers/users/push_subscriptions_controller_test.rb +++ b/test/controllers/users/push_subscriptions_controller_test.rb @@ -139,3 +139,33 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest { "endpoint" => "https://fcm.googleapis.com/fcm/send/abc123", "p256dh_key" => "123", "auth_key" => "456" } end end + +class Users::PushSubscriptionsControllerForeignWriteTest < ActionDispatch::IntegrationTest + self.use_transactional_tests = false + + setup do + sign_in :david + stub_web_push_dns_resolution + end + + test "a subscription another connection registers while this one validates isn't registered twice" do + params = { "endpoint" => "https://fcm.googleapis.com/fcm/send/foreign", "p256dh_key" => "123", "auth_key" => "456" } + Push::Subscription.any_instance.stubs(:valid?).with { foreign_insert(params); true }.returns(true) + + post user_push_subscriptions_url, params: { push_subscription: params } + + assert_response :ok + assert_equal 1, Push::Subscription.where(endpoint: params["endpoint"]).count + end + + private + def foreign_insert(params) + unless @inserted + SQLite3::Database.new(ActiveRecord::Base.connection_db_config.database) do |database| + database.execute "INSERT INTO push_subscriptions (user_id, endpoint, p256dh_key, auth_key, created_at, updated_at) " \ + "VALUES (?, ?, ?, ?, datetime('now'), datetime('now'))", [ users(:david).id, *params.values ] + end + @inserted = true + end + end +end