Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Only skip session timeout check for create and destroy. #2269

Closed
wants to merge 1 commit into from

3 participants

@drewish

My understanding of the code is that when a user hits the login page with an expired session, they get redirected to signed_in_root_path where the expired session is detected, causing them to be redirected back to the login page. It seems like it would be more efficent to just check the expiration in the first place and avoid the redirect cycle.

We discovered this on a site where we're using devise to authenticate users as part of an an OAuth flow. We discovered that if the user had logged in, waited for the session to expire, and then specifically loaded the sign in page (hitting Devise::SessionsController#new), they'd be redirected right through into the OAuth flow without being timed out. We tracked it down to the fact that we'd overridden after_sign_in_path_for to send them off to the right callback URL. In that case,
request.env["devise.skip_timeout"] is true, the require_no_authentication filter redirects them through the OAuth flow, and the session never expires.

We'd worked around it by just removing the filter e.g. _process_action_callbacks.reject! {|cb| cb.raw_filter.is_a?(Proc) } which seemed a little more hacky than it needed to be since the filter was a proc rather than a symbol.

@josevalim
Owner

@drewish thanks for the PR. Unfortunately it causes the build to fail and it does not have any tests. Could you please take a look? Thanks.

@drewish

Ah yeah I had some trouble getting the tests running the first time. I was able to get them running over the weekend and see failed test you pointed out. I'll spend some more time on this today and update the commit.

@drewish

I made a couple changes to this and added a new test but now the 'time out is not triggered on sign in' test is failing. I'm not quite sure why that is.

@teleological

I notice that if I keep the change to SessionsController (only skipping timeout for create and destroy) and revert the change to the timeoutable hook, i.e. allow last_request_at to be updated, all tests pass, including the new test, "expired session is not extended by sign in page".

@teleological

I pushed the change described above to a branch at teleological/devise@41fdc49, leaving @drewish as the author. Not setting devise.skip_timeout for sessions#new is sufficient: When a user with an expired session requests sessions#new, the timed-out session is detected and invalidated and control never reaches the part that would update last_request_at.

@josevalim
Owner

Merged the important bits, thank you!

@josevalim josevalim closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 12, 2013
  1. @drewish
This page is out of date. Refresh to see the latest.
View
2  app/controllers/devise/sessions_controller.rb
@@ -1,7 +1,7 @@
class Devise::SessionsController < DeviseController
prepend_before_filter :require_no_authentication, :only => [ :new, :create ]
prepend_before_filter :allow_params_authentication!, :only => :create
- prepend_before_filter { request.env["devise.skip_timeout"] = true }
+ prepend_before_filter :only => [:create, :destroy] { request.env["devise.skip_timeout"] = true }
# GET /resource/sign_in
def new
View
2  lib/devise/hooks/timeoutable.rb
@@ -18,7 +18,7 @@
throw :warden, :scope => scope, :message => :timeout
end
- unless env['devise.skip_trackable']
+ unless env['devise.skip_timeout'] || env['devise.skip_trackable']
warden.session(scope)['last_request_at'] = Time.now.utc
end
end
View
14 test/integration/timeoutable_test.rb
@@ -68,6 +68,20 @@ def last_request_at
assert_contain 'You are signed in'
end
+ test 'expired session is not extended by sign in page' do
+ user = sign_in_as_user
+ get expire_user_path(user)
+ assert warden.authenticated?(:user)
+
+ get "/users/sign_in"
+ assert_redirected_to "/users/sign_in"
+ follow_redirect!
+
+ assert_response :success
+ assert_contain 'Sign in'
+ assert_not warden.authenticated?(:user)
+ end
+
test 'admin does not explode on time out' do
admin = sign_in_as_admin
get expire_admin_path(admin)
Something went wrong with that request. Please try again.