diff --git a/app/controllers/users/profiles_controller.rb b/app/controllers/users/profiles_controller.rb index 96015d55..a6acc994 100644 --- a/app/controllers/users/profiles_controller.rb +++ b/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 diff --git a/app/controllers/users/push_subscriptions_controller.rb b/app/controllers/users/push_subscriptions_controller.rb index 8e263b23..6b33452f 100644 --- a/app/controllers/users/push_subscriptions_controller.rb +++ b/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 diff --git a/app/helpers/translations_helper.rb b/app/helpers/translations_helper.rb index 6142ab49..6c269ab3 100644 --- a/app/helpers/translations_helper.rb +++ b/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", "🇯🇵": "アカウントに名前を付ける" }, diff --git a/app/helpers/users/push_subscriptions_helper.rb b/app/helpers/users/push_subscriptions_helper.rb new file mode 100644 index 00000000..57c681f9 --- /dev/null +++ b/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 diff --git a/app/javascript/controllers/notifications_controller.js b/app/javascript/controllers/notifications_controller.js index 3ce53a5c..f7dfe7ec 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/app/javascript/controllers/push_subscription_sync_controller.js b/app/javascript/controllers/push_subscription_sync_controller.js new file mode 100644 index 00000000..d2636523 --- /dev/null +++ b/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 } }) + } +} diff --git a/app/models/user.rb b/app/models/user.rb index df2fee19..07f4bd03 100644 --- a/app/models/user.rb +++ b/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, diff --git a/app/models/user/transferable.rb b/app/models/user/transferable.rb index 096913aa..3a9ace04 100644 --- a/app/models/user/transferable.rb +++ b/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 diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 7e2f73c5..b901b380 100644 --- a/app/views/layouts/application.html.erb +++ b/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 %> + diff --git a/app/views/users/profiles/show.html.erb b/app/views/users/profiles/show.html.erb index 1fe919da..368853d8 100644 --- a/app/views/users/profiles/show.html.erb +++ b/app/views/users/profiles/show.html.erb @@ -69,6 +69,16 @@ +
+ <%= translation_button(:current_password) %> + + +
+
<%= translation_button(:update_password) %> diff --git a/test/controllers/cached_responses_test.rb b/test/controllers/cached_responses_test.rb index 888994d0..9168f3bf 100644 --- a/test/controllers/cached_responses_test.rb +++ b/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"] diff --git a/test/controllers/sessions/transfers_controller_test.rb b/test/controllers/sessions/transfers_controller_test.rb index 6efb37f4..18322a88 100644 --- a/test/controllers/sessions/transfers_controller_test.rb +++ b/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 diff --git a/test/controllers/users/profiles_controller_test.rb b/test/controllers/users/profiles_controller_test.rb index 0331aafb..3d82162a 100644 --- a/test/controllers/users/profiles_controller_test.rb +++ b/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 diff --git a/test/controllers/users/push_subscriptions_controller_test.rb b/test/controllers/users/push_subscriptions_controller_test.rb index e69fc4a2..cbfacb40 100644 --- a/test/controllers/users/push_subscriptions_controller_test.rb +++ b/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 diff --git a/test/models/user/transferable_test.rb b/test/models/user/transferable_test.rb new file mode 100644 index 00000000..00ce337e --- /dev/null +++ b/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 diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 61bb1c39..5cd445a8 100644 --- a/test/models/user_test.rb +++ b/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") diff --git a/test/script/admin/reset_password_test.rb b/test/script/admin/reset_password_test.rb new file mode 100644 index 00000000..637bb950 --- /dev/null +++ b/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 diff --git a/test/system/push_subscription_resync_test.rb b/test/system/push_subscription_resync_test.rb new file mode 100644 index 00000000..9d66f8f7 --- /dev/null +++ b/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