From c92996646aba2d25b2c3e235fe0c4f1a84b70d24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Valim?= Date: Wed, 16 Dec 2015 17:13:46 +0100 Subject: [PATCH] Store creation timestamp on remember cookies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: José Valim --- lib/devise.rb | 1 + lib/devise/controllers/rememberable.rb | 2 +- lib/devise/models/rememberable.rb | 54 +++++----- lib/devise/models/timeoutable.rb | 6 -- test/integration/rememberable_test.rb | 4 +- test/integration/timeoutable_test.rb | 10 -- test/models/rememberable_test.rb | 131 +++++++------------------ 7 files changed, 66 insertions(+), 142 deletions(-) diff --git a/lib/devise.rb b/lib/devise.rb index d1869651e1..012fbe455d 100644 --- a/lib/devise.rb +++ b/lib/devise.rb @@ -116,6 +116,7 @@ module Strategies mattr_accessor :remember_for @@remember_for = 2.weeks + # TODO: extend_remember_period is no longer used # If true, extends the user's remember period when remembered via cookie. mattr_accessor :extend_remember_period @@extend_remember_period = false diff --git a/lib/devise/controllers/rememberable.rb b/lib/devise/controllers/rememberable.rb index 5700fde8cf..fcd2c0ff34 100644 --- a/lib/devise/controllers/rememberable.rb +++ b/lib/devise/controllers/rememberable.rb @@ -13,7 +13,7 @@ def self.cookie_values def remember_me(resource) return if env["devise.skip_storage"] scope = Devise::Mapping.find_scope!(resource) - resource.remember_me!(resource.extend_remember_period) + resource.remember_me! cookies.signed[remember_key(resource, scope)] = remember_cookie_values(resource) end diff --git a/lib/devise/models/rememberable.rb b/lib/devise/models/rememberable.rb index 62b052fc27..9849b3ca47 100644 --- a/lib/devise/models/rememberable.rb +++ b/lib/devise/models/rememberable.rb @@ -45,11 +45,11 @@ def self.required_fields(klass) [:remember_created_at] end - # Generate a new remember token and save the record without validations - # if remember expired (token is no longer valid) or extend_remember_period is true - def remember_me!(extend_period=false) - self.remember_token = self.class.remember_token if generate_remember_token? - self.remember_created_at = Time.now.utc if generate_remember_timestamp?(extend_period) + # TODO: We were used to receive a extend period argument but we no longer do. + # Remove this for Devise 4.0. + def remember_me!(*) + self.remember_token = self.class.remember_token if respond_to?(:remember_token) + self.remember_created_at ||= Time.now.utc save(validate: false) if self.changed? end @@ -57,19 +57,13 @@ def remember_me!(extend_period=false) # it exists), and save the record without validations. def forget_me! return unless persisted? - self.remember_token = nil if respond_to?(:remember_token=) + self.remember_token = nil if respond_to?(:remember_token) self.remember_created_at = nil if self.class.expire_all_remember_me_on_sign_out save(validate: false) end - # Remember token should be expired if expiration time not overpass now. - def remember_expired? - remember_created_at.nil? || (remember_expires_at <= Time.now.utc) - end - - # Remember token expires at created time + remember_for configuration def remember_expires_at - remember_created_at + self.class.remember_for + self.class.remember_for.from_now end def rememberable_value @@ -104,27 +98,30 @@ def after_remembered protected - def generate_remember_token? #:nodoc: - respond_to?(:remember_token) && remember_expired? - end - - # Generate a timestamp if extend_remember_period is true, if no remember_token - # exists, or if an existing remember token has expired. - def generate_remember_timestamp?(extend_period) #:nodoc: - extend_period || remember_expired? - end - module ClassMethods # Create the cookie key using the record id and remember_token def serialize_into_cookie(record) - [record.to_key, record.rememberable_value] + [record.to_key, record.rememberable_value, Time.now.utc] end # Recreate the user based on the stored cookie - def serialize_from_cookie(id, remember_token) - record = to_adapter.get(id) - record if record && !record.remember_expired? && - Devise.secure_compare(record.rememberable_value, remember_token) + def serialize_from_cookie(*args) + id, token, generated_at = args + + # The token is only valid if: + # 1. we have a date + # 2. the current time does not pass the expiry period + # 3. there is a record with the given id + # 4. the record has a remember_created_at date + # 5. the token date is bigger than the remember_created_at + # 6. the token matches + if generated_at && + (self.remember_for.ago < generated_at) && + (record = to_adapter.get(id)) && + (generated_at > (record.remember_created_at || Time.now).utc) && + Devise.secure_compare(record.rememberable_value, token) + record + end end # Generate a token checking if one does not already exist in the database. @@ -135,6 +132,7 @@ def remember_token #:nodoc: end end + # TODO: extend_remember_period is no longer used Devise::Models.config(self, :remember_for, :extend_remember_period, :rememberable_options, :expire_all_remember_me_on_sign_out) end end diff --git a/lib/devise/models/timeoutable.rb b/lib/devise/models/timeoutable.rb index bdc2abc5d3..d589e24a47 100644 --- a/lib/devise/models/timeoutable.rb +++ b/lib/devise/models/timeoutable.rb @@ -26,7 +26,6 @@ def self.required_fields(klass) # Checks whether the user session has expired based on configured time. def timedout?(last_access) - return false if remember_exists_and_not_expired? !timeout_in.nil? && last_access && last_access <= timeout_in.ago end @@ -36,11 +35,6 @@ def timeout_in private - def remember_exists_and_not_expired? - return false unless respond_to?(:remember_created_at) && respond_to?(:remember_expired?) - remember_created_at && !remember_expired? - end - module ClassMethods Devise::Models.config(self, :timeout_in) end diff --git a/test/integration/rememberable_test.rb b/test/integration/rememberable_test.rb index a762eb11c2..2d82cca380 100644 --- a/test/integration/rememberable_test.rb +++ b/test/integration/rememberable_test.rb @@ -4,7 +4,7 @@ class RememberMeTest < ActionDispatch::IntegrationTest def create_user_and_remember(add_to_token='') user = create_user user.remember_me! - raw_cookie = User.serialize_into_cookie(user).tap { |a| a.last << add_to_token } + raw_cookie = User.serialize_into_cookie(user).tap { |a| a[1] << add_to_token } cookies['remember_user_token'] = generate_signed_cookie(raw_cookie) user end @@ -135,7 +135,7 @@ def cookie_expires(key) test 'do not remember with expired token' do create_user_and_remember - swap Devise, remember_for: 0 do + swap Devise, remember_for: 0.days do get users_path assert_not warden.authenticated?(:user) assert_redirected_to new_user_session_path diff --git a/test/integration/timeoutable_test.rb b/test/integration/timeoutable_test.rb index 1160571ce5..04302ebe3c 100644 --- a/test/integration/timeoutable_test.rb +++ b/test/integration/timeoutable_test.rb @@ -165,16 +165,6 @@ def last_request_at end end - test 'time out not triggered if remembered' do - user = sign_in_as_user remember_me: true - get expire_user_path(user) - assert_not_nil last_request_at - - get users_path - assert_response :success - assert warden.authenticated?(:user) - end - test 'does not crashes when the last_request_at is a String' do user = sign_in_as_user diff --git a/test/models/rememberable_test.rb b/test/models/rememberable_test.rb index c69643c6db..eb1346382d 100644 --- a/test/models/rememberable_test.rb +++ b/test/models/rememberable_test.rb @@ -13,6 +13,7 @@ def create_resource user = create_user user.expects(:valid?).never user.remember_me! + assert user.remember_created_at end test 'forget_me should not clear remember token if using salt' do @@ -33,13 +34,45 @@ def create_resource test 'serialize into cookie' do user = create_user user.remember_me! - assert_equal [user.to_key, user.authenticatable_salt], User.serialize_into_cookie(user) + id, token, date = User.serialize_into_cookie(user) + assert_equal id, user.to_key + assert_equal token, user.authenticatable_salt + assert date.is_a?(Time) end test 'serialize from cookie' do user = create_user user.remember_me! - assert_equal user, User.serialize_from_cookie(user.to_key, user.authenticatable_salt) + assert_equal user, User.serialize_from_cookie(user.to_key, user.authenticatable_salt, Time.now.utc) + end + + test 'serialize from cookie should return nil if no resource is found' do + assert_nil resource_class.serialize_from_cookie([0], "123", Time.now.utc) + end + + test 'serialize from cookie should return nil if no timestamp' do + user = create_user + user.remember_me! + assert_nil User.serialize_from_cookie(user.to_key, user.authenticatable_salt) + end + + test 'serialize from cookie should return nil if timestamp is earlier than token creation' do + user = create_user + user.remember_me! + assert_nil User.serialize_from_cookie(user.to_key, user.authenticatable_salt, 1.day.ago) + end + + test 'serialize from cookie should return nil if timestamp is older than remember_for' do + user = create_user + user.remember_created_at = 1.month.ago + user.remember_me! + assert_nil User.serialize_from_cookie(user.to_key, user.authenticatable_salt, 3.weeks.ago) + end + + test 'serialize from cookie me return nil if is a valid resource with invalid token' do + user = create_user + user.remember_me! + assert_nil User.serialize_from_cookie(user.to_key, "123", Time.now.utc) end test 'raises a RuntimeError if authenticatable_salt is nil or empty' do @@ -93,28 +126,7 @@ def user.authenticable_salt; ""; end resource.forget_me! end - test 'remember is expired if not created at timestamp is set' do - assert create_resource.remember_expired? - end - - test 'serialize should return nil if no resource is found' do - assert_nil resource_class.serialize_from_cookie([0], "123") - end - - test 'remember me return nil if is a valid resource with invalid token' do - resource = create_resource - assert_nil resource_class.serialize_from_cookie([resource.id], "123") - end - - test 'remember for should fallback to devise remember for default configuration' do - swap Devise, remember_for: 1.day do - resource = create_resource - resource.remember_me! - assert_not resource.remember_expired? - end - end - - test 'remember expires at should sum date of creation with remember for configuration' do + test 'remember expires at uses remember for configuration' do swap Devise, remember_for: 3.days do resource = create_resource resource.remember_me! @@ -125,77 +137,6 @@ def user.authenticable_salt; ""; end end end - test 'remember should be expired if remember_for is zero' do - swap Devise, remember_for: 0.days do - Devise.remember_for = 0.days - resource = create_resource - resource.remember_me! - assert resource.remember_expired? - end - end - - test 'remember should be expired if it was created before limit time' do - swap Devise, remember_for: 1.day do - resource = create_resource - resource.remember_me! - resource.remember_created_at = 2.days.ago - resource.save - assert resource.remember_expired? - end - end - - test 'remember should not be expired if it was created within the limit time' do - swap Devise, remember_for: 30.days do - resource = create_resource - resource.remember_me! - resource.remember_created_at = (30.days.ago + 2.minutes) - resource.save - assert_not resource.remember_expired? - end - end - - test 'if extend_remember_period is false, remember_me! should generate a new timestamp if expired' do - swap Devise, remember_for: 5.minutes do - resource = create_resource - resource.remember_me!(false) - assert resource.remember_created_at - - resource.remember_created_at = old = 10.minutes.ago - resource.save - - resource.remember_me!(false) - assert_not_equal old.to_i, resource.remember_created_at.to_i - end - end - - test 'if extend_remember_period is false, remember_me! should not generate a new timestamp' do - swap Devise, remember_for: 1.year do - resource = create_resource - resource.remember_me!(false) - assert resource.remember_created_at - - resource.remember_created_at = old = 10.minutes.ago.utc - resource.save - - resource.remember_me!(false) - assert_equal old.to_i, resource.remember_created_at.to_i - end - end - - test 'if extend_remember_period is true, remember_me! should always generate a new timestamp' do - swap Devise, remember_for: 1.year do - resource = create_resource - resource.remember_me!(true) - assert resource.remember_created_at - - resource.remember_created_at = old = 10.minutes.ago - resource.save - - resource.remember_me!(true) - assert_not_equal old, resource.remember_created_at - end - end - test 'should have the required_fields array' do assert_same_content Devise::Models::Rememberable.required_fields(User), [ :remember_created_at