Cut the fat and make session stores rely on request.cookie_jar and change set_session semantics to return the cookie value instead of a boolean.

7 files changed, 71 insertions, 133 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 4db9c4b84d..54c7771f4c 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *Rails 3.0.0 [beta 4/release candidate] (unreleased)*
 
+* Make session stores rely on request.cookie_jar and change set_session semantics to return the cookie value instead of a boolean. [José Valim]
+
 * OAuth 2: HTTP Token Authorization support to complement Basic and Digest Authorization.  [Rick Olson]
 
 * Fixed inconsistencies in form builder and view helpers #4432 [Neeraj Singh]
diff --git a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
index 977185d754..15493cd2eb 100644
--- a/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/abstract_store.rb
@@ -1,5 +1,6 @@
 require 'rack/utils'
 require 'rack/request'
+require 'action_dispatch/middleware/cookies'
 require 'active_support/core_ext/object/blank'
 
 module ActionDispatch
@@ -11,9 +12,6 @@ module ActionDispatch
       ENV_SESSION_KEY = 'rack.session'.freeze
       ENV_SESSION_OPTIONS_KEY = 'rack.session.options'.freeze
 
-      HTTP_COOKIE = 'HTTP_COOKIE'.freeze
-      SET_COOKIE = 'Set-Cookie'.freeze
-
       class SessionHash < Hash
         def initialize(by, env)
           super()
@@ -96,30 +94,15 @@ module ActionDispatch
       }
 
       def initialize(app, options = {})
-        # Process legacy CGI options
-        options = options.symbolize_keys
-        if options.has_key?(:session_path)
-          options[:path] = options.delete(:session_path)
-        end
-        if options.has_key?(:session_key)
-          options[:key] = options.delete(:session_key)
-        end
-        if options.has_key?(:session_http_only)
-          options[:httponly] = options.delete(:session_http_only)
-        end
-
         @app = app
         @default_options = DEFAULT_OPTIONS.merge(options)
-        @key = @default_options[:key]
-        @cookie_only = @default_options[:cookie_only]
+        @key = @default_options.delete(:key).freeze
+        @cookie_only = @default_options.delete(:cookie_only)
+        ensure_session_key!
       end
 
       def call(env)
-        session = SessionHash.new(self, env)
-
-        env[ENV_SESSION_KEY] = session
-        env[ENV_SESSION_OPTIONS_KEY] = @default_options.dup
-
+        prepare!(env)
         response = @app.call(env)
 
         session_data = env[ENV_SESSION_KEY]
@@ -129,53 +112,62 @@ module ActionDispatch
           session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.send(:loaded?)
 
           sid = options[:id] || generate_sid
+          session_data = session_data.to_hash
 
-          unless set_session(env, sid, session_data.to_hash)
-            return response
-          end
+          value = set_session(env, sid, session_data)
+          return response unless value
 
-          cookie = Rack::Utils.escape(@key) + '=' + Rack::Utils.escape(sid)
-          cookie << "; domain=#{options[:domain]}" if options[:domain]
-          cookie << "; path=#{options[:path]}" if options[:path]
-          if options[:expire_after]
-            expiry = Time.now + options[:expire_after]
-            cookie << "; expires=#{expiry.httpdate}"
-          end
-          cookie << "; Secure" if options[:secure]
-          cookie << "; HttpOnly" if options[:httponly]
-
-          headers = response[1]
-          unless headers[SET_COOKIE].blank?
-            headers[SET_COOKIE] << "\n#{cookie}"
-          else
-            headers[SET_COOKIE] = cookie
+          cookie = { :value => value }
+          unless options[:expire_after].nil?
+            cookie[:expires] = Time.now + options.delete(:expire_after)
           end
+
+          request = ActionDispatch::Request.new(env)
+          set_cookie(request, cookie.merge!(options))
         end
 
         response
       end
 
       private
+
+        def prepare!(env)
+          env[ENV_SESSION_KEY] = SessionHash.new(self, env)
+          env[ENV_SESSION_OPTIONS_KEY] = @default_options.dup
+        end
+
         def generate_sid
           ActiveSupport::SecureRandom.hex(16)
         end
 
+        def set_cookie(request, options)
+          request.cookie_jar[@key] = options
+        end
+
         def load_session(env)
           request = Rack::Request.new(env)
-          sid = request.cookies[@key]
-          unless @cookie_only
-            sid ||= request.params[@key]
-          end
+          sid   = request.cookies[@key]
+          sid ||= request.params[@key] unless @cookie_only
           sid, session = get_session(env, sid)
           [sid, session]
         end
 
+        def ensure_session_key!
+          if @key.blank?
+            raise ArgumentError, 'A key is required to write a ' +
+              'cookie containing the session data. Use ' +
+              'config.session_store SESSION_STORE, { :key => ' +
+              '"_myapp_session" } in config/application.rb'
+          end
+        end
+
         def get_session(env, sid)
           raise '#get_session needs to be implemented.'
         end
 
         def set_session(env, sid, session_data)
-          raise '#set_session needs to be implemented.'
+          raise '#set_session needs to be implemented and should return ' <<
+            'the value to be stored in the cookie (usually the sid)'
         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 0c1712bf84..92a86ee229 100644
--- a/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
@@ -1,4 +1,3 @@
-require 'action_dispatch/middleware/cookies'
 require 'active_support/core_ext/hash/keys'
 require 'active_support/core_ext/object/blank'
 
@@ -39,18 +38,10 @@ module ActionDispatch
     # "rake secret" and set the key in config/environment.rb.
     #
     # Note that changing digest or secret invalidates all existing sessions!
-    class CookieStore
-      DEFAULT_OPTIONS = {
-        :key          => '_session_id',
-        :domain       => nil,
-        :path         => "/",
-        :expire_after => nil,
-        :httponly     => true
-      }.freeze
-
+    class CookieStore < AbstractStore
       class OptionsHash < Hash
         def initialize(by, env, default_options)
-          @session_data = env[CookieStore::ENV_SESSION_KEY]
+          @session_data = env[AbstractStore::ENV_SESSION_KEY]
           merge!(default_options)
         end
 
@@ -59,96 +50,38 @@ module ActionDispatch
         end
       end
 
-      ENV_SESSION_KEY = "rack.session".freeze
-      ENV_SESSION_OPTIONS_KEY = "rack.session.options".freeze
-
       def initialize(app, options = {})
-        # Process legacy CGI options
-        # TODO Refactor and deprecate me
-        options = options.symbolize_keys
-
-        if options.has_key?(:session_path)
-          options[:path] = options.delete(:session_path)
-        end
-        if options.has_key?(:session_key)
-          options[:key] = options.delete(:session_key)
-        end
-        if options.has_key?(:session_http_only)
-          options[:httponly] = options.delete(:session_http_only)
-        end
-
-        @app = app
-
-        # The session_key option is required.
-        ensure_session_key(options[:key])
-        @key = options.delete(:key).freeze
-
-        @default_options = DEFAULT_OPTIONS.merge(options).freeze
+        super(app, options.merge!(:cookie_only => true))
         freeze
       end
 
-      def call(env)
-        env[ENV_SESSION_KEY] = AbstractStore::SessionHash.new(self, env)
-        env[ENV_SESSION_OPTIONS_KEY] = OptionsHash.new(self, env, @default_options)
-
-        status, headers, body = @app.call(env)
-
-        session_data = env[ENV_SESSION_KEY]
-        options = env[ENV_SESSION_OPTIONS_KEY]
-
-        if !session_data.is_a?(AbstractStore::SessionHash) || session_data.send(:loaded?) || options[:expire_after]
-          session_data.send(:load!) if session_data.is_a?(AbstractStore::SessionHash) && !session_data.send(:loaded?)
-          session_data = persistent_session_id!(session_data.to_hash)
-
-          cookie = { :value => session_data }
-          unless options[:expire_after].nil?
-            cookie[:expires] = Time.now + options.delete(:expire_after)
-          end
+      private
 
-          request = ActionDispatch::Request.new(env)
-          request.cookie_jar.signed[@key] = cookie.merge!(options)
+        def prepare!(env)
+          env[ENV_SESSION_KEY] = SessionHash.new(self, env)
+          env[ENV_SESSION_OPTIONS_KEY] = OptionsHash.new(self, env, @default_options)
         end
 
-        [status, headers, body]
-      end
-
-      private
-
         def load_session(env)
           request = ActionDispatch::Request.new(env)
           data = request.cookie_jar.signed[@key]
-          data = persistent_session_id!(data || {})
+          data = persistent_session_id!(data)
           data.stringify_keys!
           [data["session_id"], data]
         end
 
-        def generate_sid
-          ActiveSupport::SecureRandom.hex(16)
-        end
-
-        def ensure_session_key(key)
-          if key.blank?
-            raise ArgumentError, 'A key is required to write a ' +
-              'cookie containing the session data. Use ' +
-              'config.session_store :cookie_store, { :key => ' +
-              '"_myapp_session" } in config/application.rb'
-          end
-        end
-
-        def persistent_session_id!(data)
-          (data ||= {}).merge!(inject_persistent_session_id(data))
+        def set_cookie(request, options)
+          request.cookie_jar.signed[@key] = options
         end
 
-        def inject_persistent_session_id(data)
-          requires_session_id?(data) ? { "session_id" => generate_sid } : {}
+        def set_session(env, sid, session_data)
+          persistent_session_id!(session_data, sid)
         end
 
-        def requires_session_id?(data)
-          if data
-            data.respond_to?(:key?) && !data.key?("session_id")
-          else
-            true
-          end
+        def persistent_session_id!(data, sid=nil)
+          data ||= {}
+          data["session_id"] ||= sid || generate_sid
+          data
         end
     end
   end
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 be1d5a43a2..8df8f977e8 100644
--- a/actionpack/lib/action_dispatch/middleware/session/mem_cache_store.rb
+++ b/actionpack/lib/action_dispatch/middleware/session/mem_cache_store.rb
@@ -38,9 +38,9 @@ module ActionDispatch
           options = env['rack.session.options']
           expiry  = options[:expire_after] || 0
           @pool.set(sid, session_data, expiry)
-          return true
+          sid
         rescue MemCache::MemCacheError, Errno::ECONNREFUSED
-          return false
+          false
         end
     end
   end
diff --git a/actionpack/test/activerecord/active_record_store_test.rb b/actionpack/test/activerecord/active_record_store_test.rb
index 61bee1b66c..6d4b8e1e40 100644
--- a/actionpack/test/activerecord/active_record_store_test.rb
+++ b/actionpack/test/activerecord/active_record_store_test.rb
@@ -152,12 +152,18 @@ class ActiveRecordStoreTest < ActionController::IntegrationTest
   end
 
   private
+
     def with_test_route_set(options = {})
       with_routing do |set|
         set.draw do |map|
           match ':action', :to => 'active_record_store_test/test'
         end
-        @app = ActiveRecord::SessionStore.new(set, options.reverse_merge(:key => '_session_id'))
+
+        @app = self.class.build_app(set) do |middleware|
+          middleware.use ActiveRecord::SessionStore, options.reverse_merge(:key => '_session_id')
+          middleware.delete "ActionDispatch::ShowExceptions"
+        end
+
         yield
       end
     end
diff --git a/actionpack/test/dispatch/session/cookie_store_test.rb b/actionpack/test/dispatch/session/cookie_store_test.rb
index a6cdbf8032..21d11ff31c 100644
--- a/actionpack/test/dispatch/session/cookie_store_test.rb
+++ b/actionpack/test/dispatch/session/cookie_store_test.rb
@@ -199,7 +199,7 @@ class CookieStoreTest < ActionController::IntegrationTest
           match ':action', :to => ::CookieStoreTest::TestController
         end
 
-        options = { :key => SessionKey, :secret => SessionSecret }.merge!(options)
+        options = { :key => SessionKey }.merge!(options)
 
         @app = self.class.build_app(set) do |middleware|
           middleware.use ActionDispatch::Session::CookieStore, options
diff --git a/actionpack/test/dispatch/session/mem_cache_store_test.rb b/actionpack/test/dispatch/session/mem_cache_store_test.rb
index 5a1dcb4dab..8858a398e0 100644
--- a/actionpack/test/dispatch/session/mem_cache_store_test.rb
+++ b/actionpack/test/dispatch/session/mem_cache_store_test.rb
@@ -114,7 +114,12 @@ class MemCacheStoreTest < ActionController::IntegrationTest
         set.draw do |map|
           match ':action', :to => ::MemCacheStoreTest::TestController
         end
-        @app = ActionDispatch::Session::MemCacheStore.new(set, :key => '_session_id')
+
+        @app = self.class.build_app(set) do |middleware|
+          middleware.use ActionDispatch::Session::MemCacheStore, :key => '_session_id'
+          middleware.delete "ActionDispatch::ShowExceptions"
+        end
+
         yield
       end
     end