aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMichael Lovitt <michael@lovitt.net>2010-06-22 09:55:50 -0400
committerJeremy Kemper <jeremy@bitsweat.net>2010-06-23 11:56:35 -0700
commit49f52c3d910c8f183afc3a54ea2ae9667f23085e (patch)
tree410bc3c8fb8921397547c33fd8661f2015065946
parent0bf3baa6b3d216c6340f8d3b5d0a3ebc093e969a (diff)
downloadrails-49f52c3d910c8f183afc3a54ea2ae9667f23085e.tar.gz
rails-49f52c3d910c8f183afc3a54ea2ae9667f23085e.tar.bz2
rails-49f52c3d910c8f183afc3a54ea2ae9667f23085e.zip
Sessions should not be created until written to and session data should be destroyed on reset.
[#4938] Signed-off-by: Jeremy Kemper <jeremy@bitsweat.net>
-rw-r--r--actionpack/lib/action_controller/test_case.rb2
-rw-r--r--actionpack/lib/action_dispatch/http/request.rb2
-rw-r--r--actionpack/lib/action_dispatch/middleware/session/abstract_store.rb96
-rw-r--r--actionpack/lib/action_dispatch/middleware/session/cookie_store.rb39
-rw-r--r--actionpack/lib/action_dispatch/middleware/session/mem_cache_store.rb9
-rw-r--r--actionpack/test/activerecord/active_record_store_test.rb35
-rw-r--r--actionpack/test/dispatch/session/cookie_store_test.rb11
-rw-r--r--actionpack/test/dispatch/session/mem_cache_store_test.rb31
-rw-r--r--activerecord/lib/active_record/session_store.rb8
9 files changed, 198 insertions, 35 deletions
diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb
index 26a385011f..650eb16ac0 100644
--- a/actionpack/lib/action_controller/test_case.rb
+++ b/actionpack/lib/action_controller/test_case.rb
@@ -193,6 +193,8 @@ module ActionController
replace(session.stringify_keys)
@loaded = true
end
+
+ def exists?; true; end
end
# Superclass for ActionController functional tests. Functional tests allow you to
diff --git a/actionpack/lib/action_dispatch/http/request.rb b/actionpack/lib/action_dispatch/http/request.rb
index 98f4f5ae18..6b611823d0 100644
--- a/actionpack/lib/action_dispatch/http/request.rb
+++ b/actionpack/lib/action_dispatch/http/request.rb
@@ -195,7 +195,7 @@ module ActionDispatch
end
def reset_session
- self.session_options.delete(:id)
+ session.destroy if session
self.session = {}
end
diff --git a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
index 3e8d64b0c6..7623a94234 100644
--- a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
@@ -12,6 +12,37 @@ module ActionDispatch
ENV_SESSION_KEY = 'rack.session'.freeze
ENV_SESSION_OPTIONS_KEY = 'rack.session.options'.freeze
+ # thin wrapper around Hash that allows us to lazily
+ # load session id into session_options
+ class OptionsHash < Hash
+ def initialize(by, env, default_options)
+ @by = by
+ @env = env
+ merge!(default_options)
+ @session_id_loaded = false
+ end
+
+ alias_method :get_without_session_load, :[]
+
+ def [](key)
+ if key == :id
+ load_session_id! unless has_session_id?
+ end
+ super(key)
+ end
+
+ private
+
+ def has_session_id?
+ get_without_session_load(:id).present? || @session_id_loaded
+ end
+
+ def load_session_id!
+ self[:id] = @by.send(:extract_session_id, @env)
+ @session_id_loaded = true
+ end
+ end
+
class SessionHash < Hash
def initialize(by, env)
super()
@@ -21,45 +52,71 @@ module ActionDispatch
end
def [](key)
- load! unless @loaded
+ load_for_read!
+ super(key.to_s)
+ end
+
+ def has_key?(key)
+ load_for_read!
super(key.to_s)
end
def []=(key, value)
- load! unless @loaded
+ load_for_write!
super(key.to_s, value)
end
def to_hash
+ load_for_read!
h = {}.replace(self)
h.delete_if { |k,v| v.nil? }
h
end
def update(hash)
- load! unless @loaded
+ load_for_write!
super(hash.stringify_keys)
end
def delete(key)
- load! unless @loaded
+ load_for_write!
super(key.to_s)
end
def inspect
- load! unless @loaded
+ load_for_read!
super
end
+ def exists?
+ @by.send(:exists?, @env)
+ end
+
def loaded?
@loaded
end
+ def destroy
+ clear
+ @by.send(:destroy, @env) if @by
+ @env[ENV_SESSION_OPTIONS_KEY].delete(:id) if @env && @env[ENV_SESSION_OPTIONS_KEY]
+ @loaded = false
+ end
+
private
+
+ def load_for_read!
+ load! if !loaded? && exists?
+ end
+
+ def load_for_write!
+ load! unless loaded?
+ end
+
def load!
stale_session_check! do
id, session = @by.send(:load_session, @env)
- (@env[ENV_SESSION_OPTIONS_KEY] ||= {})[:id] = id
+ @env[ENV_SESSION_OPTIONS_KEY][:id] = id
replace(session.stringify_keys)
@loaded = true
end
@@ -75,7 +132,6 @@ module ActionDispatch
rescue LoadError, NameError => const_error
raise ActionDispatch::Session::SessionRestoreError, "Session contains objects whose class definition isn't available.\nRemember to require the classes for all objects kept in the session.\n(Original exception: #{const_error.message} [#{const_error.class}])\n"
end
-
retry
else
raise
@@ -133,7 +189,7 @@ module ActionDispatch
def prepare!(env)
env[ENV_SESSION_KEY] = SessionHash.new(self, env)
- env[ENV_SESSION_OPTIONS_KEY] = @default_options.dup
+ env[ENV_SESSION_OPTIONS_KEY] = OptionsHash.new(self, env, @default_options.dup)
end
def generate_sid
@@ -145,13 +201,22 @@ module ActionDispatch
end
def load_session(env)
- request = Rack::Request.new(env)
- sid = request.cookies[@key]
- sid ||= request.params[@key] unless @cookie_only
+ sid = current_session_id(env)
sid, session = get_session(env, sid)
[sid, session]
end
+ def extract_session_id(env)
+ request = Rack::Request.new(env)
+ sid = request.cookies[@key]
+ sid ||= request.params[@key] unless @cookie_only
+ sid
+ end
+
+ def current_session_id(env)
+ env[ENV_SESSION_OPTIONS_KEY][:id]
+ end
+
def ensure_session_key!
if @key.blank?
raise ArgumentError, 'A key is required to write a ' +
@@ -161,6 +226,10 @@ module ActionDispatch
end
end
+ def exists?(env)
+ current_session_id(env).present?
+ end
+
def get_session(env, sid)
raise '#get_session needs to be implemented.'
end
@@ -169,6 +238,11 @@ module ActionDispatch
raise '#set_session needs to be implemented and should return ' <<
'the value to be stored in the cookie (usually the sid)'
end
+
+ def destroy(env)
+ raise '#destroy needs to be implemented.'
+ end
+
end
end
end
diff --git a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
index 92a86ee229..7c5626735b 100644
--- a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
@@ -39,16 +39,6 @@ module ActionDispatch
#
# Note that changing digest or secret invalidates all existing sessions!
class CookieStore < AbstractStore
- class OptionsHash < Hash
- def initialize(by, env, default_options)
- @session_data = env[AbstractStore::ENV_SESSION_KEY]
- merge!(default_options)
- end
-
- def [](key)
- key == :id ? @session_data[:session_id] : super(key)
- end
- end
def initialize(app, options = {})
super(app, options.merge!(:cookie_only => true))
@@ -57,19 +47,28 @@ module ActionDispatch
private
- def prepare!(env)
- env[ENV_SESSION_KEY] = SessionHash.new(self, env)
- env[ENV_SESSION_OPTIONS_KEY] = OptionsHash.new(self, env, @default_options)
- end
-
def load_session(env)
- request = ActionDispatch::Request.new(env)
- data = request.cookie_jar.signed[@key]
+ data = unpacked_cookie_data(env)
data = persistent_session_id!(data)
- data.stringify_keys!
[data["session_id"], data]
end
+ def extract_session_id(env)
+ if data = unpacked_cookie_data(env)
+ data["session_id"]
+ else
+ nil
+ end
+ end
+
+ def unpacked_cookie_data(env)
+ request = ActionDispatch::Request.new(env)
+ if data = request.cookie_jar.signed[@key]
+ data.stringify_keys!
+ end
+ data
+ end
+
def set_cookie(request, options)
request.cookie_jar.signed[@key] = options
end
@@ -78,6 +77,10 @@ module ActionDispatch
persistent_session_id!(session_data, sid)
end
+ def destroy(env)
+ # session data is stored on client; nothing to do here
+ end
+
def persistent_session_id!(data, sid=nil)
data ||= {}
data["session_id"] ||= sid || generate_sid
diff --git a/actionpack/lib/action_dispatch/middleware/session/mem_cache_store.rb b/actionpack/lib/action_dispatch/middleware/session/mem_cache_store.rb
index 8df8f977e8..28e3dbd732 100644
--- a/actionpack/lib/action_dispatch/middleware/session/mem_cache_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/mem_cache_store.rb
@@ -42,6 +42,15 @@ module ActionDispatch
rescue MemCache::MemCacheError, Errno::ECONNREFUSED
false
end
+
+ def destroy(env)
+ if sid = current_session_id(env)
+ @pool.delete(sid)
+ end
+ rescue MemCache::MemCacheError, Errno::ECONNREFUSED
+ false
+ end
+
end
end
end
diff --git a/actionpack/test/activerecord/active_record_store_test.rb b/actionpack/test/activerecord/active_record_store_test.rb
index 6d4b8e1e40..736829dbf7 100644
--- a/actionpack/test/activerecord/active_record_store_test.rb
+++ b/actionpack/test/activerecord/active_record_store_test.rb
@@ -17,7 +17,6 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest
end
def get_session_id
- session[:foo]
render :text => "#{request.session_options[:id]}"
end
@@ -58,6 +57,10 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest
get '/get_session_value'
assert_response :success
assert_equal 'foo: "baz"', response.body
+
+ get '/call_reset_session'
+ assert_response :success
+ assert_not_equal [], headers['Set-Cookie']
end
end
end
@@ -92,6 +95,34 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest
end
end
+ def test_getting_session_value_after_session_reset
+ with_test_route_set do
+ get '/set_session_value'
+ assert_response :success
+ assert cookies['_session_id']
+ session_cookie = cookies.send(:hash_for)['_session_id']
+
+ get '/call_reset_session'
+ assert_response :success
+ assert_not_equal [], headers['Set-Cookie']
+
+ cookies << session_cookie # replace our new session_id with our old, pre-reset session_id
+
+ get '/get_session_value'
+ assert_response :success
+ assert_equal 'foo: nil', response.body, "data for this session should have been obliterated from the database"
+ end
+ end
+
+ def test_getting_from_nonexistent_session
+ with_test_route_set do
+ get '/get_session_value'
+ assert_response :success
+ assert_equal 'foo: nil', response.body
+ assert_nil cookies['_session_id'], "should only create session on write, not read"
+ end
+ end
+
def test_getting_session_id
with_test_route_set do
get '/set_session_value'
@@ -101,7 +132,7 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest
get '/get_session_id'
assert_response :success
- assert_equal session_id, response.body
+ assert_equal session_id, response.body, "should be able to read session id without accessing the session hash"
end
end
diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb
index b4380f7818..787524ab7b 100644
--- a/actionpack/test/dispatch/session/cookie_store_test.rb
+++ b/actionpack/test/dispatch/session/cookie_store_test.rb
@@ -83,7 +83,7 @@ class CookieStoreTest < ActionController::IntegrationTest
get '/get_session_id'
assert_response :success
- assert_equal "id: #{session_id}", response.body
+ assert_equal "id: #{session_id}", response.body, "should be able to read session id without accessing the session hash"
end
end
@@ -141,6 +141,15 @@ class CookieStoreTest < ActionController::IntegrationTest
end
end
+ def test_getting_from_nonexistent_session
+ with_test_route_set do
+ get '/get_session_value'
+ assert_response :success
+ assert_equal 'foo: nil', response.body
+ assert_nil headers['Set-Cookie'], "should only create session on write, not read"
+ end
+ end
+
def test_persistent_session_id
with_test_route_set do
cookies[SessionKey] = SignedBar
diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb
index 8858a398e0..08f8069888 100644
--- a/actionpack/test/dispatch/session/mem_cache_store_test.rb
+++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb
@@ -17,7 +17,6 @@ class MemCacheStoreTest < ActionController::IntegrationTest
end
def get_session_id
- session[:foo]
render :text => "#{request.session_options[:id]}"
end
@@ -56,6 +55,34 @@ class MemCacheStoreTest < ActionController::IntegrationTest
end
end
+ def test_getting_session_value_after_session_reset
+ with_test_route_set do
+ get '/set_session_value'
+ assert_response :success
+ assert cookies['_session_id']
+ session_cookie = cookies.send(:hash_for)['_session_id']
+
+ get '/call_reset_session'
+ assert_response :success
+ assert_not_equal [], headers['Set-Cookie']
+
+ cookies << session_cookie # replace our new session_id with our old, pre-reset session_id
+
+ get '/get_session_value'
+ assert_response :success
+ assert_equal 'foo: nil', response.body, "data for this session should have been obliterated from memcached"
+ end
+ end
+
+ def test_getting_from_nonexistent_session
+ with_test_route_set do
+ get '/get_session_value'
+ assert_response :success
+ assert_equal 'foo: nil', response.body
+ assert_nil cookies['_session_id'], "should only create session on write, not read"
+ end
+ end
+
def test_setting_session_value_after_session_reset
with_test_route_set do
get '/set_session_value'
@@ -86,7 +113,7 @@ class MemCacheStoreTest < ActionController::IntegrationTest
get '/get_session_id'
assert_response :success
- assert_equal session_id, response.body
+ assert_equal session_id, response.body, "should be able to read session id without accessing the session hash"
end
end
diff --git a/activerecord/lib/active_record/session_store.rb b/activerecord/lib/active_record/session_store.rb
index f712a2c94f..b88d550086 100644
--- a/activerecord/lib/active_record/session_store.rb
+++ b/activerecord/lib/active_record/session_store.rb
@@ -318,6 +318,14 @@ module ActiveRecord
sid
end
+ def destroy(env)
+ if sid = current_session_id(env)
+ Base.silence do
+ get_session_model(env, sid).destroy
+ end
+ end
+ end
+
def get_session_model(env, sid)
if env[ENV_SESSION_OPTIONS_KEY][:id].nil?
env[SESSION_RECORD_KEY] = find_session(sid)