From 7993845bfa76646b7575149b52c3e0f2ef1861f8 Mon Sep 17 00:00:00 2001 From: Sam Date: Wed, 9 Oct 2013 15:10:37 +1100 Subject: [PATCH] add current_user_provider so people can override current_user bevior cleanly, see http://meta.discourse.org/t/amending-current-user-logic-in-discourse/10278 --- app/controllers/application_controller.rb | 7 +- app/controllers/session_controller.rb | 2 +- app/controllers/users_controller.rb | 2 +- lib/admin_constraint.rb | 4 +- lib/auth/current_user_provider.rb | 34 ++++++++++ lib/auth/default_current_user_provider.rb | 79 +++++++++++++++++++++++ lib/current_user.rb | 75 ++++----------------- lib/discourse.rb | 9 +++ lib/homepage_constraint.rb | 5 +- lib/rate_limiter.rb | 10 ++- lib/staff_constraint.rb | 4 +- lib/user_activator.rb | 8 ++- spec/components/current_user_spec.rb | 6 +- spec/spec_helper.rb | 14 ++++ spec/support/helpers.rb | 3 +- 15 files changed, 178 insertions(+), 84 deletions(-) create mode 100644 lib/auth/current_user_provider.rb create mode 100644 lib/auth/default_current_user_provider.rb diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index a07ecd226e0..e6f1e7a3052 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,13 +1,12 @@ -require 'current_user' -require 'canonical_url' +require_dependency 'current_user' +require_dependency 'canonical_url' require_dependency 'discourse' require_dependency 'custom_renderer' -require 'archetype' +require_dependency 'archetype' require_dependency 'rate_limiter' class ApplicationController < ActionController::Base include CurrentUser - include CanonicalURL::ControllerExtensions serialization_scope :guardian diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index 74c54cf65ea..908eba5013c 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -68,7 +68,7 @@ class SessionController < ApplicationController def destroy reset_session - cookies[:_t] = nil + log_off_user render nothing: true end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a9befdbad5e..d39697ce0c7 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -139,7 +139,7 @@ class UsersController < ApplicationController register_nickname(user) if user.save - activator = UserActivator.new(user, session, cookies) + activator = UserActivator.new(user, request, session, cookies) message = activator.activation_message create_third_party_auth_records(user, auth) diff --git a/lib/admin_constraint.rb b/lib/admin_constraint.rb index 7ee35dd5943..2d5df1f5bd4 100644 --- a/lib/admin_constraint.rb +++ b/lib/admin_constraint.rb @@ -3,8 +3,8 @@ require_dependency 'current_user' class AdminConstraint def matches?(request) - return false unless request.session[:current_user_id].present? - User.admins.where(id: request.session[:current_user_id].to_i).exists? + provider = Discourse.current_user_provider.new(request.env) + provider.current_user && provider.current_user.admin? end end diff --git a/lib/auth/current_user_provider.rb b/lib/auth/current_user_provider.rb new file mode 100644 index 00000000000..d5bff657530 --- /dev/null +++ b/lib/auth/current_user_provider.rb @@ -0,0 +1,34 @@ +module Auth; end +class Auth::CurrentUserProvider + + # do all current user initialization here + def initialize(env) + raise NotImplementedError + end + + # our current user, return nil if none is found + def current_user + raise NotImplementedError + end + + # log on a user and set cookies and session etc. + def log_on_user(user,session,cookies) + raise NotImplementedError + end + + # api has special rights return true if api was detected + def is_api? + raise NotImplementedError + end + + # we may need to know very early on in the middleware if an auth token + # exists, to optimise caching + def has_auth_cookie? + raise NotImplementedError + end + + + def log_off_user(session, cookies) + raise NotImplementedError + end +end diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb new file mode 100644 index 00000000000..68f09e33854 --- /dev/null +++ b/lib/auth/default_current_user_provider.rb @@ -0,0 +1,79 @@ +require_dependency "auth/current_user_provider" + +class Auth::DefaultCurrentUserProvider + + CURRENT_USER_KEY = "_DISCOURSE_CURRENT_USER" + API_KEY = "_DISCOURSE_API" + + TOKEN_COOKIE = "_t" + + # do all current user initialization here + def initialize(env) + @env = env + @request = Rack::Request.new(env) + end + + # our current user, return nil if none is found + def current_user + return @env[CURRENT_USER_KEY] if @env.key?(CURRENT_USER_KEY) + + request = Rack::Request.new(@env) + + auth_token = request.cookies[TOKEN_COOKIE] + + current_user = nil + + if auth_token && auth_token.length == 32 + current_user = User.where(auth_token: auth_token).first + end + + if current_user && current_user.is_banned? + current_user = nil + end + + if current_user + current_user.update_last_seen! + current_user.update_ip_address!(request.ip) + end + + # possible we have an api call, impersonate + unless current_user + if api_key = request["api_key"] + if api_username = request["api_username"] + if SiteSetting.api_key_valid?(api_key) + @env[API_KEY] = true + current_user = User.where(username_lower: api_username.downcase).first + end + end + end + end + + @env[CURRENT_USER_KEY] = current_user + end + + def log_on_user(user, session, cookies) + unless user.auth_token && user.auth_token.length == 32 + user.auth_token = SecureRandom.hex(16) + user.save! + end + cookies.permanent[TOKEN_COOKIE] = { value: user.auth_token, httponly: true } + @env[CURRENT_USER_KEY] = user + end + + def log_off_user(session, cookies) + cookies[TOKEN_COOKIE] = nil + end + + + # api has special rights return true if api was detected + def is_api? + current_user + @env[API_KEY] + end + + def has_auth_cookie? + request = Rack::Request.new(@env) + cookie = request.cookies[CURRENT_USER_KEY] + !cookie.nil? && cookie.length == 32 + end +end diff --git a/lib/current_user.rb b/lib/current_user.rb index 38538031bce..dc81ca43ca0 100644 --- a/lib/current_user.rb +++ b/lib/current_user.rb @@ -1,90 +1,39 @@ module CurrentUser def self.has_auth_cookie?(env) - request = Rack::Request.new(env) - cookie = request.cookies["_t"] - !cookie.nil? && cookie.length == 32 + Discourse.current_user_provider.new(env).has_auth_cookie? end def self.lookup_from_env(env) - request = Rack::Request.new(env) - lookup_from_auth_token(request.cookies["_t"]) + Discourse.current_user_provider.new(env).current_user end - def self.lookup_from_auth_token(auth_token) - if auth_token && auth_token.length == 32 - User.where(auth_token: auth_token).first - end - end # can be used to pretend current user does no exist, for CSRF attacks def clear_current_user - @current_user = nil - @not_logged_in = true + @current_user_provider = Discourse.current_user_provider.new({}) end def log_on_user(user) - session[:current_user_id] = user.id - unless user.auth_token && user.auth_token.length == 32 - user.auth_token = SecureRandom.hex(16) - user.save! - end - set_permanent_cookie!(user) + current_user_provider.log_on_user(user,session,cookies) end - def set_permanent_cookie!(user) - cookies.permanent["_t"] = { value: user.auth_token, httponly: true } + def log_off_user + current_user_provider.log_off_user(session,cookies) end def is_api? - # ensure current user has been called - # otherwise - current_user - @is_api + current_user_provider.is_api? end def current_user - return @current_user if @current_user || @not_logged_in + current_user_provider.current_user + end - if session[:current_user_id].blank? - # maybe we have a cookie? - @current_user = CurrentUser.lookup_from_auth_token(cookies["_t"]) - session[:current_user_id] = @current_user.id if @current_user - else - @current_user ||= User.where(id: session[:current_user_id]).first + private - # I have flip flopped on this (sam), if our permanent cookie - # conflicts with our current session assume session is bust - # kill it - if @current_user && cookies["_t"] != @current_user.auth_token - @current_user = nil - end - - end - - if @current_user && @current_user.is_banned? - @current_user = nil - end - - @not_logged_in = session[:current_user_id].blank? - if @current_user - @current_user.update_last_seen! - @current_user.update_ip_address!(request.remote_ip) - end - - # possible we have an api call, impersonate - unless @current_user - if api_key = request["api_key"] - if api_username = request["api_username"] - if SiteSetting.api_key_valid?(api_key) - @is_api = true - @current_user = User.where(username_lower: api_username.downcase).first - end - end - end - end - - @current_user + def current_user_provider + @current_user_provider ||= Discourse.current_user_provider.new(request.env) end end diff --git a/lib/discourse.rb b/lib/discourse.rb index acb5bb90d5d..0f71095d6f1 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -1,5 +1,6 @@ require 'cache' require_dependency 'plugin/instance' +require_dependency 'auth/default_current_user_provider' module Discourse @@ -148,6 +149,14 @@ module Discourse end end + def self.current_user_provider + @current_user_provider || Auth::DefaultCurrentUserProvider + end + + def self.current_user_provider=(val) + @current_user_provider = val + end + private def self.maintenance_mode_key diff --git a/lib/homepage_constraint.rb b/lib/homepage_constraint.rb index bbac573ead4..247da1c904c 100644 --- a/lib/homepage_constraint.rb +++ b/lib/homepage_constraint.rb @@ -4,7 +4,8 @@ class HomePageConstraint end def matches?(request) - homepage = request.session[:current_user_id].present? ? SiteSetting.homepage : SiteSetting.anonymous_homepage + provider = Discourse.current_user_provider.new(request.env) + homepage = provider.current_user ? SiteSetting.homepage : SiteSetting.anonymous_homepage homepage == @filter end -end \ No newline at end of file +end diff --git a/lib/rate_limiter.rb b/lib/rate_limiter.rb index f43d6e449d1..4223fadb793 100644 --- a/lib/rate_limiter.rb +++ b/lib/rate_limiter.rb @@ -4,9 +4,17 @@ require_dependency 'rate_limiter/on_create_record' # A redis backed rate limiter. class RateLimiter + def self.disable + @disabled = true + end + + def self.enable + @disabled = false + end + # We don't observe rate limits in test mode def self.disabled? - Rails.env.test? + @disabled || Rails.env.test? end def initialize(user, key, max, secs) diff --git a/lib/staff_constraint.rb b/lib/staff_constraint.rb index 8892e22dd02..19c3acf4220 100644 --- a/lib/staff_constraint.rb +++ b/lib/staff_constraint.rb @@ -3,8 +3,8 @@ require_dependency 'current_user' class StaffConstraint def matches?(request) - return false unless request.session[:current_user_id].present? - User.staff.where(id: request.session[:current_user_id].to_i).exists? + provider = Discourse.current_user_provider.new(request.env) + provider.current_user && provider.current_user.staff? end end diff --git a/lib/user_activator.rb b/lib/user_activator.rb index d6d4b40b0a5..aa124d14eb5 100644 --- a/lib/user_activator.rb +++ b/lib/user_activator.rb @@ -1,10 +1,12 @@ # Responsible for dealing with different activation processes when a user is created class UserActivator - attr_reader :user, :session, :cookies - def initialize(user, session, cookies) + attr_reader :user, :request, :session, :cookies + + def initialize(user, request, session, cookies) @user = user @session = session @cookies = cookies + @request = request end def activation_message @@ -14,7 +16,7 @@ class UserActivator private def activator - factory.new(user, session, cookies) + factory.new(user, request, session, cookies) end def factory diff --git a/spec/components/current_user_spec.rb b/spec/components/current_user_spec.rb index b5b021b7dcb..f3bea462d96 100644 --- a/spec/components/current_user_spec.rb +++ b/spec/components/current_user_spec.rb @@ -3,10 +3,8 @@ require_dependency 'current_user' describe CurrentUser do it "allows us to lookup a user from our environment" do - token = EmailToken.generate_token - user = Fabricate.build(:user) - User.expects(:where).returns([user]) - CurrentUser.lookup_from_env("HTTP_COOKIE" => "_t=#{token};").should == user + user = Fabricate(:user, auth_token: EmailToken.generate_token) + CurrentUser.lookup_from_env("HTTP_COOKIE" => "_t=#{user.auth_token};").should == user end # it "allows us to lookup a user from our app" do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index abe733cdffd..939f399f5ae 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -63,8 +63,22 @@ Spork.prefork do end end + class TestCurrentUserProvider < Auth::DefaultCurrentUserProvider + def log_on_user(user,session,cookies) + session[:current_user_id] = user.id + super + end + + def log_off_user(session,cookies) + session[:current_user_id] = nil + super + end + end + config.before(:all) do DiscoursePluginRegistry.clear + Discourse.current_user_provider = TestCurrentUserProvider + require_dependency 'site_settings/local_process_provider' SiteSetting.provider = SiteSettings::LocalProcessProvider.new end diff --git a/spec/support/helpers.rb b/spec/support/helpers.rb index 4e4c3f6231a..414a719f429 100644 --- a/spec/support/helpers.rb +++ b/spec/support/helpers.rb @@ -10,7 +10,8 @@ module Helpers end def log_in_user(user) - session[:current_user_id] = user.id + provider = Discourse.current_user_provider.new(request.env) + provider.log_on_user(user,session,cookies) end def fixture_file(filename)