From 11c386fb4f949f25a6067f172864ce554da83d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarka=20Ko=C5=A1anov=C3=A1?= Date: Tue, 12 Feb 2019 09:35:34 +0100 Subject: Check if labels are available for target issuable - labels have to be in the same project/group as an issuable --- app/models/label.rb | 4 + app/services/issuable_base_service.rb | 20 +++-- app/services/labels/available_labels_service.rb | 60 +++++++++++++++ .../unreleased/security-milestone-labels.yml | 5 ++ .../labels/available_labels_service_spec.rb | 86 ++++++++++++++++++++++ 5 files changed, 167 insertions(+), 8 deletions(-) create mode 100644 app/services/labels/available_labels_service.rb create mode 100644 ee/changelogs/unreleased/security-milestone-labels.yml create mode 100644 spec/services/labels/available_labels_service_spec.rb diff --git a/app/models/label.rb b/app/models/label.rb index 1c3db3eb35d..08ab07bba7a 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -126,6 +126,10 @@ class Label < ActiveRecord::Base fuzzy_search(query, [:title, :description]) 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/ee/changelogs/unreleased/security-milestone-labels.yml b/ee/changelogs/unreleased/security-milestone-labels.yml new file mode 100644 index 00000000000..4f8abcbc8be --- /dev/null +++ b/ee/changelogs/unreleased/security-milestone-labels.yml @@ -0,0 +1,5 @@ +--- +title: Check label_ids parent when updating issue board +merge_request: +author: +type: security 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 -- cgit v1.2.3