Merge branch '17690-Protect-TeamCity-builds-for-triggering-when-a-branch-is-deleted-And-add-MR-option' into 'master'

Skip TeamCity trigger on branch delete and support MR triggers

Closes #13871 and #17690

See merge request gitlab-org/gitlab-ce!29836

6 files changed, 194 insertions, 44 deletions

diff --git a/app/models/concerns/service_push_data_validations.rb b/app/models/concerns/service_push_data_validations.rb
new file mode 100644
index 00000000000..defc5794142
--- /dev/null
+++ b/app/models/concerns/service_push_data_validations.rb
@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+# This concern is used by registerd services such as TeamCityService and
+# DroneCiService and add methods to perform validations on the received
+# data.
+
+module ServicePushDataValidations
+  extend ActiveSupport::Concern
+
+  def merge_request_valid?(data)
+    data.dig(:object_attributes, :state) == 'opened' && merge_request_unchecked?(data)
+  end
+
+  def push_valid?(data)
+    data[:total_commits_count] > 0 &&
+      !branch_removed?(data) &&
+      # prefer merge request trigger over push to avoid double builds
+      !opened_merge_requests?(data)
+  end
+
+  def tag_push_valid?(data)
+    data[:total_commits_count] > 0 && !branch_removed?(data)
+  end
+
+  private
+
+  def branch_removed?(data)
+    Gitlab::Git.blank_ref?(data[:after])
+  end
+
+  def opened_merge_requests?(data)
+    project.merge_requests
+      .opened
+      .from_project(project)
+      .from_source_branches(Gitlab::Git.ref_name(data[:ref]))
+      .exists?
+  end
+
+  def merge_request_unchecked?(data)
+    MergeRequest.state_machines[:merge_status]
+      .check_state?(data.dig(:object_attributes, :merge_status))
+  end
+end
diff --git a/app/models/project_services/drone_ci_service.rb b/app/models/project_services/drone_ci_service.rb
index 5ccc2f019cb..dbdc8345c93 100644
--- a/app/models/project_services/drone_ci_service.rb
+++ b/app/models/project_services/drone_ci_service.rb
@@ -2,6 +2,7 @@
 
 class DroneCiService < CiService
   include ReactiveService
+  include ServicePushDataValidations
 
   prop_accessor :drone_url, :token
   boolean_accessor :enable_ssl_verification
@@ -96,23 +97,4 @@ class DroneCiService < CiService
       { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" }
     ]
   end
-
-  private
-
-  def tag_push_valid?(data)
-    data[:total_commits_count] > 0 && !Gitlab::Git.blank_ref?(data[:after])
-  end
-
-  def push_valid?(data)
-    opened_merge_requests = project.merge_requests.opened.where(source_project_id: project.id,
-                                                                source_branch: Gitlab::Git.ref_name(data[:ref]))
-
-    opened_merge_requests.empty? && data[:total_commits_count] > 0 &&
-      !Gitlab::Git.blank_ref?(data[:after])
-  end
-
-  def merge_request_valid?(data)
-    data[:object_attributes][:state] == 'opened' &&
-      MergeRequest.state_machines[:merge_status].check_state?(data[:object_attributes][:merge_status])
-  end
 end
diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb
index 3245cd22e73..68c07fa37f2 100644
--- a/app/models/project_services/teamcity_service.rb
+++ b/app/models/project_services/teamcity_service.rb
@@ -2,6 +2,7 @@
 
 class TeamcityService < CiService
   include ReactiveService
+  include ServicePushDataValidations
 
   prop_accessor :teamcity_url, :build_type, :username, :password
 
@@ -19,6 +20,25 @@ class TeamcityService < CiService
   after_save :compose_service_hook, if: :activated?
   before_update :reset_password
 
+  class << self
+    def to_param
+      'teamcity'
+    end
+
+    def supported_events
+      %w(push merge_request)
+    end
+
+    def event_description(event)
+      case event
+      when 'push', 'push_events'
+        'TeamCity CI will be triggered after every push to the repository except branch delete'
+      when 'merge_request', 'merge_request_events'
+        'TeamCity CI will be triggered after a merge request has been created or updated'
+      end
+    end
+  end
+
   def compose_service_hook
     hook = service_hook || build_service_hook
     hook.save
@@ -43,10 +63,6 @@ class TeamcityService < CiService
     'requests build, that setting is in the vsc root advanced settings.'
   end
 
-  def self.to_param
-    'teamcity'
-  end
-
   def fields
     [
       { type: 'text', name: 'teamcity_url',
@@ -74,26 +90,25 @@ class TeamcityService < CiService
   end
 
   def execute(data)
-    return unless supported_events.include?(data[:object_kind])
+    case data[:object_kind]
+    when 'push'
+      execute_push(data)
+    when 'merge_request'
+      execute_merge_request(data)
+    end
+  end
 
-    auth = {
-      username: username,
-      password: password
-    }
+  private
 
+  def execute_push(data)
     branch = Gitlab::Git.ref_name(data[:ref])
-
-    Gitlab::HTTP.post(
-      build_url('httpAuth/app/rest/buildQueue'),
-      body: "<build branchName=\"#{branch}\">"\
-            "<buildType id=\"#{build_type}\"/>"\
-            '</build>',
-      headers: { 'Content-type' => 'application/xml' },
-      basic_auth: auth
-    )
+    post_to_build_queue(data, branch) if push_valid?(data)
   end
 
-  private
+  def execute_merge_request(data)
+    branch = data[:object_attributes][:source_branch]
+    post_to_build_queue(data, branch) if merge_request_valid?(data)
+  end
 
   def read_build_page(response)
     if response.code != 200
@@ -134,10 +149,21 @@ class TeamcityService < CiService
   end
 
   def get_path(path)
-    Gitlab::HTTP.get(build_url(path), verify: false,
-                                      basic_auth: {
-                                        username: username,
-                                        password: password
-                                      })
+    Gitlab::HTTP.get(build_url(path), verify: false, basic_auth: basic_auth)
+  end
+
+  def post_to_build_queue(data, branch)
+    Gitlab::HTTP.post(
+      build_url('httpAuth/app/rest/buildQueue'),
+      body: "<build branchName=#{branch.encode(xml: :attr)}>"\
+            "<buildType id=#{build_type.encode(xml: :attr)}/>"\
+            '</build>',
+      headers: { 'Content-type' => 'application/xml' },
+      basic_auth: basic_auth
+    )
+  end
+
+  def basic_auth
+    { username: username, password: password }
   end
 end
diff --git a/changelogs/unreleased/17690-Protect-TeamCity-builds-for-triggering-when-a-branch-is-deleted-And-add-MR-option.yml b/changelogs/unreleased/17690-Protect-TeamCity-builds-for-triggering-when-a-branch-is-deleted-And-add-MR-option.yml
new file mode 100644
index 00000000000..741a0faf469
--- /dev/null
+++ b/changelogs/unreleased/17690-Protect-TeamCity-builds-for-triggering-when-a-branch-is-deleted-And-add-MR-option.yml
@@ -0,0 +1,5 @@
+---
+title: Protect TeamCity builds from triggering when a branch has been deleted. And a MR-option
+merge_request: 29836
+author: Nikolay Novikov, Raphael Tweitmann
+type: fixed
diff --git a/spec/features/projects/services/user_activates_jetbrains_teamcity_ci_spec.rb b/spec/features/projects/services/user_activates_jetbrains_teamcity_ci_spec.rb
index 28d83a8b961..c50fd93e4cb 100644
--- a/spec/features/projects/services/user_activates_jetbrains_teamcity_ci_spec.rb
+++ b/spec/features/projects/services/user_activates_jetbrains_teamcity_ci_spec.rb
@@ -15,6 +15,8 @@ describe 'User activates JetBrains TeamCity CI' do
 
   it 'activates service' do
     check('Active')
+    check('Push')
+    check('Merge request')
     fill_in('Teamcity url', with: 'http://teamcity.example.com')
     fill_in('Build type', with: 'GitlabTest_Build')
     fill_in('Username', with: 'user')
diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb
index 1c434b25205..3d875bc49e7 100644
--- a/spec/models/project_services/teamcity_service_spec.rb
+++ b/spec/models/project_services/teamcity_service_spec.rb
@@ -7,10 +7,11 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
   include StubRequests
 
   let(:teamcity_url) { 'http://gitlab.com/teamcity' }
+  let(:project) { create(:project) }
 
   subject(:service) do
     described_class.create(
-      project: create(:project),
+      project: project,
       properties: {
         teamcity_url: teamcity_url,
         username: 'mic',
@@ -207,6 +208,97 @@ describe TeamcityService, :use_clean_rails_memory_store_caching do
     end
   end
 
+  describe '#execute' do
+    context 'when push' do
+      let(:data) do
+        {
+          object_kind: 'push',
+          ref: 'refs/heads/dev-123_branch',
+          after: '0220c11b9a3e6c69dc8fd35321254ca9a7b98f7e',
+          total_commits_count: 1
+        }
+      end
+
+      it 'handles push request correctly' do
+        stub_post_to_build_queue(branch: 'dev-123_branch')
+
+        expect(service.execute(data)).to include('Ok')
+      end
+
+      it 'returns nil when ref is blank' do
+        data[:after] = Gitlab::Git::BLANK_SHA
+
+        expect(service.execute(data)).to be_nil
+      end
+
+      it 'returns nil when there is no content' do
+        data[:total_commits_count] = 0
+
+        expect(service.execute(data)).to be_nil
+      end
+
+      it 'returns nil when a merge request is opened for the same ref' do
+        create(:merge_request, source_project: project, source_branch: 'dev-123_branch')
+
+        expect(service.execute(data)).to be_nil
+      end
+    end
+
+    context 'when merge_request' do
+      let(:data) do
+        {
+          object_kind: 'merge_request',
+          ref: 'refs/heads/dev-123_branch',
+          after: '0220c11b9a3e6c69dc8fd35321254ca9a7b98f7e',
+          total_commits_count: 1,
+          object_attributes: {
+            state: 'opened',
+            source_branch: 'dev-123_branch',
+            merge_status: 'unchecked'
+          }
+        }
+      end
+
+      it 'handles merge request correctly' do
+        stub_post_to_build_queue(branch: 'dev-123_branch')
+
+        expect(service.execute(data)).to include('Ok')
+      end
+
+      it 'returns nil when merge request is not opened' do
+        data[:object_attributes][:state] = 'closed'
+
+        expect(service.execute(data)).to be_nil
+      end
+
+      it 'returns nil unless merge request is marked as unchecked' do
+        data[:object_attributes][:merge_status] = 'can_be_merged'
+
+        expect(service.execute(data)).to be_nil
+      end
+    end
+
+    it 'returns nil when event is not supported' do
+      data = { object_kind: 'foo' }
+
+      expect(service.execute(data)).to be_nil
+    end
+  end
+
+  def stub_post_to_build_queue(branch:)
+    teamcity_full_url = 'http://gitlab.com/teamcity/httpAuth/app/rest/buildQueue'
+    body ||= %Q(<build branchName=\"#{branch}\"><buildType id=\"foo\"/></build>)
+    auth = %w(mic password)
+
+    stub_full_request(teamcity_full_url, method: :post).with(
+      basic_auth: auth,
+      body: body,
+      headers: {
+        'Content-Type' => 'application/xml'
+      }
+    ).to_return(status: 200, body: 'Ok', headers: {})
+  end
+
   def stub_request(status: 200, body: nil, build_status: 'success')
     teamcity_full_url = 'http://gitlab.com/teamcity/httpAuth/app/rest/builds/branch:unspecified:any,revision:123'
     auth = %w(mic password)