Matt, I'm interested in hearing what you have to say.
As I've gone through and implemented this I've learned a lot about the reference implementation and raised several questions about solving the same problems in an idiomatically Ruby way. I've noticed that the method of expiring tickets is purely based on ticket creation time regardless of activity. So if the timeout is set to 2 days but the user only hits one page and leaves for the weekend they potentially have a session sitting there waiting to get taken over by someone with physical access to their machine. The reference impl handles this by having TGT's expire based on either absolute lifetime (Default: 8 hours) or activity (Default: 2 hours). Then for long session functionality the wiki sets up the expiration policy purely based on the activity timeout but there isn't anything preventing an admin from using the dual absolute/activity timeout to force users to login every so often. With the expiration policy classes I've implemented so far we've only got one or the other available for any given situation but adding one that checks both isn't difficult. But all of this would require tracking user activity, something we don't currently have facilities for but shouldn't be difficult to add.
Something else I've noticed is that their consumable tickets get reaped on the next expiration sweep without looking at the timeout. This is done through a counter on the ticket for # of uses rather than an explicit consumed timestamp. The reason I mention this is that some use cases for the TGT could use this same functionality, it may fall into YAGNI but it may not.
Finally I've changed my approach to the problem based on what I've seen from the reference implementation and some of the issues they've had and some I could see coming up. Rather than storing the expiration policy class or any info about it in the DB a single now lives in a class level instance variable set at init time. The reference impl stores a serialized object in the DB as part of the ticket so each ticket has its own instance of the policy. There are a few problems I see with this:
- In the event we want to change the timeout policy for existing tickets there really isn't any way to without modifying EVERY ticket in the DB
- If a ticket gets assigned the NeverExpirePolicy it would have to be manually cleaned up.
- Each instance takes up some memory, while it is a small amount if you accidentally loaded 100k tickets at once it could add to the already severe problem
- Object instantiation takes time, while it doesn't take much why do it if we don't have to.
I don't know that they've ever seen #'s 1 or 2 being an issue but I know that the first one would be quite inconvenient. They do however have problems with #3 because of the way they iterate through all of the tickets to check that they are expired. Our current implementation would require this same iteration although if done in a sane way (find_each) it shouldn't kill a server. We could take advantage of ActiveRecord and have the ticket classes delegate the construction of some sql to their expiration policies enabling us to only grab the expired tickets much the same way they are currently handled in Ticket.cleanup(max_lifetime). To implement this in an easy way we'd probably need to break compatibility with ActiveRecord 2.3.x. Problem #4 isn't really likely to be a problem but on a busy system ever cycle counts so why waste them. This last point is taken care of by the use of class instance variables but AbstractExpirationPolicy.new also caches each instantiated policy (each class has its own cache) by timeout length so if we set all of the tickets to use DefaultExpirationPolicy with a 500 second timeout we'll actually only end up with one object that's shared between them all.