diff --git a/app/controllers/concerns/authentication.rb b/app/controllers/concerns/authentication.rb index 4a1d70c..2457ad9 100644 --- a/app/controllers/concerns/authentication.rb +++ b/app/controllers/concerns/authentication.rb @@ -57,7 +57,7 @@ module Authentication end def start_new_session_for(user) - user.sessions.start!(user_agent: request.user_agent, ip_address: request.remote_ip).tap do |session| + user.sessions.create!(user_agent: request.user_agent, ip_address: request.remote_ip).tap do |session| authenticated_as session end end diff --git a/app/controllers/sessions/transfers_controller.rb b/app/controllers/sessions/transfers_controller.rb index 12dff8f..aff85ab 100644 --- a/app/controllers/sessions/transfers_controller.rb +++ b/app/controllers/sessions/transfers_controller.rb @@ -11,5 +11,7 @@ class Sessions::TransfersController < ApplicationController else head :bad_request end + rescue ActiveRecord::RecordInvalid + head :bad_request end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 1ac2bfd..3a03946 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -14,6 +14,8 @@ class SessionsController < ApplicationController else render_rejection :unauthorized end + rescue ActiveRecord::RecordInvalid + render_rejection :unauthorized end def destroy diff --git a/app/models/session.rb b/app/models/session.rb index c70b6fa..bc65f90 100644 --- a/app/models/session.rb +++ b/app/models/session.rb @@ -7,13 +7,22 @@ class Session < ApplicationRecord before_create { self.last_active_at ||= Time.now } - def self.start!(user_agent:, ip_address:) - create! user_agent: user_agent, ip_address: ip_address - end + validate :user_password_unchanged, on: :create def resume(user_agent:, ip_address:) if last_active_at.before?(ACTIVITY_REFRESH_RATE.ago) update! user_agent: user_agent, ip_address: ip_address, last_active_at: Time.now end end + + private + # A password change ends every session, so one can't start on a password checked before the change. Validated in + # the save's transaction, which holds the database's write lock from the start, so a change commits either before + # this check, and fails it, or after the insert, and ends this session with the others. Compared here rather than in + # the query, which logs its values. + def user_password_unchanged + unless User.where(id: user_id).pick(:password_digest) == user.password_digest + errors.add :user, :invalid + end + end end diff --git a/test/controllers/sessions/transfers_controller_test.rb b/test/controllers/sessions/transfers_controller_test.rb index 18322a8..262fe7f 100644 --- a/test/controllers/sessions/transfers_controller_test.rb +++ b/test/controllers/sessions/transfers_controller_test.rb @@ -30,4 +30,19 @@ class Sessions::TransfersControllerTest < ActionDispatch::IntegrationTest assert_response :bad_request assert_nil parsed_cookies.signed[:session_token] end + + test "update is refused when the password changes after the link was checked" do + user = users(:david) + transfer_id = user.transfer_id + stale = User.find(user.id) + User.find(user.id).update!(password: "a new secret123456") + User.stubs(:find_by_transfer_id).with(transfer_id).returns(stale) + + assert_no_difference -> { Session.count } do + put session_transfer_url(transfer_id) + end + + assert_response :bad_request + assert_nil parsed_cookies.signed[:session_token] + end end diff --git a/test/controllers/sessions_controller_test.rb b/test/controllers/sessions_controller_test.rb index 4b20dd9..ed1f05b 100644 --- a/test/controllers/sessions_controller_test.rb +++ b/test/controllers/sessions_controller_test.rb @@ -36,6 +36,19 @@ class SessionsControllerTest < ActionDispatch::IntegrationTest assert parsed_cookies.signed[:session_token] end + test "create is refused when the password changes after it was checked" do + user = User.find(users(:david).id) + User.find(user.id).update!(password: "a new secret123456") + User.stubs(:authenticate_by).returns(user) + + assert_no_difference -> { Session.count } do + post session_url, params: { email_address: "david@37signals.com", password: "secret123456" } + end + + assert_response :unauthorized + assert_nil parsed_cookies.signed[:session_token] + end + test "create with invalid credentials" do post session_url, params: { email_address: "david@37signals.com", password: "wrong" } diff --git a/test/models/session_test.rb b/test/models/session_test.rb new file mode 100644 index 0000000..53cf5ac --- /dev/null +++ b/test/models/session_test.rb @@ -0,0 +1,20 @@ +require "test_helper" + +class SessionTest < ActiveSupport::TestCase + test "start a session" do + assert_difference -> { users(:david).sessions.count }, +1 do + users(:david).sessions.create!(user_agent: "This device", ip_address: "127.0.0.1") + end + end + + test "a session doesn't start on a password checked before it changed" do + user = User.find(users(:david).id) + User.find(user.id).update!(password: "a new secret123456") + + assert_no_difference -> { Session.count } do + assert_raises(ActiveRecord::RecordInvalid) do + user.sessions.create!(user_agent: "This device", ip_address: "127.0.0.1") + end + end + end +end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index db29028..4bd1bef 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -51,7 +51,7 @@ class UserTest < ActiveSupport::TestCase 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") + Current.session = users(:david).sessions.create!(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")