Commit 44cbfeab authored by Douwe Maan's avatar Douwe Maan

Merge branch 'adam-fix-labels-find-or-create' into 'master'

Pass user instance to Labels::FindOrCreateService or skip_authorization: true

## What does this MR do?

It fixes a bug described in #23694 when `project.owner` was passed to `Labels::FindOrCreateService`. `Labels::FindOrCreateService` expected a user instance and `project.owner` may return a group as well. This MR makes sure that we either pass a user instance or `skip_authorization: true`.

## Are there points in the code the reviewer needs to double check?

- places where we pass `skip_authorization: true`

## Does this MR meet the acceptance criteria?

- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [ ] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Fixes #23694

See merge request !7093
parents d306b0d7 e2c60369
...@@ -4,9 +4,8 @@ class LabelsFinder < UnionFinder ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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 ...@@ -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' ...@@ -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 ...@@ -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' ...@@ -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 ...@@ -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) }
before do subject(:service) { described_class.new(user, project, params) }
project.team << [user, :developer] before do
end project.team << [user, :developer]
end
context 'when label does not exist at group level' do context 'when label does not exist at group level' do
it 'creates a new label at project level' do it 'creates a new label at project level' do
expect { service.execute }.to change(project.labels, :count).by(1) expect { service.execute }.to change(project.labels, :count).by(1)
end
end end
end
context 'when label exists at group level' do context 'when label exists at group level' do
it 'returns the group label' do it 'returns the group label' do
group_label = create(:group_label, group: group, title: 'Security') 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
end
context 'when label does not exist at group level' do context 'when label does not exist at group level' do
it 'creates a new label at project leve' do it 'creates a new label at project leve' do
expect { service.execute }.to change(project.labels, :count).by(1) 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 project level' do context 'when authorization is not required' do
subject(:service) { described_class.new(nil, project, params) }
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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment