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.
This commit is contained in:
parent
20a7db4483
commit
e2c603696a
10 changed files with 73 additions and 49 deletions
|
@ -4,9 +4,8 @@ class LabelsFinder < UnionFinder
|
||||||
@params = params
|
@params = params
|
||||||
end
|
end
|
||||||
|
|
||||||
def execute(authorized_only: true)
|
def execute(skip_authorization: false)
|
||||||
@authorized_only = authorized_only
|
@skip_authorization = skip_authorization
|
||||||
|
|
||||||
items = find_union(label_ids, Label)
|
items = find_union(label_ids, Label)
|
||||||
items = with_title(items)
|
items = with_title(items)
|
||||||
sort(items)
|
sort(items)
|
||||||
|
@ -14,7 +13,7 @@ class LabelsFinder < UnionFinder
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
attr_reader :current_user, :params, :authorized_only
|
attr_reader :current_user, :params, :skip_authorization
|
||||||
|
|
||||||
def label_ids
|
def label_ids
|
||||||
label_ids = []
|
label_ids = []
|
||||||
|
@ -70,17 +69,17 @@ class LabelsFinder < UnionFinder
|
||||||
end
|
end
|
||||||
|
|
||||||
def find_project
|
def find_project
|
||||||
if authorized_only
|
if skip_authorization
|
||||||
available_projects.find_by(id: project_id)
|
|
||||||
else
|
|
||||||
Project.find_by(id: project_id)
|
Project.find_by(id: project_id)
|
||||||
|
else
|
||||||
|
available_projects.find_by(id: project_id)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def projects
|
def projects
|
||||||
return @projects if defined?(@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.in_namespace(group_id) if group_id
|
||||||
@projects = @projects.where(id: projects_ids) if projects_ids
|
@projects = @projects.where(id: projects_ids) if projects_ids
|
||||||
@projects = @projects.reorder(nil)
|
@projects = @projects.reorder(nil)
|
||||||
|
|
|
@ -738,7 +738,7 @@ class Project < ActiveRecord::Base
|
||||||
def create_labels
|
def create_labels
|
||||||
Label.templates.each do |label|
|
Label.templates.each do |label|
|
||||||
params = label.attributes.except('id', 'template', 'created_at', 'updated_at')
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -2,21 +2,24 @@ module Labels
|
||||||
class FindOrCreateService
|
class FindOrCreateService
|
||||||
def initialize(current_user, project, params = {})
|
def initialize(current_user, project, params = {})
|
||||||
@current_user = current_user
|
@current_user = current_user
|
||||||
@group = project.group
|
|
||||||
@project = project
|
@project = project
|
||||||
@params = params.dup
|
@params = params.dup
|
||||||
end
|
end
|
||||||
|
|
||||||
def execute
|
def execute(skip_authorization: false)
|
||||||
|
@skip_authorization = skip_authorization
|
||||||
find_or_create_label
|
find_or_create_label
|
||||||
end
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
|
||||||
attr_reader :current_user, :group, :project, :params
|
attr_reader :current_user, :project, :params, :skip_authorization
|
||||||
|
|
||||||
def available_labels
|
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
|
end
|
||||||
|
|
||||||
def find_or_create_label
|
def find_or_create_label
|
||||||
|
|
|
@ -39,7 +39,7 @@ module Banzai
|
||||||
end
|
end
|
||||||
|
|
||||||
def find_labels(project)
|
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
|
end
|
||||||
|
|
||||||
# Parameters to pass to `Label.find_by` based on the given arguments
|
# Parameters to pass to `Label.find_by` based on the given arguments
|
||||||
|
|
|
@ -75,7 +75,7 @@ module Gitlab
|
||||||
|
|
||||||
def create_label(name)
|
def create_label(name)
|
||||||
params = { title: name, color: nice_label_color(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
|
end
|
||||||
|
|
||||||
def user_info(person_id)
|
def user_info(person_id)
|
||||||
|
@ -133,7 +133,7 @@ module Gitlab
|
||||||
updated_at: DateTime.parse(bug['dtLastUpdated'])
|
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))
|
issue.update_attribute(:label_ids, issue_labels.pluck(:id))
|
||||||
|
|
||||||
import_issue_comments(issue, comments)
|
import_issue_comments(issue, comments)
|
||||||
|
|
|
@ -15,8 +15,8 @@ module Gitlab
|
||||||
|
|
||||||
def create!
|
def create!
|
||||||
params = attributes.except(:project)
|
params = attributes.except(:project)
|
||||||
service = ::Labels::FindOrCreateService.new(project.owner, project, params)
|
service = ::Labels::FindOrCreateService.new(nil, project, params)
|
||||||
label = service.execute
|
label = service.execute(skip_authorization: true)
|
||||||
|
|
||||||
raise ActiveRecord::RecordInvalid.new(label) unless label.persisted?
|
raise ActiveRecord::RecordInvalid.new(label) unless label.persisted?
|
||||||
|
|
||||||
|
|
|
@ -101,7 +101,7 @@ module Gitlab
|
||||||
state: raw_issue['state'] == 'closed' ? 'closed' : 'opened'
|
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))
|
issue.update_attribute(:label_ids, issue_labels.pluck(:id))
|
||||||
|
|
||||||
import_issue_comments(issue, comments)
|
import_issue_comments(issue, comments)
|
||||||
|
@ -235,7 +235,7 @@ module Gitlab
|
||||||
|
|
||||||
def create_label(name)
|
def create_label(name)
|
||||||
params = { name: name, color: nice_label_color(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
|
end
|
||||||
|
|
||||||
def format_content(raw_content)
|
def format_content(raw_content)
|
||||||
|
|
|
@ -19,7 +19,7 @@ module Gitlab
|
||||||
]
|
]
|
||||||
|
|
||||||
labels.each do |params|
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -2,7 +2,7 @@ require 'spec_helper'
|
||||||
|
|
||||||
describe Projects::LabelsController do
|
describe Projects::LabelsController do
|
||||||
let(:group) { create(:group) }
|
let(:group) { create(:group) }
|
||||||
let(:project) { create(:project, namespace: group) }
|
let(:project) { create(:empty_project, namespace: group) }
|
||||||
let(:user) { create(:user) }
|
let(:user) { create(:user) }
|
||||||
|
|
||||||
before do
|
before do
|
||||||
|
@ -73,16 +73,27 @@ describe Projects::LabelsController do
|
||||||
|
|
||||||
describe 'POST #generate' do
|
describe 'POST #generate' do
|
||||||
let(:admin) { create(:admin) }
|
let(:admin) { create(:admin) }
|
||||||
let(:project) { create(:empty_project) }
|
|
||||||
|
|
||||||
before do
|
before do
|
||||||
sign_in(admin)
|
sign_in(admin)
|
||||||
end
|
end
|
||||||
|
|
||||||
it 'creates labels' do
|
context 'personal project' do
|
||||||
post :generate, namespace_id: project.namespace.to_param, project_id: project.to_param
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
|
@ -2,7 +2,6 @@ require 'spec_helper'
|
||||||
|
|
||||||
describe Labels::FindOrCreateService, services: true do
|
describe Labels::FindOrCreateService, services: true do
|
||||||
describe '#execute' do
|
describe '#execute' do
|
||||||
let(:user) { create(:user) }
|
|
||||||
let(:group) { create(:group) }
|
let(:group) { create(:group) }
|
||||||
let(:project) { create(:project, namespace: group) }
|
let(:project) { create(:project, namespace: group) }
|
||||||
|
|
||||||
|
@ -14,37 +13,49 @@ describe Labels::FindOrCreateService, services: true do
|
||||||
}
|
}
|
||||||
end
|
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
|
context 'when label does not exist at group level' do
|
||||||
project.team << [user, :developer]
|
it 'creates a new label at project level' do
|
||||||
end
|
expect { service.execute }.to change(project.labels, :count).by(1)
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
context 'when label does not exist at group level' do
|
context 'when label exists at group level' do
|
||||||
it 'creates a new label at project level' do
|
it 'returns the group label' do
|
||||||
expect { service.execute }.to change(project.labels, :count).by(1)
|
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
|
||||||
end
|
end
|
||||||
|
|
||||||
context 'when label exists at group level' do
|
context 'when authorization is not required' do
|
||||||
it 'returns the group label' do
|
subject(:service) { described_class.new(nil, project, params) }
|
||||||
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
|
it 'returns the project label' do
|
||||||
project_label = create(:label, project: project, title: 'Security')
|
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
|
end
|
||||||
end
|
end
|
||||||
|
|
Loading…
Reference in a new issue