diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index cce96d54850..cbac2b5ed15 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -2,9 +2,11 @@ require_dependency "auth/current_user_provider" class Auth::DefaultCurrentUserProvider - CURRENT_USER_KEY ||= "_DISCOURSE_CURRENT_USER" - API_KEY ||= "_DISCOURSE_API" - TOKEN_COOKIE ||= "_t" + CURRENT_USER_KEY ||= "_DISCOURSE_CURRENT_USER".freeze + API_KEY ||= "api_key".freeze + API_KEY_ENV ||= "_DISCOURSE_API".freeze + TOKEN_COOKIE ||= "_t".freeze + PATH_INFO ||= "PATH_INFO".freeze # do all current user initialization here def initialize(env) @@ -12,12 +14,11 @@ class Auth::DefaultCurrentUserProvider @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) + request = @request auth_token = request.cookies[TOKEN_COOKIE] @@ -31,33 +32,19 @@ class Auth::DefaultCurrentUserProvider current_user = nil end - if current_user - + if current_user && should_update_last_seen? u = current_user Scheduler::Defer.later do u.update_last_seen! u.update_ip_address!(request.ip) end - end # possible we have an api call, impersonate - unless current_user - if api_key_value = request["api_key"] - api_key = ApiKey.where(key: api_key_value).includes(:user).first - if api_key.present? - @env[API_KEY] = true - api_username = request["api_username"] - - if api_key.user.present? - raise Discourse::InvalidAccess.new if api_username && (api_key.user.username_lower != api_username.downcase) - current_user = api_key.user - elsif api_username - current_user = User.find_by(username_lower: api_username.downcase) - end - - end - end + if api_key = request[API_KEY] + current_user = lookup_api_user(api_key, request) + raise Discourse::InvalidAccess unless current_user + @env[API_KEY_ENV] = true end @env[CURRENT_USER_KEY] = current_user @@ -95,8 +82,28 @@ class Auth::DefaultCurrentUserProvider end def has_auth_cookie? - request = Rack::Request.new(@env) - cookie = request.cookies[TOKEN_COOKIE] + cookie = @request.cookies[TOKEN_COOKIE] !cookie.nil? && cookie.length == 32 end + + def should_update_last_seen? + !(@request.path =~ /^\/message-bus\//) + end + + protected + + def lookup_api_user(api_key_value, request) + api_key = ApiKey.where(key: api_key_value).includes(:user).first + if api_key + api_username = request["api_username"] + if api_key.user + api_key.user if !api_username || (api_key.user.username_lower == api_username.downcase) + elsif api_username + User.find_by(username_lower: api_username.downcase) + else + nil + end + end + end + end diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb new file mode 100644 index 00000000000..ccad63bbedc --- /dev/null +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -0,0 +1,50 @@ +require 'spec_helper' +require_dependency 'auth/default_current_user_provider' + +describe Auth::DefaultCurrentUserProvider do + + def provider(url, opts=nil) + opts ||= {method: "GET"} + env = Rack::MockRequest.env_for(url, opts) + Auth::DefaultCurrentUserProvider.new(env) + end + + it "raises errors for incorrect api_key" do + expect{ + provider("/?api_key=INCORRECT").current_user + }.to raise_error(Discourse::InvalidAccess) + end + + it "finds a user for a correct per-user api key" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) + provider("/?api_key=hello").current_user.id.should == user.id + end + + it "raises for a user pretending" do + user = Fabricate(:user) + user2 = Fabricate(:user) + ApiKey.create!(key: "hello", user_id: user.id, created_by_id: -1) + + expect{ + provider("/?api_key=hello&api_username=#{user2.username.downcase}").current_user + }.to raise_error(Discourse::InvalidAccess) + end + + it "finds a user for a correct system api key" do + user = Fabricate(:user) + ApiKey.create!(key: "hello", created_by_id: -1) + provider("/?api_key=hello&api_username=#{user.username.downcase}").current_user.id.should == user.id + end + + it "should not update last seen for message bus" do + provider("/message-bus/anything/goes", method: "POST").should_update_last_seen?.should == false + provider("/message-bus/anything/goes", method: "GET").should_update_last_seen?.should == false + end + + it "should update last seen for others" do + provider("/topic/anything/goes", method: "POST").should_update_last_seen?.should == true + provider("/topic/anything/goes", method: "GET").should_update_last_seen?.should == true + end +end + diff --git a/spec/components/current_user_spec.rb b/spec/components/current_user_spec.rb index 83cd457de9e..4664c3a761d 100644 --- a/spec/components/current_user_spec.rb +++ b/spec/components/current_user_spec.rb @@ -5,10 +5,9 @@ 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) - CurrentUser.lookup_from_env("HTTP_COOKIE" => "_t=#{user.auth_token};").should == user + + env = Rack::MockRequest.env_for("/test", "HTTP_COOKIE" => "_t=#{user.auth_token};") + CurrentUser.lookup_from_env(env).should == user end - # it "allows us to lookup a user from our app" do - # end - end diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 40597637b06..1cea9d067a9 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -163,16 +163,14 @@ describe 'api' do it 'disallows phonies to bookmark posts' do PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never - lambda do - put :bookmark, bookmarked: "true", post_id: post.id, api_key: SecureRandom.hex(32), api_username: user.username, format: :json - end.should raise_error Discourse::NotLoggedIn + put :bookmark, bookmarked: "true", post_id: post.id, api_key: SecureRandom.hex(32), api_username: user.username, format: :json + response.code.to_i.should == 403 end it 'disallows blank api' do PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never - lambda do - put :bookmark, bookmarked: "true", post_id: post.id, api_key: "", api_username: user.username, format: :json - end.should raise_error Discourse::NotLoggedIn + put :bookmark, bookmarked: "true", post_id: post.id, api_key: "", api_username: user.username, format: :json + response.code.to_i.should == 403 end end end diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 235b255dafa..3f2c72d520d 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -638,7 +638,7 @@ describe TopicsController do end context "when 'login required' site setting has been enabled" do - before { SiteSetting.stubs(:login_required?).returns(true) } + before { SiteSetting.login_required = true } context 'and the user is logged in' do before { log_in(:coding_horror) } @@ -662,9 +662,9 @@ describe TopicsController do expect(response).to be_successful end - it 'redirects to the login page if invalid key is provided' do + it 'returns 403 for an invalid key' do get :show, topic_id: topic.id, slug: topic.slug, api_key: "bad" - expect(response).to redirect_to login_path + expect(response.code.to_i).to be(403) end end end diff --git a/spec/controllers/user_badges_controller_spec.rb b/spec/controllers/user_badges_controller_spec.rb index 70a2fbd0001..5320760fee2 100644 --- a/spec/controllers/user_badges_controller_spec.rb +++ b/spec/controllers/user_badges_controller_spec.rb @@ -59,7 +59,7 @@ describe UserBadgesController do it 'grants badges from master api calls' do api_key = Fabricate(:api_key) StaffActionLogger.any_instance.expects(:log_badge_grant).never - xhr :post, :create, badge_id: badge.id, username: user.username, api_key: api_key.key + xhr :post, :create, badge_id: badge.id, username: user.username, api_key: api_key.key, api_username: "system" response.status.should == 200 user_badge = UserBadge.find_by(user: user, badge: badge) user_badge.should be_present