Use Redis session store #474

typesend · 2016-09-13T23:15:41Z

See 18F/identity-private#785

monfresh

One issue that this change exposes is that when the session is removed from redis, when the user signs back in, they will see "Oops. Something went wrong. Please sign in again" due to this.

I believe this used to be the case before, except we didn't see it because we didn't clean up the expired sessions right away.

I'm not sure if this is a bug or if this is how the authenticity token works. Either way, we should fix this because it's a poor user experience to have to essentially sign in twice every time your session expires and is cleaned up.

monfresh · 2016-09-14T19:38:47Z

config/initializers/session_store.rb

+  key: '_upaya_session',
+  redis: {
+    driver: :hiredis,
+    expire_after: Figaro.env.session_timeout_in!.to_i.minutes,


We prefer requiring Figaro keys proactively via config/initializer/figaro.rb. session_timeout_in is already defined there as a required key, so we don't need the ! here. Same with domain_name below.

monfresh · 2016-09-14T19:40:22Z

config/initializers/session_store.rb

+    driver: :hiredis,
+    expire_after: Figaro.env.session_timeout_in!.to_i.minutes,
+    key_prefix: "#{Figaro.env.domain_name!}:session:",
+    url: Figaro.env.redis_url!


Please add redis_url to config/initializer/figaro.rb (please keep the order alphabetical) instead of using the bang version.

monfresh · 2016-09-14T19:53:31Z

config/initializers/session_store.rb

+  },
+  # on_redis_down: ->(e, env, sid) { do_something_will_ya!(e) },
+  # on_session_load_error: ->(e, sid) { do_something_will_ya!(e) },
+  serializer: :marshal


Why Marshal and not JSON? JSON is the default in Rails 4+, and what we are currently using for cookies. See cookies_serializer.rb.

Marshal is what the ActiveRecord session store is using now

Marshal might be slightly faster than JSON

We don't store our sessions in cookies, so config/initializers/cookies_serializer.rb may be ignored?

Ah, yes. I was confusing the two.

jessieay · 2016-10-03T17:38:25Z

What is the status on this pull request? Looks like @monfresh reviewd 19 days ago but there haven't been updates. Are we going to close and move in a different direction or is there a plan to update this?

monfresh · 2016-10-03T18:40:49Z

I believe we still need to complete this. @typesend can confirm. Ben, do you still have time to finish this, or should we assign it to someone else?

@jessieay If I recall correctly, what is holding this up is this failing test: https://github.com/18F/identity-idp/blob/master/spec/features/users/sign_in_spec.rb#L126-L149

@pkarman, @typesend and I had an in-person discussion about this, and we think the solution is to delete the session cookie whenever the session is removed from Redis.

monfresh · 2016-10-06T02:07:18Z

Paging @typesend.

typesend · 2016-10-06T17:00:55Z

Go ahead and reassign. I have too many things on my plate. :/ If there isn't already a ticket for encrypting redis / elasticache session data we need that as well per DHS' requirement (because elasticache infrastructure not considered as secure/isolated as other aws services.)

pkarman · 2016-10-06T17:05:56Z

encryption of session already prepped in #516

pkarman · 2016-10-06T20:40:27Z

I'm going to wait on this till #516 is merged, which is in turn blocked on #498

pkarman · 2016-10-07T18:34:29Z

for reference: the failing test is due to http://stackoverflow.com/a/7744647 -- invalid authenticity token error, because the (temp, un-authenticated) session has expired.

The CSRF token is stored in the session on the server side, and compared against the incoming request to verify they match.

This was not a problem with the ActiveRecord session store, for a bad reason: we currently don't auto-delete the ActiveRecord sessions when they expire. That means the CSRF check succeeds, even though the session is expired.

We can skip the CSRF check for the session create action only, where it is least needed (since the user is not yet authenticated, cross-site attacks aren't effective).

index 2f6b9ae..a01803f 100644
--- a/app/controllers/users/sessions_controller.rb
+++ b/app/controllers/users/sessions_controller.rb
@@ -3,6 +3,7 @@ module Users
     include ::ActionView::Helpers::DateHelper

     skip_before_action :session_expires_at, only: [:active]
+    skip_before_action :verify_authenticity_token, only: [:create]

     def create
       track_authentication_attempt(params[:user][:email])

pkarman · 2016-10-07T20:09:10Z

disabling CSRF check on sessions#create was deemed to risky.

instead, JS-driven timer with warning modal on the login page to prevent users from attempting to submit the form if we know in advance it is going to fail.

pkarman · 2016-10-07T20:23:31Z

I'm going to remove the WIP from this so we can get it reviewed.

However, I'm also going to mark it DO NOT MERGE until we can resolve the session encryption PR in #516.

jessieay · 2016-10-17T23:20:58Z

Added the session timeout modal separately in this PR https://github.com/18F/identity-idp/pull/578/files

pkarman · 2016-10-24T20:10:16Z

I'm marking this as ready to review and merge.

jessieay

👏 just two tiny comments from me

jessieay · 2016-10-24T20:37:59Z

config/initializers/session_store.rb

+    key_prefix: "#{Figaro.env.domain_name}:session:",
+    url: Figaro.env.redis_url
+  },
+  # on_redis_down: ->(e, env, sid) { do_something_will_ya!(e) },


what are these commented line about?

jessieay · 2016-10-24T20:42:16Z

lib/idle_session_expiration_store.rb

+  def update_session_ttl(sid)
+    expiry = default_options[:expire_after]
+    redis.expire(prefixed(sid), expiry)
+  rescue Errno::ECONNREFUSED, Redis::CannotConnectError => error


do we need / should we have a test for this?

monfresh · 2016-10-24T20:56:46Z

config/initializers/session_store.rb

+  key: '_upaya_session',
+  redis: {
+    driver: :hiredis,
+    expire_after: Figaro.env.session_timeout_in_minutes.to_i,


Looks like you accidentally removed the .minutes from the end here? I have this branch locally and it has the .minutes at the end. Otherwise, won't it expire every 8 seconds?

Or maybe I added it in my local branch when I realized the bug 😄

Given how often we make this mistake, I wonder if we should perhaps use seconds for the Figaro setting, but that would be another PR.

I just updated my PR to use Devise.timeout_in so that we are dealing with an integer rather than a string. Should we do that here, too? https://github.com/18F/identity-idp/pull/596/files

I think config files relying on one another for initialization order can get messy fast. I'd prefer to always pull from Figaro.

good call. I like the idea of having the .to_i.minutes call being in one place -- but perhaps not tenable

monfresh · 2016-10-24T21:13:42Z

lib/idle_session_expiration_store.rb

+    expiry = default_options[:expire_after]
+    redis.expire(prefixed(sid), expiry)
+  rescue Errno::ECONNREFUSED, Redis::CannotConnectError => error
+    on_redis_down.call(error, env, sid) if on_redis_down


Given that the on_redis_down option is not set in our session store config, this won't do anything. So, we should either get rid of this section, or actually configure on_redis_down to do something, and in that case, we can get rid of if on_redis_down because it will be present.

This will also address the reek/rubocop issues.

monfresh · 2016-10-24T21:22:54Z

lib/idle_session_expiration_store.rb

+    on_redis_down.call(error, env, sid) if on_redis_down
+    return false
+  end
+end


Do we know why we even need this monkey patch? Doesn't the gem expire sessions already based on the expire_after option? I will test now.

I have no idea. I was just going off what @typesend had written.

Looks like the gem works as expected out of the box. Here's how I tested:

I made the session store look like this:

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 }, # on_redis_down: ->(e, env, sid) { do_something_will_ya!(e) }, # on_session_load_error: ->(e, sid) { do_something_will_ya!(e) }, serializer: :marshal } Rails.application.config.session_store :redis_session_store, options

redis-cli flushall

bin/setup

Set Figaro.session_timeout_in_minutes to 1

foreman start

Visit localhost:3000

Open a Redis console with redis-cli, select the "0" database, select 0, and look at all the keys: keys *.

Verify there is a key that starts with localhost:3000:session.

Wait 1 minute

run keys * again and verify the key is gone

Also tested signing in and letting session time out on its own. When the user is signed out, the previous session they had is removed from Redis, and then they immediately get a new session. If they wait another minute, that session gets removed as well and no new sessions will appear in Redis until a new request is made.

I also tested that the expiry time automatically resets every time a new request is made. So, if a user signs in at 12:00:00, then makes their last request 30 seconds later, the session times out at 12:01:30, not 12:01:00.

pkarman · 2016-10-24T21:49:56Z

@monfresh thanks for the thorough test. I have zapped the custom subclass.

let's wait to merge this till after #596 is resolved.

monfresh · 2016-10-24T21:54:42Z

config/initializers/session_store.rb

@@ -1 +1,14 @@
-Rails.application.config.session_store :active_record_store, key: '_upaya_session'
+require 'idle_session_expiration_store'


Please zap this too.

**Why**: * speed of session access * auto-deletion of expired sessions * separation of data stores (db and session)

pkarman · 2016-10-25T15:15:50Z

ok @monfresh this should be good to go

monfresh

LGTM

typesend added the status - work in progress label Sep 13, 2016

typesend force-pushed the redis-self-expiring-sessions branch 3 times, most recently from 650ce0c to 4324a0f Compare September 14, 2016 03:38

monfresh suggested changes Sep 14, 2016

View reviewed changes

typesend changed the title ~~Swap session store gems and configure initializer.~~ [WIP] Swap session store gems and configure initializer. Sep 14, 2016

amoose mentioned this pull request Sep 21, 2016

Require 2FA for admin to access constrained route #486

Merged

pkarman mentioned this pull request Oct 4, 2016

Encrypt session at rest #516

Closed

pkarman self-assigned this Oct 6, 2016

pkarman force-pushed the redis-self-expiring-sessions branch from 8844a75 to 82f7ffb Compare October 7, 2016 19:13

pkarman changed the title ~~[WIP] Swap session store gems and configure initializer.~~ Use Redis session store Oct 7, 2016

pkarman added status - ready for review status - do not merge and removed status - work in progress labels Oct 7, 2016

This was referenced Oct 17, 2016

Add session timeout modal for logged out users #578

Merged

Use .minutes for Figaro env var #596

Merged

pkarman mentioned this pull request Oct 24, 2016

Tests showing sign in after session expiration+deletion #601

Closed

pkarman force-pushed the redis-self-expiring-sessions branch 3 times, most recently from cb14936 to 8dd8765 Compare October 24, 2016 19:54

pkarman force-pushed the redis-self-expiring-sessions branch from 8dd8765 to 0079ee5 Compare October 24, 2016 19:59

pkarman removed the status - do not merge label Oct 24, 2016

jessieay reviewed Oct 24, 2016

View reviewed changes

monfresh reviewed Oct 24, 2016

View reviewed changes

pkarman force-pushed the redis-self-expiring-sessions branch from 9ec0e86 to f4b5b32 Compare October 24, 2016 21:48

monfresh reviewed Oct 24, 2016

View reviewed changes

pkarman force-pushed the redis-self-expiring-sessions branch 3 times, most recently from 1904181 to 0232acf Compare October 25, 2016 14:17

Use Redis session storage

9356d69

**Why**: * speed of session access * auto-deletion of expired sessions * separation of data stores (db and session)

pkarman force-pushed the redis-self-expiring-sessions branch from 2433b89 to 9356d69 Compare October 25, 2016 15:13

monfresh approved these changes Oct 25, 2016

View reviewed changes

monfresh merged commit bff4570 into master Oct 25, 2016

monfresh deleted the redis-self-expiring-sessions branch October 25, 2016 17:52

		@@ -1 +1,14 @@
		Rails.application.config.session_store :active_record_store, key: '_upaya_session'
		require 'idle_session_expiration_store'

Use Redis session store #474

Use Redis session store #474

Conversation

typesend commented Sep 13, 2016

monfresh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay commented Oct 3, 2016

monfresh commented Oct 3, 2016

monfresh commented Oct 6, 2016

typesend commented Oct 6, 2016 via email

pkarman commented Oct 6, 2016

pkarman commented Oct 6, 2016

pkarman commented Oct 7, 2016

pkarman commented Oct 7, 2016

pkarman commented Oct 7, 2016

jessieay commented Oct 17, 2016

pkarman commented Oct 24, 2016

jessieay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessieay Oct 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monfresh Oct 24, 2016 • edited

Choose a reason for hiding this comment

pkarman commented Oct 24, 2016

Choose a reason for hiding this comment

pkarman commented Oct 25, 2016

monfresh left a comment

Choose a reason for hiding this comment

jessieay Oct 24, 2016 •

edited

monfresh Oct 24, 2016 •

edited