From ef340f6e777875e1bbb38752e64ba7bea3ab2f31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 21 Apr 2016 17:13:14 +0200 Subject: Ensure URL in all Service subclasses are valid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/helpers/issues_helper.rb | 48 +++++++---- app/models/project_services/buildkite_service.rb | 4 +- .../project_services/issue_tracker_service.rb | 2 +- app/models/project_services/jira_service.rb | 2 + app/models/project_services/slack_service.rb | 2 +- spec/helpers/issues_helper_spec.rb | 36 ++++++++ spec/models/external_wiki_service_spec.rb | 60 -------------- .../models/project_services/bamboo_service_spec.rb | 95 +++++++--------------- .../project_services/buildkite_service_spec.rb | 17 ++++ .../project_services/builds_email_service_spec.rb | 81 +++++++++--------- .../project_services/campfire_service_spec.rb | 42 ++++++++++ .../custom_issue_tracker_service_spec.rb | 49 +++++++++++ .../project_services/drone_ci_service_spec.rb | 13 +-- .../emails_on_push_service_spec.rb | 17 ++++ .../project_services/external_wiki_service_spec.rb | 65 +++++++++++++++ .../project_services/flowdock_service_spec.rb | 14 ++++ .../project_services/gemnasium_service_spec.rb | 16 ++++ .../gitlab_issue_tracker_service_spec.rb | 14 ++++ .../project_services/hipchat_service_spec.rb | 14 ++++ spec/models/project_services/irker_service_spec.rb | 14 ++-- spec/models/project_services/jira_service_spec.rb | 26 +++++- .../pivotaltracker_service_spec.rb | 42 ++++++++++ .../project_services/pushover_service_spec.rb | 20 +++-- .../project_services/redmine_service_spec.rb | 49 +++++++++++ spec/models/project_services/slack_service_spec.rb | 17 ++-- .../project_services/teamcity_service_spec.rb | 95 +++++++--------------- .../issue_tracker_service_shared_example.rb | 7 ++ 27 files changed, 579 insertions(+), 282 deletions(-) delete mode 100644 spec/models/external_wiki_service_spec.rb create mode 100644 spec/models/project_services/campfire_service_spec.rb create mode 100644 spec/models/project_services/custom_issue_tracker_service_spec.rb create mode 100644 spec/models/project_services/emails_on_push_service_spec.rb create mode 100644 spec/models/project_services/external_wiki_service_spec.rb create mode 100644 spec/models/project_services/pivotaltracker_service_spec.rb create mode 100644 spec/models/project_services/redmine_service_spec.rb create mode 100644 spec/support/issue_tracker_service_shared_example.rb diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index afe1e11a0da..198d39455d7 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -16,31 +16,49 @@ module IssuesHelper def url_for_project_issues(project = @project, options = {}) return '' if project.nil? - if options[:only_path] - project.issues_tracker.project_path - else - project.issues_tracker.project_url - end + url = + if options[:only_path] + project.issues_tracker.project_path + else + project.issues_tracker.project_url + end + + # Ensure we return a valid URL to prevent possible XSS. + URI.parse(url).to_s + rescue URI::InvalidURIError + '' end def url_for_new_issue(project = @project, options = {}) return '' if project.nil? - if options[:only_path] - project.issues_tracker.new_issue_path - else - project.issues_tracker.new_issue_url - end + url = + if options[:only_path] + project.issues_tracker.new_issue_path + else + project.issues_tracker.new_issue_url + end + + # Ensure we return a valid URL to prevent possible XSS. + URI.parse(url).to_s + rescue URI::InvalidURIError + '' end def url_for_issue(issue_iid, project = @project, options = {}) return '' if project.nil? - if options[:only_path] - project.issues_tracker.issue_path(issue_iid) - else - project.issues_tracker.issue_url(issue_iid) - end + url = + if options[:only_path] + project.issues_tracker.issue_path(issue_iid) + else + project.issues_tracker.issue_url(issue_iid) + end + + # Ensure we return a valid URL to prevent possible XSS. + URI.parse(url).to_s + rescue URI::InvalidURIError + '' end def bulk_update_milestone_options diff --git a/app/models/project_services/buildkite_service.rb b/app/models/project_services/buildkite_service.rb index 3efbfd2eec3..861cc974ec4 100644 --- a/app/models/project_services/buildkite_service.rb +++ b/app/models/project_services/buildkite_service.rb @@ -26,7 +26,7 @@ class BuildkiteService < CiService prop_accessor :project_url, :token, :enable_ssl_verification - validates :project_url, presence: true, if: :activated? + validates :project_url, presence: true, url: true, if: :activated? validates :token, presence: true, if: :activated? after_save :compose_service_hook, if: :activated? @@ -91,7 +91,7 @@ class BuildkiteService < CiService { type: 'text', name: 'project_url', placeholder: "#{ENDPOINT}/example/project" }, - + { type: 'checkbox', name: 'enable_ssl_verification', title: "Enable SSL verification" } diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb index 25045224ce5..c5501e06411 100644 --- a/app/models/project_services/issue_tracker_service.rb +++ b/app/models/project_services/issue_tracker_service.rb @@ -21,7 +21,7 @@ class IssueTrackerService < Service - validates :project_url, :issues_url, :new_issue_url, presence: true, if: :activated? + validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated? default_value_for :category, 'issue_tracker' diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index 1ed42c4f3e7..b4418ba9284 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -28,6 +28,8 @@ class JiraService < IssueTrackerService prop_accessor :username, :password, :api_url, :jira_issue_transition_id, :title, :description, :project_url, :issues_url, :new_issue_url + validates :api_url, presence: true, url: true, if: :activated? + before_validation :set_api_url, :set_jira_issue_transition_id before_update :reset_password diff --git a/app/models/project_services/slack_service.rb b/app/models/project_services/slack_service.rb index fd65027f084..7092b757549 100644 --- a/app/models/project_services/slack_service.rb +++ b/app/models/project_services/slack_service.rb @@ -22,7 +22,7 @@ class SlackService < Service prop_accessor :webhook, :username, :channel boolean_accessor :notify_only_broken_builds - validates :webhook, presence: true, if: :activated? + validates :webhook, presence: true, url: true, if: :activated? def initialize_properties if properties.nil? diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb index 543593cf389..bffe2c18b6f 100644 --- a/spec/helpers/issues_helper_spec.rb +++ b/spec/helpers/issues_helper_spec.rb @@ -30,6 +30,18 @@ describe IssuesHelper do expect(url_for_project_issues).to eq "" end + it 'returns an empty string if project_url is invalid' do + expect(project).to receive_message_chain('issues_tracker.project_url') { 'javascript:alert("foo");' } + + expect(url_for_project_issues(project)).to eq '' + end + + it 'returns an empty string if project_path is invalid' do + expect(project).to receive_message_chain('issues_tracker.project_path') { 'javascript:alert("foo");' } + + expect(url_for_project_issues(project, only_path: true)).to eq '' + end + describe "when external tracker was enabled and then config removed" do before do @project = ext_project @@ -68,6 +80,18 @@ describe IssuesHelper do expect(url_for_issue(issue.iid)).to eq "" end + it 'returns an empty string if issue_url is invalid' do + expect(project).to receive_message_chain('issues_tracker.issue_url') { 'javascript:alert("foo");' } + + expect(url_for_issue(issue.iid, project)).to eq '' + end + + it 'returns an empty string if issue_path is invalid' do + expect(project).to receive_message_chain('issues_tracker.issue_path') { 'javascript:alert("foo");' } + + expect(url_for_issue(issue.iid, project, only_path: true)).to eq '' + end + describe "when external tracker was enabled and then config removed" do before do @project = ext_project @@ -105,6 +129,18 @@ describe IssuesHelper do expect(url_for_new_issue).to eq "" end + it 'returns an empty string if issue_url is invalid' do + expect(project).to receive_message_chain('issues_tracker.new_issue_url') { 'javascript:alert("foo");' } + + expect(url_for_new_issue(project)).to eq '' + end + + it 'returns an empty string if issue_path is invalid' do + expect(project).to receive_message_chain('issues_tracker.new_issue_path') { 'javascript:alert("foo");' } + + expect(url_for_new_issue(project, only_path: true)).to eq '' + end + describe "when external tracker was enabled and then config removed" do before do @project = ext_project diff --git a/spec/models/external_wiki_service_spec.rb b/spec/models/external_wiki_service_spec.rb deleted file mode 100644 index d37978720bf..00000000000 --- a/spec/models/external_wiki_service_spec.rb +++ /dev/null @@ -1,60 +0,0 @@ -# == Schema Information -# -# Table name: services -# -# id :integer not null, primary key -# type :string(255) -# title :string(255) -# project_id :integer -# created_at :datetime -# updated_at :datetime -# active :boolean default(FALSE), not null -# properties :text -# template :boolean default(FALSE) -# push_events :boolean default(TRUE) -# issues_events :boolean default(TRUE) -# merge_requests_events :boolean default(TRUE) -# tag_push_events :boolean default(TRUE) -# note_events :boolean default(TRUE), not null -# build_events :boolean default(FALSE), not null -# - -require 'spec_helper' - -describe ExternalWikiService, models: true do - include ExternalWikiHelper - describe "Associations" do - it { should belong_to :project } - it { should have_one :service_hook } - end - - describe "Validations" do - context "active" do - before do - subject.active = true - end - - it { should validate_presence_of :external_wiki_url } - end - end - - describe 'External wiki' do - let(:project) { create(:project) } - - context 'when it is active' do - before do - properties = { 'external_wiki_url' => 'https://gitlab.com' } - @service = project.create_external_wiki_service(active: true, properties: properties) - end - - after do - @service.destroy! - end - - it 'should replace the wiki url' do - wiki_path = get_project_wiki_path(project) - expect(wiki_path).to match('https://gitlab.com') - end - end - end -end diff --git a/spec/models/project_services/bamboo_service_spec.rb b/spec/models/project_services/bamboo_service_spec.rb index 31b2c90122d..e771f35811e 100644 --- a/spec/models/project_services/bamboo_service_spec.rb +++ b/spec/models/project_services/bamboo_service_spec.rb @@ -27,86 +27,51 @@ describe BambooService, models: true do end describe 'Validations' do - describe '#bamboo_url' do - it 'does not validate the presence of bamboo_url if service is not active' do - bamboo_service = service - bamboo_service.active = false - - expect(bamboo_service).not_to validate_presence_of(:bamboo_url) - end - - it 'validates the presence of bamboo_url if service is active' do - bamboo_service = service - bamboo_service.active = true - - expect(bamboo_service).to validate_presence_of(:bamboo_url) - end - end + subject { service } - describe '#build_key' do - it 'does not validate the presence of build_key if service is not active' do - bamboo_service = service - bamboo_service.active = false + context 'when service is active' do + before { subject.active = true } - expect(bamboo_service).not_to validate_presence_of(:build_key) - end + it { is_expected.to validate_presence_of(:build_key) } + it { is_expected.to validate_presence_of(:bamboo_url) } + it_behaves_like 'issue tracker service URL attribute', :bamboo_url - it 'validates the presence of build_key if service is active' do - bamboo_service = service - bamboo_service.active = true + describe '#username' do + it 'does not validate the presence of username if password is nil' do + subject.password = nil - expect(bamboo_service).to validate_presence_of(:build_key) - end - end + expect(subject).not_to validate_presence_of(:username) + end - describe '#username' do - it 'does not validate the presence of username if service is not active' do - bamboo_service = service - bamboo_service.active = false + it 'validates the presence of username if password is present' do + subject.password = 'secret' - expect(bamboo_service).not_to validate_presence_of(:username) + expect(subject).to validate_presence_of(:username) + end end - it 'does not validate the presence of username if username is nil' do - bamboo_service = service - bamboo_service.active = true - bamboo_service.password = nil + describe '#password' do + it 'does not validate the presence of password if username is nil' do + subject.username = nil - expect(bamboo_service).not_to validate_presence_of(:username) - end + expect(subject).not_to validate_presence_of(:password) + end - it 'validates the presence of username if service is active and username is present' do - bamboo_service = service - bamboo_service.active = true - bamboo_service.password = 'secret' + it 'validates the presence of password if username is present' do + subject.username = 'john' - expect(bamboo_service).to validate_presence_of(:username) + expect(subject).to validate_presence_of(:password) + end end end - describe '#password' do - it 'does not validate the presence of password if service is not active' do - bamboo_service = service - bamboo_service.active = false - - expect(bamboo_service).not_to validate_presence_of(:password) - end - - it 'does not validate the presence of password if username is nil' do - bamboo_service = service - bamboo_service.active = true - bamboo_service.username = nil - - expect(bamboo_service).not_to validate_presence_of(:password) - end - - it 'validates the presence of password if service is active and username is present' do - bamboo_service = service - bamboo_service.active = true - bamboo_service.username = 'john' + context 'when service is inactive' do + before { subject.active = false } - expect(bamboo_service).to validate_presence_of(:password) - end + it { is_expected.not_to validate_presence_of(:build_key) } + it { is_expected.not_to validate_presence_of(:bamboo_url) } + it { is_expected.not_to validate_presence_of(:username) } + it { is_expected.not_to validate_presence_of(:password) } end end diff --git a/spec/models/project_services/buildkite_service_spec.rb b/spec/models/project_services/buildkite_service_spec.rb index 88cd624877a..60364df2015 100644 --- a/spec/models/project_services/buildkite_service_spec.rb +++ b/spec/models/project_services/buildkite_service_spec.rb @@ -26,6 +26,23 @@ describe BuildkiteService, models: true do it { is_expected.to have_one :service_hook } end + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:project_url) } + it { is_expected.to validate_presence_of(:token) } + it_behaves_like 'issue tracker service URL attribute', :project_url + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:project_url) } + it { is_expected.not_to validate_presence_of(:token) } + end + end + describe 'commits methods' do before do @project = Project.new diff --git a/spec/models/project_services/builds_email_service_spec.rb b/spec/models/project_services/builds_email_service_spec.rb index 7c23c2efccd..236df8f047d 100644 --- a/spec/models/project_services/builds_email_service_spec.rb +++ b/spec/models/project_services/builds_email_service_spec.rb @@ -1,76 +1,71 @@ require 'spec_helper' describe BuildsEmailService do - let(:build) { create(:ci_build) } - let(:data) { Gitlab::BuildDataBuilder.build(build) } - let!(:project) { create(:project, :public, ci_id: 1) } - let(:service) { described_class.new(project: project, active: true) } + let(:data) { Gitlab::BuildDataBuilder.build(create(:ci_build)) } + + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:recipients) } + + context 'when pusher is added' do + before { subject.add_pusher = true } + + it { is_expected.not_to validate_presence_of(:recipients) } + end + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:recipients) } + end + end describe '#execute' do it 'sends email' do - service.recipients = 'test@gitlab.com' + subject.recipients = 'test@gitlab.com' data[:build_status] = 'failed' + expect(BuildEmailWorker).to receive(:perform_async) - service.execute(data) + + subject.execute(data) end it 'does not send email with succeeded build and notify_only_broken_builds on' do - expect(service).to receive(:notify_only_broken_builds).and_return(true) + expect(subject).to receive(:notify_only_broken_builds).and_return(true) data[:build_status] = 'success' + expect(BuildEmailWorker).not_to receive(:perform_async) - service.execute(data) + + subject.execute(data) end it 'does not send email with failed build and build_allow_failure is true' do data[:build_status] = 'failed' data[:build_allow_failure] = true + expect(BuildEmailWorker).not_to receive(:perform_async) - service.execute(data) + + subject.execute(data) end it 'does not send email with unknown build status' do data[:build_status] = 'foo' - expect(BuildEmailWorker).not_to receive(:perform_async) - service.execute(data) - end - it 'does not send email when recipients list is empty' do - service.recipients = ' ,, ' - data[:build_status] = 'failed' expect(BuildEmailWorker).not_to receive(:perform_async) - service.execute(data) - end - end - - describe 'validations' do - - context 'when pusher is not added' do - before { service.add_pusher = false } - - it 'does not allow empty recipient input' do - service.recipients = '' - expect(service.valid?).to be false - end - - it 'does allow non-empty recipient input' do - service.recipients = 'test@example.com' - expect(service.valid?).to be true - end + subject.execute(data) end - context 'when pusher is added' do - before { service.add_pusher = true } + it 'does not send email when recipients list is empty' do + subject.recipients = ' ,, ' + data[:build_status] = 'failed' - it 'does allow empty recipient input' do - service.recipients = '' - expect(service.valid?).to be true - end + expect(BuildEmailWorker).not_to receive(:perform_async) - it 'does allow non-empty recipient input' do - service.recipients = 'test@example.com' - expect(service.valid?).to be true - end + subject.execute(data) end end end diff --git a/spec/models/project_services/campfire_service_spec.rb b/spec/models/project_services/campfire_service_spec.rb new file mode 100644 index 00000000000..3e6da42803b --- /dev/null +++ b/spec/models/project_services/campfire_service_spec.rb @@ -0,0 +1,42 @@ +# == Schema Information +# +# Table name: services +# +# id :integer not null, primary key +# type :string(255) +# title :string(255) +# project_id :integer +# created_at :datetime +# updated_at :datetime +# active :boolean default(FALSE), not null +# properties :text +# template :boolean default(FALSE) +# push_events :boolean default(TRUE) +# issues_events :boolean default(TRUE) +# merge_requests_events :boolean default(TRUE) +# tag_push_events :boolean default(TRUE) +# note_events :boolean default(TRUE), not null +# + +require 'spec_helper' + +describe CampfireService, models: true do + describe 'Associations' do + it { is_expected.to belong_to :project } + it { is_expected.to have_one :service_hook } + end + + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:token) } + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:token) } + end + end +end diff --git a/spec/models/project_services/custom_issue_tracker_service_spec.rb b/spec/models/project_services/custom_issue_tracker_service_spec.rb new file mode 100644 index 00000000000..ff976f8ec59 --- /dev/null +++ b/spec/models/project_services/custom_issue_tracker_service_spec.rb @@ -0,0 +1,49 @@ +# == Schema Information +# +# Table name: services +# +# id :integer not null, primary key +# type :string(255) +# title :string(255) +# project_id :integer +# created_at :datetime +# updated_at :datetime +# active :boolean default(FALSE), not null +# properties :text +# template :boolean default(FALSE) +# push_events :boolean default(TRUE) +# issues_events :boolean default(TRUE) +# merge_requests_events :boolean default(TRUE) +# tag_push_events :boolean default(TRUE) +# note_events :boolean default(TRUE), not null +# + +require 'spec_helper' + +describe CustomIssueTrackerService, models: true do + describe 'Associations' do + it { is_expected.to belong_to :project } + it { is_expected.to have_one :service_hook } + end + + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:project_url) } + it { is_expected.to validate_presence_of(:issues_url) } + it { is_expected.to validate_presence_of(:new_issue_url) } + it_behaves_like 'issue tracker service URL attribute', :project_url + it_behaves_like 'issue tracker service URL attribute', :issues_url + it_behaves_like 'issue tracker service URL attribute', :new_issue_url + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:project_url) } + it { is_expected.not_to validate_presence_of(:issues_url) } + it { is_expected.not_to validate_presence_of(:new_issue_url) } + end + end +end diff --git a/spec/models/project_services/drone_ci_service_spec.rb b/spec/models/project_services/drone_ci_service_spec.rb index a2cf68a9e38..3a8e67438fc 100644 --- a/spec/models/project_services/drone_ci_service_spec.rb +++ b/spec/models/project_services/drone_ci_service_spec.rb @@ -28,25 +28,18 @@ describe DroneCiService, models: true do describe 'validations' do context 'active' do - before { allow(subject).to receive(:activated?).and_return(true) } + before { subject.active = true } it { is_expected.to validate_presence_of(:token) } it { is_expected.to validate_presence_of(:drone_url) } - it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) } - it { is_expected.to allow_value('http://ci.example.com').for(:drone_url) } - it { is_expected.not_to allow_value('this is not url').for(:drone_url) } - it { is_expected.not_to allow_value('http//noturl').for(:drone_url) } - it { is_expected.not_to allow_value('ftp://ci.example.com').for(:drone_url) } + it_behaves_like 'issue tracker service URL attribute', :drone_url end context 'inactive' do - before { allow(subject).to receive(:activated?).and_return(false) } + before { subject.active = false } it { is_expected.not_to validate_presence_of(:token) } it { is_expected.not_to validate_presence_of(:drone_url) } - it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) } - it { is_expected.to allow_value('http://drone.example.com').for(:drone_url) } - it { is_expected.to allow_value('ftp://drone.example.com').for(:drone_url) } end end diff --git a/spec/models/project_services/emails_on_push_service_spec.rb b/spec/models/project_services/emails_on_push_service_spec.rb new file mode 100644 index 00000000000..e6f78898c82 --- /dev/null +++ b/spec/models/project_services/emails_on_push_service_spec.rb @@ -0,0 +1,17 @@ +require 'spec_helper' + +describe EmailsOnPushService do + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:recipients) } + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:recipients) } + end + end +end diff --git a/spec/models/project_services/external_wiki_service_spec.rb b/spec/models/project_services/external_wiki_service_spec.rb new file mode 100644 index 00000000000..5fe5ea7d2df --- /dev/null +++ b/spec/models/project_services/external_wiki_service_spec.rb @@ -0,0 +1,65 @@ +# == Schema Information +# +# Table name: services +# +# id :integer not null, primary key +# type :string(255) +# title :string(255) +# project_id :integer +# created_at :datetime +# updated_at :datetime +# active :boolean default(FALSE), not null +# properties :text +# template :boolean default(FALSE) +# push_events :boolean default(TRUE) +# issues_events :boolean default(TRUE) +# merge_requests_events :boolean default(TRUE) +# tag_push_events :boolean default(TRUE) +# note_events :boolean default(TRUE), not null +# build_events :boolean default(FALSE), not null +# + +require 'spec_helper' + +describe ExternalWikiService, models: true do + include ExternalWikiHelper + describe "Associations" do + it { should belong_to :project } + it { should have_one :service_hook } + end + + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:external_wiki_url) } + it_behaves_like 'issue tracker service URL attribute', :external_wiki_url + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:external_wiki_url) } + end + end + + describe 'External wiki' do + let(:project) { create(:project) } + + context 'when it is active' do + before do + properties = { 'external_wiki_url' => 'https://gitlab.com' } + @service = project.create_external_wiki_service(active: true, properties: properties) + end + + after do + @service.destroy! + end + + it 'should replace the wiki url' do + wiki_path = get_project_wiki_path(project) + expect(wiki_path).to match('https://gitlab.com') + end + end + end +end diff --git a/spec/models/project_services/flowdock_service_spec.rb b/spec/models/project_services/flowdock_service_spec.rb index ff7fbcaa004..b7e627e6518 100644 --- a/spec/models/project_services/flowdock_service_spec.rb +++ b/spec/models/project_services/flowdock_service_spec.rb @@ -26,6 +26,20 @@ describe FlowdockService, models: true do it { is_expected.to have_one :service_hook } end + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:token) } + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:token) } + end + end + describe "Execute" do let(:user) { create(:user) } let(:project) { create(:project) } diff --git a/spec/models/project_services/gemnasium_service_spec.rb b/spec/models/project_services/gemnasium_service_spec.rb index ecb3ccb1673..a08f1ac229f 100644 --- a/spec/models/project_services/gemnasium_service_spec.rb +++ b/spec/models/project_services/gemnasium_service_spec.rb @@ -26,6 +26,22 @@ describe GemnasiumService, models: true do it { is_expected.to have_one :service_hook } end + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:token) } + it { is_expected.to validate_presence_of(:api_key) } + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:token) } + it { is_expected.not_to validate_presence_of(:api_key) } + end + end + describe "Execute" do let(:user) { create(:user) } let(:project) { create(:project) } diff --git a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb index 3518dbd1728..7a1f106d6e3 100644 --- a/spec/models/project_services/gitlab_issue_tracker_service_spec.rb +++ b/spec/models/project_services/gitlab_issue_tracker_service_spec.rb @@ -26,6 +26,20 @@ describe GitlabIssueTrackerService, models: true do it { is_expected.to have_one :service_hook } end + describe 'Validations' do + context 'when service is active' do + subject { described_class.new(project: create(:project), active: true) } + + it { is_expected.to validate_presence_of(:issues_url) } + it_behaves_like 'issue tracker service URL attribute', :issues_url + end + + context 'when service is inactive' do + subject { described_class.new(project: create(:project), active: false) } + + it { is_expected.not_to validate_presence_of(:issues_url) } + end + end describe 'project and issue urls' do let(:project) { create(:project) } diff --git a/spec/models/project_services/hipchat_service_spec.rb b/spec/models/project_services/hipchat_service_spec.rb index d878162a220..6fb5cad5011 100644 --- a/spec/models/project_services/hipchat_service_spec.rb +++ b/spec/models/project_services/hipchat_service_spec.rb @@ -26,6 +26,20 @@ describe HipchatService, models: true do it { is_expected.to have_one :service_hook } end + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:token) } + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:token) } + end + end + describe "Execute" do let(:hipchat) { HipchatService.new } let(:user) { create(:user, username: 'username') } diff --git a/spec/models/project_services/irker_service_spec.rb b/spec/models/project_services/irker_service_spec.rb index b783b1a576e..4ee022a5171 100644 --- a/spec/models/project_services/irker_service_spec.rb +++ b/spec/models/project_services/irker_service_spec.rb @@ -29,14 +29,16 @@ describe IrkerService, models: true do end describe 'Validations' do - before do - subject.active = true - subject.properties['recipients'] = _recipients + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:recipients) } end - context 'active' do - let(:_recipients) { nil } - it { should validate_presence_of :recipients } + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:recipients) } end end diff --git a/spec/models/project_services/jira_service_spec.rb b/spec/models/project_services/jira_service_spec.rb index 2f8193170ae..5309cfb99ff 100644 --- a/spec/models/project_services/jira_service_spec.rb +++ b/spec/models/project_services/jira_service_spec.rb @@ -26,6 +26,30 @@ describe JiraService, models: true do it { is_expected.to have_one :service_hook } end + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:api_url) } + it { is_expected.to validate_presence_of(:project_url) } + it { is_expected.to validate_presence_of(:issues_url) } + it { is_expected.to validate_presence_of(:new_issue_url) } + it_behaves_like 'issue tracker service URL attribute', :api_url + it_behaves_like 'issue tracker service URL attribute', :project_url + it_behaves_like 'issue tracker service URL attribute', :issues_url + it_behaves_like 'issue tracker service URL attribute', :new_issue_url + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:api_url) } + it { is_expected.not_to validate_presence_of(:project_url) } + it { is_expected.not_to validate_presence_of(:issues_url) } + it { is_expected.not_to validate_presence_of(:new_issue_url) } + end + end + describe "Execute" do let(:user) { create(:user) } let(:project) { create(:project) } @@ -72,7 +96,7 @@ describe JiraService, models: true do context "when a password was previously set" do before do - @jira_service = JiraService.create( + @jira_service = JiraService.create!( project: create(:project), properties: { api_url: 'http://jira.example.com/rest/api/2', diff --git a/spec/models/project_services/pivotaltracker_service_spec.rb b/spec/models/project_services/pivotaltracker_service_spec.rb new file mode 100644 index 00000000000..f37edd4d970 --- /dev/null +++ b/spec/models/project_services/pivotaltracker_service_spec.rb @@ -0,0 +1,42 @@ +# == Schema Information +# +# Table name: services +# +# id :integer not null, primary key +# type :string(255) +# title :string(255) +# project_id :integer +# created_at :datetime +# updated_at :datetime +# active :boolean default(FALSE), not null +# properties :text +# template :boolean default(FALSE) +# push_events :boolean default(TRUE) +# issues_events :boolean default(TRUE) +# merge_requests_events :boolean default(TRUE) +# tag_push_events :boolean default(TRUE) +# note_events :boolean default(TRUE), not null +# + +require 'spec_helper' + +describe PivotaltrackerService, models: true do + describe 'Associations' do + it { is_expected.to belong_to :project } + it { is_expected.to have_one :service_hook } + end + + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:token) } + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:token) } + end + end +end diff --git a/spec/models/project_services/pushover_service_spec.rb b/spec/models/project_services/pushover_service_spec.rb index 96039f9491b..555d9757b47 100644 --- a/spec/models/project_services/pushover_service_spec.rb +++ b/spec/models/project_services/pushover_service_spec.rb @@ -27,14 +27,20 @@ describe PushoverService, models: true do end describe 'Validations' do - context 'active' do - before do - subject.active = true - end + context 'when service is active' do + before { subject.active = true } - it { is_expected.to validate_presence_of :api_key } - it { is_expected.to validate_presence_of :user_key } - it { is_expected.to validate_presence_of :priority } + it { is_expected.to validate_presence_of(:api_key) } + it { is_expected.to validate_presence_of(:user_key) } + it { is_expected.to validate_presence_of(:priority) } + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:api_key) } + it { is_expected.not_to validate_presence_of(:user_key) } + it { is_expected.not_to validate_presence_of(:priority) } end end diff --git a/spec/models/project_services/redmine_service_spec.rb b/spec/models/project_services/redmine_service_spec.rb new file mode 100644 index 00000000000..7d14f6e8280 --- /dev/null +++ b/spec/models/project_services/redmine_service_spec.rb @@ -0,0 +1,49 @@ +# == Schema Information +# +# Table name: services +# +# id :integer not null, primary key +# type :string(255) +# title :string(255) +# project_id :integer +# created_at :datetime +# updated_at :datetime +# active :boolean default(FALSE), not null +# properties :text +# template :boolean default(FALSE) +# push_events :boolean default(TRUE) +# issues_events :boolean default(TRUE) +# merge_requests_events :boolean default(TRUE) +# tag_push_events :boolean default(TRUE) +# note_events :boolean default(TRUE), not null +# + +require 'spec_helper' + +describe RedmineService, models: true do + describe 'Associations' do + it { is_expected.to belong_to :project } + it { is_expected.to have_one :service_hook } + end + + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } + + it { is_expected.to validate_presence_of(:project_url) } + it { is_expected.to validate_presence_of(:issues_url) } + it { is_expected.to validate_presence_of(:new_issue_url) } + it_behaves_like 'issue tracker service URL attribute', :project_url + it_behaves_like 'issue tracker service URL attribute', :issues_url + it_behaves_like 'issue tracker service URL attribute', :new_issue_url + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:project_url) } + it { is_expected.not_to validate_presence_of(:issues_url) } + it { is_expected.not_to validate_presence_of(:new_issue_url) } + end + end +end diff --git a/spec/models/project_services/slack_service_spec.rb b/spec/models/project_services/slack_service_spec.rb index 478d59be08b..a97b7560137 100644 --- a/spec/models/project_services/slack_service_spec.rb +++ b/spec/models/project_services/slack_service_spec.rb @@ -26,13 +26,18 @@ describe SlackService, models: true do it { is_expected.to have_one :service_hook } end - describe "Validations" do - context "active" do - before do - subject.active = true - end + describe 'Validations' do + context 'when service is active' do + before { subject.active = true } - it { is_expected.to validate_presence_of :webhook } + it { is_expected.to validate_presence_of(:webhook) } + it_behaves_like 'issue tracker service URL attribute', :webhook + end + + context 'when service is inactive' do + before { subject.active = false } + + it { is_expected.not_to validate_presence_of(:webhook) } end end diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb index bc7423cee69..ad24b895170 100644 --- a/spec/models/project_services/teamcity_service_spec.rb +++ b/spec/models/project_services/teamcity_service_spec.rb @@ -27,86 +27,51 @@ describe TeamcityService, models: true do end describe 'Validations' do - describe '#teamcity_url' do - it 'does not validate the presence of teamcity_url if service is not active' do - teamcity_service = service - teamcity_service.active = false - - expect(teamcity_service).not_to validate_presence_of(:teamcity_url) - end - - it 'validates the presence of teamcity_url if service is active' do - teamcity_service = service - teamcity_service.active = true - - expect(teamcity_service).to validate_presence_of(:teamcity_url) - end - end + subject { service } - describe '#build_type' do - it 'does not validate the presence of build_type if service is not active' do - teamcity_service = service - teamcity_service.active = false + context 'when service is active' do + before { subject.active = true } - expect(teamcity_service).not_to validate_presence_of(:build_type) - end + it { is_expected.to validate_presence_of(:build_type) } + it { is_expected.to validate_presence_of(:teamcity_url) } + it_behaves_like 'issue tracker service URL attribute', :teamcity_url - it 'validates the presence of build_type if service is active' do - teamcity_service = service - teamcity_service.active = true + describe '#username' do + it 'does not validate the presence of username if password is nil' do + subject.password = nil - expect(teamcity_service).to validate_presence_of(:build_type) - end - end + expect(subject).not_to validate_presence_of(:username) + end - describe '#username' do - it 'does not validate the presence of username if service is not active' do - teamcity_service = service - teamcity_service.active = false + it 'validates the presence of username if password is present' do + subject.password = 'secret' - expect(teamcity_service).not_to validate_presence_of(:username) + expect(subject).to validate_presence_of(:username) + end end - it 'does not validate the presence of username if username is nil' do - teamcity_service = service - teamcity_service.active = true - teamcity_service.password = nil + describe '#password' do + it 'does not validate the presence of password if username is nil' do + subject.username = nil - expect(teamcity_service).not_to validate_presence_of(:username) - end + expect(subject).not_to validate_presence_of(:password) + end - it 'validates the presence of username if service is active and username is present' do - teamcity_service = service - teamcity_service.active = true - teamcity_service.password = 'secret' + it 'validates the presence of password if username is present' do + subject.username = 'john' - expect(teamcity_service).to validate_presence_of(:username) + expect(subject).to validate_presence_of(:password) + end end end - describe '#password' do - it 'does not validate the presence of password if service is not active' do - teamcity_service = service - teamcity_service.active = false - - expect(teamcity_service).not_to validate_presence_of(:password) - end - - it 'does not validate the presence of password if username is nil' do - teamcity_service = service - teamcity_service.active = true - teamcity_service.username = nil - - expect(teamcity_service).not_to validate_presence_of(:password) - end - - it 'validates the presence of password if service is active and username is present' do - teamcity_service = service - teamcity_service.active = true - teamcity_service.username = 'john' + context 'when service is inactive' do + before { subject.active = false } - expect(teamcity_service).to validate_presence_of(:password) - end + it { is_expected.not_to validate_presence_of(:build_type) } + it { is_expected.not_to validate_presence_of(:teamcity_url) } + it { is_expected.not_to validate_presence_of(:username) } + it { is_expected.not_to validate_presence_of(:password) } end end diff --git a/spec/support/issue_tracker_service_shared_example.rb b/spec/support/issue_tracker_service_shared_example.rb new file mode 100644 index 00000000000..b6d7436c360 --- /dev/null +++ b/spec/support/issue_tracker_service_shared_example.rb @@ -0,0 +1,7 @@ +RSpec.shared_examples 'issue tracker service URL attribute' do |url_attr| + it { is_expected.to allow_value('https://example.com').for(url_attr) } + + it { is_expected.not_to allow_value('example.com').for(url_attr) } + it { is_expected.not_to allow_value('ftp://example.com').for(url_attr) } + it { is_expected.not_to allow_value('herp-and-derp').for(url_attr) } +end -- cgit v1.2.1