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: [PATCH] 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 +++++--- .../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 ++++++ .../project_services/bamboo_service_spec.rb | 109 ++++++------------ .../buildkite_service_spec.rb | 17 +++ .../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 +++ .../external_wiki_service_spec.rb | 17 ++- .../project_services/flowdock_service_spec.rb | 14 +++ .../gemnasium_service_spec.rb | 16 +++ .../gitlab_issue_tracker_service_spec.rb | 14 +++ .../project_services/hipchat_service_spec.rb | 14 +++ .../project_services/irker_service_spec.rb | 14 ++- .../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 ++++++++ .../project_services/slack_service_spec.rb | 17 ++- .../project_services/teamcity_service_spec.rb | 109 ++++++------------ .../issue_tracker_service_shared_example.rb | 7 ++ 26 files changed, 539 insertions(+), 242 deletions(-) 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 rename spec/models/{ => project_services}/external_wiki_service_spec.rb (78%) 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 afe1e11a0d..198d39455d 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 3efbfd2eec..861cc974ec 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 25045224ce..c5501e0641 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 1ed42c4f3e..b4418ba928 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 fd65027f08..7092b75754 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 543593cf38..bffe2c18b6 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/project_services/bamboo_service_spec.rb b/spec/models/project_services/bamboo_service_spec.rb index 31b2c90122..e771f35811 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 + subject { service } - expect(bamboo_service).not_to validate_presence_of(:bamboo_url) + context 'when service is active' do + before { subject.active = true } + + 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 + + describe '#username' do + it 'does not validate the presence of username if password is nil' do + subject.password = nil + + expect(subject).not_to validate_presence_of(:username) + end + + it 'validates the presence of username if password is present' do + subject.password = 'secret' + + expect(subject).to validate_presence_of(:username) + end end - it 'validates the presence of bamboo_url if service is active' do - bamboo_service = service - bamboo_service.active = true + describe '#password' do + it 'does not validate the presence of password if username is nil' do + subject.username = nil - expect(bamboo_service).to validate_presence_of(:bamboo_url) + expect(subject).not_to validate_presence_of(:password) + end + + it 'validates the presence of password if username is present' do + subject.username = 'john' + + expect(subject).to validate_presence_of(:password) + end end end - 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 inactive' do + before { subject.active = false } - expect(bamboo_service).not_to validate_presence_of(:build_key) - end - - it 'validates the presence of build_key if service is active' do - bamboo_service = service - bamboo_service.active = true - - expect(bamboo_service).to validate_presence_of(:build_key) - end - 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 - - expect(bamboo_service).not_to validate_presence_of(:username) - 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 - - expect(bamboo_service).not_to validate_presence_of(:username) - 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' - - expect(bamboo_service).to validate_presence_of(:username) - 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' - - 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 88cd624877..60364df201 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 7c23c2efcc..236df8f047 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) + + subject.execute(data) end it 'does not send email when recipients list is empty' do - service.recipients = ' ,, ' + subject.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 - - end - - context 'when pusher is added' do - before { service.add_pusher = true } - - it 'does allow empty recipient input' do - service.recipients = '' - expect(service.valid?).to be true - 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 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 0000000000..3e6da42803 --- /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 0000000000..ff976f8ec5 --- /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 a2cf68a9e3..3a8e67438f 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 0000000000..e6f78898c8 --- /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/external_wiki_service_spec.rb b/spec/models/project_services/external_wiki_service_spec.rb similarity index 78% rename from spec/models/external_wiki_service_spec.rb rename to spec/models/project_services/external_wiki_service_spec.rb index d37978720b..5fe5ea7d2d 100644 --- a/spec/models/external_wiki_service_spec.rb +++ b/spec/models/project_services/external_wiki_service_spec.rb @@ -28,13 +28,18 @@ describe ExternalWikiService, models: true do it { should 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 { should validate_presence_of :external_wiki_url } + 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 diff --git a/spec/models/project_services/flowdock_service_spec.rb b/spec/models/project_services/flowdock_service_spec.rb index ff7fbcaa00..b7e627e651 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 ecb3ccb167..a08f1ac229 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 3518dbd172..7a1f106d6e 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 d878162a22..6fb5cad501 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 b783b1a576..4ee022a517 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 2f8193170a..5309cfb99f 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 0000000000..f37edd4d97 --- /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 96039f9491..555d9757b4 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 0000000000..7d14f6e828 --- /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 478d59be08..a97b756013 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 bc7423cee6..ad24b89517 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 + subject { service } - expect(teamcity_service).not_to validate_presence_of(:teamcity_url) + context 'when service is active' do + before { subject.active = true } + + 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 + + describe '#username' do + it 'does not validate the presence of username if password is nil' do + subject.password = nil + + expect(subject).not_to validate_presence_of(:username) + end + + it 'validates the presence of username if password is present' do + subject.password = 'secret' + + expect(subject).to validate_presence_of(:username) + end end - it 'validates the presence of teamcity_url if service is active' do - teamcity_service = service - teamcity_service.active = true + describe '#password' do + it 'does not validate the presence of password if username is nil' do + subject.username = nil - expect(teamcity_service).to validate_presence_of(:teamcity_url) + expect(subject).not_to validate_presence_of(:password) + end + + it 'validates the presence of password if username is present' do + subject.username = 'john' + + expect(subject).to validate_presence_of(:password) + end end end - 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 inactive' do + before { subject.active = false } - expect(teamcity_service).not_to validate_presence_of(:build_type) - end - - it 'validates the presence of build_type if service is active' do - teamcity_service = service - teamcity_service.active = true - - expect(teamcity_service).to validate_presence_of(:build_type) - end - 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 - - expect(teamcity_service).not_to validate_presence_of(:username) - 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 - - expect(teamcity_service).not_to validate_presence_of(:username) - 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' - - expect(teamcity_service).to validate_presence_of(:username) - 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' - - 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 0000000000..b6d7436c36 --- /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