Make a few changes to the session store:

* Provide :cookie_only as option and as default. If false, allows SID to be retrieved from GET/POST params;

* Do not send the cookie back to the client if session id did not change;

* Make Abstract::ID implementation more modular, allowing Cookie implementation to be simpler by inheriting from it;

Signed-off-by: raggi <jftucker@gmail.com>

5 files changed, 125 insertions, 97 deletions

diff --git a/lib/rack/session/abstract/id.rb b/lib/rack/session/abstract/id.rb
index 98746705..4d211c4d 100644
--- a/lib/rack/session/abstract/id.rb
+++ b/lib/rack/session/abstract/id.rb
@@ -44,7 +44,8 @@ module Rack
           :httponly =>      true,
           :defer =>         false,
           :renew =>         false,
-          :sidbits =>       128
+          :sidbits =>       128,
+          :cookie_only =>   true
         }
 
         attr_reader :key, :default_options
@@ -52,6 +53,7 @@ module Rack
           @app = app
           @key = options[:key] || "rack.session"
           @default_options = self.class::DEFAULT_OPTIONS.merge(options)
+          @cookie_only = @default_options.delete(:cookie_only)
         end
 
         def call(env)
@@ -81,8 +83,7 @@ module Rack
         # 'rack.session.options'.
 
         def load_session(env)
-          request = Rack::Request.new(env)
-          session_id = request.cookies[@key]
+          session_id = extract_session_id(env)
 
           begin
             session_id, session = get_session(env, session_id)
@@ -95,6 +96,15 @@ module Rack
             merge(:id => session_id)
         end
 
+        # Extract session id from request object.
+
+        def extract_session_id(env)
+          request = Rack::Request.new(env)
+          sid = request.cookies[@key]
+          sid ||= request.params[@key] unless @cookie_only
+          sid
+        end
+
         # Acquires the session from the environment and the session id from
         # the session options and passes them to #set_session. If successful
         # and the :defer option is not true, a cookie will be added to the
@@ -105,20 +115,30 @@ module Rack
           options = env['rack.session.options']
           session_id = options[:id]
 
-          if not session_id = set_session(env, session_id, session, options)
+          if not data = set_session(env, session_id, session, options)
             env["rack.errors"].puts("Warning! #{self.class.name} failed to save session. Content dropped.")
           elsif options[:defer] and not options[:renew]
             env["rack.errors"].puts("Defering cookie for #{session_id}") if $VERBOSE
           else
             cookie = Hash.new
-            cookie[:value] = session_id
-            cookie[:expires] = Time.now + options[:expire_after] unless options[:expire_after].nil?
-            Utils.set_cookie_header!(headers, @key, cookie.merge(options))
+            cookie[:value] = data
+            cookie[:expires] = Time.now + options[:expire_after] if options[:expire_after]
+            set_cookie(env, headers, cookie.merge!(options))
           end
 
           [status, headers, body]
         end
 
+        # Sets the cookie back to the client with session id. We skip the cookie
+        # setting if the value didn't change (sid is the same) or expires was given.
+
+        def set_cookie(env, headers, cookie)
+          request = Rack::Request.new(env)
+          if request.cookies[@key] != cookie[:value] || cookie[:expires]
+            Utils.set_cookie_header!(headers, @key, cookie)
+          end
+        end
+
         # All thread safety and session retrival proceedures should occur here.
         # Should return [session_id, session].
         # If nil is provided as the session id, generation of a new valid id
diff --git a/lib/rack/session/cookie.rb b/lib/rack/session/cookie.rb
index 240e6c8d..99d39e9d 100644
--- a/lib/rack/session/cookie.rb
+++ b/lib/rack/session/cookie.rb
@@ -1,6 +1,7 @@
 require 'openssl'
 require 'rack/request'
 require 'rack/response'
+require 'rack/session/abstract/id'
 
 module Rack
 
@@ -21,21 +22,10 @@ module Rack
     #
     #     All parameters are optional.
 
-    class Cookie
-
+    class Cookie < Abstract::ID
       def initialize(app, options={})
-        @app = app
-        @key = options[:key] || "rack.session"
-        @secret = options[:secret]
-        @default_options = {:domain => nil,
-          :path => "/",
-          :expire_after => nil}.merge(options)
-      end
-
-      def call(env)
-        load_session(env)
-        status, headers, body = @app.call(env)
-        commit_session(env, status, headers, body)
+        @secret = options.delete(:secret)
+        super
       end
 
       private
@@ -60,25 +50,25 @@ module Rack
         env["rack.session.options"] = @default_options.dup
       end
 
-      def commit_session(env, status, headers, body)
-        session_data = Marshal.dump(env["rack.session"])
-        session_data = [session_data].pack("m*")
+      def set_session(env, session_id, session, options)
+        session_data = [Marshal.dump(session)].pack("m*")
 
         if @secret
           session_data = "#{session_data}--#{generate_hmac(session_data)}"
         end
 
         if session_data.size > (4096 - @key.size)
-          env["rack.errors"].puts("Warning! Rack::Session::Cookie data size exceeds 4K. Content dropped.")
+          env["rack.errors"].puts("Warning! Rack::Session::Cookie data size exceeds 4K.")
+          nil
         else
-          options = env["rack.session.options"]
-          cookie = Hash.new
-          cookie[:value] = session_data
-          cookie[:expires] = Time.now + options[:expire_after] unless options[:expire_after].nil?
-          Utils.set_cookie_header!(headers, @key, cookie.merge(options))
+          session_data
         end
+      end
+
+      # Overwrite set cookie to bypass content equality and always stream the cookie.
 
-        [status, headers, body]
+      def set_cookie(env, headers, cookie)
+        Utils.set_cookie_header!(headers, @key, cookie)
       end
 
       def generate_hmac(data)
diff --git a/lib/rack/session/memcache.rb b/lib/rack/session/memcache.rb
index 85f60cd1..5e77774a 100644
--- a/lib/rack/session/memcache.rb
+++ b/lib/rack/session/memcache.rb
@@ -105,7 +105,7 @@ module Rack
         end
 
         @pool.set session_id, session, expiry
-        return session_id
+        session_id
       rescue MemCache::MemCacheError, Errno::ECONNREFUSED
         # MemCache server cannot be contacted
         warn "#{self} is unable to find memcached server."
diff --git a/test/spec_session_memcache.rb b/test/spec_session_memcache.rb
index 7ee1c353..2345aee8 100644
--- a/test/spec_session_memcache.rb
+++ b/test/spec_session_memcache.rb
@@ -67,6 +67,28 @@ begin
         body.should.equal '{"counter"=>3}'
     end
 
+    it "determines session only from a cookie by default" do
+      pool = Rack::Session::Memcache.new(incrementor)
+      req = Rack::MockRequest.new(pool)
+      res = req.get("/")
+      sid = res["Set-Cookie"][session_match][1...-1]
+      req.get("/?rack.session=#{sid}").
+        body.should.equal '{"counter"=>1}'
+      req.get("/?rack.session=#{sid}").
+        body.should.equal '{"counter"=>1}'
+    end
+
+    it "determines session from params" do
+      pool = Rack::Session::Memcache.new(incrementor, :cookie_only => false)
+      req = Rack::MockRequest.new(pool)
+      res = req.get("/")
+      sid = res["Set-Cookie"][session_match][1...-1]
+      req.get("/?rack.session=#{sid}").
+        body.should.equal '{"counter"=>2}'
+      req.get("/?rack.session=#{sid}").
+        body.should.equal '{"counter"=>3}'
+    end
+
     it "survives nonexistant cookies" do
       bad_cookie = "rack.session=blarghfasel"
       pool = Rack::Session::Memcache.new(incrementor)
@@ -92,23 +114,36 @@ begin
       res.body.should.include '"counter"=>1'
     end
 
-    it "deletes cookies with :drop option" do
+    it "does not send the same session id if it did not change" do
       pool = Rack::Session::Memcache.new(incrementor)
       req = Rack::MockRequest.new(pool)
-      drop = Rack::Utils::Context.new(pool, drop_session)
-      dreq = Rack::MockRequest.new(drop)
 
       res0 = req.get("/")
       session = (cookie = res0["Set-Cookie"])[session_match]
       res0.body.should.equal '{"counter"=>1}'
 
       res1 = req.get("/", "HTTP_COOKIE" => cookie)
-      res1["Set-Cookie"][session_match].should.equal session
+      res1["Set-Cookie"].should.be.nil
       res1.body.should.equal '{"counter"=>2}'
 
+      res2 = req.get("/", "HTTP_COOKIE" => cookie)
+      res2["Set-Cookie"].should.be.nil
+      res2.body.should.equal '{"counter"=>3}'
+    end
+
+    it "deletes cookies with :drop option" do
+      pool = Rack::Session::Memcache.new(incrementor)
+      req = Rack::MockRequest.new(pool)
+      drop = Rack::Utils::Context.new(pool, drop_session)
+      dreq = Rack::MockRequest.new(drop)
+
+      res1 = req.get("/")
+      session = (cookie = res1["Set-Cookie"])[session_match]
+      res1.body.should.equal '{"counter"=>1}'
+
       res2 = dreq.get("/", "HTTP_COOKIE" => cookie)
       res2["Set-Cookie"].should.equal nil
-      res2.body.should.equal '{"counter"=>3}'
+      res2.body.should.equal '{"counter"=>2}'
 
       res3 = req.get("/", "HTTP_COOKIE" => cookie)
       res3["Set-Cookie"][session_match].should.not.equal session
@@ -121,46 +156,32 @@ begin
       renew = Rack::Utils::Context.new(pool, renew_session)
       rreq = Rack::MockRequest.new(renew)
 
-      res0 = req.get("/")
-      session = (cookie = res0["Set-Cookie"])[session_match]
-      res0.body.should.equal '{"counter"=>1}'
-
-      res1 = req.get("/", "HTTP_COOKIE" => cookie)
-      res1["Set-Cookie"][session_match].should.equal session
-      res1.body.should.equal '{"counter"=>2}'
+      res1 = req.get("/")
+      session = (cookie = res1["Set-Cookie"])[session_match]
+      res1.body.should.equal '{"counter"=>1}'
 
       res2 = rreq.get("/", "HTTP_COOKIE" => cookie)
       new_cookie = res2["Set-Cookie"]
       new_session = new_cookie[session_match]
       new_session.should.not.equal session
-      res2.body.should.equal '{"counter"=>3}'
+      res2.body.should.equal '{"counter"=>2}'
 
       res3 = req.get("/", "HTTP_COOKIE" => new_cookie)
-      res3["Set-Cookie"][session_match].should.equal new_session
-      res3.body.should.equal '{"counter"=>4}'
+      res3.body.should.equal '{"counter"=>3}'
+
+      # Old cookie was deleted
+      res4 = req.get("/", "HTTP_COOKIE" => cookie)
+      res4.body.should.equal '{"counter"=>1}'
     end
 
     it "omits cookie with :defer option" do
       pool = Rack::Session::Memcache.new(incrementor)
-      req = Rack::MockRequest.new(pool)
       defer = Rack::Utils::Context.new(pool, defer_session)
       dreq = Rack::MockRequest.new(defer)
 
-      res0 = req.get("/")
-      session = (cookie = res0["Set-Cookie"])[session_match]
+      res0 = dreq.get("/")
+      res0["Set-Cookie"].should.equal nil
       res0.body.should.equal '{"counter"=>1}'
-
-      res1 = req.get("/", "HTTP_COOKIE" => cookie)
-      res1["Set-Cookie"][session_match].should.equal session
-      res1.body.should.equal '{"counter"=>2}'
-
-      res2 = dreq.get("/", "HTTP_COOKIE" => cookie)
-      res2["Set-Cookie"].should.equal nil
-      res2.body.should.equal '{"counter"=>3}'
-
-      res3 = req.get("/", "HTTP_COOKIE" => cookie)
-      res3["Set-Cookie"][session_match].should.equal session
-      res3.body.should.equal '{"counter"=>4}'
     end
 
     it "updates deep hashes correctly" do
diff --git a/test/spec_session_pool.rb b/test/spec_session_pool.rb
index 904b5f22..60eb23a4 100644
--- a/test/spec_session_pool.rb
+++ b/test/spec_session_pool.rb
@@ -51,11 +51,9 @@ describe Rack::Session::Pool do
     res.body.should.equal '{"counter"=>1}'
   end
 
-  it "deletes cookies with :drop option" do
+  it "does not send the same session id if it did not change" do
     pool = Rack::Session::Pool.new(incrementor)
     req = Rack::MockRequest.new(pool)
-    drop = Rack::Utils::Context.new(pool, drop_session)
-    dreq = Rack::MockRequest.new(drop)
 
     res0 = req.get("/")
     session = (cookie = res0["Set-Cookie"])[session_match]
@@ -63,13 +61,30 @@ describe Rack::Session::Pool do
     pool.pool.size.should.equal 1
 
     res1 = req.get("/", "HTTP_COOKIE" => cookie)
-    res1["Set-Cookie"][session_match].should.equal session
+    res1["Set-Cookie"].should.be.nil
     res1.body.should.equal '{"counter"=>2}'
     pool.pool.size.should.equal 1
 
+    res2 = req.get("/", "HTTP_COOKIE" => cookie)
+    res2["Set-Cookie"].should.be.nil
+    res2.body.should.equal '{"counter"=>3}'
+    pool.pool.size.should.equal 1
+  end
+
+  it "deletes cookies with :drop option" do
+    pool = Rack::Session::Pool.new(incrementor)
+    req = Rack::MockRequest.new(pool)
+    drop = Rack::Utils::Context.new(pool, drop_session)
+    dreq = Rack::MockRequest.new(drop)
+
+    res1 = req.get("/")
+    session = (cookie = res1["Set-Cookie"])[session_match]
+    res1.body.should.equal '{"counter"=>1}'
+    pool.pool.size.should.equal 1
+
     res2 = dreq.get("/", "HTTP_COOKIE" => cookie)
     res2["Set-Cookie"].should.equal nil
-    res2.body.should.equal '{"counter"=>3}'
+    res2.body.should.equal '{"counter"=>2}'
     pool.pool.size.should.equal 0
 
     res3 = req.get("/", "HTTP_COOKIE" => cookie)
@@ -84,53 +99,35 @@ describe Rack::Session::Pool do
     renew = Rack::Utils::Context.new(pool, renew_session)
     rreq = Rack::MockRequest.new(renew)
 
-    res0 = req.get("/")
-    session = (cookie = res0["Set-Cookie"])[session_match]
-    res0.body.should.equal '{"counter"=>1}'
-    pool.pool.size.should.equal 1
-
-    res1 = req.get("/", "HTTP_COOKIE" => cookie)
-    res1["Set-Cookie"][session_match].should.equal session
-    res1.body.should.equal '{"counter"=>2}'
+    res1 = req.get("/")
+    session = (cookie = res1["Set-Cookie"])[session_match]
+    res1.body.should.equal '{"counter"=>1}'
     pool.pool.size.should.equal 1
 
     res2 = rreq.get("/", "HTTP_COOKIE" => cookie)
     new_cookie = res2["Set-Cookie"]
     new_session = new_cookie[session_match]
     new_session.should.not.equal session
-    res2.body.should.equal '{"counter"=>3}'
+    res2.body.should.equal '{"counter"=>2}'
     pool.pool.size.should.equal 1
 
     res3 = req.get("/", "HTTP_COOKIE" => new_cookie)
-    res3["Set-Cookie"][session_match].should.equal new_session
-    res3.body.should.equal '{"counter"=>4}'
+    res3.body.should.equal '{"counter"=>3}'
     pool.pool.size.should.equal 1
+
+    res4 = req.get("/", "HTTP_COOKIE" => cookie)
+    res4.body.should.equal '{"counter"=>1}'
+    pool.pool.size.should.equal 2
   end
 
   it "omits cookie with :defer option" do
     pool = Rack::Session::Pool.new(incrementor)
-    req = Rack::MockRequest.new(pool)
     defer = Rack::Utils::Context.new(pool, defer_session)
     dreq = Rack::MockRequest.new(defer)
 
-    res0 = req.get("/")
-    session = (cookie = res0["Set-Cookie"])[session_match]
-    res0.body.should.equal '{"counter"=>1}'
-    pool.pool.size.should.equal 1
-
-    res1 = req.get("/", "HTTP_COOKIE" => cookie)
-    res1["Set-Cookie"][session_match].should.equal session
-    res1.body.should.equal '{"counter"=>2}'
-    pool.pool.size.should.equal 1
-
-    res2 = dreq.get("/", "HTTP_COOKIE" => cookie)
-    res2["Set-Cookie"].should.equal nil
-    res2.body.should.equal '{"counter"=>3}'
-    pool.pool.size.should.equal 1
-
-    res3 = req.get("/", "HTTP_COOKIE" => cookie)
-    res3["Set-Cookie"][session_match].should.equal session
-    res3.body.should.equal '{"counter"=>4}'
+    res1 = dreq.get("/")
+    res1["Set-Cookie"].should.equal nil
+    res1.body.should.equal '{"counter"=>1}'
     pool.pool.size.should.equal 1
   end