Merge pull request #371 from basecamp/sign-out-elsewhere-on-password-change

Sign out the account's other sessions when its password changes

app/controllers/users/profiles_controller.rb

@@ -1,14 +1,23 @@
 class Users::ProfilesController < ApplicationController
   before_action :set_user
 
+  # Limited like signing in, so the current password can't be guessed here instead.
+  rate_limit to: 10, within: 3.minutes, only: :update, if: :changing_password?,
+    with: -> { redirect_to user_profile_url, alert: "Too many attempts. Try again in a few minutes." }
+
   def show
     @direct_memberships, @shared_memberships =
       Current.user.memberships.with_ordered_room.partition { |m| m.room.direct? }
   end
 
   def update
-    @user.update user_params
-    redirect_to user_profile_url, notice: update_notice
+    if @user.update user_params
+      start_new_session_for @user if @user.saved_change_to_password_digest?
+
+      redirect_to user_profile_url, notice: update_notice
+    else
+      redirect_to user_profile_url, alert: update_alert
+    end
   end
 
   private
@@ -17,10 +26,31 @@ class Users::ProfilesController < ApplicationController
     end
 
     def user_params
-      params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact
+      permitted = params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact
+
+      if changing_password?
+        permitted.merge(password_challenge: params[:user][:password_challenge].to_s)
+      else
+        permitted
+      end
+    end
+
+    # As has_secure_password reads it: an empty password leaves the current one alone.
+    def changing_password?
+      !params.dig(:user, :password).to_s.empty?
     end
 
     def update_notice
       params[:user][:avatar] ? "It may take up to 30 minutes to change everywhere." : "✓"
     end
+
+    def update_alert
+      if !@user.errors.include?(:password_challenge)
+        @user.errors.full_messages.to_sentence
+      elsif params[:user][:password_challenge].blank?
+        "Enter your current password to change it."
+      else
+        "That isn't your current password."
+      end
+    end
 end

app/controllers/users/push_subscriptions_controller.rb

@@ -5,17 +5,14 @@ class Users::PushSubscriptionsController < ApplicationController
   end
 
   def create
-    if subscription = @push_subscriptions.find_by(push_subscription_params)
-      # Existing endpoints must pass current validations
-      if subscription.valid?
-        subscription.touch
-        head :ok
-      else
-        head :unprocessable_entity
-      end
+    subscription = @push_subscriptions.find_or_initialize_by(push_subscription_params)
+    subscription.user_agent = request.user_agent if subscription.new_record?
+
+    # Existing endpoints must pass current validations too
+    if subscription.valid?
+      head register_while_signed_in(subscription) ? :ok : :unauthorized
     else
-      subscription = @push_subscriptions.create push_subscription_params.merge(user_agent: request.user_agent)
-      head subscription.persisted? ? :ok : :unprocessable_entity
+      head :unprocessable_entity
     end
   end
 
@@ -29,6 +26,24 @@ class Users::PushSubscriptionsController < ApplicationController
       @push_subscriptions = Current.user.push_subscriptions
     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, and the endpoint is looked up
+    # 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.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
+    end
+
     def push_subscription_params
       params.require(:push_subscription).permit(:endpoint, :p256dh_key, :auth_key)
     end

app/helpers/translations_helper.rb

@@ -2,6 +2,7 @@ module TranslationsHelper
   TRANSLATIONS = {
     email_address:  { "🇺🇸": "Enter your email address", "🇪🇸": "Introduce tu correo electrónico", "🇫🇷": "Entrez votre adresse courriel", "🇮🇳": "अपना ईमेल पता दर्ज करें", "🇩🇪": "Geben Sie Ihre E-Mail-Adresse ein", "🇧🇷": "Insira seu endereço de email", "🇯🇵": "メールアドレスを入力してください" },
     password: { "🇺🇸": "Enter your password", "🇪🇸": "Introduce tu contraseña", "🇫🇷": "Saisissez votre mot de passe", "🇮🇳": "अपना पासवर्ड दर्ज करें", "🇩🇪": "Geben Sie Ihr Passwort ein", "🇧🇷": "Insira sua senha", "🇯🇵": "パスワードを入力してください" },
+    current_password: { "🇺🇸": "Enter your current password", "🇪🇸": "Introduce tu contraseña actual", "🇫🇷": "Saisissez votre mot de passe actuel", "🇮🇳": "अपना वर्तमान पासवर्ड दर्ज करें", "🇩🇪": "Geben Sie Ihr aktuelles Passwort ein", "🇧🇷": "Insira sua senha atual", "🇯🇵": "現在のパスワードを入力してください" },
     update_password: { "🇺🇸": "Change password", "🇪🇸": "Cambiar contraseña", "🇫🇷": "Changer le mot de passe", "🇮🇳": "पासवर्ड बदलें", "🇩🇪": "Passwort ändern", "🇧🇷": "Alterar senha", "🇯🇵": "パスワードを変更" },
     user_name: { "🇺🇸": "Enter your name", "🇪🇸": "Introduce tu nombre", "🇫🇷": "Entrez votre nom", "🇮🇳": "अपना नाम दर्ज करें", "🇩🇪": "Geben Sie Ihren Namen ein", "🇧🇷": "Insira seu nome", "🇯🇵": "お名前を入力してください" },
     account_name: { "🇺🇸": "Name this account", "🇪🇸": "Nombre de esta cuenta", "🇫🇷": "Nommez ce compte", "🇮🇳": "इस खाते का नाम दें", "🇩🇪": "Benennen Sie dieses Konto", "🇧🇷": "Dê um nome a essa conta", "🇯🇵": "アカウントに名前を付ける" },

app/helpers/users/push_subscriptions_helper.rb

@@ -0,0 +1,7 @@
+module Users::PushSubscriptionsHelper
+  # Names the session without revealing its token, so the browser can tell when it's in a new one. The creation
+  # time keeps an id reused by a new database from looking like the same session.
+  def push_subscription_session_marker
+    Digest::SHA256.hexdigest("push-subscription-session-#{Current.session.id}-#{Current.session.created_at.to_f}") if Current.session
+  end
+end

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) {

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/models/user.rb

@@ -18,8 +18,10 @@ class User < ApplicationRecord
   enum :status, %i[ active deactivated banned ], default: :active
 
   has_secure_password validations: false
+  validate :password_challenge_matches, unless: -> { password_challenge.nil? }
 
   after_create_commit :grant_membership_to_open_rooms
+  after_update :sign_out_all_sessions, if: :saved_change_to_password_digest?
 
   scope :ordered, -> { order("LOWER(name)") }
   scope :filtered_by, ->(query) { where("name like ?", "%#{query}%") }
@@ -50,6 +52,30 @@ class User < ApplicationRecord
   end
 
   private
+    # The profile sets password_challenge whenever it changes the password; scripts that reset one don't.
+    def password_challenge_matches
+      unless password_digest_was.present? && BCrypt::Password.new(password_digest_was).is_password?(password_challenge)
+        errors.add :password_challenge, :invalid
+      end
+    end
+
+    # A new password is how someone shuts out whoever else got into their account, so it ends every session, copies
+    # of the changing browser's cookie included, and every push subscription; browsers still signed in post theirs
+    # again. Connections close once that commits, registered on the transaction because a later save in it would
+    # replace the saved change an after_commit condition reads.
+    def sign_out_all_sessions
+      sessions.delete_all
+      push_subscriptions.delete_all
+      self.class.current_transaction.after_commit { disconnect_signed_out_sessions }
+    end
+
+    # The password is saved by now, so a realtime service that can't be reached mustn't fail the change.
+    def disconnect_signed_out_sessions
+      reset_remote_connections
+    rescue => error
+      Rails.logger.warn "Could not disconnect remote connections after a password change: #{error.class}"
+    end
+
     # One statement, which SQLite runs under the write lock, so no room is closed between reading the open ones and
     # inserting. A room that Room#destroy_later closes is either closed before, and skipped, or after, and takes this
     # membership away with the others. A room that granted itself to every user when it was created is skipped too,

app/models/user/transferable.rb

@@ -3,13 +3,20 @@ module User::Transferable
 
   TRANSFER_LINK_EXPIRY_DURATION = 4.hours
 
+  included do
+    # Bound to the password, so changing it also ends sign-in links made before.
+    generates_token_for :transfer, expires_in: TRANSFER_LINK_EXPIRY_DURATION do
+      password_salt&.last(10)
+    end
+  end
+
   class_methods do
     def find_by_transfer_id(id)
-      find_signed(id, purpose: :transfer)
+      find_by_token_for(:transfer, id)
     end
   end
 
   def transfer_id
-    signed_id(purpose: :transfer, expires_in: TRANSFER_LINK_EXPIRY_DURATION)
+    generate_token_for(:transfer)
   end
 end

app/views/layouts/application.html.erb

@@ -62,6 +62,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/users/profiles/show.html.erb

@@ -69,6 +69,16 @@
         </label>
       </div>
 
+      <div class="flex align-center gap">
+        <%= translation_button(:current_password) %>
+
+        <label class="flex align-center gap flex-item-grow input input--actor">
+          <%= form.password_field :password_challenge, class: "input txt-large", autocomplete: "current-password",
+                placeholder: "Current password", required: false, maxlength: 72 %>
+          <%= image_tag "password.svg", aria: { hidden: "true" }, size: 24, class: "colorize--black" %>
+        </label>
+      </div>
+
       <div class="flex align-center gap">
         <%= translation_button(:update_password) %>
 

test/controllers/cached_responses_test.rb

@@ -136,6 +136,29 @@ class CachedResponsesTest < ActionDispatch::IntegrationTest
     assert_redirected_to new_session_url
   end
 
+  test "a session signed out by a password change isn't served from a cached snapshot or page" do
+    other_device = open_session { |session| session.host! "once.campfire.test" }
+    other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" }
+    other_device.get user_sidebar_url(:me)
+    other_device.assert_response :success
+
+    Session.expects(:find_by).never
+    ResponseCache.instance.expects(:write).never
+    other_device.get user_sidebar_url(:me)
+    other_device.assert_response :success
+    Session.unstub(:find_by)
+    ResponseCache.instance.unstub(:write)
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } },
+      headers: { "Sec-Fetch-Site" => "same-origin", "Origin" => "http://once.campfire.test" }
+    assert_redirected_to user_profile_url
+
+    other_device.get user_sidebar_url(:me)
+    other_device.assert_redirected_to new_session_url
+    get user_sidebar_url(:me)
+    assert_response :success
+  end
+
   test "conditional requests keep native validators" do
     get room_messages_url(@room)
     etag = response.headers["ETag"]

test/controllers/sessions/transfers_controller_test.rb

@@ -19,4 +19,15 @@ class Sessions::TransfersControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to root_url
     assert parsed_cookies.signed[:session_token]
   end
+
+  test "update rejects a link made before the password changed" do
+    user = users(:david)
+    transfer_id = user.transfer_id
+    user.update!(password: "a new secret123456")
+
+    put session_transfer_url(transfer_id)
+
+    assert_response :bad_request
+    assert_nil parsed_cookies.signed[:session_token]
+  end
 end

test/controllers/users/profiles_controller_test.rb

@@ -27,9 +27,148 @@ class Users::ProfilesControllerTest < ActionDispatch::IntegrationTest
     assert_equal "david@37signals.com", users(:david).email_address
   end
 
+  test "changing the password signs out the account's other devices but not this browser" do
+    other_device = open_session
+    other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" }
+    other_device.get user_profile_url
+    other_device.assert_response :success
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+    assert_redirected_to user_profile_url
+
+    other_device.get user_profile_url
+    other_device.assert_redirected_to new_session_url
+    get user_profile_url
+    assert_response :success
+    assert_equal 1, users(:david).sessions.count
+  end
+
+  test "changing the password gives this browser a new session, so a copy of its cookie stops working" do
+    copied_cookie = cookies[:session_token]
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+    assert_not_equal copied_cookie, cookies[:session_token]
+
+    copy = open_session
+    copy.cookies[:session_token] = copied_cookie
+    copy.get user_profile_url
+    copy.assert_redirected_to new_session_url
+  end
+
+  test "changing the password along with the avatar still signs this browser back in and closes live connections" do
+    remote_connections = mock
+    remote_connections.expects(:disconnect).with(reconnect: true)
+    ActionCable.server.stubs(:remote_connections).returns(mock.tap { |m| m.expects(:where).with(current_user: users(:david)).returns(remote_connections) })
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456", avatar: fixture_file_upload("moon.jpg", "image/jpeg") } }
+    assert_redirected_to user_profile_url
+    assert users(:david).reload.avatar.attached?
+
+    get user_profile_url
+    assert_response :success
+  end
+
+  test "changing the password closes the account's live connections" do
+    remote_connections = mock
+    remote_connections.expects(:disconnect).with(reconnect: true)
+    ActionCable.server.stubs(:remote_connections).returns(mock.tap { |m| m.expects(:where).with(current_user: users(:david)).returns(remote_connections) })
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+
+    assert_redirected_to user_profile_url
+  end
+
+  test "changing the password still signs out other sessions when the realtime service is unreachable" do
+    other_session = users(:david).sessions.first
+    ActionCable.server.stubs(:remote_connections).raises(RuntimeError.new("cable down"))
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+
+    assert_redirected_to user_profile_url
+    assert_not Session.exists?(other_session.id)
+  end
+
+  test "updating the profile without a new password leaves other sessions signed in" do
+    ActionCable.server.expects(:remote_connections).never
+
+    assert_no_difference -> { users(:david).sessions.count } do
+      put user_profile_url, params: { user: { name: "David", password: "" } }
+    end
+
+    assert_redirected_to user_profile_url
+  end
+
+  test "changing the password with the current one" do
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+
+    assert_redirected_to user_profile_url
+    assert_equal "✓", flash[:notice]
+    assert users(:david).reload.authenticate("a new secret123456")
+  end
+
+  test "changing the password without the current one changes nothing" do
+    assert_password_change_refused "Enter your current password to change it.",
+      user: { name: "Someone else", password: "a new secret123456" }
+  end
+
+  test "changing the password with a wrong current one changes nothing" do
+    assert_password_change_refused "That isn't your current password.",
+      user: { name: "Someone else", password: "a new secret123456", password_challenge: "not my password" }
+  end
+
+  test "editing the profile without changing the password doesn't need the current one" do
+    put user_profile_url, params: { user: { name: "Dave", bio: "Acrobat", password: "" } }
+
+    assert_redirected_to user_profile_url
+    assert_equal "Dave", users(:david).reload.name
+    assert users(:david).authenticate("secret123456")
+  end
+
+  test "attempts to change the password are rate limited like signing in" do
+    Users::ProfilesController.cache_store.stubs(:increment).returns(11)
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+    assert_redirected_to user_profile_url
+    assert_equal "Too many attempts. Try again in a few minutes.", flash[:alert]
+    assert users(:david).reload.authenticate("secret123456")
+
+    put user_profile_url, params: { user: { name: "Dave" } }
+    assert_equal "Dave", users(:david).reload.name
+  end
+
+  test "changing the password removes the other devices' push subscriptions, and this browser can register its own again" do
+    stub_web_push_dns_resolution
+    endpoint = push_subscriptions(:david_chrome).attributes.slice("endpoint", "p256dh_key", "auth_key")
+
+    put user_profile_url, params: { user: { password: "a new secret123456", password_challenge: "secret123456" } }
+    assert_empty users(:david).push_subscriptions
+
+    post user_push_subscriptions_url, params: { push_subscription: endpoint }
+    assert_response :ok
+    assert_equal [ endpoint["endpoint"] ], users(:david).push_subscriptions.pluck(:endpoint)
+  end
+
   test "updates are limited to the current user" do
     put user_profile_url(users(:jason)), params: { user: { name: "John Doe" } }
 
     assert_equal "Jason", users(:jason).reload.name
   end
+
+  private
+    def assert_password_change_refused(alert, params)
+      other_device = open_session
+      other_device.post session_url, params: { email_address: users(:david).email_address, password: "secret123456" }
+      ActionCable.server.expects(:remote_connections).never
+
+      assert_no_changes -> { [ Session.count, Push::Subscription.count ] } do
+        put user_profile_url, params: params
+      end
+
+      assert_redirected_to user_profile_url
+      assert_equal alert, flash[:alert]
+      assert users(:david).reload.authenticate("secret123456")
+      assert_equal "David", users(:david).name
+      other_device.get user_profile_url
+      other_device.assert_response :success
+    end
 end

test/controllers/users/push_subscriptions_controller_test.rb

@@ -74,4 +74,98 @@ class Users::PushSubscriptionsControllerTest < ActionDispatch::IntegrationTest
       assert_redirected_to user_push_subscriptions_url
     end
   end
+
+  test "a signed-out session can't register a subscription" do
+    Session.delete_all
+
+    assert_no_difference -> { Push::Subscription.count } do
+      post user_push_subscriptions_url, params: { push_subscription: new_subscription_params }
+    end
+
+    assert_redirected_to new_session_url
+  end
+
+  test "a session signed out while its request is in flight doesn't register a subscription" do
+    Push::Subscription.any_instance.stubs(:valid?).with { Session.delete_all; true }.returns(true)
+
+    assert_no_difference -> { Push::Subscription.count } do
+      post user_push_subscriptions_url, params: { push_subscription: new_subscription_params }
+    end
+
+    assert_response :unauthorized
+  end
+
+  test "a session signed out while its request is in flight doesn't keep an existing subscription alive" do
+    subscription = push_subscriptions(:david_chrome)
+    Push::Subscription.any_instance.stubs(:valid?).with { Session.delete_all; true }.returns(true)
+
+    assert_no_changes -> { subscription.reload.updated_at } do
+      post user_push_subscriptions_url, params: { push_subscription: subscription.attributes.slice("endpoint", "p256dh_key", "auth_key") }
+    end
+
+    assert_response :unauthorized
+  end
+
+  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-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-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
+    def new_subscription_params
+      { "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

test/models/user/transferable_test.rb

@@ -0,0 +1,40 @@
+require "test_helper"
+
+class User::TransferableTest < ActiveSupport::TestCase
+  test "a transfer id finds its user" do
+    assert_equal users(:david), User.find_by_transfer_id(users(:david).transfer_id)
+  end
+
+  test "a transfer id expires" do
+    transfer_id = users(:david).transfer_id
+
+    travel User::TRANSFER_LINK_EXPIRY_DURATION + 1.second
+
+    assert_nil User.find_by_transfer_id(transfer_id)
+  end
+
+  test "a transfer id made before a password change stops working" do
+    transfer_id = users(:david).transfer_id
+
+    users(:david).update!(password: "a new secret123456")
+
+    assert_nil User.find_by_transfer_id(transfer_id)
+    assert_equal users(:david), User.find_by_transfer_id(users(:david).transfer_id)
+  end
+
+  test "other changes keep a transfer id working" do
+    transfer_id = users(:david).transfer_id
+
+    users(:david).update!(name: "Dave", email_address: "dave@37signals.com")
+
+    assert_equal users(:david), User.find_by_transfer_id(transfer_id)
+  end
+
+  test "a transfer id finds only users in scope" do
+    transfer_id = users(:david).transfer_id
+
+    users(:david).deactivate
+
+    assert_nil User.active.find_by_transfer_id(transfer_id)
+  end
+end

test/models/user_test.rb

@@ -42,6 +42,70 @@ class UserTest < ActiveSupport::TestCase
     end
   end
 
+  test "changing the password signs out every session, the current one included" do
+    Current.session = users(:david).sessions.start!(user_agent: "This device", ip_address: "127.0.0.1")
+
+    assert_changes -> { users(:david).sessions.count }, to: 0 do
+      users(:david).update!(password: "a new secret123456")
+    end
+  end
+
+  test "changing the password closes the account's live connections" do
+    remote_connections = mock
+    remote_connections.expects(:disconnect).with(reconnect: true)
+    ActionCable.server.stubs(:remote_connections).returns(mock.tap { |m| m.expects(:where).with(current_user: users(:david)).returns(remote_connections) })
+
+    users(:david).update!(password: "a new secret123456")
+  end
+
+  test "changing the password closes live connections even when the user is saved again before commit" do
+    remote_connections = mock
+    remote_connections.expects(:disconnect).with(reconnect: true)
+    ActionCable.server.stubs(:remote_connections).returns(mock.tap { |m| m.expects(:where).with(current_user: users(:david)).returns(remote_connections) })
+
+    User.transaction do
+      users(:david).update!(password: "a new secret123456")
+      users(:david).update!(name: "Dave")
+    end
+  end
+
+  test "a password change that rolls back keeps every session and connection" do
+    ActionCable.server.expects(:remote_connections).never
+
+    assert_no_changes -> { users(:david).sessions.count } do
+      User.transaction do
+        users(:david).update!(password: "a new secret123456")
+        raise ActiveRecord::Rollback
+      end
+    end
+  end
+
+  test "changing the password removes every push subscription of the user" do
+    assert users(:david).push_subscriptions.any?
+
+    users(:david).update!(password: "a new secret123456")
+
+    assert_empty users(:david).push_subscriptions.reload
+    assert users(:jason).push_subscriptions.any?
+  end
+
+  test "a password change that rolls back keeps push subscriptions" do
+    assert_no_changes -> { users(:david).push_subscriptions.count } do
+      User.transaction do
+        users(:david).update!(password: "a new secret123456")
+        raise ActiveRecord::Rollback
+      end
+    end
+  end
+
+  test "changing other details keeps every session" do
+    ActionCable.server.expects(:remote_connections).never
+
+    assert_no_changes -> { [ users(:david).sessions.count, users(:david).push_subscriptions.count ] } do
+      users(:david).update!(name: "Dave", email_address: "dave@37signals.com")
+    end
+  end
+
   private
     def create_new_user
       User.create!(name: "User", email_address: "user@example.com", password: "secret123456")

test/script/admin/reset_password_test.rb

@@ -0,0 +1,21 @@
+require "test_helper"
+
+class ResetPasswordScriptTest < ActiveSupport::TestCase
+  test "resets a password without the current one and signs out every session" do
+    assert_output "Password has been reset\n" do
+      run_script "david@37signals.com", "a new secret123456"
+    end
+
+    assert users(:david).reload.authenticate("a new secret123456")
+    assert_empty users(:david).sessions
+  end
+
+  private
+    def run_script(*arguments)
+      original_arguments = ARGV.dup
+      ARGV.replace(arguments)
+      load Rails.root.join("script/admin/reset-password").to_s
+    ensure
+      ARGV.replace(original_arguments)
+    end
+end

test/system/push_subscription_resync_test.rb

@@ -0,0 +1,74 @@
+require "application_system_test_case"
+
+class PushSubscriptionResyncTest < ApplicationSystemTestCase
+  ENDPOINT = "https://fcm.googleapis.com/fcm/send/system-test"
+
+  setup do
+    stub_web_push_dns_resolution
+    fake_a_browser_push_subscription
+  end
+
+  teardown do
+    page.driver.browser.execute_cdp "Page.removeScriptToEvaluateOnNewDocument", identifier: @fake_subscription_script
+    page.execute_script("localStorage.clear()") if page.current_url.start_with?("http")
+  end
+
+  test "a browser posts its subscription again once per session, so it survives a password change" do
+    sign_in "david@37signals.com"
+    assert_subscription_registered
+    first_sync = subscription.updated_at
+
+    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_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
+
+  test "a subscription the server refuses stays in the browser" do
+    stub_dns_resolution("169.254.169.254")
+
+    sign_in "david@37signals.com"
+    Timeout.timeout(5) { sleep 0.1 until page.evaluate_script("localStorage.getItem('push-subscription-synced-session')") }
+    join_room rooms(:hq)
+
+    assert_not Push::Subscription.exists?(endpoint: ENDPOINT)
+    assert_not page.evaluate_script("localStorage.getItem('unsubscribed-for-test')")
+  end
+
+  private
+    # Headless Chrome has no push service, so stand in for the subscription the service worker would hold. The
+    # browser is shared with other system tests, so teardown removes this again.
+    def fake_a_browser_push_subscription
+      @fake_subscription_script = page.driver.browser.execute_cdp("Page.addScriptToEvaluateOnNewDocument", source: <<~JS)["identifier"]
+        (() => {
+          Object.defineProperty(Notification, "permission", { get: () => "granted" })
+          const subscription = {
+            toJSON: () => ({ endpoint: "#{ENDPOINT}", keys: { p256dh: "p256dh-key", auth: "auth-key" } }),
+            unsubscribe: async () => { localStorage.setItem("unsubscribed-for-test", "true") }
+          }
+          navigator.serviceWorker.getRegistration = async () => ({ pushManager: { getSubscription: async () => subscription } })
+        })()
+      JS
+    end
+
+    def change_password
+      visit user_profile_url
+      find("input[name='user[password_challenge]']").fill_in with: "secret123456"
+      find("input[name='user[password]']").fill_in with: "a new secret123456"
+      find("input[name='user[password]']").send_keys :enter
+      assert_selector ".flash", text: "✓"
+    end
+
+    def assert_subscription_registered
+      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
+
+    def subscription
+      Push::Subscription.find_by!(endpoint: ENDPOINT)
+    end
+end