From ff49f72ad939b60bc912aec581e76916d4de75e6 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 31 Jan 2017 17:21:37 -0500 Subject: [PATCH] FEATURE: per client user tokens Revamped system for managing authentication tokens. - Every user has 1 token per client (web browser) - Tokens are rotated every 10 minutes New system migrates the old tokens to "legacy" tokens, so users still remain logged on. Also introduces weekly job to expire old auth tokens. --- app/controllers/admin/users_controller.rb | 3 +- app/controllers/users_controller.rb | 4 +- app/jobs/scheduled/weekly.rb | 1 + app/models/global_setting.rb | 29 +++++ app/models/user.rb | 19 ++- app/models/user_auth_token.rb | 115 ++++++++++++++++ config/discourse_defaults.conf | 3 + config/initializers/100-secret_token.rb | 18 +-- .../20170124181409_add_user_auth_tokens.rb | 43 ++++++ lib/auth/default_current_user_provider.rb | 53 +++++--- lib/wizard.rb | 6 +- .../default_current_user_provider_spec.rb | 117 +++++++++++++---- spec/components/current_user_spec.rb | 6 +- spec/components/wizard_spec.rb | 3 +- spec/controllers/session_controller_spec.rb | 4 +- spec/controllers/users_controller_spec.rb | 39 +++--- spec/models/user_auth_token_spec.rb | 123 ++++++++++++++++++ spec/models/user_spec.rb | 9 +- spec/services/user_anonymizer_spec.rb | 6 +- 19 files changed, 495 insertions(+), 106 deletions(-) create mode 100644 app/models/user_auth_token.rb create mode 100644 db/migrate/20170124181409_add_user_auth_tokens.rb create mode 100644 spec/models/user_auth_token_spec.rb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 2359bc31e2a..9f72667af07 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -72,8 +72,7 @@ class Admin::UsersController < Admin::AdminController def log_out if @user - @user.auth_token = nil - @user.save! + @user.user_auth_tokens.destroy_all @user.logged_out render json: success_json else diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 4c06f112777..734a3e4dc2d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -417,7 +417,7 @@ class UsersController < ApplicationController else @user.password = params[:password] @user.password_required! - @user.auth_token = nil + @user.user_auth_tokens.destroy_all if @user.save Invite.invalidate_for_email(@user.email) # invite link can't be used to log in anymore secure_session["password-#{token}"] = nil @@ -701,7 +701,7 @@ class UsersController < ApplicationController private def honeypot_value - Digest::SHA1::hexdigest("#{Discourse.current_hostname}:#{Discourse::Application.config.secret_token}")[0,15] + Digest::SHA1::hexdigest("#{Discourse.current_hostname}:#{GlobalSetting.safe_secret_key_base}")[0,15] end def challenge_value diff --git a/app/jobs/scheduled/weekly.rb b/app/jobs/scheduled/weekly.rb index ffce8c1d246..1a4df35cde0 100644 --- a/app/jobs/scheduled/weekly.rb +++ b/app/jobs/scheduled/weekly.rb @@ -13,6 +13,7 @@ module Jobs ScoreCalculator.new.calculate SchedulerStat.purge_old Draft.cleanup! + UserAuthToken.cleanup! end end end diff --git a/app/models/global_setting.rb b/app/models/global_setting.rb index 36fc07ab45b..85abcce23b7 100644 --- a/app/models/global_setting.rb +++ b/app/models/global_setting.rb @@ -6,6 +6,35 @@ class GlobalSetting end end + VALID_SECRET_KEY = /^[0-9a-f]{128}$/ + # this is named SECRET_TOKEN as opposed to SECRET_KEY_BASE + # for legacy reasons + REDIS_SECRET_KEY = 'SECRET_TOKEN' + + # In Rails secret_key_base is used to encrypt the cookie store + # the cookie store contains session data + # Discourse also uses this secret key to digest user auth tokens + # This method will + # - use existing token if already set in ENV or discourse.conf + # - generate a token on the fly if needed and cache in redis + # - enforce rules about token format falling back to redis if needed + def self.safe_secret_key_base + @safe_secret_key_base ||= begin + token = secret_key_base + if token.blank? || token !~ VALID_SECRET_KEY + token = $redis.without_namespace.get(REDIS_SECRET_KEY) + unless token && token =~ VALID_SECRET_KEY + token = SecureRandom.hex(64) + $redis.without_namespace.set(REDIS_SECRET_KEY,token) + end + end + if !secret_key_base.blank? && token != secret_key_base + STDERR.puts "WARNING: DISCOURSE_SECRET_KEY_BASE is invalid, it was re-generated" + end + token + end + end + def self.load_defaults default_provider = FileProvider.from(File.expand_path('../../../config/discourse_defaults.conf', __FILE__)) default_provider.keys.concat(@provider.keys).uniq.each do |key| diff --git a/app/models/user.rb b/app/models/user.rb index 9e8df36e763..d45f0d27766 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -41,6 +41,7 @@ class User < ActiveRecord::Base has_many :user_archived_messages, dependent: :destroy has_many :email_change_requests, dependent: :destroy has_many :directory_items, dependent: :delete_all + has_many :user_auth_tokens, dependent: :destroy has_one :user_option, dependent: :destroy @@ -97,6 +98,7 @@ class User < ActiveRecord::Base before_save :update_username_lower before_save :ensure_password_is_hashed + after_save :expire_tokens_if_password_changed after_save :automatic_group_membership after_save :clear_global_notice_if_needed after_save :refresh_avatar @@ -420,7 +422,6 @@ class User < ActiveRecord::Base # special case for passwordless accounts unless password.blank? @raw_password = password - self.auth_token = nil end end @@ -971,6 +972,18 @@ class User < ActiveRecord::Base end end + def expire_tokens_if_password_changed + # NOTE: setting raw password is the only valid way of changing a password + # the password field in the DB is actually hashed, nobody should be amending direct + if @raw_password + # Association in model may be out-of-sync + UserAuthToken.where(user_id: id).destroy_all + # We should not carry this around after save + @raw_password = nil + end + end + + def hash_password(password, salt) raise StandardError.new("password is too long") if password.size > User.max_password_length Pbkdf2.hash_password(password, salt, Rails.configuration.pbkdf2_iterations, Rails.configuration.pbkdf2_algorithm) @@ -1076,7 +1089,7 @@ end # username :string(60) not null # created_at :datetime not null # updated_at :datetime not null -# name :string +# name :string(255) # seen_notification_id :integer default(0), not null # last_posted_at :datetime # email :string(513) not null @@ -1101,7 +1114,7 @@ end # ip_address :inet # moderator :boolean default(FALSE) # blocked :boolean default(FALSE) -# title :string +# title :string(255) # uploaded_avatar_id :integer # locale :string(10) # primary_group_id :integer diff --git a/app/models/user_auth_token.rb b/app/models/user_auth_token.rb new file mode 100644 index 00000000000..b6a9b290a59 --- /dev/null +++ b/app/models/user_auth_token.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true +require 'digest/sha1' + +class UserAuthToken < ActiveRecord::Base + belongs_to :user + + ROTATE_TIME = 10.minutes + # used when token did not arrive at client + URGENT_ROTATE_TIME = 1.minute + + attr_accessor :unhashed_auth_token + + def self.generate!(info) + token = SecureRandom.hex(16) + hashed_token = hash_token(token) + user_auth_token = UserAuthToken.create!( + user_id: info[:user_id], + user_agent: info[:user_agent], + client_ip: info[:client_ip], + auth_token: hashed_token, + prev_auth_token: hashed_token, + rotated_at: Time.zone.now + ) + user_auth_token.unhashed_auth_token = token + user_auth_token + end + + def self.lookup(unhashed_token, opts=nil) + + mark_seen = opts && opts[:seen] + + token = hash_token(unhashed_token) + expire_before = SiteSetting.maximum_session_age.hours.ago + + user_token = find_by("(auth_token = :token OR + prev_auth_token = :token OR + (auth_token = :unhashed_token AND legacy)) AND created_at > :expire_before", + token: token, unhashed_token: unhashed_token, expire_before: expire_before) + + if user_token && + user_token.auth_token_seen && + user_token.prev_auth_token == token && + user_token.prev_auth_token != user_token.auth_token && + user_token.rotated_at > 1.minute.ago + return nil + end + + if mark_seen && user_token && !user_token.auth_token_seen && user_token.auth_token == token + user_token.update_columns(auth_token_seen: true) + end + + user_token + end + + def self.hash_token(token) + Digest::SHA1.base64digest("#{token}#{GlobalSetting.safe_secret_key_base}") + end + + def self.cleanup! + where('rotated_at < :time', + time: SiteSetting.maximum_session_age.hours.ago - ROTATE_TIME).delete_all + + end + + def rotate!(info=nil) + user_agent = (info && info[:user_agent] || self.user_agent) + client_ip = (info && info[:client_ip] || self.client_ip) + + token = SecureRandom.hex(16) + + result = UserAuthToken.exec_sql(" + UPDATE user_auth_tokens + SET + auth_token_seen = false, + user_agent = :user_agent, + client_ip = :client_ip, + prev_auth_token = case when auth_token_seen then auth_token else prev_auth_token end, + auth_token = :new_token, + rotated_at = :now + WHERE id = :id AND (auth_token_seen or rotated_at < :safeguard_time) +", id: self.id, + user_agent: user_agent, + client_ip: client_ip&.to_s, + now: Time.zone.now, + new_token: UserAuthToken.hash_token(token), + safeguard_time: 30.seconds.ago + ) + + if result.cmdtuples > 0 + reload + self.unhashed_auth_token = token + true + else + false + end + + end +end + +# == Schema Information +# +# Table name: user_auth_tokens +# +# id :integer not null, primary key +# user_id :integer not null +# auth_token :string not null +# prev_auth_token :string +# user_agent :string +# auth_token_seen :boolean default(FALSE), not null +# legacy :boolean default(FALSE), not null +# client_ip :inet +# rotated_at :datetime +# created_at :datetime +# updated_at :datetime +# diff --git a/config/discourse_defaults.conf b/config/discourse_defaults.conf index ee87099158d..74284c88de3 100644 --- a/config/discourse_defaults.conf +++ b/config/discourse_defaults.conf @@ -147,3 +147,6 @@ relative_url_root = # this ensures backlog (ability of channels to catch up are capped) # message bus default cap is 1000, we are winding it down to 100 message_bus_max_backlog_size = 100 + +# must be a 64 byte hex string, anything else will be ignored with a warning +secret_key_base = diff --git a/config/initializers/100-secret_token.rb b/config/initializers/100-secret_token.rb index 7bbba339744..a9507c57961 100644 --- a/config/initializers/100-secret_token.rb +++ b/config/initializers/100-secret_token.rb @@ -1,14 +1,4 @@ -# We have had lots of config issues with SECRET_TOKEN to avoid this mess we are moving it to redis -# if you feel strongly that it does not belong there use ENV['SECRET_TOKEN'] -# -token = ENV['SECRET_TOKEN'] -unless token - token = $redis.get('SECRET_TOKEN') - unless token && token.length == 128 - token = SecureRandom.hex(64) - $redis.set('SECRET_TOKEN',token) - end -end - -Discourse::Application.config.secret_token = token -Discourse::Application.config.secret_key_base = token +# Not fussed setting secret_token anymore, that is only required for +# backwards support of "seamless" upgrade from Rails 3. +# Discourse has shipped Rails 3 for a very long time. +Discourse::Application.config.secret_key_base = GlobalSetting.safe_secret_key_base diff --git a/db/migrate/20170124181409_add_user_auth_tokens.rb b/db/migrate/20170124181409_add_user_auth_tokens.rb new file mode 100644 index 00000000000..d702f39774a --- /dev/null +++ b/db/migrate/20170124181409_add_user_auth_tokens.rb @@ -0,0 +1,43 @@ +class AddUserAuthTokens < ActiveRecord::Migration + def down + add_column :users, :auth_token, :string + add_column :users, :auth_token_updated_at, :datetime + execute < ?', - SiteSetting.maximum_session_age.hours.ago) - .first + @user_token = UserAuthToken.lookup(auth_token, seen: true) + current_user = @user_token.try(:user) end unless current_user @@ -105,35 +103,46 @@ class Auth::DefaultCurrentUserProvider end def refresh_session(user, session, cookies) - return if is_api? - if user && (!user.auth_token_updated_at || user.auth_token_updated_at <= 1.hour.ago) - user.update_column(:auth_token_updated_at, Time.zone.now) - cookies[TOKEN_COOKIE] = cookie_hash(user) + # if user was not loaded, no point refreshing session + # it could be an anonymous path, this would add cost + return if is_api? || !@env.key?(CURRENT_USER_KEY) + + if @user_token && @user_token.user == user + rotated_at = @user_token.rotated_at + + needs_rotation = @user_token.auth_token_seen ? rotated_at < UserAuthToken::ROTATE_TIME.ago : rotated_at < UserAuthToken::URGENT_ROTATE_TIME.ago + + if !@user_token.legacy && needs_rotation + if @user_token.rotate!(user_agent: @env['HTTP_USER_AGENT'], + client_ip: @request.ip) + cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token) + end + elsif @user_token.legacy + # make a new token + log_on_user(user, session, cookies) + end end + if !user && cookies.key?(TOKEN_COOKIE) cookies.delete(TOKEN_COOKIE) end end def log_on_user(user, session, cookies) - legit_token = user.auth_token && user.auth_token.length == 32 - expired_token = user.auth_token_updated_at && user.auth_token_updated_at < SiteSetting.maximum_session_age.hours.ago + @user_token = UserAuthToken.generate!(user_id: user.id, + user_agent: @env['HTTP_USER_AGENT'], + client_ip: @request.ip) - if !legit_token || expired_token - user.update_columns(auth_token: SecureRandom.hex(16), - auth_token_updated_at: Time.zone.now) - end - - cookies[TOKEN_COOKIE] = cookie_hash(user) + cookies[TOKEN_COOKIE] = cookie_hash(@user_token.unhashed_auth_token) make_developer_admin(user) enable_bootstrap_mode(user) @env[CURRENT_USER_KEY] = user end - def cookie_hash(user) + def cookie_hash(unhashed_auth_token) { - value: user.auth_token, + value: unhashed_auth_token, httponly: true, expires: SiteSetting.maximum_session_age.hours.from_now, secure: SiteSetting.force_https @@ -155,9 +164,9 @@ class Auth::DefaultCurrentUserProvider end def log_off_user(session, cookies) - if SiteSetting.log_out_strict && (user = current_user) - user.auth_token = nil - user.save! + user = current_user + if SiteSetting.log_out_strict && user + user.user_auth_tokens.destroy_all if user.admin && defined?(Rack::MiniProfiler) # clear the profiling cookie to keep stuff tidy @@ -165,6 +174,8 @@ class Auth::DefaultCurrentUserProvider end user.logged_out + elsif user && @user_token + @user_token.destroy end cookies.delete(TOKEN_COOKIE) end diff --git a/lib/wizard.rb b/lib/wizard.rb index ab6c5c33c02..cd442219d79 100644 --- a/lib/wizard.rb +++ b/lib/wizard.rb @@ -1,6 +1,7 @@ require_dependency 'wizard/step' require_dependency 'wizard/field' require_dependency 'wizard/step_updater' +require_dependency 'wizard/builder' class Wizard attr_reader :steps, :user @@ -76,11 +77,10 @@ class Wizard def requires_completion? return false unless SiteSetting.wizard_enabled? - first_admin = User.where(admin: true) .where.not(id: Discourse.system_user.id) - .where.not(auth_token_updated_at: nil) - .order(:auth_token_updated_at) + .joins(:user_auth_tokens) + .order('user_auth_tokens.created_at') if @user.present? && first_admin.first == @user && (Topic.count < 15) !Wizard::Builder.new(@user).build.completed? diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index e9a680dd22e..cd191ec6126 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -3,10 +3,17 @@ require_dependency 'auth/default_current_user_provider' describe Auth::DefaultCurrentUserProvider do + class TestProvider < Auth::DefaultCurrentUserProvider + attr_reader :env + def initialize(env) + super(env) + end + end + def provider(url, opts=nil) opts ||= {method: "GET"} env = Rack::MockRequest.env_for(url, opts) - Auth::DefaultCurrentUserProvider.new(env) + TestProvider.new(env) end it "raises errors for incorrect api_key" do @@ -73,21 +80,83 @@ describe Auth::DefaultCurrentUserProvider do expect(provider("/topic/anything/goes", method: "GET").should_update_last_seen?).to eq(true) end - it "correctly renews session once an hour" do + it "correctly supports legacy tokens" do + user = Fabricate(:user) + token = SecureRandom.hex(16) + user_token = UserAuthToken.create!(user_id: user.id, auth_token: token, + prev_auth_token: token, legacy: true, + rotated_at: Time.zone.now + ) + + prov = provider("/", "HTTP_COOKIE" => "_t=#{user_token.auth_token}") + expect(prov.current_user.id).to eq(user.id) + + # sets a new token up cause it got a global token + cookies = {} + prov.refresh_session(user, {}, cookies) + user.reload + + expect(user.user_auth_tokens.count).to eq(2) + expect(cookies["_t"][:value]).not_to eq(token) + end + + it "correctly rotates tokens" do SiteSetting.maximum_session_age = 3 user = Fabricate(:user) - provider('/').log_on_user(user, {}, {}) - - freeze_time 2.hours.from_now + @provider = provider('/') cookies = {} - provider("/", "HTTP_COOKIE" => "_t=#{user.auth_token}").refresh_session(user, {}, cookies) + @provider.log_on_user(user, {}, cookies) + + unhashed_token = cookies["_t"][:value] + + token = UserAuthToken.find_by(user_id: user.id) + + expect(token.auth_token_seen).to eq(false) + expect(token.auth_token).not_to eq(unhashed_token) + expect(token.auth_token).to eq(UserAuthToken.hash_token(unhashed_token)) + + # at this point we are going to try to rotate token + freeze_time 20.minutes.from_now + + provider2 = provider("/", "HTTP_COOKIE" => "_t=#{unhashed_token}") + provider2.current_user + + token.reload + expect(token.auth_token_seen).to eq(true) + + cookies = {} + provider2.refresh_session(user, {}, cookies) + expect(cookies["_t"][:value]).not_to eq(unhashed_token) + + token.reload + expect(token.auth_token_seen).to eq(false) + + + freeze_time 21.minutes.from_now + + + old_token = token.prev_auth_token + unverified_token = token.auth_token + + # old token should still work + provider2 = provider("/", "HTTP_COOKIE" => "_t=#{unhashed_token}") + expect(provider2.current_user.id).to eq(user.id) + + provider2.refresh_session(user, {}, cookies) + + token.reload + + # because this should cause a rotation since we can safely + # assume it never reached the client + expect(token.prev_auth_token).to eq(old_token) + expect(token.auth_token).not_to eq(unverified_token) - expect(user.auth_token_updated_at - Time.now).to eq(0) end it "can only try 10 bad cookies a minute" do - user = Fabricate(:user) + token = UserAuthToken.generate!(user_id: user.id) + provider('/').log_on_user(user, {}, {}) RateLimiter.stubs(:disabled?).returns(false) @@ -107,12 +176,15 @@ describe Auth::DefaultCurrentUserProvider do }.to raise_error(Discourse::InvalidAccess) expect { - env["HTTP_COOKIE"] = "_t=#{user.auth_token}" + env["HTTP_COOKIE"] = "_t=#{token.unhashed_auth_token}" provider("/", env).current_user }.to raise_error(Discourse::InvalidAccess) env["REMOTE_ADDR"] = "10.0.0.2" - provider('/', env).current_user + + expect { + provider('/', env).current_user + }.not_to raise_error end it "correctly removes invalid cookies" do @@ -123,35 +195,26 @@ describe Auth::DefaultCurrentUserProvider do expect(cookies.key?("_t")).to eq(false) end - it "recycles existing auth_token correctly" do - SiteSetting.maximum_session_age = 3 + it "logging on user always creates a new token" do user = Fabricate(:user) - provider('/').log_on_user(user, {}, {}) - - original_auth_token = user.auth_token - - freeze_time 2.hours.from_now - provider('/').log_on_user(user, {}, {}) - - user.reload - expect(user.auth_token).to eq(original_auth_token) - - freeze_time 10.hours.from_now provider('/').log_on_user(user, {}, {}) - user.reload - expect(user.auth_token).not_to eq(original_auth_token) + provider('/').log_on_user(user, {}, {}) + + expect(UserAuthToken.where(user_id: user.id).count).to eq(2) end it "correctly expires session" do SiteSetting.maximum_session_age = 2 user = Fabricate(:user) + token = UserAuthToken.generate!(user_id: user.id) + provider('/').log_on_user(user, {}, {}) - expect(provider("/", "HTTP_COOKIE" => "_t=#{user.auth_token}").current_user.id).to eq(user.id) + expect(provider("/", "HTTP_COOKIE" => "_t=#{token.unhashed_auth_token}").current_user.id).to eq(user.id) freeze_time 3.hours.from_now - expect(provider("/", "HTTP_COOKIE" => "_t=#{user.auth_token}").current_user).to eq(nil) + expect(provider("/", "HTTP_COOKIE" => "_t=#{token.unhashed_auth_token}").current_user).to eq(nil) end context "user api" do diff --git a/spec/components/current_user_spec.rb b/spec/components/current_user_spec.rb index 2cb1061e088..e7631e59963 100644 --- a/spec/components/current_user_spec.rb +++ b/spec/components/current_user_spec.rb @@ -3,10 +3,10 @@ require_dependency 'current_user' describe CurrentUser do it "allows us to lookup a user from our environment" do - user = Fabricate(:user, auth_token: EmailToken.generate_token, active: true) - EmailToken.confirm(user.auth_token) + user = Fabricate(:user, active: true) + token = UserAuthToken.generate!(user_id: user.id) - env = Rack::MockRequest.env_for("/test", "HTTP_COOKIE" => "_t=#{user.auth_token};") + env = Rack::MockRequest.env_for("/test", "HTTP_COOKIE" => "_t=#{token.unhashed_auth_token};") expect(CurrentUser.lookup_from_env(env)).to eq(user) end diff --git a/spec/components/wizard_spec.rb b/spec/components/wizard_spec.rb index 951b72e8845..280bc64df6d 100644 --- a/spec/components/wizard_spec.rb +++ b/spec/components/wizard_spec.rb @@ -122,7 +122,8 @@ describe Wizard do it "it's true for the first admin who logs in" do admin = Fabricate(:admin) - second_admin = Fabricate(:admin, auth_token_updated_at: Time.now) + second_admin = Fabricate(:admin) + UserAuthToken.generate!(user_id: second_admin.id) expect(build_simple(admin).requires_completion?).to eq(false) expect(build_simple(second_admin).requires_completion?).to eq(true) diff --git a/spec/controllers/session_controller_spec.rb b/spec/controllers/session_controller_spec.rb index 50858f7acdb..2f0cbee330e 100644 --- a/spec/controllers/session_controller_spec.rb +++ b/spec/controllers/session_controller_spec.rb @@ -505,8 +505,8 @@ describe SessionController do user.reload expect(session[:current_user_id]).to eq(user.id) - expect(user.auth_token).to be_present - expect(cookies[:_t]).to eq(user.auth_token) + expect(user.user_auth_tokens.count).to eq(1) + expect(UserAuthToken.hash_token(cookies[:_t])).to eq(user.user_auth_tokens.first.auth_token) end end diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 354f9d55f79..496a502165b 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -155,21 +155,13 @@ describe UsersController do put :perform_account_activation, token: 'asdfasdf' end - it 'returns success' do + it 'correctly logs on user' do expect(response).to be_success - end - - it "doesn't set an error" do expect(flash[:error]).to be_blank - end - - it 'logs in as the user' do expect(session[:current_user_id]).to be_present - end - - it "doesn't set @needs_approval" do expect(assigns[:needs_approval]).to be_blank end + end context 'user is not approved' do @@ -241,7 +233,7 @@ describe UsersController do render_views it 'renders referrer never on get requests' do - user = Fabricate(:user, auth_token: SecureRandom.hex(16)) + user = Fabricate(:user) token = user.email_tokens.create(email: user.email).token get :password_reset, token: token @@ -250,25 +242,25 @@ describe UsersController do end it 'returns success' do - user = Fabricate(:user, auth_token: SecureRandom.hex(16)) + user = Fabricate(:user) + user_auth_token = UserAuthToken.generate!(user_id: user.id) token = user.email_tokens.create(email: user.email).token - old_token = user.auth_token - get :password_reset, token: token put :password_reset, token: token, password: 'hg9ow8yhg98o' + expect(response).to be_success expect(assigns[:error]).to be_blank user.reload - expect(user.auth_token).to_not eq old_token - expect(user.auth_token.length).to eq 32 + expect(session["password-#{token}"]).to be_blank + + expect(UserAuthToken.where(id: user_auth_token.id).count).to eq(0) end it 'disallows double password reset' do - - user = Fabricate(:user, auth_token: SecureRandom.hex(16)) + user = Fabricate(:user) token = user.email_tokens.create(email: user.email).token get :password_reset, token: token @@ -277,10 +269,13 @@ describe UsersController do user.reload expect(user.confirm_password?('hg9ow8yhg98o')).to eq(true) + + # logged in now + expect(user.user_auth_tokens.count).to eq(1) end it "redirects to the wizard if you're the first admin" do - user = Fabricate(:admin, auth_token: SecureRandom.hex(16), auth_token_updated_at: Time.now) + user = Fabricate(:admin) token = user.email_tokens.create(email: user.email).token get :password_reset, token: token put :password_reset, token: token, password: 'hg9ow8yhg98oadminlonger' @@ -288,13 +283,17 @@ describe UsersController do end it "doesn't invalidate the token when loading the page" do - user = Fabricate(:user, auth_token: SecureRandom.hex(16)) + user = Fabricate(:user) + user_token = UserAuthToken.generate!(user_id: user.id) + email_token = user.email_tokens.create(email: user.email) get :password_reset, token: email_token.token email_token.reload + expect(email_token.confirmed).to eq(false) + expect(UserAuthToken.where(id: user_token.id).count).to eq(1) end end diff --git a/spec/models/user_auth_token_spec.rb b/spec/models/user_auth_token_spec.rb new file mode 100644 index 00000000000..289d61cadfd --- /dev/null +++ b/spec/models/user_auth_token_spec.rb @@ -0,0 +1,123 @@ +require 'rails_helper' + +describe UserAuthToken do + + it "can remove old expired tokens" do + + freeze_time Time.zone.now + SiteSetting.maximum_session_age = 1 + + user = Fabricate(:user) + token = UserAuthToken.generate!(user_id: user.id, + user_agent: "some user agent 2", + client_ip: "1.1.2.3") + + freeze_time 1.hour.from_now + UserAuthToken.cleanup! + + expect(UserAuthToken.where(id: token.id).count).to eq(1) + + freeze_time 1.second.from_now + UserAuthToken.cleanup! + + expect(UserAuthToken.where(id: token.id).count).to eq(1) + + freeze_time UserAuthToken::ROTATE_TIME.from_now + UserAuthToken.cleanup! + + expect(UserAuthToken.where(id: token.id).count).to eq(0) + + end + + it "can lookup both hashed and unhashed" do + user = Fabricate(:user) + + token = UserAuthToken.generate!(user_id: user.id, + user_agent: "some user agent 2", + client_ip: "1.1.2.3") + + lookup_token = UserAuthToken.lookup(token.unhashed_auth_token) + + expect(user.id).to eq(lookup_token.user.id) + + lookup_token = UserAuthToken.lookup(token.auth_token) + + expect(lookup_token).to eq(nil) + + token.update_columns(legacy: true) + + lookup_token = UserAuthToken.lookup(token.auth_token) + + expect(user.id).to eq(lookup_token.user.id) + end + + it "can validate token was seen at lookup time" do + + user = Fabricate(:user) + + user_token = UserAuthToken.generate!(user_id: user.id, + user_agent: "some user agent 2", + client_ip: "1.1.2.3") + + expect(user_token.auth_token_seen).to eq(false) + + UserAuthToken.lookup(user_token.unhashed_auth_token, seen: true) + + user_token.reload + expect(user_token.auth_token_seen).to eq(true) + + end + + it "can rotate with no params maintaining data" do + + user = Fabricate(:user) + + user_token = UserAuthToken.generate!(user_id: user.id, + user_agent: "some user agent 2", + client_ip: "1.1.2.3") + + user_token.update_columns(auth_token_seen: true) + expect(user_token.rotate!).to eq(true) + user_token.reload + expect(user_token.client_ip.to_s).to eq("1.1.2.3") + expect(user_token.user_agent).to eq("some user agent 2") + end + + it "can properly rotate tokens" do + + user = Fabricate(:user) + + user_token = UserAuthToken.generate!(user_id: user.id, + user_agent: "some user agent 2", + client_ip: "1.1.2.3") + + prev_auth_token = user_token.auth_token + unhashed_prev = user_token.unhashed_auth_token + + rotated = user_token.rotate!(user_agent: "a new user agent", client_ip: "1.1.2.4") + expect(rotated).to eq(false) + + user_token.update_columns(auth_token_seen: true) + + rotated = user_token.rotate!(user_agent: "a new user agent", client_ip: "1.1.2.4") + expect(rotated).to eq(true) + + user_token.reload + + expect(user_token.rotated_at).to be_within(5.second).of(Time.zone.now) + expect(user_token.client_ip).to eq("1.1.2.4") + expect(user_token.user_agent).to eq("a new user agent") + expect(user_token.auth_token_seen).to eq(false) + expect(user_token.prev_auth_token).to eq(prev_auth_token) + + # ability to auth using an old token + looked_up = UserAuthToken.lookup(unhashed_prev) + expect(looked_up.id).to eq(user_token.id) + + freeze_time(2.minute.from_now) do + looked_up = UserAuthToken.lookup(unhashed_prev) + expect(looked_up).to eq(nil) + end + end + +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 15498c992e5..c52861d2b85 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -592,21 +592,18 @@ describe User do @user.password = "ilovepasta" @user.save! - @user.auth_token = SecureRandom.hex(16) - @user.save! - expect(@user.active).to eq(false) expect(@user.confirm_password?("ilovepasta")).to eq(true) - email_token = @user.email_tokens.create(email: 'pasta@delicious.com') - old_token = @user.auth_token + UserAuthToken.generate!(user_id: @user.id) + @user.password = "passwordT" @user.save! # must expire old token on password change - expect(@user.auth_token).to_not eq(old_token) + expect(@user.user_auth_tokens.count).to eq(0) email_token.reload expect(email_token.expired).to eq(true) diff --git a/spec/services/user_anonymizer_spec.rb b/spec/services/user_anonymizer_spec.rb index 6e7a0227a50..5d463e2fccb 100644 --- a/spec/services/user_anonymizer_spec.rb +++ b/spec/services/user_anonymizer_spec.rb @@ -4,7 +4,7 @@ describe UserAnonymizer do describe "make_anonymous" do let(:admin) { Fabricate(:admin) } - let(:user) { Fabricate(:user, username: "edward", auth_token: "mysecretauthtoken") } + let(:user) { Fabricate(:user, username: "edward") } subject(:make_anonymous) { described_class.make_anonymous(user, admin) } @@ -51,6 +51,8 @@ describe UserAnonymizer do prev_username = user.username + UserAuthToken.generate!(user_id: user.id) + make_anonymous user.reload @@ -58,7 +60,7 @@ describe UserAnonymizer do expect(user.name).not_to be_present expect(user.date_of_birth).to eq(nil) expect(user.title).not_to be_present - expect(user.auth_token).to eq(nil) + expect(user.user_auth_tokens.count).to eq(0) profile = user.user_profile(true) expect(profile.location).to eq(nil)