summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAdam Niedzielski <adamsunday@gmail.com>2016-10-28 11:31:45 +0200
committerAdam Niedzielski <adamsunday@gmail.com>2016-10-28 11:31:45 +0200
commite2c603696a9647c15cd154156f13d0e203a930f1 (patch)
tree22f64b82526f74fabb9009db8f6932c4e8d74d12
parent20a7db4483904c7280093a0309a63dfd1b7ef72e (diff)
downloadgitlab-ce-e2c603696a9647c15cd154156f13d0e203a930f1.tar.gz
Pass user instance to Labels::FindOrCreateService or skip_authorization: trueadam-fix-labels-find-or-create
Do not pass project.owner because it may return a group and Labels::FindOrCreateService throws an error in this case. Fixes #23694.
-rw-r--r--app/finders/labels_finder.rb15
-rw-r--r--app/models/project.rb2
-rw-r--r--app/services/labels/find_or_create_service.rb11
-rw-r--r--lib/banzai/filter/label_reference_filter.rb2
-rw-r--r--lib/gitlab/fogbugz_import/importer.rb4
-rw-r--r--lib/gitlab/github_import/label_formatter.rb4
-rw-r--r--lib/gitlab/google_code_import/importer.rb4
-rw-r--r--lib/gitlab/issues_labels.rb2
-rw-r--r--spec/controllers/projects/labels_controller_spec.rb21
-rw-r--r--spec/services/labels/find_or_create_service_spec.rb51
10 files changed, 70 insertions, 46 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) }
-
- before do
- project.team << [user, :developer]
- end
+ 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
- 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 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
- 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 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
+ expect(service.execute).to eq group_label
+ end
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)
+ 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 project level' do
+ context 'when authorization is not required' do
+ subject(:service) { described_class.new(nil, project, params) }
+
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