From 99f4d5082b6c9f0970a092ef96c3b9ade0cef3d0 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 7 Mar 2017 13:27:34 -0500 Subject: [PATCH] FIX: Improve token rotation and increase logging - avoid access denied on bad cookie, instead just nuke it - avoid marking a token unseen for first minute post rotation - log path in user auth token logs --- app/models/user_auth_token.rb | 8 +++++++- ...307181800_add_path_to_user_auth_token_log.rb | 5 +++++ lib/auth/default_current_user_provider.rb | 17 +++++------------ .../auth/default_current_user_provider_spec.rb | 8 -------- spec/models/user_auth_token_spec.rb | 10 ++++++++-- 5 files changed, 25 insertions(+), 23 deletions(-) create mode 100644 db/migrate/20170307181800_add_path_to_user_auth_token_log.rb diff --git a/app/models/user_auth_token.rb b/app/models/user_auth_token.rb index d3564ca08c9..d9139d88b2a 100644 --- a/app/models/user_auth_token.rb +++ b/app/models/user_auth_token.rb @@ -34,6 +34,7 @@ class UserAuthToken < ActiveRecord::Base user_id: info[:user_id], user_agent: info[:user_agent], client_ip: info[:client_ip], + path: info[:path], auth_token: hashed_token) user_auth_token @@ -57,6 +58,7 @@ class UserAuthToken < ActiveRecord::Base user_id: user_token&.user_id, auth_token: token, user_agent: opts && opts[:user_agent], + path: opts && opts[:path], client_ip: opts && opts[:client_ip]) return nil @@ -64,6 +66,7 @@ class UserAuthToken < ActiveRecord::Base if user_token.auth_token != token && user_token.prev_auth_token == token && user_token.auth_token_seen changed_rows = UserAuthToken + .where("rotated_at < ?", 1.minute.ago) .where(id: user_token.id, prev_auth_token: token) .update_all(auth_token_seen: false) @@ -75,6 +78,7 @@ class UserAuthToken < ActiveRecord::Base user_id: user_token.user_id, auth_token: user_token.auth_token, user_agent: opts && opts[:user_agent], + path: opts && opts[:path], client_ip: opts && opts[:client_ip] ) end @@ -96,6 +100,7 @@ class UserAuthToken < ActiveRecord::Base user_id: user_token.user_id, auth_token: user_token.auth_token, user_agent: opts && opts[:user_agent], + path: opts && opts[:path], client_ip: opts && opts[:client_ip]) end @@ -153,7 +158,8 @@ class UserAuthToken < ActiveRecord::Base user_id: user_id, auth_token: auth_token, user_agent: user_agent, - client_ip: client_ip + client_ip: client_ip, + path: info && info[:path] ) true diff --git a/db/migrate/20170307181800_add_path_to_user_auth_token_log.rb b/db/migrate/20170307181800_add_path_to_user_auth_token_log.rb new file mode 100644 index 00000000000..c55e04c2257 --- /dev/null +++ b/db/migrate/20170307181800_add_path_to_user_auth_token_log.rb @@ -0,0 +1,5 @@ +class AddPathToUserAuthTokenLog < ActiveRecord::Migration + def change + add_column :user_auth_token_logs, :path, :string + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 3326f12dabf..4be651472ba 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -12,11 +12,6 @@ class Auth::DefaultCurrentUserProvider TOKEN_COOKIE ||= "_t".freeze PATH_INFO ||= "PATH_INFO".freeze COOKIE_ATTEMPTS_PER_MIN ||= 10 - # allow up to 10 cookie misses, this may be the case - # when requests are delayed in weird ways, for example - # on mobile when coming back online - MAX_COOKIE_MISSES ||= 10 - COOKIE_MISS_KEY ||= "cookie_misses" # do all current user initialization here def initialize(env) @@ -55,6 +50,7 @@ class Auth::DefaultCurrentUserProvider @user_token = UserAuthToken.lookup(auth_token, seen: true, user_agent: @env['HTTP_USER_AGENT'], + path: @env['REQUEST_PATH'], client_ip: @request.ip) current_user = @user_token.try(:user) @@ -131,7 +127,8 @@ class Auth::DefaultCurrentUserProvider if !@user_token.legacy && needs_rotation if @user_token.rotate!(user_agent: @env['HTTP_USER_AGENT'], - client_ip: @request.ip) + client_ip: @request.ip, + path: @env['REQUEST_PATH']) cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token) end elsif @user_token.legacy @@ -141,18 +138,14 @@ class Auth::DefaultCurrentUserProvider end if !user && cookies.key?(TOKEN_COOKIE) - cookie_miss_key = COOKIE_MISS_KEY + cookies[TOKEN_COOKIE] - misses = $redis.get(cookie_miss_key).to_i + 1 - $redis.setex(cookie_miss_key, 1.hour.to_i, misses) - if misses > MAX_COOKIE_MISSES - cookies.delete(TOKEN_COOKIE) - end + cookies.delete(TOKEN_COOKIE) end end def log_on_user(user, session, cookies) @user_token = UserAuthToken.generate!(user_id: user.id, user_agent: @env['HTTP_USER_AGENT'], + path: @env['REQUEST_PATH'], client_ip: @request.ip) cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token) diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index bbdde400fc5..a80a1b4848c 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -208,15 +208,7 @@ describe Auth::DefaultCurrentUserProvider do end it "correctly removes invalid cookies" do - cookies = {"_t" => SecureRandom.hex} - - (Auth::DefaultCurrentUserProvider::MAX_COOKIE_MISSES).times do - provider('/').refresh_session(nil, {}, cookies) - end - - expect(cookies.key?("_t")).to eq(true) - provider('/').refresh_session(nil, {}, cookies) expect(cookies.key?("_t")).to eq(false) end diff --git a/spec/models/user_auth_token_spec.rb b/spec/models/user_auth_token_spec.rb index d516c29589f..11ed5f82126 100644 --- a/spec/models/user_auth_token_spec.rb +++ b/spec/models/user_auth_token_spec.rb @@ -227,13 +227,19 @@ describe UserAuthToken do ).count).to eq(1) fake_token = SecureRandom.hex - UserAuthToken.lookup(fake_token, seen: true, user_agent: "bob", client_ip: "127.0.0.1") + UserAuthToken.lookup(fake_token, + seen: true, + user_agent: "bob", + client_ip: "127.0.0.1", + path: "/path" + ) expect(UserAuthTokenLog.where( action: "miss token", auth_token: UserAuthToken.hash_token(fake_token), user_agent: "bob", - client_ip: "127.0.0.1" + client_ip: "127.0.0.1", + path: "/path" ).count).to eq(1)