Merge branch 'bentolor/gitlab-ce-fix/bamboo-service-trigger-auth' into 'master'

Bamboo & TeamCity Services: Fix missing credentials & URL handling

_Note: Originally opened at !4367 by @bentolor_

I've also fixed the URL handling for TeamCity which is very similar to Bamboo implementation-wise.

-----

*Note:* This is a port from my [original pull request on GitHub](https://github.com/gitlabhq/gitlabhq/pull/9428)

## What does this MR do?
This improves the Bamboo Service and provides two fixes:

1. One for the situation, where the build trigger won't work because Bamboo is requiring authentication credentials for the trigger GET: 8f25aca307b49ee006172b8c2985a878800aa6b6
2. One which fixes the way how the configured Bamboo base URL is assembled to the final REST URL. fe9eb30d7ebe4a83eefea7e06f8b69b135dad15d

### Regarding credentials
The change now does provide additional HTTP Basic Auth parameters if user credentials were provided and appends an request parameter indicating the HTTP Basic Authentication should be used. This aligns interaction with Bamboo with the other calls this service executes.

### Regarding URL handling
If one had configured a `bamboo_url` like http://foo.bar/bamboo in the previous implementation the plugin directed it's request i.e. to http://foo.bar/rest/... instead of http://foo.bar/bamboo/rest/...


## Are there points in the code the reviewer needs to double check?
The second issues was probably an unwanted side effect of how Ruby's `URI.join` is working. It will only work correctly, if 
- ... the prefix URL has at least one or more  trailing `/`
- .. the appendix parts are _not_ prefixed with `/`

I need try & figure it out using the rather lacking, official stdlib documentation and playing around in `irb`. As I'm an absolute Ruby novice I'm unable to add/provide new tests.

## Why was this MR needed?
Because Gitlab does not work in our Bamboo-Environment at all: Neither it is able to trigger Bamboo runs nor does the Merge status check work. This MR at least fixes the trigger issues.

## What are the relevant issue numbers?
This MR originates from my [original pull request on GitHub](https://github.com/gitlabhq/gitlabhq/pull/9428).
Sadly the issue, that the merge status is still not working correctly for branches will still not work. But at least the trigger works. 

There happened to be very much discussion about the branch status issue in #1355 and  #2562 though that one is lost as the author retracted his branch. 

See merge request !4408

5 files changed, 57 insertions, 48 deletions

diff --git a/CHANGELOG b/CHANGELOG
index a6fc7aff95e..70dd9dd70e1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -39,6 +39,8 @@ v 8.9.0 (unreleased)
   - Add option to project to only allow merge requests to be merged if the build succeeds (Rui Santos)
   - Fix issues filter when ordering by milestone
   - Added artifacts:when to .gitlab-ci.yml - this requires GitLab Runner 1.3
+  - Bamboo Service: Fix missing credentials & URL handling when base URL contains a path (Benjamin Schmid)
+  - TeamCity Service: Fix URL handling when base URL contains a path
   - Todos will display target state if issuable target is 'Closed' or 'Merged'
   - Fix bug when sorting issues by milestone due date and filtering by two or more labels
   - Add support for using Yubikeys (U2F) for two-factor authentication
diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb
index 1d1780dcfbf..b5c76e4d4fe 100644
--- a/app/models/project_services/bamboo_service.rb
+++ b/app/models/project_services/bamboo_service.rb
@@ -1,6 +1,4 @@
 class BambooService < CiService
-  include HTTParty
-
   prop_accessor :bamboo_url, :build_key, :username, :password
 
   validates :bamboo_url, presence: true, url: true, if: :activated?
@@ -61,18 +59,7 @@ class BambooService < CiService
   end
 
   def build_info(sha)
-    url = URI.join(bamboo_url, "/rest/api/latest/result?label=#{sha}").to_s
-
-    if username.blank? && password.blank?
-      @response = HTTParty.get(url, verify: false)
-    else
-      url << '&os_authType=basic'
-      auth = {
-        username: username,
-        password: password
-      }
-      @response = HTTParty.get(url, verify: false, basic_auth: auth)
-    end
+    @response = get_path("rest/api/latest/result?label=#{sha}")
   end
 
   def build_page(sha, ref)
@@ -80,11 +67,11 @@ class BambooService < CiService
 
     if @response.code != 200 || @response['results']['results']['size'] == '0'
       # If actual build link can't be determined, send user to build summary page.
-      URI.join(bamboo_url, "/browse/#{build_key}").to_s
+      URI.join("#{bamboo_url}/", "browse/#{build_key}").to_s
     else
       # If actual build link is available, go to build result page.
       result_key = @response['results']['results']['result']['planResultKey']['key']
-      URI.join(bamboo_url, "/browse/#{result_key}").to_s
+      URI.join("#{bamboo_url}/", "browse/#{result_key}").to_s
     end
   end
 
@@ -112,8 +99,27 @@ class BambooService < CiService
   def execute(data)
     return unless supported_events.include?(data[:object_kind])
 
-    # Bamboo requires a GET and does not take any data.
-    url = URI.join(bamboo_url, "/updateAndBuild.action?buildKey=#{build_key}").to_s
-    self.class.get(url, verify: false)
+    get_path("updateAndBuild.action?buildKey=#{build_key}")
+  end
+
+  private
+
+  def build_url(path)
+    URI.join("#{bamboo_url}/", path).to_s
+  end
+
+  def get_path(path)
+    url = build_url(path)
+
+    if username.blank? && password.blank?
+      HTTParty.get(url, verify: false)
+    else
+      url << '&os_authType=basic'
+      HTTParty.get(url, verify: false,
+                        basic_auth: {
+                          username: username,
+                          password: password
+                        })
+    end
   end
 end
diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb
index b0dcb52eba1..a4a967c9bc9 100644
--- a/app/models/project_services/teamcity_service.rb
+++ b/app/models/project_services/teamcity_service.rb
@@ -1,6 +1,4 @@
 class TeamcityService < CiService
-  include HTTParty
-
   prop_accessor :teamcity_url, :build_type, :username, :password
 
   validates :teamcity_url, presence: true, url: true, if: :activated?
@@ -64,15 +62,7 @@ class TeamcityService < CiService
   end
 
   def build_info(sha)
-    url = URI.join(
-      teamcity_url,
-      "/httpAuth/app/rest/builds/branch:unspecified:any,number:#{sha}"
-    ).to_s
-    auth = {
-      username: username,
-      password: password
-    }
-    @response = HTTParty.get(url, verify: false, basic_auth: auth)
+    @response = get_path("httpAuth/app/rest/builds/branch:unspecified:any,number:#{sha}")
   end
 
   def build_page(sha, ref)
@@ -81,14 +71,11 @@ class TeamcityService < CiService
     if @response.code != 200
       # If actual build link can't be determined,
       # send user to build summary page.
-      URI.join(teamcity_url, "/viewLog.html?buildTypeId=#{build_type}").to_s
+      build_url("viewLog.html?buildTypeId=#{build_type}")
     else
       # If actual build link is available, go to build result page.
       built_id = @response['build']['id']
-      URI.join(
-        teamcity_url,
-        "/viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}"
-      ).to_s
+      build_url("viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}")
     end
   end
 
@@ -123,8 +110,8 @@ class TeamcityService < CiService
 
     branch = Gitlab::Git.ref_name(data[:ref])
 
-    self.class.post(
-      URI.join(teamcity_url, '/httpAuth/app/rest/buildQueue').to_s,
+    HTTParty.post(
+      build_url('httpAuth/app/rest/buildQueue'),
       body: "<build branchName=\"#{branch}\">"\
             "<buildType id=\"#{build_type}\"/>"\
             '</build>',
@@ -132,4 +119,18 @@ class TeamcityService < CiService
       basic_auth: auth
     )
   end
+
+  private
+
+  def build_url(path)
+    URI.join("#{teamcity_url}/", path).to_s
+  end
+
+  def get_path(path)
+    HTTParty.get(build_url(path), verify: false,
+                                  basic_auth: {
+                                    username: username,
+                                    password: password
+                                  })
+  end
 end
diff --git a/spec/models/project_services/bamboo_service_spec.rb b/spec/models/project_services/bamboo_service_spec.rb
index ec81f05fc7a..9ae461f8c2d 100644
--- a/spec/models/project_services/bamboo_service_spec.rb
+++ b/spec/models/project_services/bamboo_service_spec.rb
@@ -126,25 +126,25 @@ describe BambooService, models: true do
     it 'returns a specific URL when status is 500' do
       stub_request(status: 500)
 
-      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo')
+      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/foo')
     end
 
     it 'returns a specific URL when response has no results' do
       stub_request(body: %Q({"results":{"results":{"size":"0"}}}))
 
-      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo')
+      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/foo')
     end
 
     it 'returns a build URL when bamboo_url has no trailing slash' do
       stub_request(body: %Q({"results":{"results":{"result":{"planResultKey":{"key":"42"}}}}}))
 
-      expect(service(bamboo_url: 'http://gitlab.com').build_page('123', 'unused')).to eq('http://gitlab.com/browse/42')
+      expect(service(bamboo_url: 'http://gitlab.com/bamboo').build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/42')
     end
 
     it 'returns a build URL when bamboo_url has a trailing slash' do
       stub_request(body: %Q({"results":{"results":{"result":{"planResultKey":{"key":"42"}}}}}))
 
-      expect(service(bamboo_url: 'http://gitlab.com/').build_page('123', 'unused')).to eq('http://gitlab.com/browse/42')
+      expect(service(bamboo_url: 'http://gitlab.com/bamboo/').build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/42')
     end
   end
 
@@ -192,7 +192,7 @@ describe BambooService, models: true do
     end
   end
 
-  def service(bamboo_url: 'http://gitlab.com')
+  def service(bamboo_url: 'http://gitlab.com/bamboo')
     described_class.create(
       project: create(:empty_project),
       properties: {
@@ -205,7 +205,7 @@ describe BambooService, models: true do
   end
 
   def stub_request(status: 200, body: nil, build_state: 'success')
-    bamboo_full_url = 'http://mic:password@gitlab.com/rest/api/latest/result?label=123&os_authType=basic'
+    bamboo_full_url = 'http://mic:password@gitlab.com/bamboo/rest/api/latest/result?label=123&os_authType=basic'
     body ||= %Q({"results":{"results":{"result":{"buildState":"#{build_state}"}}}})
 
     WebMock.stub_request(:get, bamboo_full_url).to_return(
diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb
index 24a708ca849..474715d24c3 100644
--- a/spec/models/project_services/teamcity_service_spec.rb
+++ b/spec/models/project_services/teamcity_service_spec.rb
@@ -126,19 +126,19 @@ describe TeamcityService, models: true do
     it 'returns a specific URL when status is 500' do
       stub_request(status: 500)
 
-      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildTypeId=foo')
+      expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/teamcity/viewLog.html?buildTypeId=foo')
     end
 
     it 'returns a build URL when teamcity_url has no trailing slash' do
       stub_request(body: %Q({"build":{"id":"666"}}))
 
-      expect(service(teamcity_url: 'http://gitlab.com').build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildId=666&buildTypeId=foo')
+      expect(service(teamcity_url: 'http://gitlab.com/teamcity').build_page('123', 'unused')).to eq('http://gitlab.com/teamcity/viewLog.html?buildId=666&buildTypeId=foo')
     end
 
     it 'returns a build URL when teamcity_url has a trailing slash' do
       stub_request(body: %Q({"build":{"id":"666"}}))
 
-      expect(service(teamcity_url: 'http://gitlab.com/').build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildId=666&buildTypeId=foo')
+      expect(service(teamcity_url: 'http://gitlab.com/teamcity/').build_page('123', 'unused')).to eq('http://gitlab.com/teamcity/viewLog.html?buildId=666&buildTypeId=foo')
     end
   end
 
@@ -180,7 +180,7 @@ describe TeamcityService, models: true do
     end
   end
 
-  def service(teamcity_url: 'http://gitlab.com')
+  def service(teamcity_url: 'http://gitlab.com/teamcity')
     described_class.create(
       project: create(:empty_project),
       properties: {
@@ -193,7 +193,7 @@ describe TeamcityService, models: true do
   end
 
   def stub_request(status: 200, body: nil, build_status: 'success')
-    teamcity_full_url = 'http://mic:password@gitlab.com/httpAuth/app/rest/builds/branch:unspecified:any,number:123'
+    teamcity_full_url = 'http://mic:password@gitlab.com/teamcity/httpAuth/app/rest/builds/branch:unspecified:any,number:123'
     body ||= %Q({"build":{"status":"#{build_status}","id":"666"}})
 
     WebMock.stub_request(:get, teamcity_full_url).to_return(