From c53683595749dcfa223802669237480ac9ebc17f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Tue, 18 May 2010 03:18:23 +0200 Subject: 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. --- actionpack/CHANGELOG | 2 + .../middleware/session/abstract_store.rb | 82 ++++++++---------- .../middleware/session/cookie_store.rb | 99 ++++------------------ .../middleware/session/mem_cache_store.rb | 4 +- .../test/activerecord/active_record_store_test.rb | 8 +- .../test/dispatch/session/cookie_store_test.rb | 2 +- .../test/dispatch/session/mem_cache_store_test.rb | 7 +- 7 files changed, 71 insertions(+), 133 deletions(-) (limited to 'actionpack') 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 -- cgit v1.2.3