Check if user can read issue before being assignedissue_22664

4 files changed, 32 insertions, 23 deletions

diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb
index ecbfb625c5e..5e63825bf99 100644
--- a/app/models/concerns/issuable.rb
+++ b/app/models/concerns/issuable.rb
@@ -93,9 +93,9 @@ module Issuable
 
     def update_assignee_cache_counts
       # make sure we flush the cache for both the old *and* new assignees(if they exist)
-      previous_assignee = User.find_by_id(assignee_id_was)
-      previous_assignee.try(:update_cache_counts)
-      assignee.try(:update_cache_counts)
+      previous_assignee = User.find_by_id(assignee_id_was) if assignee_id_was
+      previous_assignee.update_cache_counts if previous_assignee
+      assignee.update_cache_counts if assignee
     end
 
     # We want to use optimistic lock for cases when only title or description are involved
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index ab3d2a9a0cd..4ce5fd993d9 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -36,14 +36,10 @@ class IssuableBaseService < BaseService
     end
   end
 
-  def filter_params(issuable_ability_name = :issue)
-    filter_assignee
-    filter_milestone
-    filter_labels
+  def filter_params(issuable)
+    ability_name = :"admin_#{issuable.to_ability_name}"
 
-    ability = :"admin_#{issuable_ability_name}"
-
-    unless can?(current_user, ability, project)
+    unless can?(current_user, ability_name, project)
       params.delete(:milestone_id)
       params.delete(:labels)
       params.delete(:add_label_ids)
@@ -52,14 +48,35 @@ class IssuableBaseService < BaseService
       params.delete(:assignee_id)
       params.delete(:due_date)
     end
+
+    filter_assignee(issuable)
+    filter_milestone
+    filter_labels
   end
 
-  def filter_assignee
-    if params[:assignee_id] == IssuableFinder::NONE
-      params[:assignee_id] = ''
+  def filter_assignee(issuable)
+    return unless params[:assignee_id].present?
+
+    assignee_id = params[:assignee_id]
+
+    if assignee_id.to_s == IssuableFinder::NONE
+      params[:assignee_id] = ""
+    else
+      params.delete(:assignee_id) unless assignee_can_read?(issuable, assignee_id)
     end
   end
 
+  def assignee_can_read?(issuable, assignee_id)
+    new_assignee = User.find_by_id(assignee_id)
+
+    return false unless new_assignee.present?
+
+    ability_name = :"read_#{issuable.to_ability_name}"
+    resource     = issuable.persisted? ? issuable : project
+
+    can?(new_assignee, ability_name, resource)
+  end
+
   def filter_milestone
     milestone_id = params[:milestone_id]
     return unless milestone_id
@@ -138,7 +155,7 @@ class IssuableBaseService < BaseService
 
   def create(issuable)
     merge_slash_commands_into_params!(issuable)
-    filter_params
+    filter_params(issuable)
 
     params.delete(:state_event)
     params[:author] ||= current_user
@@ -180,7 +197,7 @@ class IssuableBaseService < BaseService
     change_state(issuable)
     change_subscription(issuable)
     change_todo(issuable)
-    filter_params
+    filter_params(issuable)
     old_labels = issuable.labels.to_a
     old_mentioned_users = issuable.mentioned_users.to_a
 
diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb
index 742e834df97..35af867a098 100644
--- a/app/services/issues/base_service.rb
+++ b/app/services/issues/base_service.rb
@@ -17,10 +17,6 @@ module Issues
 
     private
 
-    def filter_params
-      super(:issue)
-    end
-
     def execute_hooks(issue, action = 'open')
       issue_data  = hook_data(issue, action)
       hooks_scope = issue.confidential? ? :confidential_issue_hooks : :issue_hooks
diff --git a/app/services/merge_requests/base_service.rb b/app/services/merge_requests/base_service.rb
index 800fd39c424..70e25956dc7 100644
--- a/app/services/merge_requests/base_service.rb
+++ b/app/services/merge_requests/base_service.rb
@@ -38,10 +38,6 @@ module MergeRequests
 
     private
 
-    def filter_params
-      super(:merge_request)
-    end
-
     def merge_requests_for(branch)
       origin_merge_requests = @project.origin_merge_requests
         .opened.where(source_branch: branch).to_a