Introduce a new base class to avoid breaking when upgrading

Third-party session store would still need to be chaged to be more
secure but only upgrading rack will not break any application.

lib/rack/session/abstract/id.rb

@@ -4,17 +4,12 @@
 require 'time'
 require 'rack/request'
 require 'rack/response'
-<<<<<<< HEAD
 begin
   require 'securerandom'
 rescue LoadError
   # We just won't get securerandom
 end
 require "digest/sha2"
-=======
-require 'securerandom'
-require 'digest/sha2'
->>>>>>> Fallback to the public id when reading the session in the pool adapter
 
 module Rack
 
@@ -90,11 +85,7 @@ def each(&block)
 
         def [](key)
           load_for_read!
-          if key == "session_id"
-            id.public_id
-          else
-            @data[key.to_s]
-          end
+          @data[key.to_s]
         end
         alias :fetch :[]
 
@@ -227,7 +218,7 @@ def stringify_keys(other)
       # Not included by default; you must require 'rack/session/abstract/id'
       # to use.
 
-      class ID
+      class Persisted
         DEFAULT_OPTIONS = {
           :key =>           'rack.session',
           :path =>          '/',
@@ -275,13 +266,11 @@ def initialize_sid
         # Monkey patch this to use custom methods for session id generation.
 
         def generate_sid(secure = @sid_secure)
-          public_id = if secure
+          if secure
             secure.hex(@sid_length)
           else
             "%0#{@sid_length}x" % Kernel.rand(2**@sidbits - 1)
           end
-
-          SessionId.new(public_id)
         rescue NotImplementedError
           generate_sid(false)
         end
@@ -311,7 +300,7 @@ def extract_session_id(env)
           request = Rack::Request.new(env)
           sid = request.cookies[@key]
           sid ||= request.params[@key] unless @cookie_only
-          sid && SessionId.new(sid)
+          sid
         end
 
         # Returns the current session id from the SessionHash.
@@ -383,7 +372,7 @@ def commit_session(env, status, headers, body)
             env["rack.errors"].puts("Deferring cookie for #{session_id}") if $VERBOSE
           else
             cookie = Hash.new
-            cookie[:value] = data.cookie_value
+            cookie[:value] = cookie_value(data)
             cookie[:expires] = Time.now + options[:expire_after] if options[:expire_after]
             cookie[:expires] = Time.now + options[:max_age] if options[:max_age]
             set_cookie(env, headers, cookie.merge!(options))
@@ -392,6 +381,10 @@ def commit_session(env, status, headers, body)
           [status, headers, body]
         end
 
+        def cookie_value(data)
+          data
+        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.
 
@@ -432,6 +425,51 @@ def destroy_session(env, sid, options)
           raise '#destroy_session not implemented'
         end
       end
+
+      class PersistedSecure < Persisted
+        class SecureSessionHash < SessionHash
+          def [](key)
+            if key == "session_id"
+              load_for_read!
+              id.public_id
+            else
+              super
+            end
+          end
+        end
+
+        def generate_sid(*)
+          public_id = super
+
+          SessionId.new(public_id)
+        end
+
+        def extract_session_id(*)
+          public_id = super
+          public_id && SessionId.new(public_id)
+        end
+
+        private
+
+        def session_class
+          SecureSessionHash
+        end
+
+        def cookie_value(data)
+          data.cookie_value
+        end
+      end
+
+      class ID < Persisted
+        def self.inherited(klass)
+          k = klass.ancestors.find { |kl| kl.respond_to?(:superclass) && kl.superclass == ID }
+          unless k.instance_variable_defined?(:"@_rack_warned")
+            warn "#{klass} is inheriting from #{ID}.  Inheriting from #{ID} is deprecated, please inherit from #{Persisted} instead" if $VERBOSE
+            k.instance_variable_set(:"@_rack_warned", true)
+          end
+          super
+        end
+      end
     end
   end
 end

lib/rack/session/cookie.rb

@@ -44,7 +44,7 @@ module Session
     #   })
     #
 
-    class Cookie < Abstract::ID
+    class Cookie < Abstract::PersistedSecure
       # Encode session cookies as Base64
       class Base64
         def encode(str)

lib/rack/session/memcache.rb

@@ -19,7 +19,7 @@ module Session
     # Note that memcache does drop data before it may be listed to expire. For
     # a full description of behaviour, please see memcache's documentation.
 
-    class Memcache < Abstract::ID
+    class Memcache < Abstract::PersistedSecure
       attr_reader :mutex, :pool
 
       DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge \
@@ -46,8 +46,8 @@ def generate_sid
         end
       end
 
-      def get_session(env, sid)
-        with_lock(env) do
+      def find_session(req, sid)
+        with_lock(req) do
           unless sid and session = get_session_with_fallback(sid)
             sid, session = generate_sid, {}
             unless /^STORED/ =~ @pool.add(sid.private_id, session)
@@ -58,18 +58,18 @@ def get_session(env, sid)
         end
       end
 
-      def set_session(env, session_id, new_session, options)
+      def write_session(req, session_id, new_session, options)
         expiry = options[:expire_after]
         expiry = expiry.nil? ? 0 : expiry + 1
 
-        with_lock(env) do
+        with_lock(req) do
           @pool.set session_id.private_id, new_session, expiry
           session_id
         end
       end
 
-      def destroy_session(env, session_id, options)
-        with_lock(env) do
+      def delete_session(req, session_id, options)
+        with_lock(req) do
           @pool.delete(session_id.public_id)
           @pool.delete(session_id.private_id)
           generate_sid unless options[:drop]

lib/rack/session/pool.rb

@@ -24,7 +24,7 @@ module Session
     #   )
     #   Rack::Handler::WEBrick.run sessioned
 
-    class Pool < Abstract::ID
+    class Pool < Abstract::PersistedSecure
       attr_reader :mutex, :pool
       DEFAULT_OPTIONS = Abstract::ID::DEFAULT_OPTIONS.merge :drop => false
 

test/spec_session_abstract_id.rb

@@ -47,7 +47,7 @@ def hex(*args)
       end
     end
     id = Rack::Session::Abstract::ID.new nil, :secure_random => secure_random.new
-    id.send(:generate_sid).public_id.must_equal 'fake_hex'
+    id.send(:generate_sid).should.equal 'fake_hex'
   end
 
 end

test/spec_session_pool.rb

@@ -154,9 +154,9 @@
     pool.pool.delete(session_id.private_id)
 
     res1 = req.get("/", "HTTP_COOKIE" => cookie)
-    res1["Set-Cookie"].must_be_nil
-    res1.body.must_equal '{"counter"=>2}'
-    pool.pool[session_id.private_id].wont_be_nil
+    res1["Set-Cookie"].should.be.nil
+    res1.body.should.equal '{"counter"=>2}'
+    pool.pool[session_id.private_id].should.not.be.nil
   end
 
   it "drops the session in the legacy id as well" do