BUGFIX: improve error messages for invalid API keys

BUGFIX: don't track last seen for message bus

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

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
+

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

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

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

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