Removed :move_issues permission (#18855).

This permission was wrongly used to allow bulk issue copy. To prevent user from moving an issue to another project, the project field should now be set to read-only in the workflow permissions. A migration does this automatically for roles that have the edit_issues permission without having the move_issues permission.

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

app/controllers/context_menus_controller.rb

@@ -31,8 +31,7 @@ class ContextMenusController < ApplicationController
 
     @can = {:edit => User.current.allowed_to?(:edit_issues, @projects),
             :log_time => (@project && User.current.allowed_to?(:log_time, @project)),
-            :move => (@project && User.current.allowed_to?(:move_issues, @project)),
-            :copy => (@issue && @project.trackers.include?(@issue.tracker) && User.current.allowed_to?(:add_issues, @project)),
+            :copy => User.current.allowed_to?(:add_issues, @projects),
             :delete => User.current.allowed_to?(:delete_issues, @projects)
             }
     if @project

app/controllers/issues_controller.rb

@@ -219,13 +219,11 @@ class IssuesController < ApplicationController
     @copy = params[:copy].present?
     @notes = params[:notes]
 
-    if User.current.allowed_to?(:move_issues, @projects)
-      @allowed_projects = Issue.allowed_target_projects_on_move
-      if params[:issue]
-        @target_project = @allowed_projects.detect {|p| p.id.to_s == params[:issue][:project_id].to_s}
-        if @target_project
-          target_projects = [@target_project]
-        end
+    @allowed_projects = Issue.allowed_target_projects
+    if params[:issue]
+      @target_project = @allowed_projects.detect {|p| p.id.to_s == params[:issue][:project_id].to_s}
+      if @target_project
+        target_projects = [@target_project]
       end
     end
     target_projects ||= @projects

app/models/issue.rb

@@ -378,8 +378,8 @@ class Issue < ActiveRecord::Base
     :if => lambda {|issue, user|
       if issue.new_record?
         issue.copy?
-      elsif user.allowed_to?(:move_issues, issue.project)
-        Issue.allowed_target_projects_on_move.count > 1
+      else
+        user.allowed_to?(:edit_issues, issue.project)
       end
     }
 
@@ -1282,16 +1282,18 @@ class Issue < ActiveRecord::Base
 
   # Returns a scope of projects that user can assign the issue to
   def allowed_target_projects(user=User.current)
-    if new_record?
-      Project.where(Project.allowed_to_condition(user, :add_issues))
-    else
-      self.class.allowed_target_projects_on_move(user)
-    end
+    current_project = new_record? ? nil : project
+    self.class.allowed_target_projects(user, current_project)
   end
 
-  # Returns a scope of projects that user can move issues to
-  def self.allowed_target_projects_on_move(user=User.current)
-    Project.where(Project.allowed_to_condition(user, :move_issues))
+  # Returns a scope of projects that user can assign issues to
+  # If current_project is given, it will be included in the scope
+  def self.allowed_target_projects(user=User.current, current_project=nil)
+    condition = Project.allowed_to_condition(user, :add_issues)
+    if current_project
+      condition = ["(#{condition}) OR #{Project.table_name}.id = ?", current_project.id]
+    end
+    Project.where(condition)
   end
 
   private

app/views/context_menus/issues.html.erb

@@ -130,7 +130,7 @@
           :class => 'icon-copy', :disabled => !@can[:copy] %></li>
 <% else %>
   <li><%= context_menu_link l(:button_copy), bulk_edit_issues_path(:ids => @issue_ids, :copy => '1'),
-                          :class => 'icon-copy', :disabled => !@can[:move] %></li>
+                          :class => 'icon-copy', :disabled => !@can[:copy] %></li>
 <% end %>
   <li><%= context_menu_link l(:button_delete), issues_path(:ids => @issue_ids, :back_url => @back),
                             :method => :delete, :data => {:confirm => issues_destroy_confirmation_message(@issues)}, :class => 'icon-del', :disabled => !@can[:delete] %></li>

db/migrate/20150208105930_replace_move_issues_permission.rb

@@ -0,0 +1,18 @@
+class ReplaceMoveIssuesPermission < ActiveRecord::Migration
+  def self.up
+    Role.all.each do |role|
+      if role.has_permission?(:edit_issues) && !role.has_permission?(:move_issues)
+        # inserts one ligne per trakcer and status
+        WorkflowPermission.connection.insert_sql(
+          "INSERT INTO #{WorkflowPermission.table_name} (tracker_id, old_status_id, role_id, type, field_name, rule)" +
+          " SELECT t.id, s.id, #{role.id}, 'WorkflowPermission', 'project_id', 'readonly'" +
+          " FROM #{Tracker.table_name} t, #{IssueStatus.table_name} s"
+        )
+      end
+    end
+  end
+
+  def self.down
+    raise IrreversibleMigration
+  end
+end

lib/redmine.rb

@@ -109,7 +109,6 @@ Redmine::AccessControl.map do |map|
     map.permission :edit_own_issue_notes, {:journals => :edit}, :require => :loggedin
     map.permission :view_private_notes, {}, :read => true, :require => :member
     map.permission :set_notes_private, {}, :require => :member
-    map.permission :move_issues, {:issues => [:bulk_edit, :bulk_update]}, :require => :loggedin
     map.permission :delete_issues, {:issues => :destroy}, :require => :member
     # Queries
     map.permission :manage_public_queries, {:queries => [:new, :create, :edit, :update, :destroy]}, :require => :member

test/fixtures/roles.yml

@@ -20,7 +20,6 @@ roles_001:
     - :manage_issue_relations
     - :manage_subtasks
     - :add_issue_notes
-    - :move_issues
     - :delete_issues
     - :view_issue_watchers
     - :add_issue_watchers
@@ -81,7 +80,6 @@ roles_002:
     - :manage_issue_relations
     - :manage_subtasks
     - :add_issue_notes
-    - :move_issues
     - :delete_issues
     - :view_issue_watchers
     - :save_queries
@@ -128,7 +126,6 @@ roles_003:
     - :edit_issues
     - :manage_issue_relations
     - :add_issue_notes
-    - :move_issues
     - :view_issue_watchers
     - :save_queries
     - :view_gantt

test/unit/issue_test.rb

@@ -1211,13 +1211,13 @@ class IssueTest < ActiveSupport::TestCase
     assert issue.save
   end
 
-  def test_allowed_target_projects_on_move_should_include_projects_with_issue_tracking_enabled
-    assert_include Project.find(2), Issue.allowed_target_projects_on_move(User.find(2))
+  def test_allowed_target_projects_should_include_projects_with_issue_tracking_enabled
+    assert_include Project.find(2), Issue.allowed_target_projects(User.find(2))
   end
 
-  def test_allowed_target_projects_on_move_should_not_include_projects_with_issue_tracking_disabled
+  def test_allowed_target_projects_should_not_include_projects_with_issue_tracking_disabled
     Project.find(2).disable_module! :issue_tracking
-    assert_not_include Project.find(2), Issue.allowed_target_projects_on_move(User.find(2))
+    assert_not_include Project.find(2), Issue.allowed_target_projects(User.find(2))
   end
 
   def test_move_to_another_project_with_same_category