Merge pull request #2420 from opf/fix/api_flickering_admin

Fix API flickering admin

lib/api/root.rb

@@ -61,7 +61,7 @@ module API
 
     helpers do
       def current_user
-        return User.current if Rails.env.test?
+        return User.current if running_in_test_env?
         user_id = env['rack.session']['user_id']
         User.current = user_id ? User.find(user_id) : User.anonymous
       end
@@ -76,6 +76,10 @@ module API
         is_authorized
       end
 
+      def running_in_test_env?
+        Rails.env.test? && ENV['CAPYBARA_DISABLE_TEST_AUTH_PROTECTION'] != 'true'
+      end
+
       # checks whether the user has
       # any of the provided permission in any of the provided
       # projects
@@ -113,6 +117,12 @@ module API
 
     # run authentication before each request
     before do
+      # Call current_user as it sets User.current.
+      # Not doing this might cause devs to use User.current without that value
+      # being set to the actually current user. That might result in standard
+      # users becoming admins and otherwise based on who called the ruby
+      # process last.
+      current_user
       authenticate
     end
 

lib/api/v3/users/users_api.rb

@@ -60,7 +60,7 @@ module API
             end
 
             delete do
-              if DeleteUserService.new(@user, User.current).call
+              if DeleteUserService.new(@user, current_user).call
                 status 202
               else
                 fail ::API::Errors::Unauthorized
@@ -71,7 +71,7 @@ module API
 
               # Authenticate lock transitions
               before do
-                if !User.current.admin?
+                unless current_user.admin?
                   fail ::API::Errors::Unauthorized
                 end
               end

spec/features/api/authentication_spec.rb

@@ -0,0 +1,81 @@
+# OpenProject is a project management system.
+# Copyright (C) 2012-2015 the OpenProject Foundation (OPF)
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License version 3.
+#
+# OpenProject is a fork of ChiliProject, which is a fork of Redmine. The copyright follows:
+# Copyright (C) 2006-2013 Jean-Philippe Lang
+# Copyright (C) 2010-2013 the ChiliProject Team
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License
+# as published by the Free Software Foundation; either version 2
+# of the License, or (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+#
+# See doc/COPYRIGHT.rdoc for more details.
+#++
+
+require 'spec_helper'
+
+describe 'Login', type: :feature do
+  after do
+    User.current = nil
+    enable_test_auth_protection
+  end
+
+  let(:user_password) { 'bob1!' * 4 }
+  let(:user) do
+    FactoryGirl.create(:user,
+                       force_password_change: false,
+                       first_login: false,
+                       login: 'bob',
+                       mail: 'bob@example.com',
+                       firstname: 'Bo',
+                       lastname: 'B',
+                       password: user_password,
+                       password_confirmation: user_password,
+                      )
+  end
+
+  let(:other_user) { FactoryGirl.create(:user) }
+
+  it 'enforces the current user to be set correctly on each api request' do
+    # login to set the session
+    visit signin_path
+    within('#login-form') do
+      fill_in('username', with: user.login)
+      fill_in('password', with: user_password)
+      click_link_or_button I18n.t(:button_login)
+    end
+
+    # simulate another user having used the process
+    # which would cause User.current to be set
+    User.current = other_user
+
+    # disable a hack in the API's authenticate method
+    # which would cause authentication to not work
+    disable_test_auth_protection
+
+    # taking /api/v3 as it does not run any authorization
+    visit '/api/v3'
+    expect(User.current).to eql(user)
+  end
+
+  def disable_test_auth_protection
+    ENV['CAPYBARA_DISABLE_TEST_AUTH_PROTECTION'] = 'true'
+  end
+
+  def enable_test_auth_protection
+    ENV.delete 'CAPYBARA_DISABLE_TEST_AUTH_PROTECTION'
+  end
+end