From e9afe28586cd887b92fa86c52db78d543a70e433 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Thu, 22 May 2014 14:55:36 -0400 Subject: [PATCH] Revert "BUGFIX: improve error messages for invalid API keys" --- lib/auth/default_current_user_provider.rb | 59 ++++++++----------- .../default_current_user_provider_spec.rb | 50 ---------------- spec/components/current_user_spec.rb | 7 ++- .../application_controller_spec.rb | 10 ++-- spec/controllers/topics_controller_spec.rb | 6 +- .../user_badges_controller_spec.rb | 2 +- 6 files changed, 40 insertions(+), 94 deletions(-) delete mode 100644 spec/components/auth/default_current_user_provider_spec.rb diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index cbac2b5ed15..cce96d54850 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -2,11 +2,9 @@ require_dependency "auth/current_user_provider" class Auth::DefaultCurrentUserProvider - 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 + CURRENT_USER_KEY ||= "_DISCOURSE_CURRENT_USER" + API_KEY ||= "_DISCOURSE_API" + TOKEN_COOKIE ||= "_t" # do all current user initialization here def initialize(env) @@ -14,11 +12,12 @@ 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 = @request + request = Rack::Request.new(@env) auth_token = request.cookies[TOKEN_COOKIE] @@ -32,19 +31,33 @@ class Auth::DefaultCurrentUserProvider current_user = nil end - if current_user && should_update_last_seen? + if current_user + 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 - 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 + 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 end @env[CURRENT_USER_KEY] = current_user @@ -82,28 +95,8 @@ class Auth::DefaultCurrentUserProvider end def has_auth_cookie? - cookie = @request.cookies[TOKEN_COOKIE] + request = Rack::Request.new(@env) + 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 deleted file mode 100644 index ccad63bbedc..00000000000 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ /dev/null @@ -1,50 +0,0 @@ -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 4664c3a761d..83cd457de9e 100644 --- a/spec/components/current_user_spec.rb +++ b/spec/components/current_user_spec.rb @@ -5,9 +5,10 @@ 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) - - env = Rack::MockRequest.env_for("/test", "HTTP_COOKIE" => "_t=#{user.auth_token};") - CurrentUser.lookup_from_env(env).should == user + CurrentUser.lookup_from_env("HTTP_COOKIE" => "_t=#{user.auth_token};").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 1cea9d067a9..40597637b06 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -163,14 +163,16 @@ describe 'api' do it 'disallows phonies to bookmark posts' do PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never - 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 + 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 end it 'disallows blank api' do PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never - put :bookmark, bookmarked: "true", post_id: post.id, api_key: "", api_username: user.username, format: :json - response.code.to_i.should == 403 + lambda do + put :bookmark, bookmarked: "true", post_id: post.id, api_key: "", api_username: user.username, format: :json + end.should raise_error Discourse::NotLoggedIn end end end diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 3f2c72d520d..235b255dafa 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.login_required = true } + before { SiteSetting.stubs(:login_required?).returns(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 'returns 403 for an invalid key' do + it 'redirects to the login page if invalid key is provided' do get :show, topic_id: topic.id, slug: topic.slug, api_key: "bad" - expect(response.code.to_i).to be(403) + expect(response).to redirect_to login_path end end end diff --git a/spec/controllers/user_badges_controller_spec.rb b/spec/controllers/user_badges_controller_spec.rb index 5320760fee2..70a2fbd0001 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, api_username: "system" + xhr :post, :create, badge_id: badge.id, username: user.username, api_key: api_key.key response.status.should == 200 user_badge = UserBadge.find_by(user: user, badge: badge) user_badge.should be_present