From e2c603696a9647c15cd154156f13d0e203a930f1 Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Fri, 28 Oct 2016 11:31:45 +0200 Subject: [PATCH] Pass user instance to Labels::FindOrCreateService or skip_authorization: true Do not pass project.owner because it may return a group and Labels::FindOrCreateService throws an error in this case. Fixes #23694. --- app/finders/labels_finder.rb | 15 +++-- app/models/project.rb | 2 +- app/services/labels/find_or_create_service.rb | 11 ++-- lib/banzai/filter/label_reference_filter.rb | 2 +- lib/gitlab/fogbugz_import/importer.rb | 4 +- lib/gitlab/github_import/label_formatter.rb | 4 +- lib/gitlab/google_code_import/importer.rb | 4 +- lib/gitlab/issues_labels.rb | 2 +- .../projects/labels_controller_spec.rb | 21 +++++-- .../labels/find_or_create_service_spec.rb | 57 +++++++++++-------- 10 files changed, 73 insertions(+), 49 deletions(-) diff --git a/app/finders/labels_finder.rb b/app/finders/labels_finder.rb index 44484d64567..865f093f04a 100644 --- a/app/finders/labels_finder.rb +++ b/app/finders/labels_finder.rb @@ -4,9 +4,8 @@ class LabelsFinder < UnionFinder @params = params end - def execute(authorized_only: true) - @authorized_only = authorized_only - + def execute(skip_authorization: false) + @skip_authorization = skip_authorization items = find_union(label_ids, Label) items = with_title(items) sort(items) @@ -14,7 +13,7 @@ class LabelsFinder < UnionFinder private - attr_reader :current_user, :params, :authorized_only + attr_reader :current_user, :params, :skip_authorization def label_ids label_ids = [] @@ -70,17 +69,17 @@ class LabelsFinder < UnionFinder end def find_project - if authorized_only - available_projects.find_by(id: project_id) - else + if skip_authorization Project.find_by(id: project_id) + else + available_projects.find_by(id: project_id) end end def projects return @projects if defined?(@projects) - @projects = authorized_only ? available_projects : Project.all + @projects = skip_authorization ? Project.all : available_projects @projects = @projects.in_namespace(group_id) if group_id @projects = @projects.where(id: projects_ids) if projects_ids @projects = @projects.reorder(nil) diff --git a/app/models/project.rb b/app/models/project.rb index fbf7012972e..ae57815c620 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -738,7 +738,7 @@ class Project < ActiveRecord::Base def create_labels Label.templates.each do |label| params = label.attributes.except('id', 'template', 'created_at', 'updated_at') - Labels::FindOrCreateService.new(owner, self, params).execute + Labels::FindOrCreateService.new(nil, self, params).execute(skip_authorization: true) end end diff --git a/app/services/labels/find_or_create_service.rb b/app/services/labels/find_or_create_service.rb index 74291312c4e..d622f9edd33 100644 --- a/app/services/labels/find_or_create_service.rb +++ b/app/services/labels/find_or_create_service.rb @@ -2,21 +2,24 @@ module Labels class FindOrCreateService def initialize(current_user, project, params = {}) @current_user = current_user - @group = project.group @project = project @params = params.dup end - def execute + def execute(skip_authorization: false) + @skip_authorization = skip_authorization find_or_create_label end private - attr_reader :current_user, :group, :project, :params + attr_reader :current_user, :project, :params, :skip_authorization def available_labels - @available_labels ||= LabelsFinder.new(current_user, project_id: project.id).execute + @available_labels ||= LabelsFinder.new( + current_user, + project_id: project.id + ).execute(skip_authorization: skip_authorization) end def find_or_create_label diff --git a/lib/banzai/filter/label_reference_filter.rb b/lib/banzai/filter/label_reference_filter.rb index c24831e68ee..9f9a96cdc65 100644 --- a/lib/banzai/filter/label_reference_filter.rb +++ b/lib/banzai/filter/label_reference_filter.rb @@ -39,7 +39,7 @@ module Banzai end def find_labels(project) - LabelsFinder.new(nil, project_id: project.id).execute(authorized_only: false) + LabelsFinder.new(nil, project_id: project.id).execute(skip_authorization: true) end # Parameters to pass to `Label.find_by` based on the given arguments diff --git a/lib/gitlab/fogbugz_import/importer.rb b/lib/gitlab/fogbugz_import/importer.rb index 65ee85ca5a9..222bcdcbf9c 100644 --- a/lib/gitlab/fogbugz_import/importer.rb +++ b/lib/gitlab/fogbugz_import/importer.rb @@ -75,7 +75,7 @@ module Gitlab def create_label(name) params = { title: name, color: nice_label_color(name) } - ::Labels::FindOrCreateService.new(project.owner, project, params).execute + ::Labels::FindOrCreateService.new(nil, project, params).execute(skip_authorization: true) end def user_info(person_id) @@ -133,7 +133,7 @@ module Gitlab updated_at: DateTime.parse(bug['dtLastUpdated']) ) - issue_labels = ::LabelsFinder.new(project.owner, project_id: project.id, title: labels).execute + issue_labels = ::LabelsFinder.new(nil, project_id: project.id, title: labels).execute(skip_authorization: true) issue.update_attribute(:label_ids, issue_labels.pluck(:id)) import_issue_comments(issue, comments) diff --git a/lib/gitlab/github_import/label_formatter.rb b/lib/gitlab/github_import/label_formatter.rb index 942dfb3312b..e572bb338ce 100644 --- a/lib/gitlab/github_import/label_formatter.rb +++ b/lib/gitlab/github_import/label_formatter.rb @@ -15,8 +15,8 @@ module Gitlab def create! params = attributes.except(:project) - service = ::Labels::FindOrCreateService.new(project.owner, project, params) - label = service.execute + service = ::Labels::FindOrCreateService.new(nil, project, params) + label = service.execute(skip_authorization: true) raise ActiveRecord::RecordInvalid.new(label) unless label.persisted? diff --git a/lib/gitlab/google_code_import/importer.rb b/lib/gitlab/google_code_import/importer.rb index 6a68e786b4f..1f4edc36928 100644 --- a/lib/gitlab/google_code_import/importer.rb +++ b/lib/gitlab/google_code_import/importer.rb @@ -101,7 +101,7 @@ module Gitlab state: raw_issue['state'] == 'closed' ? 'closed' : 'opened' ) - issue_labels = ::LabelsFinder.new(project.owner, project_id: project.id, title: labels).execute + issue_labels = ::LabelsFinder.new(nil, project_id: project.id, title: labels).execute(skip_authorization: true) issue.update_attribute(:label_ids, issue_labels.pluck(:id)) import_issue_comments(issue, comments) @@ -235,7 +235,7 @@ module Gitlab def create_label(name) params = { name: name, color: nice_label_color(name) } - ::Labels::FindOrCreateService.new(project.owner, project, params).execute + ::Labels::FindOrCreateService.new(nil, project, params).execute(skip_authorization: true) end def format_content(raw_content) diff --git a/lib/gitlab/issues_labels.rb b/lib/gitlab/issues_labels.rb index dbc759367eb..b8ca7f2f55f 100644 --- a/lib/gitlab/issues_labels.rb +++ b/lib/gitlab/issues_labels.rb @@ -19,7 +19,7 @@ module Gitlab ] labels.each do |params| - ::Labels::FindOrCreateService.new(project.owner, project, params).execute + ::Labels::FindOrCreateService.new(nil, project, params).execute(skip_authorization: true) end end end diff --git a/spec/controllers/projects/labels_controller_spec.rb b/spec/controllers/projects/labels_controller_spec.rb index 41df63d445a..8faecec0063 100644 --- a/spec/controllers/projects/labels_controller_spec.rb +++ b/spec/controllers/projects/labels_controller_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Projects::LabelsController do let(:group) { create(:group) } - let(:project) { create(:project, namespace: group) } + let(:project) { create(:empty_project, namespace: group) } let(:user) { create(:user) } before do @@ -73,16 +73,27 @@ describe Projects::LabelsController do describe 'POST #generate' do let(:admin) { create(:admin) } - let(:project) { create(:empty_project) } before do sign_in(admin) end - it 'creates labels' do - post :generate, namespace_id: project.namespace.to_param, project_id: project.to_param + context 'personal project' do + let(:personal_project) { create(:empty_project) } - expect(response).to have_http_status(302) + it 'creates labels' do + post :generate, namespace_id: personal_project.namespace.to_param, project_id: personal_project.to_param + + expect(response).to have_http_status(302) + end + end + + context 'project belonging to a group' do + it 'creates labels' do + post :generate, namespace_id: project.namespace.to_param, project_id: project.to_param + + expect(response).to have_http_status(302) + end end end end diff --git a/spec/services/labels/find_or_create_service_spec.rb b/spec/services/labels/find_or_create_service_spec.rb index cbfc63de811..7a9b34f9f96 100644 --- a/spec/services/labels/find_or_create_service_spec.rb +++ b/spec/services/labels/find_or_create_service_spec.rb @@ -2,7 +2,6 @@ require 'spec_helper' describe Labels::FindOrCreateService, services: true do describe '#execute' do - let(:user) { create(:user) } let(:group) { create(:group) } let(:project) { create(:project, namespace: group) } @@ -14,37 +13,49 @@ describe Labels::FindOrCreateService, services: true do } end - subject(:service) { described_class.new(user, project, params) } + context 'when acting on behalf of a specific user' do + let(:user) { create(:user) } + subject(:service) { described_class.new(user, project, params) } + before do + project.team << [user, :developer] + end - before do - project.team << [user, :developer] - end + context 'when label does not exist at group level' do + it 'creates a new label at project level' do + expect { service.execute }.to change(project.labels, :count).by(1) + end + end - context 'when label does not exist at group level' do - it 'creates a new label at project level' do - expect { service.execute }.to change(project.labels, :count).by(1) + context 'when label exists at group level' do + it 'returns the group label' do + group_label = create(:group_label, group: group, title: 'Security') + + expect(service.execute).to eq group_label + end + end + + context 'when label does not exist at group level' do + it 'creates a new label at project leve' do + expect { service.execute }.to change(project.labels, :count).by(1) + end + end + + context 'when label exists at project level' do + it 'returns the project label' do + project_label = create(:label, project: project, title: 'Security') + + expect(service.execute).to eq project_label + end end end - context 'when label exists at group level' do - it 'returns the group label' do - group_label = create(:group_label, group: group, title: 'Security') + context 'when authorization is not required' do + subject(:service) { described_class.new(nil, project, params) } - expect(service.execute).to eq group_label - end - end - - context 'when label does not exist at group level' do - it 'creates a new label at project leve' do - expect { service.execute }.to change(project.labels, :count).by(1) - end - end - - context 'when label exists at project level' do it 'returns the project label' do project_label = create(:label, project: project, title: 'Security') - expect(service.execute).to eq project_label + expect(service.execute(skip_authorization: true)).to eq project_label end end end