From c4fd1750f703a7649f0b3b52b25cf32fa532b5b3 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sun, 5 Jun 2016 13:45:10 +0000 Subject: Adds permission to edit and delete issues by role/tracker (#285). git-svn-id: http://svn.redmine.org/redmine/trunk@15466 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/context_menus_controller.rb | 4 +-- app/controllers/issues_controller.rb | 9 ++++++ app/models/issue.rb | 27 ++++++++++++---- app/views/issues/_action_menu.html.erb | 2 +- app/views/issues/_edit.html.erb | 29 +++++++++-------- app/views/issues/_history.html.erb | 2 +- app/views/issues/show.html.erb | 2 +- app/views/roles/_form.html.erb | 7 ++-- public/stylesheets/application.css | 1 + test/functional/issues_controller_test.rb | 50 +++++++++++++++++++++++++++++ 10 files changed, 106 insertions(+), 27 deletions(-) diff --git a/app/controllers/context_menus_controller.rb b/app/controllers/context_menus_controller.rb index 59ee3a77a..66ec35085 100644 --- a/app/controllers/context_menus_controller.rb +++ b/app/controllers/context_menus_controller.rb @@ -29,11 +29,11 @@ class ContextMenusController < ApplicationController @allowed_statuses = @issues.map(&:new_statuses_allowed_to).reduce(:&) - @can = {:edit => User.current.allowed_to?(:edit_issues, @projects), + @can = {:edit => @issues.all?(&:attributes_editable?), :log_time => (@project && User.current.allowed_to?(:log_time, @project)), :copy => User.current.allowed_to?(:copy_issues, @projects) && Issue.allowed_target_projects.any?, :add_watchers => User.current.allowed_to?(:add_issue_watchers, @projects), - :delete => User.current.allowed_to?(:delete_issues, @projects) + :delete => @issues.all?(&:deletable?) } if @project if @issue diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index 37825c995..67956667a 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -211,6 +211,10 @@ class IssuesController < ApplicationController unless User.current.allowed_to?(:copy_issues, @projects) raise ::Unauthorized end + else + unless @issues.all?(&:attributes_editable?) + raise ::Unauthorized + end end @allowed_projects = Issue.allowed_target_projects @@ -263,6 +267,10 @@ class IssuesController < ApplicationController unless User.current.allowed_to?(:add_issues, target_projects) raise ::Unauthorized end + else + unless @issues.all?(&:attributes_editable?) + raise ::Unauthorized + end end unsaved_issues = [] @@ -316,6 +324,7 @@ class IssuesController < ApplicationController end def destroy + raise Unauthorized unless @issues.all?(&:deletable?) @hours = TimeEntry.where(:issue_id => @issues.map(&:id)).sum(:hours).to_f if @hours > 0 case params[:todo] diff --git a/app/models/issue.rb b/app/models/issue.rb index 91ea73a6f..28a7f9fe6 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -172,14 +172,24 @@ class Issue < ActiveRecord::Base end end - # Returns true if user or current user is allowed to edit or add a note to the issue + # Returns true if user or current user is allowed to edit or add notes to the issue def editable?(user=User.current) - attributes_editable?(user) || user.allowed_to?(:add_issue_notes, project) + attributes_editable?(user) || notes_addable?(user) end # Returns true if user or current user is allowed to edit the issue def attributes_editable?(user=User.current) - user.allowed_to?(:edit_issues, project) + user_tracker_permission?(user, :edit_issues) + end + + # Returns true if user or current user is allowed to add notes to the issue + def notes_addable?(user=User.current) + user_tracker_permission?(user, :add_issue_notes) + end + + # Returns true if user or current user is allowed to delete the issue + def deletable?(user=User.current) + user_tracker_permission?(user, :delete_issues) end def initialize(attributes=nil, *args) @@ -429,10 +439,10 @@ class Issue < ActiveRecord::Base 'custom_fields', 'lock_version', 'notes', - :if => lambda {|issue, user| issue.new_record? || user.allowed_to?(:edit_issues, issue.project) } + :if => lambda {|issue, user| issue.new_record? || issue.attributes_editable?(user) } safe_attributes 'notes', - :if => lambda {|issue, user| user.allowed_to?(:add_issue_notes, issue.project)} + :if => lambda {|issue, user| issue.notes_addable?(user)} safe_attributes 'private_notes', :if => lambda {|issue, user| !issue.new_record? && user.allowed_to?(:set_notes_private, issue.project)} @@ -447,7 +457,7 @@ class Issue < ActiveRecord::Base } safe_attributes 'parent_issue_id', - :if => lambda {|issue, user| (issue.new_record? || user.allowed_to?(:edit_issues, issue.project)) && + :if => lambda {|issue, user| (issue.new_record? || issue.attributes_editable?(user)) && user.allowed_to?(:manage_subtasks, issue.project)} def safe_attribute_names(user=nil) @@ -1406,6 +1416,11 @@ class Issue < ActiveRecord::Base private + def user_tracker_permission?(user, permission) + roles = user.roles_for_project(project).select {|r| r.has_permission?(permission)} + roles.any? {|r| r.permissions_all_trackers?(permission) || r.permissions_tracker_ids?(permission, tracker_id)} + end + def after_project_change # Update project_id on related time entries TimeEntry.where({:issue_id => id}).update_all(["project_id = ?", project_id]) diff --git a/app/views/issues/_action_menu.html.erb b/app/views/issues/_action_menu.html.erb index c3304626d..b535faec9 100644 --- a/app/views/issues/_action_menu.html.erb +++ b/app/views/issues/_action_menu.html.erb @@ -3,5 +3,5 @@ <%= link_to l(:button_log_time), new_issue_time_entry_path(@issue), :class => 'icon icon-time-add' if User.current.allowed_to?(:log_time, @project) %> <%= watcher_link(@issue, User.current) %> <%= link_to l(:button_copy), project_copy_issue_path(@project, @issue), :class => 'icon icon-copy' if User.current.allowed_to?(:copy_issues, @project) && Issue.allowed_target_projects.any? %> -<%= link_to l(:button_delete), issue_path(@issue), :data => {:confirm => issues_destroy_confirmation_message(@issue)}, :method => :delete, :class => 'icon icon-del' if User.current.allowed_to?(:delete_issues, @project) %> +<%= link_to l(:button_delete), issue_path(@issue), :data => {:confirm => issues_destroy_confirmation_message(@issue)}, :method => :delete, :class => 'icon icon-del' if @issue.deletable? %> diff --git a/app/views/issues/_edit.html.erb b/app/views/issues/_edit.html.erb index 24a01cad0..67e33246d 100644 --- a/app/views/issues/_edit.html.erb +++ b/app/views/issues/_edit.html.erb @@ -27,21 +27,22 @@ <% end %> <% end %> - -
<%= l(:field_notes) %> - <%= f.text_area :notes, :cols => 60, :rows => 10, :class => 'wiki-edit', :no_label => true %> - <%= wikitoolbar_for 'issue_notes' %> - - <% if @issue.safe_attribute? 'private_notes' %> - <%= f.check_box :private_notes, :no_label => true %> + <% if @issue.notes_addable? %> +
<%= l(:field_notes) %> + <%= f.text_area :notes, :cols => 60, :rows => 10, :class => 'wiki-edit', :no_label => true %> + <%= wikitoolbar_for 'issue_notes' %> + + <% if @issue.safe_attribute? 'private_notes' %> + <%= f.check_box :private_notes, :no_label => true %> + <% end %> + + <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %> +
+ +
<%= l(:label_attachment_plural) %> +

<%= render :partial => 'attachments/form', :locals => {:container => @issue} %>

+
<% end %> - - <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %> -
- -
<%= l(:label_attachment_plural) %> -

<%= render :partial => 'attachments/form', :locals => {:container => @issue} %>

-
<%= f.hidden_field :lock_version %> diff --git a/app/views/issues/_history.html.erb b/app/views/issues/_history.html.erb index 993dfee3c..307ac909b 100644 --- a/app/views/issues/_history.html.erb +++ b/app/views/issues/_history.html.erb @@ -1,4 +1,4 @@ -<% reply_links = authorize_for('issues', 'edit') -%> +<% reply_links = issue.notes_addable? -%> <% for journal in journals %>
diff --git a/app/views/issues/show.html.erb b/app/views/issues/show.html.erb index 70a7fe165..2cbff32e5 100644 --- a/app/views/issues/show.html.erb +++ b/app/views/issues/show.html.erb @@ -77,7 +77,7 @@ end %> <% if @issue.description? %>
- <%= link_to l(:button_quote), quoted_issue_path(@issue), :remote => true, :method => 'post', :class => 'icon icon-comment' if authorize_for('issues', 'edit') %> + <%= link_to l(:button_quote), quoted_issue_path(@issue), :remote => true, :method => 'post', :class => 'icon icon-comment' if @issue.notes_addable? %>

<%=l(:field_description)%>

diff --git a/app/views/roles/_form.html.erb b/app/views/roles/_form.html.erb index c8a3a61cc..8b7b94c73 100644 --- a/app/views/roles/_form.html.erb +++ b/app/views/roles/_form.html.erb @@ -64,7 +64,9 @@

<%= l(:label_issue_tracking) %>

-<% permissions = %w(view_issues add_issues) %> +<% permissions = %w(view_issues add_issues edit_issues add_issue_notes delete_issues) %> + +
@@ -87,7 +89,7 @@ <% end %> <% Tracker.sorted.all.each do |tracker| %> - + "> <% permissions.each do |permission| %>
<%= tracker.name %><%= check_box_tag "role[permissions_tracker_ids][#{permission}][]", @@ -100,6 +102,7 @@ <% end %>
+
<% permissions.each do |permission| %> <%= hidden_field_tag "role[permissions_tracker_ids][#{permission}][]", '' %> diff --git a/public/stylesheets/application.css b/public/stylesheets/application.css index 83060eaa2..c276ebd27 100644 --- a/public/stylesheets/application.css +++ b/public/stylesheets/application.css @@ -152,6 +152,7 @@ table.list td.buttons img, div.buttons img {vertical-align:middle;} table.list td.reorder {width:15%; white-space:nowrap; text-align:center; } table.list table.progress td {padding-right:0px;} table.list caption { text-align: left; padding: 0.5em 0.5em 0.5em 0; } +#role-permissions-trackers table.list th {white-space:normal;} .table-list-cell {display: table-cell; vertical-align: top; padding:2px; } diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 8888cf712..dc50d1331 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -3872,6 +3872,30 @@ class IssuesControllerTest < ActionController::TestCase assert_redirected_to '/issues/11?issue_count=3&issue_position=2&next_issue_id=12&prev_issue_id=8' end + def test_update_with_permission_on_tracker_should_be_allowed + role = Role.find(1) + role.set_permission_trackers :edit_issues, [1] + role.save! + issue = Issue.generate!(:project_id => 1, :tracker_id => 1, :subject => 'Original subject') + + @request.session[:user_id] = 2 + put :update, :id => issue.id, :issue => {:subject => 'Changed subject'} + assert_response 302 + assert_equal 'Changed subject', issue.reload.subject + end + + def test_update_without_permission_on_tracker_should_be_denied + role = Role.find(1) + role.set_permission_trackers :edit_issues, [1] + role.save! + issue = Issue.generate!(:project_id => 1, :tracker_id => 2, :subject => 'Original subject') + + @request.session[:user_id] = 2 + put :update, :id => issue.id, :issue => {:subject => 'Changed subject'} + assert_response 302 + assert_equal 'Original subject', issue.reload.subject + end + def test_get_bulk_edit @request.session[:user_id] = 2 get :bulk_edit, :ids => [1, 3] @@ -4702,6 +4726,32 @@ class IssuesControllerTest < ActionController::TestCase assert_response 404 end + def test_destroy_with_permission_on_tracker_should_be_allowed + role = Role.find(1) + role.set_permission_trackers :delete_issues, [1] + role.save! + issue = Issue.generate!(:project_id => 1, :tracker_id => 1) + + @request.session[:user_id] = 2 + assert_difference 'Issue.count', -1 do + delete :destroy, :id => issue.id + end + assert_response 302 + end + + def test_destroy_without_permission_on_tracker_should_be_denied + role = Role.find(1) + role.set_permission_trackers :delete_issues, [2] + role.save! + issue = Issue.generate!(:project_id => 1, :tracker_id => 1) + + @request.session[:user_id] = 2 + assert_no_difference 'Issue.count' do + delete :destroy, :id => issue.id + end + assert_response 403 + end + def test_default_search_scope get :index -- cgit v1.2.3