Allow customization of session expiration time

[rnewman57]

Currently the expiration time of the "remember_token" cookie is hardcoded as "1 year from now" in Clearance::Authentication::InstanceMethods#sign_in :

      cookies[:remember_token] = {
        :value   => user.remember_token,
        :expires => 1.year.from_now.utc
      }

I'd like you to consider changing this so that the :expires value comes from a call to a new protected method (I'll call it Clearance::Authentication::InstanceMethods# session_expiration_time for want of a better name), which a customizer could override in his own subclass of Clearance::SessionsController.

The default would still be 1.year.from_now to match Clearance's current behavior. If session_expiration_time returns nil, the "remember_token" cookie would expire at the end of the current browser session.

With this change, if I wanted the session to expire after 1 week, I could just declare in my SessionsController:

def session_expiration_time 
   1.week.from_now
end

and if I wanted to implement a 'Remember me' flag on the sign-in page:

def create @remember_me = params[:session][:remember_me] super end

def session_expiration_time @remember_me ? 1.week.from_now : nil end

If you like this idea, I can try to create a patch or branch for it.

[croaky]

This isn't something we've ever wanted, to my knowledge. Why do you want it?

I suppose if you're not changing the default behavior and adding a custom hook, we'd be fine with it but I wonder why you think you need it.

If you write the patch, please don't use a ternary (style of the library) and try to name the hook something that fits in with other methods in Clearance::User or makes sense for how it's used in the controller. Off the top of my head,

remember_token_expires_at

[rnewman57]

I wanted to have a 'Remember me' checkbox as is commonly found on sites such as Facebook and LiveJournal, and which Clearance used to support until September. This hook seemed like the easiest way to accomplish that without changing Clearance's current default behavior.

remember_token_expires_at is a fine name for the hook.

(Why is a ternery operator a bad thing to use? That was only in my subclassing example, not in the code I proposed for Clearance itself.)

[rnewman57]

I committed a fix for this at

http://github.com/rnewman57/clearance/commit/47595fd610f3a010cf29fbc1326e7c744f90cb7b

I called the new hook remember_token_expires_at, as you suggested above.

The code change to Clearance itself was simple. Most of this commit consists of new testing code, including a should_set_cookie Shoulda macro which possibly should be added to the Shoulda project (see http://github.com/thoughtbot/shoulda/issues/86 ). Rails does not make it easy to test that the expiration date of a cookie is correctly set to what you want.

This commit also includes my previous fixes to issues #78, #77, and #23 .

[andhapp]

Regarding this particular issue, I think something like the session expiration time should live in the configuration file just like HOST. This would mean minimal changes in the library and for the users. What do you say?

[croaky]

andhapp, that makes sense

[rnewman57]

but declaring the expiration time as a configuration constant would make it impossible to implement the 'remember me' checkbox.

This is why I implemented the expiration time as a hook in my commit above, so that the application could decide at runtime what expiration time to use (including nil for a session-only cookie).

[Yardboy]

I need to throw my number in the hat supporting the request that something be done to enable customization of session expiration. I have an application built for education that shows sensitive data about teacher performance. Teachers share computers all the time, and though we preach time and again the importance of logging out, we cannot count on them being savvy enough to always do it. The district does not even want remember_me available, at this point, much less defaulted.

I realize Clearance is "opinionated", and I'm on board with that for most of my apps, but the current opinion leaves sensitive data apps out in the cold. At this point, I have an app-wide before_filter that resets the remember_me cookie, but an option such as rnewman proposes seems much cleaner.

[andhapp]

How about adding the remember_me code in a different module which only gets included if someone has specified it via the config file. The expires_at time can also go in the same config file. Just some food for thought not sure how will this work out exactly but I guess something like:

module Clearance
 module RememberMe
   def self.included(model)
       # code goes here
   end
 end
end

Config can be something like:

Clearance.configure do |config|
   config.mailer_sender = "donotreply@noreply.com"
   config.remember_me = true
end

I would like to get Dan's opinion on this, just to see if this is something acceptable (by their standards) and would get merged into Clearance or not.

[rnewman57]

The fix that I already committed above (see March 6 comment) seems a lot simpler than this.

[croaky]

I liked the idea of setting the expiration date via the configuration block, and even committed it:

http://github.com/thoughtbot/clearance/commit/1b4f2cb6c93b2e29d5b46103c77deba09da92e6f

Then, I realized that time will be at the time the app starts. If we can alter that so the remember_token_expires_at takes a value from lib/authentication.rb, I think that'd be the right place for everything.

Gotta run now. If someone wants to take a stab at it, go for it. Otherwise, I'll do it tomorrow.

Thanks, Ron, for pushing for this and the initial implementation. I snagged your cookie test helper and added you to the contributors list.

[rnewman57]

Thanks. I'm happy to see static configuration added, as long as that isn't in place of being able to dynamically select the expiration at runtime, which was the original motivation behind my work here. I don't see a way to do the dynamic selection in your commit.

Perhaps we can talk more about this tomorrow, either on IRC or in person? (I'm local to you.)

[croaky]

I didn't intend for it to be static, just wasn't thinking clearly at end of the day. :/

We should either do your approach of a method that can be overridden in app controller or wrap the time in a lambda & call it from lib/authentication. I still like the idea of as much config as possible moving to the configure block so there's one place for it.

[rnewman57]

Instead of putting a Time value such as 3.days.from_now in the configuration, how about putting a duration (e.g. 3.days) instead? That avoids the problem you mention three comments above this one.

[croaky]

Love the idea. Committed it:

http://github.com/thoughtbot/clearance/commit/5fc0a1af488b06c53562150e6810d406256f519c

Also renamed the configuration option to be cookie_duration. Shorter and sweeter, I think.

Thanks for all the input, folks. I think this is nice improvement.

[rnewman57]

Thanks. But this issue should not be 'closed' until we add the ability to override as well (so that cookie duration can be chosen at runtime and not just statically configured)

[croaky]

Re-opened as this doesn't solve Yardboy's issue.

Philosophically, I'm still anti-"remember me" checkbox, forcing the user to choose. However, I want to cleanly support the programmer's desire to set the cookie expiration to nil, which keeps the user signed in for the session only.

To do that, I'm going to implement the lambda as described earlier.

[rnewman57]

OK, how does this sound?

Clearance.configuration.cookie_duration is one of:

- not set => defaults to 1.year (which is current Clearance behavior)
- a duration in seconds
- nil, which means a session cookie that expires when the browser exits
- a lambda (Proc), which takes the SessionsController as an argument and returns either a duration in seconds or nil

The lambda needs the SessionsController argument, or else it doesn't have enough context to implement logic such as the 'remember me' checkbox.

[andhapp]

Would this not work?

lambda { params[:session][:remember_me] ? 1.week.from_now : nil }

This would be called from within lib/authentication.rb. I've just used the code in the first comment.

[rnewman57]

that works IF the lambda is called in a way so that 'self' is set to the SessionsController .

[croaky]

I'm not interested in supporting a "remember me" checkbox option. I DO want to support Yardboy's use case of having users sign in only for the session, then automatically sign out when they close their window. thoughtbot is starting a medical application were we should do the same thing.

Committed:

http://github.com/thoughtbot/clearance/commit/8c37063414bb441a777d88be1fc47b39c438fc1a

[rnewman57]

What will the value of 'self' be when this lambda is called? How will the lambda get access to the SessionController's instance variables or methods (such as 'params') ?

[croaky]

It doesn't need to do either of those things, it just needs to be a time or an empty lambda.

[rnewman57]

But if the lambda can't get to the SessionController to call methods on it, then the lambda doesn't get you anything that you don't get from a static configuration variable.

[andhapp]

@Dan - I was actually under the impression that the lambda is going to be introduced to support remember_me feature, but that's not the case.

@rnewman57 - This does not matter now but any request that hits Clearance::SessionsController will have all the methods of ApplicationController since it inherits from it. ApplicationController includes the module Clearance::Authentication so the lambda will be called in the context of an ApplicationController which will have access to params. Please correct me if this seems incorrect. Thanks. You can have a look at lib/authentication.rb as it uses params[:return_to] in one of the methods.

[rnewman57]

I think that would be true only if you change

:expires => Clearance.configuration.cookie_expiration.call

to

:expires => self.instance_eval(&Clearance.configuration.cookie_expiration)

in the sign_in method of lib/clearance/authentication.rb

[rnewman57]

I suggest adding at least two more tests:

set Clearance.configuration.cookie_expiration to a non-default value such as lambda {1.week.from_now.utc} and then check should_set_cookie("remember_token", "old-token", 1.week.from_now)

set Clearance.configuration.cookie_expiration to lambda {nil} and then check should_set_cookie("remember_token", "old-token", nil)

[rnewman57]

Here are the changes I made to test/controllers/sessions_controller_test.rb to test this new feature:

http://gist.github.com/362511

[rnewman57]

I changed the Clearance.configuration.cookie_expiration lambda call to use instance_eval, as I described on April 11 (3 comments above this comment).

I also added a test to ensure that the lambda is now being called in a context where 'self' is the current controller (and has access to 'params'). Both changes are in this diff file:

http://gist.github.com/390230

[andhapp]

@rnewman57 - Hello, it has been a while...what do we gain from the changes that you have made?

[rnewman57]

See all the comments above from April 10 -- it allows, for instance,

Clearance.configuration.cookie_expiration = lambda { params[:session][:remember_me] ? 1.week.from_now : nil }

to work.