From ab64b18a953beb1298682efa3ff159f23cd34c5e Mon Sep 17 00:00:00 2001 From: Alex Ghiculescu Date: Mon, 22 Feb 2021 21:38:18 -0600 Subject: [PATCH 1/4] Fix bugs in `extend_remember_period` This PR adds better tests for `extend_remember_period` and tries to better document what the feature is supposed to do. Currently it's not very well specified, and I'm not sure it's behaving as expected. I found https://gitlab.com/gitlab-org/gitlab/-/merge_requests/32730 and https://github.com/heartcombo/devise/issues/3950#issuecomment-217326227 which both describe how the feature should work. To summarise: - if you log in to the application regularly, `extend_remember_period` will keep your session alive - if you *don't* log into the application for a while (longer than `config.remember_for`) you'll get logged out In reality, it looks like what happens is: - if you log in to the application regularly, your session will stay alive for as long as `config.remember_for` - if you *don't* log into the application for a while (longer than `config.remember_for`) you'll stay logged in, as long as your warden session remains intact So this means for example if you keep your browser open for longer than `config.remember_for`, your rememberable cookie will expire, but you'll still be logged in. Currently how `extend_remember_period` works is that it only extends the session when [`Stratgies::Rememberable#authenticate`](https://github.com/heartcombo/devise/blob/master/lib/devise/strategies/rememberable.rb#L30) gets called. This gets called when the user logs in and a strategy is used. But if there's already a user logged in and recognised by warden, then warden [short circuits the strategies](https://github.com/wardencommunity/warden/blob/master/lib/warden/proxy.rb#L334). So the first thing this PR change is to add a hook so that `extend_remember_period` now works on every request. The next bug is that if once `config.remember_for` is up, the user is not currently forgotten. --- lib/devise/hooks/rememberable.rb | 12 ++++ lib/devise/models/rememberable.rb | 4 ++ lib/generators/templates/devise.rb | 7 ++- test/integration/rememberable_test.rb | 80 +++++++++++++++++++++------ 4 files changed, 85 insertions(+), 18 deletions(-) diff --git a/lib/devise/hooks/rememberable.rb b/lib/devise/hooks/rememberable.rb index 345f2f2403..1bd8f5ce4d 100644 --- a/lib/devise/hooks/rememberable.rb +++ b/lib/devise/hooks/rememberable.rb @@ -1,9 +1,21 @@ # frozen_string_literal: true +# This hook runs when a user logs in, if they set the `remember_me` param (eg. from a checkbox in the UI). Warden::Manager.after_set_user except: :fetch do |record, warden, options| scope = options[:scope] if record.respond_to?(:remember_me) && options[:store] != false && record.remember_me && warden.authenticated?(scope) + + Devise::Hooks::Proxy.new(warden).remember_me(record) + end +end + +# This hook runs when we retrieve a user from the session. If the user's remember session should be extended +# we do it here. +Warden::Manager.after_set_user only: :fetch do |record, warden, options| + if record.respond_to?(:extend_remember_me?) && record.extend_remember_me? && + options[:store] != false && warden.authenticated?(options[:scope]) + Devise::Hooks::Proxy.new(warden).remember_me(record) end end diff --git a/lib/devise/models/rememberable.rb b/lib/devise/models/rememberable.rb index 76ac0b8139..469350fe33 100644 --- a/lib/devise/models/rememberable.rb +++ b/lib/devise/models/rememberable.rb @@ -70,6 +70,10 @@ def extend_remember_period self.class.extend_remember_period end + def extend_remember_me? + !!self.class.extend_remember_period + end + def rememberable_value if respond_to?(:remember_token) remember_token diff --git a/lib/generators/templates/devise.rb b/lib/generators/templates/devise.rb index 1dbaddaa6e..68f1d01120 100644 --- a/lib/generators/templates/devise.rb +++ b/lib/generators/templates/devise.rb @@ -169,7 +169,12 @@ # Invalidates all the remember me tokens when the user signs out. config.expire_all_remember_me_on_sign_out = true - # If true, extends the user's remember period when remembered via cookie. + # If true, extends the user's remember period every time the user makes a request. + # As long as the user accesses the site once every `config.remember_for`, they can + # stay logged in forever. + # If false, how long the user will be remembered for is set on initial login, + # and only when the user starts a new session. So users will need to log in + # again every `config.remember_for`. # config.extend_remember_period = false # Options to be passed to the created cookie. For instance, you can set diff --git a/test/integration/rememberable_test.rb b/test/integration/rememberable_test.rb index 62547e762b..3776eb2ae3 100644 --- a/test/integration/rememberable_test.rb +++ b/test/integration/rememberable_test.rb @@ -111,7 +111,7 @@ def cookie_expires(key) assert_redirected_to root_path end - test 'does not extend remember period through sign in' do + test 'logging in does not extend remember_created_at if it is already set' do swap Devise, extend_remember_period: true, remember_for: 1.year do user = create_user user.remember_me! @@ -127,37 +127,83 @@ def cookie_expires(key) end end - test 'extends remember period when extend remember period config is true' do + test 'logging in sets remember_created_at if it is blank' do swap Devise, extend_remember_period: true, remember_for: 1.year do - create_user_and_remember - old_remember_token = nil + user = create_user + user.forget_me! + + assert_nil user.remember_created_at + + sign_in_as_user remember_me: true + user.reload + + assert warden.user(:user) == user + assert_not_nil user.remember_created_at + end + end + + test 'extends remember period on every authenticated request when extend remember period config is true' do + swap Devise, extend_remember_period: true, remember_for: 1.year, timeout_in: nil do + user = create_user_and_remember + + get root_path + assert_response :success - travel_to 1.day.ago do + travel_to 1.day.from_now do + # tomorrow remember period is extended get root_path - old_remember_token = request.cookies['remember_user_token'] + assert_response :success end - get root_path - current_remember_token = request.cookies['remember_user_token'] + travel_to 6.months.from_now do + # 6 months later, still logged in + get root_path + assert_response :success + end - refute_equal old_remember_token, current_remember_token + travel_to 13.months.from_now do + # 13 months after remember_created_at was first set, we are still logged in because period was extended + get root_path + assert_response :success + end + + travel_to 20.months.from_now do + # 20 months after remember_created_at was first set, we are still logged in because period was extended + get root_path + assert_response :success + end + + travel_to 33.months.from_now do + # don't log in for over a year, we get logged out + get root_path + assert_response :redirect + end end end test 'does not extend remember period when extend period config is false' do - swap Devise, extend_remember_period: false, remember_for: 1.year do - create_user_and_remember - old_remember_token = nil + swap Devise, extend_remember_period: false, remember_for: 1.year, timeout_in: nil do + user = create_user_and_remember + + get root_path + assert_response :success - travel_to 1.day.ago do + travel_to 1.day.from_now do get root_path - old_remember_token = request.cookies['remember_user_token'] + assert_response :success end - get root_path - current_remember_token = request.cookies['remember_user_token'] + travel_to 6.months.from_now do + # 6 months later, still logged in + get root_path + assert_response :success + end - assert_equal old_remember_token, current_remember_token + travel_to 13.months.from_now do + # 13 months after remember_created_at was first set, we are no longer logged in because period was not extended + get root_path + assert_response :redirect + end end end From 830cfdf9d8b02fb374af16d136d1a4e25b617746 Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Sun, 7 Nov 2021 11:36:23 +0000 Subject: [PATCH 2/4] Only extend remember_me if remember_me is currently active --- lib/devise/hooks/rememberable.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/devise/hooks/rememberable.rb b/lib/devise/hooks/rememberable.rb index 1bd8f5ce4d..3d69e96c96 100644 --- a/lib/devise/hooks/rememberable.rb +++ b/lib/devise/hooks/rememberable.rb @@ -16,6 +16,7 @@ if record.respond_to?(:extend_remember_me?) && record.extend_remember_me? && options[:store] != false && warden.authenticated?(options[:scope]) - Devise::Hooks::Proxy.new(warden).remember_me(record) + proxy = Devise::Hooks::Proxy.new(warden) + proxy.remember_me(record) if proxy.remember_me_is_active?(record) end end From 55c9f2d14d885f0025b3997188cd6eb6a4671eba Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Sun, 7 Nov 2021 11:39:36 +0000 Subject: [PATCH 3/4] Set a timeout for extend_remember_period tests The remember_me token will be irrelevant unless the session would timeout. A session timeout (which also won't happen if there is a valid remember_me token) is required to logout the user. --- test/integration/rememberable_test.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/rememberable_test.rb b/test/integration/rememberable_test.rb index 3776eb2ae3..b902cc83cf 100644 --- a/test/integration/rememberable_test.rb +++ b/test/integration/rememberable_test.rb @@ -143,7 +143,7 @@ def cookie_expires(key) end test 'extends remember period on every authenticated request when extend remember period config is true' do - swap Devise, extend_remember_period: true, remember_for: 1.year, timeout_in: nil do + swap Devise, extend_remember_period: true, remember_for: 1.year, timeout_in: 6.hours do user = create_user_and_remember get root_path @@ -182,7 +182,7 @@ def cookie_expires(key) end test 'does not extend remember period when extend period config is false' do - swap Devise, extend_remember_period: false, remember_for: 1.year, timeout_in: nil do + swap Devise, extend_remember_period: false, remember_for: 1.year, timeout_in: 6.hours do user = create_user_and_remember get root_path From 308fac2fffd7d6bd17bdf6cab530f4ae1f0df7c6 Mon Sep 17 00:00:00 2001 From: Simon Arlott Date: Sun, 7 Nov 2021 12:37:33 +0000 Subject: [PATCH 4/4] Add more extend_remember_period tests If the session expires then a re-authentication will occur via the remember token, which could automatically extend it via the authentication process instead of the extend process. The remember period needs to be extended even if the session has not yet expired. Arrange for this to happen and then let the session expire late enough that the remember token must have been extended for the user to still be logged in. Ensure that remember me isn't set by the extend process when remember me is not being used for the current session. --- test/integration/rememberable_test.rb | 153 +++++++++++++++++++++++++- 1 file changed, 148 insertions(+), 5 deletions(-) diff --git a/test/integration/rememberable_test.rb b/test/integration/rememberable_test.rb index b902cc83cf..56e69e98b0 100644 --- a/test/integration/rememberable_test.rb +++ b/test/integration/rememberable_test.rb @@ -142,7 +142,7 @@ def cookie_expires(key) end end - test 'extends remember period on every authenticated request when extend remember period config is true' do + test 'extends remember period on every authenticated request when extend remember period config is true (session expires)' do swap Devise, extend_remember_period: true, remember_for: 1.year, timeout_in: 6.hours do user = create_user_and_remember @@ -150,13 +150,13 @@ def cookie_expires(key) assert_response :success travel_to 1.day.from_now do - # tomorrow remember period is extended + # tomorrow, still logged in (by remember me), remember period is extended get root_path assert_response :success end travel_to 6.months.from_now do - # 6 months later, still logged in + # 6 months later, still logged in (by remember me), remember period is extended get root_path assert_response :success end @@ -181,7 +181,7 @@ def cookie_expires(key) end end - test 'does not extend remember period when extend period config is false' do + test 'does not extend remember period when extend period config is false (session expires)' do swap Devise, extend_remember_period: false, remember_for: 1.year, timeout_in: 6.hours do user = create_user_and_remember @@ -189,12 +189,13 @@ def cookie_expires(key) assert_response :success travel_to 1.day.from_now do + # tomorrow, still logged in (by remember me), remember period is extended get root_path assert_response :success end travel_to 6.months.from_now do - # 6 months later, still logged in + # 6 months later, still logged in (by remember me), remember period is extended get root_path assert_response :success end @@ -207,6 +208,148 @@ def cookie_expires(key) end end + test 'extends remember period on every authenticated request when extend remember period config is true (session still active; only session expires)' do + swap Devise, extend_remember_period: true, remember_for: 1.year, timeout_in: 8.months do + user = create_user_and_remember + + get root_path + assert_response :success + + travel_to 1.day.from_now do + # tomorrow, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 6.months.from_now do + # 6 months later, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 13.months.from_now do + # 13 months later, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 20.months.from_now do + # 20 months later, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 29.months.from_now do + # don't access for over 8 months, session is now expired, still logged in (by remember me) + get root_path + assert_response :success + end + + travel_to 42.months.from_now do + # don't access for over a year, session and remember me are now expired, we get logged out + get root_path + assert_response :redirect + end + end + end + + test 'extends remember period on every authenticated request when extend remember period config is true (session still active; both expire at the same time)' do + swap Devise, extend_remember_period: true, remember_for: 1.year, timeout_in: 8.months do + user = create_user_and_remember + + get root_path + assert_response :success + + travel_to 1.day.from_now do + # tomorrow, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 6.months.from_now do + # 6 months later, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 13.months.from_now do + # 13 months later, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 20.months.from_now do + # 20 months later, still logged in (by session), remember period is extended + get root_path + assert_response :success + end + + travel_to 33.months.from_now do + # don't access for over a year, session and remember me are now expired, we get logged out + get root_path + assert_response :redirect + end + end + end + + test 'does not extend remember period when extend period config is false (session still active)' do + swap Devise, extend_remember_period: false, remember_for: 1.year, timeout_in: 8.months do + user = create_user_and_remember + + get root_path + assert_response :success + + travel_to 1.day.from_now do + # tomorrow, still logged in (by session), remember period is not extended + get root_path + assert_response :success + end + + travel_to 6.months.from_now do + # 6 months later, still logged in (by session), remember period is not extended + get root_path + assert_response :success + end + + travel_to 13.months.from_now do + # 13 months after remember_created_at was first set, we are no longer remembered + # because the period was not extended but still logged in by the session + get root_path + assert_response :success + end + + travel_to 22.months.from_now do + # don't access for over a year, session is now expired, we get logged out + get root_path + assert_response :redirect + end + end + end + + test 'do not start remember period when remember me is not used' do + swap Devise, extend_remember_period: true, remember_for: 1.year, timeout_in: 6.hours do + sign_in_as_user + assert_nil request.cookies["remember_user_cookie"] + + get root_path + assert_response :success + + travel_to 1.hour.from_now do + # 1 hour later, still logged in (by session), remember me is not set + get root_path + assert_response :success + assert_nil request.cookies["remember_user_cookie"] + end + + travel_to 8.hours.from_now do + # 8 hours later, session has expired and remember me is not set + get root_path + assert_response :redirect + assert_nil request.cookies["remember_user_cookie"] + end + end + end + test 'do not remember other scopes' do create_user_and_remember get root_path