From 01f673be08be68247b72a8954379b3f0c7a9a9d3 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sun, 8 Feb 2015 10:20:53 +0000 Subject: [PATCH] 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 | 3 +-- app/controllers/issues_controller.rb | 12 +++++----- app/models/issue.rb | 22 ++++++++++--------- app/views/context_menus/issues.html.erb | 2 +- ...08105930_replace_move_issues_permission.rb | 18 +++++++++++++++ lib/redmine.rb | 1 - test/fixtures/roles.yml | 3 --- test/unit/issue_test.rb | 8 +++---- 8 files changed, 41 insertions(+), 28 deletions(-) create mode 100644 db/migrate/20150208105930_replace_move_issues_permission.rb diff --git a/app/controllers/context_menus_controller.rb b/app/controllers/context_menus_controller.rb index 3c4d46852..99b320d5c 100644 --- a/app/controllers/context_menus_controller.rb +++ b/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 diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index d1cac203e..de7155481 100644 --- a/app/controllers/issues_controller.rb +++ b/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 diff --git a/app/models/issue.rb b/app/models/issue.rb index aca31291d..5ea344a4f 100644 --- a/app/models/issue.rb +++ b/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 diff --git a/app/views/context_menus/issues.html.erb b/app/views/context_menus/issues.html.erb index 069c7f46e..65ddb002b 100644 --- a/app/views/context_menus/issues.html.erb +++ b/app/views/context_menus/issues.html.erb @@ -130,7 +130,7 @@ :class => 'icon-copy', :disabled => !@can[:copy] %> <% else %>
  • <%= context_menu_link l(:button_copy), bulk_edit_issues_path(:ids => @issue_ids, :copy => '1'), - :class => 'icon-copy', :disabled => !@can[:move] %>
    • + :class => 'icon-copy', :disabled => !@can[:copy] %> <% end %>
  • <%= 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] %>
    • diff --git a/db/migrate/20150208105930_replace_move_issues_permission.rb b/db/migrate/20150208105930_replace_move_issues_permission.rb new file mode 100644 index 000000000..79ed506bc --- /dev/null +++ b/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 diff --git a/lib/redmine.rb b/lib/redmine.rb index 298e153fe..23db652b7 100644 --- a/lib/redmine.rb +++ b/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 diff --git a/test/fixtures/roles.yml b/test/fixtures/roles.yml index 241c5afe6..000173bbe 100644 --- a/test/fixtures/roles.yml +++ b/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 diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb index 0384dfb0e..768899856 100644 --- a/test/unit/issue_test.rb +++ b/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 -- 2.39.5