mirror of
https://github.com/discourse/discourse.git
synced 2025-05-22 19:52:43 +08:00
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
This commit is contained in:
@ -34,6 +34,7 @@ class UserAuthToken < ActiveRecord::Base
|
|||||||
user_id: info[:user_id],
|
user_id: info[:user_id],
|
||||||
user_agent: info[:user_agent],
|
user_agent: info[:user_agent],
|
||||||
client_ip: info[:client_ip],
|
client_ip: info[:client_ip],
|
||||||
|
path: info[:path],
|
||||||
auth_token: hashed_token)
|
auth_token: hashed_token)
|
||||||
|
|
||||||
user_auth_token
|
user_auth_token
|
||||||
@ -57,6 +58,7 @@ class UserAuthToken < ActiveRecord::Base
|
|||||||
user_id: user_token&.user_id,
|
user_id: user_token&.user_id,
|
||||||
auth_token: token,
|
auth_token: token,
|
||||||
user_agent: opts && opts[:user_agent],
|
user_agent: opts && opts[:user_agent],
|
||||||
|
path: opts && opts[:path],
|
||||||
client_ip: opts && opts[:client_ip])
|
client_ip: opts && opts[:client_ip])
|
||||||
|
|
||||||
return nil
|
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
|
if user_token.auth_token != token && user_token.prev_auth_token == token && user_token.auth_token_seen
|
||||||
changed_rows = UserAuthToken
|
changed_rows = UserAuthToken
|
||||||
|
.where("rotated_at < ?", 1.minute.ago)
|
||||||
.where(id: user_token.id, prev_auth_token: token)
|
.where(id: user_token.id, prev_auth_token: token)
|
||||||
.update_all(auth_token_seen: false)
|
.update_all(auth_token_seen: false)
|
||||||
|
|
||||||
@ -75,6 +78,7 @@ class UserAuthToken < ActiveRecord::Base
|
|||||||
user_id: user_token.user_id,
|
user_id: user_token.user_id,
|
||||||
auth_token: user_token.auth_token,
|
auth_token: user_token.auth_token,
|
||||||
user_agent: opts && opts[:user_agent],
|
user_agent: opts && opts[:user_agent],
|
||||||
|
path: opts && opts[:path],
|
||||||
client_ip: opts && opts[:client_ip]
|
client_ip: opts && opts[:client_ip]
|
||||||
)
|
)
|
||||||
end
|
end
|
||||||
@ -96,6 +100,7 @@ class UserAuthToken < ActiveRecord::Base
|
|||||||
user_id: user_token.user_id,
|
user_id: user_token.user_id,
|
||||||
auth_token: user_token.auth_token,
|
auth_token: user_token.auth_token,
|
||||||
user_agent: opts && opts[:user_agent],
|
user_agent: opts && opts[:user_agent],
|
||||||
|
path: opts && opts[:path],
|
||||||
client_ip: opts && opts[:client_ip])
|
client_ip: opts && opts[:client_ip])
|
||||||
end
|
end
|
||||||
|
|
||||||
@ -153,7 +158,8 @@ class UserAuthToken < ActiveRecord::Base
|
|||||||
user_id: user_id,
|
user_id: user_id,
|
||||||
auth_token: auth_token,
|
auth_token: auth_token,
|
||||||
user_agent: user_agent,
|
user_agent: user_agent,
|
||||||
client_ip: client_ip
|
client_ip: client_ip,
|
||||||
|
path: info && info[:path]
|
||||||
)
|
)
|
||||||
|
|
||||||
true
|
true
|
||||||
|
@ -0,0 +1,5 @@
|
|||||||
|
class AddPathToUserAuthTokenLog < ActiveRecord::Migration
|
||||||
|
def change
|
||||||
|
add_column :user_auth_token_logs, :path, :string
|
||||||
|
end
|
||||||
|
end
|
@ -12,11 +12,6 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
TOKEN_COOKIE ||= "_t".freeze
|
TOKEN_COOKIE ||= "_t".freeze
|
||||||
PATH_INFO ||= "PATH_INFO".freeze
|
PATH_INFO ||= "PATH_INFO".freeze
|
||||||
COOKIE_ATTEMPTS_PER_MIN ||= 10
|
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
|
# do all current user initialization here
|
||||||
def initialize(env)
|
def initialize(env)
|
||||||
@ -55,6 +50,7 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
@user_token = UserAuthToken.lookup(auth_token,
|
@user_token = UserAuthToken.lookup(auth_token,
|
||||||
seen: true,
|
seen: true,
|
||||||
user_agent: @env['HTTP_USER_AGENT'],
|
user_agent: @env['HTTP_USER_AGENT'],
|
||||||
|
path: @env['REQUEST_PATH'],
|
||||||
client_ip: @request.ip)
|
client_ip: @request.ip)
|
||||||
|
|
||||||
current_user = @user_token.try(:user)
|
current_user = @user_token.try(:user)
|
||||||
@ -131,7 +127,8 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
|
|
||||||
if !@user_token.legacy && needs_rotation
|
if !@user_token.legacy && needs_rotation
|
||||||
if @user_token.rotate!(user_agent: @env['HTTP_USER_AGENT'],
|
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)
|
cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token)
|
||||||
end
|
end
|
||||||
elsif @user_token.legacy
|
elsif @user_token.legacy
|
||||||
@ -141,18 +138,14 @@ class Auth::DefaultCurrentUserProvider
|
|||||||
end
|
end
|
||||||
|
|
||||||
if !user && cookies.key?(TOKEN_COOKIE)
|
if !user && cookies.key?(TOKEN_COOKIE)
|
||||||
cookie_miss_key = COOKIE_MISS_KEY + cookies[TOKEN_COOKIE]
|
cookies.delete(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
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def log_on_user(user, session, cookies)
|
def log_on_user(user, session, cookies)
|
||||||
@user_token = UserAuthToken.generate!(user_id: user.id,
|
@user_token = UserAuthToken.generate!(user_id: user.id,
|
||||||
user_agent: @env['HTTP_USER_AGENT'],
|
user_agent: @env['HTTP_USER_AGENT'],
|
||||||
|
path: @env['REQUEST_PATH'],
|
||||||
client_ip: @request.ip)
|
client_ip: @request.ip)
|
||||||
|
|
||||||
cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token)
|
cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token)
|
||||||
|
@ -208,15 +208,7 @@ describe Auth::DefaultCurrentUserProvider do
|
|||||||
end
|
end
|
||||||
|
|
||||||
it "correctly removes invalid cookies" do
|
it "correctly removes invalid cookies" do
|
||||||
|
|
||||||
cookies = {"_t" => SecureRandom.hex}
|
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)
|
provider('/').refresh_session(nil, {}, cookies)
|
||||||
expect(cookies.key?("_t")).to eq(false)
|
expect(cookies.key?("_t")).to eq(false)
|
||||||
end
|
end
|
||||||
|
@ -227,13 +227,19 @@ describe UserAuthToken do
|
|||||||
).count).to eq(1)
|
).count).to eq(1)
|
||||||
|
|
||||||
fake_token = SecureRandom.hex
|
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(
|
expect(UserAuthTokenLog.where(
|
||||||
action: "miss token",
|
action: "miss token",
|
||||||
auth_token: UserAuthToken.hash_token(fake_token),
|
auth_token: UserAuthToken.hash_token(fake_token),
|
||||||
user_agent: "bob",
|
user_agent: "bob",
|
||||||
client_ip: "127.0.0.1"
|
client_ip: "127.0.0.1",
|
||||||
|
path: "/path"
|
||||||
).count).to eq(1)
|
).count).to eq(1)
|
||||||
|
|
||||||
|
|
||||||
|
Reference in New Issue
Block a user