Merged r22913-r22917 from trunk to 5.1-stable (#40946).

git-svn-id: https://svn.redmine.org/redmine/branches/5.1-stable@23167 e93f8b46-1217-0410-a6f0-8f06a7374b81

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!