Creating time tracking entry for other user through rest API fails with 403 (#32774).

git-svn-id: http://svn.redmine.org/redmine/trunk@19676 e93f8b46-1217-0410-a6f0-8f06a7374b81

4 files changed, 46 insertions, 16 deletions

diff --git a/app/controllers/timelog_controller.rb b/app/controllers/timelog_controller.rb
index d304fe3b2..782ee5d14 100644
--- a/app/controllers/timelog_controller.rb
+++ b/app/controllers/timelog_controller.rb
@@ -28,8 +28,6 @@ class TimelogController < ApplicationController
   before_action :find_optional_issue, :only => [:new, :create]
   before_action :find_optional_project, :only => [:index, :report]
 
-  before_action :authorize_logging_time_for_other_users, :only => [:create, :update]
-
   accept_rss_auth :index
   accept_api_auth :index, :show, :create, :update, :destroy
 
@@ -258,13 +256,6 @@ class TimelogController < ApplicationController
     end
   end
 
-  def authorize_logging_time_for_other_users
-    if !User.current.allowed_to?(:log_time_for_other_users, @project) && params['time_entry'].present? && params['time_entry']['user_id'].present? && params['time_entry']['user_id'].to_i != User.current.id
-      render_error :message => l(:error_not_allowed_to_log_time_for_other_users), :status => 403
-      return false
-    end
-  end
-
   def find_time_entries
     @time_entries = TimeEntry.where(:id => params[:id] || params[:ids]).
       preload(:project => :time_entry_activities).
diff --git a/app/models/time_entry.rb b/app/models/time_entry.rb
index e8ea1a88d..5f8e475d6 100644
--- a/app/models/time_entry.rb
+++ b/app/models/time_entry.rb
@@ -116,6 +116,11 @@ class TimeEntry < ActiveRecord::Base
           @invalid_issue_id = issue_id
         end
       end
+      if user_id_changed? && user_id != author_id && !user.allowed_to?(:log_time_for_other_users, project)
+        @invalid_user_id = user_id
+      else
+        @invalid_user_id = nil
+      end
     end
     attrs
   end
@@ -146,7 +151,7 @@ class TimeEntry < ActiveRecord::Base
       end
     end
     errors.add :project_id, :invalid if project.nil?
-    if user_id_changed? && user_id != author_id && !self.assignable_users.map(&:id).include?(user_id)
+    if @invalid_user_id || (user_id_changed? && user_id != author_id && !self.assignable_users.map(&:id).include?(user_id))
       errors.add :user_id, :invalid
     end
     errors.add :issue_id, :invalid if (issue_id && !issue) || (issue && project!=issue.project) || @invalid_issue_id
diff --git a/test/functional/timelog_controller_test.rb b/test/functional/timelog_controller_test.rb
index 183599938..ae8a692f0 100644
--- a/test/functional/timelog_controller_test.rb
+++ b/test/functional/timelog_controller_test.rb
@@ -383,7 +383,7 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert_equal 2, t.author_id
   end
 
-  def test_create_for_other_user_should_deny_for_user_without_permission
+  def test_create_for_other_user_should_fail_without_permission
     Role.find_by_name('Manager').remove_permission! :log_time_for_other_users
     @request.session[:user_id] = 2
 
@@ -399,8 +399,8 @@ class TimelogControllerTest < Redmine::ControllerTest
       }
     }
 
-    assert_response 403
-    assert_select 'p[id=?]', 'errorExplanation', :text => I18n.t(:error_not_allowed_to_log_time_for_other_users)
+    assert_response :success
+    assert_select_error /User is invalid/
   end
 
   def test_create_and_continue_at_project_level
@@ -668,7 +668,7 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert_select_error /Issue is invalid/
   end
 
-  def test_update_should_deny_changing_user_for_user_without_permission
+  def test_update_should_fail_when_changing_user_without_permission
     Role.find_by_name('Manager').remove_permission! :log_time_for_other_users
     @request.session[:user_id] = 2
 
@@ -679,8 +679,8 @@ class TimelogControllerTest < Redmine::ControllerTest
       }
     }
 
-    assert_response 403
-    assert_select 'p[id=?]', 'errorExplanation', :text => I18n.t(:error_not_allowed_to_log_time_for_other_users)
+    assert_response :success
+    assert_select_error /User is invalid/
   end
 
   def test_update_should_allow_updating_existing_entry_logged_on_a_locked_user
diff --git a/test/integration/api_test/time_entries_test.rb b/test/integration/api_test/time_entries_test.rb
index 33aa88aa1..8b4380915 100644
--- a/test/integration/api_test/time_entries_test.rb
+++ b/test/integration/api_test/time_entries_test.rb
@@ -144,6 +144,40 @@ class Redmine::ApiTest::TimeEntriesTest < Redmine::ApiTest::Base
     assert_select 'errors error', :text => "Hours cannot be blank"
   end
 
+  test "POST /time_entries.xml with :project_id for other user" do
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+
+    entry = new_record(TimeEntry) do
+      post(
+        '/time_entries.xml',
+        :params =>
+          {:time_entry =>
+            {:project_id => '1', :spent_on => '2010-12-02', :user_id => '3',
+             :hours => '3.5', :activity_id => '11'}},
+        :headers => credentials('jsmith'))
+    end
+    assert_response :created
+    assert_equal 3, entry.user_id
+    assert_equal 2, entry.author_id
+  end
+
+  test "POST /time_entries.xml with :issue_id for other user" do
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+
+    entry = new_record(TimeEntry) do
+      post(
+        '/time_entries.xml',
+        :params =>
+          {:time_entry =>
+            {:issue_id => '1', :spent_on => '2010-12-02', :user_id => '3',
+             :hours => '3.5', :activity_id => '11'}},
+        :headers => credentials('jsmith'))
+    end
+    assert_response :created
+    assert_equal 3, entry.user_id
+    assert_equal 2, entry.author_id
+  end
+
   test "PUT /time_entries/:id.xml with valid parameters should update time entry" do
     assert_no_difference 'TimeEntry.count' do
       put(