Merge branch 'security-milestone-labels' into 'master'

[master] Check label_ids parent when updating issue board

See merge request gitlab/gitlabhq!3027

4 files changed, 162 insertions, 8 deletions

diff --git a/app/models/label.rb b/app/models/label.rb
index 96bdb7f17c5..e13a2322c6b 100644
--- a/app/models/label.rb
+++ b/app/models/label.rb
@@ -133,6 +133,10 @@ class Label < ActiveRecord::Base
     1
   end
 
+  def self.by_ids(ids)
+    where(id: ids)
+  end
+
   def open_issues_count(user = nil)
     issues_count(user, state: 'opened')
   end
diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb
index f35ad2a9d8b..49739083868 100644
--- a/app/services/issuable_base_service.rb
+++ b/app/services/issuable_base_service.rb
@@ -70,10 +70,14 @@ class IssuableBaseService < BaseService
   end
 
   def filter_labels
-    filter_labels_in_param(:add_label_ids)
-    filter_labels_in_param(:remove_label_ids)
-    filter_labels_in_param(:label_ids)
-    find_or_create_label_ids
+    params[:add_label_ids] = labels_service.filter_labels_ids_in_param(:add_label_ids) if params[:add_label_ids]
+    params[:remove_label_ids] = labels_service.filter_labels_ids_in_param(:remove_label_ids) if params[:remove_label_ids]
+
+    if params[:label_ids]
+      params[:label_ids] = labels_service.filter_labels_ids_in_param(:label_ids)
+    elsif params[:labels]
+      params[:label_ids] = labels_service.find_or_create_by_titles.map(&:id)
+    end
   end
 
   # rubocop: disable CodeReuse/ActiveRecord
@@ -101,6 +105,10 @@ class IssuableBaseService < BaseService
     end.compact
   end
 
+  def labels_service
+    @labels_service ||= ::Labels::AvailableLabelsService.new(current_user, parent, params)
+  end
+
   def process_label_ids(attributes, existing_label_ids: nil)
     label_ids = attributes.delete(:label_ids)
     add_label_ids = attributes.delete(:add_label_ids)
@@ -118,10 +126,6 @@ class IssuableBaseService < BaseService
     new_label_ids
   end
 
-  def available_labels
-    @available_labels ||= LabelsFinder.new(current_user, project_id: @project.id, include_ancestor_groups: true).execute
-  end
-
   def handle_quick_actions_on_create(issuable)
     merge_quick_actions_into_params!(issuable)
   end
diff --git a/app/services/labels/available_labels_service.rb b/app/services/labels/available_labels_service.rb
new file mode 100644
index 00000000000..fe477d96970
--- /dev/null
+++ b/app/services/labels/available_labels_service.rb
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+module Labels
+  class AvailableLabelsService
+    attr_reader :current_user, :parent, :params
+
+    def initialize(current_user, parent, params)
+      @current_user = current_user
+      @parent = parent
+      @params = params
+    end
+
+    def find_or_create_by_titles
+      labels = params.delete(:labels)
+
+      return [] unless labels
+
+      labels = labels.split(',') if labels.is_a?(String)
+
+      labels.map do |label_name|
+        label = Labels::FindOrCreateService.new(
+          current_user,
+          parent,
+          include_ancestor_groups: true,
+          title: label_name.strip,
+          available_labels: available_labels
+        ).execute
+
+        label
+      end.compact
+    end
+
+    def filter_labels_ids_in_param(key)
+      return [] if params[key].to_a.empty?
+
+      # rubocop:disable CodeReuse/ActiveRecord
+      available_labels.by_ids(params[key]).pluck(:id)
+      # rubocop:enable CodeReuse/ActiveRecord
+    end
+
+    private
+
+    def available_labels
+      @available_labels ||= LabelsFinder.new(current_user, finder_params).execute
+    end
+
+    def finder_params
+      params = { include_ancestor_groups: true }
+
+      case parent
+      when Group
+        params[:group_id] = parent.id
+        params[:only_group_labels] = true
+      when Project
+        params[:project_id] = parent.id
+      end
+
+      params
+    end
+  end
+end
diff --git a/spec/services/labels/available_labels_service_spec.rb b/spec/services/labels/available_labels_service_spec.rb
new file mode 100644
index 00000000000..4d5c87ecc53
--- /dev/null
+++ b/spec/services/labels/available_labels_service_spec.rb
@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+require 'spec_helper'
+
+describe Labels::AvailableLabelsService do
+  let(:user) { create(:user) }
+  let(:project) { create(:project, :public, group: group) }
+  let(:group) { create(:group) }
+
+  let(:project_label) { create(:label, project: project) }
+  let(:other_project_label) { create(:label) }
+  let(:group_label) { create(:group_label, group: group) }
+  let(:other_group_label) { create(:group_label) }
+  let(:labels) { [project_label, other_project_label, group_label, other_group_label] }
+
+  context '#find_or_create_by_titles' do
+    let(:label_titles) { labels.map(&:title).push('non existing title') }
+
+    context 'when parent is a project' do
+      context 'when a user is not a project member' do
+        it 'returns only relevant label ids' do
+          result = described_class.new(user, project, labels: label_titles).find_or_create_by_titles
+
+          expect(result).to match_array([project_label, group_label])
+        end
+      end
+
+      context 'when a user is a project member' do
+        before do
+          project.add_developer(user)
+        end
+
+        it 'creates new labels for not found titles' do
+          result = described_class.new(user, project, labels: label_titles).find_or_create_by_titles
+
+          expect(result.count).to eq(5)
+          expect(result).to include(project_label, group_label)
+          expect(result).not_to include(other_project_label, other_group_label)
+        end
+      end
+    end
+
+    context 'when parent is a group' do
+      context 'when a user is not a group member' do
+        it 'returns only relevant label ids' do
+          result = described_class.new(user, group, labels: label_titles).find_or_create_by_titles
+
+          expect(result).to match_array([group_label])
+        end
+      end
+
+      context 'when a user is a group member' do
+        before do
+          group.add_developer(user)
+        end
+
+        it 'creates new labels for not found titles' do
+          result = described_class.new(user, group, labels: label_titles).find_or_create_by_titles
+
+          expect(result.count).to eq(5)
+          expect(result).to include(group_label)
+          expect(result).not_to include(project_label, other_project_label, other_group_label)
+        end
+      end
+    end
+  end
+
+  context '#filter_labels_ids_in_param' do
+    let(:label_ids) { labels.map(&:id).push(99999) }
+
+    context 'when parent is a project' do
+      it 'returns only relevant label ids' do
+        result = described_class.new(user, project, ids: label_ids).filter_labels_ids_in_param(:ids)
+
+        expect(result).to match_array([project_label.id, group_label.id])
+      end
+    end
+
+    context 'when parent is a group' do
+      it 'returns only relevant label ids' do
+        result = described_class.new(user, group, ids: label_ids).filter_labels_ids_in_param(:ids)
+
+        expect(result).to match_array([group_label.id])
+      end
+    end
+  end
+end