diff options
| -rw-r--r-- | app/controllers/watchers_controller.rb | 27 | ||||
| -rw-r--r-- | app/models/issue.rb | 4 | ||||
| -rw-r--r-- | app/views/watchers/_watchers.html.erb | 4 | ||||
| -rw-r--r-- | lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb | 18 | ||||
| -rw-r--r-- | test/functional/issues_controller_test.rb | 11 | ||||
| -rw-r--r-- | test/functional/messages_controller_test.rb | 11 | ||||
| -rw-r--r-- | test/functional/watchers_controller_test.rb | 60 | ||||
| -rw-r--r-- | test/functional/wiki_controller_test.rb | 11 | ||||
| -rw-r--r-- | test/unit/issue_test.rb | 20 |
9 files changed, 155 insertions, 11 deletions
diff --git a/app/controllers/watchers_controller.rb b/app/controllers/watchers_controller.rb index 8933de96f..28e66a386 100644 --- a/app/controllers/watchers_controller.rb +++ b/app/controllers/watchers_controller.rb @@ -41,6 +41,8 @@ class WatchersController < ApplicationController end def create + return unless authorize_for_watchable_type(:add) + user_ids = [] if params[:watcher] user_ids << (params[:watcher][:user_ids] || params[:watcher][:user_id]) @@ -51,7 +53,9 @@ class WatchersController < ApplicationController users = Principal.assignable_watchers.where(:id => user_ids).to_a users.each do |user| @watchables.each do |watchable| - Watcher.create(:watchable => watchable, :user => user) + if watchable.valid_watcher?(user) + Watcher.create(:watchable => watchable, :user => user) + end end end respond_to do |format| @@ -76,6 +80,8 @@ class WatchersController < ApplicationController end def destroy + return unless authorize_for_watchable_type(:delete) + user = Principal.find(params[:user_id]) @watchables.each do |watchable| watchable.set_watcher(user, false) @@ -156,11 +162,10 @@ class WatchersController < ApplicationController users = scope.sorted.like(params[:q]).to_a if @watchables && @watchables.size == 1 watchable_object = @watchables.first - users -= watchable_object.watcher_users - - if watchable_object.respond_to?(:visible?) - users.reject! {|user| user.is_a?(User) && !watchable_object.visible?(user)} - end + users -= watchable_object.visible_watcher_users + end + @watchables&.each do |watchable| + users.reject!{|user| !watchable.valid_watcher?(user)} end users end @@ -228,4 +233,14 @@ class WatchersController < ApplicationController objects end + + # Check permission for the watchable type for each watchable involved + def authorize_for_watchable_type(action) + if @watchables.any?{|watchable| !User.current.allowed_to?(:"#{action}_#{watchable.class.name.underscore}_watchers", watchable.project)} + render_403 + return false + else + return true + end + end end diff --git a/app/models/issue.rb b/app/models/issue.rb index 31d2f75dd..823bf90e8 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -314,9 +314,9 @@ class Issue < ActiveRecord::Base attachement.copy(:container => self) end end + unless options[:watchers] == false - self.watcher_user_ids = - issue.watcher_users.select{|u| u.status == User::STATUS_ACTIVE}.map(&:id) + self.watcher_user_ids = issue.visible_watcher_users.select{|u| u.status == User::STATUS_ACTIVE}.map(&:id) end @copied_from = issue @copy_options = options diff --git a/app/views/watchers/_watchers.html.erb b/app/views/watchers/_watchers.html.erb index b53e09d50..25d852c87 100644 --- a/app/views/watchers/_watchers.html.erb +++ b/app/views/watchers/_watchers.html.erb @@ -8,6 +8,10 @@ </div> <% end %> +<% if User.current.allowed_to?(:"view_#{watched_klass_name}_watchers", watched.project) %> <h3><%= l(:"label_#{watched_klass_name}_watchers") %> (<%= watched.watcher_users.size %>)</h3> <%= watchers_list(watched) %> +<% else %> +<h3><%= l(:"label_#{watched_klass_name}_watchers") %></h3> +<% end %> diff --git a/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb b/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb index 1a5888464..139766b21 100644 --- a/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb +++ b/lib/plugins/acts_as_watchable/lib/acts_as_watchable.rb @@ -43,6 +43,24 @@ module Redmine users end + # array of watchers that the given user is allowed to see + def visible_watcher_users(user = User.current) + if user.allowed_to?(:"view_#{self.class.name.underscore}_watchers", project) + watcher_users + else + # without permission, the user can only see themselves (if they're a watcher) + watcher_users & [user] + end + end + + # true if user can be added as a watcher + def valid_watcher?(user) + return true unless respond_to?(:visible?) + return true unless user.is_a?(User) + + visible?(user) + end + # Adds user as a watcher def add_watcher(user) if persisted? diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index db45bb287..7501843cf 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -2733,6 +2733,17 @@ class IssuesControllerTest < Redmine::ControllerTest end end + def test_show_should_not_display_watchers_without_permission + @request.session[:user_id] = 2 + Role.find(1).remove_permission! :view_issue_watchers + issue = Issue.find(1) + issue.add_watcher User.find(2) + issue.add_watcher Group.find(10) + get(:show, :params => {:id => 1}) + assert_select 'div#watchers ul', 0 + assert_select 'h3', {text: /Watchers \(\d*\)/, count: 0} + end + def test_show_should_display_watchers_with_gravatars @request.session[:user_id] = 2 issue = Issue.find(1) diff --git a/test/functional/messages_controller_test.rb b/test/functional/messages_controller_test.rb index 85f45f1d7..317c779a2 100644 --- a/test/functional/messages_controller_test.rb +++ b/test/functional/messages_controller_test.rb @@ -114,6 +114,17 @@ class MessagesControllerTest < Redmine::ControllerTest end end + def test_show_should_not_display_watchers_without_permission + @request.session[:user_id] = 2 + Role.find(1).remove_permission! :view_message_watchers + message = Message.find(1) + message.add_watcher User.find(2) + message.add_watcher Group.find(10) + get(:show, :params => {:board_id => 1, :id => 1}) + assert_select 'div#watchers ul', 0 + assert_select 'h3', {text: /Watchers \(\d*\)/, count: 0} + end + def test_get_new @request.session[:user_id] = 2 get(:new, :params => {:board_id => 1}) diff --git a/test/functional/watchers_controller_test.rb b/test/functional/watchers_controller_test.rb index 369d30b5b..67f4a4736 100644 --- a/test/functional/watchers_controller_test.rb +++ b/test/functional/watchers_controller_test.rb @@ -211,6 +211,25 @@ class WatchersControllerTest < Redmine::ControllerTest ) end + def test_new_without_view_watchers_permission + @request.session[:user_id] = 2 + Role.find(1).remove_permission! :view_issue_watchers + get :new, :params => {:object_type => 'issue', :object_id => '2'}, :xhr => true + assert_response :success + assert_match %r{name=\\"watcher\[user_ids\]\[\]\\" value=\\"2\\"}, response.body + # User should not be able to reverse engineer that User 3 is watching the issue already + assert_match %r{name=\\"watcher\[user_ids\]\[\]\\" value=\\"3\\"}, response.body + end + + def test_new_dont_show_self_when_watching_without_view_watchers_permission + @request.session[:user_id] = 2 + Role.find(1).remove_permission! :view_issue_watchers + Issue.find(2).add_watcher(User.find(2)) + get :new, :params => {:object_type => 'issue', :object_id => '2'}, :xhr => true + assert_response :success + assert_no_match %r{name=\\"watcher\[user_ids\]\[\]\\" value=\\"2\\"}, response.body + end + def test_create_as_html @request.session[:user_id] = 2 assert_difference('Watcher.count') do @@ -458,11 +477,11 @@ class WatchersControllerTest < Redmine::ControllerTest assert_response :success - # All users from two projects eCookbook (7) and Private child of eCookbook (9) - assert_select 'input', :count => 5 + # All users from two projects eCookbook (7) and Private child of eCookbook + # (9) who can see both issues + assert_select 'input', :count => 4 assert_select 'input[name=?][value="1"]', 'watcher[user_ids][]' assert_select 'input[name=?][value="2"]', 'watcher[user_ids][]' - assert_select 'input[name=?][value="3"]', 'watcher[user_ids][]' assert_select 'input[name=?][value="8"]', 'watcher[user_ids][]' assert_select 'input[name=?][value="10"]', 'watcher[user_ids][]' end @@ -559,6 +578,41 @@ class WatchersControllerTest < Redmine::ControllerTest assert !wiki_page.watched_by?(user) end + def test_destroy_without_permission + @request.session[:user_id] = 2 + wiki_page = WikiPage.find(1) + user = User.find(1) + Role.find(1).remove_permission! :delete_wiki_page_watchers + + assert wiki_page.watched_by?(user) + assert_no_difference('Watcher.count') do + delete :destroy, :params => { + :object_type => 'wiki_page', :object_id => '1', :user_id => '1' + }, :xhr => true + assert_response :forbidden + end + wiki_page.reload + assert wiki_page.watched_by?(user) + end + + def test_create_without_permission + @request.session[:user_id] = 2 + wiki_page = WikiPage.find(1) + user = User.find(1) + Role.find(1).remove_permission! :add_wiki_page_watchers + Watcher.delete_all + + assert_not wiki_page.watched_by?(user) + assert_no_difference('Watcher.count') do + post :create, :params => { + :object_type => 'wiki_page', :object_id => '1', :user_id => '1' + }, :xhr => true + assert_response :forbidden + end + wiki_page.reload + assert_not wiki_page.watched_by?(user) + end + def test_destroy_locked_user user = User.find(3) user.lock! diff --git a/test/functional/wiki_controller_test.rb b/test/functional/wiki_controller_test.rb index 1e1d98f32..ad5d34b58 100644 --- a/test/functional/wiki_controller_test.rb +++ b/test/functional/wiki_controller_test.rb @@ -150,6 +150,17 @@ class WikiControllerTest < Redmine::ControllerTest end end + def test_show_should_not_display_watchers_without_permission + @request.session[:user_id] = 2 + Role.find(1).remove_permission! :view_wiki_page_watchers + page = Project.find(1).wiki.find_page('Another_page') + page.add_watcher User.find(2) + page.add_watcher Group.find(10) + get(:show, :params => {:project_id => 1, :id => 'Another_page'}) + assert_select 'div#watchers ul', 0 + assert_select 'h3', {text: /Watchers \(\d*\)/, count: 0} + end + def test_show_should_display_section_edit_links with_settings :text_formatting => 'textile' do @request.session[:user_id] = 2 diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb index 571b7ef3b..631f2614f 100644 --- a/test/unit/issue_test.rb +++ b/test/unit/issue_test.rb @@ -1498,6 +1498,8 @@ class IssueTest < ActiveSupport::TestCase user2 = User.find(3) issue = Issue.find(8) + User.current = user + Watcher.create!(:user => user, :watchable => issue) Watcher.create!(:user => user2, :watchable => issue) @@ -1511,6 +1513,24 @@ class IssueTest < ActiveSupport::TestCase assert !issue.watched_by?(user2) end + def test_copy_should_not_copy_watchers_without_permission + user = User.find(2) + user2 = User.find(3) + issue = Issue.find(8) + + Role.find(1).remove_permission! :view_issue_watchers + User.current = user + + Watcher.create!(:user => user, :watchable => issue) + Watcher.create!(:user => user2, :watchable => issue) + + issue = Issue.new.copy_from(8) + + assert issue.save + assert issue.watched_by?(user) + assert !issue.watched_by?(user2) + end + def test_copy_should_clear_subtasks_target_version_if_locked_or_closed version = Version.new(:project => Project.find(1), :name => '2.1') version.save! |