Divorce Redis session and cookie expiration

[pkarman]

Why: We want the cookie to expire if/when the browser is closed,
but the Redis server-side expiration to expire after the configured
number of minutes.

[amoose]

There may be a simpler solution here. @jgrevich did some digging already and if we remove the expiry of the cookie, it becomes a session cookie and is deleted when the browser is closed.

Unlike other cookies, session cookies do not have an expiration date assigned to them, which is how the browser knows to treat them as session cookies.

• https://www.sjoerdlangkemper.nl/2016/04/14/preventing-csrf-with-samesite-cookie-attribute/

[monfresh]

We discussed this very same issue during the ICAM project but it wasn't deemed serious enough to address and has some consequences to think about. I'm pasting @amoose's comment from the ICAM issue:

+1 to @jgrevich's comment; closing the browser window can trigger logout, 
but could cause undesired behavior if the user has multiple tabs open.

What about active SP sessions?

Option 1: sign user out and deactivate all Identities in ICAM. 
Existing active sessions in SPs will be disabled when their cookies expire.

Option 2: sign user out and send LogoutRequest to each active SP session to 
deactivate each Identity and SP session. 
This approach requires the ability to send async requests from the application 
directly to the SPs.

[amoose]

Yes, thank you for bringing this up, Moncef. This cookie expiry does not address session expiry in a SP but it does ensure that login.gov won't preserve a session when the browser is closed and reopened.

[pkarman]

upstream PR here roidrage/redis-session-store#84

The issue is that expire_after controls both the Redis TTL and the cookie expire value.

[monfresh]

Note that the session will only be preserved if less than 8 minutes have passed between the closing and reopening of the browser AND a new request is made. If the tab wasn't closed, re-opening the browser doesn't make a new web request. Are we saying 8 minutes is too much of a risk? Would lowering the session_timeout_in_minutes value work?

[jgrevich]

@monfresh thanks for digging that up. I tested locally and by removing the expiration date does not affect multiple tabs or SP sessions. It only affects the IdP session when the browser is closed:

diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb
index 616eee95..105c6b64 100644
--- a/config/initializers/session_store.rb
+++ b/config/initializers/session_store.rb
@@ -4,7 +4,6 @@ options = {
   key: '_upaya_session',
   redis: {
     driver: :hiredis,
-    expire_after: Figaro.env.session_timeout_in_minutes.to_i.minutes,
     key_prefix: "#{Figaro.env.domain_name}:session:",
     url: Figaro.env.redis_url,
   },

It looks like we may have to do a bit more to make sure session data is wiped from redis as well.

[pkarman]

@jgrevich when you remove the expire_after config, then the data doesn't get expired from Redis.

See #1130 (comment)

[pkarman]

Here's how I understand the problem.

Because the Rails session cookie has an explicit expiration value, most browsers will persist the cookie until that expiration time. That means that exiting the browser (quitting) does not delete the session cookie.

The simple solution would be to not set an explicit expiration value on the cookie, so that the cookie is destroyed when the browser exists. That's straightforward.

What's problematic is that Rails session code for the Redis driver (and others) does not distinguish between the expire value for the server-side storage and the expire value for the browser-side cookie. It uses the same value for both.

So if we remove the expiration value to fix the cookie persistence, we also stop automatically expiring the session data on the server side.

This PR addresses that problem, by splitting the configuration into 2 different settings: one for the cookie expiration (expire_after the existing config name), and one for the server-side storage expiration (ttl the new config name for the server storage).

[pkarman]

New PR 18F/redis-session-store#1

[zachmargolis]

LGTM!

[monfresh]

lgtm