Commit 0068ba8d authored by Rémy Coutable's avatar Rémy Coutable

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
parents a78cd2ec 2f7b2057
...@@ -39,6 +39,8 @@ v 8.9.0 (unreleased) ...@@ -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) - 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 - Fix issues filter when ordering by milestone
- Added artifacts:when to .gitlab-ci.yml - this requires GitLab Runner 1.3 - 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' - 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 - 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 - Add support for using Yubikeys (U2F) for two-factor authentication
......
class BambooService < CiService class BambooService < CiService
include HTTParty
prop_accessor :bamboo_url, :build_key, :username, :password prop_accessor :bamboo_url, :build_key, :username, :password
validates :bamboo_url, presence: true, url: true, if: :activated? validates :bamboo_url, presence: true, url: true, if: :activated?
...@@ -61,18 +59,7 @@ class BambooService < CiService ...@@ -61,18 +59,7 @@ class BambooService < CiService
end end
def build_info(sha) def build_info(sha)
url = URI.join(bamboo_url, "/rest/api/latest/result?label=#{sha}").to_s @response = get_path("rest/api/latest/result?label=#{sha}")
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
end end
def build_page(sha, ref) def build_page(sha, ref)
...@@ -80,11 +67,11 @@ class BambooService < CiService ...@@ -80,11 +67,11 @@ class BambooService < CiService
if @response.code != 200 || @response['results']['results']['size'] == '0' if @response.code != 200 || @response['results']['results']['size'] == '0'
# If actual build link can't be determined, send user to build summary page. # 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 else
# If actual build link is available, go to build result page. # If actual build link is available, go to build result page.
result_key = @response['results']['results']['result']['planResultKey']['key'] 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
end end
...@@ -112,8 +99,27 @@ class BambooService < CiService ...@@ -112,8 +99,27 @@ class BambooService < CiService
def execute(data) def execute(data)
return unless supported_events.include?(data[:object_kind]) return unless supported_events.include?(data[:object_kind])
# Bamboo requires a GET and does not take any data. get_path("updateAndBuild.action?buildKey=#{build_key}")
url = URI.join(bamboo_url, "/updateAndBuild.action?buildKey=#{build_key}").to_s end
self.class.get(url, verify: false)
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
end end
class TeamcityService < CiService class TeamcityService < CiService
include HTTParty
prop_accessor :teamcity_url, :build_type, :username, :password prop_accessor :teamcity_url, :build_type, :username, :password
validates :teamcity_url, presence: true, url: true, if: :activated? validates :teamcity_url, presence: true, url: true, if: :activated?
...@@ -64,15 +62,7 @@ class TeamcityService < CiService ...@@ -64,15 +62,7 @@ class TeamcityService < CiService
end end
def build_info(sha) def build_info(sha)
url = URI.join( @response = get_path("httpAuth/app/rest/builds/branch:unspecified:any,number:#{sha}")
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)
end end
def build_page(sha, ref) def build_page(sha, ref)
...@@ -81,14 +71,11 @@ class TeamcityService < CiService ...@@ -81,14 +71,11 @@ class TeamcityService < CiService
if @response.code != 200 if @response.code != 200
# If actual build link can't be determined, # If actual build link can't be determined,
# send user to build summary page. # 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 else
# If actual build link is available, go to build result page. # If actual build link is available, go to build result page.
built_id = @response['build']['id'] built_id = @response['build']['id']
URI.join( build_url("viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}")
teamcity_url,
"/viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}"
).to_s
end end
end end
...@@ -123,8 +110,8 @@ class TeamcityService < CiService ...@@ -123,8 +110,8 @@ class TeamcityService < CiService
branch = Gitlab::Git.ref_name(data[:ref]) branch = Gitlab::Git.ref_name(data[:ref])
self.class.post( HTTParty.post(
URI.join(teamcity_url, '/httpAuth/app/rest/buildQueue').to_s, build_url('httpAuth/app/rest/buildQueue'),
body: "<build branchName=\"#{branch}\">"\ body: "<build branchName=\"#{branch}\">"\
"<buildType id=\"#{build_type}\"/>"\ "<buildType id=\"#{build_type}\"/>"\
'</build>', '</build>',
...@@ -132,4 +119,18 @@ class TeamcityService < CiService ...@@ -132,4 +119,18 @@ class TeamcityService < CiService
basic_auth: auth basic_auth: auth
) )
end 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 end
...@@ -126,25 +126,25 @@ describe BambooService, models: true do ...@@ -126,25 +126,25 @@ describe BambooService, models: true do
it 'returns a specific URL when status is 500' do it 'returns a specific URL when status is 500' do
stub_request(status: 500) 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 end
it 'returns a specific URL when response has no results' do it 'returns a specific URL when response has no results' do
stub_request(body: %Q({"results":{"results":{"size":"0"}}})) 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 end
it 'returns a build URL when bamboo_url has no trailing slash' do it 'returns a build URL when bamboo_url has no trailing slash' do
stub_request(body: %Q({"results":{"results":{"result":{"planResultKey":{"key":"42"}}}}})) 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
it 'returns a build URL when bamboo_url has a trailing slash' do it 'returns a build URL when bamboo_url has a trailing slash' do
stub_request(body: %Q({"results":{"results":{"result":{"planResultKey":{"key":"42"}}}}})) 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
end end
...@@ -192,7 +192,7 @@ describe BambooService, models: true do ...@@ -192,7 +192,7 @@ describe BambooService, models: true do
end end
end end
def service(bamboo_url: 'http://gitlab.com') def service(bamboo_url: 'http://gitlab.com/bamboo')
described_class.create( described_class.create(
project: create(:empty_project), project: create(:empty_project),
properties: { properties: {
...@@ -205,7 +205,7 @@ describe BambooService, models: true do ...@@ -205,7 +205,7 @@ describe BambooService, models: true do
end end
def stub_request(status: 200, body: nil, build_state: 'success') 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}"}}}}) body ||= %Q({"results":{"results":{"result":{"buildState":"#{build_state}"}}}})
WebMock.stub_request(:get, bamboo_full_url).to_return( WebMock.stub_request(:get, bamboo_full_url).to_return(
......
...@@ -126,19 +126,19 @@ describe TeamcityService, models: true do ...@@ -126,19 +126,19 @@ describe TeamcityService, models: true do
it 'returns a specific URL when status is 500' do it 'returns a specific URL when status is 500' do
stub_request(status: 500) 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 end
it 'returns a build URL when teamcity_url has no trailing slash' do it 'returns a build URL when teamcity_url has no trailing slash' do
stub_request(body: %Q({"build":{"id":"666"}})) 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
it 'returns a build URL when teamcity_url has a trailing slash' do it 'returns a build URL when teamcity_url has a trailing slash' do
stub_request(body: %Q({"build":{"id":"666"}})) 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
end end
...@@ -180,7 +180,7 @@ describe TeamcityService, models: true do ...@@ -180,7 +180,7 @@ describe TeamcityService, models: true do
end end
end end
def service(teamcity_url: 'http://gitlab.com') def service(teamcity_url: 'http://gitlab.com/teamcity')
described_class.create( described_class.create(
project: create(:empty_project), project: create(:empty_project),
properties: { properties: {
...@@ -193,7 +193,7 @@ describe TeamcityService, models: true do ...@@ -193,7 +193,7 @@ describe TeamcityService, models: true do
end end
def stub_request(status: 200, body: nil, build_status: 'success') 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"}}) body ||= %Q({"build":{"status":"#{build_status}","id":"666"}})
WebMock.stub_request(:get, teamcity_full_url).to_return( WebMock.stub_request(:get, teamcity_full_url).to_return(
......
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