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

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

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`.

Fixes #23694

See merge request !7093
Signed-off-by: Rémy Coutable <remy@rymai.me>

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)

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
 

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

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

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)

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?
 

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)

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

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

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