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>

8 files changed, 190 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